Add NetworkPolicy support to MCE operator - #3550
Merged
openshift-merge-bot[bot] merged 24 commits intoJul 21, 2026
Merged
Conversation
Implements create-once NetworkPolicy pattern for MCE components. Changes: - Add networkPolicies.enabled field to MultiClusterEngine CRD (default: true) - Add NetworkPoliciesConfig type to MCE spec - Implement ensureNetworkPolicies reconciliation logic - Add NetworkPoliciesValue to rendering Global values - Add networkPolicies.enabled to all component values.yaml files - Skip NetworkPolicy resources in component ensure/delete loops Create-once pattern: - MCE creates initial NetworkPolicy if missing - Operand teams adopt and manage policies - MCE deletes all MCE-created policies when globally disabled - No continuous reconciliation after creation Tracks ownership via installer labels: - installer.name = mce.Name - installer.namespace = mce.Namespace Signed-off-by: dislbenn <dbennett@redhat.com>
|
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:
WalkthroughAdds NetworkPolicies configuration to the MultiClusterEngine API and CRD, propagates it into rendered chart values, reconciles NetworkPolicy resources separately, and updates manifests, RBAC, and test scheme registration. ChangesNetworkPolicy configuration and reconciliation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/toggle_components.go (1)
1610-1626: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ensureHyperShiftapply loop is missing the NetworkPolicy skip check.Every other component's apply loop (and this same component's own delete loop,
ensureNoHyperShiftat lines 1691-1696) got thetemplate.GetKind() == "NetworkPolicy" { continue }guard — this one didn't. Currently harmless since no NetworkPolicy chart templates exist yet, but once the follow-up component PRs add one to the hypershift chart, this loop will apply/continuously reconcile it directly, defeating the create-once handoff toensureNetworkPolicies.🐛 Proposed fix
missingCRDErrorOccured := false for _, template := range templates { + // Skip NetworkPolicy resources - they are managed by ensureNetworkPolicies with create-once pattern + if template.GetKind() == "NetworkPolicy" { + continue + } + applyReleaseVersionAnnotation(template) result, err := r.applyTemplate(ctx, mce, template)🤖 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 `@controllers/toggle_components.go` around lines 1610 - 1626, The ensureHyperShift apply loop is missing the NetworkPolicy skip guard, so it will reconcile those templates directly instead of handing them off to ensureNetworkPolicies. Add the same template.GetKind() == "NetworkPolicy" continue check used in the other component apply loops and in ensureNoHyperShift, placing it inside the templates iteration in ensureHyperShift before applyReleaseVersionAnnotation/applyTemplate. Refer to the ensureHyperShift loop and template.GetKind() handling to keep the behavior consistent across apply and delete paths.
🧹 Nitpick comments (2)
api/v1/multiclusterengine_types.go (1)
72-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a shared helper for the "enabled by default" logic.
The nil-check-then-default-to-true pattern for
NetworkPolicies.Enabledis duplicated in bothpkg/rendering/renderer.go(injectValuesOverrides) andcontrollers/networkpolicy.go(ensureNetworkPolicies), per the provided context snippets. SinceNetworkPoliciesis a pointer and CRD-level defaulting won't populate it when the parent field is entirely omitted from the request, both consumers must independently replicate this fallback. Consider adding a method here, e.g.func (mce *MultiClusterEngine) NetworkPoliciesEnabled() bool, so the two call sites stay in sync if the default or semantics ever change.♻️ Proposed helper
+// NetworkPoliciesEnabled returns whether NetworkPolicies should be deployed, +// defaulting to true when unset (mirrors CRD default, which is not applied +// when the parent object is entirely omitted). +func (mce *MultiClusterEngine) NetworkPoliciesEnabled() bool { + if mce.Spec.NetworkPolicies == nil { + return true + } + return mce.Spec.NetworkPolicies.Enabled +}🤖 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 `@api/v1/multiclusterengine_types.go` around lines 72 - 84, The nil-check-and-default-to-true logic for NetworkPolicies.Enabled is duplicated across injectValuesOverrides and ensureNetworkPolicies, so add a shared helper on MultiClusterEngine to centralize the defaulting behavior. Implement a method such as NetworkPoliciesEnabled() on the MultiClusterEngine type in multiclusterengine_types.go that returns true when NetworkPolicies is nil or when Enabled is true, then update both call sites to use that method so the default stays consistent in one place.controllers/networkpolicy.go (1)
100-111: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider tolerating
AlreadyExistson Create.Get-then-Create isn't atomic; a concurrent create (e.g., two reconciles racing) would surface as a hard error instead of being treated as "already created."
♻️ Optional hardening
if err := r.Client.Create(ctx, npTemplate); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to create NetworkPolicy %s/%s: %w", npTemplate.GetNamespace(), npTemplate.GetName(), err) + if !errors.IsAlreadyExists(err) { + return ctrl.Result{}, fmt.Errorf("failed to create NetworkPolicy %s/%s: %w", npTemplate.GetNamespace(), npTemplate.GetName(), 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 `@controllers/networkpolicy.go` around lines 100 - 111, The create path in the NetworkPolicy reconcile logic should tolerate a race where another reconcile creates the object first. In the branch that handles errors.IsNotFound in the NetworkPolicy handling flow, update the r.Client.Create call to treat an AlreadyExists result as a benign outcome instead of returning a hard error, while keeping the existing success logging for the create-once pattern. Use the existing identifiers errors.IsNotFound, r.Client.Create, and the NetworkPolicy template object npTemplate to locate the code.
🤖 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 `@controllers/networkpolicy.go`:
- Around line 100-105: The create-once path in NetworkPolicy reconciliation is
missing the release-version annotation step before creating the object. In the
not-found branch of the NetworkPolicy handler, call
applyReleaseVersionAnnotation on npTemplate before r.Client.Create, matching the
sibling apply paths in toggle_components.go and the intended behavior described
by the header comment. Keep the change localized to the create-once block so the
created NetworkPolicy always carries the release-version annotation.
- Around line 61-90: The ensureNetworkPolicies flow is missing the externally
managed component guard, so enabled components that are managed outside MCE
still get rendered and applied. Add the same isComponentExternallyManaged check
used in the other ensure paths inside ensureNetworkPolicies, before calling
fetchChartOrCRDPath and renderer.RenderChart, and skip any component flagged
externally managed so NetworkPolicy creation is left alone.
- Around line 1-17: The NetworkPolicy RBAC rule is missing list permission,
which causes `ensureNetworkPolicies` to fail when it calls `Client.List` during
the delete flow. Update the `networkpolicies` RBAC entry to include `list`
alongside the existing verbs, and keep the change aligned with the
`ensureNetworkPolicies` controller logic so list operations are permitted for
cleanup and reconciliation.
- Around line 19-28: The disable path in ensureNetworkPolicies is selecting
NetworkPolicies with the wrong namespace value because mce.Namespace is empty
for this cluster-scoped resource. Update the selector logic in
ensureNetworkPolicies to use the same namespace value used when rendering the
NetworkPolicies, so the labels and namespace match and existing resources can be
found and deleted when disabled.
---
Outside diff comments:
In `@controllers/toggle_components.go`:
- Around line 1610-1626: The ensureHyperShift apply loop is missing the
NetworkPolicy skip guard, so it will reconcile those templates directly instead
of handing them off to ensureNetworkPolicies. Add the same template.GetKind() ==
"NetworkPolicy" continue check used in the other component apply loops and in
ensureNoHyperShift, placing it inside the templates iteration in
ensureHyperShift before applyReleaseVersionAnnotation/applyTemplate. Refer to
the ensureHyperShift loop and template.GetKind() handling to keep the behavior
consistent across apply and delete paths.
---
Nitpick comments:
In `@api/v1/multiclusterengine_types.go`:
- Around line 72-84: The nil-check-and-default-to-true logic for
NetworkPolicies.Enabled is duplicated across injectValuesOverrides and
ensureNetworkPolicies, so add a shared helper on MultiClusterEngine to
centralize the defaulting behavior. Implement a method such as
NetworkPoliciesEnabled() on the MultiClusterEngine type in
multiclusterengine_types.go that returns true when NetworkPolicies is nil or
when Enabled is true, then update both call sites to use that method so the
default stays consistent in one place.
In `@controllers/networkpolicy.go`:
- Around line 100-111: The create path in the NetworkPolicy reconcile logic
should tolerate a race where another reconcile creates the object first. In the
branch that handles errors.IsNotFound in the NetworkPolicy handling flow, update
the r.Client.Create call to treat an AlreadyExists result as a benign outcome
instead of returning a hard error, while keeping the existing success logging
for the create-once pattern. Use the existing identifiers errors.IsNotFound,
r.Client.Create, and the NetworkPolicy template object npTemplate to locate the
code.
🪄 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: Repository: stolostron/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e7591a4a-fa67-49e1-9d5d-56e2ef648525
📒 Files selected for processing (28)
api/v1/multiclusterengine_types.goconfig/crd/bases/multicluster.openshift.io_multiclusterengines.yamlcontrollers/backplaneconfig_controller.gocontrollers/networkpolicy.gocontrollers/toggle_components.gopkg/rendering/renderer.gopkg/templates/charts/toggle/assisted-service/values.yamlpkg/templates/charts/toggle/cluster-api-k8s/values.yamlpkg/templates/charts/toggle/cluster-api-provider-aws/values.yamlpkg/templates/charts/toggle/cluster-api-provider-azure-k8s/values.yamlpkg/templates/charts/toggle/cluster-api-provider-azure/values.yamlpkg/templates/charts/toggle/cluster-api-provider-metal3-k8s/values.yamlpkg/templates/charts/toggle/cluster-api-provider-metal3/values.yamlpkg/templates/charts/toggle/cluster-api-provider-openshift-assisted-k8s/values.yamlpkg/templates/charts/toggle/cluster-api-provider-openshift-assisted/values.yamlpkg/templates/charts/toggle/cluster-api/values.yamlpkg/templates/charts/toggle/cluster-lifecycle/values.yamlpkg/templates/charts/toggle/cluster-manager/values.yamlpkg/templates/charts/toggle/cluster-permission/values.yamlpkg/templates/charts/toggle/cluster-proxy-addon/values.yamlpkg/templates/charts/toggle/console-mce/values.yamlpkg/templates/charts/toggle/discovery-operator/values.yamlpkg/templates/charts/toggle/hive-operator/values.yamlpkg/templates/charts/toggle/hypershift/values.yamlpkg/templates/charts/toggle/image-based-install-operator/values.yamlpkg/templates/charts/toggle/maestro/values.yamlpkg/templates/charts/toggle/managed-serviceaccount/values.yamlpkg/templates/charts/toggle/server-foundation/values.yaml
- Add list verb to networkpolicies RBAC to allow List() calls during cleanup - Fix namespace selector to use mce.Spec.TargetNamespace instead of mce.Namespace (cluster-scoped resource has empty namespace) - Add isComponentExternallyManaged guard to skip externally managed components - Apply release-version annotation before Create to match sibling apply paths Signed-off-by: dislbenn <dbennett@redhat.com> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
dislbenn
force-pushed
the
networkpolicy-support
branch
from
July 7, 2026 16:08
a41bdb2 to
13d9d36
Compare
Generated from RBAC marker changes in controllers/backplaneconfig_controller.go Signed-off-by: dislbenn <dbennett@redhat.com> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Remove required constraint on message, reason, status, type fields in conditions array Signed-off-by: dislbenn <dbennett@redhat.com> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
dislbenn
force-pushed
the
networkpolicy-support
branch
from
July 7, 2026 16:13
c75d12f to
3b30d9b
Compare
NetworkPolicy controller added in earlier commit but test suite missing networkingv1 scheme registration. Tests fail when ensureNetworkPolicies() called because client can't handle NetworkPolicy type. Add networkingv1.AddToScheme to both test client scheme and manager scheme in suite_test.go. Signed-off-by: dislbenn <disaiah.bennett@ibm.com> Signed-off-by: dislbenn <dbennett@redhat.com>
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 `@bundle/manifests/multicluster.openshift.io_multiclusterengines.yaml`:
- Around line 79-91: The NetworkPolicies API only defaults the nested enabled
field, so spec.networkPolicies remains unset when omitted and the CRD won’t
materialize the parent object. Update the source type/marker for the
NetworkPolicies field so the parent spec.networkPolicies defaults to an object
with enabled=true, then regenerate the CRD so the default is preserved by the
manifest and stays in sync with the renderer.
🪄 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: Repository: stolostron/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0192f30a-704b-4911-bedf-4748364fd27e
⛔ Files ignored due to path filters (1)
api/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (7)
bundle/manifests/multicluster-engine.clusterserviceversion.yamlbundle/manifests/multicluster.openshift.io_multiclusterengines.yamlconfig/manifests/bases/multicluster-engine.clusterserviceversion.yamlconfig/rbac/role.yamlcontrollers/backplaneconfig_controller.gocontrollers/networkpolicy.gocontrollers/suite_test.go
✅ Files skipped from review due to trivial changes (1)
- config/manifests/bases/multicluster-engine.clusterserviceversion.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- controllers/backplaneconfig_controller.go
- controllers/networkpolicy.go
Collaborator
Author
|
/test test-unit |
ensureNoHyperShift delete loop already skips NetworkPolicy (managed by ensureNetworkPolicies controller), but ensureHyperShift apply loop was still applying NetworkPolicy templates. This caused hypershift NetworkPolicy to be created outside the ensureNetworkPolicies flow, bypassing the create-once pattern and proper label management. Add NetworkPolicy skip to hypershift apply loop to match delete loop. Signed-off-by: dislbenn <disaiah.bennett@ibm.com> Signed-off-by: dislbenn <dbennett@redhat.com>
Renderer accessed chart.Name() when chart is nil after load error, causing panic. Also add chartPath validation in NetworkPolicy controller before calling RenderChart. Fixes panic: runtime error: invalid memory address or nil pointer dereference Signed-off-by: dislbenn <disaiah.bennett@ibm.com> Signed-off-by: dislbenn <dbennett@redhat.com>
Split function signatures, error messages, and log calls across multiple lines to comply with SonarCloud line length requirements. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: dislbenn <dbennett@redhat.com>
dislbenn
force-pushed
the
networkpolicy-support
branch
from
July 9, 2026 13:52
c1b4b4c to
9bd4dee
Compare
Changed NetworkPolicies from pointer to value type and removed omitempty to ensure the security feature is always visible to customers. The field now appears in every MCE spec with its default value (enabled: true). Changes: - NetworkPolicies: *NetworkPoliciesConfig -> NetworkPoliciesConfig - Removed omitempty from networkPolicies field - Removed omitempty from enabled subfield - Simplified nil-checks in controller and renderer - CRD now requires networkPolicies field with enabled defaulting to true This makes the security feature discoverable and explicit rather than hidden when using defaults. Signed-off-by: dislbenn <dbennett@redhat.com>
Add comprehensive tests for NetworkPolicy deletion when disabled: - Delete all MCE-created NetworkPolicies when globally disabled - Delete multiple NetworkPolicies - Preserve NetworkPolicies from other MCE instances - Handle non-existent NetworkPolicies gracefully - Verify NetworkPolicies persist when enabled Tests verify the label-based selector correctly identifies and deletes only NetworkPolicies created by this MCE instance. Signed-off-by: dislbenn <dbennett@redhat.com>
Signed-off-by: dislbenn <dbennett@redhat.com>
- Add enabled-path tests: skip disabled components, skip externally managed components, create-once pattern with real hypershift chart - Add renderer tests: NetworkPoliciesValue injection (enabled/disabled), invalid chart path error handling - Add Helm conditional to hypershift NP template so it only renders when networkPolicies.enabled=true Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: dislbenn <dbennett@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: dislbenn <dbennett@redhat.com>
Test ensureNetworkPolicies with nonexistent target namespace to trigger Create failure when chart renders a NetworkPolicy template. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: dislbenn <dbennett@redhat.com>
Set NetworkPolicies.Enabled=true in all test MCE objects so the hypershift NP template renders and toggle_components skip logic is exercised. Fixes coverage regression from adding Helm conditional. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: dislbenn <dbennett@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: dislbenn <dbennett@redhat.com>
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dislbenn, ngraham20 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
controllers/networkpolicy.go (3)
104-120: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCreate-once path doesn't handle a concurrent
AlreadyExistson Create.Between the
Get(NotFound) andCreate, another reconcile could create the same NetworkPolicy, causingCreateto fail withAlreadyExists; the current code returns a hard error instead of treating that as success (mirroring the gracefulIsNotFoundhandling already used on the delete path).🛡️ Suggested fix
if errors.IsNotFound(err) { // Create NetworkPolicy - create-once pattern applyReleaseVersionAnnotation(npTemplate) - if err := r.Client.Create(ctx, npTemplate); err != nil { + if err := r.Client.Create(ctx, npTemplate); err != nil && !errors.IsAlreadyExists(err) { return ctrl.Result{}, fmt.Errorf( "failed to create NetworkPolicy %s/%s: %w", npTemplate.GetNamespace(), npTemplate.GetName(), 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 `@controllers/networkpolicy.go` around lines 104 - 120, Update the create-once branch in the NetworkPolicy reconcile flow to treat an AlreadyExists error from r.Client.Create as a successful concurrent creation. Preserve the existing wrapped error return for other create failures and continue the normal reconcile flow when the resource already exists.
62-66: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftDisabling a single component never cleans up its NetworkPolicy.
The comment says cleanup "relies on global deletion above if needed," but the global deletion branch only fires when
mce.Spec.NetworkPolicies.Enabledis globally false — not per component. Every other component in this codebase has a matchingensureNoXxxcleanup path invoked when that component is disabled (seeensureNoClusterManager,ensureNoHive, etc. exercised incontrollers/backplaneconfig_controller_test.go), but NetworkPolicies has no equivalent, so a policy created while a component was enabled is orphaned indefinitely once that component alone is disabled.🤖 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 `@controllers/networkpolicy.go` around lines 62 - 66, The per-component disabled branch in the NetworkPolicy reconciliation flow must remove policies previously owned by that component instead of simply continuing. Add or reuse an ensureNoNetworkPolicy-style cleanup path alongside the existing global deletion handling, invoke it when mce.Enabled(component) is false, and preserve the current creation/update flow for enabled components.
81-87: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winChart rendering errors are silently swallowed for a security-relevant resource.
Any renderer error — whether "no NetworkPolicy template" (benign) or a genuine template/values bug — is treated identically and only logged at
V(2)(debug). A real rendering defect for a given component would leave it permanently without its NetworkPolicy with no operator-visible signal.🛡️ Suggested direction
templates, errs := renderer.RenderChart(chartPath, mce, r.CacheSpec.ImageOverrides, r.CacheSpec.TemplateOverrides) if len(errs) > 0 { - // Rendering errors are non-fatal - component may not have NetworkPolicy template yet - log.V(2).Info("Chart rendering had errors", "component", component, "errors", len(errs)) + // Surface at a visible level so real template/values regressions aren't missed + log.Error(errs[0], "Chart rendering had errors for component", "component", component, "errorCount", len(errs)) continue }🤖 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 `@controllers/networkpolicy.go` around lines 81 - 87, Update the error handling after renderer.RenderChart in the NetworkPolicy reconciliation flow to distinguish the benign absence of a NetworkPolicy template from genuine rendering failures. Preserve the non-fatal behavior for components without that template, but elevate real template or values errors to an operator-visible warning or error log with the component and renderer details before continuing.controllers/networkpolicy_test.go (1)
251-297: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftTests can pass without exercising the behavior they claim to verify.
Both tests gate their real assertions behind a conditional derived from uncertain chart-rendering/creation outcomes in the test environment:
- Line 271-276:
if err == nil { ... verify create-once ... }— if theGetdoesn't find the NetworkPolicy (e.g. chart didn't render it), the test silently no-ops and still passes, so "should create NetworkPolicy from rendered template" is never actually asserted.- Line 294-296:
if err != nil { Expect(err.Error())... }— ifensureNetworkPoliciesdoesn't error, the test asserts nothing and passes, so "should return error when NetworkPolicy creation fails" can pass without ever hitting a failure.This undermines the coverage these tests are meant to add (per the PR's "creation-error" and "create-once" test additions). Consider asserting unconditionally once the test fixture reliably renders the hypershift NetworkPolicy template (e.g., ensure
CacheSpechas whatever chart/image overrides the hypershift chart needs in this env), so these tests fail loudly instead of silently passing.🤖 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 `@controllers/networkpolicy_test.go` around lines 251 - 297, Make the NetworkPolicy tests assert their intended outcomes unconditionally: in “should create NetworkPolicy from rendered template and skip on second call,” require the initial Get to succeed and always verify the second ensureNetworkPolicies call; in “should return error when NetworkPolicy creation fails,” require ensureNetworkPolicies to return an error and validate its message. Update the test fixture, including CacheSpec or required chart/image overrides, so the HyperShift NetworkPolicy renders reliably instead of conditionally bypassing assertions.
🤖 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 `@api/v1/multiclusterengine_types.go`:
- Around line 73-76: Update the NetworkPolicies handling around the
NetworkPolicies field in the MCE spec to preserve the legacy default of enabled
for objects that predate spec.networkPolicies. Add a parent-level default or
equivalent migration so an absent field does not deserialize as Enabled=false
and trigger deletion during reconciliation.
In `@pkg/rendering/renderer_test.go`:
- Around line 677-682: Replace the direct os.Setenv and deferred os.Unsetenv
calls at pkg/rendering/renderer_test.go lines 677-682 and 740-741 with t.Setenv
calls in the affected tests, preserving the existing environment variable names
and values; t.Setenv should handle cleanup automatically at test completion.
---
Nitpick comments:
In `@controllers/networkpolicy_test.go`:
- Around line 251-297: Make the NetworkPolicy tests assert their intended
outcomes unconditionally: in “should create NetworkPolicy from rendered template
and skip on second call,” require the initial Get to succeed and always verify
the second ensureNetworkPolicies call; in “should return error when
NetworkPolicy creation fails,” require ensureNetworkPolicies to return an error
and validate its message. Update the test fixture, including CacheSpec or
required chart/image overrides, so the HyperShift NetworkPolicy renders reliably
instead of conditionally bypassing assertions.
In `@controllers/networkpolicy.go`:
- Around line 104-120: Update the create-once branch in the NetworkPolicy
reconcile flow to treat an AlreadyExists error from r.Client.Create as a
successful concurrent creation. Preserve the existing wrapped error return for
other create failures and continue the normal reconcile flow when the resource
already exists.
- Around line 62-66: The per-component disabled branch in the NetworkPolicy
reconciliation flow must remove policies previously owned by that component
instead of simply continuing. Add or reuse an ensureNoNetworkPolicy-style
cleanup path alongside the existing global deletion handling, invoke it when
mce.Enabled(component) is false, and preserve the current creation/update flow
for enabled components.
- Around line 81-87: Update the error handling after renderer.RenderChart in the
NetworkPolicy reconciliation flow to distinguish the benign absence of a
NetworkPolicy template from genuine rendering failures. Preserve the non-fatal
behavior for components without that template, but elevate real template or
values errors to an operator-visible warning or error log with the component and
renderer details before continuing.
🪄 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: Repository: stolostron/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 07c33016-24e5-44c6-b2bf-5166ce620341
⛔ Files ignored due to path filters (1)
api/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (12)
Makefileapi/v1/multiclusterengine_types.gobundle/manifests/multicluster-engine.clusterserviceversion.yamlbundle/manifests/multicluster.openshift.io_multiclusterengines.yamlconfig/crd/bases/multicluster.openshift.io_multiclusterengines.yamlcontrollers/backplaneconfig_controller_test.gocontrollers/networkpolicy.gocontrollers/networkpolicy_test.gocontrollers/toggle_components_test.gopkg/rendering/renderer.gopkg/rendering/renderer_test.gopkg/templates/charts/toggle/hypershift/templates/hypershift-addon-manager-networkpolicy.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- config/crd/bases/multicluster.openshift.io_multiclusterengines.yaml
- bundle/manifests/multicluster.openshift.io_multiclusterengines.yaml
- bundle/manifests/multicluster-engine.clusterserviceversion.yaml
|
This was referenced Jul 21, 2026
This was referenced Jul 22, 2026
7 tasks
This was referenced Jul 22, 2026
kurwang
pushed a commit
to kurwang/backplane-operator
that referenced
this pull request
Jul 27, 2026
* Add NetworkPolicy support to MCE operator
Implements create-once NetworkPolicy pattern for MCE components.
Changes:
- Add networkPolicies.enabled field to MultiClusterEngine CRD (default: true)
- Add NetworkPoliciesConfig type to MCE spec
- Implement ensureNetworkPolicies reconciliation logic
- Add NetworkPoliciesValue to rendering Global values
- Add networkPolicies.enabled to all component values.yaml files
- Skip NetworkPolicy resources in component ensure/delete loops
Create-once pattern:
- MCE creates initial NetworkPolicy if missing
- Operand teams adopt and manage policies
- MCE deletes all MCE-created policies when globally disabled
- No continuous reconciliation after creation
Tracks ownership via installer labels:
- installer.name = mce.Name
- installer.namespace = mce.Namespace
Signed-off-by: dislbenn <dbennett@redhat.com>
* fix: address CodeRabbit review findings for NetworkPolicy support
- Add list verb to networkpolicies RBAC to allow List() calls during cleanup
- Fix namespace selector to use mce.Spec.TargetNamespace instead of mce.Namespace (cluster-scoped resource has empty namespace)
- Add isComponentExternallyManaged guard to skip externally managed components
- Apply release-version annotation before Create to match sibling apply paths
Signed-off-by: dislbenn <dbennett@redhat.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
* chore: regenerate manifests after RBAC updates
Generated from RBAC marker changes in controllers/backplaneconfig_controller.go
Signed-off-by: dislbenn <dbennett@redhat.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
* fix: remove required fields from condition schema
Remove required constraint on message, reason, status, type fields in conditions array
Signed-off-by: dislbenn <dbennett@redhat.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
* fix: register networkingv1 scheme in controller tests
NetworkPolicy controller added in earlier commit but test suite
missing networkingv1 scheme registration. Tests fail when
ensureNetworkPolicies() called because client can't handle
NetworkPolicy type.
Add networkingv1.AddToScheme to both test client scheme and
manager scheme in suite_test.go.
Signed-off-by: dislbenn <disaiah.bennett@ibm.com>
Signed-off-by: dislbenn <dbennett@redhat.com>
* fix: skip NetworkPolicy in hypershift apply loop
ensureNoHyperShift delete loop already skips NetworkPolicy (managed
by ensureNetworkPolicies controller), but ensureHyperShift apply loop
was still applying NetworkPolicy templates. This caused hypershift
NetworkPolicy to be created outside the ensureNetworkPolicies flow,
bypassing the create-once pattern and proper label management.
Add NetworkPolicy skip to hypershift apply loop to match delete loop.
Signed-off-by: dislbenn <disaiah.bennett@ibm.com>
Signed-off-by: dislbenn <dbennett@redhat.com>
* fix: prevent panic on chart load failure
Renderer accessed chart.Name() when chart is nil after load error,
causing panic. Also add chartPath validation in NetworkPolicy controller
before calling RenderChart.
Fixes panic: runtime error: invalid memory address or nil pointer dereference
Signed-off-by: dislbenn <disaiah.bennett@ibm.com>
Signed-off-by: dislbenn <dbennett@redhat.com>
* fix: split long lines to satisfy SonarCloud 120 char limit
Split function signatures, error messages, and log calls across multiple
lines to comply with SonarCloud line length requirements.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: dislbenn <dbennett@redhat.com>
* refactor: make networkPolicies field always visible in spec
Changed NetworkPolicies from pointer to value type and removed omitempty
to ensure the security feature is always visible to customers. The field
now appears in every MCE spec with its default value (enabled: true).
Changes:
- NetworkPolicies: *NetworkPoliciesConfig -> NetworkPoliciesConfig
- Removed omitempty from networkPolicies field
- Removed omitempty from enabled subfield
- Simplified nil-checks in controller and renderer
- CRD now requires networkPolicies field with enabled defaulting to true
This makes the security feature discoverable and explicit rather than
hidden when using defaults.
Signed-off-by: dislbenn <dbennett@redhat.com>
* test: add NetworkPolicy deletion tests
Add comprehensive tests for NetworkPolicy deletion when disabled:
- Delete all MCE-created NetworkPolicies when globally disabled
- Delete multiple NetworkPolicies
- Preserve NetworkPolicies from other MCE instances
- Handle non-existent NetworkPolicies gracefully
- Verify NetworkPolicies persist when enabled
Tests verify the label-based selector correctly identifies and deletes
only NetworkPolicies created by this MCE instance.
Signed-off-by: dislbenn <dbennett@redhat.com>
* style: align const formatting in networkpolicy tests
Signed-off-by: dislbenn <dbennett@redhat.com>
* fix: create namespace in networkpolicy tests
Tests were failing because multicluster-engine namespace didn't exist.
Added BeforeEach to create namespace and AfterEach to clean up resources.
Fixes test failures:
- should delete all MCE-created NetworkPolicies
- should delete multiple MCE-created NetworkPolicies
- should not delete NetworkPolicies from other MCE instances
- should not delete existing NetworkPolicies
Signed-off-by: dislbenn <dbennett@redhat.com>
* chore: consolidate networking.k8s.io RBAC rules in generated manifests
Run make manifests to merge duplicate networking.k8s.io apiGroup entries
(ingresses and networkpolicies) into single blocks.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: dislbenn <dbennett@redhat.com>
* fix: resolve networkpolicy test failures from namespace lifecycle in envtest
envtest lacks a garbage collector, so deleting and recreating the
namespace between specs caused 409 conflicts ("object is being deleted").
Create the namespace once in BeforeAll and only clean up NetworkPolicies
between specs.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: dislbenn <dbennett@redhat.com>
* fix: add Ordered container for BeforeAll and replace deprecated Requeue
Ginkgo requires BeforeAll inside an Ordered container. Also replace
deprecated result.Requeue with result.RequeueAfter check.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: dislbenn <dbennett@redhat.com>
* fix: remove deprecated --use-deprecated-gcs flag from setup-envtest
Flag was removed in setup-envtest release-0.23. GCS is no longer used
by default, making the flag unnecessary.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: dislbenn <dbennett@redhat.com>
* test: increase NetworkPolicy code coverage
- Add enabled-path tests: skip disabled components, skip externally
managed components, create-once pattern with real hypershift chart
- Add renderer tests: NetworkPoliciesValue injection (enabled/disabled),
invalid chart path error handling
- Add Helm conditional to hypershift NP template so it only renders
when networkPolicies.enabled=true
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: dislbenn <dbennett@redhat.com>
* style: fix import ordering in networkpolicy_test.go
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: dislbenn <dbennett@redhat.com>
* test: add coverage for NetworkPolicy creation error path
Test ensureNetworkPolicies with nonexistent target namespace to trigger
Create failure when chart renders a NetworkPolicy template.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: dislbenn <dbennett@redhat.com>
* test: enable NetworkPolicies in test MCE specs for coverage
Set NetworkPolicies.Enabled=true in all test MCE objects so the
hypershift NP template renders and toggle_components skip logic
is exercised. Fixes coverage regression from adding Helm conditional.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: dislbenn <dbennett@redhat.com>
* style: fix whitespace alignment in backplaneconfig_controller_test.go
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: dislbenn <dbennett@redhat.com>
---------
Signed-off-by: dislbenn <dbennett@redhat.com>
Signed-off-by: dislbenn <disaiah.bennett@ibm.com>
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
This was referenced Jul 27, 2026
This was referenced Aug 4, 2026
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
2 participants
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
Implements NetworkPolicy support for MultiClusterEngine operator with create-once pattern. This enables MCE to create initial NetworkPolicy resources that operand teams can then adopt and manage.
Related Issue
Part of NetworkPolicy implementation for ACM 5.0 (NetworkPolicy-DDR-Updated.md)
Changes Made
networkPolicies.enabledfield to MultiClusterEngine CRD (default: true)ensureNetworkPoliciescontroller with create-once patternCreate-once pattern:
Ownership tracking:
Uses existing installer labels:
installer.name = mce.Nameinstaller.namespace = mce.NamespaceChecklist
Additional Notes
Reviewers
/cc @cameronmwall @ngraham20
Definition of Done
Summary by CodeRabbit