NO-JIRA: Fix NetworkPolicy for openshift-capi-operator namespace #453
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@mdbooth: This pull request explicitly references no jira issue. DetailsIn response to this:
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. |
📝 WalkthroughWalkthroughAdds and updates Kubernetes NetworkPolicy manifests: two ingress policies for metrics access (including ports 8443 and 8442 and podSelector adjustments), two default-deny policies (ingress+egress) in both namespaces, and two egress-allow policies split across operator and controller namespaces. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
manifests/0000_30_cluster-api_17_default-deny.yaml (1)
50-53:⚠️ Potential issue | 🟡 MinorComment references incorrect label value for this namespace.
The comment mentions
k8s-app=cluster-capi-operator, but the corresponding allow-ingress policy for theopenshift-cluster-apinamespace usesk8s-app: capi-controllers. This is a different label entirely from what the comment states.📝 Suggested fix
# Exclude CAPI pods that need network access: This is done by the other policies # - control-plane: CAPI controller manager pods (capg, capi, capa, capz, etc.) - # - k8s-app=cluster-capi-operator: Main cluster-capi-operator pod + # - k8s-app=capi-controllers: CAPI controller pods # This ensures these pods can communicate while still denying traffic to other pods
🤖 Fix all issues with AI agents
In `@manifests/0000_30_cluster-api_17_default-deny.yaml`:
- Around line 28-31: Update the explanatory comment to use the correct label
value used by the allow-ingress policy: replace "k8s-app=cluster-capi-operator"
with "k8s-app=capi-operator" so it matches the label in
0000_30_cluster-api_14_allow-ingress-to-metrics-operators.yaml; ensure the
comment and the allow-ingress policy both reference the exact string "k8s-app:
capi-operator" to avoid future confusion.
🧹 Nitpick comments (1)
manifests/0000_30_cluster-api_14_allow-ingress-to-metrics-operators.yaml (1)
27-33: RedundantpodSelectorinfromclause.The
namespaceSelector: {}already matches all pods in all namespaces, making the separatepodSelector: {}entry redundant. When listed as separate items infrom, they're ORed together—but the first rule already covers everything.If the intent is to allow from any pod in any namespace, only
namespaceSelector: {}is needed. Alternatively, if you want a single combined rule, both selectors should be in the same list item.♻️ Simplified version (either option works)
Option 1: Just namespaceSelector (allows all pods from all namespaces)
ingress: - from: - namespaceSelector: {} - - podSelector: {} ports: - port: 8443 protocol: TCPOption 2: Combined selector (explicit AND, same effect)
ingress: - from: - namespaceSelector: {} + podSelector: {} - - podSelector: {} ports: - port: 8443 protocol: TCP
| # Exclude CAPI pods that need network access: This is done by the other policies | ||
| # - control-plane: CAPI controller manager pods (capg, capi, capa, capz, etc.) | ||
| # - k8s-app=cluster-capi-operator: Main cluster-capi-operator pod | ||
| # This ensures these pods can communicate while still denying traffic to other pods |
There was a problem hiding this comment.
Comment references incorrect label value.
The comment mentions k8s-app=cluster-capi-operator, but the corresponding allow-ingress policy in 0000_30_cluster-api_14_allow-ingress-to-metrics-operators.yaml uses k8s-app: capi-operator (without "cluster-" prefix). This inconsistency could confuse future maintainers trying to understand how the policies work together.
📝 Suggested fix
# Exclude CAPI pods that need network access: This is done by the other policies
# - control-plane: CAPI controller manager pods (capg, capi, capa, capz, etc.)
- # - k8s-app=cluster-capi-operator: Main cluster-capi-operator pod
+ # - k8s-app=capi-operator: Main cluster-capi-operator pod
# This ensures these pods can communicate while still denying traffic to other pods📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Exclude CAPI pods that need network access: This is done by the other policies | |
| # - control-plane: CAPI controller manager pods (capg, capi, capa, capz, etc.) | |
| # - k8s-app=cluster-capi-operator: Main cluster-capi-operator pod | |
| # This ensures these pods can communicate while still denying traffic to other pods | |
| # Exclude CAPI pods that need network access: This is done by the other policies | |
| # - control-plane: CAPI controller manager pods (capg, capi, capa, capz, etc.) | |
| # - k8s-app=capi-operator: Main cluster-capi-operator pod | |
| # This ensures these pods can communicate while still denying traffic to other pods |
🤖 Prompt for AI Agents
In `@manifests/0000_30_cluster-api_17_default-deny.yaml` around lines 28 - 31,
Update the explanatory comment to use the correct label value used by the
allow-ingress policy: replace "k8s-app=cluster-capi-operator" with
"k8s-app=capi-operator" so it matches the label in
0000_30_cluster-api_14_allow-ingress-to-metrics-operators.yaml; ensure the
comment and the allow-ingress policy both reference the exact string "k8s-app:
capi-operator" to avoid future confusion.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
/lgtm We'll have to remember about the default-deny network policy if we're adding stuff to the operator namespace. |
|
Scheduling tests matching the |
|
/assign @miyadav |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: damdo 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 |
openshift-ci
bot
added
the
approved
|
/hold @mdbooth I'm seeing these error in the AWS TP job Looks like we might be doing a full network partition here? |
openshift-ci
bot
added
the
do-not-merge/hold
|
I created a cluster with the PR , I could see below - We see cluster installation as successful though ( i think we already know about that , co not being degraded ) |
mdbooth
force-pushed
the
network-policy
branch
from
42fa0bb to
f7bf8a2
Compare
|
@mdbooth: This pull request explicitly references no jira issue. DetailsIn response to this:
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. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
manifests/0000_30_cluster-api_14_allow-ingress-to-metrics-operators.yaml (1)
42-71:⚠️ Potential issue | 🟡 MinorUpdate the NetworkPolicy comment to accurately reflect the diagnostics endpoint, not metrics.
Port 8442 is correctly exposed by the
machine-api-migrationcontainer as shown in the deployment (lines 62, 68-69), but the NetworkPolicy comment is misleading—it describes 8442 as a "metrics endpoint" when it's actually a diagnostics endpoint (--diagnostics-address=:8442). Similarly, port 8443 used bycapi-controllersis also a diagnostics endpoint, not metrics. Update the comment to accurately reflect this.
|
This looks good manually for now - As suggested automation is WIP |
damdo
left a comment
damdo
left a comment
There was a problem hiding this comment.
/hold
/lgtm
^ to trigger testing
|
Scheduling tests matching the |
|
/retest |
|
Hey @miyadav did you want to run other checks aside from: #453 (comment) or is this considered verified? Thanks :) |
hey @damdo , remaining tests are here , I used this same build. The test here was just a smoke test to make sure , no crashing of pod due to added network policy. So consider it to be VERIFIED , if the other PR looks good , unless any other checks you might want us to add , we can do that. |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@miyadav yeah it looks like we never had metrics collected/exposed, then I think we are good to merge! |
|
/verified by @miyadav |
openshift-ci-robot
added
the
verified
|
@miyadav: This PR has been marked as verified by DetailsIn response to this:
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. |
|
/override ci/prow/e2e-openstack-ovn-techpreview Unrelated failure |
|
@damdo: Overrode contexts on behalf of damdo: ci/prow/e2e-openstack-ovn-techpreview DetailsIn response to this:
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. |
|
@mdbooth: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
6 participants
Updates NetworkPolicy to reflect changes made in #447
Summary by CodeRabbit