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.

Sign up for GitHub

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

WIP: Don't let calico modify CNI plugins #11780

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+4 −6
Open

WIP: Don't let calico modify CNI plugins #11780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dba4861
Revert "Support overriding cni directory owner (#10929)"
VannTen Dec 10, 2024
81ee96a
Don't set use recurse for cni directory
VannTen Dec 10, 2024
77bfe72
Don't install cni when using calico
VannTen Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion roles/kubespray-defaults/defaults/main/download.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ downloads:
- etcd

cni:
enabled: true
enabled: "{{ kube_network_plugin != 'calico' }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @VannTen, Why do we don't need to install cni when calico is enabled? I think we still need this, in some user cases, the user needs to run macvlan/ipvlan cni and calico at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because install it's own and we end up with race, see #11747.

Good point about the multi- net plugin case though 🤔
/approve cancel

Copy link
Contributor

Choose a reason for hiding this comment

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

the CNI and calico installations this should be independent, not mutually exclusive. The similarity is that they both share the hostPath /opt/cni/bin. calico only installs the binary calico and calico-ipam into /opt/cni/bin, so it expects /opt/cni/bin to be present on the host, and if it is not, it creates.

It seems that revert #10407 should solve the problem, but I'm not sure what the owner variable does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The kube_owner was introduced for CIS compliance stuff, but I think it has been used a bit randomly since then 🤔 see #8952

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the CNI and calico installations this should be independent, not mutually exclusive

That would make more sense to me, but it seems calico is saying otherwise : projectcalico/calico#6004 (comment) (unless I'm parsing that wrong about the "installing upstream containernetworking/plugins binaries"

In that case, 81ee96a should suffice, I think 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

but it seems calico is saying otherwise : projectcalico/calico#6004 (comment)

calico installs these CNI plugins: flannel, bandwidth, host-local, portmap, etc. These plugins may be required to work with calico (optional), so calico installs them by default. However, other plugins such as macvlan, ipvlan, etc. (https://github.com/containernetworking/plugins/tree/main/plugins) are not installed by calico. so we do need to install the whole cni plugins.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, 81ee96a should suffice, I think 🤔

what the recurse variable does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It applies the file attribute on everything in that dir tree. see #11747 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok ! Thanks for the explanation, I haven't looked closely at the CNI bits in K8s before 👍

In that case, I think we should instead keep our install (but still fixes those recurse/permissions stuff) and patches the calico manifests to not install the CNI.
It seems their cni plugins are slightly different according to projectcalico/calico#6004 (comment) (bumped deps, etc). I'm not sure if we should ship their versions or not 🤔 when using calico.

The simpler path would be not to, that's for sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

It applies the file attribute on everything in that dir tree. see #11747 (comment)

the all CNI binary attributes must be executable, or we can have a task for changing the file attributes under the /opt/cni/bin

I think we should instead keep our install (but still fixes those recurse/permissions stuff)

agree

and patches the calico manifests to not install the CNI.

It's difficult for us, Calico, to install not only these CNI plugins: flannel, bandwidth, host-local, port map, etc., but also their own CNI plugins(Calico and calico-ipam). It looks like the key to solving the problem is to change the properties of these files.

file: true
version: "{{ cni_version }}"
dest: "{{ local_release_dir }}/cni-plugins-linux-{{ image_arch }}-{{ cni_version }}.tgz"
Expand Down
2 changes: 0 additions & 2 deletions roles/network_plugin/cni/defaults/main.yml
View file
Open in desktop

This file was deleted.

5 changes: 2 additions & 3 deletions roles/network_plugin/cni/tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@
path: /opt/cni/bin
state: directory
mode: "0755"
owner: "{{ cni_bin_owner }}"
recurse: true
owner: "{{ kube_owner }}"

- name: CNI | Copy cni plugins
unarchive:
src: "{{ downloads.cni.dest }}"
dest: "/opt/cni/bin"
mode: "0755"
owner: "{{ cni_bin_owner }}"
owner: "{{ kube_owner }}"
remote_src: true
1 change: 1 addition & 0 deletions roles/network_plugin/meta/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
---
dependencies:
- role: network_plugin/cni
when: kube_network_plugin != 'calico'

- role: network_plugin/cilium
when: kube_network_plugin == 'cilium' or cilium_deploy_additionally | default(false) | bool
Expand Down