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.

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

bpf: exclude pod's reply traffic from egress gateway logic #17869

Merged
merged 2 commits into from Nov 22, 2021

Conversation

jibi
Copy link
Member

@jibi jibi commented Nov 12, 2021

Currently all pod traffic matching source IP and destination CIDR of an
egress policy will be forwarded to an egress gateway.

This means we will incorrectly forward to an egress gateway also all
reply traffic from connections destined to a pod, breaking said
connections.

This commit fixes this by adding an additional check to make sure reply
traffic (i.e. connections not originating from a pod) is excluded from
the egress gateway logic.

Fixes: #17866
Signed-off-by: Gilberto Bertin gilberto@isovalent.com

@jibi jibi added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.10 feature/egress-gateway Impacts the egress IP gateway feature. labels Nov 12, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.6 Nov 12, 2021
@brb
Copy link
Member

brb commented Nov 12, 2021

I think we might want to have a CI test for it.

@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 Nov 15, 2021
@jibi jibi force-pushed the pr/jibi/egress-gw-exclude-reply-traffic branch from 61eecd0 to 5a20717 Compare November 17, 2021 09:10
@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 Nov 17, 2021
@cilium cilium deleted a comment from maintainer-s-little-helper bot Nov 17, 2021
@jibi
Copy link
Member Author

jibi commented Nov 17, 2021

/test

double checked locally that without the actual fix the new test fails:

K8sEgressGatewayTest tunnel disabled with endpointRoutes enabled egress gw policy both egress gw and basic connectivity work
at /home/jibi/go/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:527

[Expected command: kubectl exec -n kube-system log-gatherer-8rctj -- curl --path-as-is -s -D /dev/stderr --fail --connect-timeout 5 --max-time 20 http://10.0.0.176:80 -w "time-> DNS: '%{time_namelookup}(%{remote_ip})', Connect: '%{time_connect}',Transfer '%{time_starttransfer}', total '%{time_total}'"
To succeed, but it failed:
..

@jibi jibi requested review from pchaigno and brb November 17, 2021 09:11
@jibi jibi marked this pull request as ready for review November 17, 2021 09:11
@jibi jibi requested a review from a team November 17, 2021 09:11
@jibi jibi requested a review from a team as a code owner November 17, 2021 09:11
@jibi jibi force-pushed the pr/jibi/egress-gw-exclude-reply-traffic branch from 5a20717 to 426f8a6 Compare November 17, 2021 09:22
@kkourt kkourt self-requested a review November 17, 2021 09:29
@jibi jibi force-pushed the pr/jibi/egress-gw-exclude-reply-traffic branch 2 times, most recently from 079d4ca to e2497b4 Compare November 17, 2021 10:11
bpf/bpf_lxc.c Outdated Show resolved Hide resolved
bpf/lib/nodeport.h Outdated Show resolved Hide resolved
bpf/lib/nodeport.h Outdated Show resolved Hide resolved
test/k8sT/Egress.go Show resolved Hide resolved
test/k8sT/Egress.go Show resolved Hide resolved
@jibi jibi force-pushed the pr/jibi/egress-gw-exclude-reply-traffic branch from e2497b4 to a898866 Compare November 18, 2021 14:20
@jibi
Copy link
Member Author

jibi commented Nov 18, 2021

/test

@jibi
Copy link
Member Author

jibi commented Nov 18, 2021

/test-runtime

vm provisioning failed with

15:27:17  ==> runtime: Machine booted and ready!
15:27:17  ==> runtime: Checking for guest additions in VM...
15:27:17      runtime: The guest additions on this VM do not match the installed version of
15:27:17      runtime: VirtualBox! In most cases this is fine, but in rare cases it can
15:27:17      runtime: prevent things such as shared folders from working properly. If you see
15:27:17      runtime: shared folder errors, please make sure the guest additions within the
15:27:17      runtime: virtual machine match the version of VirtualBox you have installed on
15:27:17      runtime: your host and reload your VM.
15:27:17      runtime: 
15:27:17      runtime: Guest Additions Version: 6.0.0 r127566
15:27:17      runtime: VirtualBox Version: 6.1
15:27:17  ==> runtime: Setting hostname...
15:27:18  ==> runtime: Configuring and enabling network interfaces...
15:27:36  ==> runtime: Exporting NFS shared folders...
15:27:36  ==> runtime: Preparing to edit /etc/exports. Administrator privileges will be required...
15:27:36  ==> runtime: Mounting NFS shared folders...
15:53:02  Cancelling nested steps due to timeout
15:53:02  Sending interrupt signal to process

Job 'Cilium-PR-K8s-GKE' hit: #17545 (91.20% similarity)

@jibi jibi force-pushed the pr/jibi/egress-gw-exclude-reply-traffic branch from a898866 to abff505 Compare November 19, 2021 09:24
Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

🚀

@jibi jibi force-pushed the pr/jibi/egress-gw-exclude-reply-traffic branch from abff505 to 922efdf Compare November 19, 2021 12:56
@jibi jibi requested a review from pchaigno November 22, 2021 10:23
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

One non-blocking nit. Besides that 🚀 🌔

bpf/bpf_lxc.c Outdated Show resolved Hide resolved
@jibi jibi force-pushed the pr/jibi/egress-gw-exclude-reply-traffic branch from 922efdf to 9418bc4 Compare November 22, 2021 10:47
@jibi
Copy link
Member Author

jibi commented Nov 22, 2021

/test

Job 'Cilium-PR-K8s-1.22-kernel-4.9' hit: #17919 (92.89% similarity)

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDatapathConfig Host firewall With native routing and endpoint routes

Failure Output

FAIL: Kubernetes DNS did not become ready in time

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

bpf/bpf_lxc.c Show resolved Hide resolved
Currently all pod traffic matching source IP and destination CIDR of an
egress policy will be forwarded to an egress gateway.

This means we will incorrectly forward to an egress gateway also all
reply traffic from connections destined to a pod, breaking said
connections.

This commit fixes this by adding an additional check to make sure reply
traffic (i.e. connections not originating from a pod) is excluded from
the egress gateway logic.

Fixes: #17866
Signed-off-by: Gilberto Bertin <gilberto@isovalent.com>
to make it more explicit its purpose

Signed-off-by: Gilberto Bertin <gilberto@isovalent.com>
@jibi jibi force-pushed the pr/jibi/egress-gw-exclude-reply-traffic branch from 9418bc4 to 30a5753 Compare November 22, 2021 14:10
@jibi
Copy link
Member Author

jibi commented Nov 22, 2021

One last change: moved the

	if (ct_ret == CT_REPLY || ct_ret == CT_RELATED)
			goto skip_egress_gateway;

check in bpf_lxc before the policy lookup (to possibly avoid a non necessary map lookup)

@jibi
Copy link
Member Author

jibi commented Nov 22, 2021

/test

Job 'Cilium-PR-K8s-1.21-kernel-4.19' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sServicesTest Checks service across nodes Tests NodePort BPF Tests with direct routing Tests NodePort

Failure Output

FAIL: Request from testclient-hgtxh pod to service http://[fd04::12]:30084 failed

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.21-kernel-4.19 so I can create a new GitHub issue to track it.

@jibi
Copy link
Member Author

jibi commented Nov 22, 2021

/test-4.19

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 22, 2021
@gandro gandro merged commit c117362 into master Nov 22, 2021
@gandro gandro deleted the pr/jibi/egress-gw-exclude-reply-traffic branch November 22, 2021 18:11
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.6 Nov 23, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.6 Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/egress-gateway Impacts the egress IP gateway feature. kind/bug This is a bug in the Cilium logic. 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.
Projects
No open projects
1.10.6
Backport done to v1.10
Development

Successfully merging this pull request may close these issues.

Reply traffic from pods matching a policy is incorrectly being SNAT'ed
6 participants