[DNM] d/s EVPN E2E: overlapping L3/L2 CUDNs isolation tests over L3/L2 VPNs#3245
Open
jechen0648 wants to merge 3 commits into
Open
[DNM] d/s EVPN E2E: overlapping L3/L2 CUDNs isolation tests over L3/L2 VPNs#3245jechen0648 wants to merge 3 commits into
jechen0648 wants to merge 3 commits into
Conversation
WalkthroughThis PR extends the OpenShift OVN-Kubernetes test suite to support EVPN testing scenarios where multiple networks share overlapping CIDR subnets using a shared VTEP. It introduces MAC-VRF Docker proxy mode to handle subnet overlap, adds a new EVPN test type, and updates isolation verification to account for intentional IP overlap with VNI-based isolation. ChangesEVPN Overlapping Subnet Shared VTEP Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jechen0648 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/route_advertisements.go (1)
2718-2724: ⚡ Quick winAdd a defensive nil check for
spec.EVPN.Although the comment on Line 2715 states that
testNetworkSpecis guaranteed to have an EVPN config (becauseoverlappingSpecGenis only called whentestedNetworkType == cudnAdvertisedEVPNUnmanagedSharedVTEP), accessingspec.EVPN.MACVRFandspec.EVPN.IPVRFwithout first checkingspec.EVPN != nilcould panic if the runtime assumption is violated.🛡️ Proposed defensive guard
overlappingSpecGen := func() *udnv1.NetworkSpec { spec := testNetworkSpec.DeepCopy() + if spec.EVPN == nil { + ginkgo.Fail("overlappingSpecGen requires an EVPN network spec") + } if spec.EVPN.MACVRF != nil { spec.EVPN.MACVRF.VNI = randomVNI() }🤖 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/e2e/route_advertisements.go` around lines 2718 - 2724, The code assumes spec.EVPN is non-nil but may panic; wrap the existing mutations to spec.EVPN (accesses to spec.EVPN.MACVRF, spec.EVPN.IPVRF and setting spec.EVPN.VTEP) in a defensive nil check: if spec.EVPN != nil { ... } so randomVNI() is only called and VTEP set when EVPN exists; refer to symbols spec.EVPN, MACVRF, IPVRF, VTEP, randomVNI, testNetworkSpec/overlappingSpecGen and testedNetworkType/cudnAdvertisedEVPNUnmanagedSharedVTEP to locate the code to guard.
🤖 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/e2e/route_advertisements.go`:
- Around line 2130-2137: The code assigns bridgeName and vxlanName from testName
twice (bridgeName = "br"+testName; vxlanName = "vx"+testName), making the first
assignments dead; remove the duplicated pair so only one assignment remains
(keep the assignment nearest the VTEP context where vtepName =
sharedNodeIPsVTEPName is set) and ensure references to bridgeName, vxlanName,
testName and vtepName remain unchanged.
---
Nitpick comments:
In `@test/e2e/route_advertisements.go`:
- Around line 2718-2724: The code assumes spec.EVPN is non-nil but may panic;
wrap the existing mutations to spec.EVPN (accesses to spec.EVPN.MACVRF,
spec.EVPN.IPVRF and setting spec.EVPN.VTEP) in a defensive nil check: if
spec.EVPN != nil { ... } so randomVNI() is only called and VTEP set when EVPN
exists; refer to symbols spec.EVPN, MACVRF, IPVRF, VTEP, randomVNI,
testNetworkSpec/overlappingSpecGen and
testedNetworkType/cudnAdvertisedEVPNUnmanagedSharedVTEP to locate the code to
guard.
🪄 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: 9008e1c5-001b-42f1-a6d5-51dfab92a3d4
⛔ Files ignored due to path filters (1)
openshift/test/generated/zz_generated.annotations.gois excluded by!**/generated/**
📒 Files selected for processing (4)
openshift/test/tests.gotest/e2e/evpn.gotest/e2e/kubevirt.gotest/e2e/route_advertisements.go
openshift-ci
Bot
added
the
needs-rebase
…ervers
Add a macVRFUseProxyDockerNetwork bool parameter to runEVPNNetworkAndServers
and a useProxyDockerNetwork bool parameter to setupMACVRFExternalContainer.
When true, setupMACVRFExternalContainer creates the Docker network without
explicit subnets (Docker picks a random unused range, avoiding rejection of
a second network with the same CUDN CIDR), then reassigns the container IPs
to match the actual CUDN subnets via a single sh -c command that:
- flushes the Docker-assigned IPs from the interface
- enables IPv6 if needed (CreateNetwork infers IP family from subnets;
nil subnets produce an IPv4-only network that disables IPv6)
- adds each CUDN address with the correct prefix length
The Docker bridge forwards L2 frames by MAC regardless of IP addressing,
so the proxy subnet has no functional effect on EVPN traffic.
For non-overlapping call-sites such as kubevirt.go, false is passed in.
Signed-off-by: Jean Chen <jechen@redhat.com>
… table
Add a single overlapping-subnet entry to the otherNetworksToTest table inside
DescribeTableSubtree("When the tested network is of type") to verify traffic
isolation between two EVPN CUDNs that share the same subnet but use different
VNIs. The entry is appended only when the outer tested network type is
cudnAdvertisedEVPNUnmanagedSharedVTEP, so exactly three test cases are
produced (one per shared-VTEP topology: L3 IP-VRF, L2 MAC-VRF, L2 MAC-VRF +
IP-VRF).
Key implementation details:
New cudnAdvertisedEVPNOverlappingCIDRSharedVTEP network type: Uses shared
VTEP (kind primary-network subnet and sharedNodeIPsVTEPName). Both overlapping
CUDNs within the same test share a single bridge/VXLAN on the external FRR
(names derived from testName, same as cudnAdvertisedEVPNUnmanagedSharedVTEP).
Isolation between the two overlapping networks is enforced by their different
VNIs, which come from independent AllocateBGP allocations.
Single adaptive overlappingSpecGen closure: DeepCopies testNetworkSpec (the
spec of the currently-tested outer network) and stamps fresh VNIs from the
inner bgpAlloc, clearing VTEP so the shared VTEP is used. This replaces three
separate topology-specific generators and avoids nil-return skip logic.
macVRFUseProxyDockerNetwork: Passed as true for the overlapping type so Docker
does not reject creating a second network with the same CUDN CIDR. See the
companion commit for details on the proxy-network mechanism.
Hostname-based isolation checks for overlapping IPs: When a source pod shares
the same IP as the tested pod, testPodToHostnameAndExpect verifies it reaches
itself (its own hostname), confirming traffic stays within its own CUDN. When
the source pod has a different IP, its same-network peer may still share the
tested pod's IP; in that case routing within the other network delivers the
packet to the peer rather than to the tested pod. The peer is determined
symmetrically (otherPodSameNode ↔ otherPodDiffNode) so both allocation orders
are handled correctly regardless of which pod happens to get the overlapping IP.
North-south VNI isolation check: Since both overlapping networks assign the
same IP to their MAC-VRF external containers (both derive container IPs from
the shared CUDN subnet), a hostname check is added inside "Both networks are
isolated" to verify that testPod reaches its own external server at the shared
IP and not the other network's container. The check reuses the outer-scope
externalServers (testNetwork's servers, which have correct Docker-registered
IPs) so no additional setup is required.
getExternalServerIPForFamily helper: Extracts the repeated pattern of fetching
an external container's network interface and returning its IP for a given
family into a shared closure, removing duplication across three call sites.
Signed-off-by: Jean Chen <jechen@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…p by OTE Signed-off-by: Jean Chen <jechen@redhat.com>
jechen0648
force-pushed
the
overlapping_cudns_over_evpn_ds
branch
from
bf84d06 to
ecf4816
Compare
openshift-ci
Bot
removed
the
needs-rebase
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/e2e/evpn.go`:
- Around line 744-771: The info.containerIPs field is not updated after the IP
reassignment shell commands are executed, causing the returned info object to
contain outdated Docker-assigned IPs instead of the new CUDN IPs. After the
ExecExternalContainerCommand call succeeds in the useProxyDockerNetwork block,
update info.containerIPs by assigning the containerIPs variable (which contains
the correct CUDN IP addresses) to info.containerIPs so that subsequent code and
callers receive the correct IP addresses.
🪄 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: 33919fe9-3100-4f36-ab38-31cd9f8de0e5
⛔ Files ignored due to path filters (1)
openshift/test/generated/zz_generated.annotations.gois excluded by!**/generated/**
📒 Files selected for processing (4)
openshift/test/tests.gotest/e2e/evpn.gotest/e2e/kubevirt.gotest/e2e/route_advertisements.go
💤 Files with no reviewable changes (1)
- test/e2e/route_advertisements.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/kubevirt.go
- openshift/test/tests.go
Comment on lines
+744
to
+771
| if useProxyDockerNetwork { | ||
| shellCmds := []string{ | ||
| fmt.Sprintf("ip addr flush dev %s", info.containerInterface), | ||
| } | ||
| // CreateNetwork infers IP family from the subnets passed to it; with nil | ||
| // subnets it creates an IPv4-only network, which disables IPv6 on the | ||
| // interface. Enable it so we can add IPv6 CUDN addresses. | ||
| if ipFamilies.Has(utilnet.IPv6) { | ||
| shellCmds = append(shellCmds, | ||
| fmt.Sprintf("sysctl -w net.ipv6.conf.%s.disable_ipv6=0", info.containerInterface)) | ||
| } | ||
| for i, subnet := range subnets { | ||
| _, ipNet, parseErr := net.ParseCIDR(subnet) | ||
| if parseErr != nil { | ||
| return nil, fmt.Errorf("failed to parse CUDN subnet %s: %w", subnet, parseErr) | ||
| } | ||
| prefixLen, _ := ipNet.Mask.Size() | ||
| addr := fmt.Sprintf("%s/%d", containerIPs[i], prefixLen) | ||
| shellCmds = append(shellCmds, | ||
| fmt.Sprintf("ip addr add %s dev %s", addr, info.containerInterface)) | ||
| } | ||
| if _, err := infraprovider.Get().ExecExternalContainerCommand(container, | ||
| []string{"sh", "-c", strings.Join(shellCmds, " && ")}); err != nil { | ||
| return nil, fmt.Errorf("failed to reassign IPs on container %s (network %s): %w", container.Name, networkName, err) | ||
| } | ||
| framework.Logf("Reassigned container %s (network %s) IPs to CUDN IPs: %v", container.Name, networkName, containerIPs) | ||
| } | ||
|
|
There was a problem hiding this comment.
info.containerIPs is not updated after proxy mode IP reassignment, causing wrong IPs in FRR neighbor entries.
In proxy mode, createEVPNExternalContainer returns info.containerIPs containing Docker's randomly-assigned IPs. After the shell commands reassign the container interface to CUDN addresses, info.containerIPs is never updated. This causes:
- Lines 784-787 to create FRR neighbor entries with wrong IPs
- Caller to receive wrong IPs via returned
info
🐛 Proposed fix: Update info.containerIPs after reassignment
if _, err := infraprovider.Get().ExecExternalContainerCommand(container,
[]string{"sh", "-c", strings.Join(shellCmds, " && ")}); err != nil {
return nil, fmt.Errorf("failed to reassign IPs on container %s (network %s): %w", container.Name, networkName, err)
}
+ info.containerIPs = containerIPs
framework.Logf("Reassigned container %s (network %s) IPs to CUDN IPs: %v", container.Name, networkName, containerIPs)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if useProxyDockerNetwork { | |
| shellCmds := []string{ | |
| fmt.Sprintf("ip addr flush dev %s", info.containerInterface), | |
| } | |
| // CreateNetwork infers IP family from the subnets passed to it; with nil | |
| // subnets it creates an IPv4-only network, which disables IPv6 on the | |
| // interface. Enable it so we can add IPv6 CUDN addresses. | |
| if ipFamilies.Has(utilnet.IPv6) { | |
| shellCmds = append(shellCmds, | |
| fmt.Sprintf("sysctl -w net.ipv6.conf.%s.disable_ipv6=0", info.containerInterface)) | |
| } | |
| for i, subnet := range subnets { | |
| _, ipNet, parseErr := net.ParseCIDR(subnet) | |
| if parseErr != nil { | |
| return nil, fmt.Errorf("failed to parse CUDN subnet %s: %w", subnet, parseErr) | |
| } | |
| prefixLen, _ := ipNet.Mask.Size() | |
| addr := fmt.Sprintf("%s/%d", containerIPs[i], prefixLen) | |
| shellCmds = append(shellCmds, | |
| fmt.Sprintf("ip addr add %s dev %s", addr, info.containerInterface)) | |
| } | |
| if _, err := infraprovider.Get().ExecExternalContainerCommand(container, | |
| []string{"sh", "-c", strings.Join(shellCmds, " && ")}); err != nil { | |
| return nil, fmt.Errorf("failed to reassign IPs on container %s (network %s): %w", container.Name, networkName, err) | |
| } | |
| framework.Logf("Reassigned container %s (network %s) IPs to CUDN IPs: %v", container.Name, networkName, containerIPs) | |
| } | |
| if useProxyDockerNetwork { | |
| shellCmds := []string{ | |
| fmt.Sprintf("ip addr flush dev %s", info.containerInterface), | |
| } | |
| // CreateNetwork infers IP family from the subnets passed to it; with nil | |
| // subnets it creates an IPv4-only network, which disables IPv6 on the | |
| // interface. Enable it so we can add IPv6 CUDN addresses. | |
| if ipFamilies.Has(utilnet.IPv6) { | |
| shellCmds = append(shellCmds, | |
| fmt.Sprintf("sysctl -w net.ipv6.conf.%s.disable_ipv6=0", info.containerInterface)) | |
| } | |
| for i, subnet := range subnets { | |
| _, ipNet, parseErr := net.ParseCIDR(subnet) | |
| if parseErr != nil { | |
| return nil, fmt.Errorf("failed to parse CUDN subnet %s: %w", subnet, parseErr) | |
| } | |
| prefixLen, _ := ipNet.Mask.Size() | |
| addr := fmt.Sprintf("%s/%d", containerIPs[i], prefixLen) | |
| shellCmds = append(shellCmds, | |
| fmt.Sprintf("ip addr add %s dev %s", addr, info.containerInterface)) | |
| } | |
| if _, err := infraprovider.Get().ExecExternalContainerCommand(container, | |
| []string{"sh", "-c", strings.Join(shellCmds, " && ")}); err != nil { | |
| return nil, fmt.Errorf("failed to reassign IPs on container %s (network %s): %w", container.Name, networkName, err) | |
| } | |
| info.containerIPs = containerIPs | |
| framework.Logf("Reassigned container %s (network %s) IPs to CUDN IPs: %v", container.Name, networkName, containerIPs) | |
| } |
🤖 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/e2e/evpn.go` around lines 744 - 771, The info.containerIPs field is not
updated after the IP reassignment shell commands are executed, causing the
returned info object to contain outdated Docker-assigned IPs instead of the new
CUDN IPs. After the ExecExternalContainerCommand call succeeds in the
useProxyDockerNetwork block, update info.containerIPs by assigning the
containerIPs variable (which contains the correct CUDN IP addresses) to
info.containerIPs so that subsequent code and callers receive the correct IP
addresses.
Contributor
|
@jechen0648: 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. |
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
1 participant
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.
📑 Description
Fixes #
Additional Information for reviewers
✅ Checks
How to verify it
Summary by CodeRabbit
Tests
Test Infrastructure