WIP: Don't let calico modify CNI plugins #11780
Conversation
This reverts commit 5d822ad.
This is not needed and has potential bad side effects (setting the mode / owner of files/directories below.
Calico install its cni plugins itself
k8s-ci-robot
added
release-note-action-required
k8s-ci-robot
added
approved
k8s-ci-robot
added
the
ok-to-test
|
Thanks, looks good to me. My original workaround didn't make any default behavior change because I wasn't familiar with CNI's and if that would have caused any side effects. |
| @@ -1,2 +0,0 @@ | |||
| --- | |||
There was a problem hiding this comment.
Nope. I'm reverting it because it's a workaround which is no longer needed, and the behavior change for existing users:
- Is pretty small
- Should be irrelevant now.
|
HI @cyclinder |
| @@ -447,7 +447,7 @@ downloads: | |||
| - etcd | |||
|
|
|||
| cni: | |||
| enabled: true | |||
| enabled: "{{ kube_network_plugin != 'calico' }}" | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Because install it's own and we end up with race, see #11747.
Good point about the multi- net plugin case though 🤔
/approve cancel
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The kube_owner was introduced for CIS compliance stuff, but I think it has been used a bit randomly since then 🤔 see #8952
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
In that case, 81ee96a should suffice, I think 🤔
what the recurse variable does?
There was a problem hiding this comment.
It applies the file attribute on everything in that dir tree. see #11747 (comment)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
removed
the
approved
VannTen
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
VannTen
changed the title
What type of PR is this?
/kind bug
What this PR does / why we need it:
Special note to reviewers
The implementation of this PR needs to be changed, see #11780 (comment)
Which issue(s) this PR fixes:
Fixes #11747
Does this PR introduce a user-facing change?:
/label tide/merge-method-merge