Allow icmp fragmentation needed agent option #8218

fristonio · 2019-06-05T12:50:22Z

Fixes [NetPol] Allow ICMP messages of a certain type/code #8129
Add a cilium-agent flag allow-icmp-frag-needed which allows ICMP type3 Code 4 type packets which can are used for TCP Path MTU.

This change is

pkg/option/config.go

coveralls · 2019-06-05T13:07:17Z

Coverage decreased (-0.004%) to 44.066% when pulling 28e9724 on fristonio:allow-icmp-frag-needed into c703ce7 on cilium:master.

brb

Thanks for the contribution! Please see my comments below.

daemon/daemon_main.go

bpf/lib/policy.h

tgraf · 2019-06-06T07:55:20Z

This looks fantastic

tgraf · 2019-06-06T07:55:26Z

test-me-please

tgraf · 2019-06-06T10:40:40Z

@fristonio

00:55:59.097      runtime: Jun 06 08:51:33 runtime cilium-agent[28160]: level=warning msg="In file included from /var/lib/cilium/bpf/bpf_overlay.c:37:" subsys=daemon
00:55:59.097      runtime: Jun 06 08:51:33 runtime cilium-agent[28160]: level=warning msg="/var/lib/cilium/bpf/lib/policy.h:147:18: error: variable has incomplete type 'struct icmphdr'" subsys=daemon
00:55:59.097      runtime: Jun 06 08:51:33 runtime cilium-agent[28160]: level=warning msg="                struct icmphdr icmphdr;" subsys=daemon
00:55:59.097      runtime: Jun 06 08:51:33 runtime cilium-agent[28160]: level=warning msg="                               ^" subsys=daemon
00:55:59.097      runtime: Jun 06 08:51:33 runtime cilium-agent[28160]: level=warning msg="/var/lib/cilium/bpf/lib/policy.h:147:10: note: forward declaration of 'struct icmphdr'" subsys=daemon
00:55:59.097      runtime: Jun 06 08:51:33 runtime cilium-agent[28160]: level=warning msg="                struct icmphdr icmphdr;" subsys=daemon
00:55:59.097      runtime: Jun 06 08:51:33 runtime cilium-agent[28160]: level=warning msg="                       ^" subsys=daemon
00:55:59.097      runtime: Jun 06 08:51:33 runtime cilium-agent[28160]: level=warning msg="/var/lib/cilium/bpf/lib/policy.h:158:22: error: use of undeclared identifier 'ICMP_DEST_UNREACH'" subsys=daemon
00:55:59.097      runtime: Jun 06 08:51:33 runtime cilium-agent[28160]: level=warning msg="                if(icmphdr.type == ICMP_DEST_UNREACH && icmphdr.code == ICMP_FRAG_NEEDED)" subsys=daemon
00:55:59.097      runtime: Jun 06 08:51:33 runtime cilium-agent[28160]: level=warning msg="                                   ^" subsys=daemon
00:55:59.097      runtime: Jun 06 08:51:33 runtime cilium-agent[28160]: level=warning msg="/var/lib/cilium/bpf/lib/policy.h:158:59: error: use of undeclared identifier 'ICMP_FRAG_NEEDED'" subsys=daemon
00:55:59.097      runtime: Jun 06 08:51:33 runtime cilium-agent[28160]: level=warning msg="                if(icmphdr.type == ICMP_DEST_UNREACH && icmphdr.code == ICMP_FRAG_NEEDED)" subsys=daemon
00:55:59.097      runtime: Jun 06 08:51:33 runtime cilium-agent[28160]: level=warning msg="                                                                        ^" subsys=daemon
00:55:59.097      runtime: Jun 06 08:51:33 runtime cilium-agent[28160]: level=warning msg="3 errors generated." subsys=daemon

It will make sense to add ALLOW_ICMP_FRAG_NEEDED to the following list in bpf/Makefile:

LXC_OPTIONS = \
         -DSKIP_DEBUG \
         -DHAVE_LPM_MAP_TYPE \
         -DHAVE_LRU_MAP_TYPE \
         -DENABLE_IPV4 \
         -DENABLE_IPV4:-DHAVE_LPM_MAP_TYPE \
         -DENABLE_IPV4:-DHAVE_LPM_MAP_TYPE:-DHAVE_LRU_MAP_TYPE \
         -DENABLE_IPV6 \
         -DENABLE_IPV6:-DHAVE_LPM_MAP_TYPE \
         -DENABLE_IPV6:-DHAVE_LPM_MAP_TYPE:-DHAVE_LRU_MAP_TYPE \
         -DENABLE_IPV6:-DENABLE_IPV4 \
         -DENABLE_IPV6:-DENABLE_IPV4:-DENABLE_ROUTING \
         -DENABLE_IPV4:-DENABLE_IPV6:-DHAVE_LPM_MAP_TYPE:-DHAVE_LRU_MAP_TYPE \
         -DENABLE_HOST_REDIRECT:-DENABLE_IPV4:-DENABLE_IPV6 \
         -DENABLE_HOST_REDIRECT:-DENABLE_IPV4:-DENABLE_IPV6:-DENABLE_NAT46

tgraf · 2019-06-06T10:41:21Z

You can then test-compile your code by running make in bpf/

tgraf · 2019-06-06T22:29:27Z

test-me-please

fristonio · 2019-06-07T04:16:40Z

Will take a look at this as soon. as I get access to my laptop.

vadorovsky · 2019-06-07T13:47:50Z

test-me-please

fristonio · 2019-06-08T16:32:43Z

@tgraf I could not figure out the source of error from the CI logs. It's showing Cancelling nested steps due to timeout and error is in K8s 1.10 net-next vm provisioning fail. I am not sure if this is due to my PR or something else. Can you take a look?

ianvernon · 2019-06-08T23:25:42Z

@fristonio we've had some issues with timeouts affecting our CI - these are actively being worked on. I'll run the build again and see if the issue you observed reproduces. thanks for your patience!

ianvernon · 2019-06-08T23:25:47Z

test-me-please

fristonio · 2019-06-09T06:05:06Z

Thanks a lot @ianvernon
The build still errored and was again due to a timeout.

06:08:02      Test NAT46 connectivity between containers [It]
06:08:02      /home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/runtime-gopath/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:410
06:08:02  
06:08:02      Endpoints not ready after timeout
[2019-06-09T00:38:02.852Z]     Expected
[2019-06-09T00:38:02.852Z]         <bool>: false
[2019-06-09T00:38:02.852Z]     to be true

But this was while checking NAT46 connectivity, I am not sure if this is due to code changes or a timeout problem like before.
I will try to debug the issue locally once and see if I can find something.

tgraf · 2019-06-10T11:22:05Z

The program is now over the instruction limit:

Jun 09 00:33:49 runtime cilium-agent[15492]: level=error msg="Command execution failed" cmd="[tc filter replace dev lxc9e98214713e4 ingress prio 1 handle 1 bpf da obj 2074_next/bpf_lxc.o sec from-container]" error="exit status 1" subsys=datapath-loader
Jun 09 00:33:49 runtime cilium-agent[15492]: level=warning subsys=datapath-loader
Jun 09 00:33:49 runtime cilium-agent[15492]: level=warning msg="Prog section '2/10' rejected: Invalid argument (22)!" subsys=datapath-loader
Jun 09 00:33:49 runtime cilium-agent[15492]: level=warning msg=" - Type:         3" subsys=datapath-loader
Jun 09 00:33:49 runtime cilium-agent[15492]: level=warning msg=" - Attach Type:  0" subsys=datapath-loader
Jun 09 00:33:49 runtime cilium-agent[15492]: level=warning msg=" - Instructions: 4107 (11 over limit)" subsys=datapath-loader
Jun 09 00:33:49 runtime cilium-agent[15492]: level=warning msg=" - License:      GPL" subsys=datapath-loader
Jun 09 00:33:49 runtime cilium-agent[15492]: level=warning subsys=datapath-loader
Jun 09 00:33:49 runt

I have a local change to split LB into a separate section to relax this. I'll push it and you can rebase on top of that.

fristonio · 2019-06-10T13:42:43Z

By instruction limits you mean the max instruction limits(4096) on a BPF program right?
Also, how do we split LB into separate sections, is this splitting done using tail calls?
I would like to explore a bit more regarding this, can you point me to the code sections for this?

tgraf · 2019-06-11T20:20:19Z

By instruction limits you mean the max instruction limits(4096) on a BPF program right?

Yes

Also, how do we split LB into separate sections, is this splitting done using tail calls?

Exactly

I would like to explore a bit more regarding this, can you point me to the code sections for this?

Sure, I've pushed the branch here:
https://github.com/cilium/cilium/compare/pr/tgraf/split-lxc-prog

borkmann · 2019-06-12T14:30:33Z

test-me-please

brb · 2019-06-13T09:08:48Z

CI failed due to the same issue with instr count:

Jun 12 15:45:35 runtime cilium-agent[17494]: level=warning msg="Prog section '2/10' rejected: Invalid argument (22)!" subsys=datapath-loader
Jun 12 15:45:35 runtime cilium-agent[17494]: level=warning msg=" - Type:         3" subsys=datapath-loader
Jun 12 15:45:35 runtime cilium-agent[17494]: level=warning msg=" - Attach Type:  0" subsys=datapath-loader
Jun 12 15:45:35 runtime cilium-agent[17494]: level=warning msg=" - Instructions: 4107 (11 over limit)" subsys=datapath-loader
Jun 12 15:45:35 runtime cilium-agent[17494]: level=warning msg=" - License:      GPL" subsys=datapath-loader

joestringer · 2019-06-13T18:41:57Z

@brb yeah, the LXC prog splitout will need to be done first. @fristonio , will you look into that? Happy to provide pointers/help if you have any questions.

fristonio · 2019-06-13T18:47:00Z

Yeah sure, I would love to. I am not very experienced with BPF but will surely try to help out.

aanm · 2019-06-14T08:42:29Z

@fristonio needs rebase against master.

vadorovsky · 2019-06-17T17:50:48Z

test-me-please

aanm · 2019-06-19T15:49:52Z

@fristonio needs a new rebase. It seems the tests failed last time. Can you rebase so we can trigger the CI again?

brb · 2019-07-02T09:32:20Z

@fristonio Let us know if we should do the rebase.

fristonio · 2019-07-02T13:41:01Z

Sorry, I was a bit busy the past week.
Also, I think since this change is not in the master yet the tests will fail again.

tgraf · 2019-07-09T11:13:19Z

test-me-please

stale · 2019-08-08T11:51:46Z

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.

pkg/option/config.go

tgraf · 2019-08-26T19:54:54Z

@fristonio This looks ready to merge, just needs a rebase.

fristonio · 2019-08-26T21:49:37Z

@tgraf I have rebased the pull request, but I am not sure if this will pass the ginkgo tests as I think that the split-lxc-prog branch changes are still not in master.

tgraf · 2019-08-27T08:55:27Z

test-me-please

* Add flag allow-icmp-frag-needed for cilium-agent to allow ICMP type3 code 4 packets in policy. * This will allow TCP Path MTU discovery not to fail. Signed-off-by: Deepesh Pathak <deepshpathak@gmail.com>

fristonio requested review from a team June 5, 2019 12:50

fristonio requested a review from a team as a code owner June 5, 2019 12:50

houndci-bot reviewed Jun 5, 2019

View reviewed changes

pkg/option/config.go Outdated Show resolved Hide resolved

fristonio force-pushed the allow-icmp-frag-needed branch from e77ed56 to e67135d Compare June 5, 2019 12:53

brb requested changes Jun 5, 2019

View reviewed changes

daemon/daemon_main.go Outdated Show resolved Hide resolved

bpf/lib/policy.h Outdated Show resolved Hide resolved

fristonio force-pushed the allow-icmp-frag-needed branch 2 times, most recently from d0faf34 to cdb39aa Compare June 5, 2019 15:15

tgraf added the pending-review label Jun 5, 2019

tgraf added this to the 1.6.0 milestone Jun 5, 2019

brb added sig/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 Jun 5, 2019

tgraf added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jun 6, 2019

fristonio force-pushed the allow-icmp-frag-needed branch from cdb39aa to af8ff96 Compare June 6, 2019 19:40

fristonio requested a review from a team as a code owner June 6, 2019 19:40

fristonio force-pushed the allow-icmp-frag-needed branch from af8ff96 to 0c5e193 Compare June 8, 2019 16:36

joestringer approved these changes Jun 11, 2019

View reviewed changes

aanm added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 14, 2019

fristonio force-pushed the allow-icmp-frag-needed branch from 2994a7d to d6cb298 Compare June 14, 2019 16:47

vadorovsky approved these changes Jun 17, 2019

View reviewed changes

fristonio force-pushed the allow-icmp-frag-needed branch from d6cb298 to 43b0a76 Compare July 2, 2019 13:38

stale bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Aug 8, 2019

fristonio force-pushed the allow-icmp-frag-needed branch from 43b0a76 to 64aacc6 Compare August 13, 2019 13:59

stale bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Aug 13, 2019

houndci-bot reviewed Aug 13, 2019

View reviewed changes

pkg/option/config.go Outdated Show resolved Hide resolved

fristonio force-pushed the allow-icmp-frag-needed branch from 64aacc6 to de8f360 Compare August 13, 2019 14:01

fristonio force-pushed the allow-icmp-frag-needed branch from de8f360 to 4496cdf Compare August 26, 2019 21:47

tgraf removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Aug 27, 2019

pmtu: add flag allow-icmp-frag-needed to allow ICMP frag needed packets

28e9724

* Add flag allow-icmp-frag-needed for cilium-agent to allow ICMP type3 code 4 packets in policy. * This will allow TCP Path MTU discovery not to fail. Signed-off-by: Deepesh Pathak <deepshpathak@gmail.com>

fristonio force-pushed the allow-icmp-frag-needed branch from 4496cdf to 28e9724 Compare August 27, 2019 11:02

tgraf merged commit 7435ff3 into cilium:master Aug 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow icmp fragmentation needed agent option #8218

Allow icmp fragmentation needed agent option #8218

fristonio commented Jun 5, 2019 •

edited by tgraf

Loading

coveralls commented Jun 5, 2019 •

edited

Loading

brb left a comment

tgraf commented Jun 6, 2019

tgraf commented Jun 6, 2019

tgraf commented Jun 6, 2019 •

edited

Loading

tgraf commented Jun 6, 2019

tgraf commented Jun 6, 2019

fristonio commented Jun 7, 2019

vadorovsky commented Jun 7, 2019

fristonio commented Jun 8, 2019

ianvernon commented Jun 8, 2019

ianvernon commented Jun 8, 2019

fristonio commented Jun 9, 2019

tgraf commented Jun 10, 2019

fristonio commented Jun 10, 2019

tgraf commented Jun 11, 2019

borkmann commented Jun 12, 2019

brb commented Jun 13, 2019

joestringer commented Jun 13, 2019

fristonio commented Jun 13, 2019

aanm commented Jun 14, 2019

vadorovsky commented Jun 17, 2019

aanm commented Jun 19, 2019

brb commented Jul 2, 2019

fristonio commented Jul 2, 2019

tgraf commented Jul 9, 2019

stale bot commented Aug 8, 2019

tgraf commented Aug 26, 2019

fristonio commented Aug 26, 2019

tgraf commented Aug 27, 2019

Allow icmp fragmentation needed agent option #8218

Allow icmp fragmentation needed agent option #8218

Conversation

fristonio commented Jun 5, 2019 • edited by tgraf Loading

coveralls commented Jun 5, 2019 • edited Loading

brb left a comment

Choose a reason for hiding this comment

tgraf commented Jun 6, 2019

tgraf commented Jun 6, 2019

tgraf commented Jun 6, 2019 • edited Loading

tgraf commented Jun 6, 2019

tgraf commented Jun 6, 2019

fristonio commented Jun 7, 2019

vadorovsky commented Jun 7, 2019

fristonio commented Jun 8, 2019

ianvernon commented Jun 8, 2019

ianvernon commented Jun 8, 2019

fristonio commented Jun 9, 2019

tgraf commented Jun 10, 2019

fristonio commented Jun 10, 2019

tgraf commented Jun 11, 2019

borkmann commented Jun 12, 2019

brb commented Jun 13, 2019

joestringer commented Jun 13, 2019

fristonio commented Jun 13, 2019

aanm commented Jun 14, 2019

vadorovsky commented Jun 17, 2019

aanm commented Jun 19, 2019

brb commented Jul 2, 2019

fristonio commented Jul 2, 2019

tgraf commented Jul 9, 2019

stale bot commented Aug 8, 2019

tgraf commented Aug 26, 2019

fristonio commented Aug 26, 2019

tgraf commented Aug 27, 2019

fristonio commented Jun 5, 2019 •

edited by tgraf

Loading

coveralls commented Jun 5, 2019 •

edited

Loading

tgraf commented Jun 6, 2019 •

edited

Loading