ROX-35435: Support differing versions of Central and SecuredCluster - #246
ROX-35435: Support differing versions of Central and SecuredCluster#246vladbologa wants to merge 34 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThe deployment flow now supports independent Central and SecuredCluster operator versions. It plans per-instance deployments, generates instance-specific images and Konflux variables, validates mixed versions, and scopes OLM and non-OLM lifecycle operations to each operator instance. ChangesSplit Operator Support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DeployCommand
participant Config
participant ensureOperatorDeployed
participant OperatorDeployment
participant Kubernetes
DeployCommand->>Config: apply Central and SecuredCluster tags
Config-->>ensureOperatorDeployed: provide operator instances
ensureOperatorDeployed->>OperatorDeployment: reconcile each instance
OperatorDeployment->>Kubernetes: deploy resources in instance namespace
Kubernetes-->>OperatorDeployment: return deployment state
OperatorDeployment-->>ensureOperatorDeployed: report reconciliation result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://cdn.github.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
3c45e8d to
3776248
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/deployer/operator_instance_test.go (1)
74-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake override fixtures distinct from the global version.
These cases can pass if downstream planning accidentally uses
Roxie.Versionfor a component, because it equals one expected component version.
internal/deployer/operator_instance_test.go#L74-L109: use a third Roxie version and verify the sensor instance still usesSecuredCluster.Version.internal/deployer/operator_instance_test.go#L123-L139: setRoxie.Versionbelow the Central override so this case proves Central participates in newest-version selection.internal/deployer/operator_instance_test.go#L151-L165: use a third Roxie version and assert no global-version-only images are collected when both components override it.🤖 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_test.go` around lines 74 - 109, The operator instance tests use Roxie versions that can mask incorrect global-version handling. In internal/deployer/operator_instance_test.go:74-109, choose a distinct third Roxie version and keep asserting the sensor uses SecuredCluster.Version; at 123-139, set Roxie.Version below the Central override to verify Central drives newest-version selection; at 151-165, use a distinct Roxie version and assert no images for the global-only version are collected when both components override it.
🤖 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.
Nitpick comments:
In `@internal/deployer/operator_instance_test.go`:
- Around line 74-109: The operator instance tests use Roxie versions that can
mask incorrect global-version handling. In
internal/deployer/operator_instance_test.go:74-109, choose a distinct third
Roxie version and keep asserting the sensor uses SecuredCluster.Version; at
123-139, set Roxie.Version below the Central override to verify Central drives
newest-version selection; at 151-165, use a distinct Roxie version and assert no
images for the global-only version are collected when both components override
it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 57f74974-e5ca-442d-b25f-d4c9e9bd1f49
📒 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
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/deployer/config.go
- internal/deployer/konflux.go
- internal/deployer/konflux_test.go
- internal/deployer/operator_instance.go
- internal/deployer/deployer.go
- cmd/deploy.go
- internal/deployer/acs_images.go
- internal/deployer/operator.go
- internal/deployer/deploy_via_operator.go
There was a problem hiding this comment.
Pull request overview
Adds mixed-version deployment support so Roxie can deploy Central and SecuredCluster at different versions on the same cluster, while preserving the existing single-version behavior.
Changes:
- Introduces per-component version overrides (
central.version/securedCluster.version, plus CLI flags) and resolves effective versions from config. - Enables dual non-OLM operator deployments (separate namespaces + per-instance reconciler toggles + distinct cluster-scoped RBAC names) and improves teardown/mode switching logic.
- Updates image planning (including operator/bundle images) and deployment summaries to reflect effective component versions.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/deployer/operator.go | Refactors non-OLM operator deployment/teardown to support multiple operator instances and per-instance RBAC names; gates CRD apply to the newest planned operator version. |
| internal/deployer/operator_instance.go | Adds effective version resolution, mixed-version detection, operator instance planning, and “newest operator version” selection. |
| internal/deployer/operator_instance_test.go | Adds unit tests for effective versions, operator instance planning, newest-version selection, and mixed-version image planning. |
| internal/deployer/konflux.go | Refactors Konflux env-var population to support per-instance operator versions (merge-style API). |
| internal/deployer/konflux_test.go | Updates tests for new Konflux APIs and adds coverage for per-instance version merging. |
| internal/deployer/deployer.go | Updates deployment summaries to display effective Central/SecuredCluster versions rather than the global Roxie version. |
| internal/deployer/deploy_via_operator.go | Extends operator deployment logic to deploy/validate multiple non-OLM operator instances and handle transitions between modes/namespaces. |
| internal/deployer/config.go | Adds version override fields to Central and SecuredCluster config structs. |
| internal/deployer/acs_images.go | Updates image selection to include unique effective main versions plus per-instance operator/bundle images (deduped). |
| cmd/deploy.go | Adds CLI flags for per-component tags, validates unsupported mode combinations for mixed versions, and adjusts version-gated checks/Konflux behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3776248 to
d63b2af
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
mclasmeier
left a comment
mclasmeier
left a comment
There was a problem hiding this comment.
I am not through yet.
59b1ec9 to
114c96f
Compare
|
@coderabbitai can you do a full review again? |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmd/deploy.go`:
- Around line 462-482: Update the !hasSupport error returned in the
OperatorInstances loop to include the failing instance’s identity and
instance.Version, matching the detail already provided by the parse-error
branch. Preserve the existing constraint text while making the message
distinguish whether Central or SecuredCluster failed.
🪄 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: fc62d098-8e11-41fc-811c-166905f78563
📒 Files selected for processing (11)
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.gointernal/helpers/tag.go
| } | ||
|
|
||
| if deploySettings.HasMixedVersions() { | ||
| globalLogger.Dimf("Mixed versions detected (configured via --central-tag / --secured-cluster-tag or central.operator / securedCluster.operator)") |
There was a problem hiding this comment.
nit, I'd suggest to propagate the logger from here for consistency -- I think currently we are nowhere using the globalLogger directly.
| // This is why we use the operator version here when checking version constraints. | ||
| // Check every operator instance that will be deployed. | ||
| for _, instance := range deploySettings.OperatorInstances() { | ||
| hasSupport, err := stackroxversions.SupportsAdditionalPrinterColumns(instance.Version) |
There was a problem hiding this comment.
Hm, we are conflating the central&securedCluster versions here, right?
If central has earlyReadiness disabled and the central version supports it, but the secured Cluster version doesn't support it, we would still fail, right?
| } | ||
|
|
||
| // OperatorBundleImage returns the operator bundle image for a specific operator version. | ||
| func OperatorBundleImage(operatorVersion string, konflux bool) string { |
There was a problem hiding this comment.
I have been asking myself, why you changed this function from
func OperatorBundleImage(config Config) string { ... }to
func OperatorBundleImage(operatorVersion string, konflux bool) string { ... }the answer seems to be: you made it working for "OperatorInstances". This got me thinking:
-
Isn't it actually a bogus decision that we have
roxie.konfluxImagesnow? Seems to me that this should actually also be a field inOperatorConfig! -
The
OperatorInstance-- do we really need it? I have the impression we could simplify the code more if we were just using theOperatorConfiginstead -- that essentially describes how an operator instance should look like. -
The only thing that the Operator Instance currently provides, what the OperatorConfig currently doens't provide is the "suffix". But we could add that to
OperatorConfig-- in the usual case a user wouldn't have a reason to set it, but even if the user did set it, it wouldn't harm. And we gut drop theOperatorInstanceand get rid of the conversion, no?
WDYT?
There was a problem hiding this comment.
I think we should start with OperatorConfig, because as it right now it allows overriding some fields that don't make sense per individual operator (SkipDeployment, DeployViaOlm and also KonfluxImages if we add it to the struct).
E.g. now you can do things like:
central.operator.deployViaOlm = true
securedCluster.operator.deployViaOlm = false
So to address this, and also your concerns (btw at point 3, there's also the namespace that OperatorInstance adds), I'm thinking of something like this:
// OperatorInstanceConfig describes how to deploy a single operator instance.
type OperatorInstanceConfig struct {
Version string `yaml:"version,omitempty"`
EnvVars map[string]string `yaml:"envVars,omitempty"`
Namespace string `yaml:"namespace,omitempty"`
RoleNameSuffix string `yaml:"roleNameSuffix,omitempty"`
}
// OperatorConfig is the top-level operator configuration.
type OperatorConfig struct {
SkipDeployment *bool `yaml:"skipDeployment,omitempty"`
DeployViaOlm *bool `yaml:"deployViaOlm,omitempty"`
KonfluxImages bool `yaml:"konfluxImages,omitempty"`
OperatorInstanceConfig `yaml:",inline"`
}
type CentralConfig struct {
Operator OperatorInstanceConfig `yaml:"operator,omitempty"`
// ... other fields
}
type SecuredClusterConfig struct {
Operator OperatorInstanceConfig `yaml:"operator,omitempty"`
// ... other fields
}
type Config struct {
Roxie RoxieConfig `yaml:"roxie,omitempty"`
Operator OperatorConfig `yaml:"operator,omitempty"`
Central CentralConfig `yaml:"central,omitempty"`
SecuredCluster SecuredClusterConfig `yaml:"securedCluster,omitempty"`
}
This is backwards compatible (OperatorInstanceConfig is added inline to OperatorConfig), doesn't allow inconsistent configurations, and also OperatorInstance can go away, we just return []OperatorConfig.
And also OperatorBundleImage can take just a config.
There was a problem hiding this comment.
Why would konfluxImages not make sense for per-component operators?
I mean, I am not saying that this would be a particular useful combination but I guess we wouldn't need to add validation of some sort to exclude this case if it technically works fine:
e.g.
central with konflux operator
securedCluster with upstream operator
why special-case this?
There was a problem hiding this comment.
I'd have to refactor imagesForConfig in acs_images.go for that to work, but it's doable.
And what should we do with the existing onfig.Roxie.KonfluxImages? Keep it and propagate it to both configs, and it can be overridden?
There was a problem hiding this comment.
Regarding Roxie.KonfluxImages, since it is already used in my qa-e2e-tests PR, we better keep that working for now. But I only need it for the single operator use-case. I think "Keep it, propagate it to all operator configs (if changed -- to keep the sub-configs empty in the normal case), allow overriding" is sensible.
| } | ||
|
|
||
| // Detect current operator deployment mode | ||
| instances := d.preparedOperatorInstances() |
There was a problem hiding this comment.
Move down, because it is not needed yet?
| return d.ensureOperatorDeployedOLM(ctx) | ||
| } | ||
|
|
||
| // Switching from OLM to non-OLM in the system namespace. |
There was a problem hiding this comment.
Am I overlooking something? This comment seems wrong.
Only after L68, after looking at currentMode, we can deduce from that that we are switching from OLM to non-OLM? It could also be that currentMode == OperatorModeNonOLM, in which case the comment would be wrong.
| instance := OperatorInstance{Namespace: ns} | ||
| switch ns { | ||
| case operatorNamespaceCentral: | ||
| instance.RoleNameSuffix = "central" |
There was a problem hiding this comment.
I guess we should define constants for these and use those constants consistently.
d1c85ce to
879ae62
Compare
|
@CodeRabbit fresh review please |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
internal/deployer/deploy_via_operator.go (2)
249-252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude the namespace in the warning.
Two operator instances can now fail this lookup. The message does not identify which namespace failed.
♻️ Proposed fix
- d.logger.Warningf("Could not retrieve operator image: %v", err) + d.logger.Warningf("Could not retrieve operator image in %s: %v", instance.Namespace, err)🤖 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 249 - 252, Update the warning in the operator image lookup error path to include the relevant namespace alongside the error details, using the namespace variable available in the surrounding deploy flow. Keep the existing return behavior unchanged.
107-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the namespace-to-instance mapping into one helper.
The same namespace-to-
RoleNameSuffixmapping appears ininternal/deployer/operator.gointeardownOperatorNonOLMandteardownAllOperatorClusterRBAC. A single helper, for exampleoperatorInstanceForNamespace(ns string) OperatorInstanceConfig, would keep the three sites in sync.🤖 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 107 - 113, Extract the namespace-to-OperatorInstanceConfig mapping into a shared operatorInstanceForNamespace(ns string) helper, including the RoleNameSuffix assignments for operatorNamespaceCentral and operatorNamespaceSensor. Replace the duplicated mapping logic in the current deployment path and in teardownOperatorNonOLM and teardownAllOperatorClusterRBAC, preserving each site’s existing behavior.internal/deployer/operator.go (1)
602-606: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRole-suffix string literals bypass the existing constants in
internal/deployer/operator.go. Both teardown helpers hardcode"central"and"sensor"forRoleNameSuffix, whileinternal/deployer/deploy_via_operator.gousesroleNameSuffixCentralandroleNameSuffixSensor. If a constant value changes, these two sites target stale ClusterRole and ClusterRoleBinding names and leave cluster-scoped resources behind.
internal/deployer/operator.go#L602-L606: replace the literals in theteardownAllOperatorClusterRBACinstance list withroleNameSuffixCentralandroleNameSuffixSensor.internal/deployer/operator.go#L626-L632: replace the literals in theteardownOperatorNonOLMnamespace switch with the same constants.🤖 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 602 - 606, Replace the hardcoded "central" and "sensor" role suffixes with roleNameSuffixCentral and roleNameSuffixSensor in teardownAllOperatorClusterRBAC’s instance list and teardownOperatorNonOLM’s namespace switch in internal/deployer/operator.go at lines 602-606 and 626-632, respectively, so both teardown paths reuse the deployment constants.cmd/deploy_test.go (1)
46-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd table cases for
--central-tagand--secured-cluster-tag.The table covers
--tagin short and long form, but not the two new flags. Cases that assertcfg.Central.Operator.Versionandcfg.SecuredCluster.Operator.Version, plus one case that combines--tagwith a single component override, would lock in the precedence behavior.💚 Proposed test cases
{ name: "central-tag flag", args: []string{"--central-tag", "4.8.0"}, assert: func(t *testing.T, cfg deployer.Config) { assert.Equal(t, helpers.MainTag("4.8.0"), cfg.Central.Operator.Version, "Central.Operator.Version mismatch") }, }, { name: "secured-cluster-tag overrides tag for SecuredCluster only", args: []string{"--tag", "4.7.0", "--secured-cluster-tag", "4.8.0"}, assert: func(t *testing.T, cfg deployer.Config) { assert.Equal(t, helpers.MainTag("4.7.0"), cfg.Roxie.Version, "Roxie.Version mismatch") assert.Equal(t, helpers.MainTag("4.8.0"), cfg.SecuredCluster.Operator.Version, "SecuredCluster.Operator.Version mismatch") assert.Empty(t, cfg.Central.Operator.Version, "Central.Operator.Version should stay unset") }, },🤖 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 `@cmd/deploy_test.go` around lines 46 - 59, Add table-driven cases alongside the existing tag cases in the deploy configuration tests for --central-tag and --secured-cluster-tag, asserting the corresponding Central.Operator.Version and SecuredCluster.Operator.Version values. Include a combined --tag plus --secured-cluster-tag case to verify the component override takes precedence while Roxie keeps the base tag and Central.Operator.Version remains unset.
🤖 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 `@cmd/deploy.go`:
- Around line 499-508: Update the early-readiness validation around
checkEarlyReadinessSupport so each check runs only when its corresponding
component is included in components: gate the Central check with the Central
deployment condition and the SecuredCluster check with the SecuredCluster
deployment condition, while preserving the existing EarlyReadinessEnabled and
error-return behavior.
- Around line 489-497: The mixed-version check in
deploySettings.HasMixedVersions must compare normalized operator tags rather
than raw MainTag values. Update HasMixedVersions to compare
CentralVersion().ToOperatorTag() with SecuredClusterVersion().ToOperatorTag(),
preserving the existing callers and rejection behavior for genuinely different
versions.
In `@internal/deployer/config.go`:
- Around line 79-82: Update the comments for OperatorInstanceConfig,
CentralConfig.Operator, and SecuredClusterConfig.Operator to match
OperatorInstances(): per-component Version values are used in both single and
mixed modes, while mixed-mode per-component configuration fully replaces rather
than overrides or merges with the top-level Operator configuration.
In `@internal/deployer/operator.go`:
- Around line 647-649: Update the OLM branch in the operator teardown flow to
call teardownOperatorNonOLM after teardownOperatorOLM completes successfully,
ensuring split deployments in operatorNamespaceCentral and
operatorNamespaceSensor are removed while preserving the existing error
propagation behavior.
- Around line 171-179: Update the bundle image selection around
NewestOperatorVersion so it uses the planned operator instance matching that
effective version, including its KonfluxImages setting, rather than constructing
OperatorInstanceConfig from the global configuration. Reuse the matching
instance’s BundleImage() result, while preserving the existing fallback to
d.config.Operator.Version when no newest version is configured.
---
Nitpick comments:
In `@cmd/deploy_test.go`:
- Around line 46-59: Add table-driven cases alongside the existing tag cases in
the deploy configuration tests for --central-tag and --secured-cluster-tag,
asserting the corresponding Central.Operator.Version and
SecuredCluster.Operator.Version values. Include a combined --tag plus
--secured-cluster-tag case to verify the component override takes precedence
while Roxie keeps the base tag and Central.Operator.Version remains unset.
In `@internal/deployer/deploy_via_operator.go`:
- Around line 249-252: Update the warning in the operator image lookup error
path to include the relevant namespace alongside the error details, using the
namespace variable available in the surrounding deploy flow. Keep the existing
return behavior unchanged.
- Around line 107-113: Extract the namespace-to-OperatorInstanceConfig mapping
into a shared operatorInstanceForNamespace(ns string) helper, including the
RoleNameSuffix assignments for operatorNamespaceCentral and
operatorNamespaceSensor. Replace the duplicated mapping logic in the current
deployment path and in teardownOperatorNonOLM and
teardownAllOperatorClusterRBAC, preserving each site’s existing behavior.
In `@internal/deployer/operator.go`:
- Around line 602-606: Replace the hardcoded "central" and "sensor" role
suffixes with roleNameSuffixCentral and roleNameSuffixSensor in
teardownAllOperatorClusterRBAC’s instance list and teardownOperatorNonOLM’s
namespace switch in internal/deployer/operator.go at lines 602-606 and 626-632,
respectively, so both teardown paths reuse the deployment constants.
🪄 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: e7049817-2618-4a62-b008-6598e78d28e7
📒 Files selected for processing (12)
cmd/deploy.gocmd/deploy_test.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_instances.gointernal/deployer/operator_instances_test.gointernal/helpers/tag.go
| if t == "" { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
what about
| if t == "" { | |
| return "" | |
| } |
for conciseness?
| return fmt.Sprintf("%s/release-operator:%s", constants.DefaultRegistry, config.Operator.Version) | ||
| // KonfluxOperatorImage returns the Konflux-built operator image reference for an operator version. | ||
| func KonfluxOperatorImage(operatorVersion string) string { | ||
| return fmt.Sprintf("%s/release-operator:%s", constants.DefaultRegistry, operatorVersion) |
There was a problem hiding this comment.
FTR, we are duplicating this image ref construction.
Would be cool if we find a way around this, thereby consolidating the image ref construction for the konflux case in a single function.
There was a problem hiding this comment.
Good point, this is now done by OperatorImage(). Removed it in 3f5b6c0
| // PopulateKonfluxEnvVars adds RELATED_IMAGE_* entries to the instance's EnvVars | ||
| // for its version. It is a no-op when Konflux images are not enabled for the instance. | ||
| // Explicitly-provided env vars (e.g. from --operator-env) take precedence and are not overwritten. | ||
| func PopulateKonfluxEnvVars(instance *OperatorInstanceConfig) { |
There was a problem hiding this comment.
nit, function name doesn't match anymore. It's now more something like "ensureKonfluxEnvVarsIfNeeded" (not proposing exactly that).
| constants.DefaultRegistry, | ||
| imageSuffix, | ||
| config.Operator.Version, // Konflux built images use the "operator tag". | ||
| operatorTag, |
There was a problem hiding this comment.
I would personally keep the comment, because it is surprising to me that here we actually use the operator tag for main image, etc.
(side note: we will probably need to rework this when tackling the other ticket about configurable image registries)
| } | ||
|
|
||
| PopulateKonfluxEnvVars(config) | ||
| instance := &OperatorInstanceConfig{Version: "4.9.2", KonfluxImages: ptr.To(true)} |
There was a problem hiding this comment.
nit, In new Go code we can simply use new(value) and don't need to pull in ptr.To.
| if c.SecuredCluster.Operator.Version != "" { | ||
| return c.SecuredCluster.Operator.Version | ||
| } | ||
| return c.Roxie.Version |
| func (c *Config) OperatorInstances() []OperatorInstanceConfig { | ||
| if !c.HasMixedVersions() { | ||
| return []OperatorInstanceConfig{{ | ||
| Version: c.CentralVersion(), |
There was a problem hiding this comment.
wouldn't it be more natural to reference c.Operator.Version here?
(In particular, given that we access other fields in c.Operator below).
There was a problem hiding this comment.
I know, but it doesn't work in a particular edge case.
Let's say that Roxie.Version and Operator.Version are 4.10, but Central.Operator.Version and SecuredCluster.Operator.Version are both 4.11.
Then HasMixedVersions will return false, but c.Operator.Version will be 4.10 instead of 4.11.
There used to be a check to guard against this configuration, but you asked me to remove it in another comment.
| if instanceCfg.KonfluxImagesSet() { | ||
| return instanceCfg.KonfluxImages | ||
| } | ||
| return c.Roxie.KonfluxImages |
There was a problem hiding this comment.
The same thing I have said previously about Roxie.Version vs. Operator.Version also applies here: I see the former as the wrong choice which we ought to keep for backwards compatibility, but after initialization including the propagation of these values into their proper places (operator configurations), we shouldn't be accessing c.Roxie.KonfluxImages any longer.
| }} | ||
| } | ||
|
|
||
| centralEnvVars := make(map[string]string, len(c.Central.Operator.EnvVars)+1) |
There was a problem hiding this comment.
I think this be renamed to something like centralOperatorEnvVars, sensorOperatorEnvVars, because these are clearly not env vars for central/sensor.
| maps.Copy(sensorEnvVars, c.SecuredCluster.Operator.EnvVars) | ||
| sensorEnvVars[envCentralReconcilerEnabled] = "false" | ||
|
|
||
| return []OperatorInstanceConfig{ |
There was a problem hiding this comment.
Why are we assembling these structs from scratch here?
Also, aren't we throwing away the things like configurable rule name suffix, configurable namespace?
I mean, we already have OperatorInstanceConfig's in Central and SecuredCluster -- the thing that is missing IMHO is the merging, i.e. making the per-component configurations overwrite/extend the global operator config. We can use mergo.Merge for that without listing all fields individually.
Also, we can use mergo.Mergo for setting defaults for unset values (namespace, suffix).
There was a problem hiding this comment.
In an older version, the operator configs were additive, but you suggested that "replacing would be the more conservative thing to do for the initial roll out of this feature". So now they are doing that.
But your comment also made me realize that I didn't want to have configurable namespaces and suffixes. It happened when I removed the OperatorInstance struct in cde615467fe0d5fe7025f8c19bb8ba272c105592and moved them to OperatorInstanceConfig, and I didn't realize the implication.
See also #246 (comment)
| // Remove any leftover dual-operator namespaces before installing via OLM. | ||
| desired := []OperatorInstanceConfig{{Namespace: operatorNamespaceSystem}} |
There was a problem hiding this comment.
comment and code don't match?
Comments refers -- as I understand it -- to per-component-operators (plural) but we are only instructing deletion of the global namespace?
There was a problem hiding this comment.
desired means which namespace should exist, and the others can be deleted
So in this case rhacs-operator-system should exist, and rhacs-operator-central & rhacs-operator-sensor can be deleted.
This ofc doesn't work if the user sets customized namespaces, but that's not something I actually wanted to have (see #246 (comment))
| if d.config.Operator.DeployViaOlmEnabled() { | ||
| return d.ensureOperatorDeployedOLM(ctx) | ||
| } | ||
|
|
There was a problem hiding this comment.
nit, for me it is always helpful to add a comment like
| // Non-OLM case. | |
after the initial seperation of cases (L61-L63).
| instances := d.config.OperatorInstances() | ||
| if err := d.teardownStaleOperatorNamespaces(ctx, instances); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I think this is not what I would expect.
Say I deploy with one config, which does configures per-component operators.
Now I want to deploy with a new config, which does not do that.
This teardownStaleOperatorNamespaces() invocation would use the latter and not find anything to be torn down, no?
My expectation would be that it tears down the previously deployed per-component operators.
There was a problem hiding this comment.
I believe there are generally speaking two approaches here:
- Inspect the cluster and try to identify where an operator might be hiding (could be non-standard namespace, making it cumbersome).
- Use
LoadManifestSecret()and rely on that.
There was a problem hiding this comment.
The logic actually works the other way around:
instancescontains what we want to deploy (the desired state).teardownStaleOperatorNamespaces()then goes and checks existing namespaces, and removes those that are not needed, from the list of possible namespaces (rhacs-operator-system,rhacs-operator-central,rhacs-operator-sensor).
So if you were in single mode now switching to dual mode, teardownStaleOperatorNamespaces will detect the presence rhacs-operator-system, it will know that it's not needed, and it will remove it. Conversely, if you're in dual namespace mode and converting to single, teardownStaleOperatorNamespaces will determine that rhacs-operator-central and rhacs-operator-sensor are not needed and will remove them.
BTW this logic is covered by tests 4 and 5 of the manual tests (and to some extent also test 7).
There was a problem hiding this comment.
Ah, I understand your concern now. 🤦
I never intended to make the namespaces and the role name suffixes user configurable, but by adding them to OperatorInstanceConfig, I did just that. You wanted to remove the OperatorInstance struct, and I didn't fully realize the consequences.
So the teardown logic now doesn't work if the user sets custom namespaces, true. But TBH I'd prefer not to introduce logic for user customizable namespaces in role names in this PR, it's again scope creep.
Description
Roxie can now deploy Central and SecuredCluster at different 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.operator.version/--central-tagandsecuredCluster.operator.version/--secured-cluster-tag.CentralVersion()/SecuredClusterVersion(). When they differ,HasMixedVersions()is true.--tag/roxie.version.OperatorConfigblocks also allow setting custom env vars per operator instance.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 dual-operator namespaces and component CRs/workloads.--tag 4.11.1only. Single operator inrhacs-operator-system, no reconciler toggles, no dual-operator namespaces.4.11.1/ SecuredCluster4.10.0.rhacs-operator-systemremoved; dual operators stood up correctly.--tag 4.11.1. Both dual-operator namespaces removed; single operator deployed torhacs-operator-system.--konflux --central-tag 4.11.1 --secured-cluster-tag 4.10.0. Each operator received correct per-versionRELATED_IMAGE_*env vars and rewritten operator image. CRD apply skipped for older operator.--olm --tag 4.11.1. Dual-operator namespaces removed; OLM deployed operator torhacs-operator-systemvia CatalogSource/Subscription.Summary by CodeRabbit
New Features
Bug Fixes