Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Perform reverse NAT at host interface #15354

Merged
merged 1 commit into from
Aug 2, 2021
Merged

Perform reverse NAT at host interface #15354

merged 1 commit into from
Aug 2, 2021

Conversation

krishgobinath
Copy link
Contributor

@krishgobinath krishgobinath commented Mar 16, 2021 •
edited
Loading

Perform egress Reverse NAT at host interface.
Fixes: #11914

Description: if ENABLE_EGRESS_REVNAT_AT_HOST flag is set, conntrack lookup happens for all outbound traffic at host interface and reverse NAT translation happens at host Interface.

Signed-off-by: Gobinath gobinathk@google.com

@krishgobinath krishgobinath requested review from a team March 16, 2021 01:00
@krishgobinath krishgobinath requested a review from a team as a code owner March 16, 2021 01:00
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Mar 16, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Mar 16, 2021
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@pchaigno pchaigno added dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Mar 25, 2021
pchaigno
pchaigno requested changes Mar 25, 2021
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request message describes what the code changes implement, but not why these changes are necessary. Could you extend on that and also include the same in the commit message itself?

Right now it's hard to review as it's unclear what you're trying to fix/improve.

@pchaigno
Copy link
Member

You will also need to rebase and sign-off your commit.

@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 25, 2021
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 25, 2021
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@krishgobinath
Copy link
Contributor Author

krishgobinath commented Jul 22, 2021 •
edited
Loading

Thanks. I left a few non-blocking nit comments. Please address them in a separate PR. Also, could you update the KPR documentation in the follow-up PR?

I have updated the PR with requested changes. Regarding documentation, we can address once we un-hide the netfilter-compatible-mode flag, after fixing issues mentioned here. #16166

@joestringer
Copy link
Member

test-me-please

@joestringer
Copy link
Member

Searching for the "command ... failed" message from the output of the ci-gke job, it looks like known issue #16938.

@krishgobinath krishgobinath mentioned this pull request Jul 23, 2021
Closed
@krishgobinath
Copy link
Contributor Author

krishgobinath commented Jul 23, 2021 •
edited
Loading

Regarding test-1.19-5.4 , test K8sDatapathConfig Host firewall With VXLAN failed.
It was recorded earlier here. The same error code [Operation Timeout] was seen by other people as well. Looks like this is known Issue.

@krishgobinath krishgobinath mentioned this pull request Jul 23, 2021
Closed
@krishgobinath
Copy link
Contributor Author

Created CI issue to tracktest-gke failures.

@joestringer
Copy link
Member

Thanks for triaging the failures, this helps us track & understand flakes in the system and also understand whether this PR has any link to the failures.

At a high level, the PR looks good to me (ie in terms of testing, how risky it is, general structure). I didn't spend a lot of time looking at the details (comments, exact flows for traffic when the flag is enabled etc.). Assuming @brb @pchaigno are happy with the PR now I think it should be ready-to-merge.

@joestringer
Copy link
Member

Github is reporting these conflicting files:
install/kubernetes/cilium/templates/cilium-configmap.yaml
pkg/option/config.go

If you rebase, we can retrigger the smoke tests and as long as those pass (ie no build breakage) then no further testing should be necessary.

Perform reverse NAT at Host Interface
2da68e2 
Description: if NETFILTER_COMPAT_MODE flag is set, guarantees that traffic will
pass-through kernel netfilter and that Iptables rules are enforced on traffic.

Problem: if Nodeport traffic ingress on the same node as a backend pod's node,
NAT and reverse NAT translations at 2 different interfaces.
On ingress path, NAT happens at Node's host interface, pass through kernel stack.
on egress path reverse NAT happen at Pod's Veth interface and bypasses kernel stack.
This asymmetric behavior, reflects in kernel conntrack entries and having a IPTABLE
rule to DROP any packets if conntrack state is INVALID which results in packet DROP.

Fixes: cilium#11914

Signed-off-by: Gobinath Krishnamoorthy <gobinathk@google.com>
@krishgobinath
Copy link
Contributor Author

krishgobinath commented Jul 27, 2021 •
edited
Loading

Github is reporting these conflicting files:
install/kubernetes/cilium/templates/cilium-configmap.yaml
pkg/option/config.go

If you rebase, we can retrigger the smoke tests and as long as those pass (ie no build breakage) then no further testing should be necessary.

@brb @pchaigno I don't think any requested changes are pending here. Can one of you merge this PR ?

@pchaigno
Copy link
Member

pchaigno commented Jul 30, 2021 •
edited
Loading

test-me-please

I'll try to have a look tomorrow morning or late this evening. Hopefully, we can triage all test failures to existing flakes and merge.

@krishgobinath krishgobinath mentioned this pull request Jul 30, 2021
Closed
@krishgobinath
Copy link
Contributor Author

krishgobinath commented Jul 30, 2021 •
edited
Loading

  1. runtime (test-runtime) failure noticed earlier and tracked here
  2. gke-stable (test-gke) failure noticed earlier and tracked here

@pchaigno
Copy link
Member

As noticed by @krishgobinath, the two failing required tests failed with known flakes. Other tests are passing and all team review requests are covered. Marking ready to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 30, 2021
@nathanjsweet nathanjsweet merged commit dc4b06e into cilium:master Aug 2, 2021
1.10.0 automation moved this from In progress to Done Aug 2, 2021
@krishgobinath krishgobinath deleted the revnat branch August 3, 2021 01:19
@nbusseneau nbusseneau mentioned this pull request Sep 1, 2021
Closed
nbusseneau added a commit to nbusseneau/cilium that referenced this pull request Sep 6, 2021
@nbusseneau
Revert "Perform reverse NAT at Host Interface"
2c14543 
This reverts commit dc4b06e.

Rationale:

- PR cilium#15354 introduced a new test that is frequently failing:
  `K8sServicesTest Checks service across nodes Tests NodePort BPF Tests with vxlan Test NodePort with netfilterCompatMode=true`
- This flake is tracked in issue cilium#17060.
- We are reverting the original PR so as to remove the flake from the
  CI until we have a fix and are confident the new test is not flaky.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
@nbusseneau nbusseneau mentioned this pull request Sep 6, 2021
Merged
borkmann pushed a commit that referenced this pull request Sep 7, 2021
@nbusseneau @borkmann
Revert "Perform reverse NAT at Host Interface"
9e2d217 
This reverts commit dc4b06e.

Rationale:

- PR #15354 introduced a new test that is frequently failing:
  `K8sServicesTest Checks service across nodes Tests NodePort BPF Tests with vxlan Test NodePort with netfilterCompatMode=true`
- This flake is tracked in issue #17060.
- We are reverting the original PR so as to remove the flake from the
  CI until we have a fix and are confident the new test is not flaky.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
@brb brb mentioned this pull request Sep 28, 2021
Closed
@krishgobinath krishgobinath mentioned this pull request Sep 30, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer left review comments

@Weil0ng Weil0ng Weil0ng approved these changes

@twpayne twpayne twpayne approved these changes

@kaworu kaworu kaworu approved these changes

@brb brb brb approved these changes

@pchaigno pchaigno pchaigno approved these changes

@borkmann borkmann Awaiting requested review from borkmann

Assignees

@borkmann borkmann

@joestringer joestringer

Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.10.0
Done
Milestone
No milestone
9 participants
@krishgobinath @pchaigno @Weil0ng @brb @joestringer @twpayne @kaworu @borkmann @nathanjsweet