Separate calico-node and calico-cni-plugin service accounts #7106

MichalFupso · 2022-12-19T14:33:52Z

Description

Split of the calico-node account to calico-node and calico-cni-plugin service account.

Related issues/PRs

Todos

Tests
Documentation
Release note

Release Note

Separate calico-node and calico-cni-plugin service accounts

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

docs-pr-required: This change requires a change to the documentation that has not been completed yet.
docs-completed: This change has all necessary documentation completed.
docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

release-note-required: This PR has user-facing changes. Most PRs should have this label.
release-note-not-required: This PR has no user-facing changes.

Other optional labels:

cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

CLAassistant · 2022-12-19T14:33:59Z

All committers have signed the CLA.

caseydavenport

A few minor comments. I think you'll want to do a few things:

Test this on a live cluster to make sure all the permissions are correct (check the calico-node and CNI plugin logs for unauthorized permissions errors)
Run make generate to update the manifests - you changed the templates in the charts/ dir, but the artifacts need re-rendering afterwards.
You will need to make the equivalent changes in the tigera/operator repository, since most users install Calico that way nowadays. https://github.com/tigera/operator/blob/master/pkg/render/node.go#L287

caseydavenport · 2022-12-19T17:19:14Z

charts/calico/templates/calico-node-rbac.yaml

+  # Used for creating service account tokens to be used by the CNI plugin
+  - apiGroups: [""]
+    resources:
+      - serviceaccounts/token


This permission should only be on the calico-node serviceaccount, not on the CNI plugin. We don't want any serviceaccount to have permissions to create its own token.

charts/calico/templates/calico-node-rbac.yaml

caseydavenport · 2022-12-19T17:20:04Z

charts/calico/templates/calico-node-rbac.yaml

@@ -138,26 +123,6 @@ rules:
      - create
      - update
 {{- if eq .Values.network "calico" }}
-  # These permissions are required for Calico CNI to perform IPAM allocations.


I think with testing you'll find that calico/node also needs some of these permissions.

caseydavenport · 2022-12-19T17:20:42Z

charts/calico/templates/calico-node-rbac.yaml

@@ -17,14 +17,6 @@ rules:
 {{- end }}
    verbs:
      - create
-  # The CNI plugin needs to get pods, nodes, and namespaces.


I think calico/node also needs some of these, unless they are already covered somewhere else.

caseydavenport · 2022-12-19T17:21:17Z

charts/calico/templates/calico-node-rbac.yaml

-      - create
-      - update
-      - delete
-  # The CNI plugin and calico/node need to be able to create a default


This comment suggests calico-node still needs the IPAMConfigs permission

caseydavenport · 2022-12-19T17:23:06Z

node/pkg/cni/token_watch.go

@@ -217,7 +218,7 @@ func Run() {
 	if err != nil {
 		logrus.WithError(err).Fatal("Failed to create in cluster client set")
 	}
-	tr := NewTokenRefresher(clientset, NamespaceOfUsedServiceAccount(), CNIServiceAccountName())
+	tr := NewTokenRefresher(clientset, NamespaceOfUsedServiceAccount(), NodeServiceAccountName())


The goal of this PR is so that calico/node is generating new tokens for the CNI plugin's service account, so I think we want to leave this line unchanged.

caseydavenport · 2022-12-19T17:23:47Z

node/pkg/cni/token_watch.go

+	return defaultCNIPluginAccountName
+}
+
+func NodeServiceAccountName() string {


I don't think we need this function - we shouldn't ever be doing any actions on the calico-node serviceaccount.

charts/calico/templates/calico-node-rbac.yaml

node/tests/k8st/infra/calico-kdd.yaml

node/pkg/cni/token_watch.go

manifests/tigera-operator.yaml

caseydavenport · 2023-03-03T18:03:30Z

/sem-approve

caseydavenport · 2023-03-14T18:54:10Z

charts/calico/templates/calico-node-rbac.yaml

@@ -242,3 +284,15 @@ subjects:
  name: calico-node
  namespace: kube-system
 {{- end }}
+kind: ClusterRoleBinding


I think this can be a RoleBinding since it's only for a single serviceaccount in one namespace.

In your other PR, this is a RoleBinding: https://github.com/tigera/operator/pull/2393/files#diff-d4d31d15e69aa9a0bad7776e758dfbd50b8e0e1a969fd16719f56c470cac4e7bR316

MichalFupso · 2023-03-23T21:07:57Z

/sem-approve

mgleung · 2023-03-23T21:18:19Z

/sem-approve

mgleung · 2023-03-23T23:25:53Z

/sem-approve

caseydavenport · 2023-03-24T16:44:59Z

/sem-approve

See: projectcalico/calico#7106

* [calico] Make version 3.26.1 default * [calico] Separate calico-node and calico-cni-plugin service accounts See: projectcalico/calico#7106

MichalFupso added 2 commits December 19, 2022 09:22

calico-cni-plugin user

92ecc2f

add calico-cni-plugin account

2ccfb10

MichalFupso requested a review from a team as a code owner December 19, 2022 14:33

marvin-tigera added this to the Calico v3.25.0 milestone Dec 19, 2022

marvin-tigera added docs-pr-required Change is not yet documented release-note-required Change has user-facing impact (no matter how small) labels Dec 19, 2022

caseydavenport requested changes Dec 19, 2022

View reviewed changes

mgleung modified the milestones: Calico v3.25.0, Calico v3.26.0 Jan 5, 2023

MichalFupso added 4 commits January 13, 2023 14:36

calico-cni-plugin updates

776f615

cni account name update

2782961

naming update

a1d66ef

naming

e53ca6c

caseydavenport requested changes Jan 23, 2023

View reviewed changes

manifest files update

658126e

caseydavenport reviewed Jan 27, 2023

View reviewed changes

manifests/tigera-operator.yaml Outdated Show resolved Hide resolved

MichalFupso added 2 commits February 27, 2023 16:58

Merge branch 'master' into calico-cni-plugin

03bfd2d

calico-cni token test

df303f6

caseydavenport approved these changes Mar 3, 2023

View reviewed changes

calico-node token

3c61908

caseydavenport approved these changes Mar 14, 2023

View reviewed changes

caseydavenport reviewed Mar 14, 2023

View reviewed changes

MichalFupso added 2 commits March 15, 2023 16:57

Merge branch 'master' into calico-cni-plugin

7e80e32

calico-cni-plugin service account

bdf436a

caseydavenport approved these changes Mar 23, 2023

View reviewed changes

MichalFupso removed the docs-pr-required Change is not yet documented label Mar 23, 2023

MichalFupso added the docs-not-required Docs not required for this change label Mar 23, 2023

static check fix

6649e27

imports

e373cea

caseydavenport changed the title ~~calico-cni-plugin user~~ Separate calico-node and calico-cni-plugin service accounts Mar 24, 2023

MichalFupso merged commit 5ddead4 into projectcalico:master Mar 24, 2023

paulcostinean mentioned this pull request May 6, 2023

Failed to "KillPodSandbox" due to calico connection is unauthorized #7220

Closed

kimullaa mentioned this pull request Jun 7, 2023

Failed to Upgrade Calico v3.25.1 to v3.26.0 tigera/docs#716

Closed

dghubble mentioned this pull request Jun 19, 2023

Update Calico from v3.25.1 to v3.26.1 poseidon/terraform-render-bootstrap#358

Merged

mzaian added a commit to mzaian/kubespray that referenced this pull request Sep 11, 2023

[calico] Separate calico-node and calico-cni-plugin service accounts

fcf21c2

See: projectcalico/calico#7106

mzaian mentioned this pull request Sep 11, 2023

[calico] Make version 3.26.1 default kubernetes-sigs/kubespray#10416

Merged

mzaian added a commit to mzaian/kubespray that referenced this pull request Sep 14, 2023

[calico] Separate calico-node and calico-cni-plugin service accounts

3f21223

See: projectcalico/calico#7106

mzaian added a commit to mzaian/kubespray that referenced this pull request Sep 18, 2023

[calico] Separate calico-node and calico-cni-plugin service accounts

ac6d8db

See: projectcalico/calico#7106

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate calico-node and calico-cni-plugin service accounts #7106

Separate calico-node and calico-cni-plugin service accounts #7106

MichalFupso commented Dec 19, 2022 •

edited by caseydavenport

Loading

CLAassistant commented Dec 19, 2022 •

edited

Loading

caseydavenport left a comment

caseydavenport Dec 19, 2022 •

edited

Loading

caseydavenport Dec 19, 2022

caseydavenport Dec 19, 2022

caseydavenport Dec 19, 2022

caseydavenport Dec 19, 2022

caseydavenport Dec 19, 2022

caseydavenport commented Mar 3, 2023

caseydavenport Mar 14, 2023

caseydavenport Mar 14, 2023

MichalFupso commented Mar 23, 2023

mgleung commented Mar 23, 2023

mgleung commented Mar 23, 2023

caseydavenport commented Mar 24, 2023

Separate calico-node and calico-cni-plugin service accounts #7106

Separate calico-node and calico-cni-plugin service accounts #7106

Conversation

MichalFupso commented Dec 19, 2022 • edited by caseydavenport Loading

Description

Related issues/PRs

Todos

Release Note

Reminder for the reviewer

CLAassistant commented Dec 19, 2022 • edited Loading

caseydavenport left a comment

Choose a reason for hiding this comment

caseydavenport Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

caseydavenport Dec 19, 2022

Choose a reason for hiding this comment

caseydavenport Dec 19, 2022

Choose a reason for hiding this comment

caseydavenport Dec 19, 2022

Choose a reason for hiding this comment

caseydavenport Dec 19, 2022

Choose a reason for hiding this comment

caseydavenport Dec 19, 2022

Choose a reason for hiding this comment

caseydavenport commented Mar 3, 2023

caseydavenport Mar 14, 2023

Choose a reason for hiding this comment

caseydavenport Mar 14, 2023

Choose a reason for hiding this comment

MichalFupso commented Mar 23, 2023

mgleung commented Mar 23, 2023

mgleung commented Mar 23, 2023

caseydavenport commented Mar 24, 2023

MichalFupso commented Dec 19, 2022 •

edited by caseydavenport

Loading

CLAassistant commented Dec 19, 2022 •

edited

Loading

caseydavenport Dec 19, 2022 •

edited

Loading