Conversation
dosubot
bot
added
the
size:L
Summary of ChangesHello @zbb88888, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the network address translation capabilities of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
这个 PR 增加了对 SNAT EIP 和 FIP EIP 之间 hairpin 模式的支持，允许 VPC 内的 Pod 通过 EIP 地址访问其他 Pod。实现方式是引入了一个新的 HAIRPIN_SNAT iptables 链，并在 add_snat 和 del_snat 中增加了相应的逻辑来管理 hairpin 规则。整体实现逻辑清晰，注释也很详尽。我的审查意见主要集中在提高 shell 脚本的健壮性上。目前的实现多处使用了 iptables-save | grep 的方式来检查规则是否存在，这种方式比较脆弱，容易因为 iptables-save 输出格式的变化而出错。我建议改用 iptables -C 命令来检查规则，这样更可靠，也更符合 iptables 的最佳实践。
zbb88888
force-pushed
the
fix-nat-gw-fip-not-ping-snat
branch
from
56ecfa5 to
4038d82
Compare
|
/gemini review |
zbb88888
force-pushed
the
fix-nat-gw-fip-not-ping-snat
branch
from
4038d82 to
1913851
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for hairpin SNAT, enabling internal services to communicate via their external IPs. The changes in nat-gateway.sh are well-implemented, adding new functions to manage hairpin rules and integrating them into the existing SNAT logic. The approach to detect internal CIDRs and conditionally add hairpin rules is sound. I have one suggestion to improve the performance of the add_hairpin_snat function by reducing redundant iptables-save calls.
zbb88888
force-pushed
the
fix-nat-gw-fip-not-ping-snat
branch
from
1913851 to
b6a1e6c
Compare
Signed-off-by: zbb88888 <jmdxjsjgcxy@gmail.com>
zbb88888
force-pushed
the
fix-nat-gw-fip-not-ping-snat
branch
from
b6a1e6c to
16472bc
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces hairpin SNAT functionality to the nat-gateway.sh script, enabling internal VMs to access other internal VMs via Floating IPs. Key changes include adding hairpin-snat-add and hairpin-snat-del commands, creating a new HAIRPIN_SNAT iptables chain, and implementing is_internal_cidr to determine if a SNAT rule requires hairpin SNAT. The add_snat and del_snat functions were updated to conditionally manage hairpin SNAT rules. The review comments suggest a performance optimization for both add_hairpin_snat and del_hairpin_snat functions, recommending that the iptables-save command be executed once outside the processing loop to cache results and avoid redundant calls for each rule.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for hairpin SNAT in the VPC NAT gateway, enabling internal VMs to access services on other internal VMs via their external IPs (EIP/FIP). The modifications in nat-gateway.sh correctly implement the hairpin logic by adding a new HAIRPIN_SNAT iptables chain and associated rules. The approach to automatically manage hairpin rules alongside SNAT rules is well-designed. The e2e tests in e2e_test.go have been appropriately expanded to validate this new feature. My review has identified a minor robustness issue in the shell script and a bug within the new e2e tests that should be addressed.
zbb88888
force-pushed
the
fix-nat-gw-fip-not-ping-snat
branch
from
eb9ef16 to
671026f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
此 PR 增加了对 hairpin SNAT 的支持，允许内部虚拟机通过其外部 IP (EIP/FIP) 相互访问。这是通过在 nat-gateway.sh 中添加一个新的 HAIRPIN_SNAT iptables 链和相关逻辑来实现的，当管理内部 CIDR 的 SNAT 规则时，会自动添加/删除 hairpin 规则。代码变更结构清晰，并包含了相应的 E2E 测试来验证新功能。
我的审查主要集中在提高 shell 脚本的健壮性和修复新 E2E 测试中的一个错误。总的来说，这是一次很棒的更新。
|
|
||
| // Verify hairpin SNAT rule is created for shared SNAT as well | ||
| ginkgo.By("Verifying hairpin SNAT rule exists for shared snat") | ||
| verifyHairpinSnatRule(vpcNatGwPodName, overlaySubnetV4Cidr, shareEip.Spec.V4ip, true) |
There was a problem hiding this comment.
这里的 shareEip 变量引用的是用于创建的初始对象，其中 Spec.V4ip 是空的，因为 IP 是动态分配的。测试应该在验证 hairpin 规则之前获取更新后的 IptablesEIP 对象，以从其 status 中获取已分配的 IP。使用 shareEip.Spec.V4ip 会传递一个空的 EIP 字符串，导致测试失败。
| verifyHairpinSnatRule(vpcNatGwPodName, overlaySubnetV4Cidr, shareEip.Spec.V4ip, true) | |
| verifyHairpinSnatRule(vpcNatGwPodName, overlaySubnetV4Cidr, iptablesEIPClient.Get(sharedEipName).Status.IP, true) |
zbb88888
force-pushed
the
fix-nat-gw-fip-not-ping-snat
branch
from
671026f to
902316f
Compare
zbb88888
force-pushed
the
fix-nat-gw-fip-not-ping-snat
branch
from
902316f to
5273855
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for hairpin SNAT, which is crucial for enabling communication between internal VMs using their external Floating IPs. The changes involve modifications to the nat-gateway.sh script to manage hairpin iptables rules and additions to the E2E tests to verify this new functionality. The implementation is mostly solid, and the tests are comprehensive. I've identified one area for improvement in the shell script to make rule deletion more robust. Overall, this is a great addition that addresses an important networking scenario.
|
I think this would be an interesting feature to have |
@SkalaNetworks I will finish it, too busy recently! |
Issue #5572 问题分析
场景：
- VM A (10.0.1.6) 配置 SNAT 访问外网，EIP 为 10.1.69.219
- VM B (10.0.1.11) 绑定 FIP，EIP 为 10.1.69.216
- VM A 和 VM B 在同一内网 10.0.1.0/24
问题：VM A 无法 ping VM B 的 FIP (10.1.69.216)
根因分析：
VM A (10.0.1.6) ---> FIP EIP (10.1.69.216)
|
NAT GW DNAT
↓
dst: 10.0.1.11 (但 src 还是 10.0.1.6)
|
VM B 收到包，发现源是同子网
↓
直接回包给 10.0.1.6 (绕过 NAT GW)
|
VM A 收到来自 10.0.1.11 的包，但期望来自 10.1.69.216
↓
连接失败（不对称路由）
解决方案：添加 hairpin SNAT 规则
iptables -t nat -A HAIRPIN_SNAT -s 10.0.1.0/24 -d 10.0.1.0/24 -j SNAT --to-source <SNAT_EIP>
这样源地址被改成 EIP，VM B 回包时会发往 EIP，强制经过 NAT GW，conntrack 自动完成反向转换。
---
重新评估当前实现
1. 耦合问题：hairpin 逻辑嵌入在 add_snat/del_snat 里，如果将来要单独管理 hairpin 规则会比较麻烦
2. 多 SNAT 场景：如果同一个 internalCIDR 有多个 SNAT 规则（不同 EIP），当前实现只会用第一个 EIP 创建 hairpin 规则，删除时也只删除对应的那条
3. 与 FIP 的关系：实际上 hairpin 只在有 FIP 的时候才需要，但当前实现是只要 SNAT 的 CIDR 是内网就添加
|
2 participants
Pull Request
What type of this PR
Examples of user facing changes:
add_snat(eip,cidr) ├─ 添加 SHARED_SNAT（幂等：已存在则跳过） └─ 如果 cidr == vpcCidr → add_hairpin_snat(eip,cidr) ├─ 相同规则存在 → 跳过 ├─ 不同规则存在 → 警告并跳过 └─ 无规则 → 添加 del_snat(eip,cidr) ├─ 删除 SHARED_SNAT（幂等：不存在则跳过） └─ 如果 cidr == vpcCidr → del_hairpin_snat(eip,cidr) ├─ 规则存在 → 删除 └─ 规则不存在 → 跳过公网场景使用 vpc-nat-gw 只允许绑定一个子网，也只允许路由通一个子网。（vpc-nat-gw controller 需要简化 vpc 内其他路由的逻辑）
Which issue(s) this PR fixes
Fixes #(issue-number)
#5572 (comment)