CNTRLPLANE-2698: add network policies for apiserver operator and operands#2029
CNTRLPLANE-2698: add network policies for apiserver operator and operands#2029dusk125 wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@dusk125: This pull request references API-1631 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds four new Kubernetes NetworkPolicy manifests (two under bindata and two under manifests) to enforce default-deny posture and specific allow rules (egress and metrics ingress), and updates the operator starter to include the two operand networkpolicy assets in its static resources list. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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
do-not-merge/work-in-progress
|
/retitle WIP: CNTRLPLANE-2698: add network policies for apiserver operator and operands |
openshift-ci
bot
changed the title
|
@dusk125: This pull request references CNTRLPLANE-2698 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dusk125 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 |
manifests/0000_20_kube-apiserver-operator_11_networkpolicies.yaml
Outdated
Show resolved
Hide resolved
|
@dusk125: This pull request references CNTRLPLANE-2698 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. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@manifests/0000_20_kube-apiserver-operator_11_networkpolicies.yaml`:
- Around line 52-56: The NetworkPolicy named allow-to-apiserver currently uses
podSelector: {} and egress: - {} which permits all pods unrestricted outbound
traffic; change it to target only operator pods by replacing the empty
podSelector with the selector used elsewhere (matchLabels: {app:
kube-apiserver-operator}) so only pods labeled app: kube-apiserver-operator get
the broad egress; keep the egress rules as-is but ensure the policyTypes and
name (allow-to-apiserver) remain unchanged to preserve intent and ordering with
default-deny semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f064883c-cbdc-4ddb-a70c-03bd760315e5
📒 Files selected for processing (2)
manifests/0000_12_kube-apiserver-operand_networkpolicies.yamlmanifests/0000_20_kube-apiserver-operator_11_networkpolicies.yaml
| podSelector: {} | ||
| egress: | ||
| - {} | ||
| policyTypes: | ||
| - Egress |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the file
file=$(fd '0000_20_kube-apiserver-operator_11_networkpolicies.yaml' | head -n1)
echo "Found file: $file"
echo "=== File structure and lines 52-56 ==="
sed -n '45,60p' "$file" | cat -n
echo ""
echo "=== Full context for allow-to-apiserver policy ==="
rg -n -B5 -A10 'name:\s*allow-to-apiserver' "$file"
echo ""
echo "=== Check for app labels in kube-apiserver-operator deployments ==="
fd '.*kube-apiserver-operator.*\.yaml' | head -5 | while read f; do
if rg -q 'app.*kube-apiserver-operator' "$f"; then
echo "Found label in: $f"
rg -B2 -A2 'app.*kube-apiserver-operator' "$f" | head -10
fi
doneRepository: openshift/cluster-kube-apiserver-operator
Length of output: 1470
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="manifests/0000_20_kube-apiserver-operator_11_networkpolicies.yaml"
echo "=== FULL FILE CONTENT ==="
cat -n "$file"
echo ""
echo "=== Check for default-deny policy ==="
rg -n 'name:.*default-deny|name:.*deny' "$file"
echo ""
echo "=== Check deployments for app label ==="
fd 'deployment.*\.yaml' | xargs rg -l 'kube-apiserver-operator' | head -3 | while read f; do
echo "File: $f"
rg -C3 'app.*kube-apiserver-operator|labels:' "$f" | head -20
doneRepository: openshift/cluster-kube-apiserver-operator
Length of output: 6621
allow-to-apiserver allows all egress for all pods in the namespace, including those that don't need it.
The policy uses podSelector: {} with egress: - {}, which selects every pod and permits unrestricted outbound traffic. While the intent stated in the file (lines 41–42) is to handle variable API server endpoints, this blanket allowance applies to every workload in the namespace, not just the operator. The default-deny policy cannot override it due to NetworkPolicy additive semantics—once any policy permits traffic, it is allowed.
The namespace contains an app: kube-apiserver-operator label (confirmed in the deployment) that is already used in allow-to-kube-apiserver and allow-to-metrics policies in the same file. Scoping allow-to-apiserver to only operator pods aligns with least-privilege and the pattern established by the other policies.
Proposed tightening
spec:
- podSelector: {}
+ podSelector:
+ matchLabels:
+ app: kube-apiserver-operator
egress:
- {}
policyTypes:
- Egress📝 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.
| podSelector: {} | |
| egress: | |
| - {} | |
| policyTypes: | |
| - Egress | |
| podSelector: | |
| matchLabels: | |
| app: kube-apiserver-operator | |
| egress: | |
| - {} | |
| policyTypes: | |
| - Egress |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@manifests/0000_20_kube-apiserver-operator_11_networkpolicies.yaml` around
lines 52 - 56, The NetworkPolicy named allow-to-apiserver currently uses
podSelector: {} and egress: - {} which permits all pods unrestricted outbound
traffic; change it to target only operator pods by replacing the empty
podSelector with the selector used elsewhere (matchLabels: {app:
kube-apiserver-operator}) so only pods labeled app: kube-apiserver-operator get
the broad egress; keep the egress rules as-is but ensure the policyTypes and
name (allow-to-apiserver) remain unchanged to preserve intent and ordering with
default-deny semantics.
dusk125
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/retest-required |
1 similar comment
|
/retest-required |
manifests/0000_20_kube-apiserver-operator_11_networkpolicies.yaml
Outdated
Show resolved
Hide resolved
|
@dusk125: This pull request references CNTRLPLANE-2698 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
manifests/0000_20_kube-apiserver-operator_networkpolicy-allow.yaml (1)
24-27: Consider restricting ingress source for metrics (optional).The ingress rule allows any pod from any namespace to connect to port 8443. While this is flexible for Prometheus scraping across different cluster configurations, a more restrictive approach could limit sources to the monitoring namespace if known:
ingress: - from: - namespaceSelector: matchLabels: name: openshift-monitoring ports: - protocol: TCP port: 8443If the monitoring namespace varies across deployments, the current open approach is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@manifests/0000_20_kube-apiserver-operator_networkpolicy-allow.yaml` around lines 24 - 27, The ingress rule currently allows unrestricted sources to connect to port 8443; update the ingress block (the ingress entry that contains ports: - protocol: TCP port: 8443) to restrict sources by adding a from: section with a namespaceSelector that matches the monitoring namespace (for example, namespaceSelector: matchLabels: name: openshift-monitoring) so only the monitoring namespace can scrape metrics; if the monitoring namespace is variable, make this change conditional or document why the open ingress must remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@manifests/0000_20_kube-apiserver-operator_networkpolicy-allow.yaml`:
- Around line 24-27: The ingress rule currently allows unrestricted sources to
connect to port 8443; update the ingress block (the ingress entry that contains
ports: - protocol: TCP port: 8443) to restrict sources by adding a from: section
with a namespaceSelector that matches the monitoring namespace (for example,
namespaceSelector: matchLabels: name: openshift-monitoring) so only the
monitoring namespace can scrape metrics; if the monitoring namespace is
variable, make this change conditional or document why the open ingress must
remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e25cb21d-26c5-4dda-ab44-006991d48a4c
📒 Files selected for processing (9)
bindata/assets/kube-apiserver/networkpolicy-operand-allow.yamlbindata/assets/kube-apiserver/networkpolicy-operand-default-deny.yamlbindata/assets/kube-apiserver/networkpolicy-operator-allow.yamlbindata/assets/kube-apiserver/networkpolicy-operator-default-deny.yamlmanifests/0000_12_kube-apiserver-operand_networkpolicy-allow.yamlmanifests/0000_12_kube-apiserver-operand_networkpolicy-default-deny.yamlmanifests/0000_20_kube-apiserver-operator_networkpolicy-allow.yamlmanifests/0000_20_kube-apiserver-operator_networkpolicy-default-deny.yamlpkg/operator/starter.go
✅ Files skipped from review due to trivial changes (1)
- pkg/operator/starter.go
|
@dusk125: This pull request references CNTRLPLANE-2698 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. |
| - key: app | ||
| operator: In | ||
| values: | ||
| - guard |
There was a problem hiding this comment.
Do guard pods actually need any network access? I've come across it for another component and realized that they don't for that component.
There was a problem hiding this comment.
I thought they did requests to the readyz endpoint of their target pod to know when/if it dies
|
/retest-required |
|
@dusk125: The following test 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. |
|
/retest-required |
|
/label tide/merge-method-squash |
openshift-ci
bot
added
the
tide/merge-method-squash
|
/retest-required |
|
/label remove tide/merge-method-squash |
|
@dusk125: This pull request references CNTRLPLANE-2698 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. |
3 participants
Adds NetworkPolicy resources for both operator and operand namespaces:
Note: kube-apiserver static pods use
hostNetwork: trueand bypass NetworkPolicy entirely. I'm including a default-deny here anyway as a point of documentation and (big if) that were to ever change in the future, we're still locked down.