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

bpf: added --bpf-lb-bypass-fib-lookup flag #14978

Merged
merged 2 commits into from
Feb 18, 2021
Merged

bpf: added --bpf-lb-bypass-fib-lookup flag #14978

merged 2 commits into from
Feb 18, 2021

Conversation

skuffe
Copy link
Contributor

@skuffe skuffe commented Feb 15, 2021 •
edited

This commit adds a boolean flag, which when enabled will cause the BPF nodeport reverse NAT handling to attempt to use a MAC address which was previously associated with the remote endpoint when sending its reply, instead of doing a FIB lookup.

If the flag is toggled off, this optimization is disabled, and a FIB lookup is always done.

This is useful if Cilium is to be used on nodes which default gateway is a FHRP VIP/MAC, such as with VRRP or Cisco's HSRP.

Fixes: #14911

Signed-off-by: Danni Skov Høglund skuffe@pwnz.dk

Added --bpf-lb-bypass-fib-lookup flag, which toggles the BPF nodeport reverse NAT FIB lookup optimization

@maintainer-s-little-helper 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 Feb 15, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Feb 15, 2021
@skuffe skuffe marked this pull request as ready for review February 15, 2021 11:06
@skuffe skuffe requested review from a team February 15, 2021 11:06
@skuffe skuffe requested a review from a team as a code owner February 15, 2021 11:06
@twpayne twpayne added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Feb 15, 2021
@maintainer-s-little-helper 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 Feb 15, 2021
borkmann
borkmann requested changes Feb 15, 2021
View reviewed changes
bpf/lib/nodeport.h Show resolved Hide resolved
pkg/defaults/defaults.go Show resolved Hide resolved
@borkmann borkmann requested a review from brb February 15, 2021 13:43
@brb
Copy link
Member

brb commented Feb 15, 2021

@skuffe Thanks. Could you squash both commits?

@skuffe
bpf: added --bpf-lb-bypass-fib-lookup flag
e571a9a 
This commit adds a boolean flag, which when enabled will cause the BPF
nodeport reverse NAT handling to attempt to use a MAC address which was
previously associated with the remote endpoint when sending its reply,
instead of doing a FIB lookup.

If the flag is toggled off, this optimization is disabled, and a FIB
lookup is always done.

This is useful if Cilium is to be used on nodes which default gateway
is a FHRP VIP/MAC, such as with VRRP or Cisco's HSRP.

Fixes: cilium#14911

Signed-off-by: Danni Skov Høglund <skuffe@pwnz.dk>
@skuffe
Copy link
Contributor Author

skuffe commented Feb 15, 2021

@skuffe Thanks. Could you squash both commits?

done

@brb
Copy link
Member

brb commented Feb 15, 2021

test-me-please

@skuffe skuffe requested review from a team as code owners February 15, 2021 16:53
@skuffe skuffe requested review from a team, joestringer and kkourt February 15, 2021 16:53
@skuffe skuffe requested a review from borkmann February 15, 2021 16:59
@brb
Copy link
Member

brb commented Feb 15, 2021

test-me-please

@ciliumbot
Copy link

Build finished.

3 similar comments
@ciliumbot
Copy link

Build finished.

@ciliumbot
Copy link

Build finished.

@ciliumbot
Copy link

Build finished.

brb
brb approved these changes Feb 16, 2021
View reviewed changes
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.

Thanks!

Have you tested both SNAT and DSR mode?

@brb
Copy link
Member

brb commented Feb 16, 2021

test-me-please

kkourt
kkourt approved these changes Feb 16, 2021
View reviewed changes
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.

Thanks!

@skuffe
Copy link
Contributor Author

skuffe commented Feb 16, 2021 •
edited

Thanks!

Have you tested both SNAT and DSR mode?

Yup,. tested both, as well as with externalTrafficPolicy: Cluster and externalTrafficPolicy: Local set on the service.

Would there be any more settings that could be interesting to test this against?

@brb
Copy link
Member

brb commented Feb 16, 2021

Would there be any more settings that could be interesting to test this against?

No need for more, as SNAT/DSR should be enough.

@brb
Copy link
Member

brb commented Feb 16, 2021

@skuffe Could you close and then reopen the PR? GH actions got stuck, and need to be retriggered.

@borkmann borkmann added the sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. label Feb 16, 2021
@skuffe skuffe closed this Feb 16, 2021
@skuffe skuffe reopened this Feb 16, 2021
@skuffe
docs & helm cm template + values.yaml
c101171 
This commit adds a section to the Getting Started -> Kubernetes Without
kube-proxy, describing what this flag does and when to use it.

Also added the required bits to the Helm template and values.yaml

Signed-off-by: Danni Skov Høglund <skuffe@pwnz.dk>
@aanm
Copy link
Member

aanm commented Feb 16, 2021

test-me-please

@skuffe
Copy link
Contributor Author

skuffe commented Feb 17, 2021

Hmm, that's odd that some of those tests failed. Functionally - in the default configuration, there should be no difference from whats otherwise on the master branch.
Flaky test?

@brb
Copy link
Member

brb commented Feb 18, 2021

test-runtime

@brb
Copy link
Member

brb commented Feb 18, 2021

net-next hit the flake #12511.

@brb
Copy link
Member

brb commented Feb 18, 2021

test-net-next

@brb
Copy link
Member

brb commented Feb 18, 2021

test-gke

@aanm aanm merged commit b3f0387 into cilium:master Feb 18, 2021
1.10.0 automation moved this from In progress to Done Feb 18, 2021
@liuyuan10
Copy link
Contributor

@brb If I disable bpf-lb-bypass-fib-lookup, will there be problem when the l2 entry is not available so that fib_lookup returns BPF_FIB_LKUP_RET_NO_NEIGH?

@brb
Copy link
Member

brb commented Feb 24, 2021

@liuyuan10 Yep, in that case the packet will be dropped. To avoid this, for the relevant actors, we do arping when cilium-agent is configured with either ipsec or kube-proxy replacement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twpayne twpayne twpayne approved these changes

@brb brb brb approved these changes

@kkourt kkourt kkourt approved these changes

@borkmann borkmann borkmann approved these changes

@christarazi christarazi christarazi approved these changes

@joestringer joestringer Awaiting requested review from joestringer

Assignees

@twpayne twpayne

@brb brb

@kkourt kkourt

@borkmann borkmann

@joestringer joestringer

Labels
release-note/minor This PR changes functionality that users may find relevant to operating 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
Development

Successfully merging this pull request may close these issues.

BPF Service implementation - Gateway problem with FHRP
10 participants
@skuffe @brb @ciliumbot @aanm @liuyuan10 @twpayne @kkourt @borkmann @christarazi @joestringer