Fix DRA NetworkPolicy: allow API server and DNS egress#1836
Conversation
✅ Deploy Preview for openshift-kmm ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @abyrne55. Thanks for your PR. I'm waiting for a rh-ecosystem-edge member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
Walkthrough
ChangesDRA Network Policy Egress
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
abyrne55
force-pushed
the
fix/dra-intree-nodeselector
branch
from
aec61d2 to
97a5983
Compare
abyrne55
changed the title
abyrne55
force-pushed
the
fix/dra-intree-nodeselector
branch
from
97a5983 to
45def6e
Compare
abyrne55
force-pushed
the
fix/dra-intree-nodeselector
branch
from
45def6e to
db6aa1e
Compare
abyrne55
changed the title
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/networkpolicy/networkpolicy.go`:
- Around line 204-206: Update the network policy test expectation to match
DRANetworkPolicy’s current egress behavior: the test in networkpolicy_test.go
should no longer assert that result.Spec.Egress is empty, and instead should
verify that it contains a single empty networkingv1.NetworkPolicyEgressRule
representing allow-all egress. Use the DRANetworkPolicy symbol and the egress
assertion in the test to locate the check and adjust it accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5ad72af-3005-4408-b3ab-02156ea854c5
📒 Files selected for processing (1)
internal/networkpolicy/networkpolicy.go
| Egress: []networkingv1.NetworkPolicyEgressRule{ | ||
| {}, // Allow all egress — DRA kubelet plugin needs API server access for ResourceSlice management | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the DRANetworkPolicy test and its Egress assertions
fd -t f 'networkpolicy_test.go' internal/networkpolicy --exec rg -n -C2 'Spec.Egress' {}Repository: rh-ecosystem-edge/kernel-module-management
Length of output: 823
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant portion of the test file with names and assertions
sed -n '150,235p' internal/networkpolicy/networkpolicy_test.go | cat -n
printf '\n---\n'
# Inspect the implementation around the allow-all egress rule
sed -n '180,225p' internal/networkpolicy/networkpolicy.go | cat -nRepository: rh-ecosystem-edge/kernel-module-management
Length of output: 5099
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the DRANetworkPolicy test and show its surrounding assertions
rg -n -C6 'DRANetworkPolicy|draNetworkPolicy' internal/networkpolicy/networkpolicy_test.goRepository: rh-ecosystem-edge/kernel-module-management
Length of output: 1340
Update the DRA network policy test expectation
internal/networkpolicy/networkpolicy_test.go still asserts Expect(result.Spec.Egress).To(BeEmpty()), but DRANetworkPolicy now returns a single empty egress rule. Assert the allow-all egress rule instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/networkpolicy/networkpolicy.go` around lines 204 - 206, Update the
network policy test expectation to match DRANetworkPolicy’s current egress
behavior: the test in networkpolicy_test.go should no longer assert that
result.Spec.Egress is empty, and instead should verify that it contains a single
empty networkingv1.NetworkPolicyEgressRule representing allow-all egress. Use
the DRANetworkPolicy symbol and the egress assertion in the test to locate the
check and adjust it accordingly.
db6aa1e to
4c2f775
Compare
abyrne55
changed the title
|
@abyrne55 Great catch! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abyrne55, TomerNewman 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 |
|
/retest |
|
@abyrne55 the PR is good, you just need to run "make fmt" and "make lint", and push the changes in order for the ci/prow/lint job to pass |
The DRA NetworkPolicy declares both Ingress and Egress policy types but has no egress rules, which blocks all outbound traffic from the DRA kubelet plugin pod. This prevents the plugin from reaching the API server to create/update ResourceSlices. Allow egress to the API server (TCP 443) and DNS (TCP/UDP 53). Signed-off-by: Anthony Byrne <abyrne@redhat.com>
abyrne55
force-pushed
the
fix/dra-intree-nodeselector
branch
from
4c2f775 to
60fd872
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/networkpolicy/networkpolicy.go (1)
207-219: 🔒 Security & Privacy | 🔵 TrivialConsider scoping DRA egress
Aports-only egress rule matches all destinations, so this allows TCP 443 and TCP/UDP 53 to any endpoint. Narrowingtoto the API server and CoreDNS pods would reduce exposure where the cluster can express it; otherwise this is a common pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/networkpolicy/networkpolicy.go` around lines 207 - 219, The DRA egress rules in the NetworkPolicy are ports-only, so they currently allow TCP 443 and TCP/UDP 53 to any destination; update the policy-building logic in networkpolicy.go to scope these rules with explicit destination selectors where possible. In the code that constructs the DRA egress policy, add `to` restrictions for the API server and CoreDNS pods instead of relying on unrestricted `NetworkPolicyEgressRule` entries, while keeping the existing port definitions intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/networkpolicy/networkpolicy.go`:
- Around line 207-219: The DRA egress rules in the NetworkPolicy are ports-only,
so they currently allow TCP 443 and TCP/UDP 53 to any destination; update the
policy-building logic in networkpolicy.go to scope these rules with explicit
destination selectors where possible. In the code that constructs the DRA egress
policy, add `to` restrictions for the API server and CoreDNS pods instead of
relying on unrestricted `NetworkPolicyEgressRule` entries, while keeping the
existing port definitions intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf709d44-1ddf-48b1-b8df-73f1d550d927
📒 Files selected for processing (2)
internal/networkpolicy/networkpolicy.gointernal/networkpolicy/networkpolicy_test.go
|
/lgtm |
openshift-merge-bot
Bot
merged commit 3138a60
into
rh-ecosystem-edge:main
3 participants
The DRA NetworkPolicy (from edbb3a3) declares both Ingress and Egress policy types but has no egress rules, which blocks all outbound traffic from the DRA kubelet plugin pod. This prevents the plugin from reaching the API server to create/update ResourceSlices. This PR allows egress to the API server (TCP 443) and DNS (TCP/UDP 53).
This PR was written in part with the assistance of generative AI.
Summary by CodeRabbit