🐛 Recreate bootstrap token if it was cleaned up #11520

AndiDog · 2024-12-02T14:59:30Z

What this PR does / why we need it:

If the join token was cleaned up by Kubernetes before CAPI was able to refresh its expiry, CAPI currently fails. This happened in a few cases for us, using machine pools, and required manual intervention to unset the token so that CAPI would recreate it. This change takes care to autoheal the situation by recreating the token.

Which issue(s) this PR fixes:

Seems to be the same issue as #11034.

/area bootstrap

k8s-ci-robot · 2024-12-02T14:59:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign neolit123 for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

g-gaston

looks good, just some nits

g-gaston · 2024-12-02T23:18:28Z

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

@@ -371,6 +371,11 @@ func (r *KubeadmConfigReconciler) refreshBootstrapTokenIfNeeded(ctx context.Cont

 	secret, err := getToken(ctx, remoteClient, token)
 	if err != nil {
+		if apierrors.IsNotFound(err) {
+			log.Error(err, "token secret is gone, triggering creation of new token")


Suggested change

log.Error(err, "token secret is gone, triggering creation of new token")

log.Error(err, "Bootstrap token secret is gone, triggering creation of new token")

Also, big nit, but should this really be an error log? Even if this not part of the normal flow, if we have a way to recover from it, should we surface it as an error? My concern is if this would be confusing from someone looking over the logs.

Agree, and done. I had initially made it an error since it's rare, but that's admittedly not a good reason.

g-gaston · 2024-12-02T23:20:54Z

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

 		return ctrl.Result{}, errors.Wrapf(err, "failed to refresh bootstrap token")
 	}
 	return ctrl.Result{
 		RequeueAfter: r.tokenCheckRefreshOrRotationInterval(),
 	}, nil
 }

+func (r *KubeadmConfigReconciler) createNewBootstrapToken(ctx context.Context, config *bootstrapv1.KubeadmConfig, scope *Scope, remoteClient client.Client) (ctrl.Result, error) {


Maybe? Given the message you log, looks like this method is not for the first token creation, but for when the token needs to be recreated.

Suggested change

func (r *KubeadmConfigReconciler) createNewBootstrapToken(ctx context.Context, config *bootstrapv1.KubeadmConfig, scope *Scope, remoteClient client.Client) (ctrl.Result, error) {

func (r *KubeadmConfigReconciler) recreateNewBootstrapToken(ctx context.Context, config *bootstrapv1.KubeadmConfig, scope *Scope, remoteClient client.Client) (ctrl.Result, error) {

Fair point. But, talking about language nits 😆, I'm removing "new" since that else duplicates the "re" prefix.

g-gaston · 2024-12-02T23:22:56Z

@AndiDog is this a dup of #11037 ?

fabriziopandini · 2024-12-03T09:31:24Z

Yes, it seems a dup of #11037, but it has the big advantage that this PR comes with unit tests.

It would be great if @AndiDog and @archerwu9425 can join forcces and come out with one PR preserving test coverage and addressing this comment (re-create behaviour should be limited to MP owned bootstrap tokens).

AndiDog · 2024-12-03T15:35:34Z

Sorry, I wasn't aware there's already a PR. Please feel free to choose where to continue with reviews. I covered all places where the secret could be seen as not found – the Get and the Update of the token.

AndiDog · 2024-12-03T15:52:33Z

I limited the new behavior to machine pools

fabriziopandini

Last two nits otherwise lgtm pending fix for linter errors

fabriziopandini · 2024-12-06T10:16:05Z

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

@@ -371,6 +371,11 @@ func (r *KubeadmConfigReconciler) refreshBootstrapTokenIfNeeded(ctx context.Cont

 	secret, err := getToken(ctx, remoteClient, token)
 	if err != nil {
+		if apierrors.IsNotFound(err) && scope.ConfigOwner.IsMachinePool() {
+			log.Info("Bootstrap token secret is gone, triggering creation of new token")


nit

Suggested change

log.Info("Bootstrap token secret is gone, triggering creation of new token")

log.Info("Bootstrap token secret is not found, triggering creation of new token")

same in L410

Might be better to move this log line and the following line blanking out the Token into recreateBootstrapToken

q: can we drop blanking out the boolstrap token entirely, given that we are setting it to a new value inside recreateBootstrapToken

Reworded.

Semantically, the function name recreateBootstrapToken doesn't denote that the secret wasn't found, so I'd rather leave the log line here where we determined that the secret wasn't found and therefore need to call the recreation functionality.

We could drop setting the field to "", but then if token creation fails, CAPI tries to read the non-existing secret again. If we set an empty string, the next reconciliation run will try to create the token. Your call here – I have no strong opinion which way to go.

fabriziopandini · 2024-12-06T10:38:18Z

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller_test.go

@@ -1441,6 +1441,154 @@ func TestBootstrapTokenRotationMachinePool(t *testing.T) {
 	g.Expect(foundNew).To(BeTrue())
 }

+func TestBootstrapTokenRefreshIfTokenSecretCleanedMachine(t *testing.T) {


Can we have a single test with two test run? (se we can have a better test description)

Suggested change

func TestBootstrapTokenRefreshIfTokenSecretCleanedMachine(t *testing.T) {

func TestBootstrapTokenRefresh(t *testing.T) {

t.Run("should not recreate the token for Machines", func(t *testing.T) {

...

})

t.Run("should recreate the token for MachinePools", func(t *testing.T)

...

})

}

k8s-ci-robot added area/bootstrap Issues or PRs related to bootstrap providers cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 2, 2024

k8s-ci-robot requested review from g-gaston and JoelSpeed December 2, 2024 14:59

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 2, 2024

g-gaston reviewed Dec 2, 2024

View reviewed changes

AndiDog force-pushed the recreate-bootstrap-token-if-cleaned branch from 5350450 to 8716e12 Compare December 3, 2024 15:33

AndiDog force-pushed the recreate-bootstrap-token-if-cleaned branch from 8716e12 to 9d22a92 Compare December 3, 2024 15:52

AndiDog mentioned this pull request Dec 3, 2024

🐛 create bootstrap token if not found in refresh process #11037

Closed

fabriziopandini reviewed Dec 6, 2024

View reviewed changes

Recreate bootstrap token if it was cleaned up

51a4011

AndiDog force-pushed the recreate-bootstrap-token-if-cleaned branch from 9d22a92 to 51a4011 Compare December 16, 2024 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Recreate bootstrap token if it was cleaned up #11520

🐛 Recreate bootstrap token if it was cleaned up #11520

AndiDog commented Dec 2, 2024

k8s-ci-robot commented Dec 2, 2024

g-gaston left a comment

g-gaston Dec 2, 2024

AndiDog Dec 3, 2024

g-gaston Dec 2, 2024

AndiDog Dec 3, 2024

g-gaston commented Dec 2, 2024

fabriziopandini commented Dec 3, 2024 •

edited

Loading

AndiDog commented Dec 3, 2024

AndiDog commented Dec 3, 2024

fabriziopandini left a comment

fabriziopandini Dec 6, 2024

fabriziopandini Dec 6, 2024

AndiDog Dec 16, 2024

fabriziopandini Dec 6, 2024 •

edited

Loading

AndiDog Dec 16, 2024

	log.Error(err, "token secret is gone, triggering creation of new token")
	log.Error(err, "Bootstrap token secret is gone, triggering creation of new token")

	func (r KubeadmConfigReconciler) createNewBootstrapToken(ctx context.Context, config bootstrapv1.KubeadmConfig, scope *Scope, remoteClient client.Client) (ctrl.Result, error) {
	func (r KubeadmConfigReconciler) recreateNewBootstrapToken(ctx context.Context, config bootstrapv1.KubeadmConfig, scope *Scope, remoteClient client.Client) (ctrl.Result, error) {

	log.Info("Bootstrap token secret is gone, triggering creation of new token")
	log.Info("Bootstrap token secret is not found, triggering creation of new token")

-func TestBootstrapTokenRefreshIfTokenSecretCleanedMachine(t *testing.T) {
+func TestBootstrapTokenRefresh(t *testing.T) {
+   t.Run("should not recreate the token for Machines", func(t *testing.T) {
+      ...
+   })
+   t.Run("should recreate the token for MachinePools", func(t *testing.T)
+      ...
+   })
+}

🐛 Recreate bootstrap token if it was cleaned up #11520

Are you sure you want to change the base?

🐛 Recreate bootstrap token if it was cleaned up #11520

Conversation

AndiDog commented Dec 2, 2024

k8s-ci-robot commented Dec 2, 2024

g-gaston left a 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

Choose a reason for hiding this comment

g-gaston commented Dec 2, 2024

fabriziopandini commented Dec 3, 2024 • edited Loading

AndiDog commented Dec 3, 2024

AndiDog commented Dec 3, 2024

fabriziopandini left a 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

fabriziopandini Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini commented Dec 3, 2024 •

edited

Loading

fabriziopandini Dec 6, 2024 •

edited

Loading