CNTRLPLANE-2905: add network policies#414
Conversation
|
@dusk125: This pull request references CNTRLPLANE-2905 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 epic 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
/retest-required |
1 similar comment
|
/retest-required |
manifests/0000_25_openshift-controller-manager-operator_01_network-policy-operator.yaml
Outdated
Show resolved
Hide resolved
manifests/0000_25_openshift-controller-manager_01_network-policy-controller-manager.yaml
Outdated
Show resolved
Hide resolved
...s/0000_25_openshift-route-controller-manager_01_network-policy-route-controller-manager.yaml
Outdated
Show resolved
Hide resolved
WalkthroughAdds Kubernetes NetworkPolicy manifests: default-deny and explicit allow (ingress TCP 8443, unrestricted egress) for openshift-controller-manager, route-controller-manager, and openshift-controller-manager-operator; and registers four controller-manager-related assets in the operator's static resource list. 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)
📝 Coding Plan for PR comments
Comment |
|
@dusk125: This pull request references CNTRLPLANE-2905 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 epic 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/assets/openshift-controller-manager/networkpolicy-allow.yaml`:
- Around line 8-24: The NetworkPolicy named allow-controller-manager currently
permits ingress on port 8443 but lacks a source restriction; update the
spec.ingress of the allow-controller-manager NetworkPolicy to include a from
clause that limits traffic to the openshift-monitoring namespace (e.g., add a
from: - namespaceSelector with an appropriate label selector matching the
monitoring namespace) so only pods from openshift-monitoring can reach port 8443
on pods selected by spec.podSelector (app: openshift-controller-manager-a,
controller-manager: "true").
In
`@bindata/assets/openshift-controller-manager/route-controller-manager-networkpolicy-allow.yaml`:
- Around line 8-24: The NetworkPolicy named allow-route-controller-manager in
namespace openshift-route-controller-manager currently permits ingress to pods
matching labels app: route-controller-manager and route-controller-manager:
"true" on TCP port 8443 from any source; update the spec.ingress entry to
include a from block that restricts sources to the openshift-monitoring
namespace (use namespaceSelector with matchLabels or metadata.name selector for
the monitoring namespace) so Prometheus alone can scrape metrics, or if open
access is intended, change the comment above to say it allows ingress from any
source rather than from openshift-monitoring.
In
`@manifests/0000_25_openshift-controller-manager-operator_01_network-policy-operator.yaml`:
- Around line 8-26: The doc/comment claims ingress is limited "from
openshift-monitoring" but the NetworkPolicy allow-operator (namespace
openshift-controller-manager-operator, podSelector app:
openshift-controller-manager-operator) currently has no from block and therefore
allows all sources; fix by adding a from block to the spec.ingress that
restricts traffic to the openshift-monitoring namespace (e.g., add a from: -
namespaceSelector: matchLabels: kubernetes.io/metadata.name:
openshift-monitoring) so the port: 8443 rule truly only allows Prometheus from
that namespace, or alternatively update the comment to accurately state that the
rule allows all sources—choose one and make the change in the NetworkPolicy
manifest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2eea60ce-ba75-46c4-9a24-3663c5f3bbdd
📒 Files selected for processing (7)
bindata/assets/openshift-controller-manager/networkpolicy-allow.yamlbindata/assets/openshift-controller-manager/networkpolicy-default-deny.yamlbindata/assets/openshift-controller-manager/route-controller-manager-networkpolicy-allow.yamlbindata/assets/openshift-controller-manager/route-controller-manager-networkpolicy-default-deny.yamlmanifests/0000_25_openshift-controller-manager-operator_01_network-policy-default-deny.yamlmanifests/0000_25_openshift-controller-manager-operator_01_network-policy-operator.yamlpkg/operator/starter.go
| # Ingress: | ||
| # - Allow ingress on port 8443 (metrics) from the openshift-monitoring namespace | ||
| # so that Prometheus can scrape metrics from the controller-manager pods. | ||
| apiVersion: networking.k8s.io/v1 | ||
| kind: NetworkPolicy | ||
| metadata: | ||
| name: allow-controller-manager | ||
| namespace: openshift-controller-manager | ||
| spec: | ||
| podSelector: | ||
| matchLabels: | ||
| app: openshift-controller-manager-a | ||
| controller-manager: "true" | ||
| ingress: | ||
| - ports: | ||
| - protocol: TCP | ||
| port: 8443 |
There was a problem hiding this comment.
Same issue: comment mentions restricted source but ingress allows all.
Lines 8-10 claim ingress is "from the openshift-monitoring namespace", but the ingress rule has no from selector. This allows any source to connect to port 8443. Same fix as the route-controller-manager policy if restriction is intended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindata/assets/openshift-controller-manager/networkpolicy-allow.yaml` around
lines 8 - 24, The NetworkPolicy named allow-controller-manager currently permits
ingress on port 8443 but lacks a source restriction; update the spec.ingress of
the allow-controller-manager NetworkPolicy to include a from clause that limits
traffic to the openshift-monitoring namespace (e.g., add a from: -
namespaceSelector with an appropriate label selector matching the monitoring
namespace) so only pods from openshift-monitoring can reach port 8443 on pods
selected by spec.podSelector (app: openshift-controller-manager-a,
controller-manager: "true").
| # Ingress: | ||
| # - Allow ingress on port 8443 (metrics) from the openshift-monitoring namespace | ||
| # so that Prometheus can scrape metrics from the route-controller-manager pods. | ||
| apiVersion: networking.k8s.io/v1 | ||
| kind: NetworkPolicy | ||
| metadata: | ||
| name: allow-route-controller-manager | ||
| namespace: openshift-route-controller-manager | ||
| spec: | ||
| podSelector: | ||
| matchLabels: | ||
| app: route-controller-manager | ||
| route-controller-manager: "true" | ||
| ingress: | ||
| - ports: | ||
| - protocol: TCP | ||
| port: 8443 |
There was a problem hiding this comment.
Comment claims restricted source but ingress rule allows all sources.
The comment on lines 8-10 states ingress is allowed "from the openshift-monitoring namespace", but the ingress rule at lines 21-24 has no from selector, meaning it allows TCP 8443 from any source in the cluster.
If the intent is to restrict to Prometheus only, add a namespace selector:
🔒 Proposed fix to restrict ingress source
ingress:
+ - from:
+ - namespaceSelector:
+ matchLabels:
+ kubernetes.io/metadata.name: openshift-monitoring
ports:
- protocol: TCP
port: 8443If allowing from all sources is intentional, update the comment to reflect that.
📝 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.
| # Ingress: | |
| # - Allow ingress on port 8443 (metrics) from the openshift-monitoring namespace | |
| # so that Prometheus can scrape metrics from the route-controller-manager pods. | |
| apiVersion: networking.k8s.io/v1 | |
| kind: NetworkPolicy | |
| metadata: | |
| name: allow-route-controller-manager | |
| namespace: openshift-route-controller-manager | |
| spec: | |
| podSelector: | |
| matchLabels: | |
| app: route-controller-manager | |
| route-controller-manager: "true" | |
| ingress: | |
| - ports: | |
| - protocol: TCP | |
| port: 8443 | |
| # Ingress: | |
| # - Allow ingress on port 8443 (metrics) from the openshift-monitoring namespace | |
| # so that Prometheus can scrape metrics from the route-controller-manager pods. | |
| apiVersion: networking.k8s.io/v1 | |
| kind: NetworkPolicy | |
| metadata: | |
| name: allow-route-controller-manager | |
| namespace: openshift-route-controller-manager | |
| spec: | |
| podSelector: | |
| matchLabels: | |
| app: route-controller-manager | |
| route-controller-manager: "true" | |
| ingress: | |
| - from: | |
| - namespaceSelector: | |
| matchLabels: | |
| kubernetes.io/metadata.name: openshift-monitoring | |
| ports: | |
| - protocol: TCP | |
| port: 8443 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bindata/assets/openshift-controller-manager/route-controller-manager-networkpolicy-allow.yaml`
around lines 8 - 24, The NetworkPolicy named allow-route-controller-manager in
namespace openshift-route-controller-manager currently permits ingress to pods
matching labels app: route-controller-manager and route-controller-manager:
"true" on TCP port 8443 from any source; update the spec.ingress entry to
include a from block that restricts sources to the openshift-monitoring
namespace (use namespaceSelector with matchLabels or metadata.name selector for
the monitoring namespace) so Prometheus alone can scrape metrics, or if open
access is intended, change the comment above to say it allows ingress from any
source rather than from openshift-monitoring.
| # Ingress: | ||
| # - Allow ingress on port 8443 (metrics) from the openshift-monitoring namespace | ||
| # so that Prometheus can scrape metrics from the operator. | ||
| apiVersion: networking.k8s.io/v1 | ||
| kind: NetworkPolicy | ||
| metadata: | ||
| name: allow-operator | ||
| namespace: openshift-controller-manager-operator | ||
| annotations: | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| include.release.openshift.io/single-node-developer: "true" | ||
| spec: | ||
| podSelector: | ||
| matchLabels: | ||
| app: openshift-controller-manager-operator | ||
| ingress: | ||
| - ports: | ||
| - protocol: TCP | ||
| port: 8443 |
There was a problem hiding this comment.
Same documentation inconsistency: comment says "from openshift-monitoring" but rule allows all sources.
The comment claims ingress is restricted to the openshift-monitoring namespace, but the ingress rule at lines 23-26 has no from selector. This is consistent with the other allow policies but the documentation should match the implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@manifests/0000_25_openshift-controller-manager-operator_01_network-policy-operator.yaml`
around lines 8 - 26, The doc/comment claims ingress is limited "from
openshift-monitoring" but the NetworkPolicy allow-operator (namespace
openshift-controller-manager-operator, podSelector app:
openshift-controller-manager-operator) currently has no from block and therefore
allows all sources; fix by adding a from block to the spec.ingress that
restricts traffic to the openshift-monitoring namespace (e.g., add a from: -
namespaceSelector: matchLabels: kubernetes.io/metadata.name:
openshift-monitoring) so the port: 8443 rule truly only allows Prometheus from
that namespace, or alternatively update the comment to accurately state that the
rule allows all sources—choose one and make the change in the NetworkPolicy
manifest.
|
/payload 4.22 nightly blocking |
|
@dusk125: trigger 14 job(s) of type blocking for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a53ecd80-1cb9-11f1-955c-2264a6e7c378-0 trigger 65 job(s) of type informing for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a53ecd80-1cb9-11f1-955c-2264a6e7c378-1 |
|
@LiangquanLi930 Could you please help approve , thanks . |
manifests/0000_25_openshift-controller-manager_01_network-policy-controller-manager.yaml
Outdated
Show resolved
Hide resolved
|
@dusk125: This pull request references CNTRLPLANE-2905 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 epic 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. |
|
/label tide/merge-method-squash |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
bindata/assets/openshift-controller-manager/route-controller-manager-networkpolicy-allow.yaml (1)
8-23:⚠️ Potential issue | 🟠 MajorRestrict metrics ingress or fix the comment.
The comment says port
8443is only opened toopenshift-monitoring, but this rule has nofromselector, so it allows ingress on8443from all sources. That leaves the metrics endpoint broader than intended.🔒 Proposed fix
ingress: - - ports: + - from: + - namespaceSelector: + matchLabels: + kubernetes.io/metadata.name: openshift-monitoring + ports: - protocol: TCP port: 8443If open access is intentional, update Lines 9-10 to say that explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindata/assets/openshift-controller-manager/route-controller-manager-networkpolicy-allow.yaml` around lines 8 - 23, The NetworkPolicy named allow-route-controller-manager in namespace openshift-route-controller-manager currently allows ingress to pods with label route-controller-manager on TCP port 8443 from any source, contradicting the comment that it should only allow openshift-monitoring; either restrict the rule by adding a from clause that selects the openshift-monitoring namespace (e.g., namespaceSelector matching metadata.name: openshift-monitoring or a suitable podSelector) to limit ingress to Prometheus, or if open access was intended, update the comment to state that port 8443 is intentionally open to all sources.bindata/assets/openshift-controller-manager/networkpolicy-allow.yaml (1)
8-23:⚠️ Potential issue | 🟠 MajorRestrict metrics ingress or fix the comment.
Same issue here: Lines 9-10 say metrics ingress is from
openshift-monitoring, but the rule has nofromclause, so port8443is reachable from all sources.🔒 Proposed fix
ingress: - - ports: + - from: + - namespaceSelector: + matchLabels: + kubernetes.io/metadata.name: openshift-monitoring + ports: - protocol: TCP port: 8443If cluster-wide access is intended, please make the comment match the actual policy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindata/assets/openshift-controller-manager/networkpolicy-allow.yaml` around lines 8 - 23, The NetworkPolicy named allow-controller-manager in namespace openshift-controller-manager currently exposes port 8443 to all sources (spec.ingress has no from clause) while the comment says it should allow ingress only from the openshift-monitoring namespace; either update the comment to reflect cluster-wide access or restrict the rule by adding a from clause under spec.ingress that limits traffic to pods in the openshift-monitoring namespace (e.g., a namespaceSelector matching metadata.name: openshift-monitoring or a podSelector as appropriate) so the policy behavior matches the documented intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@bindata/assets/openshift-controller-manager/networkpolicy-allow.yaml`:
- Around line 8-23: The NetworkPolicy named allow-controller-manager in
namespace openshift-controller-manager currently exposes port 8443 to all
sources (spec.ingress has no from clause) while the comment says it should allow
ingress only from the openshift-monitoring namespace; either update the comment
to reflect cluster-wide access or restrict the rule by adding a from clause
under spec.ingress that limits traffic to pods in the openshift-monitoring
namespace (e.g., a namespaceSelector matching metadata.name:
openshift-monitoring or a podSelector as appropriate) so the policy behavior
matches the documented intent.
In
`@bindata/assets/openshift-controller-manager/route-controller-manager-networkpolicy-allow.yaml`:
- Around line 8-23: The NetworkPolicy named allow-route-controller-manager in
namespace openshift-route-controller-manager currently allows ingress to pods
with label route-controller-manager on TCP port 8443 from any source,
contradicting the comment that it should only allow openshift-monitoring; either
restrict the rule by adding a from clause that selects the openshift-monitoring
namespace (e.g., namespaceSelector matching metadata.name: openshift-monitoring
or a suitable podSelector) to limit ingress to Prometheus, or if open access was
intended, update the comment to state that port 8443 is intentionally open to
all sources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b517a00-bb3a-41e6-87b8-534a9184167e
📒 Files selected for processing (2)
bindata/assets/openshift-controller-manager/networkpolicy-allow.yamlbindata/assets/openshift-controller-manager/route-controller-manager-networkpolicy-allow.yaml
openshift-ci
bot
added
the
tide/merge-method-squash
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dusk125, liouk The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@dusk125: 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. |
4 participants
Adds NetworkPolicy resources for both operator and operand namespaces
Summary by CodeRabbit