Skip to content

datapath,daemon: Add check for loadBalancerSourceRanges #12841

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
Merged
merged 10 commits into from
Aug 20, 2020
Merged

datapath,daemon: Add check for loadBalancerSourceRanges #12841

merged 10 commits into from
Aug 20, 2020

Conversation

brb
Copy link
Member

@brb brb commented Aug 11, 2020 •
edited
Loading

This PR extends the kube-proxy replacement by adding a support for LoadBalancer service source range checks which:

If specified and supported by the platform, this will restrict traffic through the cloud-provider load-balancer will be restricted to the specified client IPs. This field will be ignored if the cloud-provider does not support the feature." More info: https://kubernetes.io/docs/tasks/access-application-cluster/configure-cloud-provider-firewall/

(https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#servicespec-v1-core)

TODO for an immediate follow-up:

  • Update the kube-proxy replacement guide wrt the feature.
  • Update the docs wrt to the src range map size.
  • Introduce cilium bpf source-range list, add it to bugtool and replace time.Sleep() in the extended integration test with it.

Reviewable per commit.

Add a check for loadBalancerSourceRanges to the kube-proxy replacement

Reported-by: John Watson johnw@planetscale.com (@dctrwatson)

@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 Aug 11, 2020
@brb brb added the release-note/major This PR introduces major new functionality to Cilium. label Aug 11, 2020
@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 Aug 11, 2020
@brb brb force-pushed the pr/brb/lb-src-addr-v2 branch 3 times, most recently from 8f798c1 to 54f6bce Compare August 11, 2020 13:33
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 37.084% when pulling 8f798c1d9b6fea606b97d0ed0f11fc0712c7735b on pr/brb/lb-src-addr-v2 into b920dfd on master.

@coveralls
Copy link

coveralls commented Aug 11, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.03%) to 37.094% when pulling 812a8ce978f7293167387156149a19fdd189df0a on pr/brb/lb-src-addr-v2 into 85600be on master.

@brb brb force-pushed the pr/brb/lb-src-addr-v2 branch 4 times, most recently from ba18582 to b20152f Compare August 11, 2020 15:56
@brb
Copy link
Member Author

brb commented Aug 11, 2020

test-me-please

@brb brb force-pushed the pr/brb/lb-src-addr-v2 branch from b20152f to 5ad5a2f Compare August 13, 2020 15:20
@brb brb added area/daemon Impacts operation of the Cilium daemon. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Aug 13, 2020
@brb brb requested a review from borkmann August 13, 2020 15:27
@brb
Copy link
Member Author

brb commented Aug 13, 2020

test-me-please

@brb brb marked this pull request as ready for review August 13, 2020 15:30
@brb brb requested a review from a team August 13, 2020 15:30
@brb brb requested a review from a team as a code owner August 13, 2020 15:30
@brb brb requested a review from a team August 13, 2020 15:30
@brb brb requested review from a team as code owners August 13, 2020 15:30
@brb brb requested review from a team August 13, 2020 15:30
@brb brb force-pushed the pr/brb/lb-src-addr-v2 branch from 5ad5a2f to d40abdf Compare August 13, 2020 15:42
@brb
Copy link
Member Author

brb commented Aug 13, 2020

test-me-please

@joestringer joestringer added the dont-merge/blocked Another PR must be merged before this one. label Aug 13, 2020
@brb
test: Add integration test for LB src range check
29dae25 
Currently, the test contains some sleeps for waiting until cilium-agents
have updated their LB src range maps. In the future, it will be replaced
by checking whether "cilium bpf source-list list" (TODO) contains
relevant entries.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb force-pushed the pr/brb/lb-src-addr-v2 branch from 9592f30 to 29dae25 Compare August 20, 2020 09:45
@brb
Copy link
Member Author

brb commented Aug 20, 2020

test-me-please

@borkmann borkmann merged commit c142e5e into master Aug 20, 2020
@borkmann borkmann deleted the pr/brb/lb-src-addr-v2 branch August 20, 2020 12:47
@gandro gandro mentioned this pull request Aug 20, 2020
Merged
@borkmann
Copy link
Member

Wrt Introduce cilium bpf source-range list, add it to bugtool and replace time.Sleep() in the extended integration test with it. couldn't we just extend the cilium service list output and extend the dump to show src CIDR restrictions for the affected services?

@brb brb mentioned this pull request Aug 25, 2020
Merged
@brb brb mentioned this pull request Sep 11, 2020
Merged
@brb brb mentioned this pull request Sep 28, 2020
Closed
@brb
Copy link
Member Author

brb commented Sep 28, 2020

Wrt Introduce cilium bpf source-range list, add it to bugtool and replace time.Sleep() in the extended integration test with it. couldn't we just extend the cilium service list output and extend the dump to show src CIDR restrictions for the affected services?

Thanks, created #13303 to track this.

@tklauser tklauser mentioned this pull request Nov 3, 2020
Merged
julianwiedmann added a commit to julianwiedmann/cilium that referenced this pull request Oct 7, 2022
@julianwiedmann
test: bgp: wait for LB SourceRange entry to show up
f407168 
As suggested in cilium#12841, replace the
hard-coded time.Sleep() with a peek into the SourceRanges map.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@borkmann borkmann borkmann approved these changes

@joestringer joestringer joestringer approved these changes

@aanm aanm Awaiting requested review from aanm

Assignees
No one assigned
Labels
area/daemon Impacts operation of the Cilium daemon. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/major This PR introduces major new functionality to Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@brb @coveralls @borkmann @joestringer @aanm @gandro