Add automountServiceAccountToken option to alertmanager spec

[stgrace]

Description

Add automountServiceAccountToken option to alertmanager spec. As a best practice it's best to disable mounting a service account token for pods that do not need to interact with the Kubernetes API spec, hence this pull request.

Type of change

• CHANGE (fix or feature that would cause existing functionality to not work as expected)
• FEATURE (non-breaking change which adds functionality)
• BUGFIX (non-breaking change which fixes an issue)
• ENHANCEMENT (non-breaking change which improves existing functionality)
• NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

Add automountServiceAccountToken option to alertmanager spec

[ArthurSens]

I'd rather have it disabled by default and not have the option to turn it back on 🤔. Do you see a use-case where alertmanager would need the serviceaccount mounted?

[simonpasquier]

IIUC the PR doesn't change the current behavior: the SA token will not be automatically mounted until the user sets the field to true.

Do you see a use-case where alertmanager would need the serviceaccount mounted?

One example is when using kube-rbac-proxy in front of Alertmanager. One could probably use SA token volume projection but it's more involved that just setting automountServiceAccountToken.

We might want to have a similar change for the ThanosRuler CRD but it doesn't have to be in this PR.

[stgrace]

Hi @simonpasquier

IIUC the PR doesn't change the current behavior: the SA token will not be automatically mounted until the user sets the field to true.

Not sure what you mean? By default, all pods mount their serviceaccounttoken in kubernetes. Adding this option to alertmanager and setting it to false allows users to opt out of this default behavior.

[stgrace]

I'd rather have it disabled by default and not have the option to turn it back on 🤔. Do you see a use-case where alertmanager would need the serviceaccount mounted?

I'd also think that disabling it and not having the option to turn it back on is a good idea if there is no reason for this component to have access to the kubernetes API

[simonpasquier]

By default, all pods mount their serviceaccounttoken in kubernetes.

Yes sorry I wasn't clear enough: I meant to say that for people that have already opted out for API credential automounting at the SA level (https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#opt-out-of-api-credential-automounting), it won't change anything. Similarly for people that rely on automounting at the SA level, the token will still be mounted unless they explicitly set spec.automountServiceAccountToken: false

As a follow-up, this snippet achieves the same goal:

apiVersion: monitoring.rhobs/v1
kind: Alertmanager
metadata:
  name: example
spec:
   ...
  volumeMounts:
  - mountPath: /var/run/secrets/tokens
    name: sa-token
  volumes:
  - name: sa-token
    projected:
      sources:
      - serviceAccountToken:
          path: sa-token

[simonpasquier]

if there is no reason for this component to have access to the kubernetes API

this isn't a valid assumption (see my comment earlier about kube-rbac-proxy).

[stgrace]

Yes sorry I wasn't clear enough: I meant to say that for people that have already opted out for API credential automounting at the SA level (https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#opt-out-of-api-credential-automounting), it won't change anything. Similarly for people that rely on automounting at the SA level, the token will still be mounted unless they explicitly set spec.automountServiceAccountToken: false

I understand that if you disable it at the SA level it doesn't change anything, but I still think it's a good idea to have it as an option to also disable it at the pod level.

[simonpasquier]

I understand that if you disable it at the SA level it doesn't change anything, but I still think it's a good idea to have it as an option to also disable it at the pod level.

Yes I agree. But to be clear, I don't think that we should default to automountServiceAccountToken: false.

[stgrace]

@simonpasquier Is there anything that needs to be changed for this PR then?

[vimalk78]

@stgrace any updates planned for this PR?

[stgrace]

@stgrace any updates planned for this PR?

What updates are exactly required? For me this is ready to be merged?

[ArthurSens]

Sorry for taking this long to review, just have some small nits.

[ArthurSens]

Suggested change

	// If you don't want the kubelet to automatically mount a ServiceAccount's API credentials, you can opt out of the default behavior.
	// You can opt out of automounting API credentials on /var/run/secrets/kubernetes.io/serviceaccount/token for a pod by setting automountServiceAccountToken: false on the pod spec.
	AutomountServiceAccountToken *bool `json:"automountServiceAccountToken,omitempty"`
	// AutomountServiceAccountToken defines if the ServiceAccount token is mounted into Alertmanager's pod or not.
	// +optional
	AutomountServiceAccountToken *bool `json:"automountServiceAccountToken,omitempty"`

[simonpasquier]

I think that the original comment is useful because otherwise it might be unclear when this field is useful.

Suggested change

	// If you don't want the kubelet to automatically mount a ServiceAccount's API credentials, you can opt out of the default behavior.
	// You can opt out of automounting API credentials on /var/run/secrets/kubernetes.io/serviceaccount/token for a pod by setting automountServiceAccountToken: false on the pod spec.
	AutomountServiceAccountToken *bool `json:"automountServiceAccountToken,omitempty"`
	// AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in the pod.
	// If the service account has `automountServiceAccountToken: true`, set the field to `false` to opt out of automounting API credentials.
	AutomountServiceAccountToken *bool `json:"automountServiceAccountToken,omitempty"`

[ArthurSens]

Sounds good to me, let's not forget about the extra comment // +optional 🙂

[stgrace]

Sorry for the late response,
Added the latest comment from @simonpasquier, with the // +optional

[vimalk78]

hi @stgrace , can you please incorporate the comment.
If you don't have time, i am happy to open another PR which includes your commit.

[simonpasquier]

Thanks!