-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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: Always create manual long lived token #19970
base: master
Are you sure you want to change the base?
fix: Always create manual long lived token #19970
Conversation
✅ Preview Environment deployed on Bunnyshell
See: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
✅ Preview Environment created on Bunnyshell but will not be auto-deployedSee: Environment Details Available commands (reply to this comment):
|
55fb526
to
cb33bf0
Compare
util/clusterauth/clusterauth.go
Outdated
secret := &corev1.Secret{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
GenerateName: serviceAccount.Name + "-token-", | ||
Namespace: serviceAccount.Namespace, | ||
Name: sa + "-long-lived-token", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do something like, that would make it even more obvious and specific imo.
Name: sa + "-long-lived-token", | |
Name: sa + ".argocd.token", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about that, IMO the SA and the token are an unit, and I the user change the name of the service account (default "argocd-manager") I don't think we should override that decision in the token. OTOH if it's worththile to add a suffix / prefix, I think the SA name should be the place to do that.
Wdyt ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you see my suggested change I meant to add that as a suffix to the sa and not just that as the secret name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've seen that. What I mean is that the relation to argocd is already encoded into the service account name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The location of the string has changed (I pulled it in a constant to avoid typo), but the discussion is still relevant
~~AFAICT the missing tests are about assumptions about the service account
token in the tests, I'll adapt those ASAP.~~
|
616a6fe
to
d94dc7e
Compare
It looks like the fake.NewClientset does not update secrets annotated with the serviceaccount annotation with a token. Which is probably coherent with it's documentation. |
9308949
to
548c42a
Compare
The -race failure appears unrelated, AFAICT (permissions errors in repository/ tests) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #19970 +/- ##
=========================================
Coverage ? 55.79%
=========================================
Files ? 320
Lines ? 44364
Branches ? 0
=========================================
Hits ? 24754
Misses ? 17048
Partials ? 2562 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Max Gautier <[email protected]>
Referencing the secrets in the `secrets` fields of the ServiceAccount will make kubernetes consider it an auto-generated secret, and: 1. Warn about its usage in API responses 2. Subject it to automatic cleanup after roughly one year if unused See: - https://kubernetes.io/docs/reference/access-authn-authz/service-accounts-admin/#legacy-serviceaccount-token-cleaner - https://kubernetes.io/docs/reference/access-authn-authz/service-accounts-admin/#create-token Signed-off-by: Max Gautier <[email protected]>
Kubernetes legacy ServiceAccount tokens will generate warnings on use and be garbage collected if not used for one year, starting with Kubernetes 1.30. Use an explicitly created token secret with a fixed name, based on the service account name. Signed-off-by: Max Gautier <[email protected]>
This align clusterauth_test with the expected behavior modified by the previous commit, and is more in line with the way Kubernetes service account secrets are handled by the token controller. Signed-off-by: Max Gautier <[email protected]>
c14c48d
to
4dbab57
Compare
Test all 3 cases: - token secret already exists -> we should not create another - token secret does not exists - error on secret creation. Since getOrCreateServiceAccountSecretToken does not actually look at the content of the token itself, we remove the part of the test related to this. Signed-off-by: Max Gautier <[email protected]>
4dbab57
to
c2634ec
Compare
Is there something I'm supposed to do related to the license issue ? Apparently seeing the details is restricted to maintainers
|
Argocd should use explicitly created long-lived token, not the automatic ones created when a service account is created (on cluster with kubernetes version < 1.24).
Furthermore, it should not add itself the secrets it creates to the
secrets
array on the service account, because this makes kubernetes think this is an automated secret.See : https://kubernetes.io/docs/reference/access-authn-authz/service-accounts-admin/#create-token
Fixes #16859
Checklist: