Skip to content

Conversation

@lunarwhite
Copy link
Member

@lunarwhite lunarwhite commented Oct 30, 2025

Motivation

Fixes: https://issues.redhat.com/browse/CM-764

The user-defined network policy controller only watches the CertManager CR, not the NetworkPolicy resources it manages. Without a NetworkPolicy watch, the controller only detects deletions during periodic resync (every 10 minutes). This window could unintentionally cause a security gap since NetworkPolicies are security controls.

Change

Add NetworkPolicy informer/watch to the user-defined network policy controller in the cert-manager namespace, following the same pattern used by the static resources controller.

E2E validation

  • Manually delete the user-defined network policy cert-manager-user-* created by the controller, the new ones would be reconciled back in seconds
  • Logs and events of the operator controller looks normal
  • Reconciliation can still work effectively when managed NPs modified

/cc @bharath-b-rh

otherwise reconciliation only happens during periodic resync (10 mins)
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 30, 2025
@openshift-ci openshift-ci bot requested a review from bharath-b-rh October 30, 2025 11:35
@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 30, 2025

@lunarwhite: This pull request references CM-764 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 "4.21.0" version, but no target version was set.

In response to this:

Motivation

Fixes: https://issues.redhat.com/browse/CM-764

The user-defined network policy controller only watches the CertManager CR, not the NetworkPolicy resources it manages. Without a NetworkPolicy watch, the controller only detects deletions during periodic resync (every 10 minutes). This window could unintentionally cause a security gap.

Change

Add NetworkPolicy informer/watch to the user-defined network policy controller in the cert-manager namespace, following the same pattern used by the static resources controller.

E2E validation

  • Manually delete the user-defined network policy cert-manager-user-* created by the controller, the new ones would be reconciled back in seconds
  • Logs and events of the operator controller looks normal
  • Reconciliation can still work effectively when managed NPs modified

/cc @bharath-b-rh

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.

@coderabbitai
Copy link

coderabbitai bot commented Oct 30, 2025

Walkthrough

Constructor signature of CertManagerNetworkPolicyUserDefinedController updated to accept kubeInformersForNamespaces parameter. The controller now stores this informer factory and uses it to wire NetworkPolicy informers for the cert-manager namespace, enabling reconciliation on NetworkPolicy resource changes.

Changes

Cohort / File(s) Summary
CertManagerNetworkPolicyUserDefinedController updates
pkg/controller/deployment/cert_manager_controller_set.go, pkg/controller/deployment/cert_manager_networkpolicy.go
Constructor updated to accept kubeInformersForNamespaces parameter. New field added to controller struct to store the informer factory. WithInformersQueueKeyFunc wired to support NetworkPolicy informers with "cluster" queue key. Runtime package imported for queue key function signature support.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Review the new WithInformersQueueKeyFunc implementation to verify queue key logic and how informers are wired
  • Confirm parameter ordering in constructor call matches updated function signature
  • Verify kubeInformersForNamespaces is properly stored and used within the controller

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "CM-764: Add NetworkPolicy informer to user-defined network policy controller" directly and accurately reflects the primary change in the changeset. The modifications add a NetworkPolicy informer to the user-defined network policy controller, wiring it into the cert-manager namespace as described in the title. The title is specific, concise, and clearly conveys the main objective to a team member reviewing the commit history.
Description Check ✅ Passed The PR description is directly related to the changeset and provides valuable context including the motivation (addressing a security gap by reducing the detection window from ~10 minutes to seconds), the specific change being made (adding a NetworkPolicy informer following an established pattern), and concrete E2E validation evidence. The description references the Jira issue CM-764 and demonstrates thoughtful consideration of the security implications, making it clear and meaningful rather than generic or off-topic.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 2a97312 and 42e5d40.

📒 Files selected for processing (2)
  • pkg/controller/deployment/cert_manager_controller_set.go (1 hunks)
  • pkg/controller/deployment/cert_manager_networkpolicy.go (4 hunks)
🔇 Additional comments (5)
pkg/controller/deployment/cert_manager_networkpolicy.go (4)

10-10: LGTM - Runtime import correctly added.

The runtime import is necessary for the runtime.Object type used in the queue key function at line 116.


87-87: LGTM - Informer factory field correctly added.

The field enables the controller to access namespace-scoped informers, specifically for watching NetworkPolicy resources in the cert-manager namespace.


92-106: LGTM - Constructor correctly updated.

The constructor signature has been properly updated to accept the kubeInformersForNamespaces parameter, and it's correctly stored in the struct field. This change is consistent with the constructor pattern used by other controllers in the codebase.


113-120: Excellent - NetworkPolicy informer correctly wired to close security window.

This change enables the controller to immediately detect and reconcile NetworkPolicy deletions instead of waiting for the periodic resync (~10 minutes). Key aspects:

  • The queue key function correctly returns "cluster" for the singleton CertManager CR.
  • The informer is properly scoped to the cert-manager namespace.
  • Watching all NetworkPolicies in the namespace ensures comprehensive coverage, which is appropriate given the namespace is dedicated to cert-manager.
  • Any create/update/delete event for NetworkPolicies will now trigger immediate reconciliation, restoring user-defined policies if deleted.

This directly addresses the security concern outlined in the PR objectives.

pkg/controller/deployment/cert_manager_controller_set.go (1)

51-51: LGTM - Constructor call correctly updated.

The call to NewCertManagerNetworkPolicyUserDefinedController has been properly updated to include the kubeInformersForNamespaces parameter, matching the constructor signature change in pkg/controller/deployment/cert_manager_networkpolicy.go. The parameter order is correct and consistent with other controller constructors in the same function.

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.5.0)

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 and usage tips.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 30, 2025

@lunarwhite: This pull request references CM-764 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 "4.21.0" version, but no target version was set.

In response to this:

Motivation

Fixes: https://issues.redhat.com/browse/CM-764

The user-defined network policy controller only watches the CertManager CR, not the NetworkPolicy resources it manages. Without a NetworkPolicy watch, the controller only detects deletions during periodic resync (every 10 minutes). This window could unintentionally cause a security gap since NetworkPolicies are security controls.

Change

Add NetworkPolicy informer/watch to the user-defined network policy controller in the cert-manager namespace, following the same pattern used by the static resources controller.

E2E validation

  • Manually delete the user-defined network policy cert-manager-user-* created by the controller, the new ones would be reconciled back in seconds
  • Logs and events of the operator controller looks normal
  • Reconciliation can still work effectively when managed NPs modified

/cc @bharath-b-rh

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.

@bharath-b-rh
Copy link
Contributor

/lgtm
/approve
/label docs-approved
/label px-approved
/cherrypick cert-manager-1.18

@openshift-cherrypick-robot

@bharath-b-rh: once the present PR merges, I will cherry-pick it on top of cert-manager-1.18 in a new PR and assign it to you.

In response to this:

/lgtm
/approve
/label docs-approved
/label px-approved
/cherrypick cert-manager-1.18

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 added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Oct 30, 2025
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 30, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bharath-b-rh, lunarwhite

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

The pull request process is described here

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 Oct 30, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 30, 2025

@lunarwhite: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@lunarwhite
Copy link
Member Author

/label qe-approved

CI is green

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Oct 30, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 30, 2025

@lunarwhite: This pull request references CM-764 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 "4.21.0" version, but no target version was set.

In response to this:

Motivation

Fixes: https://issues.redhat.com/browse/CM-764

The user-defined network policy controller only watches the CertManager CR, not the NetworkPolicy resources it manages. Without a NetworkPolicy watch, the controller only detects deletions during periodic resync (every 10 minutes). This window could unintentionally cause a security gap since NetworkPolicies are security controls.

Change

Add NetworkPolicy informer/watch to the user-defined network policy controller in the cert-manager namespace, following the same pattern used by the static resources controller.

E2E validation

  • Manually delete the user-defined network policy cert-manager-user-* created by the controller, the new ones would be reconciled back in seconds
  • Logs and events of the operator controller looks normal
  • Reconciliation can still work effectively when managed NPs modified

/cc @bharath-b-rh

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-merge-bot openshift-merge-bot bot merged commit 37ab8bf into openshift:master Oct 30, 2025
11 checks passed
@lunarwhite lunarwhite deleted the fix-np branch October 30, 2025 15:28
@openshift-cherrypick-robot

@bharath-b-rh: new pull request created: #343

In response to this:

/lgtm
/approve
/label docs-approved
/label px-approved
/cherrypick cert-manager-1.18

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. docs-approved Signifies that Docs has signed off on this PR jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants