bpf: Skip policy enforcements for service loopback case #15321

aditighag · 2021-03-11T21:35:49Z

bpf: Skip policy enforcements for service loopback case

When an endpoint connects to itself via service clusterIP, we hairpin the
flow using a loopback IP address (configured using --ipv4-service-loopback-address).
The destination clusterIP (on egress) and loopback IP (on ingress) map to incorrect identities.
As a result, the subsequent egress and ingress policy enforcements lead to packet drops.

This is evident from the monitor output -

<- endpoint 1844 flow 0x96c8d52 identity 55108->unknown state new ifindex 0 orig-ip 0.0.0.0: 10.12.0.123:58242 -> 172.20.0.130:80 tcp SYN Policy verdict log: flow 0x96c8d52 local EP ID 1844, remote ID world, proto 6, egress, action deny, match none, 169.254.42.1:58242 -> 10.12.0.123:80 tcp SYN

Skip policy enforcements (ingress and egress) in such cases since we
should allow a pod to connect to itself.

Release note

Fix an issue where packets are dropped when a pod connects to itself via a service clusterIP.

Signed-off-by: Aditi Ghag aditi@cilium.io
Co-authored-by: Paul Chaignon paul@cilium.io
Fixes: #11593

aditighag · 2021-03-11T23:04:07Z

test-me-please

pchaigno

Just posting to keep the context on changes, but I have a new version locally. I added the same for the IPv6 path and removed policy verdict notifications, but I'm still testing changes to address my second comment (ingress path). It's a bit more tortuous than I thought.

bpf/bpf_lxc.c

joestringer

Fix itself LGTM, one minor nit for better code consistency. Would also be good to see an integration test to guard against regressions and run against different datapath modes.

bpf/bpf_lxc.c

pchaigno · 2021-03-12T21:41:12Z

The single failure is https://jenkins.cilium.io/job/Cilium-PR-K8s-1.20-kernel-4.9/953/testReport/junit/Suite-k8s-1/20/K8sDatapathConfig_AutoDirectNodeRoutes_Check_direct_connectivity_with_per_endpoint_routes/.
Let's see if it's a flake:
test-only --k8s_version=1.20 --kernel_version=4.9 --focus="K8sDatapathConfig AutoDirectNodeRoutes Check direct connectivity with per endpoint routes"

pchaigno · 2021-03-12T21:46:17Z

We've hit a complexity issue on 4.9:

2021-03-12T20:24:59.215993300Z level=error msg="Command execution failed" cmd="[tc filter replace dev lxc065036eb6f09 egress prio 1 handle 1 bpf da obj 878_next/bpf_lxc.o sec to-container]" error="exit status 1" subsys=datapath-loader
2021-03-12T20:24:59.216059006Z level=warning subsys=datapath-loader
2021-03-12T20:24:59.216099499Z level=warning msg="Prog section 'to-container' rejected: Argument list too long (7)!" subsys=datapath-loader
2021-03-12T20:24:59.216136474Z level=warning msg=" - Type:         3" subsys=datapath-loader
2021-03-12T20:24:59.216172463Z level=warning msg=" - Attach Type:  0" subsys=datapath-loader
2021-03-12T20:24:59.216223310Z level=warning msg=" - Instructions: 1737 (0 over limit)" subsys=datapath-loader
2021-03-12T20:24:59.216260150Z level=warning msg=" - License:      GPL" subsys=datapath-loader
2021-03-12T20:24:59.216295383Z level=warning subsys=datapath-loader
2021-03-12T20:24:59.216331021Z level=warning msg="Verifier analysis:" subsys=datapath-loader
2021-03-12T20:24:59.216366262Z level=warning subsys=datapath-loader
2021-03-12T20:24:59.216402404Z level=warning msg="Skipped 4596416 bytes, use 'verb' option for the full verbose log." subsys=datapath-loader
2021-03-12T20:24:59.221033201Z level=warning msg="1332: (71) r2 = *(u8 *)(r10 -84)" subsys=datapath-loader
2021-03-12T20:24:59.221035300Z level=warning msg="BPF program is too large. Proccessed 98305 insn" subsys=datapath-loader

pchaigno · 2021-04-01T07:54:13Z

I've removed the v1.9 backport label until we address the complexity issue.

brb · 2021-04-01T08:13:28Z

I've removed the v1.9 backport label until we address the complexity issue.

It's a very corner case for the policy enforcement. Shall we just say - this won't work on v1.9, so just use v1.10 instead if you want this?

pchaigno · 2021-04-01T08:15:27Z

It already works in v1.8, we can't break it in v1.9.

brb · 2021-04-01T08:22:25Z

Then we can list it as undefined behavior on < v1.10. If users are hit by this, then they 1) can switch to the socket-LB or 2) modify a netpol to accept packets from the svc loopback IP addr.

Test for PR cilium#15321 - tests the case where a pod connects to itself via service clusterIP when selected by a policy. Signed-off-by: Aditi Ghag <aditi@cilium.io>

[ upstream commit 799bc1 ] Test for PR #15321 - tests the case where a pod connects to itself via service clusterIP when selected by a policy. Signed-off-by: Aditi Ghag <aditi@cilium.io>

Test for PR #15321 - tests the case where a pod connects to itself via service clusterIP when selected by a policy. Signed-off-by: Aditi Ghag <aditi@cilium.io>

[ upstream commit 69f10ed ] Test for PR #15321 - tests the case where a pod connects to itself via service clusterIP when selected by a policy. Signed-off-by: Aditi Ghag <aditi@cilium.io>

[ upstream commit 69f10ed ] Test for PR cilium#15321 - tests the case where a pod connects to itself via service clusterIP when selected by a policy. backport-1.8: Resolved conflicts in test change and variable aligning Signed-off-by: Aditi Ghag <aditi@cilium.io> Signed-off-by: Glib Smaga <code@gsmaga.com>

[ upstream commit 69f10ed ] Test for PR #15321 - tests the case where a pod connects to itself via service clusterIP when selected by a policy. Signed-off-by: Aditi Ghag <aditi@cilium.io>

[ upstream commit 69f10ed ] Test for PR #15321 - tests the case where a pod connects to itself via service clusterIP when selected by a policy. backport-1.8: Resolved conflicts in test change and variable aligning Signed-off-by: Aditi Ghag <aditi@cilium.io> Signed-off-by: Glib Smaga <code@gsmaga.com>

[ upstream commit 69f10ed ] Test for PR #15321 - tests the case where a pod connects to itself via service clusterIP when selected by a policy. Signed-off-by: Aditi Ghag <aditi@cilium.io> Signed-off-by: Martynas Pumputis <m@lambda.lt>

aditighag added release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.7 labels Mar 11, 2021

aditighag requested review from a team and jibi March 11, 2021 21:35

maintainer-s-little-helper bot assigned jibi Mar 11, 2021

aditighag marked this pull request as draft March 11, 2021 21:38

pchaigno self-requested a review March 11, 2021 21:40

maintainer-s-little-helper bot assigned pchaigno Mar 11, 2021

pchaigno added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. labels Mar 11, 2021

aditighag force-pushed the pr/aditighag/svc-policy-loopback-fix branch from 91ba240 to c759535 Compare March 11, 2021 22:02

aditighag added the priority/release-blocker label Mar 11, 2021

aditighag force-pushed the pr/aditighag/svc-policy-loopback-fix branch 3 times, most recently from 3749b7f to 0bae7f5 Compare March 12, 2021 07:40

aditighag requested a review from joestringer March 12, 2021 07:55

maintainer-s-little-helper bot assigned joestringer Mar 12, 2021

pchaigno requested changes Mar 12, 2021

View reviewed changes

bpf/bpf_lxc.c Outdated Show resolved Hide resolved

bpf/bpf_lxc.c Outdated Show resolved Hide resolved

pchaigno force-pushed the pr/aditighag/svc-policy-loopback-fix branch 2 times, most recently from 078659e to 58fb824 Compare March 12, 2021 18:21

pchaigno mentioned this pull request Mar 12, 2021

v1.8: bpf: Skip policy enforcements for service loopback case #15336

Closed

joestringer reviewed Mar 12, 2021

View reviewed changes

bpf/bpf_lxc.c Outdated Show resolved Hide resolved

pchaigno reviewed Mar 12, 2021

View reviewed changes

bpf/bpf_lxc.c Outdated Show resolved Hide resolved

pchaigno approved these changes Mar 12, 2021

View reviewed changes

joestringer reviewed Mar 12, 2021

View reviewed changes

bpf/bpf_lxc.c Outdated Show resolved Hide resolved

aanm added backport-pending/1.9 and removed needs-backport/1.9 labels Mar 31, 2021

pchaigno removed the needs-backport/1.9 label Apr 1, 2021

joestringer mentioned this pull request Apr 20, 2021

Prepare for release v1.8.9 #15794

Merged

christarazi mentioned this pull request Apr 27, 2021

[v1.9] bpf: Skip policy enforcements for service loopback case #15709

Merged

This was referenced Apr 28, 2021

Prepare for release v1.10.0-rc1 #15896

Closed

Prepare for release v1.10.0-rc1 #15897

Merged

aditighag mentioned this pull request Apr 29, 2021

test: Extend the clusterIP tests with policy #15928

Merged

rolinh pushed a commit that referenced this pull request Apr 30, 2021

test: Extend the hairpin flow test with policy

69f10ed

Test for PR #15321 - tests the case where a pod connects to itself via service clusterIP when selected by a policy. Signed-off-by: Aditi Ghag <aditi@cilium.io>

christarazi added the backport-pending/1.9 label May 3, 2021

ti-mo added backport-done/1.9 and removed backport-pending/1.9 labels May 5, 2021

joestringer mentioned this pull request May 13, 2021

Prepare for release v1.9.7 #16131

Merged

skmatti mentioned this pull request Apr 12, 2022

Conntrack entry mismatch leads to policy enforcement on reply packet for service loopback case #19406

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

bpf: Skip policy enforcements for service loopback case #15321

bpf: Skip policy enforcements for service loopback case #15321

Uh oh!

aditighag commented Mar 11, 2021 •

edited

Loading

Uh oh!

aditighag commented Mar 11, 2021

Uh oh!

pchaigno left a comment

Uh oh!

Uh oh!

Uh oh!

joestringer left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pchaigno commented Mar 12, 2021 •

edited

Loading

Uh oh!

pchaigno commented Mar 12, 2021

Uh oh!

pchaigno commented Apr 1, 2021

Uh oh!

brb commented Apr 1, 2021

Uh oh!

pchaigno commented Apr 1, 2021

Uh oh!

brb commented Apr 1, 2021 •

edited

Loading

Uh oh!

Uh oh!

bpf: Skip policy enforcements for service loopback case #15321

bpf: Skip policy enforcements for service loopback case #15321

Uh oh!

Conversation

aditighag commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aditighag commented Mar 11, 2021

Uh oh!

pchaigno left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pchaigno commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pchaigno commented Mar 12, 2021

Uh oh!

pchaigno commented Apr 1, 2021

Uh oh!

brb commented Apr 1, 2021

Uh oh!

pchaigno commented Apr 1, 2021

Uh oh!

brb commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

aditighag commented Mar 11, 2021 •

edited

Loading

pchaigno commented Mar 12, 2021 •

edited

Loading

brb commented Apr 1, 2021 •

edited

Loading