@julianwiedmann @gentoo-root Thank you for review! When using iptables masquerade instead of bpf masquerade, SNAT is not required. I plan to enhance the cDefinesMap and explanation of suggested change. I will re-request after that.

@julianwiedmann @gentoo-root @ti-mo Thank you for your patience. The previous changes had some ambiguities. To clarify the bug and the fix, I added cDefinesMap["ENABLE_IPTABLES_MASQUERADE_IPV4"] and cDefinesMap["ENABLE_IPTABLES_MASQUERADE_IPV6"] to explicitly handle SNAT accordingly. Additionally, I provided a clear and detailed explanation of the issue #36572 and the commit description.
The issue is that when using iptables masquerade, the source address is already nat-ed by iptables. So, we should skip SNAT when using iptables masquerade. If we don't skip SNAT, the source address is nat-ed twice. It also unnecessarily occupies entry in the SNAT_MAPPING map. And if it exceeds SNAT_COLLISION_RETRIES, it leads to connection failures due to DROP_NAT_NO_MAPPING.(In the beginning, since there are no entries in the SNAT_MAPPING, source port is preserved. However, as entries accumulate, port nat occurs)
This patch ensures that SNAT is skipped when using iptables masquerade. It prevents duplicate SNAT, avoids unnecessary use of the SNAT_MAPPING map, and mitigates connection failures caused by packet drops. Additionally, by eliminating the complexity caused by duplicate SNAT, it reduces troubleshooting time and clarifies the masquerade process.
The new define was named based on the code reference below.

*Update Branch(Rebase)

I don't see how this can be correct for the case of using BPF Nodeport without BPF masquerade. Please elaborate a lot more.

We're intentionally tracking connections in the SNAT engine for this case, and applying SNAT as needed to avoid conflicts between LB-to-remote-backend connections and host-originating / iptables-masqueraded connections.

@julianwiedmann Thank you for your detailed explanation. I agree with your opinion, After reviewing the case you explained, I realized that skipping SNAT in iptables-masquerade cannot prevent the conflict with LB-to-remote-backend.

flow1: client -> node:nodeport(32000) >>>> SNAT engine >>>> node:natedPort(34000) -> remote-backend
flow2: host -> external >>>> iptables-masquerade >>>> host:natedPort(34000) -> external

I hadn't considered that scenario, so thank you for pointing it out. This PR should not be merged. To elaborate, during my attempts to reproduce this issue, I observed a rare case where the SNAT engine performed SNAT on an egress packet, but the reverse NAT for the response packet failed. This occurred when the --bpf-nat-global-max limit was reached. I tried skipping SNAT in order to resolve this issue and packet drop, but it's clear now that skipping SNAT is not the right approach. Instead, I will focus on analyzing why reverse NAT occasionally fails. If you have encountered or received reports about a similar issue, I'd greatly appreciate it if you could share them.
In summary, this PR should not be merged. I will focus on the reverse NAT issue and submit a new PR with an appropriate fix.

In summary, this PR should not be merged. I will focus on the reverse NAT issue and submit a new PR with an appropriate fix.

Thank you for the feedback! Looking forward to what you come up with :).

I'll flip this PR to draft then for now.

I observed a rare case where the SNAT engine performed SNAT on an egress packet, but the reverse NAT for the response packet failed.

@julianwiedmann Hello, I'd like to share the root cause of the issue that caused REV NAT to fail, as mentioned earlier. This issue was mitigated by #35304, which introduced the idea of recreating evicted reverse entries during NAT, using the otuple. I think this is a fantastic approach!
While #35304 has significantly reduced the issue, there are still rare cases where the reverse entry is evicted when snat_v4_rev_nat_handle_mapping() attempts to find it.
To address this, I'd like to propose an alternative: if snat_v4_rev_nat_handle_mapping() cannot find the reverse entry, it could attempt to recreate it using the original entry(assuming the original entry has not been deleted). What are your thoughts on this approach?

I observed a rare case where the SNAT engine performed SNAT on an egress packet, but the reverse NAT for the response packet failed.

@julianwiedmann Hello, I'd like to share the root cause of the issue that caused REV NAT to fail, as mentioned earlier. This issue was mitigated by #35304, which introduced the idea of recreating evicted reverse entries during NAT, using the otuple. I think this is a fantastic approach! While #35304 has significantly reduced the issue, there are still rare cases where the reverse entry is evicted when snat_v4_rev_nat_handle_mapping() attempts to find it. To address this, I'd like to propose an alternative: if snat_v4_rev_nat_handle_mapping() cannot find the reverse entry, it could attempt to recreate it using the original entry(assuming the original entry has not been deleted). What are your thoughts on this approach?

Sorry, I don't think I'll have the cycles to co-design a solution for the problem you encountered. But maybe you can connect with @sugangli and discuss this in more detail - hack up a minimal reproducer that demonstrates the problem etc?

Hi @gyutaeb,

IIUC, retrieving the original entry from the rev nat path could be a bit tricky. Could you give an example on how it could be done? Feel free to tag me in the new PR once you are ready.

Hi, @sugangli
I will reach out to you once the code is ready. I'm excited to collaborate with you. Currently, I am working on a PR related to #31213, and it will take a few more weeks to complete. Thank you for your support!

Hi @julianwiedmann @sugangli.
I have examined the idea mentioned above and would like to share the results.

First of all, recreating the reverse entry in snat_v4_rev_nat_handle_mapping() seems too difficult. This is because recreating it requires the otuple, which is only stored in the reverse entry. To recreate reverse entries deleted by LRU, we could store the otuple in a separate data structure. However, this approach would unnecessarily increase software complexity, making it less desirable.
Instead, I have a new idea to mitigate network failures caused by LRU eviction. The idea is to recreate the deleted original entry in snat_v4_rev_nat_handle_mapping().

The issue occurs as follows:

• snat_v4_rev_nat_handle_mapping() correctly finds the reverse entry.
• When the egress packet reaches snat_v4_nat_handle_mapping(), the original entry has already been evicted by LRU.
• Since the original entry is missing, snat_v4_new_mapping() assigns a new source port for NAT.
• If this happens, the endpoint socket detects a port change and sends an RST packet.

This issue can still occur, and recreating the original entry in snat_v4_rev_nat_handle_mapping() might be a good way to address it. I'd love to hear your thoughts on this. I plan to prepare a PoC code in the next few weeks.

Additionally, it would be helpful to provide a tool that allows users to determine whether their bpf-nat-global-max value is appropriate. Normally, the max value is not reached, but in cases where keepalive is not used, it can reach the limit. Moreover, when LRU eviction occurs, it can take time for users to determine whether it was caused by LRU.

Therefore, it would be beneficial to develop a tool like the one proposed in kubectl-cilium. There are two possible approaches: adding this functionality to the cilium CLI within cilium-agent, or, integrating it into kubectl-cilium.

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

This pull request has not seen any activity since it was marked stale.
Closing.