Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

fix: Use specific service account token secret for join #1515

Merged
merged 3 commits into from
Aug 8, 2022
Merged

fix: Use specific service account token secret for join #1515

merged 3 commits into from
Aug 8, 2022

Conversation

jimmidyson
Copy link
Contributor

@jimmidyson jimmidyson commented Aug 5, 2022 •
edited
Loading

With LegacyServiceAccountTokenNoAutoGeneration feature graduating to beta in
k8s v1.24, a token is not automatically generated when a service account is created.
This commit fixes this by requesting a specific service token account secret which
is populated by the token controller. This is backwards compatible with previous
versions of Kubernetes.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 5, 2022
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2022
@jimmidyson jimmidyson requested a review from makkes August 5, 2022 20:36
makkes
makkes suggested changes Aug 8, 2022
View reviewed changes
Copy link
Contributor

@makkes makkes left a comment

Choose a reason for hiding this comment

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

It feels like the PR is doing more than what the title implies in that it generally aligns kubefed with K8s 1.24.

I'd prefer for the PR title and description to clarify what this is about.

Otherwise lgtm except some smaller remarks.

pkg/kubefedctl/join.go Outdated Show resolved Hide resolved
pkg/kubefedctl/util/util.go Outdated Show resolved Hide resolved
pkg/kubefedctl/join.go Outdated Show resolved Hide resolved
pkg/kubefedctl/join.go Outdated Show resolved Hide resolved
@jimmidyson
Copy link
Contributor Author

Thanks for the review @makkes!

It feels like the PR is doing more than what the title implies in that it generally aligns kubefed with K8s 1.24.

You're right, I did that to test, but I will separate that out into a different PR and instead try just enabling the LegacyServiceAccountTokenNoAutoGeneration feature gate in KinD tests to ensure that the functionality works and we don't get false positives in our integration tests. Does that sound OK to you?

@jimmidyson
Copy link
Contributor Author

Ah I can't enable LegacyServiceAccountTokenNoAutoGeneration in previous versions because it was introduced in v1.24 in beta and therefore enabled by default. I suppose what I could do is still take out the 1.24 upgrade stuff in this PR and run with existing tests to check there's no regression - that's what I'll do now.

@makkes
Copy link
Contributor

makkes commented Aug 8, 2022

Ah I can't enable LegacyServiceAccountTokenNoAutoGeneration in previous versions because it was introduced in v1.24 in beta and therefore enabled by default. I suppose what I could do is still take out the 1.24 upgrade stuff in this PR and run with existing tests to check there's no regression - that's what I'll do now.

That sounds perfect. Thanks so much!

@jimmidyson
fix: Use specific service account token secret
02afe88 
With `LegacyServiceAccountTokenNoAutoGeneration` feature graduating to beta in
k8s v1.24, a token is not automatically generated when a service account is created.
This commit fixes this by requesting a specific service token account secret which
is populated by the token controller. This is backwards compatible with previous
versions of Kubernetes.
@jimmidyson
refactor: More clearly define secret names in host and joining clusters
17d21fa
@jimmidyson
Copy link
Contributor Author

@makkes Done as discussed.

makkes
makkes reviewed Aug 8, 2022
View reviewed changes
Copy link
Contributor

@makkes makkes left a comment

Choose a reason for hiding this comment

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

lgtm except the doc change

pkg/kubefedctl/join.go Outdated Show resolved Hide resolved
@jimmidyson
Copy link
Contributor Author

@makkes Done for reals this time 😂

makkes
makkes approved these changes Aug 8, 2022
View reviewed changes
Copy link
Contributor

@makkes makkes left a comment

Choose a reason for hiding this comment

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

🥳 Thank you!!

@makkes
Copy link
Contributor

makkes commented Aug 8, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jimmidyson, makkes

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 7946858 into kubernetes-retired:master Aug 8, 2022
@jimmidyson jimmidyson deleted the join-secret-creation branch August 8, 2022 12:38
@ebiiim ebiiim mentioned this pull request Nov 24, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@makkes makkes makkes approved these changes

@font font Awaiting requested review from font

@gyliu513 gyliu513 Awaiting requested review from gyliu513

Assignees

@makkes makkes

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jimmidyson @makkes @k8s-ci-robot