Skip to content

CM-763: Fix infinite update loop for user-defined network policies - #478

Closed
swghosh wants to merge 2 commits into
openshift:masterfrom
swghosh:fix-CM-763
Closed

CM-763: Fix infinite update loop for user-defined network policies#478
swghosh wants to merge 2 commits into
openshift:masterfrom
swghosh:fix-CM-763

Conversation

@swghosh

@swghosh swghosh commented Aug 19, 2026

Copy link
Copy Markdown
Member

Summary

  • Fix infinite reconciliation loop in CertManagerNetworkPolicyUserDefinedController that caused continuous NetworkPolicyUpdated events every 400-800ms when multiple user-defined network policies were configured
  • Add apiequality.Semantic.DeepEqual comparison of existing vs desired spec and labels before issuing an update — skip the write and event when nothing changed
  • Make the UserNetworkPolicyReconcileSuccess event conditional on actual changes to eliminate steady-state event noise

Root Cause

createOrUpdateNetworkPolicy unconditionally called Update() on every sync cycle for existing policies without comparing whether the spec or labels actually differed. Each update triggered informer re-queues, creating a tight reconciliation loop.

Test plan

  • Unit tests for createOrUpdateNetworkPolicy covering create, no-op update, and spec-change update scenarios
  • Manual verification: enable defaultNetworkPolicy: "true" with multiple networkPolicies[], confirm no repeated NetworkPolicyUpdated events after initial creation
  • Verify NetworkPolicyCreated events still fire on first policy creation
  • Verify NetworkPolicyUpdated events fire when spec is actually changed

Always review AI generated responses prior to use.
Generated with Claude Code via openshift-developer plugin

Summary by CodeRabbit

  • Bug Fixes

    • Prevented unnecessary updates when network policies are already up to date.
    • Success events are now emitted only when a policy is created or changed.
    • Improved error reporting for network policy operations.
  • Tests

    • Added coverage for policy creation, unchanged policies, and updates after specification changes.

swghosh and others added 2 commits August 19, 2026 16:21
The controller was unconditionally calling Update() on every reconciliation
cycle for existing user-defined network policies, even when spec and labels
were unchanged. This caused an infinite reconciliation loop due to informer
re-queues, generating excessive NetworkPolicyUpdated events every 400-800ms.

Add equality checks using apiequality.Semantic.DeepEqual to compare existing
and desired spec/labels before issuing an update. Also make the success event
conditional on whether any policy was actually created or updated, eliminating
unnecessary event noise during steady-state operation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cover the three key scenarios in createOrUpdateNetworkPolicy:
- Creating a new policy emits NetworkPolicyCreated and returns changed=true
- Updating with identical spec/labels is a no-op (no event, changed=false)
- Updating with different spec emits NetworkPolicyUpdated and returns changed=true

These tests verify the fix for the infinite reconciliation loop that occurred
when multiple user-defined network policies were configured.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 19, 2026
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 19, 2026
@openshift-ci

openshift-ci Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci-robot

openshift-ci-robot commented Aug 19, 2026

Copy link
Copy Markdown

@swghosh: This pull request references CM-763 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.1.0" version, but no target version was set.

Details

In response to this:

Summary

  • Fix infinite reconciliation loop in CertManagerNetworkPolicyUserDefinedController that caused continuous NetworkPolicyUpdated events every 400-800ms when multiple user-defined network policies were configured
  • Add apiequality.Semantic.DeepEqual comparison of existing vs desired spec and labels before issuing an update — skip the write and event when nothing changed
  • Make the UserNetworkPolicyReconcileSuccess event conditional on actual changes to eliminate steady-state event noise

Root Cause

createOrUpdateNetworkPolicy unconditionally called Update() on every sync cycle for existing policies without comparing whether the spec or labels actually differed. Each update triggered informer re-queues, creating a tight reconciliation loop.

Test plan

  • Unit tests for createOrUpdateNetworkPolicy covering create, no-op update, and spec-change update scenarios
  • Manual verification: enable defaultNetworkPolicy: "true" with multiple networkPolicies[], confirm no repeated NetworkPolicyUpdated events after initial creation
  • Verify NetworkPolicyCreated events still fire on first policy creation
  • Verify NetworkPolicyUpdated events fire when spec is actually changed

Always review AI generated responses prior to use.
Generated with Claude Code via openshift-developer plugin

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2026
@openshift-ci

openshift-ci Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-ci

openshift-ci Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: swghosh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 19, 2026
@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3a08ac57-97f1-4a2f-87bf-18b20ddeca59

📥 Commits

Reviewing files that changed from the base of the PR and between 4d40f30 and d9309ac.

📒 Files selected for processing (2)
  • pkg/controller/deployment/cert_manager_networkpolicy.go
  • pkg/controller/deployment/cert_manager_networkpolicy_test.go

Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.


Walkthrough

NetworkPolicy reconciliation now returns whether resources changed, skips no-op updates, and emits success events only after creation or update. Unit tests cover creation, unchanged reconciliation, and specification changes.

Changes

NetworkPolicy reconciliation

Layer / File(s) Summary
Reconciliation change tracking
pkg/controller/deployment/cert_manager_networkpolicy.go
The controller compares existing policy specs and labels with desired values. It returns change status from reconciliation and emits success events only when a policy changes.
Reconciliation behavior tests
pkg/controller/deployment/cert_manager_networkpolicy_test.go
Tests verify creation events, no-op reconciliation without events, and update events after a port change.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to d9309

The PR prevents unchanged network policies from being rewritten and adds focused tests; no actionable merge-blocking risk remains beyond normal checks and review.

Possibly related PRs


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Container-Privileges ❓ Inconclusive Evidence collection is still in progress. Need inspect the pull-request diff and relevant Kubernetes/container manifests for the listed privilege settings.
✅ Passed checks (14 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: fixing an infinite update loop for user-defined network policies.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed The PR adds only static Go test names; it adds no Ginkgo title calls and no test title contains generated, environment-dependent, or runtime values.
Test Structure And Quality ✅ Passed The added tests use Go's testing package with a fresh fake client, not Ginkgo; they have no waits or cluster-scoped resources, and assertions include diagnostic messages.
Microshift Test Compatibility ✅ Passed The added tests use Go's testing package and a fake Kubernetes client; they add no Ginkgo e2e tests or unavailable MicroShift APIs.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The added file contains standard Go unit tests using testing.T and a fake Kubernetes client. It adds no Ginkgo e2e test or multi-node/HA assumption.
Topology-Aware Scheduling Compatibility ✅ Passed The PR changes NetworkPolicy reconciliation and adds unit tests only; the diff introduces no node selectors, affinity, topology spread, replica, toleration, or PDB scheduling constraints.
Ote Binary Stdout Contract ✅ Passed The PR changes only network-policy reconciliation and unit tests; the base-to-HEAD Go diff adds no stdout, klog, suite-setup, or process-entry-point writes.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The added tests use standard testing.T with a fake Kubernetes client and contain no Ginkgo tests, IPv4 assumptions, public endpoints, or external connectivity.
No-Weak-Crypto ✅ Passed The PR changes only NetworkPolicy reconciliation and tests; scans found no MD5, SHA1, DES, RC4, Blowfish, ECB, custom crypto, or secret comparisons.
No-Sensitive-Data-In-Logs ✅ Passed The PR adds no sensitive logging. New success events are generic, and policy-name events/error context already existed unchanged; the change only suppresses no-op events.
✨ Finishing Touches 💡 1
⚔️ Resolve merge conflicts 💡
  • Resolve merge conflict in branch fix-CM-763
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions


Comment @coderabbitai help to get the list of available commands.

@swghosh

swghosh commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

/close

dummy PR created by /jira:solve for evals.

@openshift-ci openshift-ci Bot closed this Aug 19, 2026
@openshift-ci

openshift-ci Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

@swghosh: Closed this PR.

Details

In response to this:

/close

dummy PR created by /jira:solve for evals.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants