-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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.
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
Add lbipam support for shared ips #28806
Conversation
Commit da87298 does not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
Hi @usiegl00, thank you for working on this. However, this work does not check for the conditions mentioned in #21776 which we need in order to have feature parity with MetalLB. My suggestion is to change the allocator in the The algorithm would go something like:
That way we nicely handle all scenarios. I know its a lot more work, but such are the requirements of the feature. These behaviors also need to be tested. |
Commits da87298, 3428661 do not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
Hi @dylandreimerink, thank you for the detailed code layout. I have begun to implement your algorithm. |
Commits da87298, 3428661, a7889b0 do not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
Commits da87298, 3428661, a7889b0, 152836e do not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
152836e
to
251d564
Compare
Commits ed1d8a6, 5cbbcbe, df15eb7, 251d564 do not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
426b161
to
2ce4954
Compare
2ce4954
to
070f794
Compare
That is a good point, I think it should. In dual stack mode we expect a service to get an IPv4 and IPv6 address which will both have to be in different ranges. This also made be think about how to handle requests for specific IPs when dealing with shared IPs 🤔. Currently if two service request the same IP we will error on the second request. But if they have a sharing key set and are compatible then we might want to allow that. That would also mean that we could have multiple IPv4 or IPv6 IPs that are shared under a given sharing key which perhaps complicates things.
We typically like doc changes to be in the same PR if possible. |
Yeah, my current strategy is to try and slot a service into the existing ips belonging to the sharing key and adding a new ip to the existing if it cannot. This is a simple way of ensuring optimal density when ip space is limited. I also was thinking about enabling the use of multiple comma-separated sharing keys. This would enable services with both sharing keys to slot into both sharing key svcip groups, whereas a service with just one of the keys would only have to slot in with the rest that share that key. This would enable a service with two or more ips to share them in a predictable manner with other services that have conflicting ports. |
070f794
to
3a51df8
Compare
3a51df8
to
9090715
Compare
10dc0bb
to
6cc6df8
Compare
@dylandreimerink Dylan, I've finished work on the pr it is passing the test I added and I hope to get it reviewed and ready to be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for the work you have put into this PR! Overall really nice, though I do think I spotted some things that need to be addressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@usiegl00 thanks again for making the adjustments. I have been playing these changes a bit to test things out and found some bugs. While debugging I concluded that the code had become hard to read due to the functions becoming to big so I took it upon myself to do a bit of restructuring. I figured that would be easier than to communicate all of my nit picks. I forked your branch and added 3 commits atop with tests for the bug, the bugfix and my structural changes https://github.com/dylandreimerink/cilium/tree/feature/lbipam-shared-ips
I also noticed that the history of this PR is getting a bit messy, we are not at 9 commits, not including these fixes. My suggestion is that you merge these commits into you own branch, then squash all code related commits into one commit (including my additions) and let the docs commit be a separate commit.
0b412fa
to
b10be3c
Compare
@dylandreimerink Thanks for the assist! I've squashed everything down to 2 commits. (••) ( ••)>⌐■-■ (⌐■_■) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The LB-IPAM logic looks good to me! 🎉
2db4b18
to
8b7d749
Compare
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs alright
Commit 68b15ff does not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
68b15ff
to
d7f9325
Compare
@dylandreimerink Would you trigger a test again? I just had to rebase on main to pull in some auxiliary fixes. Thanks! |
/test |
@dylandreimerink This is ready to be merged, the only failing test also failed on main. |
Updated the service compatibility check. Made the namespace check non conditional since its a security check. Rewrote some of the logic to be more consice. And added comments and rational to certain checks to remind us of why we do them. After the addition of the shared IP logic, some functions became to large. This commit splits the code into smaller functions so the overall flow is easier to follow. When 2 services have a sharing key and the second service would update so it is now incompatible with the other service it would keep the same IP address due to a lack of compatibility checking during updates and a logic bug in the ingress IP import code. Add a test for sharing keys. Add logic for stacking multiple ips for one sharing key. Add lbipam support for sharing keys. Co-authored-by: Dylan Reimerink <dylan.reimerink@isovalent.com> Signed-off-by: usiegl00 <50933431+usiegl00@users.noreply.github.com>
Signed-off-by: usiegl00 <50933431+usiegl00@users.noreply.github.com>
d7f9325
to
8a07eef
Compare
/test |
Follow-up of cilium#28806 to allow sharing an IP across namespaces. The previous changes to allow sharing an IP between several services forbid to do so across namespaces because a service is a namespaced resource and doing so could cause security issues. However, for non-tenanted clusters needing to really reduce IP usage, it still makes sense to allow users to share IPs across namespaces. This change thus re-introduces that ability by having the user specify an annotation on both services, thus granting sharing. Signed-off-by: Marc 'risson' Schmitt <marc.schmitt@risson.space>
Follow-up of cilium#28806 to allow sharing an IP across namespaces. The previous changes to allow sharing an IP between several services forbid to do so across namespaces because a service is a namespaced resource and doing so could cause security issues. However, for non-tenanted clusters needing to really reduce IP usage, it still makes sense to allow users to share IPs across namespaces. This change thus re-introduces that ability by having the user specify an annotation on both services, thus granting sharing. Signed-off-by: Marc 'risson' Schmitt <marc.schmitt@risson.space>
Follow-up of cilium#28806 to allow sharing an IP across namespaces. The previous changes to allow sharing an IP between several services forbid to do so across namespaces because a service is a namespaced resource and doing so could cause security issues. However, for non-tenanted clusters needing to really reduce IP usage, it still makes sense to allow users to share IPs across namespaces. This change thus re-introduces that ability by having the user specify an annotation on both services, thus granting sharing. Signed-off-by: Marc 'risson' Schmitt <marc.schmitt@risson.space>
Follow-up of cilium#28806 to allow sharing an IP across namespaces. The previous changes to allow sharing an IP between several services forbid to do so across namespaces because a service is a namespaced resource and doing so could cause security issues. However, for non-tenanted clusters needing to really reduce IP usage, it still makes sense to allow users to share IPs across namespaces. This change thus re-introduces that ability by having the user specify an annotation on both services, thus granting sharing. Signed-off-by: Marc 'risson' Schmitt <marc.schmitt@risson.space>
Follow-up of cilium#28806 to allow sharing an IP across namespaces. The previous changes to allow sharing an IP between several services forbid to do so across namespaces because a service is a namespaced resource and doing so could cause security issues. However, for non-tenanted clusters needing to really reduce IP usage, it still makes sense to allow users to share IPs across namespaces. This change thus re-introduces that ability by having the user specify an annotation on both services, thus granting sharing. Signed-off-by: Marc 'risson' Schmitt <marc.schmitt@risson.space>
Follow-up of #28806 to allow sharing an IP across namespaces. The previous changes to allow sharing an IP between several services forbid to do so across namespaces because a service is a namespaced resource and doing so could cause security issues. However, for non-tenanted clusters needing to really reduce IP usage, it still makes sense to allow users to share IPs across namespaces. This change thus re-introduces that ability by having the user specify an annotation on both services, thus granting sharing. Signed-off-by: Marc 'risson' Schmitt <marc.schmitt@risson.space>
This disables the checks that prevent the sharing of ips between services.
Fixes: #21776