Handle incompatible automount token setting In service account YAML

[ihcsim]

Following #4145, the inject command will return an error with pods YAML that has the automountServiceAccountToken property set to false. The same validation should be applied to service accounts YAML. E.g., linkerd inject should return an error with the following YAML:

kind: ServiceAccount
apiVersion: v1
metadata:
  name: emoji
  namespace: emojivoto
automountServiceAccountToken: false

[han-so1omon]

New contributor here. I'm gonna hop on this and see if I can figure it out.

[han-so1omon]

From pkg/inject/inject.go:getFreshWorkloadObj() it appears that kind: ServiceAccount is not injectable. Maybe a simple question, but what does this entail?

[mayankshah1607]

Hi @han-so1omon
The current behavior is to straight away error out inject when a manifest contains automountServiceAccountToken: false. However, a ServiceAccount object, not being supported by the injection process, instead causes the CLI to throw a warning about the unsupported object despite the automountServiceAccountToken: false field. This issue entails the consistency in the behavior of inject w.r.t automountServiceAccountToken.

Ideally, we should be able to add support for the ServiceAccount object by having the inject CLI not ignore it, and throw an appropriate warning message when it contains automountServiceAccountToken: false (rather than outright failing it like in other cases, as ServiceAccount may not necessarily belong to a workload being injected).

You may want to check out the discussion thread on this PR - #4346

Feel free to reach out on slack if you have any questions! 😄

[ihcsim]

@han-so1omon Hop on the #contributors channel. We can help you out there.

[adleong]

I don't think that this is the right solution to this problem. As @han-so1omon correctly pointed out, ServiceAccounts are not injectable. What we really care about is that if a ServiceAccount with automountServiceAccountToken: false is used by an injected workload, that workload will fail to start.

Unfortunately, just failing if a ServiceAccount with automountServiceAccountToken: false is in the inject input doesn't check this. The given ServiceAccount might not actually be used by any of the workloads in the file (in which case inject would fail when it shouldn't) or the workloads in the file might use an existing ServiceAccount with automountServiceAccountToken: false (in which case inject would succeed but the workload would fail to start).

Ultimately, trying to catch this at the inject stage seems difficult. Would it be easier to catch this as a validating admission webhook?

[han-so1omon]

I think I agree with @adleong . Among other things, a failure for serviceaccount yaml automountServiceAccountToken: false may cause incompatibility of linkerd inject for users of linkerd with valid workflows.

If I am understanding your suggestion correctly @adleong , all serviceaccount yaml will pass, and the failure will be delegated to validating admission webhooks. Since I am a new contributor and am not entirely familiar with the architecture, could you explain how this is different from the current behavior? Is validating admission webhooks during runtime, pre-runtime but post-inject, or something else?

I believe the idea here is to invalidate incorrect serviceaccount configurations prior to kubectl apply. I'm still interested in pursuing a PR for this issue.

[adleong]

Take a look at https://github.com/linkerd/linkerd2/blob/main/controller/proxy-injector/webhook.go

The proxy injector is a webhook that sees every pod as they are created and adds the proxy sidecar when appropriate. I think it could also validate that the pod's service account has the correct setting for automoundServiceAccountToken and reject the pod if it does not. @alpeb and @ihcsim have more experience with webhooks and may be able to give more concrete advice.

[han-so1omon]

@adleong Thanks. I'll do some digging and write back here.

[alpeb]

Agreed that the injector webhook is an excellent place to validate this. In the newReport() function in pkg/inject/report.go a series of checks are performed which will flag whether the pod is injectable or not. You can add a check there that will see if the containers have a serviceaccount volume mounted.

One caveat is that linkerd inject --manual might also make use of this function but in that case it will be ok for a serviceaccount mount to not be there 😉

[han-so1omon]

I'm adding a k8s ServiceAccountInformer to the controller/k8s/api.go so that I can get information about automountServiceAccountToken status during the webhook admissions in the proxy-injector. When I do this the controller/k8s::API::syncChecks hangs with "waiting for caches to sync".

Does anyone have an idea why this would happen?

So far I have added service account support to controller/k8s/api.go::APIResource, controller/k8s/api.go::API, controller/k8s/api.go::NewAPI. Relevant parts shown below. There are some other steps necessary to fix this issue that are not shown.

...
const (
	CJ APIResource = iota
        ...
	SA
	...
)

// API provides shared informers for all Kubernetes objects
type API struct {
	Client kubernetes.Interface

	cj       batchv1beta1informers.CronJobInformer
        ...
	sa       coreinformers.ServiceAccountInformer
	...

	syncChecks        []cache.InformerSynced
	sharedInformers   informers.SharedInformerFactory
	spSharedInformers sp.SharedInformerFactory
	tsSharedInformers ts.SharedInformerFactory
}
...

func NewAPI(
	k8sClient kubernetes.Interface,
	spClient spclient.Interface,
	tsClient tsclient.Interface,
	resources ...APIResource,
) *API {
	sharedInformers := informers.NewSharedInformerFactory(k8sClient, 10*time.Minute)
        ...

	for _, resource := range resources {
		switch resource {
		case CJ:
			api.cj = sharedInformers.Batch().V1beta1().CronJobs()
			api.syncChecks = append(api.syncChecks, api.cj.Informer().HasSynced)
                ...
		case SA:
			api.sa = sharedInformers.Core().V1().ServiceAccounts()
			api.syncChecks = append(api.syncChecks, api.sa.Informer().HasSynced)
                ...
}

...

// SA provides access to a shared informer and listener for ServiceAccounts.
func (api *API) SA() coreinformers.ServiceAccountInformer {
	if api.sa == nil {
		panic("SA informer not configured")
	}
	return api.sa
}
...

Edit: showing controller/cmd/proxy-injector/main.go and providing some
debug information.

func Main(args []string) {
	webhook.Launch(
		[]k8s.APIResource{k8s.SA, k8s.NS, k8s.Deploy, k8s.RC, k8s.RS, k8s.Job, k8s.DS, k8s.SS, k8s.Pod, k8s.CJ},
        ...
}

In controller/cmd/proxy-injector/main.go::Main, if I remove k8s.SA, then injection starts fine, but it won't initialize the SA Informer. If I add another k8s resource to controller/cmd/proxy-injector/main.go::Main, e.g. I have tried to add k8s.MWC, then I again run into the issue in the proxy-injector pod, process hang at "waiting for caches to sync"

[part-time-githubber]

I ran into this today. The automountServiceAccountToken was set to false in the deployment. But I think there is an operational issue. There did not seem to be any evidence of what was happening. No events on the deployment. Proxy injector logs at the default level had no logs. Changing to debug level was still the same behaviour. We are using stable-2.8.1 at the moment.

[jimil749]

Agreed that the injector webhook is an excellent place to validate this. In the newReport() function in pkg/inject/report.go a series of checks are performed which will flag whether the pod is injectable or not. You can add a check there that will see if the containers have a serviceaccount volume mounted.

Hi @alpeb!
Can we check for serviceAccountName in the pod spec and then maybe then fetch the value of automountServiceAccountToken field from that service account to validate the workload or just check if the volume is mounted onto the pod's container?

[alpeb]

Can we check for serviceAccountName in the pod spec and then maybe then fetch the value of automountServiceAccountToken field from that service account to validate the workload or just check if the volume is mounted onto the pod's counter?

Sounds to me the latter should do. Looks like we already have a check in place, but do nothing if the mount is not there:

linkerd2/pkg/inject/inject.go

Lines 535 to 541 in b5dddf5

	if saVolumeMount != nil {
	values.GetGlobal().Proxy.SAMountPath = &l5dcharts.VolumeMountPath{
	Name: saVolumeMount.Name,
	MountPath: saVolumeMount.MountPath,
	ReadOnly: saVolumeMount.ReadOnly,
	}
	}