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.

Sign up for GitHub

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

Jump to bottom

fix: Support Kubernetes v1.24. Fixes #8320 #9680

Closed
wants to merge 17 commits into from
Closed

fix: Support Kubernetes v1.24. Fixes #8320 #9680

wants to merge 17 commits into from

Conversation

terrytangyuan
Copy link
Member

Fixes #8320. On top of existing work in #9620.

Co-authored-by: Alex Collins [email protected]
Signed-off-by: Yuan Tang [email protected]
Signed-off-by: Alex Collins [email protected]

@terrytangyuan terrytangyuan mentioned this pull request Sep 25, 2022
Merged
@terrytangyuan terrytangyuan marked this pull request as draft September 25, 2022 00:57
jessesuen
jessesuen reviewed Sep 30, 2022
View reviewed changes
Copy link
Member

@jessesuen jessesuen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Will this change bump the workflow minimum k8s version to something like v1.22 (when the TokenRequestAPI was marked stable)?

EDIT: No, I think not since we look to the legacy location / behavior.

jessesuen
jessesuen reviewed Sep 30, 2022
View reviewed changes
workflow/controller/operator.go
Comment on lines -3690 to +3691
return "", fmt.Errorf("service account %s/%s does not have any secrets", account.Namespace, account.Name)
}
return account.Secrets[0].Name, nil
return secrets.SecretName(account), nil
Copy link
Member

@jessesuen jessesuen Sep 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think we are breaking backwards compatibility here.

Users of v1.23 and below, are not conditioned to generate ServiceAccount secrets yet and so having the assumption that there exists the secret by naming convention is going to break a lot of.

I think the implementation needs to have both implementations and preserve the older v1.23 assumed behavior for backward compatibility

EDIT: Nevermind, I just saw the implementation of SecretName and see that it is doing just that.

jessesuen
jessesuen reviewed Sep 30, 2022
View reviewed changes
docs/manually-create-secrets.md
Comment on lines +3 to +5
As of Kubernetes v1.24, secrets are not automatically created for service accounts by default. [Find out how to create these yourself manually](https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#manually-create-a-service-account-api-token).

Argo discovers your token by name, not annotation. They must be named `${serviceAccountName}.service-account-token`.
Copy link
Member

Choose a reason for hiding this comment

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

I think we could do better job explaining when users need to do this. When using Agent http template? Auth delegation in api server? Does this need to be mentioned in the SSO docs?

jessesuen
jessesuen reviewed Sep 30, 2022
View reviewed changes
Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

I am wondering if we have considered alternative techniques that doesn't require user to create a ServiceAccount Secret as an added step for v1.24. For example, using the token request API?

@terrytangyuan
Copy link
Member Author

@alexec @sarabala1979 Any thoughts on Jesse's questions above? Is the token request API something you thought about when agent was initially designed/implemented?

@alexec
Copy link
Contributor

alexec commented Sep 30, 2022

This is meant to be a cheap option that basically keeps the old behaviour. I’m happy for someone to contribute a more involved fix.

@terrytangyuan
Copy link
Member Author

I'll investigate the approach that uses the TokenRequest API. It seems like a better option instead of using a token that never expires.

@sarabala1979
Copy link
Member

@terrytangyuan Mainly we need to think about the API calls increase. Will Controller call once to create a secret for SA or Will it call every reconcile time?

@terrytangyuan
Copy link
Member Author

@terrytangyuan Mainly we need to think about the API calls increase. Will Controller call once to create a secret for SA or Will it call every reconcile time?

We'll need to manage the lifecycle of tokens so that they can be rotated and re-requested only when necessary (e.g. based a configured timeout/expiration for bearer tokens) instead of making requests frequently.

@terrytangyuan terrytangyuan mentioned this pull request Oct 7, 2022
Closed
@terrytangyuan
Copy link
Member Author

Remaining issues:

@terrytangyuan terrytangyuan deleted the support-k8s-124 branch July 12, 2023 20:26
@agilgur5
Copy link
Contributor

For reference, #9620 was re-opened and completed after this was closed, so marking this as superseded

@agilgur5 agilgur5 added solution/superseded This PR or issue has been superseded by another one (slightly different from a duplicate) area/api Argo Server API area/server area/controller Controller issues, panics area/sso-rbac labels Jul 13, 2024
@argoproj argoproj locked as resolved and limited conversation to collaborators Jul 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jessesuen jessesuen jessesuen left review comments

Assignees
No one assigned
Labels
area/api Argo Server API area/controller Controller issues, panics area/server area/sso-rbac solution/superseded This PR or issue has been superseded by another one (slightly different from a duplicate)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Kubernetes 1.24: Token-handling code assumes auto-created service account token secrets
5 participants
@terrytangyuan @alexec @sarabala1979 @agilgur5 @jessesuen