Exposing serviceAccountName in the TenantControlPlane CRD

[hamza-boudouche]

Hello,

I would like to propose adding the possibility to specify a service account in the TenantControlPlane spec that will be passed down to the pods (through the deployment).

Possibly something like tenantcontrolplane.spec.controlPlane.deployment.serviceAccountName mapped to deploy.spec.template.spec.serviceAccountName.

[prometherion]

I'm interested in the use case you're trying to achieve, considering the sensitive information, what are you trying to achieve?

Asking that because AFAIK we're not mounting any ServiceAccount in the TCP pod, if the use case is relevant I'm open in supporting it, such as accepting PRs.

[prometherion]

Add a mutating admission webhook to use the default service account if it's not specified (so all existing e2e tests will still be passing).

We can take advantage of the Kubebuilder default marker, that is leveraging the OAPIv3 default feature.

Add a validating admission webhook to check if the specified service account exists

I would avoid this kind of check, Kubernetes is already taking care of that by creating a Pod with a non referenced ServiceAccount.

Events:
  Type     Reason        Age                From                   Message
  ----     ------        ----               ----                   -------
  Warning  FailedCreate  1s (x12 over 11s)  replicaset-controller  Error creating: pods "hello-node-78d5f5dfb5-" is forbidden: error looking up service account default/foo: serviceaccount "foo" not found

[hamza-boudouche]

We can take advantage of the Kubebuilder default marker, that is leveraging the OAPIv3 default feature.

I suppose this means that we have to hard code the name of the default service account in the marker. I believe this is less than ideal. I was thinking of accepting a default service account name as a flag here for example and then adding logic for it here

I would avoid this kind of check, Kubernetes is already taking care of that by creating a Pod with a non referenced ServiceAccount.

From a user's pov I'd prefer getting an explicit error on creation. This way I can avoid having to dig into the underlying resources to find out why my control plane isn't Ready. This also preserves a nice encapsulation of the internal implementation details (that a user might not necessarily be familiar with). What do you think?

[prometherion]

We have to keep the current behaviour to avoid breaking changes: I just checked the code and I was wrong, we're NOT disabling the ServiceAccount auto mount, meaning that the default one (default) is going to be mounted.

If a user wants to customize that we're providing the new API field that allows customising it.

From a user's pov I'd prefer getting an explicit error on creation

This is Kubernetes design, by non-blocking the creation of resources, since the order is not guaranteed: it eventually completes.
Kamaji sticks to these behaviours since recognized by the community.

This also preserves a nice encapsulation of the internal implementation details

If the user doesn't provide a particular ServiceAccount, the default one is going to be used, as it's right now.

[hamza-boudouche]

This is Kubernetes design, by non-blocking the creation of resources, since the order is not guaranteed: it eventually completes.

That seems reasonable.

If the user doesn't provide a particular ServiceAccount, the default one is going to be used, as it's right now.

Yes, that's the goal of the mutating admission webhook I mentioned earlier.

It seems that we're on the same page regarding what should be done now. I will start working on this.

[prometherion]

Thanks, keep me posted or poke me privately, happy to guide you through the code base! 🚀