ROX-33465: Remove OCP 3 support, make OCP auto-sensing more reliable#19895
Draft
mclasmeier wants to merge 33 commits intomasterfrom
Draft
ROX-33465: Remove OCP 3 support, make OCP auto-sensing more reliable#19895mclasmeier wants to merge 33 commits intomasterfrom
mclasmeier wants to merge 33 commits intomasterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The new OpenShift auto-sensing logic in
srox.autoSenseOpenshiftVersionnow only keys offconfig.openshift.io/v1; it might be worth explicitly considering/encoding what should happen in restricted-API orhelm templatescenarios where that group is not visible (e.g., should there be an explicit fallback or note to users that OpenShift auto-detect will not trigger in that case). - The temporary
replace github.com/stackrox/helmtestingo.modwould benefit from a clear TODO or tracking reference indicating when/how it should be removed once upstreamhelmtesthas OpenShift 4.12 schema support.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new OpenShift auto-sensing logic in `srox.autoSenseOpenshiftVersion` now only keys off `config.openshift.io/v1`; it might be worth explicitly considering/encoding what should happen in restricted-API or `helm template` scenarios where that group is not visible (e.g., should there be an explicit fallback or note to users that OpenShift auto-detect will not trigger in that case).
- The temporary `replace github.com/stackrox/helmtest` in `go.mod` would benefit from a clear TODO or tracking reference indicating when/how it should be removed once upstream `helmtest` has OpenShift 4.12 schema support.
## Individual Comments
### Comment 1
<location path="image/templates/helm/shared/templates/_openshift.tpl" line_range="27-29" />
<code_context>
- {{ $_ := set $env "openshift" 3 }}
- {{ end }}
- {{ include "srox.note" (list $ (printf "Based on API server properties, we have inferred that you are deploying into an OpenShift %d.x cluster. Set the `env.openshift` property explicitly to 3 or 4 to override the auto-sensed value." $env.openshift)) }}
+ {{/* We only support OpenShift 4. */}}
+ {{ $_ := set $env "openshift" 4 }}
+ {{ include "srox.note" (list $ (printf "Based on API server properties, we have inferred that you are deploying into an OpenShift %d.x cluster." $env.openshift)) }}
{{ end }}
+
</code_context>
<issue_to_address>
**suggestion:** The note message can be misleading when the user explicitly sets `env.openshift: true`.
In the `kindIs "bool"` branch, this note always claims the value was inferred from the API server, but when the user explicitly sets `env.openshift: true`, it isn’t inferred. Consider either only emitting the note when the value was auto-detected, or adjusting the wording to also cover explicit configuration (e.g., "Based on API server properties or your configuration...").
```suggestion
{{/* We only support OpenShift 4. */}}
{{ $_ := set $env "openshift" 4 }}
{{ include "srox.note" (list $ (printf "Based on API server properties or your configuration, we have determined that you are deploying into an OpenShift %d.x cluster." $env.openshift)) }}
```
</issue_to_address>
### Comment 2
<location path="image/templates/helm/stackrox-secured-cluster/templates/admission-controller.yaml" line_range="28-29" />
<code_context>
{{- if or ._rox.scannerV4._indexerEnabled ._rox.scannerV4._matcherEnabled }}
{{- if ._rox.env.openshift }}
- {{- if eq ._rox.env.openshift 3 }}
- {{ include "srox.warn" (list . "On OpenShift 3.x no SecurityContextConstraint will be configured for Scanner V4.") }}
</code_context>
<issue_to_address>
**issue (bug_risk):** Using a truthy check on `. _rox.env.openshift` may apply the SCC annotation for unsupported OpenShift versions.
Since `._rox.env.openshift` is now an integer, any non-zero (including unsupported) value will be truthy and cause `openshift.io/required-scc` to be added for versions like `1` or `5`. To match the intended behavior of only supporting version 4, use an explicit equality check instead, e.g. `if eq ._rox.env.openshift 4`.
</issue_to_address>
### Comment 3
<location path="tests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats" line_range="44-42" />
<code_context>
run_generate_and_get_bundle_test k8s "k8s-test-cluster"
}
-@test "[openshift3] roxctl sensor generate and get-bundle" {
- run_generate_and_get_bundle_test openshift "oc3-test-cluster" --openshift-version 3
-}
-
@test "[openshift4] roxctl sensor generate and get-bundle" {
</code_context>
<issue_to_address>
**suggestion (testing):** Replace the removed OpenShift 3 test with a negative test that asserts version 3 is now rejected
This test previously validated the `--openshift-version 3` path, which is now invalid. Rather than dropping coverage, please add a bats test that runs `roxctl sensor generate --openshift-version 3` and asserts a non‑zero exit and the new error text (e.g., containing `only the value '4' is supported`) to document and enforce the new "no OpenShift 3" behavior.
</issue_to_address>
### Comment 4
<location path="CHANGELOG.md" line_range="33" />
<code_context>
### Technical Changes
+- OpenShift 3 removed from Helm charts and roxctl manifest bundle generation.
+
## [4.10.0]
</code_context>
<issue_to_address>
**suggestion (typo):** Consider clarifying that OpenShift 3 support is what has been removed.
Consider rephrasing to "OpenShift 3 support removed from Helm charts and roxctl manifest bundle generation" to make it grammatically clear what is being removed.
```suggestion
- OpenShift 3 support removed from Helm charts and roxctl manifest bundle generation.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+27
to
+29
| {{/* We only support OpenShift 4. */}} | ||
| {{ $_ := set $env "openshift" 4 }} | ||
| {{ include "srox.note" (list $ (printf "Based on API server properties, we have inferred that you are deploying into an OpenShift %d.x cluster." $env.openshift)) }} |
Contributor
There was a problem hiding this comment.
suggestion: The note message can be misleading when the user explicitly sets env.openshift: true.
In the kindIs "bool" branch, this note always claims the value was inferred from the API server, but when the user explicitly sets env.openshift: true, it isn’t inferred. Consider either only emitting the note when the value was auto-detected, or adjusting the wording to also cover explicit configuration (e.g., "Based on API server properties or your configuration...").
Suggested change
| {{/* We only support OpenShift 4. */}} | |
| {{ $_ := set $env "openshift" 4 }} | |
| {{ include "srox.note" (list $ (printf "Based on API server properties, we have inferred that you are deploying into an OpenShift %d.x cluster." $env.openshift)) }} | |
| {{/* We only support OpenShift 4. */}} | |
| {{ $_ := set $env "openshift" 4 }} | |
| {{ include "srox.note" (list $ (printf "Based on API server properties or your configuration, we have determined that you are deploying into an OpenShift %d.x cluster." $env.openshift)) }} |
Contributor
There was a problem hiding this comment.
Sure! I'm generating a new review now.
Contributor
There was a problem hiding this comment.
Hey @mclasmeier, I've posted a new review for you!
Comment on lines
+28
to
29
| {{- if ._rox.env.openshift }} | ||
| {{- $_ := set $annotations "openshift.io/required-scc" "restricted-v2" -}} |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Using a truthy check on . _rox.env.openshift may apply the SCC annotation for unsupported OpenShift versions.
Since ._rox.env.openshift is now an integer, any non-zero (including unsupported) value will be truthy and cause openshift.io/required-scc to be added for versions like 1 or 5. To match the intended behavior of only supporting version 4, use an explicit equality check instead, e.g. if eq ._rox.env.openshift 4.
Contributor
Author
There was a problem hiding this comment.
Please check again, I have changed the autosensing code.
Contributor
There was a problem hiding this comment.
Sure! I'm generating a new review now.
Contributor
There was a problem hiding this comment.
Hey @mclasmeier, I've posted a new review for you!
| @@ -41,10 +41,6 @@ run_generate_and_get_bundle_test() { | |||
| run_generate_and_get_bundle_test k8s "k8s-test-cluster" | |||
| } | |||
Contributor
There was a problem hiding this comment.
suggestion (testing): Replace the removed OpenShift 3 test with a negative test that asserts version 3 is now rejected
This test previously validated the --openshift-version 3 path, which is now invalid. Rather than dropping coverage, please add a bats test that runs roxctl sensor generate --openshift-version 3 and asserts a non‑zero exit and the new error text (e.g., containing only the value '4' is supported) to document and enforce the new "no OpenShift 3" behavior.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR removes OpenShift 3 support project-wide: deletes OpenShift 3 detection/conditional logic, deprecates the OPENSHIFT_CLUSTER enum value, updates Helm templates and tests to target OpenShift 4, and makes CLI/renderer/validation changes to reject or fail OpenShift 3 inputs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.69.3)Failed to read Trivy output file: ENOENT: no such file or directory, open '/inmem/1275/nsjail-a68e2923-9a1c-406b-a5c1-040db4df76f5/merged/.trivy-output.json' Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yaml (1)
40-43:⚠️ Potential issue | 🟠 MajorUpdate stale nested OpenShift schema to 4.12.
Line 43 still uses
openshift-4.1.0while this file now targetsopenshift-4.12(Line 9). This inconsistency can make the matrix drift and cause schema-dependent test behavior to diverge.Suggested fix
- name: "on OpenShift 4" server: availableSchemas: - - openshift-4.1.0 + - openshift-4.12🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yaml` around lines 40 - 43, Replace the stale schema value under the availableSchemas entry: find the nested availableSchemas list that currently contains "openshift-4.1.0" and update that string to match the file's target "openshift-4.12" (e.g., "openshift-4.12" or "openshift-4.12.0" to match project convention) so the availableSchemas entry is consistent with the top-level target.
🧹 Nitpick comments (1)
roxctl/sensor/generate/openshift.go (1)
23-39: Dead code due to hardcoded cluster type.Since
s.cluster.Typeis unconditionally set toOPENSHIFT4_CLUSTERon line 23:
- Line 31: The condition
s.cluster.GetType() == storage.ClusterType_OPENSHIFT4_CLUSTERis always true, makings.cluster.AdmissionControllerEventsalwaystrue.- Line 36: The condition
s.cluster.GetType() != storage.ClusterType_OPENSHIFT4_CLUSTERis always false, sos.disableAuditLogCollectiondefaults tofalse(audit logs enabled).This logic is functionally correct for the intended behavior (OpenShift 4 only), but the conditions are now redundant and could be simplified.
♻️ Optional simplification
func (s *sensorGenerateOpenShiftCommand) ConstructOpenShift() error { s.cluster.Type = storage.ClusterType_OPENSHIFT4_CLUSTER switch s.openshiftVersion { case 0: case 4: default: return errox.InvalidArgs.Newf("invalid OpenShift version %d, currently only the value '4' is supported", s.openshiftVersion) } - s.cluster.AdmissionControllerEvents = s.cluster.GetType() == storage.ClusterType_OPENSHIFT4_CLUSTER + s.cluster.AdmissionControllerEvents = true // This is intentionally NOT feature-flagged, because we always want to set the correct (auto) value, // even if we turn off the flag before shipping. if s.disableAuditLogCollection == nil { - s.disableAuditLogCollection = pointers.Bool(s.cluster.GetType() != storage.ClusterType_OPENSHIFT4_CLUSTER) + s.disableAuditLogCollection = pointers.Bool(false) } s.cluster.DynamicConfig.DisableAuditLogs = *s.disableAuditLogCollection return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@roxctl/sensor/generate/openshift.go` around lines 23 - 39, The code sets s.cluster.Type unconditionally to storage.ClusterType_OPENSHIFT4_CLUSTER, so remove the redundant runtime checks and simplify: after setting s.cluster.Type and validating s.openshiftVersion, directly set s.cluster.AdmissionControllerEvents = true, and if s.disableAuditLogCollection is nil set it to pointers.Bool(false) (since OpenShift 4 should enable audit logs by default), then assign s.cluster.DynamicConfig.DisableAuditLogs = *s.disableAuditLogCollection; keep the openshiftVersion switch and references to s.cluster.Type, s.cluster.AdmissionControllerEvents, s.disableAuditLogCollection, and s.cluster.DynamicConfig.DisableAuditLogs to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@image/templates/helm/shared/templates/_openshift.tpl`:
- Around line 38-40: Replace the current non-fatal warning with a hard failure
for unsupported OpenShift versions: in the _openshift.tpl template, where the
conditional checks (and (ne $env.openshift 0) (ne $env.openshift 4)) and
currently calls include "srox.warning", change it to call Helm's fail with a
clear error message (including $env.openshift) so rendering aborts for any
non-zero, non-4 value; mention $env.openshift, the include "srox.warning" call
being replaced, and that this prevents downstream templates that check
._rox.env.openshift (and render OpenShift 4-only resources like the
restricted-v2 SCC annotation/binding) from producing broken manifests.
In
`@pkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-autosense.test.yaml`:
- Around line 12-21: The parent test fixture incorrectly sets env.openshift:
true causing all child cases to use the explicit override path; remove the
top-level "set: env.openshift: true" so the default/auto-sense path is
exercised, then keep the child case named "and env.openshift is set to true"
with its own set: env.openshift: true to test the explicit override; ensure the
top-level expect that checks the inferred OpenShift note remains valid for the
auto-sense case and the configmap assertion still applies.
---
Outside diff comments:
In
`@pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yaml`:
- Around line 40-43: Replace the stale schema value under the availableSchemas
entry: find the nested availableSchemas list that currently contains
"openshift-4.1.0" and update that string to match the file's target
"openshift-4.12" (e.g., "openshift-4.12" or "openshift-4.12.0" to match project
convention) so the availableSchemas entry is consistent with the top-level
target.
---
Nitpick comments:
In `@roxctl/sensor/generate/openshift.go`:
- Around line 23-39: The code sets s.cluster.Type unconditionally to
storage.ClusterType_OPENSHIFT4_CLUSTER, so remove the redundant runtime checks
and simplify: after setting s.cluster.Type and validating s.openshiftVersion,
directly set s.cluster.AdmissionControllerEvents = true, and if
s.disableAuditLogCollection is nil set it to pointers.Bool(false) (since
OpenShift 4 should enable audit logs by default), then assign
s.cluster.DynamicConfig.DisableAuditLogs = *s.disableAuditLogCollection; keep
the openshiftVersion switch and references to s.cluster.Type,
s.cluster.AdmissionControllerEvents, s.disableAuditLogCollection, and
s.cluster.DynamicConfig.DisableAuditLogs to locate the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0b802fd5-6430-4c4e-a492-2105e9186095
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
CHANGELOG.mdgo.modimage/templates/helm/shared/templates/02-scanner-v4-01-security.yamlimage/templates/helm/shared/templates/_openshift.tplimage/templates/helm/stackrox-secured-cluster/internal/defaults/30-base-config.yaml.htplimage/templates/helm/stackrox-secured-cluster/templates/_init.tpl.htplimage/templates/helm/stackrox-secured-cluster/templates/admission-controller.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/central.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-auth.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-autosense.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner-v4.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config-disabled/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-non-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-non-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/override/override.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/rhacs/rhacs.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/stackrox/stackrox.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/audit-logs.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/env.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/legacy-settings.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-slim.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-v4.test.yamlroxctl/central/generate/k8s.goroxctl/sensor/generate/openshift.gotests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats
💤 Files with no reviewable changes (4)
- tests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats
- image/templates/helm/stackrox-secured-cluster/internal/defaults/30-base-config.yaml.htpl
- pkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yaml
- image/templates/helm/stackrox-secured-cluster/templates/_init.tpl.htpl
pkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-autosense.test.yaml
Outdated
Show resolved
Hide resolved
50d66bb to
2f9e4cd
Compare
Contributor
🚀 Build Images ReadyImages are ready for commit 86903ae. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-640-g86903ae0f7 |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
srox.autoSenseOpenshiftVersion, the note text still claims the version is inferred "based on API server properties" even whenenv.openshift=trueis mapped unconditionally to 4 without checking. _rox._apiServer.apiResources; consider differentiating the message for explicit overrides vs. true auto-detection to avoid confusing users. - The new
srox.failfor unsupportedenv.openshiftvalues is helpful, but it will also trigger if someone passes a non-integer-like value (e.g. a string that cannot be cast byint); if that case is possible in your charts, you may want a clearer error path or validation before theintconversion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `srox.autoSenseOpenshiftVersion`, the note text still claims the version is inferred "based on API server properties" even when `env.openshift=true` is mapped unconditionally to 4 without checking `. _rox._apiServer.apiResources`; consider differentiating the message for explicit overrides vs. true auto-detection to avoid confusing users.
- The new `srox.fail` for unsupported `env.openshift` values is helpful, but it will also trigger if someone passes a non-integer-like value (e.g. a string that cannot be cast by `int`); if that case is possible in your charts, you may want a clearer error path or validation before the `int` conversion.
## Individual Comments
### Comment 1
<location path="tests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats" line_range="44-42" />
<code_context>
run_generate_and_get_bundle_test k8s "k8s-test-cluster"
}
-@test "[openshift3] roxctl sensor generate and get-bundle" {
- run_generate_and_get_bundle_test openshift "oc3-test-cluster" --openshift-version 3
-}
-
@test "[openshift4] roxctl sensor generate and get-bundle" {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a negative test that asserts `--openshift-version 3` now fails as expected
Since we’ve removed the OpenShift 3 happy-path test, we no longer verify that `--openshift-version 3` now fails. Please add a bats test that runs `roxctl sensor generate` with `--openshift-version 3` and asserts it exits with an error and shows the new "only the value '4' is supported" message. This will clearly document and lock in the new rejection behavior.
Suggested implementation:
```shell
run_generate_and_get_bundle_test k8s "k8s-test-cluster"
}
@test "[openshift3] roxctl sensor generate rejects unsupported openshift version" {
run roxctl sensor generate --openshift-version 3
# Command must fail
[ "$status" -ne 0 ]
# Error message should clearly state only '4' is supported
[[ "$output" == *"only the value '4' is supported"* ]]
}
@test "[openshift4] roxctl sensor generate and get-bundle" {
run_generate_and_get_bundle_test openshift "oc4-test-cluster" --openshift-version 4
}
```
If `roxctl sensor generate` in your project requires mandatory positional arguments or additional flags (for example, a cluster name or output directory), you should update the new test’s `run roxctl sensor generate --openshift-version 3` line to include the minimal valid arguments used elsewhere in this file so that argument validation passes and the test specifically exercises the `--openshift-version` validation logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-slim.test.yaml (1)
20-22: Consider removing the now-redundant nested test block.With the OpenShift 3 variant removed, the nested
tests:block containing only "on openshift 4" (with no additional configuration) is redundant—the parent test already runs against theopenshift-4.12schema. Removing lines 20-22 would simplify the test structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-slim.test.yaml` around lines 20 - 22, Remove the redundant nested tests block that only contains the "on openshift 4" entry: delete the inner `tests:` block and its single item ("on openshift 4") from the test YAML so the parent test remains and the redundant nested `tests:` block is gone; locate the nested `tests:` key and the literal "on openshift 4" in the scanner-slim.test.yaml and remove those lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-slim.test.yaml`:
- Around line 20-22: Remove the redundant nested tests block that only contains
the "on openshift 4" entry: delete the inner `tests:` block and its single item
("on openshift 4") from the test YAML so the parent test remains and the
redundant nested `tests:` block is gone; locate the nested `tests:` key and the
literal "on openshift 4" in the scanner-slim.test.yaml and remove those lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7604c9c5-b143-4982-9a93-73370d7936f8
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
CHANGELOG.mdgo.modimage/templates/helm/shared/templates/02-scanner-v4-01-security.yamlimage/templates/helm/shared/templates/_openshift.tplimage/templates/helm/stackrox-secured-cluster/internal/defaults/30-base-config.yaml.htplimage/templates/helm/stackrox-secured-cluster/templates/_init.tpl.htplimage/templates/helm/stackrox-secured-cluster/templates/admission-controller.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/central.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-auth.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-autosense.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner-v4.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config-disabled/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-non-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-non-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/override/override.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/rhacs/rhacs.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/stackrox/stackrox.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/audit-logs.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/env.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/legacy-settings.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-slim.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-v4.test.yamlroxctl/central/generate/k8s.goroxctl/sensor/generate/openshift.gotests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats
💤 Files with no reviewable changes (4)
- pkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yaml
- tests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats
- image/templates/helm/stackrox-secured-cluster/templates/_init.tpl.htpl
- image/templates/helm/stackrox-secured-cluster/internal/defaults/30-base-config.yaml.htpl
✅ Files skipped from review due to trivial changes (18)
- pkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-auth.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-non-release/opensource.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/scanner-v4.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-release/development_build.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/override/override.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/rhacs/rhacs.test.yaml
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/injected-cabundle-cm.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/scanner.test.yaml
- roxctl/central/generate/k8s.go
- CHANGELOG.md
- pkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config-disabled/admission-control.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/injected-cabundle-cm.test.yaml
- go.mod
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/stackrox/stackrox.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-non-release/development_build.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/central.test.yaml
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/audit-logs.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-release/opensource.test.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yaml
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/env.test.yaml
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/openshift-monitoring.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-monitoring.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-autosense.test.yaml
- roxctl/sensor/generate/openshift.go
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-v4.test.yaml
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/legacy-settings.test.yaml
2f9e4cd to
cc5e645
Compare
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The temporary
replace github.com/stackrox/helmtestin go.mod is pinned to a specific commit; consider adding a follow-up plan (e.g., TODO with a JIRA reference) to switch back to a tagged release or remove the replace once the new helmtest version is published to avoid carrying a long-term fork override.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The temporary `replace github.com/stackrox/helmtest` in go.mod is pinned to a specific commit; consider adding a follow-up plan (e.g., TODO with a JIRA reference) to switch back to a tagged release or remove the replace once the new helmtest version is published to avoid carrying a long-term fork override.
## Individual Comments
### Comment 1
<location path="roxctl/central/generate/k8s.go" line_range="168-176" />
<code_context>
clusterType := storage.ClusterType_OPENSHIFT4_CLUSTER
switch openshiftVersion {
case 0:
- case 3:
- clusterType = storage.ClusterType_OPENSHIFT_CLUSTER
case 4:
default:
- return 0, errors.Errorf("invalid OpenShift version %d, supported values are '3' and '4'", openshiftVersion)
</code_context>
<issue_to_address>
**suggestion:** Reduce the OpenShift version switch to a validation check only
Since `clusterType` is always initialized to `OPENSHIFT4_CLUSTER` and the `switch` no longer changes it for 0 or 4, you can replace the `switch` with a simple validation:
```go
clusterType := storage.ClusterType_OPENSHIFT4_CLUSTER
if openshiftVersion != 0 && openshiftVersion != 4 {
return 0, errors.Errorf("invalid OpenShift version %d, only '4' is currently supported", openshiftVersion)
}
return clusterType, nil
```
This drops the unused cases and makes it explicit that 0 is "unspecified/default" and 4 is the only supported version.
```suggestion
clusterType := storage.ClusterType_OPENSHIFT4_CLUSTER
if openshiftVersion != 0 && openshiftVersion != 4 {
return 0, errors.Errorf("invalid OpenShift version %d, only '4' is currently supported", openshiftVersion)
}
return clusterType, nil
})
validFormats := []string{"kubectl", "helm", "helm-values"}
flagWrap.Var(&fileFormatWrapper{DeploymentFormat: &k8sConfig.DeploymentFormat}, "output-format", fmt.Sprintf("The deployment tool to use (%s).", strings.Join(validFormats, ", ")), "central")
```
</issue_to_address>
### Comment 2
<location path="tests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats" line_range="44-42" />
<code_context>
run_generate_and_get_bundle_test k8s "k8s-test-cluster"
}
-@test "[openshift3] roxctl sensor generate and get-bundle" {
- run_generate_and_get_bundle_test openshift "oc3-test-cluster" --openshift-version 3
-}
-
@test "[openshift4] roxctl sensor generate and get-bundle" {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a negative/validation test for unsupported OpenShift versions instead of only removing the OpenShift 3 happy-path test.
Since the new logic rejects any version other than 0/4, it would be helpful to add a bats test that calls `roxctl sensor generate` with `--openshift-version 3` (and optionally another invalid value like `5`), asserts the command fails, and verifies the error message matches the new validation text. This will make the new behavior explicit and help catch regressions where unsupported versions might be accepted again.
Suggested implementation:
```shell
@test "[openshift4] roxctl sensor generate and get-bundle" {
run_generate_and_get_bundle_test openshift "oc4-test-cluster" --openshift-version 4
}
@test "[openshift invalid version 3] roxctl sensor generate fails with validation error" {
# Attempt to generate a sensor bundle for an unsupported OpenShift 3 cluster
run roxctl sensor generate cluster \
--name "oc3-invalid-cluster" \
--openshift \
--openshift-version 3
# Command must fail for unsupported OpenShift versions
[ "$status" -ne 0 ]
# Output should contain the validation error for unsupported OpenShift version 3
[[ "$output" == *"unsupported OpenShift version: 3"* ]] || {
echo "Unexpected error output for OpenShift version 3:"
echo "$output"
return 1
}
}
@test "[openshift invalid version 5] roxctl sensor generate fails with validation error" {
# Attempt to generate a sensor bundle for another unsupported OpenShift version
run roxctl sensor generate cluster \
--name "oc5-invalid-cluster" \
--openshift \
--openshift-version 5
# Command must fail for unsupported OpenShift versions
[ "$status" -ne 0 ]
# Output should contain the validation error for unsupported OpenShift version 5
[[ "$output" == *"unsupported OpenShift version: 5"* ]] || {
echo "Unexpected error output for OpenShift version 5:"
echo "$output"
return 1
}
}
```
1. Adjust the `roxctl` invocation if this test suite uses a wrapper or variable (e.g., `$ROXCTL` or a helper function) instead of calling `roxctl` directly.
2. Replace `"unsupported OpenShift version: X"` with the exact validation message emitted by the new logic so the substring check matches the real output.
3. If `sensor generate cluster` requires additional mandatory flags (e.g., central endpoint, output dir, auth), add the same minimal required options here as used in `run_generate_and_get_bundle_test` to ensure the failure reason is strictly the OpenShift version validation and not missing parameters.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
9c1b8a7 to
da7ce9f
Compare
da7ce9f to
870f534
Compare
Contributor
Author
|
@coderabbitai Could I get a new review? |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yaml (1)
40-43:⚠️ Potential issue | 🟠 MajorUpdate the nested OpenShift schema to 4.12 for support-boundary consistency.
Line 43 still pins
openshift-4.1.0while this file now defaults toopenshift-4.12. That keeps coverage on an unsupported baseline and can mask regressions against the current minimum supported OpenShift version.Suggested fix
- name: "on OpenShift 4" server: availableSchemas: - - openshift-4.1.0 + - openshift-4.12🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yaml` around lines 40 - 43, Update the nested OpenShift schema under the availableSchemas key so it targets the current minimum supported OpenShift version: replace the entry "openshift-4.1.0" with "openshift-4.12" (the block with name "on OpenShift 4" and key availableSchemas) to ensure tests and coverage use the supported baseline.
🧹 Nitpick comments (3)
pkg/renderer/central_db_test.go (1)
209-210: Make the OpenShift 3 failure assertion more specific.The current check validates a generic render wrapper message, so unrelated OpenShift-3 render failures can also satisfy this test. Prefer asserting an unsupported-platform reason to keep this test targeted.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Suggested assertion tightening
- assert.Contains(suite.T(), err.Error(), "rendering Helm chart") + assert.Contains(suite.T(), err.Error(), "OpenShift 3")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/renderer/central_db_test.go` around lines 209 - 210, The current test only checks for the generic "rendering Helm chart" wrapper which is too broad; tighten the assertion by checking that err.Error() contains the unsupported-platform reason for OpenShift 3 (e.g. assert.Contains(suite.T(), err.Error(), "unsupported platform") and/or "OpenShift 3") instead of the generic message so the test specifically validates an OpenShift-3 unsupported-platform failure; update the assertion that uses err and assert.Contains to look for the unsupported-platform text (or the project constant/error identifier for unsupported platform if one exists) in place of "rendering Helm chart".pkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-autosense.test.yaml (1)
8-20: Add one fixture for the newenv.openshift: truebranch.This file now covers auto-sensing and unsupported numeric input, but it never exercises the boolean override that
image/templates/helm/shared/templates/_openshift.tplnow maps directly to OpenShift 4. A regression there would still pass this suite.🧪 Suggested fixture
- name: "Detect OpenShift 4 if env.openshift is unset and server resources are visible" server: visibleSchemas: - openshift-4.12 expect: | .notes | assertThat(contains("we have inferred that you are deploying into an OpenShift 4")) .configmaps["injected-cabundle-stackrox-central-services"] | assertThat(. != null) +- name: "Render OpenShift resources when env.openshift is explicitly true" + set: + env.openshift: true + expect: | + .configmaps["injected-cabundle-stackrox-central-services"] | assertThat(. != null) - name: "Fail if env.openshift is set to an unsupported version" set: env.openshift: 3As per coding guidelines, focus on major issues impacting performance, readability, maintainability and security.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-autosense.test.yaml` around lines 8 - 20, The test suite is missing a fixture that exercises the boolean override branch where env.openshift: true is treated as OpenShift 4 by image/templates/helm/shared/templates/_openshift.tpl; add a new test case entry (similar to the existing "Detect OpenShift 4..." case) that sets env.openshift: true under set:, and assert the same outcomes (notes message containing "we have inferred that you are deploying into an OpenShift 4" and the existence of .configmaps["injected-cabundle-stackrox-central-services"]). Ensure the new fixture uses a unique name (e.g., "Boolean override: env.openshift true maps to OpenShift 4") so it runs alongside the existing autosense and unsupported-version tests.pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/legacy-settings.test.yaml (1)
58-64: Keep the explicitOPENSHIFT4_CLUSTERpath independent of auto-sensing.Lines 62-64 make the fake server look like OpenShift, so
ROX_OPENSHIFT_APIcan still be satisfied by discovery even if the explicitcluster.type: OPENSHIFT4_CLUSTERhandling regresses. Removing that server override here would keep this fixture focused on the legacy-mode translation it is naming and asserting.🎯 Suggested test simplification
- name: OpenShift in Legacy mode accepts OpenShift4 cluster type values: cluster: type: OPENSHIFT4_CLUSTER - server: - visibleSchemas: - - openshift-4.12 expect: | .secrets["helm-cluster-config"].stringData["config.yaml"] | fromyaml | .clusterConfig.staticConfig.type | assertThat(. == "OPENSHIFT4_CLUSTER") envVars(.deployments.sensor; "sensor")["ROX_OPENSHIFT_API"] | assertThat(. == "true")As per coding guidelines, focus on major issues impacting performance, readability, maintainability and security.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/legacy-settings.test.yaml` around lines 58 - 64, The test currently sets both cluster.type: OPENSHIFT4_CLUSTER and a fake server visibleSchemas: [openshift-4.12], which lets discovery satisfy ROX_OPENSHIFT_API; remove the server.visibleSchemas override (the server.visibleSchemas: - openshift-4.12 block) so the test only relies on the explicit cluster.type: OPENSHIFT4_CLUSTER path and thus detects regressions in the legacy-mode translation handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 557-558: The go.mod currently pins github.com/stackrox/helmtest to
a pseudo-version referencing a PR commit via the replace directive; once
helmtest PR `#201` is merged and a release is published, update the replace line
for github.com/stackrox/helmtest to use the official released semantic version
(replace the pseudo-version v0.0.0-20260408100353-99e7fa7c6df6 with the released
tag), removing any reliance on PR commits and ensuring reproducible builds.
In `@tests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats`:
- Around line 53-57: The test calls the helper run_generate_and_get_bundle_test
which enforces success, so change the test to invoke the sensor generate command
in a way that allows failure (either replace run_generate_and_get_bundle_test
with a helper that does not assert success, or call the command directly via
run) so the subsequent assert_failure and assert_output checks can validate the
error; update the test to use that non-asserting invocation (referencing
run_generate_and_get_bundle_test) and keep the assert_failure and assert_output
--partial "only '4' is currently supported" assertions.
---
Outside diff comments:
In
`@pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yaml`:
- Around line 40-43: Update the nested OpenShift schema under the
availableSchemas key so it targets the current minimum supported OpenShift
version: replace the entry "openshift-4.1.0" with "openshift-4.12" (the block
with name "on OpenShift 4" and key availableSchemas) to ensure tests and
coverage use the supported baseline.
---
Nitpick comments:
In
`@pkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-autosense.test.yaml`:
- Around line 8-20: The test suite is missing a fixture that exercises the
boolean override branch where env.openshift: true is treated as OpenShift 4 by
image/templates/helm/shared/templates/_openshift.tpl; add a new test case entry
(similar to the existing "Detect OpenShift 4..." case) that sets env.openshift:
true under set:, and assert the same outcomes (notes message containing "we have
inferred that you are deploying into an OpenShift 4" and the existence of
.configmaps["injected-cabundle-stackrox-central-services"]). Ensure the new
fixture uses a unique name (e.g., "Boolean override: env.openshift true maps to
OpenShift 4") so it runs alongside the existing autosense and
unsupported-version tests.
In
`@pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/legacy-settings.test.yaml`:
- Around line 58-64: The test currently sets both cluster.type:
OPENSHIFT4_CLUSTER and a fake server visibleSchemas: [openshift-4.12], which
lets discovery satisfy ROX_OPENSHIFT_API; remove the server.visibleSchemas
override (the server.visibleSchemas: - openshift-4.12 block) so the test only
relies on the explicit cluster.type: OPENSHIFT4_CLUSTER path and thus detects
regressions in the legacy-mode translation handling.
In `@pkg/renderer/central_db_test.go`:
- Around line 209-210: The current test only checks for the generic "rendering
Helm chart" wrapper which is too broad; tighten the assertion by checking that
err.Error() contains the unsupported-platform reason for OpenShift 3 (e.g.
assert.Contains(suite.T(), err.Error(), "unsupported platform") and/or
"OpenShift 3") instead of the generic message so the test specifically validates
an OpenShift-3 unsupported-platform failure; update the assertion that uses err
and assert.Contains to look for the unsupported-platform text (or the project
constant/error identifier for unsupported platform if one exists) in place of
"rendering Helm chart".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f7933b81-b4fd-4021-927b-8ae285aca959
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
CHANGELOG.mdgo.modimage/templates/helm/shared/templates/02-scanner-v4-01-security.yamlimage/templates/helm/shared/templates/_openshift.tplimage/templates/helm/stackrox-secured-cluster/internal/defaults/30-base-config.yaml.htplimage/templates/helm/stackrox-secured-cluster/templates/_init.tpl.htplimage/templates/helm/stackrox-secured-cluster/templates/admission-controller.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/central.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-auth.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-autosense.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner-v4.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config-disabled/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-non-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-non-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/override/override.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/rhacs/rhacs.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/stackrox/stackrox.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/audit-logs.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/env.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/legacy-settings.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-slim.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-v4.test.yamlpkg/renderer/central_db_test.goroxctl/central/generate/k8s.goroxctl/sensor/generate/openshift.gotests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats
💤 Files with no reviewable changes (3)
- image/templates/helm/stackrox-secured-cluster/templates/_init.tpl.htpl
- pkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yaml
- image/templates/helm/stackrox-secured-cluster/internal/defaults/30-base-config.yaml.htpl
go.mod
Outdated
Comment on lines
+557
to
+558
| // Temporary: Use helmtest PR #201 for openshift-4.12 schema support | ||
| replace github.com/stackrox/helmtest => github.com/stackrox/helmtest v0.0.0-20260408100353-99e7fa7c6df6 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
MODULE="github.com/stackrox/helmtest"
VERSION="v0.0.0-20260408100353-99e7fa7c6df6"
COMMIT="${VERSION##*-}"
echo "Checking Go proxy metadata..."
curl -fsS "https://proxy.golang.org/${MODULE}/@v/${VERSION}.info" | tee /tmp/helmtest.info
echo "Checking Go proxy module file..."
curl -fsS "https://proxy.golang.org/${MODULE}/@v/${VERSION}.mod" >/tmp/helmtest.mod
wc -c /tmp/helmtest.mod
echo "Checking commit reachability on GitHub refs..."
git ls-remote https://github.com/stackrox/helmtest.git | grep -i "${COMMIT}" || {
echo "Commit ${COMMIT} not found in advertised refs."
exit 1
}
echo "OK: version is currently resolvable and commit is reachable."Repository: stackrox/stackrox
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
MODULE="github.com/stackrox/helmtest"
VERSION="v0.0.0-20260408100353-99e7fa7c6df6"
COMMIT="${VERSION##*-}"
echo "Module: $MODULE"
echo "Version: $VERSION"
echo "Commit: $COMMIT"
echo ""
echo "Attempting to check Go proxy metadata..."
curl -fsS "https://proxy.golang.org/${MODULE}/@v/${VERSION}.info" 2>&1 | head -20 || echo "Metadata check failed"
echo ""
echo "Attempting to check Go proxy module file..."
curl -fsS "https://proxy.golang.org/${MODULE}/@v/${VERSION}.mod" 2>&1 | head -20 || echo "Module file check failed"
echo ""
echo "Checking commit reachability on GitHub refs..."
git ls-remote https://github.com/stackrox/helmtest.git 2>&1 | grep -i "${COMMIT}" || echo "Commit ${COMMIT} not found in advertised refs"Repository: stackrox/stackrox
Length of output: 1104
🏁 Script executed:
# Check the go.mod file context around lines 557-558
head -560 go.mod | tail -10Repository: stackrox/stackrox
Length of output: 715
🌐 Web query:
stackrox helmtest PR 201 merged release version
💡 Result:
PR stackrox/helmtest #201 (“Include openshift-4.12 schema”) is still Open (not merged) as of April 8, 2026, so there is no release version that includes it yet. [1]
The most recent published release shown on the repo is v0.0.8, and because #201 is not merged, it is not part of v0.0.8 (or any released tag). [2]
Sources: [1] [2]
Switch to a stable released version once helmtest#201 is merged and tagged.
The pinned pseudo-version is currently resolvable via Go proxy and the commit is reachable on GitHub (it's in PR #201). However, this dependency on an open PR is not a long-term stable approach. Once PR #201 lands and a release is published, update this to the released version tag instead of the pseudo-version to ensure reproducible builds are not dependent on PR branches or commit accessibility changing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` around lines 557 - 558, The go.mod currently pins
github.com/stackrox/helmtest to a pseudo-version referencing a PR commit via the
replace directive; once helmtest PR `#201` is merged and a release is published,
update the replace line for github.com/stackrox/helmtest to use the official
released semantic version (replace the pseudo-version
v0.0.0-20260408100353-99e7fa7c6df6 with the released tag), removing any reliance
on PR commits and ensuring reproducible builds.
Contributor
Author
There was a problem hiding this comment.
this will be done as soon as we have a new tag.
Contributor
There was a problem hiding this comment.
@mclasmeier, understood — the comment in go.mod already calls this out as temporary (// Temporary: Use helmtest PR #201 for openshift-4.12 schema support), so this is well-documented. Just remember to swap the pseudo-version for the proper semver tag once helmtest#201 is merged and tagged.
ecf43d3 to
9d0db55
Compare
Contributor
Author
|
@coderabbitai Could I get a new review? |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
image/templates/helm/stackrox-secured-cluster/internal/compatibility-translation.yaml (1)
13-15: Consider failing explicitly forOPENSHIFT_CLUSTERinput rather than silent downgrade.When
cluster.typeisOPENSHIFT_CLUSTER(legacy OpenShift 3), this translation now yieldsopenshift: false, which would result inKUBERNETES_CLUSTERbeing set downstream. While the validation inpkg/cluster/validation.gowould rejectOPENSHIFT_CLUSTERelsewhere, the compatibility translation layer silently treating it as non-OpenShift could cause confusion.Consider adding explicit handling to fail with a clear migration message when
OPENSHIFT_CLUSTERis encountered:💡 Suggested approach
type: | {{- if eq .rawValue "OPENSHIFT_CLUSTER" -}} {{ fail "OpenShift 3.x (OPENSHIFT_CLUSTER) is no longer supported. Please use OPENSHIFT4_CLUSTER or enable auto-detection." }} {{- end }} env: openshift: {{ eq .rawValue "OPENSHIFT4_CLUSTER" }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@image/templates/helm/stackrox-secured-cluster/internal/compatibility-translation.yaml` around lines 13 - 15, The compatibility translation silently maps legacy OPENSHIFT_CLUSTER to non-OpenShift; update the template in compatibility-translation.yaml under the type block to explicitly detect .rawValue == "OPENSHIFT_CLUSTER" and call the template fail with a clear migration message (e.g., "OpenShift 3.x (OPENSHIFT_CLUSTER) is no longer supported...") before the env section, leaving the existing openshift: check for .rawValue == "OPENSHIFT4_CLUSTER" intact.central/clusters/zip/render_test.go (1)
100-107: Dead code: theelsebranch is unreachable.Since
TestRenderOpenshiftEnvonly runs withstorage.ClusterType_OPENSHIFT4_CLUSTER(line 42-44), the else branch at lines 100-107 can never execute. Consider removing this dead code path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@central/clusters/zip/render_test.go` around lines 100 - 107, The else branch inside TestRenderOpenshiftEnv is dead because the test only sets storage.ClusterType_OPENSHIFT4_CLUSTER; remove the unreachable else block (the code that checks for absence of "node-inventory" and asserts env.NodeInventoryContainerEnabled == "false") from TestRenderOpenshiftEnv, or alternatively convert the else branch into a separate test that explicitly sets a non-OpenShift cluster type; update the test to only contain the assertions relevant to storage.ClusterType_OPENSHIFT4_CLUSTER (refer to the TestRenderOpenshiftEnv function and the getEnvVarValue/complianceCont.Env checks to locate the code to remove or refactor).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@central/clusters/zip/render_test.go`:
- Around line 100-107: The else branch inside TestRenderOpenshiftEnv is dead
because the test only sets storage.ClusterType_OPENSHIFT4_CLUSTER; remove the
unreachable else block (the code that checks for absence of "node-inventory" and
asserts env.NodeInventoryContainerEnabled == "false") from
TestRenderOpenshiftEnv, or alternatively convert the else branch into a separate
test that explicitly sets a non-OpenShift cluster type; update the test to only
contain the assertions relevant to storage.ClusterType_OPENSHIFT4_CLUSTER (refer
to the TestRenderOpenshiftEnv function and the getEnvVarValue/complianceCont.Env
checks to locate the code to remove or refactor).
In
`@image/templates/helm/stackrox-secured-cluster/internal/compatibility-translation.yaml`:
- Around line 13-15: The compatibility translation silently maps legacy
OPENSHIFT_CLUSTER to non-OpenShift; update the template in
compatibility-translation.yaml under the type block to explicitly detect
.rawValue == "OPENSHIFT_CLUSTER" and call the template fail with a clear
migration message (e.g., "OpenShift 3.x (OPENSHIFT_CLUSTER) is no longer
supported...") before the env section, leaving the existing openshift: check for
.rawValue == "OPENSHIFT4_CLUSTER" intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 60ad43a7-1673-4f04-9b64-1455f0dace5e
⛔ Files ignored due to path filters (3)
generated/internalapi/central/v1/token_service.pb.gois excluded by!**/*.pb.go,!**/generated/**generated/storage/cluster.pb.gois excluded by!**/*.pb.go,!**/generated/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (55)
CHANGELOG.mdcentral/cluster/datastore/datastore_impl_postgres_test.gocentral/clusters/zip/render_test.gocentral/complianceoperator/v2/integration/datastore/datastore_impl_test.gocentral/complianceoperator/v2/integration/service/convert_test.gocentral/complianceoperator/v2/integration/service/service_impl_test.gocentral/graphql/resolvers/test_utils.gocentral/telemetry/centralclient/client.gocentral/views/platformcve/view_test.gogo.modimage/embed_charts.goimage/templates/helm/shared/templates/02-scanner-v4-01-security.yamlimage/templates/helm/shared/templates/_openshift.tplimage/templates/helm/stackrox-secured-cluster/internal/cluster-config.yaml.tpl.htplimage/templates/helm/stackrox-secured-cluster/internal/compatibility-translation.yamlimage/templates/helm/stackrox-secured-cluster/internal/defaults/30-base-config.yaml.htplimage/templates/helm/stackrox-secured-cluster/sensor-chart-upgrade.md.htplimage/templates/helm/stackrox-secured-cluster/templates/_init.tpl.htplimage/templates/helm/stackrox-secured-cluster/templates/admission-controller.yamlimage/templates/helm/stackrox-secured-cluster/values.yaml.htplmigrator/migrations/m_211_to_m_212_admission_control_config/migration_test.gopkg/cluster/validation.gopkg/cluster/validation_test.gopkg/helm/charts/tests/centralservices/testdata/helmtest/central.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-auth.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-autosense.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner-v4.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config-disabled/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-non-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-non-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/override/override.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/rhacs/rhacs.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/stackrox/stackrox.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/audit-logs.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/env.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/legacy-settings.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-slim.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-v4.test.yamlpkg/renderer/central_db_test.gopkg/renderer/kubernetes_test.goproto/storage/cluster.protoroxctl/central/generate/generate_test.goroxctl/central/generate/k8s.goroxctl/sensor/generate/openshift.gosensor/kubernetes/telemetry/gatherers/cluster.gotests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats
💤 Files with no reviewable changes (8)
- central/graphql/resolvers/test_utils.go
- central/complianceoperator/v2/integration/datastore/datastore_impl_test.go
- image/templates/helm/stackrox-secured-cluster/values.yaml.htpl
- central/cluster/datastore/datastore_impl_postgres_test.go
- roxctl/central/generate/generate_test.go
- image/templates/helm/stackrox-secured-cluster/internal/defaults/30-base-config.yaml.htpl
- pkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yaml
- image/templates/helm/stackrox-secured-cluster/templates/_init.tpl.htpl
✅ Files skipped from review due to trivial changes (19)
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-non-release/development_build.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/scanner-v4.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-release/development_build.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/stackrox/stackrox.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/injected-cabundle-cm.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/central.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-non-release/opensource.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-release/opensource.test.yaml
- image/templates/helm/stackrox-secured-cluster/sensor-chart-upgrade.md.htpl
- pkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-auth.test.yaml
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-slim.test.yaml
- pkg/cluster/validation_test.go
- proto/storage/cluster.proto
- pkg/helm/charts/tests/centralservices/testdata/helmtest/scanner.test.yaml
- CHANGELOG.md
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/rhacs/rhacs.test.yaml
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-v4.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/override/override.test.yaml
- roxctl/central/generate/k8s.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/injected-cabundle-cm.test.yaml
- tests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/env.test.yaml
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yaml
- image/templates/helm/stackrox-secured-cluster/templates/admission-controller.yaml
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/openshift-monitoring.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-autosense.test.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
image/embed_charts.go (1)
274-278:⚠️ Potential issue | 🟠 Major
addScriptsnow rejects legacy OpenShift cluster type.Line 274 no longer accepts
OPENSHIFT_CLUSTER, so those clusters now hit the invalid-type error at Line 277. Givencentral/clusters/deployer.go(Line 128-130) still treatsOPENSHIFT_CLUSTERas OpenShift, this is a compatibility regression.Proposed fix
- } else if values.ClusterType == storage.ClusterType_OPENSHIFT4_CLUSTER.String() { + } else if values.ClusterType == storage.ClusterType_OPENSHIFT4_CLUSTER.String() || + values.ClusterType == storage.ClusterType_OPENSHIFT_CLUSTER.String() { return i.scripts(values, osScriptsFileMap) }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@image/embed_charts.go` around lines 274 - 278, The addScripts branch currently only accepts storage.ClusterType_OPENSHIFT4_CLUSTER.String(), causing legacy OPENSHIFT_CLUSTER to fall through and trigger the error; update the conditional in the function containing this code (the addScripts/embedding logic that calls i.scripts) to treat both storage.ClusterType_OPENSHIFT4_CLUSTER.String() and storage.ClusterType_OPENSHIFT_CLUSTER.String() as OpenShift by calling i.scripts for either value (or use a helper that detects “is OpenShift”), and only return the invalid-cluster-type error for genuinely unsupported cluster types.
♻️ Duplicate comments (1)
tests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats (1)
53-57:⚠️ Potential issue | 🔴 CriticalTest logic is incorrect—helper enforces success before the failure assertion is reached.
The
run_generate_and_get_bundle_testhelper (line 31-38) callsassert_successaftergenerate_bundle. When--openshift-version 3causes the command to fail, the helper's internalassert_successwill fail the test beforeassert_failureon line 55 is reached.Proposed fix
`@test` "[openshift3] roxctl sensor generate rejects unsupported openshift version" { - run_generate_and_get_bundle_test openshift "oc3-test-cluster" --openshift-version 3 + generate_bundle openshift --name "oc3-test-cluster" --openshift-version 3 assert_failure assert_output --partial "only '4' is currently supported" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats` around lines 53 - 57, The helper run_generate_and_get_bundle_test currently calls generate_bundle then unconditionally calls assert_success, which prevents tests expecting failure (like the openshift v3 case) from reaching their own assert_failure; fix by either removing the unconditional assert_success in run_generate_and_get_bundle_test or adding an optional parameter (e.g., expect_failure) so the helper skips assert_success when a failure is expected, then update the failing test to call run_generate_and_get_bundle_test with expect_failure=true (or call generate_bundle directly and assert_failure/assert_output there). Ensure references include run_generate_and_get_bundle_test, generate_bundle, and assert_success so reviewers can find and adjust the helper behavior.
🧹 Nitpick comments (1)
pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yaml (1)
40-43: Consider updating the nested schema to match the minimum supported version.The nested test "on OpenShift 4" still uses
openshift-4.1.0while the top-level and other tests useopenshift-4.12. Since 4.12 is the minimum supported version per this PR, consider updating this for consistency—unless intentionally testing against an older OCP 4 schema for backward compatibility within the OCP 4 line.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yaml` around lines 40 - 43, Update the nested test entry named "on OpenShift 4" so its availableSchemas value matches the minimum supported OCP schema; locate the "on OpenShift 4" test block and change the availableSchemas entry from "openshift-4.1.0" to "openshift-4.12" (unless you intentionally need to test older 4.1 compatibility).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@central/telemetry/centralclient/client.go`:
- Around line 116-118: The Orchestrator telemetry change emits
storage.ClusterType_OPENSHIFT4_CLUSTER when env.Openshift.BooleanSetting() is
true but there is no test covering that path and test config still uses the old
OPENSHIFT_CLUSTER string; add a unit/integration test that sets env.Openshift to
true and asserts the emitted orchestrator equals
storage.ClusterType_OPENSHIFT4_CLUSTER.String(), update any test fixture/config
(e.g., qa-tests-backend/helm/values.openshift.yaml) to reference
OPENSHIFT4_CLUSTER, and search for other usages of the old OPENSHIFT_CLUSTER
value to migrate them so tests and aggregations validate the new enum value
(reference symbols: env.Openshift, orchestrator variable,
storage.ClusterType_OPENSHIFT4_CLUSTER, and the client.go telemetry emission
code).
In
`@image/templates/helm/stackrox-secured-cluster/internal/compatibility-translation.yaml`:
- Line 15: The openshift compatibility mapping currently only checks for
"OPENSHIFT4_CLUSTER" and drops legacy "OPENSHIFT_CLUSTER"; update the template
expression that sets the openshift field (the line with openshift: {{ eq
.rawValue "OPENSHIFT4_CLUSTER" }}) to treat "OPENSHIFT_CLUSTER" as an alias too
by checking both values (e.g., using a logical OR between eq .rawValue
"OPENSHIFT4_CLUSTER" and eq .rawValue "OPENSHIFT_CLUSTER") so legacy inputs are
classified as OpenShift while preserving OCP4 behavior.
In `@pkg/renderer/central_db_test.go`:
- Around line 209-210: Replace the non-fatal assertion on the error with a fatal
one so the test does not dereference nil: change the call to
assert.Error(suite.T(), err) to require.Error(suite.T(), err) (or add an
explicit nil check before calling err.Error()) where the test invokes render and
inspects err, ensuring the subsequent assert.Contains(suite.T(), err.Error(),
"You have specified OpenShift version 3") only runs when err is non-nil.
In `@roxctl/central/generate/k8s.go`:
- Line 173: Update the cluster type mapping and error guidance so OpenShift 3 is
no longer exposed: in the clusterStringToType map remove the "openshift" key or
change its value from OPENSHIFT_CLUSTER to OPENSHIFT4_CLUSTER so the CLI will
resolve "openshift" to the OpenShift 4 type, and update the error text produced
by the OpenShift version check (the message returned when openshiftVersion != 4)
to explicitly tell users to use --cluster-type openshift4 (or the exact
openshift4 enum) rather than the deprecated openshift mapping.
In `@roxctl/sensor/generate/openshift.go`:
- Line 28: Update the error text returned when an unsupported OpenShift version
is provided (the call that constructs errox.InvalidArgs.Newf using
s.openshiftVersion) so the message contains the exact phrase "only '4' is
currently supported" (e.g., "invalid OpenShift version %d, only '4' is currently
supported") to match the bats test expectation.
---
Outside diff comments:
In `@image/embed_charts.go`:
- Around line 274-278: The addScripts branch currently only accepts
storage.ClusterType_OPENSHIFT4_CLUSTER.String(), causing legacy
OPENSHIFT_CLUSTER to fall through and trigger the error; update the conditional
in the function containing this code (the addScripts/embedding logic that calls
i.scripts) to treat both storage.ClusterType_OPENSHIFT4_CLUSTER.String() and
storage.ClusterType_OPENSHIFT_CLUSTER.String() as OpenShift by calling i.scripts
for either value (or use a helper that detects “is OpenShift”), and only return
the invalid-cluster-type error for genuinely unsupported cluster types.
---
Duplicate comments:
In `@tests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats`:
- Around line 53-57: The helper run_generate_and_get_bundle_test currently calls
generate_bundle then unconditionally calls assert_success, which prevents tests
expecting failure (like the openshift v3 case) from reaching their own
assert_failure; fix by either removing the unconditional assert_success in
run_generate_and_get_bundle_test or adding an optional parameter (e.g.,
expect_failure) so the helper skips assert_success when a failure is expected,
then update the failing test to call run_generate_and_get_bundle_test with
expect_failure=true (or call generate_bundle directly and
assert_failure/assert_output there). Ensure references include
run_generate_and_get_bundle_test, generate_bundle, and assert_success so
reviewers can find and adjust the helper behavior.
---
Nitpick comments:
In
`@pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yaml`:
- Around line 40-43: Update the nested test entry named "on OpenShift 4" so its
availableSchemas value matches the minimum supported OCP schema; locate the "on
OpenShift 4" test block and change the availableSchemas entry from
"openshift-4.1.0" to "openshift-4.12" (unless you intentionally need to test
older 4.1 compatibility).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0792bcd6-2f40-4fb1-8e15-f2b652b85144
⛔ Files ignored due to path filters (3)
generated/internalapi/central/v1/token_service.pb.gois excluded by!**/*.pb.go,!**/generated/**generated/storage/cluster.pb.gois excluded by!**/*.pb.go,!**/generated/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (55)
CHANGELOG.mdcentral/cluster/datastore/datastore_impl_postgres_test.gocentral/clusters/zip/render_test.gocentral/complianceoperator/v2/integration/datastore/datastore_impl_test.gocentral/complianceoperator/v2/integration/service/convert_test.gocentral/complianceoperator/v2/integration/service/service_impl_test.gocentral/graphql/resolvers/test_utils.gocentral/telemetry/centralclient/client.gocentral/views/platformcve/view_test.gogo.modimage/embed_charts.goimage/templates/helm/shared/templates/02-scanner-v4-01-security.yamlimage/templates/helm/shared/templates/_openshift.tplimage/templates/helm/stackrox-secured-cluster/internal/cluster-config.yaml.tpl.htplimage/templates/helm/stackrox-secured-cluster/internal/compatibility-translation.yamlimage/templates/helm/stackrox-secured-cluster/internal/defaults/30-base-config.yaml.htplimage/templates/helm/stackrox-secured-cluster/sensor-chart-upgrade.md.htplimage/templates/helm/stackrox-secured-cluster/templates/_init.tpl.htplimage/templates/helm/stackrox-secured-cluster/templates/admission-controller.yamlimage/templates/helm/stackrox-secured-cluster/values.yaml.htplmigrator/migrations/m_211_to_m_212_admission_control_config/migration_test.gopkg/cluster/validation.gopkg/cluster/validation_test.gopkg/helm/charts/tests/centralservices/testdata/helmtest/central.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-auth.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-autosense.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner-v4.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config-disabled/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-non-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-non-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/override/override.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/rhacs/rhacs.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/stackrox/stackrox.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/audit-logs.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/env.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/legacy-settings.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-slim.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-v4.test.yamlpkg/renderer/central_db_test.gopkg/renderer/kubernetes_test.goproto/storage/cluster.protoroxctl/central/generate/generate_test.goroxctl/central/generate/k8s.goroxctl/sensor/generate/openshift.gosensor/kubernetes/telemetry/gatherers/cluster.gotests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats
💤 Files with no reviewable changes (8)
- image/templates/helm/stackrox-secured-cluster/values.yaml.htpl
- image/templates/helm/stackrox-secured-cluster/templates/_init.tpl.htpl
- central/graphql/resolvers/test_utils.go
- central/cluster/datastore/datastore_impl_postgres_test.go
- central/complianceoperator/v2/integration/datastore/datastore_impl_test.go
- image/templates/helm/stackrox-secured-cluster/internal/defaults/30-base-config.yaml.htpl
- roxctl/central/generate/generate_test.go
- pkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yaml
Comment on lines
116
to
118
| if env.Openshift.BooleanSetting() { | ||
| orchestrator = storage.ClusterType_OPENSHIFT_CLUSTER.String() | ||
| orchestrator = storage.ClusterType_OPENSHIFT4_CLUSTER.String() | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify telemetry consumers/assertions are migrated to OPENSHIFT4_CLUSTER.
# Expected: no remaining consumer logic that only recognizes OPENSHIFT_CLUSTER for OpenShift telemetry.
rg -n -C3 --type=go --type=yaml --type=json 'Orchestrator|OPENSHIFT_CLUSTER|OPENSHIFT4_CLUSTER'Repository: stackrox/stackrox
Length of output: 50374
🏁 Script executed:
#!/bin/bash
# Search for where Orchestrator telemetry value is consumed/filtered in queries/dashboards
# Look for patterns like filtering, comparing, or aggregating on cluster type/orchestrator
rg -n "OPENSHIFT_CLUSTER|OPENSHIFT4_CLUSTER" --type=go -C2 | grep -v "node\|namespace\|compliance" | head -100Repository: stackrox/stackrox
Length of output: 7199
🏁 Script executed:
#!/bin/bash
# Find telemetry aggregation/dashboarding code
fd -e go -e ts -e js | xargs grep -l "telemetry.*[Oo]rchestrator\|aggregat.*cluster" 2>/dev/null | head -20Repository: stackrox/stackrox
Length of output: 371
🏁 Script executed:
#!/bin/bash
# Search central package telemetry client specifically
fd -type f -path "*/central/*telemetry*" -name "*.go" | head -20Repository: stackrox/stackrox
Length of output: 231
🏁 Script executed:
#!/bin/bash
# Check the actual file mentioned in the review
cat central/telemetry/centralclient/client.go | head -150Repository: stackrox/stackrox
Length of output: 5071
🏁 Script executed:
#!/bin/bash
# Look for filters or assertions on the Orchestrator value in telemetry/metrics
rg -n "Orchestrator.*OPENSHIFT|OPENSHIFT.*Orchestrator" --type=go | head -30Repository: stackrox/stackrox
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if the old OPENSHIFT_CLUSTER test value needs updating
cat qa-tests-backend/helm/values.openshift.yamlRepository: stackrox/stackrox
Length of output: 707
🏁 Script executed:
#!/bin/bash
# Search for telemetry assertions or test validations on cluster type
rg -n "OPENSHIFT_CLUSTER.*String\|String.*OPENSHIFT_CLUSTER" --type=go | head -30Repository: stackrox/stackrox
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look for any migration notes or comments related to this change
rg -n "OPENSHIFT4_CLUSTER|OPENSHIFT_CLUSTER" -B2 -A2 --type=go | grep -i "migration\|change\|old\|new" | head -40Repository: stackrox/stackrox
Length of output: 859
🏁 Script executed:
#!/bin/bash
# Check if test config needs updating - search for all helm test values files using this setting
fd -name "values*.yaml" | xargs grep -l "OPENSHIFT" 2>/dev/nullRepository: stackrox/stackrox
Length of output: 293
🏁 Script executed:
#!/bin/bash
# Look for where the telemetry property "Orchestrator" is documented or configured
rg -n '"Orchestrator"' --type=go | head -20Repository: stackrox/stackrox
Length of output: 313
🏁 Script executed:
#!/bin/bash
# Check if there are any related git commits or comments about this migration
git log --oneline --all -20 -- central/telemetry/centralclient/client.go 2>/dev/null || echo "Git not available"Repository: stackrox/stackrox
Length of output: 306
🏁 Script executed:
#!/bin/bash
# Search for documentation or migration guides
fd -name "*.md" | xargs grep -l "OPENSHIFT_CLUSTER\|telemetry.*orchestr" 2>/dev/null | head -10Repository: stackrox/stackrox
Length of output: 291
🏁 Script executed:
#!/bin/bash
# Check the test file to see what value is expected
cat central/telemetry/centralclient/client_test.go | grep -A5 -B5 "Orchestrator"Repository: stackrox/stackrox
Length of output: 674
🏁 Script executed:
#!/bin/bash
# Look for any GraphQL/dashboard code that might filter on this value
fd -name "*.graphql" -o -name "*dashboard*" | head -20Repository: stackrox/stackrox
Length of output: 291
🏁 Script executed:
#!/bin/bash
# Verify the enum values in proto/storage
rg "OPENSHIFT_CLUSTER|OPENSHIFT4_CLUSTER" generated/storage/*.pb.go | head -20Repository: stackrox/stackrox
Length of output: 658
Add test validation for the OpenShift telemetry value, and update test configuration to use OPENSHIFT4_CLUSTER.
The change to emit OPENSHIFT4_CLUSTER instead of OPENSHIFT_CLUSTER in the "Orchestrator" telemetry property is breaking for downstream aggregations that filter on the old string value. Test coverage is incomplete—there's no test case validating the env.Openshift code path, and test configuration files (qa-tests-backend/helm/values.openshift.yaml) still reference the old enum value, indicating the migration is incomplete across the codebase. Add a test case for the OpenShift scenario and ensure all configuration references are migrated to the new value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@central/telemetry/centralclient/client.go` around lines 116 - 118, The
Orchestrator telemetry change emits storage.ClusterType_OPENSHIFT4_CLUSTER when
env.Openshift.BooleanSetting() is true but there is no test covering that path
and test config still uses the old OPENSHIFT_CLUSTER string; add a
unit/integration test that sets env.Openshift to true and asserts the emitted
orchestrator equals storage.ClusterType_OPENSHIFT4_CLUSTER.String(), update any
test fixture/config (e.g., qa-tests-backend/helm/values.openshift.yaml) to
reference OPENSHIFT4_CLUSTER, and search for other usages of the old
OPENSHIFT_CLUSTER value to migrate them so tests and aggregations validate the
new enum value (reference symbols: env.Openshift, orchestrator variable,
storage.ClusterType_OPENSHIFT4_CLUSTER, and the client.go telemetry emission
code).
| type: | | ||
| env: | ||
| openshift: {{ if eq .rawValue "OPENSHIFT4_CLUSTER" }} 4 {{ else }} {{ eq .rawValue "OPENSHIFT_CLUSTER" }} {{ end }} | ||
| openshift: {{ eq .rawValue "OPENSHIFT4_CLUSTER" }} |
Contributor
There was a problem hiding this comment.
Preserve legacy OPENSHIFT_CLUSTER mapping in compatibility translation.
Line 15 drops support for legacy OPENSHIFT_CLUSTER input in the compatibility layer. That can misclassify existing legacy values as non-OpenShift and push them down the Kubernetes path. Keep accepting OPENSHIFT_CLUSTER as an alias while still emitting OCP4 behavior.
Proposed fix
- openshift: {{ eq .rawValue "OPENSHIFT4_CLUSTER" }}
+ openshift: {{ or (eq .rawValue "OPENSHIFT4_CLUSTER") (eq .rawValue "OPENSHIFT_CLUSTER") }}As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| openshift: {{ eq .rawValue "OPENSHIFT4_CLUSTER" }} | |
| openshift: {{ or (eq .rawValue "OPENSHIFT4_CLUSTER") (eq .rawValue "OPENSHIFT_CLUSTER") }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@image/templates/helm/stackrox-secured-cluster/internal/compatibility-translation.yaml`
at line 15, The openshift compatibility mapping currently only checks for
"OPENSHIFT4_CLUSTER" and drops legacy "OPENSHIFT_CLUSTER"; update the template
expression that sets the openshift field (the line with openshift: {{ eq
.rawValue "OPENSHIFT4_CLUSTER" }}) to treat "OPENSHIFT_CLUSTER" as an alias too
by checking both values (e.g., using a logical OR between eq .rawValue
"OPENSHIFT4_CLUSTER" and eq .rawValue "OPENSHIFT_CLUSTER") so legacy inputs are
classified as OpenShift while preserving OCP4 behavior.
Contributor
Author
|
/fixxx |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
image/embed_charts.go (1)
271-279:⚠️ Potential issue | 🟠 MajorPreserve legacy
OPENSHIFT_CLUSTERas an input alias inaddScripts.Line 274 drops deprecated enum handling, so Line 277 can now fail sensor bundle generation for legacy cluster type values that still exist in compatibility paths.
Proposed fix
func (i *Image) addScripts(values *charts.MetaValues) ([]*loader.BufferedFile, error) { if values.ClusterType == storage.ClusterType_KUBERNETES_CLUSTER.String() { return i.scripts(values, k8sScriptsFileMap) - } else if values.ClusterType == storage.ClusterType_OPENSHIFT4_CLUSTER.String() { + } else if values.ClusterType == storage.ClusterType_OPENSHIFT4_CLUSTER.String() || + values.ClusterType == storage.ClusterType_OPENSHIFT_CLUSTER.String() { return i.scripts(values, osScriptsFileMap) } return nil, errors.Errorf("unable to create sensor bundle, invalid cluster type for cluster %s", values.ClusterName) }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@image/embed_charts.go` around lines 271 - 279, The addScripts function currently only checks for ClusterType_KUBERNETES_CLUSTER and ClusterType_OPENSHIFT4_CLUSTER and will fail for legacy OPENSHIFT_CLUSTER values; update addScripts (function name: addScripts, variables: values, k8sScriptsFileMap, osScriptsFileMap) to also treat storage.ClusterType_OPENSHIFT_CLUSTER.String() as an alias for OPENSHIFT4 (i.e., call i.scripts(values, osScriptsFileMap) when values.ClusterType equals that deprecated enum string) so legacy compatibility paths continue to generate sensor bundles.
♻️ Duplicate comments (3)
central/telemetry/centralclient/client.go (1)
117-119:⚠️ Potential issue | 🟠 MajorThe comment documents the compatibility concern but doesn't resolve the telemetry inconsistency.
While the added comment usefully documents the known issue, the underlying problem remains: Sensor telemetry reports
OPENSHIFT4_CLUSTERwhile Central telemetry reportsOPENSHIFT_CLUSTERfor the same cluster type. This divergence impacts downstream analytics that aggregate on theOrchestratorproperty.As noted in the previous review, this requires test coverage for the
env.Openshiftcode path and migration of telemetry consumers to handle the new value consistently. Consider addressing this in a follow-up to ensure telemetry data integrity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@central/telemetry/centralclient/client.go` around lines 117 - 119, The telemetry mismatch is that centralclient sets orchestrator using storage.ClusterType_OPENSHIFT_CLUSTER.String() while sensor reports OPENSHIFT4_CLUSTER; update the central telemetry path where orchestrator is assigned (the variable orchestrator in client.go) to normalize OpenShift values to the canonical name used by sensor (e.g., map storage.ClusterType_OPENSHIFT_CLUSTER and any env.Openshift code paths to the OPENSHIFT4_CLUSTER string), add unit tests exercising the env.Openshift code path to assert the normalized value is emitted, and add a short note or migration helper for consumers to accept both legacy OPENSHIFT_CLUSTER and the canonical OPENSHIFT4_CLUSTER during transition.image/templates/helm/stackrox-secured-cluster/internal/compatibility-translation.yaml (1)
15-15:⚠️ Potential issue | 🟠 MajorKeep legacy
OPENSHIFT_CLUSTERcompatibility in translation mapping.Line 15 currently maps only
OPENSHIFT4_CLUSTER; older configs withOPENSHIFT_CLUSTERare translated as non-OpenShift, which can send them down the Kubernetes path.Proposed fix
- openshift: {{ eq .rawValue "OPENSHIFT4_CLUSTER" }} + openshift: {{ or (eq .rawValue "OPENSHIFT4_CLUSTER") (eq .rawValue "OPENSHIFT_CLUSTER") }}As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@image/templates/helm/stackrox-secured-cluster/internal/compatibility-translation.yaml` at line 15, The translation currently sets openshift only when .rawValue equals "OPENSHIFT4_CLUSTER", so legacy "OPENSHIFT_CLUSTER" is treated as non-OpenShift; update the mapping for the openshift key to treat either .rawValue == "OPENSHIFT4_CLUSTER" or .rawValue == "OPENSHIFT_CLUSTER" as true (i.e., use an OR of eq checks against .rawValue) so older configs are correctly translated to OpenShift.pkg/renderer/kubernetes_test.go (1)
120-135:⚠️ Potential issue | 🔴 CriticalLand the renderer-side OCP3 rejection with this test.
This new assertion depends on
pkg/renderer/kubernetes.goreturning an error forstorage.ClusterType_OPENSHIFT_CLUSTER. That gap was already identified on an earlier revision, and I still don't see matching production code in the provided context, so this test looks red until the validation is added.Use this to verify the reject path exists in the renderer:
#!/bin/bash set -euo pipefail echo "Render/postProcessConfig implementations:" rg -n -A20 -B5 '^func (Render|RenderScannerOnly|RenderCentralDBOnly|render|postProcessConfig)\b' pkg/renderer/kubernetes.go echo echo "OpenShift 3 handling in the renderer package:" rg -n 'OpenShift version 3|ClusterType_OPENSHIFT_CLUSTER' pkg/renderer --type go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/renderer/kubernetes_test.go` around lines 120 - 135, The renderer currently lacks validation to reject OpenShift 3 clusters; add a check in the entry points such as the Render function (and other entry wrappers like RenderScannerOnly/RenderCentralDBOnly or postProcessConfig if they delegate validation) in kubernetes.go that inspects conf.ClusterType and returns an error when it equals storage.ClusterType_OPENSHIFT_CLUSTER with the message containing "You have specified OpenShift version 3"; ensure the same validation path is hit by tests by placing it early in Render (or in a shared postProcessConfig called by Render) so the TestRenderFailsForOpenShift3 assertion passes.
🧹 Nitpick comments (2)
central/complianceoperator/v2/integration/datastore/datastore_impl_test.go (1)
260-260: Avoid raw enum literals in DB seed data.Using
5here is brittle; prefer the generated enum constant so intent survives enum evolution.♻️ Proposed fix
- _, err = s.db.DB.Exec(context.Background(), "insert into clusters (id, name, status_providermetadata_cluster_type, type) values ($1, $2, $3, $4)", testconsts.Cluster2, "cluster2", 2, 5) + _, err = s.db.DB.Exec(context.Background(), "insert into clusters (id, name, status_providermetadata_cluster_type, type) values ($1, $2, $3, $4)", testconsts.Cluster2, "cluster2", 2, storage.ClusterType_OPENSHIFT4_CLUSTER)As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@central/complianceoperator/v2/integration/datastore/datastore_impl_test.go` at line 260, The test seed uses a raw enum literal `5` in the Exec call for inserting into clusters; replace that numeric literal with the generated enum constant instead (i.e., import and use the enum constant from the package that defines cluster/provider metadata and pass that constant in place of `5` in the s.db.DB.Exec call that inserts into clusters), so change the insert call where s.db.DB.Exec(...) sets the cluster type to use the named enum (for example ClusterTypeFoo or ProvidermetadataClusterTypeFoo) to make the test resilient to enum reordering.roxctl/sensor/generate/openshift.go (1)
22-42: LGTM with optional simplification opportunity.The version validation correctly rejects unsupported OpenShift versions with the error message matching the bats test expectation.
Note: Since
s.cluster.Typeis unconditionally set toOPENSHIFT4_CLUSTERat line 23, the conditions at lines 31 and 36 are now always true/false respectively. This is functionally correct but could be simplified in a follow-up if desired.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@roxctl/sensor/generate/openshift.go` around lines 22 - 42, The ConstructOpenShift method sets s.cluster.Type to OPENSHIFT4_CLUSTER unconditionally, so simplify the redundant runtime checks: set s.cluster.AdmissionControllerEvents = true directly instead of comparing GetType(), set the nil-default for s.disableAuditLogCollection to pointers.Bool(false) (instead of pointers.Bool(s.cluster.GetType() != storage.ClusterType_OPENSHIFT4_CLUSTER)), and keep assigning s.cluster.DynamicConfig.DisableAuditLogs = *s.disableAuditLogCollection; update only these lines in ConstructOpenShift to remove the unnecessary GetType() branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 33: Update the changelog line "OpenShift 3 support removed from Helm
charts and roxctl manifest bundle generation." to explicitly state the new
minimum supported OpenShift version by appending "Minimum supported OpenShift
version: OpenShift 4.12" (or integrate that phrase into the sentence) so the
note reads clearly that OCP3 was removed and the new floor is OpenShift 4.12.
---
Outside diff comments:
In `@image/embed_charts.go`:
- Around line 271-279: The addScripts function currently only checks for
ClusterType_KUBERNETES_CLUSTER and ClusterType_OPENSHIFT4_CLUSTER and will fail
for legacy OPENSHIFT_CLUSTER values; update addScripts (function name:
addScripts, variables: values, k8sScriptsFileMap, osScriptsFileMap) to also
treat storage.ClusterType_OPENSHIFT_CLUSTER.String() as an alias for OPENSHIFT4
(i.e., call i.scripts(values, osScriptsFileMap) when values.ClusterType equals
that deprecated enum string) so legacy compatibility paths continue to generate
sensor bundles.
---
Duplicate comments:
In `@central/telemetry/centralclient/client.go`:
- Around line 117-119: The telemetry mismatch is that centralclient sets
orchestrator using storage.ClusterType_OPENSHIFT_CLUSTER.String() while sensor
reports OPENSHIFT4_CLUSTER; update the central telemetry path where orchestrator
is assigned (the variable orchestrator in client.go) to normalize OpenShift
values to the canonical name used by sensor (e.g., map
storage.ClusterType_OPENSHIFT_CLUSTER and any env.Openshift code paths to the
OPENSHIFT4_CLUSTER string), add unit tests exercising the env.Openshift code
path to assert the normalized value is emitted, and add a short note or
migration helper for consumers to accept both legacy OPENSHIFT_CLUSTER and the
canonical OPENSHIFT4_CLUSTER during transition.
In
`@image/templates/helm/stackrox-secured-cluster/internal/compatibility-translation.yaml`:
- Line 15: The translation currently sets openshift only when .rawValue equals
"OPENSHIFT4_CLUSTER", so legacy "OPENSHIFT_CLUSTER" is treated as non-OpenShift;
update the mapping for the openshift key to treat either .rawValue ==
"OPENSHIFT4_CLUSTER" or .rawValue == "OPENSHIFT_CLUSTER" as true (i.e., use an
OR of eq checks against .rawValue) so older configs are correctly translated to
OpenShift.
In `@pkg/renderer/kubernetes_test.go`:
- Around line 120-135: The renderer currently lacks validation to reject
OpenShift 3 clusters; add a check in the entry points such as the Render
function (and other entry wrappers like RenderScannerOnly/RenderCentralDBOnly or
postProcessConfig if they delegate validation) in kubernetes.go that inspects
conf.ClusterType and returns an error when it equals
storage.ClusterType_OPENSHIFT_CLUSTER with the message containing "You have
specified OpenShift version 3"; ensure the same validation path is hit by tests
by placing it early in Render (or in a shared postProcessConfig called by
Render) so the TestRenderFailsForOpenShift3 assertion passes.
---
Nitpick comments:
In `@central/complianceoperator/v2/integration/datastore/datastore_impl_test.go`:
- Line 260: The test seed uses a raw enum literal `5` in the Exec call for
inserting into clusters; replace that numeric literal with the generated enum
constant instead (i.e., import and use the enum constant from the package that
defines cluster/provider metadata and pass that constant in place of `5` in the
s.db.DB.Exec call that inserts into clusters), so change the insert call where
s.db.DB.Exec(...) sets the cluster type to use the named enum (for example
ClusterTypeFoo or ProvidermetadataClusterTypeFoo) to make the test resilient to
enum reordering.
In `@roxctl/sensor/generate/openshift.go`:
- Around line 22-42: The ConstructOpenShift method sets s.cluster.Type to
OPENSHIFT4_CLUSTER unconditionally, so simplify the redundant runtime checks:
set s.cluster.AdmissionControllerEvents = true directly instead of comparing
GetType(), set the nil-default for s.disableAuditLogCollection to
pointers.Bool(false) (instead of pointers.Bool(s.cluster.GetType() !=
storage.ClusterType_OPENSHIFT4_CLUSTER)), and keep assigning
s.cluster.DynamicConfig.DisableAuditLogs = *s.disableAuditLogCollection; update
only these lines in ConstructOpenShift to remove the unnecessary GetType()
branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 83780462-35a3-440b-8d8a-6683c7353b11
⛔ Files ignored due to path filters (3)
generated/storage/cluster.pb.gois excluded by!**/*.pb.go,!**/generated/**go.sumis excluded by!**/*.sumproto/storage/proto.lockis excluded by!**/*.lock
📒 Files selected for processing (55)
CHANGELOG.mdcentral/cluster/datastore/datastore_impl_postgres_test.gocentral/clusters/zip/render_test.gocentral/complianceoperator/v2/integration/datastore/datastore_impl_test.gocentral/complianceoperator/v2/integration/service/convert_test.gocentral/complianceoperator/v2/integration/service/service_impl_test.gocentral/graphql/resolvers/cluster_vulnerabilities_postgres_test.gocentral/graphql/resolvers/test_utils.gocentral/telemetry/centralclient/client.gocentral/views/platformcve/view_test.gogo.modimage/embed_charts.goimage/templates/helm/shared/templates/02-scanner-v4-01-security.yamlimage/templates/helm/shared/templates/_openshift.tplimage/templates/helm/stackrox-secured-cluster/internal/cluster-config.yaml.tpl.htplimage/templates/helm/stackrox-secured-cluster/internal/compatibility-translation.yamlimage/templates/helm/stackrox-secured-cluster/internal/defaults/30-base-config.yaml.htplimage/templates/helm/stackrox-secured-cluster/sensor-chart-upgrade.md.htplimage/templates/helm/stackrox-secured-cluster/templates/_init.tpl.htplimage/templates/helm/stackrox-secured-cluster/templates/admission-controller.yamlimage/templates/helm/stackrox-secured-cluster/values.yaml.htplmigrator/migrations/m_211_to_m_212_admission_control_config/migration_test.gopkg/cluster/validation.gopkg/cluster/validation_test.gopkg/helm/charts/tests/centralservices/testdata/helmtest/central.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-auth.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-autosense.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner-v4.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config-disabled/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-non-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-non-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/override/override.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/rhacs/rhacs.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/stackrox/stackrox.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/audit-logs.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/env.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/legacy-settings.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-slim.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-v4.test.yamlpkg/renderer/central_db_test.gopkg/renderer/kubernetes_test.goproto/storage/cluster.protoroxctl/central/generate/generate_test.goroxctl/central/generate/k8s.goroxctl/sensor/generate/openshift.gotests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats
💤 Files with no reviewable changes (7)
- image/templates/helm/stackrox-secured-cluster/values.yaml.htpl
- central/graphql/resolvers/test_utils.go
- roxctl/central/generate/generate_test.go
- image/templates/helm/stackrox-secured-cluster/templates/_init.tpl.htpl
- image/templates/helm/stackrox-secured-cluster/internal/defaults/30-base-config.yaml.htpl
- central/cluster/datastore/datastore_impl_postgres_test.go
- pkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yaml
|
|
||
| ### Technical Changes | ||
|
|
||
| - OpenShift 3 support removed from Helm charts and roxctl manifest bundle generation. |
Contributor
There was a problem hiding this comment.
Include the new minimum supported OpenShift version in this changelog note.
Line 33 documents OCP3 removal, but it should also state the new floor (OpenShift 4.12) so upgrade impact is explicit.
Suggested changelog tweak
-- OpenShift 3 support removed from Helm charts and roxctl manifest bundle generation.
+- ROX-33465: OpenShift 3 support removed from Helm charts and roxctl manifest bundle generation.
+ Minimum supported OpenShift version is now 4.12.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 33, Update the changelog line "OpenShift 3 support
removed from Helm charts and roxctl manifest bundle generation." to explicitly
state the new minimum supported OpenShift version by appending "Minimum
supported OpenShift version: OpenShift 4.12" (or integrate that phrase into the
sentence) so the note reads clearly that OCP3 was removed and the new floor is
OpenShift 4.12.
added 28 commits
April 9, 2026 14:13
Adjust computation in central/graphql/resolvers/cluster_vulnerabilities_postgres_test.go
6305d0b to
86903ae
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Review commit-wise.
This changes removes OpenShift 3 support across the board.
Originally it started as just an improvement of the auto-sensing logic for OpenShift within the Helm chart, but since the suggested change -- checking for a different, CRD-based API -- is OpenShift 4 only, this also triggered the removal of OpenShift 3 from the code, which we don't support anymore since a long time. The oldest supported OpenShift version, which is currently in extended support, is 4.12.
Hence this change is also dependent on a change to helmtests. As soon as that PR is merged, I will inject the proper version here.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!