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.

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

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

Merged
merged 14 commits into from
Mar 24, 2023
116 changes: 81 additions & 35 deletions charts/calico/templates/calico-node-rbac.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,6 @@ rules:
{{- end }}
verbs:
- create
# The CNI plugin needs to get pods, nodes, and namespaces.
Copy link
Member

Choose a reason for hiding this comment

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

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

- apiGroups: [""]
resources:
- pods
- nodes
- namespaces
verbs:
- get
# EndpointSlices are used for Service-based network policy rule
# enforcement.
- apiGroups: ["discovery.k8s.io"]
Expand Down Expand Up @@ -76,13 +68,6 @@ rules:
verbs:
- list
- watch
# The CNI plugin patches pods/status.
- apiGroups: [""]
resources:
- pods/status
verbs:
- patch
# Calico monitors various CRDs for config.
- apiGroups: ["crd.projectcalico.org"]
resources:
- globalfelixconfigs
Expand Down Expand Up @@ -138,26 +123,6 @@ rules:
- create
- update
{{- if eq .Values.network "calico" }}
# These permissions are required for Calico CNI to perform IPAM allocations.
Copy link
Member

Choose a reason for hiding this comment

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

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

- apiGroups: ["crd.projectcalico.org"]
resources:
- blockaffinities
- ipamblocks
- ipamhandles
verbs:
- get
- list
- create
- update
- delete
# The CNI plugin and calico/node need to be able to create a default
Copy link
Member

Choose a reason for hiding this comment

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

This comment suggests calico-node still needs the IPAMConfigs permission

# IPAMConfiguration
- apiGroups: ["crd.projectcalico.org"]
resources:
- ipamconfigs
verbs:
- get
- create
# Block affinities must also be watchable by confd for route aggregation.
- apiGroups: ["crd.projectcalico.org"]
resources:
Expand All @@ -176,6 +141,61 @@ rules:

---

metadata:
caseydavenport marked this conversation as resolved.
Show resolved Hide resolved
name: calico-cni-plugin
rules:
# Used for creating service account tokens to be used by the CNI plugin
- apiGroups: [""]
resources:
- serviceaccounts/token
Copy link
Member

@caseydavenport caseydavenport Dec 19, 2022

Choose a reason for hiding this comment

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

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.

resourceNames:
{{- if eq .Values.network "flannel" }}
- canal-cni
{{- else }}
- calico-cni-plugin
{{- end }}
verbs:
- create
# The CNI plugin needs to get pods, nodes, and namespaces.
- apiGroups: [""]
resources:
- pods
- nodes
- namespaces
verbs:
- get
{{- if eq .Values.datastore "kubernetes" }}
- apiGroups: [""]
resources:
- pods/status
verbs:
- patch
{{- if eq .Values.network "calico" }}
# These permissions are required for Calico CNI to perform IPAM allocations.
- apiGroups: ["crd.projectcalico.org"]
resources:
- blockaffinities
- ipamblocks
- ipamhandles
verbs:
- get
- list
- create
- update
- delete
# The CNI plugin and calico/node need to be able to create a default
# IPAMConfiguration
- apiGroups: ["crd.projectcalico.org"]
resources:
- ipamconfigs
verbs:
- get
- create
{{- end }}
{{- end }}

---

{{- if eq .Values.network "flannel" }}
# Flannel ClusterRole
# Pulled from https://github.com/coreos/flannel/blob/master/Documentation/kube-flannel-rbac.yml
Expand Down Expand Up @@ -227,6 +247,19 @@ subjects:
- kind: ServiceAccount
name: canal
namespace: kube-system
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
name: canal-calico-cni
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: calico-cni-plugin
subjects:
- kind: ServiceAccount
name: canal-cni-plugin
namespace: kube-system
{{- else }}
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
Expand All @@ -240,4 +273,17 @@ subjects:
- kind: ServiceAccount
name: calico-node
namespace: kube-system
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
name: calico-cni-plugin
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: calico-cni-plugin
subjects:
- kind: ServiceAccount
name: calico-cni-plugin
namespace: kube-system
{{- end }}
15 changes: 12 additions & 3 deletions node/pkg/cni/token_watch.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ import (
)

const (
defaultServiceAccountName = "calico-node"
defaultNodeAccountName = "calico-node"
defaultCNIPluginAccountName = "calico-cni-plugin"
serviceAccountNamespace = "/var/run/secrets/kubernetes.io/serviceaccount/namespace"
tokenFile = "/var/run/secrets/kubernetes.io/serviceaccount/token"
defaultCNITokenValiditySeconds = 24 * 60 * 60
Expand Down Expand Up @@ -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())
Copy link
Member

Choose a reason for hiding this comment

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

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.

tokenChan := tr.TokenChan()
go tr.Run()

Expand All @@ -244,7 +245,15 @@ func CNIServiceAccountName() string {
logrus.WithField("name", sa).Debug("Using service account from CALICO_CNI_SERVICE_ACCOUNT")
return sa
}
return defaultServiceAccountName
return defaultCNIPluginAccountName
}

func NodeServiceAccountName() string {
Copy link
Member

Choose a reason for hiding this comment

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

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

if na := os.Getenv("CALICO_NODE_SERVICE_ACCOUNT"); na != "" {
logrus.WithField("name", na).Debug("Using service account from CALICO_NODE_SERVICE_ACCOUNT")
return na
}
return defaultNodeAccountName
}

// writeKubeconfig writes an updated kubeconfig file to disk that the CNI plugin can use to access the Kubernetes API.
Expand Down
7 changes: 7 additions & 0 deletions node/tests/k8st/infra/calico-kdd.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@ metadata:
name: calico-node
namespace: kube-system
---
# Source: calico/templates/calico-node.yaml
caseydavenport marked this conversation as resolved.
Show resolved Hide resolved
apiVersion: v1
kind: ServiceAccount
metadata:
name: calico-cni-plugin
namespace: kube-system
---
# Source: calico/templates/calico-config.yaml
# This ConfigMap is used to configure a self-hosted Calico installation.
kind: ConfigMap
Expand Down