kubeadm: decouple the bootstraptoken API from the kubeadm API #102964

neolit123 · 2021-06-17T21:43:23Z

What this PR does / why we need it:

Since kubeadm started using bootstrap tokens for bootstrap, it included a wrapper API (BoostrapToken and BoostrapTokenString) + some utilities to convert to and from a v1.Secret.

This API + utilities were duplicated in both the public main kubeadm API (versioned) and the internal API used for conversion.
This is completely not needed if the same API + utilities are extracted to their own group, so that kubeadm can use this both externally and internally the same way it is using metav1 or corev1.

Decouple the API + utilities into an new API group called bootstraptoken
Include a public version v1, since this mechanism has been used in a GA workflow for 10+ releases.
- Conversion is not needed for the time being since this is the only version. The rest of kubeadm can just use the public type for the time being the same way we are using a v1.Pod.
Do not remove the same API + utilities from v1beta2 since there is binding with the "output" API. Problem should be resolved when we remove v1beta2:
- api: output/v1alpha1 depends on kubeadm/v1beta2 kubeadm#2427
- deprecate and remove the kubeadm v1beta2 API kubeadm#2459

Which issue(s) this PR fixes:

xref kubernetes/kubeadm#2427
xref kubernetes/kubeadm#1796

Special notes for your reviewer:

better to review one commit at a time.

Does this PR introduce a user-facing change?

kubeadm: move the BootstrapToken* API and related utilities from v1beta3 to a separate API group/version - bootstraptoken/v1.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

neolit123 · 2021-06-17T21:49:02Z

/kind cleanup
/priority important-soon
/triage accepted

/cc @fabriziopandini

neolit123 · 2021-06-17T21:59:57Z

/hold for review

neolit123 · 2021-06-17T22:03:43Z

cmd/kubeadm/app/apis/bootstraptoken/v1/utils.go

@@ -0,0 +1,212 @@
+/*


these and the tests in utils_test.go are copied from the versioned / internal bootstrap* files.

neolit123 · 2021-06-17T22:05:22Z

cmd/kubeadm/app/apis/bootstraptoken/v1/zz_generated.deepcopy.go

+)
+
+// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
+func (in *BootstrapToken) DeepCopyInto(out *BootstrapToken) {


DeepCopyInto is needed because the BootstrapToken type is used in conversion in the "kubeadm" group.

SataQiu

/lgtm

SataQiu · 2021-06-24T04:35:38Z

/retest

neolit123 · 2021-06-29T14:21:42Z

/cc @SataQiu @pacoxu

i think @fabriziopandini might be on PTO until next week. the 8th of July is code freeze and we should get this merged before then. do you have any more comments here?

pacoxu · 2021-07-01T04:00:28Z

cmd/kubeadm/app/apis/bootstraptoken/v1/utils.go

+// and internal usage. It also automatically validates that the given token
+// is of the right format
+func NewBootstrapTokenString(token string) (*BootstrapTokenString, error) {
+	substrs := bootstraputil.BootstrapTokenRegexp.FindStringSubmatch(token)


Overall LGTM
/lgtm

When I go thought the code, I find IsValidBootstrapToken , BootstrapTokenRegexp are only used in /cmd/

Not sure if we need to move Regexp and isVaild func in kubernetes/staging/src/k8s.io/cluster-bootstrap/token/util/helpers.go to this utils.

And there is an old comment:

kubernetes/staging/src/k8s.io/cluster-bootstrap/token/util/helpers.go

Line 30 in ea07644

// TODO(dixudx): refactor this to util/secrets and util/tokens

@dixudx any ideas?

originally we wanted the kubeadm bootstraptoken utils to live in k8s.io/cluster-bootstrap, but the idea was rejected with the argument that these introduce wrapper APIs that are kubeadm specific. only the bootstrap token Secret format matters as a specification and not the kubeadm originated BootstrapTokenString.

so what we ended up having is:

generic utils that can be used outside of kubeadm and residing in a library under k8s.io/cluster-bootstrap

kubeadm specific code in /cmd/kubeadm/...api including the one responsible converting token data <-> secret

overall what is in k8s.io/cluster-bootstrap is a library API at this point and moving it can be a breaking change to users, so we rather not do it.

things like BootstrapTokenRegexp and IsValidBootstrapToken makes sense to be in k8s.io/cluster-bootstrap. if someone wishes to use bootstrap tokens with their own bootstrap mechanism and without kubeadm.
the kubeadm "token data <-> secret" and other utils are quite useful but were not accepted in k8s.io/cluster-bootstrap.

// TODO(dixudx): refactor this to util/secrets and util/tokens

this TODO seems still relevant as something that can be done in /staging/... /cluster-bootstrap, e.g. having to separate files tokens.go and secrets.go. i don't remember the discussions about it.

k8s-ci-robot · 2021-07-01T04:01:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, pacoxu

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:

~~cmd/kubeadm/OWNERS~~ [neolit123]

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

neolit123 · 2021-07-01T20:01:47Z

i will rebase this later today.

Package bootstraptoken contains an API and utilities wrapping the "bootstrap.kubernetes.io/token" Secret type to ease its usage in kubeadm. The API is released as v1, since these utilities have been part of a GA workflow for 10+ releases. The "bootstrap.kubernetes.io/token" Secret type is also GA.

- Make v1beta3 use bootstraptoken/v1 instead of local copies - Make the internal API use bootstraptoken/v1 - Update validation, /cmd, /util and other packages - Update v1beta2 conversion

Given bootstraptoken/v1 is now a separate GV, there is no need to duplicate the API and utilities inside v1beta3 and the internal version. v1beta2 must continue to use its internal copy due, since output/v1alpha1 embeds the v1beta2.BootstrapToken object. See issue 2427 in k/kubeadm.

neolit123 · 2021-07-01T21:13:38Z

@pacoxu rebased; conflict was in v1beta3/doc.go

pacoxu · 2021-07-02T05:45:40Z

/lgtm
/hold cancel
since there are no objections

pacoxu · 2021-07-02T09:11:52Z

/retest

fejta-bot · 2021-07-02T13:10:12Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot requested review from fabriziopandini and SataQiu June 17, 2021 21:43

neolit123 force-pushed the 1.22-decouple-bootstraptoken-api branch from 8c97641 to 6363000 Compare June 17, 2021 21:48

This was referenced Jun 17, 2021

api: output/v1alpha1 depends on kubeadm/v1beta2 kubernetes/kubeadm#2427

Closed

v1beta3 config kubernetes/kubeadm#1796

Closed

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 17, 2021

neolit123 commented Jun 17, 2021

View reviewed changes

neolit123 force-pushed the 1.22-decouple-bootstraptoken-api branch 3 times, most recently from 28b7eac to cb41d8f Compare June 17, 2021 22:11

SataQiu reviewed Jun 24, 2021

View reviewed changes

k8s-ci-robot assigned SataQiu Jun 24, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2021

k8s-ci-robot requested review from pacoxu and SataQiu June 29, 2021 14:21

pacoxu reviewed Jul 1, 2021

View reviewed changes

k8s-ci-robot assigned pacoxu Jul 1, 2021

pacoxu approved these changes Jul 1, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2021

neolit123 added 4 commits July 2, 2021 00:11

kubeadm: use the bootstraptoken/v1 API across the code base

14fa296

- Make v1beta3 use bootstraptoken/v1 instead of local copies - Make the internal API use bootstraptoken/v1 - Update validation, /cmd, /util and other packages - Update v1beta2 conversion

kubeadm: update v1beta3's godoc changelog

622f69b

neolit123 force-pushed the 1.22-decouple-bootstraptoken-api branch from 41be23d to 622f69b Compare July 1, 2021 21:12

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 1, 2021

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 2, 2021

k8s-ci-robot merged commit ce3bf86 into kubernetes:master Jul 2, 2021

k8s-ci-robot added this to the v1.22 milestone Jul 2, 2021

fabriziopandini mentioned this pull request Jul 4, 2021

Consider how to decouple the bootstraptoken API from the kubeadm API kubernetes-sigs/cluster-api#4875

Closed

everpeace mentioned this pull request Jul 7, 2021

Fix BootStrapToken API link kubernetes-sigs/lwkd#106

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: decouple the bootstraptoken API from the kubeadm API #102964

kubeadm: decouple the bootstraptoken API from the kubeadm API #102964

neolit123 commented Jun 17, 2021 •

edited

Loading

neolit123 commented Jun 17, 2021

neolit123 commented Jun 17, 2021

neolit123 Jun 17, 2021

neolit123 Jun 17, 2021

SataQiu left a comment

SataQiu commented Jun 24, 2021

neolit123 commented Jun 29, 2021

pacoxu Jul 1, 2021 •

edited

Loading

pacoxu Jul 1, 2021

neolit123 Jul 1, 2021 •

edited

Loading

neolit123 Jul 1, 2021

k8s-ci-robot commented Jul 1, 2021

neolit123 commented Jul 1, 2021

neolit123 commented Jul 1, 2021

pacoxu commented Jul 2, 2021

pacoxu commented Jul 2, 2021

fejta-bot commented Jul 2, 2021

kubeadm: decouple the bootstraptoken API from the kubeadm API #102964

kubeadm: decouple the bootstraptoken API from the kubeadm API #102964

Conversation

neolit123 commented Jun 17, 2021 • edited Loading

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

neolit123 commented Jun 17, 2021

neolit123 commented Jun 17, 2021

neolit123 Jun 17, 2021

Choose a reason for hiding this comment

neolit123 Jun 17, 2021

Choose a reason for hiding this comment

SataQiu left a comment

Choose a reason for hiding this comment

SataQiu commented Jun 24, 2021

neolit123 commented Jun 29, 2021

pacoxu Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

pacoxu Jul 1, 2021

Choose a reason for hiding this comment

neolit123 Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

neolit123 Jul 1, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 1, 2021

neolit123 commented Jul 1, 2021

neolit123 commented Jul 1, 2021

pacoxu commented Jul 2, 2021

pacoxu commented Jul 2, 2021

fejta-bot commented Jul 2, 2021

neolit123 commented Jun 17, 2021 •

edited

Loading

pacoxu Jul 1, 2021 •

edited

Loading

neolit123 Jul 1, 2021 •

edited

Loading