ROX-35435: Support differing versions of Central and SecuredCluster#246
ROX-35435: Support differing versions of Central and SecuredCluster#246vladbologa wants to merge 2 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/deployer/acs_images.go`:
- Around line 38-40: Update the image collection loop in the deployer flow
around OperatorInstances so single-operator configurations consistently use the
explicit Operator.Version override instead of mixing it with the resolved
instance version; alternatively, unify OperatorBundleImageForVersion and the OLM
Konflux environment-variable path on the same resolved version. Add a regression
test covering both Roxie.Version and Operator.Version being set, ensuring all
generated image references use one consistent operator version.
In `@internal/deployer/deploy_via_operator.go`:
- Around line 138-141: Update the early-return logic in the deployment flow
around isOperatorVersionCorrect so it also verifies the manager environment
matches instance.EnvVars, including component-specific reconciler flags and
Konflux/custom variables. Only return when both the image version and
environment are correct; otherwise reapply the desired Deployment using the
existing operator deployment path, preserving the split-mode flags defined by
the operator instance plan.
- Around line 110-125: The cleanup loop in deploy_via_operator.go around
AllOperatorNamespaces must attempt teardown for every undesired known namespace
without gating on operatorDeploymentExists or namespaceExists, allowing
teardownOperatorNonOLMInNamespace to remove namespaced and cluster-scoped
resources idempotently. In operator.go around the non-OLM cleanup flow, invoke
cleanup even when no Deployment is detected so partial resources are removed;
apply the change at both affected sites.
In `@internal/deployer/operator_instance.go`:
- Around line 78-82: Update the single-operator path in NeedsSplitOperators to
derive the operator version from the effective component version, honoring equal
Roxie and Operator overrides instead of using Roxie.Version directly. Preserve
the fallback behavior when no converted version is available, and extend
TestEffectiveVersions_BothOverridesSame_NoSplit with an assertion that the
deployed operator uses the override version.
In `@internal/deployer/operator.go`:
- Around line 575-598: Update the teardown flow around the kubectl deletion
calls and waitForNamespaceDeletion to propagate genuine deletion or timeout
errors instead of ignoring them or returning nil. Keep --ignore-not-found=true
for idempotent cleanup, but capture each runKubectl error and return it
immediately; likewise return the waitForNamespaceDeletion error before logging
success. Only log the success message and return nil after all cleanup steps
complete successfully.
- Around line 217-222: Update the EnvVars logging block in the operator
deployment flow to stop emitting environment values, since entries may contain
credentials or tokens. Preserve the existing count log and, if individual
entries are still needed, log only each variable’s name from envVarsToSortedList
without accessing or formatting ev["value"].
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: b2693e61-392e-405e-a4bb-6dd14e9c6436
📒 Files selected for processing (10)
cmd/deploy.gointernal/deployer/acs_images.gointernal/deployer/config.gointernal/deployer/deploy_via_operator.gointernal/deployer/deployer.gointernal/deployer/konflux.gointernal/deployer/konflux_test.gointernal/deployer/operator.gointernal/deployer/operator_instance.gointernal/deployer/operator_instance_test.go
| for _, instance := range config.OperatorInstances() { | ||
| add(fmt.Sprintf("%s/%s%s:%s", imageRegistry, operatorPrefix, "operator", instance.Version)) | ||
| add(OperatorBundleImageForVersion(instance.Version, config.Roxie.KonfluxImagesEnabled())) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep single-operator version resolution consistent.
For Roxie.Version="4.9.x" and Operator.Version="4.8.0", OperatorInstances() resolves 4.9.0, so this image list contains 4.9.0; OperatorBundleImage and the OLM Konflux env-var path use 4.8.0. Preserve the explicit operator-version override here or unify all paths on one resolved version, with a regression test for both fields being set.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/deployer/acs_images.go` around lines 38 - 40, Update the image
collection loop in the deployer flow around OperatorInstances so single-operator
configurations consistently use the explicit Operator.Version override instead
of mixing it with the resolved instance version; alternatively, unify
OperatorBundleImageForVersion and the OLM Konflux environment-variable path on
the same resolved version. Add a regression test covering both Roxie.Version and
Operator.Version being set, ensuring all generated image references use one
consistent operator version.
| for _, ns := range AllOperatorNamespaces { | ||
| if desiredNS[ns] { | ||
| continue | ||
| } | ||
| if !d.operatorDeploymentExists(ctx, ns) && !d.namespaceExists(ns) { | ||
| continue | ||
| } | ||
| d.logger.Infof("🔄 Removing operator from unwanted namespace %s...", ns) | ||
| instance := OperatorInstance{Namespace: ns} | ||
| switch ns { | ||
| case operatorNamespaceCentral: | ||
| instance.RoleNameSuffix = "central" | ||
| case operatorNamespaceSensor: | ||
| instance.RoleNameSuffix = "sensor" | ||
| } | ||
| if err := d.teardownOperatorNonOLMInNamespace(ctx, instance); err != nil { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Make operator cleanup unconditional and idempotent.
Both paths infer that no cleanup is required from missing namespaced Deployments, but partially failed or manually altered deployments may still leave namespaces or cluster-scoped RBAC.
internal/deployer/deploy_via_operator.go#L110-L125: attempt cleanup for every undesired known instance, including its ClusterRole and ClusterRoleBinding.internal/deployer/operator.go#L652-L664: run non-OLM cleanup even when no Deployment is detected so partial resources are removed.
📍 Affects 2 files
internal/deployer/deploy_via_operator.go#L110-L125(this comment)internal/deployer/operator.go#L652-L664
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/deployer/deploy_via_operator.go` around lines 110 - 125, The cleanup
loop in deploy_via_operator.go around AllOperatorNamespaces must attempt
teardown for every undesired known namespace without gating on
operatorDeploymentExists or namespaceExists, allowing
teardownOperatorNonOLMInNamespace to remove namespaced and cluster-scoped
resources idempotently. In operator.go around the non-OLM cleanup flow, invoke
cleanup even when no Deployment is detected so partial resources are removed;
apply the change at both affected sites.
| if exists { | ||
| if d.isOperatorVersionCorrect(ctx, instance) { | ||
| d.logger.Infof("✓ Operator already deployed with correct version in %s", instance.Namespace) | ||
| return nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Reconcile instance environment, not just its image version.
A matching image causes an early return even if the manager is missing the component-specific reconciler flags or has stale Konflux/custom variables. In split mode, missing flags can allow both operators to reconcile the same component.
Compare the manager environment with instance.EnvVars, or reapply the desired Deployment when it differs. The split plan in internal/deployer/operator_instance.go lines 77-110 relies on these flags.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/deployer/deploy_via_operator.go` around lines 138 - 141, Update the
early-return logic in the deployment flow around isOperatorVersionCorrect so it
also verifies the manager environment matches instance.EnvVars, including
component-specific reconciler flags and Konflux/custom variables. Only return
when both the image version and environment are correct; otherwise reapply the
desired Deployment using the existing operator deployment path, preserving the
split-mode flags defined by the operator instance plan.
| if !c.NeedsSplitOperators() { | ||
| version := helpers.ConvertMainTagToOperatorTag(c.Roxie.Version) | ||
| if version == "" { | ||
| version = c.Operator.Version | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Derive the single operator from the effective component version.
When both overrides are equal, split mode is disabled, but this branch still uses Roxie.Version. Thus global 4.9.0 plus two 4.8.0 overrides deploys a 4.9.0 operator, violating override precedence.
Proposed fix
if !c.NeedsSplitOperators() {
- version := helpers.ConvertMainTagToOperatorTag(c.Roxie.Version)
+ version := helpers.ConvertMainTagToOperatorTag(c.EffectiveCentralVersion())
if version == "" {
version = c.Operator.Version
}Add an operator-instance assertion to TestEffectiveVersions_BothOverridesSame_NoSplit to prevent regression.
📝 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.
| if !c.NeedsSplitOperators() { | |
| version := helpers.ConvertMainTagToOperatorTag(c.Roxie.Version) | |
| if version == "" { | |
| version = c.Operator.Version | |
| } | |
| if !c.NeedsSplitOperators() { | |
| version := helpers.ConvertMainTagToOperatorTag(c.EffectiveCentralVersion()) | |
| if version == "" { | |
| version = c.Operator.Version | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/deployer/operator_instance.go` around lines 78 - 82, Update the
single-operator path in NeedsSplitOperators to derive the operator version from
the effective component version, honoring equal Roxie and Operator overrides
instead of using Roxie.Version directly. Preserve the fallback behavior when no
converted version is available, and extend
TestEffectiveVersions_BothOverridesSame_NoSplit with an assertion that the
deployed operator uses the override version.
| if len(instance.EnvVars) > 0 { | ||
| d.logger.Dimf(" • Custom operator env vars: %d", len(instance.EnvVars)) | ||
| for _, envVar := range envVarsToSortedList(instance.EnvVars) { | ||
| ev := envVar.(map[string]any) | ||
| d.logger.Dimf(" %s=%s", ev["name"], ev["value"]) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log operator environment values.
Operator.EnvVars is arbitrary configuration and may contain credentials or tokens. Log names or only the count, not values.
Proposed fix
for _, envVar := range envVarsToSortedList(instance.EnvVars) {
ev := envVar.(map[string]any)
- d.logger.Dimf(" %s=%s", ev["name"], ev["value"])
+ d.logger.Dimf(" %s=<redacted>", ev["name"])
}📝 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.
| if len(instance.EnvVars) > 0 { | |
| d.logger.Dimf(" • Custom operator env vars: %d", len(instance.EnvVars)) | |
| for _, envVar := range envVarsToSortedList(instance.EnvVars) { | |
| ev := envVar.(map[string]any) | |
| d.logger.Dimf(" %s=%s", ev["name"], ev["value"]) | |
| } | |
| if len(instance.EnvVars) > 0 { | |
| d.logger.Dimf(" • Custom operator env vars: %d", len(instance.EnvVars)) | |
| for _, envVar := range envVarsToSortedList(instance.EnvVars) { | |
| ev := envVar.(map[string]any) | |
| d.logger.Dimf(" %s=<redacted>", ev["name"]) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/deployer/operator.go` around lines 217 - 222, Update the EnvVars
logging block in the operator deployment flow to stop emitting environment
values, since entries may contain credentials or tokens. Preserve the existing
count log and, if individual entries are still needed, log only each variable’s
name from envVarsToSortedList without accessing or formatting ev["value"].
| d.runKubectl(ctx, k8s.KubectlOptions{ | ||
| Args: []string{"delete", "namespace", operatorNamespace, "--wait=false"}, | ||
| Args: []string{"delete", "namespace", instance.Namespace, "--wait=false"}, | ||
| IgnoreErrors: true, | ||
| }) | ||
|
|
||
| // Delete cluster-scoped resources created by non-OLM flow. | ||
| clusterResources := []struct { | ||
| for _, resource := range []struct { | ||
| name string | ||
| kind string | ||
| }{ | ||
| {name: "rhacs-operator-manager-rolebinding", kind: "clusterrolebinding"}, | ||
| {name: "rhacs-operator-manager-role", kind: "clusterrole"}, | ||
| } | ||
| for _, resource := range clusterResources { | ||
| {name: instance.ClusterRoleBindingName(), kind: "clusterrolebinding"}, | ||
| {name: instance.ClusterRoleName(), kind: "clusterrole"}, | ||
| } { | ||
| d.runKubectl(ctx, k8s.KubectlOptions{ | ||
| Args: []string{"delete", resource.kind, resource.name, "--ignore-not-found=true"}, | ||
| IgnoreErrors: true, | ||
| }) | ||
| } | ||
|
|
||
| if err := d.waitForNamespaceDeletion(operatorNamespace); err != nil { | ||
| d.logger.Warningf("Namespace %s deletion incomplete: %v", operatorNamespace, err) | ||
| if err := d.waitForNamespaceDeletion(instance.Namespace); err != nil { | ||
| d.logger.Warningf("Namespace %s deletion incomplete: %v", instance.Namespace, err) | ||
| } | ||
|
|
||
| d.logger.Successf("✓ Non-OLM operator resources removed from %s", instance.Namespace) | ||
| return nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate teardown failures instead of reporting false success.
Every delete failure is ignored, namespace-deletion failure is reduced to a warning, and the function always returns nil. Callers therefore cannot detect incomplete cleanup despite explicitly handling its error.
Use --ignore-not-found=true, but return genuine deletion and timeout failures before logging success.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/deployer/operator.go` around lines 575 - 598, Update the teardown
flow around the kubectl deletion calls and waitForNamespaceDeletion to propagate
genuine deletion or timeout errors instead of ignoring them or returning nil.
Keep --ignore-not-found=true for idempotent cleanup, but capture each runKubectl
error and return it immediately; likewise return the waitForNamespaceDeletion
error before logging success. Only log the success message and return nil after
all cleanup steps complete successfully.
1 participant
Description
Roxie can deploy Central and SecuredCluster at different ACS versions on the same cluster (for compatibility testing), while keeping the single-version path unchanged.
Config and CLI
roxie.version/--tagremains the global default.central.version/--central-tagandsecuredCluster.version/--secured-cluster-tag.EffectiveCentralVersion()/EffectiveSecuredClusterVersion(). When they differ,NeedsSplitOperators()is true.--tag/roxie.version.Operators
When versions match (common case):
rhacs-operator-system(unchanged).When versions differ:
rhacs-operator-centralwithSECURED_CLUSTER_RECONCILER_ENABLED=false.rhacs-operator-sensorwithCENTRAL_RECONCILER_ENABLED=false.…-central/…-sensor).This is derived automatically from version mismatch; users do not configure reconciler toggles or operator namespaces.
Limitations
--olm/ OLM deploy are not supported (hard error).roxie deploy operator(operator-only) are not supported (hard error).Approaches considered
roxie deployinvocationsCENTRAL_LABEL_SELECTOR/ …)Manual testing on an infra cluster
4.11.1, SecuredCluster4.10.0. Confirmed dual operators, correct reconciler toggles, matching product images;rhacs-operator-systemunused.roxie teardownremoved both split operator namespaces and component CRs/workloads.--tag 4.11.1only. Single operator inrhacs-operator-system, no reconciler toggles, no split namespaces.4.11.1/ SecuredCluster4.10.0.rhacs-operator-systemremoved; split operators stood up correctly.Summary by CodeRabbit
New Features
Bug Fixes