MCO-1944: Add NetworkPolicy Test case#6283
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughThis PR adds a disruptive serial Ginkgo suite that provisions a ChangesMCO network policy connectivity coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant NetTestPod
participant MCCPod
participant MCOPod
participant KubernetesAPI
TestSuite->>NetTestPod: create and wait for readiness
NetTestPod->>KubernetesAPI: fetch /healthz
KubernetesAPI-->>NetTestPod: ok
TestSuite->>MCCPod: test access to nettest:8080
MCCPod-->>TestSuite: connection failure
NetTestPod->>MCCPod: request :9001/metrics
MCCPod-->>NetTestPod: Unauthorized
NetTestPod->>MCCPod: connect to :8797
MCCPod-->>NetTestPod: connection failure
MCOPod->>KubernetesAPI: fetch /healthz
KubernetesAPI-->>MCOPod: ok
Suggested reviewers: 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ptalgulk01 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
|
/retest |
ptalgulk01
force-pushed
the
add-networpolicy-testcase
branch
from
fc41c29 to
7c16b3e
Compare
ptalgulk01
changed the title
|
@ptalgulk01: This pull request references MCO-1944 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 "5.0.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
![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
🧹 Nitpick comments (1)
test/extended-priv/mco_network_policies.go (1)
61-64: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winExternal dependency on google.com will be flaky in disconnected/restricted CI.
Reaching
https://google.com(and asserting theMovedredirect body) makes this step dependent on outbound internet and Google's response text. On disconnected or egress-restricted clusters this fails for reasons unrelated to theallow-all-egresspolicy. Consider validating egress against an in-cluster or otherwise guaranteed endpoint, or gate this on a connectivity precheck.🤖 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 `@test/extended-priv/mco_network_policies.go` around lines 61 - 64, The egress verification in the test pod currently depends on google.com and a specific redirect response, which is flaky in restricted or disconnected CI. Update the check in the test body around exutil.RemoteShPod to use a guaranteed in-cluster or prevalidated endpoint instead of external internet, or add a connectivity precheck before running it. Keep the assertion tied to the allow-all-egress validation by referencing the same test step and RemoteShPod-based curl call.
🤖 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 `@test/extended-priv/mco_network_policies.go`:
- Around line 101-112: The ingress-block check in the MCC-to-test-pod path is
currently a false positive because the test pod only runs sleep and nothing
listens on port 8080. Update the logic around getMachineConfigControllerPod,
NewNamespacedResource, and the curl call in the TEST 3 block so the target pod
runs a simple HTTP server (or another reachable listener) before asserting the
connection is denied, and keep a positive control that confirms the listener
works when not blocked. This ensures the RemoteShContainer failure actually
validates the NetworkPolicy ingress restriction rather than an unopened port.
- Around line 124-132: The port checks in the extended-priv MCO network policy
test are hitting ports that the MCC pod does not actually listen on, so the
failures don’t prove NetworkPolicy is working. Update the probes in the test
that uses exutil.RemoteShPod and mccPodIP to target a real exposed listener,
such as the kube-rbac-proxy metrics port or the Service endpoint, so the
assertions verify network blocking rather than closed ports.
---
Nitpick comments:
In `@test/extended-priv/mco_network_policies.go`:
- Around line 61-64: The egress verification in the test pod currently depends
on google.com and a specific redirect response, which is flaky in restricted or
disconnected CI. Update the check in the test body around exutil.RemoteShPod to
use a guaranteed in-cluster or prevalidated endpoint instead of external
internet, or add a connectivity precheck before running it. Keep the assertion
tied to the allow-all-egress validation by referencing the same test step and
RemoteShPod-based curl call.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1d9be135-55b7-4eb3-b963-051f4cb10fac
📒 Files selected for processing (1)
test/extended-priv/mco_network_policies.go
| exutil.By("TEST 3: Verify MCC pod cannot reach test pod (test pod ingress is blocked)") | ||
| mccPodName, err := getMachineConfigControllerPod(oc.AsAdmin()) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Failed to get MCC pod") | ||
| o.Expect(mccPodName).NotTo(o.BeEmpty(), "MCC pod not found") | ||
|
|
||
| testPod := NewNamespacedResource(oc.AsAdmin(), "pod", MachineConfigNamespace, testPodName) | ||
| testPodIP := testPod.GetOrFail("{.status.podIP}") | ||
|
|
||
| // Try to reach test pod from MCC (should fail due to ingress block) | ||
| _, err = exutil.RemoteShContainer(oc.AsAdmin(), MachineConfigNamespace, mccPodName, "machine-config-controller", "curl", "-sk", "--connect-timeout", "5", fmt.Sprintf("http://%s:8080", testPodIP)) | ||
| o.Expect(err).To(o.HaveOccurred(), "MCC pod should not be able to reach test pod") | ||
| logger.Infof("OK!\n") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Ingress-block assertion is a false positive. test/extended-priv/mco_network_policies.go:101-112 only starts sleep 36000, so nothing is listening on :8080; this curl can fail even if the NetworkPolicy is ineffective. Run a simple server in the pod, or add an allowed control, so the failure actually proves ingress is blocked.
🤖 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 `@test/extended-priv/mco_network_policies.go` around lines 101 - 112, The
ingress-block check in the MCC-to-test-pod path is currently a false positive
because the test pod only runs sleep and nothing listens on port 8080. Update
the logic around getMachineConfigControllerPod, NewNamespacedResource, and the
curl call in the TEST 3 block so the target pod runs a simple HTTP server (or
another reachable listener) before asserting the connection is denied, and keep
a positive control that confirms the listener works when not blocked. This
ensures the RemoteShContainer failure actually validates the NetworkPolicy
ingress restriction rather than an unopened port.
There was a problem hiding this comment.
It seems to be imporant, nothing is listening in the test pod.
There was a problem hiding this comment.
@sergiordlr Agreed — since the test pod is only running sleep 36000, there's no listener on port 8080, so the "MCC cannot reach test pod" assertion would pass even if the NetworkPolicy weren't blocking anything. That makes it a false positive rather than a real verification of the ingress block.
A simple fix would be to start a lightweight HTTP server as the pod's command instead of sleep, e.g.:
"--command", "--", "python3", "-c",
"import http.server,socketserver; s=socketserver.TCPServer(('0.0.0.0',8080), http.server.SimpleHTTPRequestHandler); s.serve_forever()",or with nc:
"--command", "--", "sh", "-c", "while true; do nc -lk -p 8080 -e /bin/true; done",(the exact command depends on what's available in registry.access.redhat.com/ubi9/ubi-minimal:latest — ubi-minimal doesn't ship nc or python3 by default, so you may need microdnf install first, or switch to an image that already includes a simple server, such as registry.access.redhat.com/ubi9/ubi-minimal with busybox, or a test-utility image already used elsewhere in this test suite).
Once the pod has an actual listener on 8080:
- The MCC → test-pod curl failure genuinely proves ingress is blocked.
- You could also add a positive control (e.g., verify a pod with an allow-ingress label can reach the same port) to further validate the policy is being enforced rather than the port simply being closed.
There was a problem hiding this comment.
I recommend checking presubmit tests ones, I see duplicate scenarios. this are already there policy existence, deletion blocked, update blocked, coexistence
|
|
||
| // Test-scoped variables | ||
| testPodName := "nettest" | ||
| testPodImage := "registry.access.redhat.com/ubi9/ubi-minimal:latest" |
There was a problem hiding this comment.
We should use images stored in quay.io/openshifttest since they will be cloned in disconnected clusters.
https://quay.io/organization/openshifttest
Can all checks be executed in a disconnected cluster?
If not, can we discriminate which check can be executed in a disconnected cluster and ignore the ones that can't? (it could be done by using IsDisconnectedCluster function)
We should tag the test as connected only if there are checks that cannot run in a disconnected env and we don't want to discriminate.
| _ = oc.AsAdmin().WithoutNamespace().Run("delete").Args("pod", "-n", MachineConfigNamespace, testPodName, "--ignore-not-found=true").Execute() | ||
| }() |
There was a problem hiding this comment.
In openshift-test-private in the workflow.go file we created a Pod resource and a Job resource.
The idea was to abstract everything involving pods and jobs and encapsulate everything in there.
It looks like that those resources haven't already been migrated because they will be migrated with the ocb test cases.
Nevertheless, we should try to continue with this idea, and encapsulate everything regarding pods inside the Pod resource. Even if we make the same call to exutils.RemoteShPod, for example. At least everything will be encapsulated and will be easier to use and maintain.
| "--command", "--", "sleep", "36000", | ||
| ).Execute() | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Failed to create test pod") | ||
| exutil.AssertPodToBeReady(oc, testPodName, MachineConfigNamespace) |
There was a problem hiding this comment.
Once we use a Pod resource we can start using our gomega matches like "HaveCondition("Ready", "status", "true") to check the resources.
| mcoComponents := []string{"allow-machine-config-controller", "allow-machine-config-operator", "allow-machine-os-builder"} | ||
| for _, component := range mcoComponents { | ||
| np := NewNamespacedResource(oc.AsAdmin(), "networkpolicy", MachineConfigNamespace, component) | ||
| ingressSpec := np.GetOrFail("{.spec.ingress}") | ||
| o.Expect(ingressSpec).To(o.ContainSubstring("9001"), "%s does not allow port 9001", component) | ||
| o.Expect(ingressSpec).To(o.ContainSubstring("TCP"), "%s does not specify TCP protocol", component) | ||
| } |
There was a problem hiding this comment.
If we verify that 9001 is present in the MCO ones, it means that we need to check that it is not present in the rest, right?
| exutil.By("TEST 3: Verify MCC pod cannot reach test pod (test pod ingress is blocked)") | ||
| mccPodName, err := getMachineConfigControllerPod(oc.AsAdmin()) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Failed to get MCC pod") | ||
| o.Expect(mccPodName).NotTo(o.BeEmpty(), "MCC pod not found") | ||
|
|
||
| testPod := NewNamespacedResource(oc.AsAdmin(), "pod", MachineConfigNamespace, testPodName) | ||
| testPodIP := testPod.GetOrFail("{.status.podIP}") | ||
|
|
||
| // Try to reach test pod from MCC (should fail due to ingress block) | ||
| _, err = exutil.RemoteShContainer(oc.AsAdmin(), MachineConfigNamespace, mccPodName, "machine-config-controller", "curl", "-sk", "--connect-timeout", "5", fmt.Sprintf("http://%s:8080", testPodIP)) | ||
| o.Expect(err).To(o.HaveOccurred(), "MCC pod should not be able to reach test pod") | ||
| logger.Infof("OK!\n") |
There was a problem hiding this comment.
It seems to be imporant, nothing is listening in the test pod.
| _, err = exutil.RemoteShPod(oc.AsAdmin(), MachineConfigNamespace, testPodName, "curl", "-sk", "--connect-timeout", "5", fmt.Sprintf("https://%s:8443/metrics", mccPodIP)) | ||
| o.Expect(err).To(o.HaveOccurred(), "Port 8443 should be blocked") | ||
| logger.Infof("OK!\n") | ||
|
|
||
| // Test port 443 (should fail - connection blocked) | ||
| _, err = exutil.RemoteShPod(oc.AsAdmin(), MachineConfigNamespace, testPodName, "curl", "-sk", "--connect-timeout", "5", fmt.Sprintf("https://%s:443", mccPodIP)) | ||
| o.Expect(err).To(o.HaveOccurred(), "Port 443 should be blocked") | ||
| logger.Infof("OK!\n") |
There was a problem hiding this comment.
Is the MCC pod actually listening in the those ports?
sh-5.1$ ss -tulnp
Netid State Recv-Q Send-Q Local Address:Port Peer Address:Port Process
tcp LISTEN 0 4096 127.0.0.1:8797 0.0.0.0:* users:(("machine-config-",pid=1,fd=7))
tcp LISTEN 0 4096 *:9001 *:*
|
|
||
| exutil.By("TEST 5: Verify VAP blocks deletion of all 5 managed policies") | ||
| for _, netpol := range expectedNetPols { | ||
| err = oc.AsAdmin().WithoutNamespace().Run("delete").Args("networkpolicy", "-n", MachineConfigNamespace, netpol).Execute() |
There was a problem hiding this comment.
Let's avoid using "oc....Run..." commands. Lets use a NamespacedResourceList
It seems that ResourceList structs don't have a Delete method, let's add it so that it can be reused in the future.
| {"allow-all-egress", `{"spec":{"egress":[{"ports":[{"port":443,"protocol":"TCP"}]}]}}`}, | ||
| } | ||
| for _, tc := range patchTests { | ||
| err = oc.AsAdmin().WithoutNamespace().Run("patch").Args("networkpolicy", tc.name, "-n", MachineConfigNamespace, "--type=merge", "-p", tc.patch).Execute() |
There was a problem hiding this comment.
Let's avoid using "oc....Run" commands, let's use a NamespacedResource and use the patch method.
|
Summary of changes made to address review comments:
Re: Line 110 (test pod ingress test): |
ptalgulk01
force-pushed
the
add-networpolicy-testcase
branch
from
7c16b3e to
10352e7
Compare
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 `@test/extended-priv/mco_network_policies.go`:
- Around line 74-77: Replace broad HaveOccurred assertions in both
network-policy checks at test/extended-priv/mco_network_policies.go lines 74-77
and 91-94 with validation that the connection fails specifically due to a
timeout, or expose a real listener on the test pod for the port-8080 check and
retain equivalent timeout validation. Ensure both tests cannot pass because the
target port is merely closed or refuses connections.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d133fa09-4ae2-4bcc-8f18-4dc52239258c
📒 Files selected for processing (2)
test/extended-priv/mco_network_policies.gotest/extended-priv/resource.go
| // Try to reach test pod from MCC (should fail due to default-deny ingress block) | ||
| _, err = exutil.RemoteShContainer(oc.AsAdmin(), MachineConfigNamespace, mccPodName, "machine-config-controller", "wget", "-q", "-O-", "--timeout=5", fmt.Sprintf("http://%s:8080", testPodIP)) | ||
| o.Expect(err).To(o.HaveOccurred(), "MCC pod should not be able to reach test pod due to default-deny") | ||
| logger.Infof("OK!\n") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
NetworkPolicy assertions are false positives due to broad error matching on closed ports. Both of these tests intend to verify that a NetworkPolicy drops traffic, but because they use o.Expect(err).To(o.HaveOccurred()), they will pass on any connection failure, including "Connection refused" when a port is closed. The target ports are not actually accessible (the test pod has no listener on 8080, and the MCC pod only binds 8797 to 127.0.0.1), so these tests will pass even if the NetworkPolicy is completely missing.
test/extended-priv/mco_network_policies.go#L74-L77: run a listener on the test pod (e.g., a simple python/nc server instead ofsleep 36000), or explicitly assert thaterrcontains a connection timeout message rather than "Connection refused".test/extended-priv/mco_network_policies.go#L91-L94: assert that the error is specifically a timeout. Since 8797 is bound to127.0.0.1, you should either pick a port exposed on the Pod IP or rely on the timeout string so the test doesn't pass for the wrong reason.
📍 Affects 1 file
test/extended-priv/mco_network_policies.go#L74-L77(this comment)test/extended-priv/mco_network_policies.go#L91-L94
🤖 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 `@test/extended-priv/mco_network_policies.go` around lines 74 - 77, Replace
broad HaveOccurred assertions in both network-policy checks at
test/extended-priv/mco_network_policies.go lines 74-77 and 91-94 with validation
that the connection fails specifically due to a timeout, or expose a real
listener on the test pod for the port-8080 check and retain equivalent timeout
validation. Ensure both tests cannot pass because the target port is merely
closed or refuses connections.
|
@ptalgulk01: 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. |
| logger "github.com/openshift/machine-config-operator/test/extended-priv/util/logext" | ||
| ) | ||
|
|
||
| var _ = g.Describe("[sig-mco][Suite:openshift/machine-config-operator/security][Serial][Disruptive] MCO Network Policies", func() { |
There was a problem hiding this comment.
Where's our openshift/machine-config-operator/security suite?
5 participants
Test Coverage:
allow-machine-config-controller, allow-machine-config-operator,
allow-machine-os-builder, default-deny)
Implementation Details:
getMachineConfigOperatorPod)
MachineConfigOperatorPodSelector constants to const.go
Summary by CodeRabbit
/healthzreachability, and expected blocked/unreachable connectivity between MCO components and the test pod.