fix: don't mount SA token when automountServiceAccountToken: false. Fixes #12848

[MinyiZ]

Fixes #12848

Motivation

#10945 introduced a regression as it is mounting a serviceaccount token on the main container when automountServiceAccountToken: false.

The security docs say

The main container will have the service account token mounted, allowing the main container to patch pods (among other permissions). Set automountServiceAccountToken to false to prevent this.

#10937 was not a bug. It is possible to explicitly mount a token to container and script template workflows even if automountServiceAccountToken: false so the code change was not needed.

Modifications

• Reverts fix: mount SA token when automountServiceAccountToken: false. Fixes #10937 #10945
• Add test for resource template with automountServiceAccountToken: false and executor.serviceAccountName set

Verification

Tests passed

[agilgur5]

Regarding the tests here and from #10945, a resource template should correctly fail if automountServiceAccountToken: false since, well, there is no SA token for it to use

[MinyiZ]

Regarding the tests here and from #10945, a resource template should correctly fail if automountServiceAccountToken: false since, well, there is no SA token for it to use

I've fixed the tests from #10945 to demonstrate that it is possible to explicitly mount a token to container and script template workflows even if automountServiceAccountToken: false so the code change was not needed.

I believe resource templates shouldn't fail if automountServiceAccountToken: false because the exec token is mounted at

argo-workflows/workflow/controller/operator.go

Line 3453 in 25bbb71

mainCtr := woc.newExecContainer(common.MainContainerName, tmpl)

and I've added an e2e test to demonstrate that.

[agilgur5]

I believe resource templates shouldn't fail if automountServiceAccountToken: false because the exec token is mounted at

Ah nice, I wasn't sure if resource templates handled executor.serviceAccountName since they use a main container.

I've fixed the tests from #10945 to demonstrate that it is possible to explicitly mount a token to container and script template workflows even if automountServiceAccountToken: false so the code change was not needed.

I saw that, but I don't think that test is very functional / necessary. Basically you're manually mounting the token instead of automounting it. A user shouldn't really ever do that manually though, they should just automount. That is also testing k8s behavior rather than Argo behavior at that point.
I think both those tests should just be removed.

agilgur5 changed the title fix: resource template etting duplicate exec-sa-token volume mounts with automountServiceAccountToken: false. Fixes #12848 fix: don't mount SA token when automountServiceAccountToken: false. Fixes #12848 12 hours ago

MinyiZ changed the title fix: don't mount SA token when automountServiceAccountToken: false. Fixes #12848 fix: resource template getting duplicate exec-sa-token volume mounts with automountServiceAccountToken: false. Fixes #12848 5 hours ago

Regarding the PR title, my change was intentional. The PR title should summarize what the PR does, not what the issue is. The PR title and issue title should not be equivalent, as they are now.
The actual source code change is not specific to resource templates either, as the code path does not check for a resource template, which is why I did not label it as such

[MinyiZ]

I think both those tests should just be removed.

Yep agreed. Done.

Regarding the PR title, my change was intentional.

Ooops sorry about that. I unintentionally reverted your change when I was fixing up a typo. I've changed it back now.

[agilgur5]

LGTM, thanks for taking a look and fixing this!