Conversation
f066ed0 to
b0510f5
Compare
|
Tag for build #221570 is 💻 For deploying this image using the dev scripts, run the following first: export MAIN_IMAGE_TAG='3.68.x-220-ge207aaae10'📦 You can also generate an installation bundle with: docker run -i --rm stackrox/main:3.68.x-220-ge207aaae10 central generate interactive > bundle.zip🕹️ A |
SimonBaeumer
left a comment
There was a problem hiding this comment.
Looks good so far, pulled @RTann in to give feedback on the configuration.
| // Settings for the local Scanner component, which is responsible for vulnerability scanning of container | ||
| // images stored in a cluster-local image repository. | ||
| //+operator-sdk:csv:customresourcedefinitions:type=spec,order=7,displayName="Local Scanner Component Settings" | ||
| Scanner *ScannerComponentSpec `json:"scanner,omitempty"` |
There was a problem hiding this comment.
Do you see a chance that we can protect the API changes by the ROX_LOCAL_IMAGE_SCANNING flag?
There was a problem hiding this comment.
Apart from introducing whole another operators, which IMHO is definitely not worth the effort, I can see the following options.
Regarding visibility of this feature in the OpenShift UI:
- attempt to initially hide the new API in the UI using some magic annotations, not sure if it's actually possible
- initially document this config section as "pre-alpha, don't look here"
Orthogonal to the above: introduce an environment variable (that can be set in the operator Subscription), which would control whether the reconciliation fails with an error in case spec.scanner is non-nil, or proceeds as in this PR. This flag could also control whether our validation webhook prevents specifying a scanner or not.
Actually e2e-testing the behaviour in both cases would require some additional effort.
There was a problem hiding this comment.
So WDYT @SimonBaeumer ? Given the recent developments, perhaps we should just not worry about the feature flag at all, since this PR will not make it to release 68, anyway, and hopefully the complete thing will land in release 69?
| } | ||
|
|
||
| // ScannerComponentSpec defines settings for the "scanner" component. | ||
| type ScannerComponentSpec struct { |
There was a problem hiding this comment.
@RTann Can you give us insights how the local scanner should be configured?
Can we assume that the local scanner has the same configuration options like Central?
There was a problem hiding this comment.
I believe the only difference between the main Scanner and a Secured Cluster's Scanner is the image name.
Scanner and Scanner-DB remain as scanner and scanner-db. The "local" Scanner and Scanner-DB will be scanner-slim and scanner-db-slim. Internally, they use an environment variable to differentiate (ROX_SLIM_MODE), as they use the same binary, but the env var is embedded into the images already.
Aside from that, I don't think any of the configurations need to be different. Perhaps we can look into some metrics at some point to see if we should default to 3 replicas or some other number, but I'd say it's fine for now. I believe the ConfigMap and Service would all be the same, too.
| @@ -1,32 +1,31 @@ | |||
| **/go.sum | |||
There was a problem hiding this comment.
can we move this and tools/detect-large-files.sh into a different PR?
There was a problem hiding this comment.
The change to operator/... path is a pre-requisite, but yes, I plan to restructure this and the followup PR a bit to make them smaller.
| } | ||
|
|
||
| // ScannerComponentSpec defines settings for the "scanner" component. | ||
| type ScannerComponentSpec struct { |
There was a problem hiding this comment.
I believe the only difference between the main Scanner and a Secured Cluster's Scanner is the image name.
Scanner and Scanner-DB remain as scanner and scanner-db. The "local" Scanner and Scanner-DB will be scanner-slim and scanner-db-slim. Internally, they use an environment variable to differentiate (ROX_SLIM_MODE), as they use the same binary, but the env var is embedded into the images already.
Aside from that, I don't think any of the configurations need to be different. Perhaps we can look into some metrics at some point to see if we should default to 3 replicas or some other number, but I'd say it's fine for now. I believe the ConfigMap and Service would all be the same, too.
| //+operator-sdk:csv:customresourcedefinitions:type=spec,order=6,displayName="Kubernetes Audit Logs Ingestion Settings" | ||
| AuditLogs *AuditLogsSpec `json:"auditLogs,omitempty"` | ||
|
|
||
| // Settings for the local Scanner component, which is responsible for vulnerability scanning of container |
There was a problem hiding this comment.
@RTann so how should we call this scanner in the description which is shown in the OpenShift UI? "Local scanner"? "Secured cluster scanner"? Just "scanner" and hope the user does not get confused?
4e0e958 to
e207aaa
Compare
|
Superseded by #691 |
Description
First stab at exposing local scanner in SecuredCluster CR.
Checklist
If any of these don't apply, please comment below.
Testing Performed
TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why you did not do so. (Valid reasons include "CI is sufficient" or "No testable changes")
In addition to reviewing your code, reviewers must also review your testing instructions, and make sure they are sufficient.
For more details, ref the Confluence page about this section.