Add Amazon VPC CNI route-agent handler - #4100
Conversation
|
Operator discovery: >>> Creating PR #6: submariner-operator feat/aws-vpc-cni-discovery -> devel |
|
🤖 Created branch: z_pr4100/sanek9/feat/aws-vpc-cni-handler |
|
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:
WalkthroughAdds an IPv4 AWS VPC CNI route-agent handler, registers it with plugin discovery and startup, programs ingress and CNI policy-table routes, handles Kubernetes events and reconciliation, and adds route-programming tests. ChangesAWS VPC CNI route-agent
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Kubernetes
participant AWSVPCHandler
participant Netlink
Kubernetes->>AWSVPCHandler: Pod, node, and endpoint events
AWSVPCHandler->>Netlink: Program ingress and policy routes
Netlink-->>AWSVPCHandler: Route state
AWSVPCHandler->>AWSVPCHandler: Reconcile tracked routes and CIDRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
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 |
|
Part of the proposal overview: #4101 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
pkg/routeagent_driver/handlers/awsvpc/tables.go (1)
167-186: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftFragile heuristic for discovering the local gateway VTEP.
localGatewayVTEPreturns the first route onvx-submarinerwith a non-nil, non-unspecifiedGw, assuming exactly one such route exists and it always points to the active gateway's VTEP. If the main table on a worker ever contains more than oneGw-bearing route on this link (e.g. a stale route left over from a prior gateway during failover, before cleanup completes, or a route from another mechanism), this silently picks an arbitrary one, which would then get replicated into every discovered AWS CNI PBR table viaensureTableRoute— potentially misrouting all remote-cluster traffic from workers until the state settles.Given
vtepForNodeLocked(ingress.go) already derives a VTEP directly viavxlan.GetVtepIPAddressFromfrom a known node IP, it would be more robust forlocalGatewayVTEPto do the same from the active gateway's known IP (if that's tracked in handler state) rather than reverse-engineering it from arbitrary existing routes on the link.🤖 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 `@pkg/routeagent_driver/handlers/awsvpc/tables.go` around lines 167 - 186, Update localGatewayVTEP to derive the VTEP with the same vxlan.GetVtepIPAddressFrom approach used by vtepForNodeLocked, using the active gateway’s tracked IP from Handler state. Remove the route-list iteration that selects the first non-unspecified Gw, and preserve nil behavior when the gateway identity or VTEP lookup is unavailable.pkg/routeagent_driver/handlers/awsvpc/handler_test.go (1)
65-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for CNI PBR table replication (tables.go).
The suite validates plugin selection, pod/node ingress routes, and gateway-local exclusion, but there's no test exercising
syncCNITableRoutesLocked/discoverCNIRoutingTables/ensureTableRoute— the worker-side remote-CIDR replication into AWS CNI policy tables, which is a core piece of this PR's stated purpose. Consider adding a case that seeds a fake "from<podIP>lookup<table>" rule on a worker node and asserts remote CIDR + VTEP-prefix routes get programmed into that table (and removed on cleanup).🤖 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 `@pkg/routeagent_driver/handlers/awsvpc/handler_test.go` around lines 65 - 130, Add coverage in the Amazon VPC CNI handler tests for worker-side CNI policy-table replication: seed a fake “from podIP lookup table” rule, then assert remote CIDR and VTEP-prefix routes are programmed into that table and removed during cleanup. Exercise the existing syncCNITableRoutesLocked, discoverCNIRoutingTables, and ensureTableRoute behavior while preserving the current route and gateway-local tests.
🤖 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 `@pkg/routeagent_driver/handlers/awsvpc/handler.go`:
- Around line 436-447: Update Handler to retain the pod informer cache store
during Init, then change programAllPodIngressRoutes to iterate over cached pod
objects instead of calling the CoreV1 Pods List API. Remove its ctx parameter
and update callers in TransitionToGateway and reconcile accordingly, preserving
onPod processing for every cached pod.
- Around line 358-385: Update the Handler state by adding and initializing
remoteEndpointSubnets keyed by endpoint identity, then revise syncRemoteEndpoint
to replace each endpoint’s recorded IPv4 subnet list on add/update and remove
that endpoint’s record on deletion. Rebuild remoteCIDRs from all remaining
endpoint subnet records before syncing routes and ingress policy, preserving
shared subnets until no active endpoint advertises them.
In `@pkg/routeagent_driver/handlers/awsvpc/ingress.go`:
- Around line 34-55: Update Handler.onPod and the delete handling around
removeIngressRouteByIP so the pod IP is captured separately before deletion and
passed through explicitly. Ensure removeIngressRouteByIP uses the captured IP
rather than re-deriving it from pod.Status, including for
DeletedFinalStateUnknown or stale delete objects, while preserving current add
and non-deleted behavior.
In `@pkg/routeagent_driver/main.go`:
- Line 169: Move awsvpc.NewHandler from the unconditional handler setup into the
for _, family := range cidr.ExtractIPFamilies(env.ClusterCidr) loop, passing
family instead of k8snet.IPv4. Remove the existing hardcoded instantiation so
IPv6-only clusters skip the IPv4-specific handler while dual-stack clusters
initialize it per family.
---
Nitpick comments:
In `@pkg/routeagent_driver/handlers/awsvpc/handler_test.go`:
- Around line 65-130: Add coverage in the Amazon VPC CNI handler tests for
worker-side CNI policy-table replication: seed a fake “from podIP lookup table”
rule, then assert remote CIDR and VTEP-prefix routes are programmed into that
table and removed during cleanup. Exercise the existing
syncCNITableRoutesLocked, discoverCNIRoutingTables, and ensureTableRoute
behavior while preserving the current route and gateway-local tests.
In `@pkg/routeagent_driver/handlers/awsvpc/tables.go`:
- Around line 167-186: Update localGatewayVTEP to derive the VTEP with the same
vxlan.GetVtepIPAddressFrom approach used by vtepForNodeLocked, using the active
gateway’s tracked IP from Handler state. Remove the route-list iteration that
selects the first non-unspecified Gw, and preserve nil behavior when the gateway
identity or VTEP lookup is unavailable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d3a8662-b396-49fe-a565-0f7b56eec40b
📒 Files selected for processing (7)
pkg/cni/plugins.gopkg/routeagent_driver/handlers/awsvpc/handler.gopkg/routeagent_driver/handlers/awsvpc/handler_test.gopkg/routeagent_driver/handlers/awsvpc/ingress.gopkg/routeagent_driver/handlers/awsvpc/tables.gopkg/routeagent_driver/handlers/handlers_test.gopkg/routeagent_driver/main.go
eda9f83 to
33a6341
Compare
Program VTEP host routes and node-ingress PBR for Amazon VPC CNI so active-gateway return traffic and secondary-ENI tables work on EKS. Signed-off-by: sanek9 <sanya0996@gmail.com>
33a6341 to
8622e3c
Compare
|
Hi @sanek9, thanks for tackling #3697. A few Qs: A. Do we really need a dedicated handler here? The core issue is replicating routes to custom PBR tables. Could we extend the existing apiVersion: v1 It should be generic (works for any kubeproxy based CNI using custom tables) B. Gateway ingress datapath The PR programs per-pod /32 routes on the gateway. Submariner's design relies on the CNI to handle ingress datapath after IPsec decryption - we'd like to stick with this approach. Can you clarify:
Thanks! |
|
Hi @yboaron! Following your advice, I have prepared a branch with the https://github.com/sanek9/submariner/tree/feat/configurable-route-tables And I set AI to work on figuring it out. On a personal note, it feels like there are quite a lot of AWS-specific requirements... You can't just join any cluster out of the box. You need to add the CIDR to the security group... For someone setting this up for the first time, it's a bit tricky. For instance, we use Karpenter, where as far as I understand, setting Below, I'm attaching the answer to why 1. Why Per-Pod Routes Are Required for Unicast VXLANA shared subnet can only be used when it has one next hop. With AWS VPC CNI, For example: Installing a route such as: would send traffic for A Prefix aggregation is possible only when the CNI guarantees a distinct, Consequently, per-pod 2. AWS PBR replication: return-path diagnosisDate: 2026-07-30 Change under testThe AWS cluster runs: The following ConfigMap was added in apiVersion: v1
kind: ConfigMap
metadata:
name: submariner-route-tables
data:
tables: "2,3"Every route-agent restarted and logged: PBR result: fixedOn AWS non-gateway node Policy route lookups therefore select the local gateway VTEP: This confirms that the ConfigMap feature fixes the AWS worker-side PBR gap End-to-end result: still failing
To make the return path observable, an explicit pinned probe was created:
TCP from connector to listener timed out: Important correction: AWS gateway underlay route is expectedThe websites gateway has the correct forward path to its local listener: The tunnel/XFRM policies match both directions for the cluster CIDRs: After the reply returns from websites to the AWS gateway, the AWS gateway This is not itself a bug. As clarified by yboaron in Submariner PR #4099, For AWS VPC CNI, the pod IP is a secondary VPC ENI IP. The VPC fabric is What is proven
Root cause: AWS forwarding and Security Group configurationAWS credentials were renewed and the VPC configuration was read after the 1. Source/destination check is enabled on the AWS gatewayThe active AWS gateway is EC2 instance For the ingress flow, the gateway forwards a decrypted packet whose source is 2. Worker ENI Security Group denies TCP from the remote pod CIDRThe source worker The probe pod IP The SG has no TCP ingress CIDR rule for Ruled out
Required test configurationThe following changes were applied for the current validation:
The same pinned test then succeeded: The AWS gateway still selected the expected CNI/VPC route: Persistent configuration for new clustersTreat these settings as cloud infrastructure configuration, ideally managed in
For each newly joined cluster, validate both layers: CleanupTemporary namespace |
|
Hi @sanek9! Thanks for the detailed lab report and for implementing the ConfigMap approach - it's great to see it validated in your environment! Honestly, I'm finding it challenging to follow all the requirements and changes across the discussion. To help both the broader community and me understand and provide feedback, could you provide a top-down overview of what you're trying to achieve? What Would Help
It seems like there are multiple independent problems here: Problem A: Worker egress routing
Problem B: Worker ingress routing
Problem C: AWS infrastructure prerequisites
Other problems? DNS/CoreDNS related issues? Can you send a clear top-down explanation as a Submariner enhancement proposal?
Bottom line, we want to support this work, but let's organize it so the community can engage effectively. Does that make sense? Thanks again for your persistence on this! |
|
Hi @yboaron! Thank you for your patience and understanding. To be honest, I've started getting a bit confused by all of this myself. I will try to draft the enhancement proposals. I don't know yet how long it will take me, but as I understand it, I need to create 3 enhancement proposals:
I will need some time for this.... so I'll submit them as they are ready... |
|
Hi @sanek9! Thanks for understanding, appreciate your willingness to document this properly Take whatever time you need, getting it right is more important than speed. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further |
3 participants
What this PR does / why we need it
Amazon VPC CNI (EKS) is not a kube-proxy / OVN-style overlay. Pods get VPC ENI
addresses, and AWS CNI installs custom PBR tables for secondary ENIs. The
existing kubeproxy VXLAN route-agent path assumes a different model: routes in
the main table and a shared host network path that “just works” once VXLAN is up.
On EKS that is not enough:
mainare ignored fortraffic that AWS CNI sends through its ENI-specific tables, so packets leave
via the VPC default route instead of Submariner (see Submariner Route Agent fails to populate custom PBR tables created by AWS CNI, causing intermittent cross-cluster connectivity loss #3697).
VTEP. Putting node InternalIP
/32intomainvia VTEP breaks VXLAN underlay(FDB destination is the node IP). Pod
/32via VTEP belongs inmain; node/32must live in a dedicated PBR table selected only for cable/remote ingress.Security Groups typically drop foreign pod CIDRs on the VPC path, so “just open
the VPC route” is not a reliable substitute for a correct Submariner datapath.
A separate
awsvpchandler keeps this logic out of the generic kubeproxy pathand activates only when
SUBMARINER_NETWORKPLUGIN=amazon-vpc-cni(set by theoperator discovery companion PR).
What it does:
/32via VTEP inmain; node/32in a dedicated PBR tablevx-submarinerexistsFixes / addresses
Depends on / related
amazon-vpc-cniChecklist
Cross-links (this series)
Made with Cursor
Summary by CodeRabbit
amazon-vpc-cni) to the supported network plugins list./32pod ingress routes via VTEPs and manages gateway node ingress routing using dedicated policy-based routing tables.