cilium icon indicating copy to clipboard operation
cilium copied to clipboard

bpf:nat: restore a NAT entry if its REV NAT is not found

Open sugangli opened this issue 1 year ago • 3 comments

NAT Map LRU behavior could evict any entry in the MAP. This change restore a reverse NAT entry when it is expected but not found. Previously, we were only relying on existence of forwarding NAT to determine if the NAT pair exists or not.

This fix is verified by using the same repro steps in #34833. After the fix, we saw much less timeout from the client, but the case like eviction happens between TCP SYN and TCP SYN ACK will still drop the returning packets.

Fixes: #34833

sugangli avatar Oct 08 '24 19:10 sugangli

/test

joestringer avatar Oct 09 '24 22:10 joestringer

Commit 50ad1eb429bee80a7480fdeb58c04cc20dc37fd4 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

Hi @joestringer, let me know if I need to add anything else to this PR.

sugangli avatar Oct 16 '24 16:10 sugangli

/test

sypakine avatar Oct 21 '24 15:10 sypakine

@gentoo-root Do you have intentions to review this PR or can you find someone else from @cilium/sig-datapath ?

joestringer avatar Oct 21 '24 21:10 joestringer

Could you please also add the IPv6 part of the fix?

Agree with Tom that this needs tests. I could see a bpf_host test under bpf/tests/ that walks through the desired packet flow, and reproduces the whole scenario you're fixing:

  1. transmit a packet, have it SNATed
  2. receive a reply for that connection, check that it gets revSNATed
  3. manually delete the revSNAT entry
  4. receive a second reply, check that it no longer gets revSNATed
  5. transmit a second packet, validate that it re-creates the missing state
  6. receive a third reply, check that it gets revSNATed again

@julianwiedmann There are existing flows for 1 and 2. Adding following tests to cover 3 to 6. And I also added the support for ipv6.

sugangli avatar Oct 29 '24 18:10 sugangli

Commit 4fd8da215d6d5b463e1a980cfbaba05556cc2330 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

/test

youngnick avatar Nov 20 '24 05:11 youngnick

/test

sypakine avatar Nov 22 '24 17:11 sypakine

Commit f2261bdb10469d13bfe68cad4217ccee28cd4af3 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

Thanks, these changes all look good to me.

@julianwiedmann do you mind taking another look as well?

tommyp1ckles avatar Nov 26 '24 05:11 tommyp1ckles

lgtm, thank you! I still feel this is a bit hack-ish, and hope we can find a solution to this issue that covers the whole problem space.

@julianwiedmann +1. I remember we have a on-going effort to combine fwd NAT and rev NAT into a single entry? That could eliminate all the complexity here.

sugangli avatar Nov 26 '24 16:11 sugangli

/test

julianwiedmann avatar Nov 27 '24 17:11 julianwiedmann

Commit eb6cf24c65fdc6ecbac7f56070487a1d091f776c does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

/test

aanm avatar Nov 28 '24 12:11 aanm