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
Support defining IPAM pools using CiliumPodIPPool CRD #25824
Conversation
bdc8a49
to
db359a1
Compare
db359a1
to
d2fd3a5
Compare
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.
Awesome work! Left a bit of early feedback
af5a6c1
to
950d4ca
Compare
51f0a51
to
b8050d8
Compare
/test |
/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.
LGTM. A nice to have would be to start modularizing operator's IPAM related code as we're touching these parts.
DeletePool(ctx context.Context, pool *cilium_v2alpha1.CiliumPodIPPool) error | ||
} | ||
|
||
func StartIPPoolAllocator( |
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.
How hard would it be to implement this as a hive module instead? I suppose we'd need to pull out PooledAllocatorProvider
first from the legacy code plus make alloc.Start
depend on IPPoolAllocator
to make sure it's started first?
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.
I initially thought about implementing this as a hive cell. Unfortunately, it would require lifting quite a bit of IPAM code around. For example, I think we cannot make alloc.Start
depend on IPPoolAllocator
unconditionally, since only once specific implementation (namely ipam/allocator/multipool.Allocator
) requires the pooled allocator, while all the others don't. Initially, I wanted to include IPPoolAllocator
into ipam/allocator.AllocatorProvider
, but IIRC that lead to nasty cyclic dependencies. Also, given this PR is already quite large and should make it into the tree for 1.14, I didn't want to include additional refactoring.
Admittedly it's not ideal to introduce new code in a non-modular fashion, but based on the above I'd defer addressing modularization of this code together with the other IPAM bits to a follow-up PR. I'll discuss the specifics with @gandro and @cilium/sig-foundations in a Slack thread or GH issue. Hope that's OK?
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.
Yeah, given the feature freeze coming up in a week, I don't think it's realistic to tackle a larger refactor at the moment unfortunately 😿
The upside is that this is very little and simple code, so it should not make any future refactor more complicated
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.
Awesome work!
Merge button is green! 🚢 |
In (*PoolAllocator).updateCIDRSets the CIDRs are currently parsed twice, once using netip.ParsePrefix and once using net.ParseCIDR. Avoid this and pass the CIDRs as []netip.Prefix and at the same time also convert downstream functions in ipam/cidrset to take a netip.Prefix instead of *net.IPNet. Ref. #25824 (comment) Ref. #25991 (comment) Signed-off-by: Tobias Klauser <tobias@cilium.io>
In (*PoolAllocator).updateCIDRSets the CIDRs are currently parsed twice, once using netip.ParsePrefix and once using net.ParseCIDR. Avoid this and pass the CIDRs as []netip.Prefix and at the same time also convert downstream functions in ipam/cidrset to take a netip.Prefix instead of *net.IPNet. Ref. #25824 (comment) Ref. #25991 (comment) Signed-off-by: Tobias Klauser <tobias@cilium.io>
These were introduced in cilium/cilium#25824 and are used to define pod IP pools in the multi-pool IPAM mode. See cilium/cilium#24764 for more information. Updates cilium/cilium#25470 Signed-off-by: Tobias Klauser <tobias@cilium.io>
The CiliumPodIPPool resource was introduced in cilium/cilium#25824 and is used to define pod IP pools in the multi-pool IPAM mode. See cilium/cilium#24764 for more information. Updates cilium/cilium#25470 Signed-off-by: Tobias Klauser <tobias@cilium.io>
The CiliumPodIPPool resource was introduced in cilium/cilium#25824 and is used to define pod IP pools in the multi-pool IPAM mode. See cilium/cilium#24764 for more information. Updates cilium/cilium#25470 Signed-off-by: Tobias Klauser <tobias@cilium.io>
The CiliumPodIPPool resource was introduced in cilium/cilium#25824 and is used to define pod IP pools in the multi-pool IPAM mode. See cilium/cilium#24764 for more information. Updates cilium/cilium#25470 Signed-off-by: Tobias Klauser <tobias@cilium.io>
The CiliumPodIPPool resource was introduced in cilium/cilium#25824 and is used to define pod IP pools in the multi-pool IPAM mode. See cilium/cilium#24764 for more information. Updates cilium/cilium#25470 Signed-off-by: Tobias Klauser <tobias@cilium.io>
The CiliumPodIPPool resource was introduced in cilium/cilium#25824 and is used to define pod IP pools in the multi-pool IPAM mode. See cilium/cilium#24764 for more information. Updates cilium/cilium#25470 Signed-off-by: Tobias Klauser <tobias@cilium.io>
In (*PoolAllocator).updateCIDRSets the CIDRs are currently parsed twice, once using netip.ParsePrefix and once using net.ParseCIDR. Avoid this and pass the CIDRs as []netip.Prefix and at the same time also convert downstream functions in ipam/cidrset to take a netip.Prefix instead of *net.IPNet. Ref. cilium#25824 (comment) Ref. cilium#25991 (comment) Signed-off-by: Tobias Klauser <tobias@cilium.io>
This PR introduces the
CiliumPodIPPool
CRD and adds support to use it to define IP pools in Cilium's multi-pool IPAM mode (ref. #22762).An example
CiliumPodIPPool
CRD definition of the pool namedjupiter
with the IPv4 CIDRs10.10.0.0/16
and10.20.0.0/16
, an IPv4 mask size of27
bits, the IPv6 CIDRfd00:100::/80
and an IPv6 mask size of96
bits looks as follows:These CRDs are watched by cilium-operator. Deletion and update of existing pools are supported, however updates changing mask sizes on existing pools are rejected.
Please see individual commits for details.
Manually tested on kind as follows:
The following will be added in a separate follow-up PR, see meta issue #25470:
CiliumPodIPPool
s from operatoripam.operator.multiPoolMap
helm flagsExtends #24764