KubeadmConfig controller should not rely on previously set status

[fabriziopandini]

What did you expect to happen:
As per comment #3122 (comment) and #3125 (comment)

The KubeadmConfig controller implements a subset of reconcile logic in the main reconcile loop

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

Lines 198 to 242 in 7b97621

	switch {
	// Wait for the infrastructure to be ready.
	case !cluster.Status.InfrastructureReady:
	log.Info("Cluster infrastructure is not ready, waiting")
	return ctrl.Result{}, nil
	// Migrate plaintext data to secret.
	case config.Status.BootstrapData != nil && config.Status.DataSecretName == nil:
	if err := r.storeBootstrapData(ctx, scope, config.Status.BootstrapData); err != nil {
	return ctrl.Result{}, err
	}
	return ctrl.Result{}, patchHelper.Patch(ctx, config)
	// Reconcile status for machines that already have a secret reference, but our status isn't up to date.
	// This case solves the pivoting scenario (or a backup restore) which doesn't preserve the status subresource on objects.
	case configOwner.DataSecretName() != nil && (!config.Status.Ready || config.Status.DataSecretName == nil):
	config.Status.Ready = true
	config.Status.DataSecretName = configOwner.DataSecretName()
	return ctrl.Result{}, patchHelper.Patch(ctx, config)
	// Status is ready means a config has been generated.
	case config.Status.Ready:
	// If the BootstrapToken has been generated for a join and the infrastructure is not ready.
	// This indicates the token in the join config has not been consumed and it may need a refresh.
	if (config.Spec.JoinConfiguration != nil && config.Spec.JoinConfiguration.Discovery.BootstrapToken != nil) && !configOwner.IsInfrastructureReady() {
	token := config.Spec.JoinConfiguration.Discovery.BootstrapToken.Token
	remoteClient, err := r.remoteClientGetter(ctx, r.Client, util.ObjectKey(cluster), r.scheme)
	if err != nil {
	log.Error(err, "error creating remote cluster client")
	return ctrl.Result{}, err
	}
	log.Info("refreshing token until the infrastructure has a chance to consume it")
	err = refreshToken(remoteClient, token)
	if err != nil {
	// It would be nice to re-create the bootstrap token if the error was "not found", but we have no way to update the Machine's bootstrap data
	return ctrl.Result{}, errors.Wrapf(err, "failed to refresh bootstrap token")
	}
	// NB: this may not be sufficient to keep the token live if we don't see it before it expires, but when we generate a config we will set the status to "ready" which should generate an update event
	return ctrl.Result{
	RequeueAfter: DefaultTokenTTL / 2,
	}, nil
	}
	// In any other case just return as the config is already generated and need not be generated again.
	return ctrl.Result{}, nil
	}

We should move this to separated reconcile func, and make sure the logic does not rely on previously set status. Instead, it should check for the real state of things. E.g. Instead of checking status for ensuring the secret was created in a previous reconciliation loop, we should check if the secret actually exists.

/kind bug
/area bootstrap

[fabriziopandini]

Nb. if we implement lookup for existing secrets, this logic potentially can be removed

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

Lines 801 to 803 in 7b97621

	if !apierrors.IsAlreadyExists(err) {
	return errors.Wrapf(err, "failed to create bootstrap data secret for KubeadmConfig %s/%s", scope.Config.Namespace, scope.Config.Name)
	}

[vincepri]

/priority backlog
/milestone v0.3.x

[vincepri]

/kind cleanup
/milestone v0.4.0

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fabriziopandini]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[vincepri]

/lifecycle frozen

[CecileRobertMichon]

/help

[k8s-ci-robot]

@CecileRobertMichon:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

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/test-infra repository.

[vincepri]

/milestone v1.0

[sbueringer]

/assign @fabriziopandini

[fabriziopandini]

/unassign

[ramineni]

/assign

[killianmuldoon]

@ramineni Have you had time to work on this?

[ramineni]

@killianmuldoon sorry for the delay . Got time to get back on this.

Could you help me understand is this issue still relevant now? I see code is changed around

L204: case config.Status.BootstrapData != nil && config.Status.DataSecretName == nil:

this doesn't exist in reconcile loop anymore,

Nb. if we implement lookup for existing secrets, this logic potentially can be removed

Is this the pending item?

Thanks,

[fabriziopandini]

/triage accepted

[k8s-triage-robot]

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[fabriziopandini]

The Cluster API project currently lacks enough active contributors to adequately respond to all issues and PRs.

This is more a code smell than something that creates real issues, and the issue is mostly inactive since sept 2022
/Close

[k8s-ci-robot]

@fabriziopandini: Closing this issue.

In response to this:

The Cluster API project currently lacks enough active contributors to adequately respond to all issues and PRs.

This is more a code small than something that creates real issues, and the issue is mostly inactive since sept 2022
/Close

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/test-infra repository.