-
Notifications
You must be signed in to change notification settings - Fork 254
feat: snat azuredns to node ip in podsubnet scenarios #3861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR modifies SNAT (Source NAT) behavior for Azure DNS traffic in pod subnet scenarios to consistently use the node IP instead of the primary IP across different networking modes. This ensures consistency between VNet scale cases (which already SNAT to node IP) and pod subnet cases.
- Changes Linux iptables rules to SNAT Azure DNS traffic to host primary IP instead of NC primary IP
- Updates Windows NAT policy to remove explicit VirtualIP specification for DNS traffic
- Updates corresponding unit tests to reflect the new SNAT target IP addresses
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
cns/restserver/internalapi_linux.go | Modified SNAT rules to use req.HostPrimaryIP instead of ncPrimaryIP for DNS traffic |
cns/restserver/internalapi_linux_test.go | Updated test expectations to reflect new SNAT target IP (10.0.0.4 instead of 240.1.2.1) |
cni/network/network_windows.go | Removed VirtualIP specification from Windows NAT policy for DNS traffic |
cni/network/network_test.go | Updated Windows test expectations to match new NAT policy structure |
cni/network/invoker_cns.go | Changed DNS SNAT rules to use snatHostIPJump instead of snatPrimaryIPJump |
cni/network/invoker_cns_test.go | Updated test expectations for new SNAT target IP (10.0.0.3 instead of 10.0.1.20) |
/azp run Azure Container Networking PR |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Giving a weak approval on this. It looks reasonable to me, but I want a second pair of eyes on this, as I haven't worked directly on CNI changes lately.
@QxBytes The changes look okay, but didn't we decide to do this based on a config parameter? |
This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days |
Reason for Change:
Changes podsubnet windows and linux (including cilium case) to snat azure dns traffic to node ip. Currently vnet scale case snats to node ip but podsubnet cases do not. Queried for cx that have policies which rely on previous behavior and no cases are found.
Issue Fixed:
Requirements:
Notes:
Affects only dns traffic (short connections)
Tested:
1.6.24 cns (azure linux 2.0) upgrading to this version (1.7.x) (cns base image now azure linux 3.0)
When cni adds the iptables rules they should use whatever the node maps the iptables command to
For following,
nslookup google.com 168.63.129.16
succeeds, tcpdump/packet capture on node shows snat to the node ip instead of the primary ipPodsubnet cilium linux
Podsubnet azure linux
Podsubnet azure windows
Vnetscale cilium linux
Vnetscale azure linux
Vnetscale azure windows
Mostly green run (this pr does not affect overlay): https://msazure.visualstudio.com/One/_build/results?buildId=131651989&view=results