ROX-33465: Remove OCP 3 support, make OCP auto-sensing more reliable#19921
Draft
mclasmeier wants to merge 35 commits intomasterfrom
Draft
ROX-33465: Remove OCP 3 support, make OCP auto-sensing more reliable#19921mclasmeier wants to merge 35 commits intomasterfrom
mclasmeier wants to merge 35 commits intomasterfrom
Conversation
added 30 commits
April 9, 2026 12:28
…openshift.io/v1 (openshift 4 only).
Adjust computation in central/graphql/resolvers/cluster_vulnerabilities_postgres_test.go
|
Skipping CI for Draft Pull Request. |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="image/templates/helm/shared/templates/_openshift.tpl" line_range="10-14" />
<code_context>
- If "true" is passed for $._rox.env.openshift the OpenShift version is detected based on the Kubernetes cluster version.
- If the Kubernetes version is not available (i.e. when using Helm template) auto-sensing falls back on OpenShift 3 to be
- backward compatible.
+ If "true" is passed for $._rox.env.openshift, this is unconditionally mapped to OpenShift version "4", because that is the only
+ major version we currently support.
*/}}
</code_context>
<issue_to_address>
**issue (bug_risk):** Align handling of string vs boolean values for `env.openshift=true` to avoid potential type issues.
The logic and docs treat `env.openshift` as a boolean, but Helm users often set `openshift: "true"` as a string. That will skip the `kindIs "bool"` branch and hit `int $env.openshift`, which will fail for the string "true". Either normalize string values (e.g., handle "true"/numeric strings before calling `int`) or add a guard that rejects non-numeric strings with a clear error message.
</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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (6)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR removes OpenShift 3 support across the codebase: deprecates the old enum, eliminates OpenShift 3-specific Helm/template and runtime logic, updates validation and generation tools to accept only OpenShift 4, and removes or adjusts tests and fixtures that referenced OpenShift 3. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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)
central/views/platformcve/view_test.go (1)
943-961:⚠️ Potential issue | 🟠 MajorThis drops coverage of the legacy platform bucket without removing it.
After removing all
OPENSHIFT_CLUSTERfixtures here, the suite can only ever observeGetOpenshiftClusterCount() == 0, but production still builds and returns a separateopenshift_cluster_countbucket (central/views/platformcve/view_impl.go:150-159,central/views/platformcve/db_response.go:1-25). That means upgraded databases with legacy cluster rows are no longer covered by tests even though the public view still exposes the old bucket.Either remove the legacy bucket in production as part of this PR, or keep one compatibility fixture/assertion that locks down how existing
OPENSHIFT_CLUSTERrows are handled.Also applies to: 984-987
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@central/views/platformcve/view_test.go` around lines 943 - 961, Tests removed all OPENSHIFT_CLUSTER fixtures causing coverage for the legacy openshift_cluster_count bucket to be lost while production still exposes that bucket (see GetOpenshiftClusterCount(), view_impl.go:150-159 and db_response.go legacy mapping). Fix by either (A) restoring a single legacy fixture with PlatformType set to storage.ClusterType_OPENSHIFT_CLUSTER (or ProviderType/labels as in legacy rows) in the test setup and add an assertion that GetOpenshiftClusterCount() returns the expected legacy count, or (B) remove the legacy openshift_cluster_count bucket code from view_impl.go and db_response.go in this PR so tests and production behavior match; choose one approach and update the tests to reflect that decision.
🧹 Nitpick comments (2)
image/embed_charts.go (1)
272-278: Return a specific error for deprecatedOPENSHIFT_CLUSTERvalues.The current fallback emits a generic “invalid cluster type” message. A specific OpenShift 3 message (and including the actual cluster type in default errors) will make failures easier to triage.
Proposed refactor
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() { - return i.scripts(values, osScriptsFileMap) - } - return nil, errors.Errorf("unable to create sensor bundle, invalid cluster type for cluster %s", - values.ClusterName) + switch values.ClusterType { + case storage.ClusterType_KUBERNETES_CLUSTER.String(): + return i.scripts(values, k8sScriptsFileMap) + case storage.ClusterType_OPENSHIFT4_CLUSTER.String(): + return i.scripts(values, osScriptsFileMap) + case storage.ClusterType_OPENSHIFT_CLUSTER.String(): + return nil, errors.Errorf("unable to create sensor bundle for cluster %q: OpenShift 3.x is not supported", values.ClusterName) + default: + return nil, errors.Errorf("unable to create sensor bundle for cluster %q: invalid cluster type %q", values.ClusterName, values.ClusterType) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@image/embed_charts.go` around lines 272 - 278, The current branch in the function that dispatches on values.ClusterType (checking storage.ClusterType_KUBERNETES_CLUSTER and storage.ClusterType_OPENSHIFT4_CLUSTER) returns a generic error; update this logic to detect the deprecated OpenShift v3 token (storage.ClusterType_OPENSHIFT_CLUSTER.String()) and return a clear, specific error message like "OpenShift 3 cluster types are no longer supported" when that value is seen, and change the final fallback error to include the actual values.ClusterType (e.g., "unable to create sensor bundle, invalid cluster type: <actual-type> for cluster <ClusterName>") so failures are triageable; keep existing calls to i.scripts(values, ...) for the supported cluster types.central/complianceoperator/v2/integration/datastore/datastore_impl_test.go (1)
260-260: Replace magic enum literal with the generated enum constant.
Using a raw5for cluster type makes this test fragile if enum values ever shift.Proposed refactor
- _, 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, int32(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 currently inserts a magic literal "5" for the cluster type in the s.db.DB.Exec call; replace that raw integer with the generated enum constant for the cluster type (instead of 5) coming from the package that defines cluster types (e.g., use the appropriate ClusterType enum value such as clusterpb.ClusterType_<NAME> or provisioner.ClusterType_<NAME>), update imports if necessary, and ensure the Exec call uses testconsts.Cluster2, "cluster2", 2, <ENUM_CONSTANT> so the test won't break if enum numeric values change.
🤖 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 32-40: The template currently coerces string inputs with int which
turns invalid strings into 0; add a guard that rejects non-numeric string values
before calling set(..., int ...). Specifically, before the block that does set
$env "openshift" (int $env.openshift), check if kindIs "string" $env.openshift
and if not (regexMatch "^[0-9]+$" $env.openshift) then call include "srox.fail"
with a clear message referencing the invalid $env.openshift value; keep the
existing boolean and numeric flows intact so only non-numeric strings are
rejected early.
In
`@image/templates/helm/stackrox-secured-cluster/internal/compatibility-translation.yaml`:
- Around line 13-15: The template currently treats only "OPENSHIFT4_CLUSTER" as
OpenShift by evaluating {{ eq .rawValue "OPENSHIFT4_CLUSTER" }}, which silently
maps legacy "OPENSHIFT_CLUSTER" to false; update the compatibility rule to
preserve legacy semantics by checking for both values (e.g., treat .rawValue ==
"OPENSHIFT4_CLUSTER" OR .rawValue == "OPENSHIFT_CLUSTER" as true) or add an
explicit pre-check that rejects/normalizes the deprecated "OPENSHIFT_CLUSTER"
value before this translation; modify the template condition that references
.rawValue and the string "OPENSHIFT4_CLUSTER" accordingly so OpenShift-specific
resources continue to render (or fail fast) for callers still emitting the
legacy value (see callers like roxctl/scanner/clustertype/wrapper.go and
central/telemetry/centralclient/client.go).
---
Outside diff comments:
In `@central/views/platformcve/view_test.go`:
- Around line 943-961: Tests removed all OPENSHIFT_CLUSTER fixtures causing
coverage for the legacy openshift_cluster_count bucket to be lost while
production still exposes that bucket (see GetOpenshiftClusterCount(),
view_impl.go:150-159 and db_response.go legacy mapping). Fix by either (A)
restoring a single legacy fixture with PlatformType set to
storage.ClusterType_OPENSHIFT_CLUSTER (or ProviderType/labels as in legacy rows)
in the test setup and add an assertion that GetOpenshiftClusterCount() returns
the expected legacy count, or (B) remove the legacy openshift_cluster_count
bucket code from view_impl.go and db_response.go in this PR so tests and
production behavior match; choose one approach and update the tests to reflect
that decision.
---
Nitpick comments:
In `@central/complianceoperator/v2/integration/datastore/datastore_impl_test.go`:
- Line 260: The test currently inserts a magic literal "5" for the cluster type
in the s.db.DB.Exec call; replace that raw integer with the generated enum
constant for the cluster type (instead of 5) coming from the package that
defines cluster types (e.g., use the appropriate ClusterType enum value such as
clusterpb.ClusterType_<NAME> or provisioner.ClusterType_<NAME>), update imports
if necessary, and ensure the Exec call uses testconsts.Cluster2, "cluster2", 2,
<ENUM_CONSTANT> so the test won't break if enum numeric values change.
In `@image/embed_charts.go`:
- Around line 272-278: The current branch in the function that dispatches on
values.ClusterType (checking storage.ClusterType_KUBERNETES_CLUSTER and
storage.ClusterType_OPENSHIFT4_CLUSTER) returns a generic error; update this
logic to detect the deprecated OpenShift v3 token
(storage.ClusterType_OPENSHIFT_CLUSTER.String()) and return a clear, specific
error message like "OpenShift 3 cluster types are no longer supported" when that
value is seen, and change the final fallback error to include the actual
values.ClusterType (e.g., "unable to create sensor bundle, invalid cluster type:
<actual-type> for cluster <ClusterName>") so failures are triageable; keep
existing calls to i.scripts(values, ...) for the supported cluster types.
🪄 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: 308b03ae-a623-4676-ad6e-818afa56369c
⛔ 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/templates/_init.tpl.htpl
- central/graphql/resolvers/test_utils.go
- central/cluster/datastore/datastore_impl_postgres_test.go
- pkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yaml
- roxctl/central/generate/generate_test.go
- image/templates/helm/stackrox-secured-cluster/values.yaml.htpl
- image/templates/helm/stackrox-secured-cluster/internal/defaults/30-base-config.yaml.htpl
image/templates/helm/stackrox-secured-cluster/internal/compatibility-translation.yaml
Show resolved
Hide resolved
Contributor
🚀 Build Images ReadyImages are ready for commit 49c1168. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-642-g49c11684e9 |
Contributor
Author
|
@coderabbitio, review again |
Contributor
|
✅ Actions performedReview triggered.
|
e2c71f2 to
49c1168
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19921 +/- ##
==========================================
- Coverage 49.61% 49.60% -0.01%
==========================================
Files 2765 2765
Lines 208541 208510 -31
==========================================
- Hits 103464 103440 -24
+ Misses 97401 97395 -6
+ Partials 7676 7675 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!