CORENET-7128: add network segmentation OTP test migration#3233
Open
anuragthehatter wants to merge 5 commits into
Open
CORENET-7128: add network segmentation OTP test migration#3233anuragthehatter wants to merge 5 commits into
anuragthehatter wants to merge 5 commits into
Conversation
|
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:
WalkthroughThis PR adds OTP network-segmentation test-runner wiring and dependency updates, embedded fixture extraction, 100+ parameterized YAML/JSON test templates, a large test utilities library for UDN/CUDN/multicast flows, and multiple Ginkgo OTP e2e test suites (segmentation, multicast, services, and policy). ChangesOTP Network Segmentation Testing
🎯 4 (Complex) | ⏱️ ~60 minutes
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anuragthehatter 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 |
openshift-ci
Bot
added
the
do-not-merge/invalid-owners-file
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (3)
openshift/pkg/otp/testdata/networking/networkpolicy/netpol-30920-75540.yaml (1)
10-14: ⚡ Quick winDocument that the invalid namespace label value is intentional.
values: ["invalid value"]is schema-invalid for standard Kubernetes label values; if this is a negative-path fixture, please add an inline comment (or metadata annotation) stating that it is intentionally invalid to prevent accidental cleanup or reuse as a valid manifest.🤖 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 `@openshift/pkg/otp/testdata/networking/networkpolicy/netpol-30920-75540.yaml` around lines 10 - 14, The namespace label selector using key: name with operator: In and values: ["invalid value"] is intentionally schema-invalid for this negative-path test; document that intent by adding an inline YAML comment above the values array or a metadata annotation (e.g., a test intent annotation) stating "intentionally invalid label value" so maintainers and cleanup scripts know this fixture is not a valid manifest (locate the values field under the selector for key: name / operator: In and add the comment or annotation there, or attach it to metadata).openshift/pkg/otp/testdata/networking/service-generic-template.yaml (1)
19-19: ⚡ Quick winRemove unused
PORTparameter or wire it intospec.ports[0].port.
PORTis declared but not consumed, which makes the template interface misleading.Suggested minimal cleanup
parameters: - name: SERVICENAME - name: NAMESPACE - name: internalTrafficPolicy - name: externalTrafficPolicy - name: ipFamilyPolicy - name: PROTOCOL - name: SELECTOR - name: serviceType -- name: PORTAlso applies to: 34-34
🤖 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 `@openshift/pkg/otp/testdata/networking/service-generic-template.yaml` at line 19, Remove the unused template parameter PORT or wire it into the Service spec: either delete the PORT parameter from the template's parameters list, or use it to set spec.ports[0].port (and any related containerPort/value mappings) so the declared PORT parameter is actually consumed by the Service manifest; update any parameter references (PORT) accordingly to ensure consistency with spec.ports[0].port.openshift/test/otp/network_segmentation_services.go (1)
693-709: ⚡ Quick winDrop the
bash -cwrapper around the curl poll.The service address is interpolated into a shell command, and the subprocess is not tied to
cxt. Callingcurldirectly viaexec.CommandContextavoids shell parsing and lets the poll timeout cancel the request cleanly.Safer direct exec
- var svcURL, svcCmd [2]string + var svcURL [2]string for i := 0; i < 2; i++ { svcURL[i] = net.JoinHostPort(svcExternalIP[i], "27017") - svcCmd[i] = fmt.Sprintf("curl %s --connect-timeout 30", svcURL[i]) - e2e.Logf("\n svcCmd: %v\n", svcCmd[i]) + e2e.Logf("\n svcURL: %v\n", svcURL[i]) err = wait.PollUntilContextTimeout(context.TODO(), 10*time.Second, 120*time.Second, false, func(cxt context.Context) (bool, error) { - output, err1 := exec.Command("bash", "-c", svcCmd[i]).Output() + output, err1 := exec.CommandContext(cxt, "curl", "--connect-timeout", "30", svcURL[i]).Output() if err1 != nil || !strings.Contains(string(output), "Hello OpenShift") { e2e.Logf("got err:%v, and try next round", err1) return false, nil }🤖 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 `@openshift/test/otp/network_segmentation_services.go` around lines 693 - 709, The current loop builds a shell string in svcCmd and calls exec.Command("bash", "-c", ...) inside wait.PollUntilContextTimeout, which prevents the poll’s context (cxt) from canceling the curl process and risks shell injection; change the code to call exec.CommandContext(cxt, "curl", svcURL[i], "--connect-timeout", "30") (or equivalent args) instead of using "bash -c" and svcCmd, remove the shell wrapper, and use the returned output/error from exec.CommandContext directly so the request is cancelled when the poll context times out; keep references to svcURL, wait.PollUntilContextTimeout and the poll callback so location is obvious.
🤖 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 `@openshift/pkg/otp/testdata/networking/hello-pod-daemonset.yaml`:
- Around line 14-15: Replace the deprecated nodeSelector key by updating the
nodeSelector entry named "beta.kubernetes.io/os" to the stable GA key
"kubernetes.io/os" while keeping the existing value (e.g., "linux") so the
DaemonSet's node selection continues to work; locate the nodeSelector block
(symbol: nodeSelector) and change the label key from "beta.kubernetes.io/os" to
"kubernetes.io/os".
In
`@openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_singlestack_template.yaml`:
- Line 17: The template uses an invalid interpolation `${{PREFIX}}` for
hostSubnet; replace the double-brace form with the proper OpenShift template
parameter syntax (e.g., `${PREFIX}`) in the hostSubnet entry and ensure the
`PREFIX` parameter is declared in the template parameters block; also scan for
other `\$\{\{...\}\}` occurrences and correct them to single-brace `${...}`
usage to match OpenShift template processing.
In
`@openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_template.yaml`:
- Around line 21-24: Indent the container hardening fields so they are nested
under securityContext: move allowPrivilegeEscalation and capabilities (with
capabilities.drop) one level deeper under securityContext in
udn_test_pod_template.yaml so they become
securityContext.allowPrivilegeEscalation and securityContext.capabilities.drop;
ensure YAML uses the same indentation style as other container fields and that
capabilities.drop remains a list (["ALL"]) to be valid to the API server.
In
`@openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-multiple-CIDRs-template.yaml`:
- Around line 12-27: Add an explicit egress-only policyTypes entry to the
NetworkPolicy spec so it does not implicitly deny ingress; update the resource
with spec.policyTypes: ["Egress"] in the NetworkPolicy definition (the document
using keys like spec, podSelector, egress and ipBlock cidr placeholders such as
"${cidrIpv4}", "${cidrIpv6}" in the file
ipBlock-egress-dual-multiple-CIDRs-template.yaml) by inserting the
spec.policyTypes line alongside spec, before egress, ensuring the policy is
explicitly egress-only.
In `@openshift/pkg/otp/utils/network_segmentation_utils.go`:
- Around line 359-362: The code unconditionally indexes cudncrd.Values[0] and
[1] in CreateCUDNCRDMatchExpSingleStack (and similar places noted at lines
374-375, 473-474, 486-487), which will panic if fewer than two values are
provided; add a guard that validates len(cudncrd.Values) >= 2 before using
Values[0]/Values[1], and if the check fails return/log a clear error (or
short-circuit the wait.PollUntilContextTimeout callback with (false,
fmt.Errorf(...))) so the caller gets a descriptive failure instead of a panic;
update all occurrences (the CreateCUDNCRDMatchExpSingleStack function and the
other match-expression helper calls referenced) to perform this validation.
- Around line 778-784: The second socat invocation in the dual-stack negative
path builds clientCmd without the '-' stdin endpoint (see podIP2, clientCmd, and
e2eoutput.RunHostCmd usage), making the command malformed and possibly hiding
real connectivity failures; update the second fmt.Sprintf to include the same
stdin delimiter (the '-' token used in the first command) when dialing
net.JoinHostPort(podIP2, strconv.Itoa(port)), keep the timeout and sh -c wrapper
intact, and ensure the test still ignores client-side errors while asserting
serverCmdOutput does not contain "hello".
- Around line 747-749: The defer that calls serverCmd.Process.Kill() is executed
before verifying the Background() call succeeded, risking a nil-pointer
dereference; change the order so you call
oc.AsAdmin().WithoutNamespace().Run("exec").Args(...).Background(), then check
serverCmdErr with o.Expect(serverCmdErr).NotTo(o.HaveOccurred()) (or handle the
error), and only after that install defer serverCmd.Process.Kill() using the
returned serverCmd to ensure Process is non-nil.
- Around line 844-867: The layer2 dual-stack branch returns immediately after
calling CreateLayer2DualStackUDNCRD which can race the controller; after
invoking UdnCRDResource.CreateLayer2DualStackUDNCRD(oc) add the same readiness
wait used in the single-stack branch by calling WaitUDNCRDApplied(oc, namespace,
udncrd.Crdname) and assert no error (o.Expect(err).NotTo(o.HaveOccurred())),
mirroring the logic used with CreateLayer2SingleStackUDNCRD.
In `@openshift/test/otp/network_segmentation_multicast.go`:
- Around line 244-252: You delete mcastPodList[2] via
otputils.RemoveResource(...) then immediately call otputils.ChkMcatRcvOnPod(...)
and o.Expect(...) for the same mcastPodList[2], which is stale after deletion;
either move the RemoveResource call to after all ChkMcatRcvOnPod checks or
remove the third check/expectation (the chkRes3 variable and its o.Expect) so
you only assert receive state for existing pods (keep the
otputils.ChkMcatRcvOnPod calls and o.Expect for mcastPodList[0] and
mcastPodList[1], and adjust sleep placement if you choose to move deletion).
In `@openshift/test/otp/network_segmentation_policy.go`:
- Around line 478-486: The ipv6single branch sets subnet to a single-entry slice
but later code indexes subnet[i-2] for i=2 and i=3, causing an out-of-range
panic; update the ipv6single initialization of the subnet variable (the subnet
slice used alongside ipStackType) to contain two entries (matching the
two-element shape used by ipv4single/dual cases), e.g. duplicate the IPv6 CIDR
so subnet has length 2, and make the same fix in the corresponding duplicate
block around the later occurrence (the second subnet initialization referenced
in the review).
- Around line 165-169: The journalctl assertions are too broad and can match
unrelated entries; update the checks around
otputils.DebugNodeWithOptionsAndChroot (where cmd, output, journalctlErr are
used) to scope the search to this test run by either (1) including the test
namespace and policy/pod name in the grep (use the test's namespace and
policyName/podName variables in the fmt.Sprintf that builds cmd) so you search
for lines containing both verdict=allow/drop and the namespace/policy
identifier, or (2) capture a precise timestamp immediately before the traffic
step (e.g., time.Now().UTC().Format(time.RFC3339)) and use that timestamp in the
journalctl --since argument so you only inspect new log lines; apply the same
change to the other occurrences mentioned.
- Around line 117-123: The deferred cleanup currently hardcodes undoPatchInfo to
set policyAuditConfig.destination to "", which can clobber a preexisting value;
instead capture the current value before applying the test patch (use
oc.AsAdmin().WithoutNamespace().Run("get").Args(patchSResource,"-o","jsonpath=...")
or equivalent to read
.spec.defaultNetwork.ovnKubernetesConfig.policyAuditConfig.destination into a
variable), then build undoPatchInfo using that captured value and use it in the
defer patch call (leave references to oc, patchSResource, patchInfo, and the
existing defer/patch flow intact), so the cleanup restores the exact original
destination rather than forcing an empty string.
In `@openshift/test/otp/network_segmentation_services.go`:
- Around line 826-829: The negative curl checks are using pod2ns1.Name (a pod in
cudnNS[0]) as the source for requests from cudnNS[1]; update the CurlPod2SvcFail
calls to use the pod that actually exists in that namespace (e.g., replace
pod2ns1.Name with the namespace-local pod variable such as pod2ns2.Name or the
pod variable created for cudnNS[1]) so CurlPod2SvcFail(oc, cudnNS[1], cudnNS[2],
...) and the symmetric check use the correct source pod; apply the same fix for
the later occurrences around the other block mentioned (the calls near the
1017-1020 region).
- Around line 602-645: The test unconditionally creates IPv4 single‑stack UDNs
and should be gated by cluster IP family; call otputils.CheckIPStackType(oc)
(same helper used in other service tests) at the start of the test and if it
returns "ipv6single" call g.Skip with a message (or otherwise only proceed for
"ipv4single"/dualstack), so the setup for udncrd/udnl2crd (variables udncrd,
udnl2crd and their CreateUdnCRDSingleStack/CreateLayer2SingleStackUDNCRD calls)
is not executed on IPv6‑only clusters.
- Around line 1307-1315: The verification rebuilds externalIP/externalIPv6 from
node ordering (using otputils.GetNodeIP and nodeList) which is fragile; instead,
for each preserved Service object read its spec.externalIPs and use those values
for the externalIP/externalIPv6 arrays used for the curl checks, and keep using
otputils.GetNodeIP only for node-local reachability assertions; so replace the
loop that appends nodeIP2/nodeIP1 with code that fetches each preserved Service
(by the same names used in the setup), extracts service.Spec.ExternalIPs into
externalIP/externalIPv6, and fall back to GetNodeIP only when you need the
specific node-local address for per-node checks.
In `@openshift/test/otp/network_segmentation.go`:
- Around line 806-819: After annotating the pod (annotationConf applied via
oc.AsAdmin().WithoutNamespace().Run("annotate") on pod2Name), add a short
polling loop that waits until the OVN policy change has taken effect before
calling otputils.CurlPod2PodPass / PingPod2PodPass / VerifyConnPod2Pod /
CurlNode2PodPass / PingNode2PodPass; implement this by polling (with a timeout)
either the pod's annotation/status that indicates readiness or by repeatedly
attempting the network check (with small backoff) until it succeeds, then
proceed to the existing assertions to avoid flakes while the controller
reconciles.
- Around line 250-271: The test is starting tcpdump after WaitPodReady so
kubelet has stopped sending startup probes; move the tcpdump capture to run
immediately after creating the pod (after calling pod3.CreateUdnPodWithProbe)
and before otputils.WaitPodReady so you capture startup-probe SYNs; specifically
start the background exec (the command built in tcpdumpCmd3 and variables
cmdTcpdump3/cmdOutput3) before calling otputils.WaitPodReady(oc, pod3.Namespace,
pod3.Name), keep the same timeout/cleanup (defer cmdTcpdump3.Process.Kill and
cmdTcpdump3.Wait) and then assert on cmdOutput3 as before.
- Around line 1115-1118: The test currently does a single
oc.AsAdmin().Run("get").Args("event").Output() and asserts eventOutput contains
"failed to allocate new IPs", which races the async event publication; change
this to poll until the namespace event stream contains the expected message
(e.g., use a wait.PollImmediate or Gomega Eventually loop around
oc.AsAdmin().Run("get").Args("event").Output()), retrying the get and checking
for the substring with a reasonable timeout and interval, and replace the direct
o.Expect(eventOutput)... assertion with the polling check that fails only after
the timeout; reference oc.AsAdmin().Run("get").Args("event").Output(),
eventOutput, and the substring "failed to allocate new IPs" when implementing
the loop.
- Around line 662-663: The test currently chooses an arbitrary machineset via
existingMS := oc.AsAdmin().WithoutNamespace().Run("get").Args("machineset",
"-n", "openshift-machine-api", "-o=jsonpath={.items[0].metadata.name}").Output()
which can pick the wrong pool; change selection to deterministically pick the
machineset that owns or matches the specific worker node(s) you will scale
(e.g., query machinesets matching the node labels/machine references) and
capture its name and original replica count into clearly named variables (e.g.,
targetMachineSetName, originalReplicaCount). Then use those variables in the
scale and cleanup logic (replace usages of existingMS and the later total-worker
vs original replica comparison) so the test always scales and restores the same
machineset and avoids hanging on multi-machineset clusters.
- Around line 52-63: The readiness poll currently treats any output without
"False" as success; update the anonymous poll function used in wait.Poll so an
empty or missing readiness snapshot does not count as "all ready": after calling
oc.AsAdmin().Run("get").Args("pods", "-n",
namespace).Template(template).Output() check if stdout is empty or contains no
"True" entries and return false in that case, and only return true when you
observe at least one "True" and no "False" (instead of just checking
strings.Contains(stdout, "False")); this ensures the pod readiness check in the
wait loop only succeeds when pods are actually reported Ready.
---
Nitpick comments:
In `@openshift/pkg/otp/testdata/networking/networkpolicy/netpol-30920-75540.yaml`:
- Around line 10-14: The namespace label selector using key: name with operator:
In and values: ["invalid value"] is intentionally schema-invalid for this
negative-path test; document that intent by adding an inline YAML comment above
the values array or a metadata annotation (e.g., a test intent annotation)
stating "intentionally invalid label value" so maintainers and cleanup scripts
know this fixture is not a valid manifest (locate the values field under the
selector for key: name / operator: In and add the comment or annotation there,
or attach it to metadata).
In `@openshift/pkg/otp/testdata/networking/service-generic-template.yaml`:
- Line 19: Remove the unused template parameter PORT or wire it into the Service
spec: either delete the PORT parameter from the template's parameters list, or
use it to set spec.ports[0].port (and any related containerPort/value mappings)
so the declared PORT parameter is actually consumed by the Service manifest;
update any parameter references (PORT) accordingly to ensure consistency with
spec.ports[0].port.
In `@openshift/test/otp/network_segmentation_services.go`:
- Around line 693-709: The current loop builds a shell string in svcCmd and
calls exec.Command("bash", "-c", ...) inside wait.PollUntilContextTimeout, which
prevents the poll’s context (cxt) from canceling the curl process and risks
shell injection; change the code to call exec.CommandContext(cxt, "curl",
svcURL[i], "--connect-timeout", "30") (or equivalent args) instead of using
"bash -c" and svcCmd, remove the shell wrapper, and use the returned
output/error from exec.CommandContext directly so the request is cancelled when
the poll context times out; keep references to svcURL,
wait.PollUntilContextTimeout and the poll callback so location is obvious.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 3182567e-d012-4404-b1b9-9aa14c7d2520
⛔ Files ignored due to path filters (1)
openshift/go.sumis excluded by!**/*.sum
📒 Files selected for processing (76)
openshift/cmd/ovn-kubernetes-tests-ext/main.goopenshift/go.modopenshift/pkg/otp/testdata/bindata.goopenshift/pkg/otp/testdata/networking/OWNERSopenshift/pkg/otp/testdata/networking/hello-pod-daemonset.yamlopenshift/pkg/otp/testdata/networking/hellosdn.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/multicast/multicast-rc.jsonopenshift/pkg/otp/testdata/networking/network_segmentation/networkpolicy/allow-sctpclient.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/networkpolicy/allow-to-all-namespaces.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/rc-ping-for-pod-template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/sctp/load-sctp-module.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/sctp/sctpclient.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/sctp/sctpserver.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_dualstack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_layer2_dualstack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_layer2_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_localnet_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_localnet_singlestack_with_vlan_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_dualstack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_layer2_dualstack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_layer2_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_dualstack2_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_layer2_dualstack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_layer2_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_nad_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_statefulset_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_annotation_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_annotation_template_node.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_liveness_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_readiness_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_startup_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_template_node.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_with_multiplenetworkpolicy.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-egress-red.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-from-all-namespaces.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-from-same-namespace.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-ingress-red.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-same-namespace.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-to-openshift-dns.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-to-same-namespace.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/default-allow-egress.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/default-deny-egress.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/default-deny-ingress.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/egress-allow-all.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/egress-ingress-62524.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/egress_49696.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/generic-networkpolicy-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ingress-allow-all.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ingress_49696.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-dual-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-single-CIDR-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-single-CIDR-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-single-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-dual-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-dual-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-single-CIDR-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-single-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/netpol-30920-75540.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/vap-npprotection-blockdeletion.yamlopenshift/pkg/otp/testdata/networking/ping-for-pod-specific-node-template.yamlopenshift/pkg/otp/testdata/networking/ping-for-pod-template.yamlopenshift/pkg/otp/testdata/networking/service-generic-template.yamlopenshift/pkg/otp/testdata/networking/statefulset-hello.yamlopenshift/pkg/otp/testdata/networking/testpod.yamlopenshift/pkg/otp/utils/network_segmentation_utils.goopenshift/pkg/otp/utils/utils.goopenshift/test/otp/network_segmentation.goopenshift/test/otp/network_segmentation_multicast.goopenshift/test/otp/network_segmentation_policy.goopenshift/test/otp/network_segmentation_services.go
anuragthehatter
changed the title
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@anuragthehatter: This pull request references CORENET-7128 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. |
anuragthehatter
force-pushed
the
ote-otp-netseg-migration
branch
2 times, most recently
from
50e8dc7 to
9e57184
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openshift/pkg/otp/testdata/networking/networkpolicy/default-allow-egress.yaml (1)
11-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove trailing blank line.
Static analysis flagged extra blank lines at end of file.
Proposed fix
policyTypes: - Egress -🤖 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 `@openshift/pkg/otp/testdata/networking/networkpolicy/default-allow-egress.yaml` around lines 11 - 12, The file default-allow-egress.yaml contains an extra trailing blank line at the end of the document; remove the empty line(s) so the file ends immediately after the final YAML content (ensure no extra newline-only lines at EOF), and save the file so static analysis no longer reports the trailing blank line.Source: Linters/SAST tools
openshift/pkg/otp/testdata/networking/networkpolicy/allow-to-openshift-dns.yaml (1)
19-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove trailing blank line.
Static analysis flagged extra blank lines at end of file.
Proposed fix
policyTypes: - Egress -🤖 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 `@openshift/pkg/otp/testdata/networking/networkpolicy/allow-to-openshift-dns.yaml` around lines 19 - 20, Open allow-to-openshift-dns.yaml and remove the extra blank line(s) at the end of the file so the YAML document ends immediately after the last line; ensure the file ends with a single newline (no trailing empty lines) to satisfy static analysis.Source: Linters/SAST tools
🤖 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
`@openshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_dualstack_template.yaml`:
- Around line 20-22: The template uses the wrong parameter placeholder syntax
for hostSubnet (it currently uses ${{IPv4PREFIX}} and ${{IPv6PREFIX}}), which
won't be resolved by OpenShift templates; update the hostSubnet occurrences to
use the standard OpenShift template parameter syntax ${IPv4PREFIX} and
${IPv6PREFIX} (leave other params like "${IPv6CIDR}" as-is if they are intended
string values) so the hostSubnet fields resolve correctly during CRD creation.
In
`@openshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_singlestack_template.yaml`:
- Line 24: The template uses OpenShift parameter syntax incorrectly for
hostSubnet: replace the unresolved token pattern "${{PREFIX}}" with the proper
"${PREFIX}" so the hostSubnet parameter will be substituted at template
instantiation; locate the hostSubnet entry in the template (near other tokens
like "${CRDNAME}" and "${ROLE}") and update it to use the same "${...}"
parameter form for PREFIX.
In
`@openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_readiness_template.yaml`:
- Around line 13-27: Remove the duplicated top-level "spec" key so there is a
single spec block containing the container "hello-pod", and change the
readinessProbe fields readinessProbe.httpGet.port,
readinessProbe.failureThreshold, and readinessProbe.periodSeconds to use
unquoted template parameters (PORT, FAILURETHRESHOLD, PERIODSECONDS) so each
value is exactly the ${...} token (no surrounding quotes) to allow oc process to
render numeric values.
In
`@openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_template_node.yaml`:
- Around line 21-24: The YAML has malformed nesting: move
allowPrivilegeEscalation and capabilities under the securityContext mapping so
they become properties of securityContext instead of sibling keys; specifically,
indent allowPrivilegeEscalation and capabilities (and capabilities.drop) one
level deeper under securityContext in the Pod container spec so securityContext
contains allowPrivilegeEscalation: false and capabilities: { drop: ["ALL"] }
rather than leaving securityContext empty.
In `@openshift/test/otp/network_segmentation_services.go`:
- Around line 697-709: The code currently builds svcCmd[i] and invokes
exec.Command("bash", "-c", svcCmd[i]) inside the wait.PollUntilContextTimeout
callback, which risks shell injection; change it to call exec.Command directly
with explicit arguments (e.g., exec.Command("curl", svcURL[i],
"--connect-timeout", "30")) and capture its Output() / error. Update the
anonymous func used in wait.PollUntilContextTimeout and any uses of svcCmd[i] so
they construct args without a shell, preserve the existing check for "Hello
OpenShift" in output, and keep the same timeout/poll behavior and error logging
via e2e.Logf and o.Expect.
---
Outside diff comments:
In
`@openshift/pkg/otp/testdata/networking/networkpolicy/allow-to-openshift-dns.yaml`:
- Around line 19-20: Open allow-to-openshift-dns.yaml and remove the extra blank
line(s) at the end of the file so the YAML document ends immediately after the
last line; ensure the file ends with a single newline (no trailing empty lines)
to satisfy static analysis.
In
`@openshift/pkg/otp/testdata/networking/networkpolicy/default-allow-egress.yaml`:
- Around line 11-12: The file default-allow-egress.yaml contains an extra
trailing blank line at the end of the document; remove the empty line(s) so the
file ends immediately after the final YAML content (ensure no extra newline-only
lines at EOF), and save the file so static analysis no longer reports the
trailing blank line.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: c2b2128c-6fa3-4ae7-8327-8c1d2ba6a94f
⛔ Files ignored due to path filters (1)
openshift/go.sumis excluded by!**/*.sum
📒 Files selected for processing (73)
openshift/cmd/ovn-kubernetes-tests-ext/main.goopenshift/go.modopenshift/pkg/otp/testdata/bindata.goopenshift/pkg/otp/testdata/networking/OWNERSopenshift/pkg/otp/testdata/networking/hello-pod-daemonset.yamlopenshift/pkg/otp/testdata/networking/hellosdn.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/multicast/multicast-rc.jsonopenshift/pkg/otp/testdata/networking/network_segmentation/networkpolicy/allow-to-all-namespaces.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/rc-ping-for-pod-template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/sctp/load-sctp-module.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_dualstack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_layer2_dualstack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_layer2_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_localnet_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_localnet_singlestack_with_vlan_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_dualstack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_layer2_dualstack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_layer2_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_dualstack2_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_layer2_dualstack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_layer2_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_nad_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_statefulset_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_annotation_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_annotation_template_node.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_liveness_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_readiness_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_startup_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_template_node.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_with_multiplenetworkpolicy.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-egress-red.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-from-all-namespaces.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-from-same-namespace.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-ingress-red.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-same-namespace.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-to-openshift-dns.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-to-same-namespace.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/default-allow-egress.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/default-deny-egress.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/default-deny-ingress.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/egress-allow-all.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/egress-ingress-62524.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/egress_49696.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/generic-networkpolicy-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ingress-allow-all.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ingress_49696.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-dual-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-single-CIDR-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-single-CIDR-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-single-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-dual-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-dual-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-single-CIDR-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-single-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/netpol-30920-75540.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/vap-npprotection-blockdeletion.yamlopenshift/pkg/otp/testdata/networking/ping-for-pod-specific-node-template.yamlopenshift/pkg/otp/testdata/networking/ping-for-pod-template.yamlopenshift/pkg/otp/testdata/networking/service-generic-template.yamlopenshift/pkg/otp/testdata/networking/statefulset-hello.yamlopenshift/pkg/otp/testdata/networking/testpod.yamlopenshift/pkg/otp/utils/network_segmentation_utils.goopenshift/pkg/otp/utils/utils.goopenshift/test/otp/network_segmentation.goopenshift/test/otp/network_segmentation_multicast.goopenshift/test/otp/network_segmentation_policy.goopenshift/test/otp/network_segmentation_services.go
✅ Files skipped from review due to trivial changes (13)
- openshift/pkg/otp/testdata/networking/OWNERS
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_layer2_singlestack_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/default-deny-ingress.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-from-all-namespaces.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/default-deny-egress.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_dualstack2_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/generic-networkpolicy-template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/netpol-30920-75540.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-ingress-red.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-to-same-namespace.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-dual-CIDRs-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-multiple-CIDRs-template.yaml
🚧 Files skipped from review as they are similar to previous changes (39)
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-single-multiple-CIDRs-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-same-namespace.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ingress_49696.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-dual-multiple-CIDRs-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ingress-allow-all.yaml
- openshift/pkg/otp/testdata/networking/ping-for-pod-template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_nad_template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_layer2_singlestack_template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/networkpolicy/allow-to-all-namespaces.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-single-CIDR-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-single-CIDR-template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_statefulset_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-from-same-namespace.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/multicast/multicast-rc.json
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_annotation_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/egress-allow-all.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-egress-red.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-single-multiple-CIDRs-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-dual-CIDRs-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-single-CIDR-template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/sctp/load-sctp-module.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/rc-ping-for-pod-template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_singlestack_template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_localnet_singlestack_with_vlan_template.yaml
- openshift/pkg/otp/testdata/networking/ping-for-pod-specific-node-template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_dualstack_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/egress-ingress-62524.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_annotation_template_node.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/egress_49696.yaml
- openshift/pkg/otp/testdata/bindata.go
- openshift/pkg/otp/testdata/networking/networkpolicy/vap-npprotection-blockdeletion.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_localnet_singlestack_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-CIDRs-template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_with_multiplenetworkpolicy.yaml
- openshift/test/otp/network_segmentation_policy.go
- openshift/go.mod
- openshift/test/otp/network_segmentation_multicast.go
- openshift/pkg/otp/utils/network_segmentation_utils.go
- openshift/test/otp/network_segmentation.go
anuragthehatter
force-pushed
the
ote-otp-netseg-migration
branch
from
9e57184 to
ba54b71
Compare
Author
|
@coderabbitai resolve |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
openshift/pkg/otp/testdata/networking/service-generic-template.yaml (1)
19-35: ⚡ Quick winRemove the unused
PORTparameter or wire it intospec.ports[0].port.
PORTis declared but never consumed, which makes the template contract ambiguous.Suggested patch (remove unused parameter)
parameters: - name: SERVICENAME - name: NAMESPACE - name: internalTrafficPolicy - name: externalTrafficPolicy - name: ipFamilyPolicy - name: PROTOCOL - name: SELECTOR - name: serviceType -- name: PORT🤖 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 `@openshift/pkg/otp/testdata/networking/service-generic-template.yaml` around lines 19 - 35, The template declares a parameter named PORT but never uses it; either remove the PORT parameter or wire it into the Service spec by replacing the hardcoded/templated port (currently "port: 27017" or "protocol: \"${PROTOCOL}\"/targetPort: 8080" block) so spec.ports[0].port is set from "${PORT}". Update the parameters list to remove PORT if unused, or update the Service spec (the ports block referenced in the file) to consume "${PORT}" and ensure parameter name PORT remains documented and validated.
🤖 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
`@openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-single-multiple-CIDRs-template.yaml`:
- Around line 12-21: The NetworkPolicy manifest is missing an explicit
policyTypes field, which can cause it to default to include ingress semantics;
update the NetworkPolicy (the block containing podSelector: {}, egress:, and the
ipBlock cidr entries) to include policyTypes: - Egress so the policy is
explicitly egress-only and only enforces the egress rules for the selected pods.
In `@openshift/pkg/otp/utils/network_segmentation_utils.go`:
- Around line 991-992: The regexes in network_segmentation_utils.go are
interpolating raw neighborip, mcastip and intf into patterns (see
regexp.MustCompile calls that build reg1/reg2 and the similar occurrence at line
~1069), causing dots and other metacharacters to be treated as regex tokens; fix
by wrapping these dynamic inputs with regexp.QuoteMeta (e.g., use
QuoteMeta(neighborip), QuoteMeta(mcastip), QuoteMeta(intf)) before concatenating
into the pattern so the values are escaped, and update all similar regex
constructions (including the other occurrence referenced) to use QuoteMeta.
- Around line 976-980: The current code in StartMcastTrafficOnPod launches a
goroutine that calls e2eoutput.RunHostCmd and asserts with o.Expect while the
omping command already backgrounds itself (it includes '&'), making failures
non-deterministic; fix by removing the detached goroutine and running the
RunHostCmd call synchronously so you can directly check and assert the returned
error (or alternatively capture the error from the goroutine via a channel and
assert on that value in the main goroutine), and also remove the trailing '&'
from the ompingCmd string if you want the process to be managed by the test
process; locate the ompingCmd construction and the RunHostCmd invocation in
StartMcastTrafficOnPod and ensure the assertion occurs on the returned err in
the main flow rather than inside a detached goroutine.
---
Nitpick comments:
In `@openshift/pkg/otp/testdata/networking/service-generic-template.yaml`:
- Around line 19-35: The template declares a parameter named PORT but never uses
it; either remove the PORT parameter or wire it into the Service spec by
replacing the hardcoded/templated port (currently "port: 27017" or "protocol:
\"${PROTOCOL}\"/targetPort: 8080" block) so spec.ports[0].port is set from
"${PORT}". Update the parameters list to remove PORT if unused, or update the
Service spec (the ports block referenced in the file) to consume "${PORT}" and
ensure parameter name PORT remains documented and validated.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 4d8f510e-6789-4a57-81d0-24a354b46163
⛔ Files ignored due to path filters (1)
openshift/go.sumis excluded by!**/*.sum
📒 Files selected for processing (73)
openshift/cmd/ovn-kubernetes-tests-ext/main.goopenshift/go.modopenshift/pkg/otp/testdata/bindata.goopenshift/pkg/otp/testdata/networking/OWNERSopenshift/pkg/otp/testdata/networking/hello-pod-daemonset.yamlopenshift/pkg/otp/testdata/networking/hellosdn.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/multicast/multicast-rc.jsonopenshift/pkg/otp/testdata/networking/network_segmentation/networkpolicy/allow-to-all-namespaces.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/rc-ping-for-pod-template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/sctp/load-sctp-module.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_dualstack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_layer2_dualstack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_layer2_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_localnet_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_localnet_singlestack_with_vlan_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_dualstack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_layer2_dualstack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_layer2_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_dualstack2_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_layer2_dualstack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_layer2_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_singlestack_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_nad_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_statefulset_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_annotation_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_annotation_template_node.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_liveness_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_readiness_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_startup_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_template.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_template_node.yamlopenshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_with_multiplenetworkpolicy.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-egress-red.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-from-all-namespaces.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-from-same-namespace.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-ingress-red.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-same-namespace.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-to-openshift-dns.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-to-same-namespace.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/default-allow-egress.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/default-deny-egress.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/default-deny-ingress.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/egress-allow-all.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/egress-ingress-62524.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/egress_49696.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/generic-networkpolicy-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ingress-allow-all.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ingress_49696.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-dual-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-single-CIDR-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-single-CIDR-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-single-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-dual-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-dual-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-single-CIDR-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-single-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/netpol-30920-75540.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/vap-npprotection-blockdeletion.yamlopenshift/pkg/otp/testdata/networking/ping-for-pod-specific-node-template.yamlopenshift/pkg/otp/testdata/networking/ping-for-pod-template.yamlopenshift/pkg/otp/testdata/networking/service-generic-template.yamlopenshift/pkg/otp/testdata/networking/statefulset-hello.yamlopenshift/pkg/otp/testdata/networking/testpod.yamlopenshift/pkg/otp/utils/network_segmentation_utils.goopenshift/pkg/otp/utils/utils.goopenshift/test/otp/network_segmentation.goopenshift/test/otp/network_segmentation_multicast.goopenshift/test/otp/network_segmentation_policy.goopenshift/test/otp/network_segmentation_services.go
✅ Files skipped from review due to trivial changes (13)
- openshift/pkg/otp/testdata/networking/networkpolicy/default-allow-egress.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/default-deny-egress.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_layer2_singlestack_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ingress_49696.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-single-multiple-CIDRs-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-dual-multiple-CIDRs-template.yaml
- openshift/pkg/otp/testdata/networking/OWNERS
- openshift/pkg/otp/testdata/networking/network_segmentation/networkpolicy/allow-to-all-namespaces.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_singlestack_template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/sctp/load-sctp-module.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_layer2_dualstack_template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_layer2_dualstack_template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_layer2_singlestack_template.yaml
🚧 Files skipped from review as they are similar to previous changes (48)
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-same-namespace.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_startup_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-ingress-red.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-to-same-namespace.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-single-CIDR-template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_template_node.yaml
- openshift/pkg/otp/testdata/networking/ping-for-pod-template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_dualstack_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/vap-npprotection-blockdeletion.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/egress-allow-all.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-multiple-CIDRs-template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/multicast/multicast-rc.json
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_readiness_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/default-deny-ingress.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_annotation_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ingress-allow-all.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_singlestack_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/egress_49696.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_singlestack_template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_localnet_singlestack_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-CIDRs-template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_layer2_dualstack_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/netpol-30920-75540.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-from-all-namespaces.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_statefulset_template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_with_multiplenetworkpolicy.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_layer2_singlestack_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-single-CIDR-template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_nad_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-from-same-namespace.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-dual-CIDRs-template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_localnet_singlestack_with_vlan_template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/cudn_crd_matchexp_dualstack_template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-egress-red.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/egress-ingress-62524.yaml
- openshift/pkg/otp/testdata/networking/ping-for-pod-specific-node-template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/rc-ping-for-pod-template.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_annotation_template_node.yaml
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_test_pod_template.yaml
- openshift/cmd/ovn-kubernetes-tests-ext/main.go
- openshift/pkg/otp/testdata/networking/networkpolicy/generic-networkpolicy-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-dual-CIDRs-template.yaml
- openshift/test/otp/network_segmentation_multicast.go
- openshift/pkg/otp/testdata/networking/network_segmentation/udn/udn_crd_dualstack2_template.yaml
- openshift/go.mod
- openshift/test/otp/network_segmentation_services.go
- openshift/test/otp/network_segmentation_policy.go
- openshift/test/otp/network_segmentation.go
✅ Action performedComments resolved. Approval is disabled; enable |
anuragthehatter
force-pushed
the
ote-otp-netseg-migration
branch
from
ba54b71 to
38251da
Compare
Author
|
@coderabbitai resolve |
✅ Action performedComments resolved. Approval is disabled; enable |
anuragthehatter
force-pushed
the
ote-otp-netseg-migration
branch
4 times, most recently
from
570cb4d to
645b34c
Compare
Author
|
hitting bug: https://redhat.atlassian.net/browse/OCPBUGS-48731 |
Add OTP test infrastructure to main.go (test/otp import, OTP lifecycle handling, exutil.WithCleanup) and shared utilities for network segmentation test migration. Includes: - main.go: OTP test discovery and informing lifecycle for [OTP] tests - utils.go: shared OTP helper functions - network_segmentation_utils.go: UDN/CUDN pod, NAD, CRD, multicast helpers - testdata/: bindata embed, shared networking templates, and network_segmentation-specific templates (UDN/CUDN, SCTP, multicast, network policy) - go.mod/go.sum: dependency updates for OTP framework Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
anuragthehatter
force-pushed
the
ote-otp-netseg-migration
branch
4 times, most recently
from
53c9f8e to
e07aa17
Compare
Migrate 4 UDN network policy tests from openshift-tests-private: 78292, 79092, 79093, 79094 Covers ingress/egress policies in L3/L2 UDN NADs and CIDR block with except clause in L3 CUDN. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Migrate 8 network segmentation service tests from openshift-tests-private: 76017, 76732, 75942, 76014, 78767, 78768, 44790, 79163 Covers service access across UDN namespaces, Pod2Service/NodePort for L2/L3 UDN, LoadBalancer service, CUDN L2/L3 services, and ExternalIP services. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Migrate 11 network segmentation tests from openshift-tests-private: 75223, 75254, 75503, 75955, 75984, 76939, 77542, 77742, 78152, 78381, 79095 Covers UDN pod isolation, kubelet probes, overlapping CIDRs, join subnet overlap, OVN gateway migration, scaled node, default network ports on UDN pods, kapi/dns traffic, and IP exhaustion events. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Migrate 6 network segmentation multicast tests from openshift-tests-private: 78447, 78446, 78381, 78448, 78450, 78382 Covers UDN multicast enable/disable, delete/add pods multicast, CUDN multicast subscribe/send/receive, multiple multicast groups, same groups across namespaces, and CUDN multicast isolation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
anuragthehatter
force-pushed
the
ote-otp-netseg-migration
branch
from
e07aa17 to
af42231
Compare
Contributor
|
The OWNERS file contains untrusted users, which makes it INVALID. The following users are mentioned in OWNERS file(s) but are untrusted for the following reasons. One way to make the user trusted is to add them as members of the openshift org. You can then trigger verification by writing
|
Contributor
|
@anuragthehatter: 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. |
Author
|
@asood-rh @SachinNinganure Pls help review this one |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
openshift-tests-privateto the OTE frameworknetwork_segmentation_utils.go) and testdata templates[JIRA:Networking][OTP]Test breakdown
Dependencies
Depends on PR #3200 (first two commits cherry-picked as base)
Test plan
go buildcompiles cleanlygo vetpasses on all packages🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores