Pass vlan tagged packets back to the kernel stack. #16772

kvaster · 2021-07-05T06:23:24Z

VLAN packets will be catched with bpf on main interface first. We
need to passs it back to the kernel stack. VLAN info/tag will be stripped and
packet will be reenqueued on proper interface or dropped.

Also we do not need to process VLAN packets on egress, cause either
we've already processed such packets in case bpf program is attached
to VLAN interface or we do not need to process such packets at all in
other way.

VLAN tagged packets will be processed in the folowing way:

If vlan tag is zero packet will be processed as usual (cause it is targeted for main interface)
For each device will be allowed only corresponding vlan tags.
By default will be allowed only tags from devices controlled by cilium.
Any other tags may be allowed via option (--vlan-bpf-bypass), --vlan-bpf-bypass 0 may be used for allowing all available vlan tags.
All other vlan tags will be always dropped for security reason.

Concerns:

Probably ifindexes and vlan tags should be sorted to make configuration predictable.
Probably something like * should be used insteadof 0 for allowing all available vlan tags.

Previous pull request and discussion: #15534

Fixes: #14579

Signed-off-by: Viktor Kuzmin kvaster@gmail.com

Make NodePort BPF to work on VLAN devices

pkg/datapath/linux/config/config.go

twpayne · 2021-07-05T13:22:49Z

Please can you add a release note to the PR description, as documented in step 11 here.

kvaster · 2021-07-06T12:16:38Z

fixed!

twpayne · 2021-07-08T14:48:53Z

test-me-please

kvaster · 2021-07-08T17:58:17Z

I've updated PR to fix some CI problems - cmdref docs.

kvaster · 2021-07-12T07:31:22Z

Any updates ?

twpayne

Looks good from a CLI perspective, but I need the experienced datapath folk to review the datapath aspects.

joestringer

This seems much closer to striking the right balance between providing the configurability you're looking for without surprising users too much with traffic bypassing Cilium logic.

I've requested a few changes below to make the change easier to reason about, both from a user perspective (semantics/naming and traceability) but also from a developer perspective (make it super simple to follow the flow of logic, test it so we can be sure that different variations of the option produce the desired behaviour).

The only thing I didn't raise below is the complexity concern. I strongly suspect that if you configure too many VLANs in this option, bpf program load will fail and the Cilium container will crash out because it cannot configure the datapath in the way you have requested. This could happen either when you first enable the option (because you specified too many tags), or it could plausibly happen later on when you upgrade to a new release, because each release can change the datapath. When you upgrade, there could be just enough additional BPF instructions introduced by this logic that when combined with the rest of the new datapath logic, it pushes the program complexity over some limit. I don't know where that limit is, and this PR is not proposing any testing for it. For now if this is a super-advanced option, maybe you're willing to take on that risk and deal with the consequences / roll back & report issues upstream when you hit them during upgrade. But we may want to think a bit more about integration testing and how we can detect such problems earlier in the development cycle so we can start the discussions early on how to address them.

pkg/datapath/linux/config/config.go

daemon/cmd/daemon_main.go

pkg/option/config.go

bpf/bpf_host.c

pkg/datapath/linux/config/config.go

kvaster · 2021-07-12T18:30:55Z

From my point of view it's rather safe to just bypass any non-zero vlan tag to kernel and allow it to do it's job, bit cilium policy is to control everything on devices controlled by cilium. In 99% situations there will be no or 1-2 vlan tags for 1-2 devices. But maybe it's a good idea to generate just return CTX_ACT_OK if 'all' vlans are requested.

bpf/bpf_host.c

kvaster · 2021-07-12T18:44:33Z

Thanks for comments. Will get back tomorrow with changes.

maintainer-s-little-helper · 2021-07-16T07:44:44Z

Commit 3e74eb655d512e97d6a87478787341ba0e7126ca does not contain "Signed-off-by".

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

kvaster · 2021-07-16T08:36:34Z

I've pressed 'update branch' by an accident... Sorry...

kvaster · 2021-07-17T07:32:34Z

What are next steps ? Is there anything more I can do to help this finalize ?

brb · 2021-07-20T08:44:32Z

Thanks. Marking as ready to merge.

@kvaster As a follow up, could you extend the kube-proxy-replacement guide by documenting your change?

errordeveloper · 2021-07-20T08:47:27Z

Unasigning myself as this PR has 3 reviews already.

brb · 2021-07-20T09:05:14Z

CC'ing @pchaigno and @jrfastab for visibility. This PR should fix host-fw and IPSec on vlan devices.

nathanjsweet

From a hubble and api perspective. LGTM. The datapath stuff makes sense to me too.

joestringer · 2021-07-21T00:26:59Z

The bot has marked not ready-to-merge because there are still @cilium/helm and @cilium/hubble reviews outstanding. @errordeveloper , it looks like you were pinged for those aspects (including the hubble API changes). Could you take a brief look specifically at those areas or consider raising this in the sig-hubble so that someone with the relevant expertise can take a look?

kvaster · 2021-07-21T07:00:05Z

Thanks. Marking as ready to merge.

@kvaster As a follow up, could you extend the kube-proxy-replacement guide by documenting your change?

I will do this as a follow up PR!

errordeveloper

Protobuf and helm changes LGTM!

kvaster · 2021-07-23T07:24:27Z

It seems that all reviews are passed...

jrajahalme · 2021-08-09T16:55:09Z

pkg/datapath/linux/config/config.go

+		// if it's vlan device and we're controlling vlan main device
+		// and either all vlans are allowed, or we're controlling vlan device or vlan is explicitly allowed
+		if ok && devices[vlan.ParentIndex] && (devices[vlan.Index] || allowedVlans[vlan.VlanId]) {
+			vlansByIfIndex[vlan.ParentIndex] = append(vlansByIfIndex[vlan.ParentIndex], vlan.VlanId)


This appending of vlanIDs that may not be ordered introduced a new unit test flake #17104.

@kvaster would you be able to look into this? Usually the solution is just to sort the slice once it's been assembled but before it is used to generate code.

Never mind, looks like @jrajahalme beat you to it :) Feel free to review though: #17105

Was away... Seems problem was already solved... I will make one more PR soon as discussed - docs update.

jhutchins · 2021-09-03T17:45:30Z

Is there an upcoming 1.10 release that will include this fix in the near future?

joestringer · 2021-09-08T19:10:05Z

Hi @jhutchins , typically more invasive features / changes are scheduled for the next upcoming release in order to minimize the risk of breakage for existing users. This will be included in the upcoming v1.11 release.

Follow-up for PR: cilium#16772 Signed-off-by: Viktor Kuzmin <kvaster@gmail.com>

Follow-up for PR: #16772 Signed-off-by: Viktor Kuzmin <kvaster@gmail.com>

kvaster requested review from a team July 5, 2021 06:23

kvaster requested review from a team as code owners July 5, 2021 06:23

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 5, 2021

kvaster requested review from borkmann and twpayne July 5, 2021 06:23

maintainer-s-little-helper bot assigned borkmann and twpayne Jul 5, 2021

twpayne reviewed Jul 5, 2021

View reviewed changes

pkg/datapath/linux/config/config.go Outdated Show resolved Hide resolved

twpayne added dont-merge/needs-release-note release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jul 5, 2021

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 5, 2021

kvaster requested a review from twpayne July 7, 2021 11:14

kkourt removed the dont-merge/needs-release-note label Jul 12, 2021

brb requested a review from joestringer July 12, 2021 08:31

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

twpayne approved these changes Jul 12, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned twpayne Jul 12, 2021

joestringer requested changes Jul 12, 2021

View reviewed changes

joestringer reviewed Jul 12, 2021

View reviewed changes

pkg/datapath/linux/config/config.go Outdated Show resolved Hide resolved

joestringer reviewed Jul 12, 2021

View reviewed changes

bpf/bpf_host.c Outdated Show resolved Hide resolved

kvaster requested a review from a team as a code owner July 13, 2021 17:47

maintainer-s-little-helper bot unassigned brb Jul 16, 2021

maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. and removed dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. labels Jul 16, 2021

brb added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 20, 2021

errordeveloper removed their assignment Jul 20, 2021

errordeveloper removed their request for review July 20, 2021 08:46

nathanjsweet approved these changes Jul 20, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned nathanjsweet Jul 20, 2021

maintainer-s-little-helper bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 20, 2021

errordeveloper approved these changes Jul 21, 2021

View reviewed changes

joestringer merged commit 9086640 into cilium:master Jul 23, 2021

This was referenced Aug 9, 2021

CI: RuntimePrivilegedUnitTests Run Tests #17104

Closed

policy: Adjust existing policy for visibility annotations #16258

Merged

jrajahalme reviewed Aug 9, 2021

View reviewed changes

kvaster mentioned this pull request Oct 5, 2021

Add documentation for vlan bpf bypass. #17539

Merged

kvaster added a commit to kvaster/cilium that referenced this pull request Oct 7, 2021

Add documentation for vlan bpf bypass.

181b203

Follow-up for PR: cilium#16772 Signed-off-by: Viktor Kuzmin <kvaster@gmail.com>

aanm pushed a commit that referenced this pull request Oct 13, 2021

Add documentation for vlan bpf bypass.

05a3daf

Follow-up for PR: #16772 Signed-off-by: Viktor Kuzmin <kvaster@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass vlan tagged packets back to the kernel stack. #16772

Pass vlan tagged packets back to the kernel stack. #16772

kvaster commented Jul 5, 2021 •

edited

twpayne commented Jul 5, 2021

kvaster commented Jul 6, 2021

twpayne commented Jul 8, 2021

kvaster commented Jul 8, 2021 •

edited

kvaster commented Jul 12, 2021

twpayne left a comment

joestringer left a comment

kvaster commented Jul 12, 2021 •

edited

kvaster commented Jul 12, 2021

maintainer-s-little-helper bot commented Jul 16, 2021

kvaster commented Jul 16, 2021

kvaster commented Jul 17, 2021

brb commented Jul 20, 2021

errordeveloper commented Jul 20, 2021

brb commented Jul 20, 2021

nathanjsweet left a comment

joestringer commented Jul 21, 2021

kvaster commented Jul 21, 2021

errordeveloper left a comment

kvaster commented Jul 23, 2021

jrajahalme Aug 9, 2021

joestringer Aug 9, 2021

joestringer Aug 9, 2021

kvaster Aug 10, 2021

jhutchins commented Sep 3, 2021

joestringer commented Sep 8, 2021

Pass vlan tagged packets back to the kernel stack. #16772

Pass vlan tagged packets back to the kernel stack. #16772

Conversation

kvaster commented Jul 5, 2021 • edited

twpayne commented Jul 5, 2021

kvaster commented Jul 6, 2021

twpayne commented Jul 8, 2021

kvaster commented Jul 8, 2021 • edited

kvaster commented Jul 12, 2021

twpayne left a comment

Choose a reason for hiding this comment

joestringer left a comment

Choose a reason for hiding this comment

kvaster commented Jul 12, 2021 • edited

kvaster commented Jul 12, 2021

maintainer-s-little-helper bot commented Jul 16, 2021

kvaster commented Jul 16, 2021

kvaster commented Jul 17, 2021

brb commented Jul 20, 2021

errordeveloper commented Jul 20, 2021

brb commented Jul 20, 2021

nathanjsweet left a comment

Choose a reason for hiding this comment

joestringer commented Jul 21, 2021

kvaster commented Jul 21, 2021

errordeveloper left a comment

Choose a reason for hiding this comment

kvaster commented Jul 23, 2021

jrajahalme Aug 9, 2021

Choose a reason for hiding this comment

joestringer Aug 9, 2021

Choose a reason for hiding this comment

joestringer Aug 9, 2021

Choose a reason for hiding this comment

kvaster Aug 10, 2021

Choose a reason for hiding this comment

jhutchins commented Sep 3, 2021

joestringer commented Sep 8, 2021

kvaster commented Jul 5, 2021 •

edited

kvaster commented Jul 8, 2021 •

edited

kvaster commented Jul 12, 2021 •

edited