ipam: allow /32 and /128 IP pools by juliusmh · Pull Request #34618 · cilium/cilium

juliusmh · 2024-08-30T11:27:00Z

This PR makes CiliumPodIPPools that have mask /32 or /128 usable, not restricting the first and last addresses. This can be used to give Pods specific IP addresses. The only way to achieve this today is by requesting a /30 and blocking an unnecessary big chunk of the range.

Commits:

ipam: allow /32 and /128 IP pools
documentation: IP pools with /32 or /128 clarification

Related: #17026 (fixed IP proposal)
Related: #28637

Multi-Pool IPAM now allows the use of /32 or /128 CIDRs in CiliumPodIPPools

maintainer-s-little-helper · 2024-08-30T11:27:05Z

Commits 34179f1, 9d68e1f 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

juliusmh · 2024-08-30T14:53:43Z

/test

ghost

Thank you for the PR @juliusmh! Docs look good.

gandro

Thanks for your PR!

This is clever, but I would prefer it if we removed the "first-last" limitation in general via an opt-in flag, not just for /32 or /128 CIDRs. Similarly to how lifted that restriction for LBIPAM, where it is configurable in the pool: #26488 (comment)

However, I'm fine to merge this approach, since it is a small improvement in the right direction (since /32 and /128 CIDRs are completely unusable at the moment). I've left some minor feedback

pkg/ipam/service/ipallocator/allocator.go

juliusmh · 2024-09-08T11:42:11Z

Hi @gandro, thanks for the feedback. I'm not confident enough with making changes to the API (yet), but I agree it should be aligned with the LBIPAM pool in the future. Regarding your feedback, I did the following changes:

adjusted the "reserved IPs" logic for pools with less than 3 addresses (/31, /32, /127 and /128)
added unit tests for NewCIDRRange and addrsInPrefix
added short explanation to GoDoc
documented the changes @lambdanis

Btw, I used git rebase and fixup, let me know if this causes any inconvenience for you.

gandro

Awesome, thank you so much!

Btw, I used git rebase and fixup, let me know if this causes any inconvenience for you.

That is exactly the way we prefer it!

gandro · 2024-09-09T08:08:12Z

/test

gandro · 2024-09-09T08:14:21Z

Please note, the go linter is failing with the following error:

 Error: pkg/ipam/service/ipallocator/allocator_test.go:15:42: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)
  		panic(fmt.Errorf("net.ParseCIDR: %+v", err))

CallMeFoxie · 2024-09-09T08:17:23Z

Would it be possible the "waste first and last" IP range size be configurable? Because we give out /28 or /29 to ip pools (because most nodes run around 10 pods at max anyways) so it would be wasting quite a few IPs.

Also it would solve the forever issue of being it configurable if. Or is it too much of a problem?

gandro · 2024-09-09T09:03:00Z

Would it be possible the "waste first and last" IP range size be configurable? Because we give out /28 or /29 to ip pools (because most nodes run around 10 pods at max anyways) so it would be wasting quite a few IPs.

Yes. This is what I have proposed here: #34618 (review)

I strongly feel that this option to relax the first-last-restriction should be per pool, and not global. The only reason we do it for very small pools globally and by default is that they are not usable otherwise.

However, @juliusmh has already mentioned that no changes to the CRD are planned in this PR. Changes to the API thus will require additional contributions not expected of this PR here.

bewing · 2024-09-09T12:56:53Z

Technically this would also apply to /31 and /127 prefixes as well?

gandro · 2024-09-09T15:30:51Z

Technically this would also apply to /31 and /127 prefixes as well?

As mentioned here #34618 (comment) this PR does this now.

gandro · 2024-09-09T15:34:08Z

I triaged CI. Most issues are flakes, but there appear to be two real issues. Could you take a look @juliusmh? Thanks!

The linting issue mentioned above:

 Error: pkg/ipam/service/ipallocator/allocator_test.go:15:42: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)
  		panic(fmt.Errorf("net.ParseCIDR: %+v", err))

The unit tests in github.com/cilium/cilium/pkg/ipam/allocator/multipool are failing with the following failure:

--- FAIL: Test_addrsInPrefix (0.00s)
--- FAIL: Test_addrsInPrefix/zero (0.00s)

    pool_allocator_test.go:497: addrsInPrefix() = 2, want 0

juliusmh · 2024-09-09T19:13:55Z

@gandro

adjusted linting
the test case failing is interesting. If using p := netip.Prefix{}, addrs is 2 because p.Bits() == -1. Added explicit check for validity with p.IsValid() before calculating

oblazek · 2024-09-10T06:54:44Z

/test

juliusmh · 2024-09-12T09:08:31Z

@oblazek @gandro is there anything left to do?

dylandreimerink · 2024-09-12T09:27:09Z

is there anything left to do?

Mostly getting CI green. I have seen the gink-go tests failing across multiple PRs, not sure if the test has been fixed already. Best cause of action is to rebase on the main branch to get the latest fixes and we will try to run CI again to see if that does it.

Signed-off-by: Julius Hinze <[email protected]>

dylandreimerink · 2024-09-12T10:02:09Z

/test

juliusmh · 2024-09-12T11:59:51Z

Just Conformance Cluster Mesh (ci-clustermesh) failing. I assume it's a flake as well?

Extend `NewCIDRRange` with a `WithAllowFirstLastIPs` functional option that includes the first and last IPs of a CIDR in the allocatable range. This is needed for AWS ENI prefix delegation where /28 prefixes are exclusively assigned to a node and all 16 IPs are usable as there is no shared network segment requiring base/broadcast reservation. Also refactor `ForEach` to use `r.base` directly instead of assuming a +1 offset from the CIDR base, making it correct for both default and `AllowFirstLastIPs` ranges. Relates to [cilium/design-cfps#87](cilium/design-cfps#87) Followup to #34618 See #28637 (comment) Signed-off-by: Hadrien Patte <[email protected]>

juliusmh requested review from a team as code owners August 30, 2024 11:27

juliusmh requested review from a user and doniacld August 30, 2024 11:27

maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Aug 30, 2024

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Aug 30, 2024

juliusmh force-pushed the multipool_single_ips branch from 9d68e1f to 2f0b65c Compare August 30, 2024 11:28

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Aug 30, 2024

juliusmh force-pushed the multipool_single_ips branch from 2f0b65c to f580d52 Compare August 30, 2024 11:38

ghost changed the title ~~multipool single ips~~ ipam: allow /32 and /128 IP pools Sep 2, 2024

ghost approved these changes Sep 2, 2024

View reviewed changes

gandro reviewed Sep 3, 2024

View reviewed changes

pkg/ipam/service/ipallocator/allocator.go Outdated Show resolved Hide resolved

juliusmh force-pushed the multipool_single_ips branch 2 times, most recently from 4ff5320 to 7c41c76 Compare September 8, 2024 11:36

gandro approved these changes Sep 9, 2024

View reviewed changes

gandro removed the request for review from doniacld September 9, 2024 08:05

gandro added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Sep 9, 2024

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 Sep 9, 2024

gandro added area/ipam IP address management, including cloud IPAM area/multipool Affects Multi-Pool IPAM labels Sep 9, 2024

gandro mentioned this pull request Sep 9, 2024

Setting a /32 or /128 IP blockSize on a pool not possible #28637

Open

2 tasks

juliusmh force-pushed the multipool_single_ips branch from 7c41c76 to 67190b3 Compare September 9, 2024 18:56

This was referenced Sep 10, 2024

CI: Conformance Ginkgo: K8sDatapathServicesTest Checks N/S loadbalancing Supports IPv4 fragments [It] #34717

Closed

CI: Conformance Ginkgo: K8sDatapathServicesTest Checks N/S loadbalancing [It] Tests with XDP, direct routing, Hybrid and Random #34716

Closed

juliusmh added 2 commits September 12, 2024 11:56

ipam: allow /31, /32, /127, /128 IP pools

69dbc63

Signed-off-by: Julius Hinze <[email protected]>

documentation: IP pools with /32 or /128 clarification

c1d429e

Signed-off-by: Julius Hinze <[email protected]>

juliusmh force-pushed the multipool_single_ips branch from 67190b3 to c1d429e Compare September 12, 2024 09:56

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 Sep 12, 2024

dylandreimerink added this pull request to the merge queue Sep 12, 2024

Merged via the queue into cilium:main with commit afdee51 Sep 12, 2024

juliusmh deleted the multipool_single_ips branch September 16, 2024 11:56

MohammedNoureldin mentioned this pull request Dec 28, 2024

Will cilium IPAM support specified ip address of pod? #17026

Open

HadrienPatte mentioned this pull request Mar 27, 2026

ipam: add AllowFirstLastIPs option to ipallocator.NewCIDRRange #45025

Open

Conversation

juliusmh commented Aug 30, 2024 • edited by gandro Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maintainer-s-little-helper bot commented Aug 30, 2024

Uh oh!

juliusmh commented Aug 30, 2024

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

gandro left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

juliusmh commented Sep 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gandro left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gandro commented Sep 9, 2024

Uh oh!

gandro commented Sep 9, 2024

Uh oh!

CallMeFoxie commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gandro commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bewing commented Sep 9, 2024

Uh oh!

gandro commented Sep 9, 2024

Uh oh!

gandro commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juliusmh commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oblazek commented Sep 10, 2024

Uh oh!

juliusmh commented Sep 12, 2024

Uh oh!

dylandreimerink commented Sep 12, 2024

Uh oh!

dylandreimerink commented Sep 12, 2024

Uh oh!

juliusmh commented Sep 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

juliusmh commented Aug 30, 2024 •

edited by gandro

Loading

gandro left a comment •

edited

Loading

juliusmh commented Sep 8, 2024 •

edited

Loading

gandro left a comment •

edited

Loading

CallMeFoxie commented Sep 9, 2024 •

edited

Loading

gandro commented Sep 9, 2024 •

edited

Loading

gandro commented Sep 9, 2024 •

edited

Loading

juliusmh commented Sep 9, 2024 •

edited

Loading