ipam: Add ability to automatically create `CiliumPodIPPool` resources in multi-pool IPAM mode #25991

gandro · 2023-06-07T15:43:04Z

Before this PR, we populated the multi-pool allocator from pre-defined pools based on the multi-pool-map flag. Since we added a custom CiliumPodIPPool resource however (c.f. #25824), there was a mismatch between those pre-defined pools (which are static and not backed up by a resource) and pools defined via CRD.

This PR addresses this mismatch by instead creating a CiliumPodIPPool resource for every pre-defined pool present in auto-create-cilium-pod-ip-pools. This means that those pre-defined pools are now regular pools that are automatically created upon operator startup. The name of the new flag is taken from the auto-create-cilium-node-resource flag which also creates resources upon startup.

Having the option to pre-define pools at installation time is important, because cilium-agent requires pod CIDRs at least from the default pool before it becomes ready. By providing an option to have cilium-operator auto-create some pre-defined pools, we solve this bootstrapping problem in an easy way. The alternative would require users to manually create
default pools after running helm install, which increases operational complexity.

Note that if the pre-defined pools already exist, the operator will not modify re-create or update them. This behavior is intentional, to avoid overwriting potential changes made by the user.

Meta: #25470

ℹ️ Review per commit

Example values.yaml:

ipam:
    mode: multi-pool
    operator:
        autoCreateCiliumPodIPPools:
            default:
              ipv4:
                cidrs:
                  - 10.10.0.0/16
                maskSize: 24
            mars:
              ipv4:
                cidrs:
                  - 10.20.0.0/16
                maskSize: 24
            other:
              ipv6:
                cidrs:
                  - fd00:100::/80
                maskSize: 96

debug:
    enabled: true

# Kind values
image:
    override: localhost:5000/cilium/cilium-dev:local
    pullPolicy: IfNotPresent
operator:
    replicas: 1
    image:
        override: localhost:5000/cilium/operator-generic:local
        pullPolicy: IfNotPresent

# Datapath requirements for multi-pool
tunnel: disabled
autoDirectNodeRoutes: true
ipv4NativeRoutingCIDR: 10.10.0.0/8
endpointRoutes:
    enabled: true
extraConfig:
    enable-local-node-route: "false"
kubeProxyReplacement: "strict"
bpf:
    masquerade: true

Will create three IP pools (default, mars and other), as can be seen with:

$ kubectl get cpip -o yaml   # abbreviated output
apiVersion: v1
items:
- apiVersion: cilium.io/v2alpha1
  kind: CiliumPodIPPool
  metadata:
    name: default
  spec:
    ipv4:
      cidrs:
      - 10.10.0.0/16
      maskSize: 24
- apiVersion: cilium.io/v2alpha1
  kind: CiliumPodIPPool
  metadata:
    name: mars
  spec:
    ipv4:
      cidrs:
      - 10.20.0.0/16
      maskSize: 24
- apiVersion: cilium.io/v2alpha1
  kind: CiliumPodIPPool
  metadata:
    name: other
  spec:
    ipv6:
      cidrs:
      - fd00:100::/80
      maskSize: 96
kind: List
metadata:
  resourceVersion: ""

Proper CI and documentation will be added in a follow-up PR.

qmonnet

Looks good as far as I'm concerned, there's one report to address below

pkg/ipam/allocator/multipool/allocator.go

gandro · 2023-06-07T16:12:13Z

/test

tklauser

Nice work!

Very minor comments and a suggestion for a follow-up change inline.

operator/watchers/cilium_podippool.go

pkg/ipam/allocator/multipool/allocator.go

install/kubernetes/cilium/templates/cilium-configmap.yaml

operator/watchers/cilium_podippool.go

tommyp1ckles · 2023-06-08T21:23:19Z

operator/watchers/cilium_podippool.go

+			for _, cidr := range strings.Split(value, ",") {
+				_, err := netip.ParsePrefix(cidr)
+				if err != nil {
+					return cilium_v2alpha1.IPPoolSpec{}, fmt.Errorf("invalid value for key %q: %w", poolKeyIPv4CIDRs, err)


For errors such as this one (and similar below), it might be helpful for someone reading logs if you specify what kind of validation failed.

Not sure what you mean by that. It currently prints the following error if this line is hit, what do you think is missing?

level=fatal msg="Failed to parse IP pool spec in \"auto-create-cilium-pod-ip-pools\" flag" error="invalid value for key \"ipv4-cidrs\": netip.ParsePrefix(\"172.300.0.0/16\"): ParseAddr(\"172.300.0.0\"): IPv4 field has value >255" poolName=broken poolSpec="ipv4-cidrs:172.300.0.0/16,172.17.0.0/16;ipv4-mask-size:24" subsys=watchers

operator/watchers/cilium_podippool.go

This commit fixes the name of the logging subsystem used by the multi-pool allocator. The multi-pool allocator was previously named cluster-pool-v2beta2 during development, but has since been renamed. Fixes: 3bffa18 ("operator: Add core logic for IPAM pools") Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

This changes the Golang definition of the CilumPodIPPool type to use pointers instead of values for the IPv4/IPv6 fields. This is required because those fields are marked as optional in our kubebuilder annotations, but the fields of the structs themselves are required. This caused an issue that it was not possible to create or update CiliumPodIPPool resources from Golang, because Golang uses a zero-valued default struct for the IPv4 or IPv6 fields, which in turn causes validation errors because the required fields within the IPv4/IPv6 struct where now zero/empty. For example, if one created an IPv4 only pool: ``` error: CiliumPodIPPool.cilium.io "default" is invalid: [ spec.ipv6.maskSize: Invalid value:0: spec.ipv6.maskSize in body should be greater than or equal to 1, spec.ipv6.cidrs: Required value ] ``` This commit also changes the integer size of the mask size field to a more appropriate width. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

Before this commit, we populated the multi-pool allocator from pre-defined pools based on the `multi-pool-map` flag. Since we added a custom `CiliumPodIPPool` resource however, there was a mismatch between those pre-defined pools (which are static and not backed up by a resource) and pools defined via CRD. This commit addresses this mismatch by creating a `CiliumPodIPPool` resource for every pre-defined pool present in `multi-pool-map`. This means that those pre-defined pools are now regular pools that are automatically created upon operator startup. Having the option to pre-define pools at installation time is important, because cilium-agent requires pod CIDRs at least from the default pool before it becomes ready. By providing an option to have cilium-operator auto-create some pre-defined pools, we solve this bootstrapping problem in an easy way. The alternative would require users to manually create default pools after running `helm install`, which increases operational complexity. Note that if the pre-defined pools already exist, the operator will not modify re-create or update them. This behavior is intentional, to avoid overwriting potential changes made by the user. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

This commit renames the `multi-pool-map` to `auto-create-cilium-pod-ip-pools` to better reflect it's new functionality. The name is taken from the `auto-create-cilium-node-resource` flag which also creates resources upon startup. In addition, the flag is also now unhidden and its Helm value syntax is changed to be more similar to the schema of the CiliumPodIPPool resource. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

This commit removes the `AddPool` function which is no longer used now that all call sites use `UpsertPool` instead. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

gandro · 2023-06-12T15:14:22Z

I believe I addressed all nits. Rebased and restarting CI

gandro · 2023-06-12T15:14:29Z

/test

gandro · 2023-06-12T18:24:09Z

test-1.26-net-next had a VM provisioning error (failed to fetch docker image) https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/708/console
ci-ginkgo failed with invalid value: 31414: provided port is already allocated (e.g. CI: K8sPolicyTest Multi-node policy test with L7 policy using connectivity-check to check datapath: provided port is already allocated #13071) (https://github.com/cilium/cilium/actions/runs/5245376421/jobs/9472783968)
ci-ginko also failed with with a provisioning error (https://github.com/cilium/cilium/actions/runs/5245376421/jobs/9472786521)
ci-ginko also failed with CI: K8sDatapathConfig Transparent encryption DirectRouting Check connectivity with transparent encryption and direct routing with bpf_host #25964 (https://github.com/cilium/cilium/actions/runs/5245376421/jobs/9472788454)
ci-aks failed with an connectivity infra flake between cilium-cli and kube-apiserver error dialing backend: write unix @->/tunnel-uds/proxysocket: write: broken pipe, this is the same root cause as #26032, #26034, #26014

Restarting all failed tests

gandro · 2023-06-12T18:24:55Z

/test-1.26-net-next

Job 'Cilium-PR-K8s-1.26-kernel-net-next' hit: #25958 (87.92% similarity)

gandro · 2023-06-12T18:25:44Z

/ci-aks

gandro · 2023-06-13T08:03:55Z

CI-net-next hit #25958

gandro · 2023-06-13T08:04:11Z

/test-1.26-net-next

sayboras

the multiPoolMap attribute in helm is added as part of the 1.14 dev cycle, so we can skip the deprecation step for renaming.

Helm changes lgtm ✔️

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. cilium#25824 (comment) Ref. cilium#25991 (comment) Signed-off-by: Tobias Klauser <tobias@cilium.io>

gandro requested a review from tklauser June 7, 2023 15:43

gandro requested review from a team as code owners June 7, 2023 15:43

gandro requested review from tommyp1ckles, christarazi, sayboras and qmonnet June 7, 2023 15:43

gandro changed the title ~~Pr/gandro/multi pool auto create cpip~~ ipam: Add ability to automatically create CiliumPodIPPool resources in multi-pool IPAM mode Jun 7, 2023

gandro force-pushed the pr/gandro/multi-pool-auto-create-cpip branch from 5abec83 to 61c2ec4 Compare June 7, 2023 15:52

gandro mentioned this pull request Jun 7, 2023

IPAM Multi-Pool Follow-ups #25470

Closed

29 tasks

gandro force-pushed the pr/gandro/multi-pool-auto-create-cpip branch from 61c2ec4 to 738b612 Compare June 7, 2023 16:00

qmonnet approved these changes Jun 7, 2023

View reviewed changes

pkg/ipam/allocator/multipool/allocator.go Show resolved Hide resolved

tklauser approved these changes Jun 8, 2023

View reviewed changes

operator/watchers/cilium_podippool.go Outdated Show resolved Hide resolved

operator/watchers/cilium_podippool.go Outdated Show resolved Hide resolved

pkg/ipam/allocator/multipool/allocator.go Show resolved Hide resolved

tommyp1ckles reviewed Jun 8, 2023

View reviewed changes

christarazi approved these changes Jun 9, 2023

View reviewed changes

gandro added 5 commits June 12, 2023 15:44

multipool: Remove unused AddPool function

1b22d25

This commit removes the `AddPool` function which is no longer used now that all call sites use `UpsertPool` instead. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

gandro force-pushed the pr/gandro/multi-pool-auto-create-cpip branch from 738b612 to 1b22d25 Compare June 12, 2023 14:29

tklauser mentioned this pull request Jun 12, 2023

IPAM multipool followups #26138

Merged

maintainer-s-little-helper bot mentioned this pull request Jun 12, 2023

CI: Cilium K8s Client connection reset by peer #25958

Closed

sayboras approved these changes Jun 13, 2023

View reviewed changes

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 Jun 13, 2023

gandro merged commit e2555a0 into cilium:main Jun 13, 2023
64 of 65 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ipam: Add ability to automatically create `CiliumPodIPPool` resources in multi-pool IPAM mode #25991

ipam: Add ability to automatically create `CiliumPodIPPool` resources in multi-pool IPAM mode #25991

gandro commented Jun 7, 2023 •

edited by tklauser

qmonnet left a comment

gandro commented Jun 7, 2023

tklauser left a comment

tommyp1ckles Jun 8, 2023 •

edited

gandro Jun 12, 2023 •

edited

gandro commented Jun 12, 2023

gandro commented Jun 12, 2023

gandro commented Jun 12, 2023

gandro commented Jun 12, 2023 •

edited by maintainer-s-little-helper bot

gandro commented Jun 12, 2023

gandro commented Jun 13, 2023

gandro commented Jun 13, 2023

sayboras left a comment

ipam: Add ability to automatically create CiliumPodIPPool resources in multi-pool IPAM mode #25991

ipam: Add ability to automatically create CiliumPodIPPool resources in multi-pool IPAM mode #25991

Conversation

gandro commented Jun 7, 2023 • edited by tklauser

qmonnet left a comment

Choose a reason for hiding this comment

gandro commented Jun 7, 2023

tklauser left a comment

Choose a reason for hiding this comment

tommyp1ckles Jun 8, 2023 • edited

Choose a reason for hiding this comment

gandro Jun 12, 2023 • edited

Choose a reason for hiding this comment

gandro commented Jun 12, 2023

gandro commented Jun 12, 2023

gandro commented Jun 12, 2023

gandro commented Jun 12, 2023 • edited by maintainer-s-little-helper bot

gandro commented Jun 12, 2023

gandro commented Jun 13, 2023

gandro commented Jun 13, 2023

sayboras left a comment

Choose a reason for hiding this comment

ipam: Add ability to automatically create `CiliumPodIPPool` resources in multi-pool IPAM mode #25991

ipam: Add ability to automatically create `CiliumPodIPPool` resources in multi-pool IPAM mode #25991

gandro commented Jun 7, 2023 •

edited by tklauser

tommyp1ckles Jun 8, 2023 •

edited

gandro Jun 12, 2023 •

edited

gandro commented Jun 12, 2023 •

edited by maintainer-s-little-helper bot