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.

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

IPAM Multi-Pool Follow-ups #25470

Closed
26 of 29 tasks
gandro opened this issue May 16, 2023 · 9 comments
Closed
26 of 29 tasks

IPAM Multi-Pool Follow-ups #25470

gandro opened this issue May 16, 2023 · 9 comments
Assignees
Labels
area/ipam Impacts IP address management functionality. area/multipool Affects Multi-Pool IPAM

Comments

@gandro
Copy link
Member

gandro commented May 16, 2023

This issue tracks the remaining work items around the Multi-Pool IPAM mode:

Best effort/cleanup:

@gandro gandro added release-blocker/1.14 This issue will prevent the release of the next version of Cilium. area/ipam Impacts IP address management functionality. labels May 16, 2023
tklauser added a commit to cilium/cilium-cli that referenced this issue Jun 15, 2023
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>
tklauser added a commit to cilium/cilium-cli that referenced this issue Jun 15, 2023
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>
tklauser added a commit to cilium/cilium-cli that referenced this issue Jun 15, 2023
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>
tklauser added a commit to cilium/cilium-cli that referenced this issue Jun 16, 2023
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>
tklauser added a commit to cilium/cilium-cli that referenced this issue Jun 19, 2023
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>
tklauser added a commit to cilium/cilium-cli that referenced this issue Jun 19, 2023
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>
@gandro
Copy link
Member Author

gandro commented Jun 27, 2023

Removing the release blocker label from this. All necessary parts have been merged. The rest are smaller improvements that can be done in v1.15 as part of the stabilization of the feature

@chestack
Copy link

multiple CIDRs per cluster/node is a highly useful feature, especially for workloads from different tenants. Are there any plans to enable this feature for cloud-related(ENI, Alibaba...) ipam modes?

@gandro
Copy link
Member Author

gandro commented Oct 23, 2023

multiple CIDRs per cluster/node is a highly useful feature, especially for workloads from different tenants. Are there any plans to enable this feature for cloud-related(ENI, Alibaba...) ipam modes?

Not at the moment, there are no plans to support cloud-based IPAM modes.

One difficulty in particular is that this will require deeper changes to the protocol between cilium-operator and per-node cilium-agent in ENI mode, as at the allocation mechanism for ENI modes and multi-pool work fundamentally different. In ENI mode, the operator decides when and how many IPs a node receives, where as with multi-pool, the agent issues an explicit request. There are multiple options to explore, but such a change requires a design document first.

@gandro gandro added the area/multipool Affects Multi-Pool IPAM label Oct 24, 2023
@gandro
Copy link
Member Author

gandro commented Oct 24, 2023

I'm closing this issue. There are remaining bugs, but those are better tracked via the area/multipool label.

@gandro gandro closed this as completed Oct 24, 2023
@chaunceyjiang
Copy link
Member

Hi @gandro It seems to me that Add check to ensure IPAM pools don't overlap has not been implemented yet. Is that correct? I would like to take up this issue.

@gandro
Copy link
Member Author

gandro commented Nov 1, 2023

Hi @gandro It seems to me that Add check to ensure IPAM pools don't overlap has not been implemented yet. Is that correct? I would like to take up this issue.

Yes, that has not yet been implemented. Any help here is very much appreciated 🎉 !
I've added a note about it in #26966 but it can be implemented separately.

One issue with it is that we're not sure how to best address it:

  • I don't think we can check this with CEL, since it is a cross-resource check.
  • Maybe we could do it via webhooks, but that's not something we can install via helm install
  • Maybe we could have a check in the operator? The question is how we expose/reject conflicting pools. A simple first version is to just log it as an error, but this will probably not be seen by most users. A more sophisticated version could extend the CiliumPodIPPool CRD with a status field, where we then write a "ok"/"not ok" flag into the status field of the resource. We could then also teach the operator to only use "ok" pools for allocation.

I'm very open to other ideas as well!

@chaunceyjiang
Copy link
Member

chaunceyjiang commented Nov 2, 2023

Perhaps we can build a webhook into operator. Similar to https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation, This way, we don't need to install an additional webhook-server.

@gandro
Copy link
Member Author

gandro commented Nov 2, 2023

Perhaps we can build a webhook into operator. Similar to https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation, This way, we don't need to install an additional webhook-server.

Ah, good point. Yeah, I think we have discussed a ValidatingAdmissionWebhook inside the operator in the past too, it would e.g. also be useful for #23152 - but I think there are some operational concerns about webhooks too (e.g. #23152 (comment)).

I'm not part of sig-k8s, so don't feel like I have the expertise to judge how complicated/tricky a webhook solution could be. On the other hand, having something which is opt-in might also allow us to gain a bit of experience with this.
It seems to me that adding webhook infrastructure might be something that warrants a CFP.

@chaunceyjiang
Copy link
Member

ut I think there are some operational concerns about webhooks too (e.g. #23152 (comment)).

Oh, you are right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ipam Impacts IP address management functionality. area/multipool Affects Multi-Pool IPAM
Projects
None yet
Development

No branches or pull requests

4 participants