🐛 Make joinConfiguration.discovery.bootstrapToken.token optional #12107

Amulyam24 · 2025-04-17T08:21:44Z

What this PR does / why we need it:
In the cluster-api provider for IBM cloud, we set the API server endpoint explicitly in the join configuration. With the recent change via #11949, it has become mandatory to set the token value and cluster deployment fails with

KubeadmConfigTemplate.bootstrap.cluster.x-k8s.io "capibm-e2e-2nmy7v-md-0" is invalid: spec.template.spec.joinConfiguration.discovery.bootstrapToken.token: Invalid value: "": spec.template.spec.joinConfiguration.discovery.bootstrapToken.token in body should be at least 1 chars long,

This PR adds the change to not set a hard validation for token and use the one generated by bootstrap provider in case it is empty.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Amulyam24 · 2025-04-17T08:21:54Z

/cc @Karthik-K-N

Karthik-K-N · 2025-04-17T09:02:32Z

/cc @chrischdi @fabriziopandini

fabriziopandini · 2025-04-17T09:25:51Z

bootstrap/kubeadm/api/v1beta1/kubeadm_types.go

@@ -585,7 +585,7 @@ type BootstrapTokenDiscovery struct {
 	// token is a token used to validate cluster information
 	// fetched from the control-plane.
 	// +required
-	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:MinLength=0


I looked into #12019 that fixed a similar issue by making the field optional and adding omit empty, which seems a cleaner option to allow users to not set a field.

@JoelSpeed WDYT?

Also, Is this a non-breaking change? (I assume it is not breaking, but I would like to double check)

I looked into #12019 that fixed a similar issue by making the field optional and adding omit empty, which seems a cleaner option to allow users to not set a field.

Right, making it optional would be better, will wait for @JoelSpeed's response.

Also, Is this a non-breaking change? (I assume it is not breaking, but I would like to double check)

It should be a non breaking change.

Technically any change we would make here could be considered breaking.

Adding a min length of 0 to a required field is odd and I would avoid personally. A required field where the zero value is valid does not actually behave as a required field for those using structured clients. For those using unstructured clients, it's even weirder, they have to specify the key, and possibly then set the zero value.

The lesser of two evils here, if there needs to be a case where this field is empty, it to make the field optional with omitempty.

Who consumes this API? Do we understand what the consumers will do if they see an empty string here?

Thank you for the response, Token is being currently used in kubeadm_config controller and we have logic written to handle both the cases, that is to respect the value if set or generate one. Reference

cluster-api/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

Lines 1270 to 1284 in 3e6bc6a

// if BootstrapToken already contains a token, respect it; otherwise create a new bootstrap token for the node to join

if config.Spec.JoinConfiguration.Discovery.BootstrapToken.Token == "" {

remoteClient, err := r.ClusterCache.GetClient(ctx, util.ObjectKey(cluster))

if err != nil {

return ctrl.Result{}, err

}

token, err := createToken(ctx, remoteClient, r.TokenTTL)

if err != nil {

return ctrl.Result{}, errors.Wrapf(err, "failed to create new bootstrap token")

}

config.Spec.JoinConfiguration.Discovery.BootstrapToken.Token = token

log.V(3).Info("Altering JoinConfiguration.Discovery.BootstrapToken.Token")

}

As per suggestions, changed to field to optional. PTAL.

Amulyam24 · 2025-04-23T08:09:31Z

@fabriziopandini @JoelSpeed, PTAL if the change is okay?

JoelSpeed · 2025-04-23T10:06:59Z

bootstrap/kubeadm/api/v1beta1/kubeadm_types.go

@@ -584,10 +584,10 @@ type Discovery struct {
 type BootstrapTokenDiscovery struct {


This object now has no required field, so there will be difference in behaviour between not specifying the object, and specifying bootstrapToken: {}.

This is generally not desirable.

Is it valid to have an empty bootstrapToken? In the current use case, which alternative field would be used instead?

From code reading what I can see is that the both the scenario's are handled currently, If boostStrapToken is not set and its nil, its being initialized first.
Also we have necessary checks for respecting the already set values as well.
Reference:

cluster-api/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

Lines 1242 to 1284 in 3e6bc6a

// otherwise it is necessary to ensure token discovery is properly configured

if config.Spec.JoinConfiguration.Discovery.BootstrapToken == nil {

config.Spec.JoinConfiguration.Discovery.BootstrapToken = &bootstrapv1.BootstrapTokenDiscovery{}

}

// calculate the ca cert hashes if they are not already set

if len(config.Spec.JoinConfiguration.Discovery.BootstrapToken.CACertHashes) == 0 {

hashes, err := certificates.GetByPurpose(secret.ClusterCA).Hashes()

if err != nil {

log.Error(err, "Unable to generate Cluster CA certificate hashes")

return ctrl.Result{}, err

}

config.Spec.JoinConfiguration.Discovery.BootstrapToken.CACertHashes = hashes

}

// if BootstrapToken already contains an APIServerEndpoint, respect it; otherwise inject the APIServerEndpoint endpoint defined in cluster status

apiServerEndpoint := config.Spec.JoinConfiguration.Discovery.BootstrapToken.APIServerEndpoint

if apiServerEndpoint == "" {

if !cluster.Spec.ControlPlaneEndpoint.IsValid() {

log.V(1).Info("Waiting for Cluster Controller to set Cluster.Spec.ControlPlaneEndpoint")

return ctrl.Result{RequeueAfter: 10 * time.Second}, nil

}

apiServerEndpoint = cluster.Spec.ControlPlaneEndpoint.String()

config.Spec.JoinConfiguration.Discovery.BootstrapToken.APIServerEndpoint = apiServerEndpoint

log.V(3).Info("Altering JoinConfiguration.Discovery.BootstrapToken.APIServerEndpoint", "apiServerEndpoint", apiServerEndpoint)

}

// if BootstrapToken already contains a token, respect it; otherwise create a new bootstrap token for the node to join

if config.Spec.JoinConfiguration.Discovery.BootstrapToken.Token == "" {

remoteClient, err := r.ClusterCache.GetClient(ctx, util.ObjectKey(cluster))

if err != nil {

return ctrl.Result{}, err

}

token, err := createToken(ctx, remoteClient, r.TokenTTL)

if err != nil {

return ctrl.Result{}, errors.Wrapf(err, "failed to create new bootstrap token")

}

config.Spec.JoinConfiguration.Discovery.BootstrapToken.Token = token

log.V(3).Info("Altering JoinConfiguration.Discovery.BootstrapToken.Token")

}

Please correct If I missed anything @chrischdi @fabriziopandini

I think we are ok

Discovery has two mutually exclusive options, FileDiscovery and TokenDiscovery.

If File discovery is set, TokenDiscovery must not be set
If File discovery is not set, TokenDiscovery is used, either with the values specified by the users or, if those value are missing, with defaults.

Considering this, there wont be any difference in behaviour between not specifying the object, and specifying empty bootstrapToken: in both cases you accept all the defaults for TokenDiscovery

fabriziopandini · 2025-04-28T16:12:57Z

I'm merging to get this fix in tomorrow's patch release
/lgtm
/approve

k8s-ci-robot · 2025-04-28T16:13:04Z

LGTM label has been added.

Git tree hash: 1732d327cb8bccf3fed135e106cbf19d668447cf

k8s-ci-robot · 2025-04-28T16:13:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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:

~~OWNERS~~ [fabriziopandini]

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

mboersma · 2025-04-28T19:18:13Z

/area bootstrap

fabriziopandini · 2025-04-28T23:06:49Z

/cherry-pick release-1.10

k8s-infra-cherrypick-robot · 2025-04-28T23:07:38Z

@fabriziopandini: #12107 failed to apply on top of branch "release-1.10":

Applying: Make joinConfiguration.discovery.bootstrapToken.token optional
Using index info to reconstruct a base tree...
A	bootstrap/kubeadm/api/v1beta2/kubeadm_types.go
M	bootstrap/kubeadm/config/crd/bases/bootstrap.cluster.x-k8s.io_kubeadmconfigs.yaml
M	bootstrap/kubeadm/config/crd/bases/bootstrap.cluster.x-k8s.io_kubeadmconfigtemplates.yaml
M	controlplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanes.yaml
M	controlplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanetemplates.yaml
Falling back to patching base and 3-way merge...
Auto-merging internal/apis/bootstrap/kubeadm/v1alpha4/kubeadm_types.go
CONFLICT (content): Merge conflict in internal/apis/bootstrap/kubeadm/v1alpha4/kubeadm_types.go
Auto-merging controlplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanetemplates.yaml
CONFLICT (content): Merge conflict in controlplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanetemplates.yaml
Auto-merging controlplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanes.yaml
CONFLICT (content): Merge conflict in controlplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanes.yaml
Auto-merging bootstrap/kubeadm/config/crd/bases/bootstrap.cluster.x-k8s.io_kubeadmconfigtemplates.yaml
CONFLICT (content): Merge conflict in bootstrap/kubeadm/config/crd/bases/bootstrap.cluster.x-k8s.io_kubeadmconfigtemplates.yaml
Auto-merging bootstrap/kubeadm/config/crd/bases/bootstrap.cluster.x-k8s.io_kubeadmconfigs.yaml
CONFLICT (content): Merge conflict in bootstrap/kubeadm/config/crd/bases/bootstrap.cluster.x-k8s.io_kubeadmconfigs.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Make joinConfiguration.discovery.bootstrapToken.token optional

In response to this:

/cherry-pick release-1.10

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

fabriziopandini · 2025-04-28T23:12:38Z

@Amulyam24 @Karthik-K-N looks like it is required a manual cherry pick for the 1.10 branch

🐛 Make joinConfiguration.discovery.bootstrapToken.token optional

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 17, 2025

k8s-ci-robot added the do-not-merge/needs-area PR is missing an area label label Apr 17, 2025

k8s-ci-robot requested review from g-gaston and jackfrancis April 17, 2025 08:21

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 17, 2025

k8s-ci-robot requested a review from Karthik-K-N April 17, 2025 08:21

Amulyam24 changed the title ~~Set min length validation to 0 for Bootstrap token~~ 🐛 Set min length validation to 0 for Bootstrap token Apr 17, 2025

Amulyam24 force-pushed the token branch from dd0d609 to 1809dc0 Compare April 17, 2025 08:36

k8s-ci-robot requested review from chrischdi and fabriziopandini April 17, 2025 09:02

fabriziopandini reviewed Apr 17, 2025

View reviewed changes

Amulyam24 force-pushed the token branch 2 times, most recently from ec3058e to 2426f1d Compare April 21, 2025 05:22

Amulyam24 changed the title ~~🐛 Set min length validation to 0 for Bootstrap token~~ 🐛 Make joinConfiguration.discovery.bootstrapToken.token optional Apr 21, 2025

Amulyam24 force-pushed the token branch 2 times, most recently from c1f3b94 to fadadca Compare April 21, 2025 06:18

Make joinConfiguration.discovery.bootstrapToken.token optional

d42875c

Amulyam24 force-pushed the token branch from fadadca to d42875c Compare April 21, 2025 06:21

JoelSpeed reviewed Apr 23, 2025

View reviewed changes

k8s-ci-robot assigned fabriziopandini Apr 28, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 28, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2025

k8s-ci-robot added the area/bootstrap Issues or PRs related to bootstrap providers label Apr 28, 2025

k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Apr 28, 2025

k8s-ci-robot merged commit 8f348b6 into kubernetes-sigs:main Apr 28, 2025
19 checks passed

k8s-ci-robot added this to the v1.11 milestone Apr 28, 2025

mboersma mentioned this pull request Apr 28, 2025

MachinePool dataSecretName can't be an empty string. kubernetes-sigs/cluster-api-provider-azure#5589

Open

Amulyam24 pushed a commit to Amulyam24/cluster-api that referenced this pull request Apr 29, 2025

Merge pull request kubernetes-sigs#12107 from Amulyam24/token

33e6b89

🐛 Make joinConfiguration.discovery.bootstrapToken.token optional

Amulyam24 mentioned this pull request Apr 29, 2025

[release-1.10] 🐛 Make joinConfiguration.discovery.bootstrapToken.token optional #12136

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Make joinConfiguration.discovery.bootstrapToken.token optional #12107

🐛 Make joinConfiguration.discovery.bootstrapToken.token optional #12107

Amulyam24 commented Apr 17, 2025 •

edited

Loading

Amulyam24 commented Apr 17, 2025

Karthik-K-N commented Apr 17, 2025

fabriziopandini Apr 17, 2025

Amulyam24 Apr 17, 2025

JoelSpeed Apr 17, 2025

Karthik-K-N Apr 17, 2025

Amulyam24 Apr 21, 2025

Amulyam24 commented Apr 23, 2025

JoelSpeed Apr 23, 2025

Karthik-K-N Apr 28, 2025

fabriziopandini Apr 28, 2025

fabriziopandini commented Apr 28, 2025 •

edited

Loading

k8s-ci-robot commented Apr 28, 2025

k8s-ci-robot commented Apr 28, 2025

mboersma commented Apr 28, 2025

fabriziopandini commented Apr 28, 2025

k8s-infra-cherrypick-robot commented Apr 28, 2025

fabriziopandini commented Apr 28, 2025

	// if BootstrapToken already contains a token, respect it; otherwise create a new bootstrap token for the node to join
	if config.Spec.JoinConfiguration.Discovery.BootstrapToken.Token == "" {
	remoteClient, err := r.ClusterCache.GetClient(ctx, util.ObjectKey(cluster))
	if err != nil {
	return ctrl.Result{}, err
	}

	token, err := createToken(ctx, remoteClient, r.TokenTTL)
	if err != nil {
	return ctrl.Result{}, errors.Wrapf(err, "failed to create new bootstrap token")
	}

	config.Spec.JoinConfiguration.Discovery.BootstrapToken.Token = token
	log.V(3).Info("Altering JoinConfiguration.Discovery.BootstrapToken.Token")
	}

		@@ -584,10 +584,10 @@ type Discovery struct {
		type BootstrapTokenDiscovery struct {

	// otherwise it is necessary to ensure token discovery is properly configured
	if config.Spec.JoinConfiguration.Discovery.BootstrapToken == nil {
	config.Spec.JoinConfiguration.Discovery.BootstrapToken = &bootstrapv1.BootstrapTokenDiscovery{}
	}

	// calculate the ca cert hashes if they are not already set
	if len(config.Spec.JoinConfiguration.Discovery.BootstrapToken.CACertHashes) == 0 {
	hashes, err := certificates.GetByPurpose(secret.ClusterCA).Hashes()
	if err != nil {
	log.Error(err, "Unable to generate Cluster CA certificate hashes")
	return ctrl.Result{}, err
	}
	config.Spec.JoinConfiguration.Discovery.BootstrapToken.CACertHashes = hashes
	}

	// if BootstrapToken already contains an APIServerEndpoint, respect it; otherwise inject the APIServerEndpoint endpoint defined in cluster status
	apiServerEndpoint := config.Spec.JoinConfiguration.Discovery.BootstrapToken.APIServerEndpoint
	if apiServerEndpoint == "" {
	if !cluster.Spec.ControlPlaneEndpoint.IsValid() {
	log.V(1).Info("Waiting for Cluster Controller to set Cluster.Spec.ControlPlaneEndpoint")
	return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
	}

	apiServerEndpoint = cluster.Spec.ControlPlaneEndpoint.String()
	config.Spec.JoinConfiguration.Discovery.BootstrapToken.APIServerEndpoint = apiServerEndpoint
	log.V(3).Info("Altering JoinConfiguration.Discovery.BootstrapToken.APIServerEndpoint", "apiServerEndpoint", apiServerEndpoint)
	}

	// if BootstrapToken already contains a token, respect it; otherwise create a new bootstrap token for the node to join
	if config.Spec.JoinConfiguration.Discovery.BootstrapToken.Token == "" {
	remoteClient, err := r.ClusterCache.GetClient(ctx, util.ObjectKey(cluster))
	if err != nil {
	return ctrl.Result{}, err
	}

	token, err := createToken(ctx, remoteClient, r.TokenTTL)
	if err != nil {
	return ctrl.Result{}, errors.Wrapf(err, "failed to create new bootstrap token")
	}

	config.Spec.JoinConfiguration.Discovery.BootstrapToken.Token = token
	log.V(3).Info("Altering JoinConfiguration.Discovery.BootstrapToken.Token")
	}

🐛 Make joinConfiguration.discovery.bootstrapToken.token optional #12107

🐛 Make joinConfiguration.discovery.bootstrapToken.token optional #12107

Conversation

Amulyam24 commented Apr 17, 2025 • edited Loading

Amulyam24 commented Apr 17, 2025

Karthik-K-N commented Apr 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amulyam24 commented Apr 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini commented Apr 28, 2025 • edited Loading

k8s-ci-robot commented Apr 28, 2025

k8s-ci-robot commented Apr 28, 2025

mboersma commented Apr 28, 2025

fabriziopandini commented Apr 28, 2025

k8s-infra-cherrypick-robot commented Apr 28, 2025

fabriziopandini commented Apr 28, 2025

Amulyam24 commented Apr 17, 2025 •

edited

Loading

fabriziopandini commented Apr 28, 2025 •

edited

Loading