CORENET-6816: Add NetworkPolicies for CNO and its operands#2892
CORENET-6816: Add NetworkPolicies for CNO and its operands#2892danwinship wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@danwinship: This pull request references CORENET-2953 which is a valid 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. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
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:
WalkthroughAppends multiple Kubernetes NetworkPolicy resources (namespace-wide default-deny and component-scoped policies) across bindata and manifests, and updates unit tests to expect the additional rendered NetworkPolicy objects. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
approved
danwinship
force-pushed
the
default-networkpolicies
branch
from
e048e7a to
17fb479
Compare
|
/test e2e-gcp-ovn |
danwinship
force-pushed
the
default-networkpolicies
branch
from
17fb479 to
cad642b
Compare
|
/test e2e-gcp-ovn |
1 similar comment
|
/test e2e-gcp-ovn |
|
/test e2e-metal-ipi-ovn-ipv6 |
danwinship
force-pushed
the
default-networkpolicies
branch
from
cad642b to
9b46870
Compare
|
/test e2e-metal-ipi-ovn-ipv6 |
danwinship
changed the title
|
@danwinship: This pull request references CORENET-6816 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 story to target the "4.22.0" version, but no target version was set. 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. |
danwinship
force-pushed
the
default-networkpolicies
branch
from
9b46870 to
c834076
Compare
danwinship
changed the title
|
/assign @knobunc |
danwinship
force-pushed
the
default-networkpolicies
branch
from
c834076 to
2fc5761
Compare
|
/test e2e-metal-ipi-ovn-ipv6 |
openshift-merge-robot
added
the
needs-rebase
danwinship
force-pushed
the
default-networkpolicies
branch
from
2fc5761 to
6f22966
Compare
openshift-merge-robot
removed
the
needs-rebase
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@bindata/network/network-metrics/003-networkpolicy.yaml`:
- Around line 1-15: The NetworkPolicy resource (kind: NetworkPolicy,
metadata.name: network-metrics in namespace openshift-multus) currently allows
ingress to port 8443 from any source; update spec.ingress (for the podSelector
matchLabels: app: network-metrics-daemon and port: 8443) to add an ingress.from
that restricts sources to the openshift-monitoring namespace by using a
namespaceSelector matching that namespace (e.g., namespaceSelector.matchLabels:
name: openshift-monitoring), so only pods in the openshift-monitoring namespace
(prometheus-k8s ServiceAccount) can reach the metrics port.
In `@bindata/network/ovn-kubernetes/common/openshift-host-network-ns.yaml`:
- Around line 11-23: The default-deny NetworkPolicy named default-deny in the
openshift-host-network namespace blocks all traffic; add companion allow-list
NetworkPolicy resources in the same namespace to permit required host-network
traffic (e.g., control-plane API/etcd access, node metrics scraping, and any
necessary DNS/NTP) by creating NetworkPolicy objects that target the same
namespace and use podSelector/namespaceSelector/IPBlock and specific
ports/protocols to allow ingress and egress for control-plane endpoints and
metrics endpoints; ensure the new policies are scoped narrowly (use explicit
ports and selectors) and reference the existing NetworkPolicy by name
(default-deny) to make intent clear.
🧹 Nitpick comments (1)
bindata/network-diagnostics/networkpolicy.yaml (1)
35-37: Consider enforcing "no egress" via policyTypes.The comment states "network-check-target does no egress," but since
Egressis not inpolicyTypes, egress is actually unrestricted. If the intent is defense-in-depth, addEgresstopolicyTypeswithout anyegress:rules to explicitly deny all outbound traffic.♻️ Proposed change to enforce no egress
policyTypes: # network-check-target does no egress - Ingress + - Egress
|
@danwinship: This pull request references CORENET-6816 which is a valid 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. |
|
/retest |
|
|
||
| // Check rendered object | ||
| g.Expect(len(objs)).To(Equal(10)) | ||
| g.Expect(len(objs)).To(Equal(11)) |
There was a problem hiding this comment.
@danwinship Is this counting additional object i.e. network policy? In openshift-multus there are two objects increased.
There was a problem hiding this comment.
This unit test is only calling renderMultusAdmissionController, without calling renderMultus first. So the only new object is the admission controller's new NetworkPolicy.
|
|
||
| // It's important that the namespace is first | ||
| g.Expect(len(objs)).To(Equal(32), "Expected 32 multus related objects") | ||
| g.Expect(len(objs)).To(Equal(34), "Expected 34 multus related objects") |
There was a problem hiding this comment.
@danwinship Could you clarify, in context of the previous response, how does a network policy results in increasing the count by 2?
There was a problem hiding this comment.
in this case, objs is the result of renderMultus, which renders the contents of bindata/network/multus and bindata/network/network-metrics, so it gets the new default-deny policy from the former and the new "allow ingress to metrics" NetworkPolicy from the latter.
|
/unassign knobunc |
| policyTypes: | ||
| - Ingress | ||
| - Egress | ||
| ingress: |
There was a problem hiding this comment.
Did you check stuff like EIP or services healthcheck? Not sure how those run on managed clusters so just asking.
There was a problem hiding this comment.
The policy doesn't affect "traffic that is processed by ovn-kubernetes", it affects "traffic that passes through a pod in the openshift-ovn-kubernetes namespace". So EgressIP would only be affected if, say, some ovn-k process was acting as a proxy server. I'm not totally familiar with everything we've done in ovn-k, but I'm pretty sure we don't do anything like that.
And even if we did, it would probably go through the ovnkube-node pods, not ovnkube-control-plane, so it would still be hostNetwork even on HCP (which is also the answer for service health checks).
There was a problem hiding this comment.
OK for the service health checks.
The EIP healthcheck is a connection that ovnkube-control-plane pod does to ovnkube-node pods on port 9107 to check their liveliness. I am not sure it is that same port on hosted clusters since things are proxied there in a number of weird ways. From the perspective of this policy that would be an egress connection, right? If so, it's covered by the allow-all egress rule here but might want to add it as doc.
|
/retest |
danwinship
force-pushed
the
default-networkpolicies
branch
from
0a33f49 to
8b63839
Compare
|
@danwinship: This pull request references CORENET-6816 which is a valid 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: 1
🧹 Nitpick comments (1)
bindata/network/node-identity/managed/node-identity-networkpolicy.yaml (1)
17-19: Comment is misleading —- {}allows all egress, not just apiserver.The comment on line 18 states "Allow to apiserver", but
- {}is an allow-all egress rule. If the intent is to restrict egress to only the apiserver, this rule should be tightened with specific destinations. If allowing all egress is intentional (e.g., because restricting to apiserver is impractical), consider updating the comment to reflect the actual behavior.📝 Suggested comment clarification (if allow-all is intentional)
egress: - # Allow to apiserver + # Allow all egress (apiserver, DNS, etc.) - {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindata/network/node-identity/managed/node-identity-networkpolicy.yaml` around lines 17 - 19, The comment "Allow to apiserver" is incorrect because the egress rule using the empty rule "- {}" under egress allows all outbound traffic; either replace the allow-all egress rule with a specific egress rule that targets the apiserver (e.g., by setting hostname/IP/ports in the egress rule) so egress only goes to the apiserver, or, if the allow-all behavior is intentional, update the comment to state that this is an intentional allow-all egress rule; refer to the egress stanza and the "- {}" rule in node-identity-networkpolicy.yaml when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindata/network/ovn-kubernetes/managed/networkpolicy.yaml`:
- Around line 13-16: The ingress rule under the ingress block has a malformed
ports structure: the port entry under ports must be a list item. Edit the
ingress -> ports section (the ingress key and its ports list) and change the
single mapping "port: 9108" to be a list entry by prefixing it with a dash so
the ports value is a YAML sequence (e.g., ports: - port: 9108).
---
Nitpick comments:
In `@bindata/network/node-identity/managed/node-identity-networkpolicy.yaml`:
- Around line 17-19: The comment "Allow to apiserver" is incorrect because the
egress rule using the empty rule "- {}" under egress allows all outbound
traffic; either replace the allow-all egress rule with a specific egress rule
that targets the apiserver (e.g., by setting hostname/IP/ports in the egress
rule) so egress only goes to the apiserver, or, if the allow-all behavior is
intentional, update the comment to state that this is an intentional allow-all
egress rule; refer to the egress stanza and the "- {}" rule in
node-identity-networkpolicy.yaml when making the change.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (22)
bindata/kube-proxy/000-ns.yamlbindata/network-diagnostics/000-ns.yamlbindata/network-diagnostics/networkpolicy.yamlbindata/network/frr-k8s/000-ns.yamlbindata/network/iptables-alerter/004-networkpolicy.yamlbindata/network/multus-admission-controller/004-networkpolicy.yamlbindata/network/multus/000-ns.yamlbindata/network/network-metrics/003-networkpolicy.yamlbindata/network/node-identity/common/001-node-identity-namespace.yamlbindata/network/node-identity/managed/node-identity-networkpolicy.yamlbindata/network/ovn-kubernetes/common/000-ns.yamlbindata/network/ovn-kubernetes/common/openshift-host-network-ns.yamlbindata/network/ovn-kubernetes/managed/networkpolicy.yamlbindata/networking-console-plugin/000-namespace.yamlbindata/networking-console-plugin/005-networkpolicy.yamlmanifests/0000_70_cluster-network-operator_00_namespace.yamlpkg/network/kube_proxy_test.gopkg/network/multus_admission_controller_test.gopkg/network/multus_test.gopkg/network/network_metrics_test.gopkg/network/ovn_kubernetes_test.gopkg/network/render_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/network/ovn_kubernetes_test.go
- bindata/network/ovn-kubernetes/common/000-ns.yaml
- pkg/network/network_metrics_test.go
- manifests/0000_70_cluster-network-operator_00_namespace.yaml
- bindata/network/iptables-alerter/004-networkpolicy.yaml
- bindata/networking-console-plugin/000-namespace.yaml
- bindata/network-diagnostics/000-ns.yaml
- pkg/network/kube_proxy_test.go
- bindata/network/ovn-kubernetes/common/openshift-host-network-ns.yaml
- pkg/network/multus_test.go
- bindata/network/multus-admission-controller/004-networkpolicy.yaml
- bindata/network/multus/000-ns.yaml
- bindata/kube-proxy/000-ns.yaml
danwinship
force-pushed
the
default-networkpolicies
branch
from
8b63839 to
7b431e8
Compare
|
@danwinship: This pull request references CORENET-6816 which is a valid 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. |
danwinship
force-pushed
the
default-networkpolicies
branch
from
7b431e8 to
11814a6
Compare
danwinship
force-pushed
the
default-networkpolicies
branch
from
11814a6 to
d8d15af
Compare
|
/retest |
|
The hypershift issues might be legit /retest |
|
/retest |
|
Thanks Dan, this looks good to me. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, knobunc 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 |
|
sigh, hypershift-e2e-aks is currently failing to even build images or something, so we can't tell if it's still having the maybe-NetworkPolicy-related failure. |
|
/retest |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
@danwinship: The following tests failed, say
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
NetworkPolicies for CNO and its operands.
For the most part I added
default-denypolicies to every Namespace yaml, to ensure that Namespaces end up isolated right away, and then added individual policies for each operand after that, based on the current best practices (which aren't always that "best" since we can't really match host-network traffic...)Summary by CodeRabbit