[AWS CCM] Permission to create SA token

[nckturner]

Permission to create servcice account tokens

• We need the ability to create service account token because this is required by clientbuilder/controller-manager framework which we will be using in 1.21.
• This is required for the CCM to use 1 SA per controller, which follows principle of least privilege and makes audit logs easier to understand

Note: edited to remove change from daemonset -> deployment. I think that change might still be valuable, at least as an option, but should be distinct from this change.

[nckturner]

/hold

[nckturner]

I'm not 100% about these yet. I was thinking it might be better to use a deployment so that replicas is independent from master node count, and a deployment of 1 seems to match the kops default of other components like KCM slightly better (although I'm not sure what the governing controller is). This means we will need to disable hostnetwork though, and I notice KCM is running with it enabled, so I'd like to understand whether there's any reason we need to run CCM the same way (getting metrics without a service?). Or, maybe it should be configurable.

Regarding the need for creating the service account token, I have a PR open to move the CCM to use the provided cloud provider libraries, and this, for some reason, requires permissions to create a service account token. We might want to make this permission optional (we shouldn't need it in all cases, like if we're using certificates for client identity, and even if we are using a service account, we aren't the token controller so.. why do we need this permission? ...). I need to look into this one a bit more but wanted to open this as a placeholder.

[johngmyers]

Wouldn't we want some redundancy for this, like we do for controller-manager?

[nckturner]

Yeah, maybe start with a default of 2.

[olemarkus]

You can set repicas to 1 or 2 based on the amount of CP nodes here: pkg/model/components/cloudconfiguration.go
You'd just have to add instance groups to the struct. These are created in upup/pkg/fi/cloudup/populate_cluster_spec.go

[johngmyers]

The concern I have with turning off host network would be whether we would get a dependency loop between this and the CNI.

[rifelpet]

The advantage of using a DaemonSet is that the level of redundancy for the CCM pod matches the redundancy for the control plane hosts. Users with 1 control plane host get 1 of each control plane pod.

We could also settle on using a template function to set the Deployment replicas to either 1 or 2 based on the number of control plane instance groups.

We'll also want to use topologySpreadConstraints if we do go with a Deployment.

[johngmyers]

With a topologySpreadConstraints preventing two on the same node we wouldn't have to turn off host network.

[olemarkus]

The concern I have with turning off host network would be whether we would get a dependency loop between this and the CNI.

I don't think this should be a problem. CNIs shouldn't be dependent on CCM. One thing that may be a concern is if it access the metadata API and IMDSv2 max-hop-limit is set to 1.

The advantage of using a DaemonSet is that the level of redundancy for the CCM pod matches the redundancy for the control plane hosts. Users with 1 control plane host get 1 of each control plane pod.

KCM etc are static pods, which is why they have hostnetworking and why we have higher redundancy than what we need for controllers with leader election.

We could also settle on using a template function to set the Deployment replicas to either 1 or 2 based on the number of control plane instance groups.

Not a huge fan of template functions here technically. But there is a number of controllers/operators where we could use this functionality.

[johngmyers]

My main concerns for a cyclic dependency would be the volume controller and the route controller. If the CNI can't get nodes to run its operators to get itself running that would be a problem.

[nckturner]

I decided to remove the change from Daemonset -> Deployment from this PR, and if we decide its still desired we'll create a separate PR. The more important change is giving the permissions required for the controller's 1.21 changes, where we use the upstream libraries for controller initialization. These libraries (clientbuilder) require SA token create permissions because they create a SA token per controller. This is a good thing, because it gives each controller its own identity, which helps for audit logs, and reduces the blast radius of potential vulnerabilities present in a single controller. SA per controller is enabled/disabled with --use-service-account-credentials. See #33310 and #99291 for more context.

[johngmyers]

in kubernetes/kubernetes#99291 Jordan Ligget states:

It makes sense for CCM to be able to request tokens specifically for the service accounts for the control loops in the CCM. That is possible to grant in a narrow way (grant "serviceaccounts/tokens" on just the resourceNames for the expected service accounts), but I couldn't find where the CCM role is defined.

But it seems this PR is granting broad "serviceaccounts/tokens" permissions, not just on the resourceNames for the expected service accounts.

[nckturner]

Thanks @johngmyers, I've restricted it to be just create token permissions on "node-controller", "service-controller" and "route-controller" resourceNames. This is sufficient for the cloud provider (there are technically 2 node controllers but they both use the same name). [1] [2] [3] [4]

[johngmyers]

@nckturner Thanks. You need to run hack/update-expected.sh and include those changes in your PR.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers

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 [johngmyers]

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

[nckturner]

/unhold