Skip to content

ROX-13034: fix gRPC scanner #3469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
SimonBaeumer merged 6 commits into from
Nov 7, 2022
Merged

ROX-13034: fix gRPC scanner #3469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
64a9cc9
X-Smart-Branch-Parent: master
SimonBaeumer Nov 7, 2022
d1e26ed
ROX-13034 - Fix gRPC scanner image integration endpoint
SimonBaeumer Oct 18, 2022
ae4918b
trigger-ci
SimonBaeumer Oct 18, 2022
f496113
trigger-ci
SimonBaeumer Oct 25, 2022
66a486a
changelog
SimonBaeumer Oct 25, 2022
a188490
Move default scanner endpoint to different package to avoid import cycle
SimonBaeumer Nov 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ Please avoid adding duplicate information across this changelog and JIRA/doc inp
- ROX-11592: Support to Get / Update / Mutate / Remove of groups via the `props` field and without the `props.id` field
being set in the `/v1/groups` endpoint have been removed.
- The unused "ComplianceRunSchedule" resource has been removed.
- ROX-13034: Central reaches out to scanner `scanner.<namespace>.svc` now to respect OpenShift's `NO_PROXY` configuration.
Copy link
Copy Markdown
Contributor

@dhaus67 dhaus67 Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make sure this gets enough visibility for customers, as this one has impact on existing proxy settings and requires updates for them (e.g. imagine people having scanner.<namespace> in their NO_PROXY).

Copy link
Copy Markdown
Contributor

@RTann RTann Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Just be sure to add the training and docs labels to the Jira

Copy link
Copy Markdown
Contributor Author

@SimonBaeumer SimonBaeumer Oct 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question.
I assume the changelog is published to customers and is sufficient.
+1 on the training label that support is aware.

What exactly does the docs label do?
I think mentioning it in the docs is overkill, if it adds to changelog sure, this must be there.

Copy link
Copy Markdown
Contributor Author

@SimonBaeumer SimonBaeumer Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaurav-nelson can you share your opinion?

Copy link
Copy Markdown
Contributor

@gaurav-nelson gaurav-nelson Nov 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can mention it in the release notes and also add a note in the proxy configuration section.

@SimonBaeumer related to @dhaus67's comment, if someone have scanner.<namespace> in their NO_PROXY settings:

  • What issues they will face if they don't change anything?
  • How can they fix those issues? I guess its by updating their NO_PROXY settings, so how to update those settings?
  • Can they run a command to fix it? or do they have to manually change things?

Copy link
Copy Markdown
Contributor Author

@SimonBaeumer SimonBaeumer Nov 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaurav-nelson

Copy link
Copy Markdown
Contributor Author

@SimonBaeumer SimonBaeumer Nov 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaurav-nelson Are there any action items I should follow up with?


### Deprecated Features

Expand Down
7 changes: 1 addition & 6 deletions central/imageintegration/store/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,10 @@ import (
"fmt"

"github.com/stackrox/rox/generated/storage"
"github.com/stackrox/rox/pkg/env"
"github.com/stackrox/rox/pkg/scanners"
"github.com/stackrox/rox/pkg/scanners/clairify"
)

var (
scannerEndpoint = fmt.Sprintf("scanner.%s.svc", env.Namespace.Setting())
)

// DefaultImageIntegrations are the default public registries
var DefaultImageIntegrations = []*storage.ImageIntegration{
{
Expand Down Expand Up @@ -126,7 +121,7 @@ var (
},
IntegrationConfig: &storage.ImageIntegration_Clairify{
Clairify: &storage.ClairifyConfig{
Endpoint: fmt.Sprintf("https://%s:8080", scannerEndpoint),
Endpoint: fmt.Sprintf("https://%s:8080", clairify.GetScannerEndpoint()),
},
},
}
Expand Down
1 change: 1 addition & 0 deletions pkg/env/sensor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package env
var (
// CentralEndpoint is used to provide Central's reachable endpoint to a sensor.
CentralEndpoint = RegisterSetting("ROX_CENTRAL_ENDPOINT", WithDefault("central.stackrox.svc:443"))

// AdvertisedEndpoint is used to provide the Sensor with the endpoint it
// should advertise to services that need to contact it, within its own cluster.
AdvertisedEndpoint = RegisterSetting("ROX_ADVERTISED_ENDPOINT", WithDefault("sensor.stackrox.svc:443"))
Expand Down
14 changes: 10 additions & 4 deletions pkg/scanners/clairify/clairify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,16 @@ const (
var (
_ scannerTypes.Scanner = (*clairify)(nil)
_ scannerTypes.ImageVulnerabilityGetter = (*clairify)(nil)
)

var (
log = logging.LoggerForModule()
log = logging.LoggerForModule()
scannerEndpoint = fmt.Sprintf("scanner.%s.svc", env.Namespace.Setting())
)

// GetScannerEndpoint returns the scanner endpoint with a configured namespace. env.ScannerGRPCEndpoint is only used by Sensor.
Copy link
Copy Markdown
Contributor

@RTann RTann Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

env.ScannerGRPCEndpoint should probably also be scanner.<namespace>.svc:8443, right? or perhaps we should not use that at all and just use this instead. Otherwise, wouldn't OpenShifts using non-stackrox namespaces has "local scanning" issues?

Copy link
Copy Markdown
Contributor Author

@SimonBaeumer SimonBaeumer Nov 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default for the scanner endpoint is set by Helm in Sensor's deployment spec. The ROX_NAMESPACE env var which the function depends on is never set on Sensor, thus has no effect.

https://github.com/stackrox/stackrox/blob/master/image/templates/helm/stackrox-secured-cluster/templates/sensor.yaml.htpl#L124-L129

We can align this behaviour but would prefer to do it in a different PR to merge this one first.

func GetScannerEndpoint() string {
return scannerEndpoint
}

// Creator provides the type scanners.Creator to add to the scanners Registry.
func Creator(set registries.Set) (string, func(integration *storage.ImageIntegration) (scannerTypes.Scanner, error)) {
return TypeString, func(integration *storage.ImageIntegration) (scannerTypes.Scanner, error) {
Expand Down Expand Up @@ -144,9 +148,11 @@ func createGRPCConnectionToScanner(conf *storage.ClairifyConfig) (*grpc.ClientCo
return nil, err
}

// Checking for an empty endpoint can't be removed because of backward-compatibility. Existing image
// integrations are configured in the database on Central's startup and are not updated dynamically.
endpoint := conf.GetGrpcEndpoint()
if endpoint == "" {
endpoint = fmt.Sprintf("scanner.%s:8443", env.Namespace.Setting())
endpoint = fmt.Sprintf("%s:8443", GetScannerEndpoint())
}

// Note: it is possible we call `grpc.Dial` multiple times per endpoint,
Expand Down