Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: mount SA token when automountServiceAccountToken: false. Fixes #10937 #10945

Merged
merged 5 commits into from
Apr 24, 2023

Conversation

maxsxu
Copy link
Contributor

@maxsxu maxsxu commented Apr 19, 2023

Fixes #10937

…isabled

Signed-off-by: Max Xu <xuhuan@live.cn>
@maxsxu maxsxu marked this pull request as ready for review April 19, 2023 14:11
@maxsxu
Copy link
Contributor Author

maxsxu commented Apr 19, 2023

@terrytangyuan PTAL, thanks!

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed-off-by: Max Xu <xuhuan@live.cn>
Signed-off-by: Max Xu <xuhuan@live.cn>
@maxsxu
Copy link
Contributor Author

maxsxu commented Apr 21, 2023

Thanks. Could you add a test in https://github.com/argoproj/argo-workflows/tree/master/test/e2e?

@terrytangyuan Added two e2e tests, PTAL

@terrytangyuan
Copy link
Member

Test failure is related to your new test

For K8s 1.24+, KSA won't create token secret automatically.

Signed-off-by: Max Xu <xuhuan@live.cn>
@maxsxu
Copy link
Contributor Author

maxsxu commented Apr 22, 2023

Test failure is related to your new test

Fixed by adding Secret/argo.service-account-token. Now all CI passed.

It's due to the KSA token secret issue of K8s 1.24+, the CI / E2E Tests (test-functional, minimal) runs on 1.26 so it failed, but the CI / E2E Tests (test-functional, v1.21.2+k3s1, minimal) runs on 1.21 so it succeeds.

@terrytangyuan terrytangyuan merged commit 8786b46 into argoproj:master Apr 24, 2023
@maxsxu maxsxu deleted the fix-sa-token branch April 25, 2023 01:23
JPZ13 pushed a commit to pipekit/argo-workflows that referenced this pull request Jul 4, 2023
dpadhiar pushed a commit to dpadhiar/argo-workflows that referenced this pull request May 9, 2024
…isabled. Fixes argoproj#10937 (argoproj#10945)

Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
@agilgur5 agilgur5 changed the title fix: unable to connect cluster when AutomountServiceAccountToken is disabled. Fixes #10937 fix: mount SA token when automountServiceAccountToken: false. Fixes #10937 Oct 26, 2024
@agilgur5 agilgur5 added area/controller Controller issues, panics area/executor solution/invalid This is incorrect. Also can be used for spam labels Oct 26, 2024
@agilgur5
Copy link
Contributor

agilgur5 commented Oct 26, 2024

Per #12848 (comment), this seems to have caused a regression: an SA token shouldn't be mounted to the main container if automountServiceAccountToken: false -- you have literally instructed it not to. An error trying to reach the k8s API with no SA token would be correct behavior.

})
pod.Spec.Containers[i] = c
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setupServiceAccount function would also be the proper place to make such changes

WaitForWorkflow(fixtures.ToBeSucceeded, time.Minute*10).
Then().
ExpectWorkflow(func(t *testing.T, metadata *metav1.ObjectMeta, status *wfv1.WorkflowStatus) {
assert.Equal(t, wfv1.WorkflowSucceeded, status.Phase)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually should fail; the main container shouldn't have an accessible SA if you specified automountServiceAccountToken: false -- you have literally instructed it not to have one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controller Controller issues, panics area/executor solution/invalid This is incorrect. Also can be used for spam
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to connect cluster when specified executor.serviceAccountName but autoMountServiceAccountToken: false
4 participants