Fix calico host local ipam #11022

robertvolkmann · 2024-03-21T18:07:38Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

The init container upgrade-ipam should only be added for calico-ipam, because otherwise it would clear the state of the host-local plugin, potentially causing it to reassign IPs that are still in use.

Additionally, the environment variable USE_POD_CIDR is necessary for the host-local plugin.

https://github.com/projectcalico/calico/blob/4efd1bfd914b0c59086531c8c5a5ac5b593c18b1/charts/calico/templates/calico-node.yaml#L279
https://github.com/projectcalico/calico/blob/4efd1bfd914b0c59086531c8c5a5ac5b593c18b1/charts/calico/templates/calico-typha.yaml#L133

NONE

Otherwise, the init container upgrade-ipam would clear the state of the host-local plugin, potentially causing it to reassign IPs that are still in use.

https://github.com/projectcalico/calico/blob/4efd1bfd914b0c59086531c8c5a5ac5b593c18b1/charts/calico/templates/calico-node.yaml#L279 https://github.com/projectcalico/calico/blob/4efd1bfd914b0c59086531c8c5a5ac5b593c18b1/charts/calico/templates/calico-typha.yaml#L133

k8s-ci-robot · 2024-03-21T18:07:47Z

Welcome @robertvolkmann!

It looks like this is your first PR to kubernetes-sigs/kubespray 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubespray has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-03-21T18:07:47Z

Hi @robertvolkmann. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cyclinder

Thanks for the catch!

cyclinder · 2024-03-22T02:09:35Z

roles/network_plugin/calico/templates/calico-node.yml.j2

@@ -428,7 +432,7 @@ spec:
          hostPath:
            path: /run/xtables.lock
            type: FileOrCreate
-{% if calico_datastore == "kdd" %}
+{% if calico_datastore == "kdd" and not calico_ipam_host_local %}


This looks wrong, look at the comment: It's used when upgrading from host-local to calico-ipam, we need this only when ipam is host-local. and you can see https://github.com/projectcalico/calico/blob/4efd1bfd914b0c59086531c8c5a5ac5b593c18b1/charts/calico/templates/calico-node.yaml#L641

Please let me know if I'm wrong.

Yeah, this was also not obvious to me. If the user wants to use host-local as IPAM, then migrating the host-local state to calico-ipam is wrong because host-local should be used.

The manifest in the Helm Chart allows for a non-working setup because it doesn't support calico as the network backend with host-local IPAM. See projectcalico/calico#8642.

In my setup, where I use Calico only for policy enforcement (i.e., calico_backend: none and calico_ipam_host_local: true), migrating to calico-ipam involves removing the running state of the host-local IPAM, potentially causing it to reassign IPs that are still in use. You can refer to this link for more details: https://github.com/projectcalico/calico/blob/4efd1bfd914b0c59086531c8c5a5ac5b593c18b1/manifests/calico-policy-only.yaml#L4781.

sorry for the delay. you are right! We need mount this only when the ipam is calico-ipam.

cyclinder · 2024-03-22T02:10:48Z

roles/network_plugin/calico/templates/calico-typha.yml.j2

-          # Configure route aggregation based on pod CIDR.
-          # - name: USE_POD_CIDR
-          #   value: "true"
+{% if calico_ipam_host_local %}


It looks like there's some formatting error. doesn't it?

I've reviewed it again, and everything seems correct on my end. If you notice any issues or have any specific suggestions, please feel free to point them out.

HI @cyclinder
Do you have any comments ?

@yankay sorry for the delay.

and everything seems correct on my end

aha, it's maybe a github issue.

yankay · 2024-03-22T19:11:19Z

/ok-to-test

cyclinder · 2024-04-03T05:49:55Z

thanks!

/lgtm
/cc @yankay

yankay · 2024-04-03T07:48:14Z

Thanks @robertvolkmann @cyclinder
/approve

k8s-ci-robot · 2024-04-03T07:48:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robertvolkmann, yankay

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [yankay]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Prevent upgrade-ipam for host-local IPAM Otherwise, the init container upgrade-ipam would clear the state of the host-local plugin, potentially causing it to reassign IPs that are still in use. * USE_POD_CIDR required for host-local https://github.com/projectcalico/calico/blob/4efd1bfd914b0c59086531c8c5a5ac5b593c18b1/charts/calico/templates/calico-node.yaml#L279 https://github.com/projectcalico/calico/blob/4efd1bfd914b0c59086531c8c5a5ac5b593c18b1/charts/calico/templates/calico-typha.yaml#L133

robertvolkmann added 2 commits March 21, 2024 19:02

Prevent upgrade-ipam for host-local IPAM

335296e

Otherwise, the init container upgrade-ipam would clear the state of the host-local plugin, potentially causing it to reassign IPs that are still in use.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 21, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 21, 2024

k8s-ci-robot requested review from cyclinder and MrFreezeex March 21, 2024 18:07

robertvolkmann mentioned this pull request Mar 21, 2024

Prevent upgrade-ipam for host-local IPAM projectcalico/calico#8642

Closed

3 tasks

cyclinder reviewed Mar 22, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 22, 2024

k8s-ci-robot requested a review from yankay April 3, 2024 05:49

k8s-ci-robot assigned cyclinder Apr 3, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 3, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 3, 2024

k8s-ci-robot merged commit 3067e56 into kubernetes-sigs:master Apr 3, 2024
60 checks passed

robertvolkmann deleted the fix-calico-host-local-ipam branch April 17, 2024 10:00

mzaian mentioned this pull request Apr 26, 2024

Release Proposal v2.25 #11126

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix calico host local ipam #11022

Fix calico host local ipam #11022

robertvolkmann commented Mar 21, 2024

k8s-ci-robot commented Mar 21, 2024

k8s-ci-robot commented Mar 21, 2024

cyclinder left a comment

cyclinder Mar 22, 2024

robertvolkmann Mar 22, 2024 •

edited

Loading

cyclinder Apr 3, 2024

cyclinder Mar 22, 2024

robertvolkmann Mar 22, 2024

yankay Apr 3, 2024

cyclinder Apr 3, 2024

yankay commented Mar 22, 2024

cyclinder commented Apr 3, 2024

yankay commented Apr 3, 2024

k8s-ci-robot commented Apr 3, 2024

Fix calico host local ipam #11022

Fix calico host local ipam #11022

Conversation

robertvolkmann commented Mar 21, 2024

k8s-ci-robot commented Mar 21, 2024

k8s-ci-robot commented Mar 21, 2024

cyclinder left a comment

Choose a reason for hiding this comment

cyclinder Mar 22, 2024

Choose a reason for hiding this comment

robertvolkmann Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

cyclinder Apr 3, 2024

Choose a reason for hiding this comment

cyclinder Mar 22, 2024

Choose a reason for hiding this comment

robertvolkmann Mar 22, 2024

Choose a reason for hiding this comment

yankay Apr 3, 2024

Choose a reason for hiding this comment

cyclinder Apr 3, 2024

Choose a reason for hiding this comment

yankay commented Mar 22, 2024

cyclinder commented Apr 3, 2024

yankay commented Apr 3, 2024

k8s-ci-robot commented Apr 3, 2024

robertvolkmann Mar 22, 2024 •

edited

Loading