Skip to content

Add e2e test for EgressIP pod IP leakage on secondary host interface#31427

Open
arghosh93 wants to merge 1 commit into
openshift:mainfrom
arghosh93:egressip-secondary-host-leak-test
Open

Add e2e test for EgressIP pod IP leakage on secondary host interface#31427
arghosh93 wants to merge 1 commit into
openshift:mainfrom
arghosh93:egressip-secondary-host-leak-test

Conversation

@arghosh93

@arghosh93 arghosh93 commented Jul 24, 2026

Copy link
Copy Markdown

Tests that a new pod's real IP does not leak as source during EgressIP reconciliation when using a secondary (non-OVN) host interface.

upstream PR: ovn-kubernetes/ovn-kubernetes#6591

Summary by CodeRabbit

  • Tests
    • Added end-to-end coverage for EgressIP traffic on nodes with secondary network interfaces.
    • Verifies that traffic uses the configured EgressIP and prevents pod IP address leakage during reconciliation.
    • Added stress and packet-capture validation to improve confidence in EgressIP behavior under load.

Tests that a new pod's real IP does not leak as source during EgressIP
reconciliation when using a secondary (non-OVN) host interface.

upstream PR: ovn-kubernetes/ovn-kubernetes#6591

Signed-off-by: Arnab Ghosh <arnabghosh89@gmail.com>
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: automatic mode

@openshift-ci

openshift-ci Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: arghosh93
Once this PR has been reviewed and has the lgtm label, please assign tssurya for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci
openshift-ci Bot requested review from martinkennelly and tssurya July 24, 2026 11:33
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Walkthrough

Adds a gated E2E scenario for secondary-host EgressIP behavior. It configures a secondary interface, captures trigger-pod traffic, stresses reconciliation, and verifies packets use the expected EgressIP without pod source-IP leakage.

Changes

Secondary-host EgressIP validation

Layer / File(s) Summary
Trigger pod traffic generation
test/extended/networking/egressip_helpers.go
Adds a node-affined agnhost pod helper that repeatedly sends identifiable curl traffic.
Secondary-host EgressIP scenario
test/extended/networking/egressip.go
Adds NMState setup, platform and handler gates, packet sniffers, reconciliation stress pods, EgressIP creation, and source-IP assertions.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant EgressIPTest
  participant NMState
  participant EgressIPController
  participant TriggerPod
  participant PacketSniffer
  EgressIPTest->>NMState: Apply secondary interface policy
  EgressIPTest->>EgressIPController: Create secondary-interface EgressIP
  EgressIPTest->>TriggerPod: Create traffic trigger
  TriggerPod->>PacketSniffer: Generate identifiable traffic
  EgressIPTest->>PacketSniffer: Inspect matching packets
  PacketSniffer-->>EgressIPTest: Return observed source IPs
Loading

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error, 2 warnings)

Check name Status Explanation Resolution
No-Sensitive-Data-In-Logs ❌ Error The new E2E path logs pod/node identifiers and raw sniffer lines, including pod.Spec.NodeName and pod IPs, which can expose internal hostnames/IPs. Redact node names and raw packet-sniffer lines; log only aggregate counts or masked identifiers in this test path.
Single Node Openshift (Sno) Test Compatibility ⚠️ Warning The new e2e test requires at least 3 worker nodes and lacks any SNO skip/guard, so it will fail on Single Node OpenShift. Add a [Skipped:SingleReplicaTopology] label or a runtime single-node topology check that skips before the 3-worker prerequisite.
Ipv6 And Disconnected Network Test Compatibility ⚠️ Warning The new test hardcodes IPv4-only addresses (10.10.20.1/100, v4ExternalIP) and lacks any IPv6-family detection or fallback. Gate the test on IP family and use IPv6-safe addresses/CIDRs via GetIPAddressFamily/GetIPFamilyForCluster; remove v4-only literals.
✅ Passed checks (12 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: adding an e2e test for EgressIP pod IP leakage on a secondary host interface.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed The new Ginkgo titles are fixed strings; no UUIDs, node names, IPs, namespaces, or other generated values appear in test names.
Test Structure And Quality ✅ Passed The new E2E test follows repo patterns: setup/cleanup is split across BeforeEach/AfterEach and defers, cluster waits use bounded Eventually, and the main assertions are diagnostic.
Microshift Test Compatibility ✅ Passed PASS: The new Ginkgo It is inside a Describe tagged [apigroup:operator.openshift.io], which MicroShift jobs auto-skip, so its OVN/NMState/multi-node assumptions are exempt.
Topology-Aware Scheduling Compatibility ✅ Passed Only e2e test code changed; no manifests/operators/controllers. The new pod affinity is hostname-based and gated by BareMetal plus 3 workers.
Ote Binary Stdout Contract ✅ Passed No stdout writes or logger redirection issues were added in process-level code; the new code is limited to Ginkgo test callbacks and helpers.
No-Weak-Crypto ✅ Passed The changed test/helper code only uses uuid for log correlation and string matching; no weak crypto primitives or secret comparisons were added.
Container-Privileges ✅ Passed PASS: The new trigger pod spec only sets node affinity and container command; no new privileged, hostNetwork, hostPID, hostIPC, or allowPrivilegeEscalation fields were added.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
test/extended/networking/egressip.go (1)

708-712: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Sequential stress-pod creation is slow.

CreateExecPodOrFail blocks until each pod is Running, so creating 50 pods serially adds substantial wall-clock time to the spec. Consider launching creations concurrently (e.g. a bounded errgroup/sync.WaitGroup) if reconciliation stress, not readiness ordering, is the goal.

🤖 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/networking/egressip.go` around lines 708 - 712, The stress pods
in the creation loop are started serially through CreateExecPodOrFail,
unnecessarily extending the test duration. Update the stress-pod setup around
CreateExecPodOrFail to launch pod creations concurrently, using bounded
concurrency or a synchronized wait group, while preserving unique pod names and
waiting for all creations to complete before continuing.
🤖 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 `@test/extended/networking/egressip.go`:
- Around line 708-712: The stress pods in the creation loop are started serially
through CreateExecPodOrFail, unnecessarily extending the test duration. Update
the stress-pod setup around CreateExecPodOrFail to launch pod creations
concurrently, using bounded concurrency or a synchronized wait group, while
preserving unique pod names and waiting for all creations to complete before
continuing.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: ec4a8ed5-551c-4484-8cdd-341d44c46c3e

📥 Commits

Reviewing files that changed from the base of the PR and between 398edca and 4b9644e.

📒 Files selected for processing (2)
  • test/extended/networking/egressip.go
  • test/extended/networking/egressip_helpers.go

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling required tests:
/test e2e-aws-csi
/test e2e-aws-ovn-fips
/test e2e-aws-ovn-microshift
/test e2e-aws-ovn-microshift-serial
/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2
/test e2e-gcp-csi
/test e2e-gcp-ovn
/test e2e-gcp-ovn-upgrade
/test e2e-metal-ipi-ovn-ipv6
/test e2e-vsphere-ovn
/test e2e-vsphere-ovn-upi

@arghosh93

Copy link
Copy Markdown
Author

/retest

@openshift-ci

openshift-ci Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

@arghosh93: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-vsphere-ovn-upi 4b9644e link true /test e2e-vsphere-ovn-upi
ci/prow/e2e-aws-ovn-microshift-serial 4b9644e link true /test e2e-aws-ovn-microshift-serial
ci/prow/e2e-gcp-ovn 4b9644e link true /test e2e-gcp-ovn
ci/prow/e2e-aws-csi 4b9644e link true /test e2e-aws-csi
ci/prow/e2e-aws-ovn-microshift 4b9644e link true /test e2e-aws-ovn-microshift
ci/prow/e2e-gcp-csi 4b9644e link true /test e2e-gcp-csi
ci/prow/e2e-aws-ovn-fips 4b9644e link true /test e2e-aws-ovn-fips
ci/prow/e2e-aws-ovn-serial-2of2 4b9644e link true /test e2e-aws-ovn-serial-2of2
ci/prow/e2e-aws-ovn-serial-1of2 4b9644e link true /test e2e-aws-ovn-serial-1of2

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant