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/multipool: Introduce specific ip family annotations for specifying ip pools #28244

Conversation

hargrovee
Copy link

@hargrovee hargrovee commented Sep 21, 2023

In dual-stack mode, it's necessary to allocate both IPv4 and IPv6 addresses for pods. However, custom IP pools may only focus on either IPv4 or IPv6, which can result in the custom IP pool being configured with only IPv4 or IPv6. When specifying an IP pool for a namespace or pod through annotations, this can cause pod creation to fail because the specified IP pool lacks the IPv4 or IPv6 family.

This PR introduces two IP-family specific annotations: ipam.cilium.io/ipv4-pool and ipam.cilium.io/ipv6-pool. When specifying pools for pods or namespaces, combining these annotations allows for handling more scenarios.

This PR covers the use cases mentioned here #28204 (comment) and has been implemented based on the suggestions here #28204 (review).

@hargrovee hargrovee requested review from a team as code owners September 21, 2023 12:11
@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 Sep 21, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Sep 21, 2023
@hargrovee hargrovee force-pushed the introduce-specific-IP-family-annotations-for-specifying-IP-pools branch 5 times, most recently from bfed145 to 91b8c0a Compare September 22, 2023 00:41
@gandro gandro added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/ipam IP address management, including cloud IPAM area/ipam Impacts IP address management functionality. labels Sep 25, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Sep 25, 2023
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, looks good overall. Thanks for taking care of this!

I've left some minor feedback about the code itself.

pkg/ipam/metadata/manager.go Outdated Show resolved Hide resolved
pkg/ipam/metadata/manager.go Outdated Show resolved Hide resolved
pkg/ipam/metadata/manager.go Show resolved Hide resolved
pkg/ipam/metadata/manager.go Outdated Show resolved Hide resolved
@hargrovee hargrovee force-pushed the introduce-specific-IP-family-annotations-for-specifying-IP-pools branch 2 times, most recently from 00703b5 to 0abae38 Compare September 25, 2023 14:50
@maintainer-s-little-helper
Copy link

Commit dd6cdc950f0f7378d61cb66d79413f14b935a81e 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

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Sep 25, 2023
@hargrovee hargrovee force-pushed the introduce-specific-IP-family-annotations-for-specifying-IP-pools branch from dd6cdc9 to 0abae38 Compare September 25, 2023 15:18
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Sep 25, 2023
@hargrovee hargrovee force-pushed the introduce-specific-IP-family-annotations-for-specifying-IP-pools branch from 0abae38 to 86b847b Compare September 25, 2023 15:19
@hargrovee
Copy link
Author

Thanks! I have made the changes.

Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, looks good! One remaining minor suggestion

pkg/ipam/metadata/manager.go Outdated Show resolved Hide resolved
@hargrovee hargrovee force-pushed the introduce-specific-IP-family-annotations-for-specifying-IP-pools branch from 86b847b to cc7213b Compare September 26, 2023 15:10
@hargrovee hargrovee force-pushed the introduce-specific-IP-family-annotations-for-specifying-IP-pools branch 4 times, most recently from 274edee to a5e3b6b Compare September 26, 2023 17:01
@hargrovee
Copy link
Author

Thank you. I made the changes. Please let me know if there are any other issues. Appreciate your guidance.

Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last suggestion, then it should be good to go!

pkg/ipam/metadata/manager.go Outdated Show resolved Hide resolved
@hargrovee hargrovee force-pushed the introduce-specific-IP-family-annotations-for-specifying-IP-pools branch from a5e3b6b to a1d53c8 Compare September 27, 2023 12:13
Huagong Wang added 2 commits September 27, 2023 20:13
This adds two annotations, "ipam.cilium.io/ipv4-pool" and
"ipam.cilium.io/ipv6-pool". The former can be added to pods
or namespaces to specify an IPv4 pool for workloads, while
the latter corresponds to specifying an IPv6 pool for workloads.

Signed-off-by: Huagong Wang <wanghuagong@kylinos.cn>
Now, it will first check if the pod has annotations
ipam.cilium.io/ipv4-pool or ipam.cilium.io/ipv6-pool;
if so, it will return the pool from the specific IP family.
If these annotations are not present, it will check if
the pod has the annotation ipam.cilium.io/ip-pool, and if
found, it will return the pool as usual. If the pod doesn't
have any annotations, it will then check if the namespace in
which the pod resides has these annotations in the same order.
If neither the pod nor the namespace has these annotations,
it will return the default IP pool.

Signed-off-by: Huagong Wang <wanghuagong@kylinos.cn>
@hargrovee hargrovee force-pushed the introduce-specific-IP-family-annotations-for-specifying-IP-pools branch from a1d53c8 to 19bd9c2 Compare September 27, 2023 12:13
@hargrovee
Copy link
Author

Thank you for your guidance and thoughtful suggestions. Much appreciated.

Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@gandro
Copy link
Member

gandro commented Sep 28, 2023

/test

@gandro
Copy link
Member

gandro commented Sep 28, 2023

@tommyp1ckles PTAL

@gandro gandro removed the request for review from tommyp1ckles October 3, 2023 08:47
@gandro gandro merged commit ae30c5c into cilium:main Oct 3, 2023
61 checks passed
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 3, 2023
@hargrovee hargrovee deleted the introduce-specific-IP-family-annotations-for-specifying-IP-pools branch October 13, 2023 02:44
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. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/ipam IP address management, including cloud IPAM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants