refactor: use switch/case for early returns only

Also adds a requeueAfter parameter, because I realized doing this work
that we should get an immediate second crack at the config before it's
consumed.

Author: sethp-nr

controllers/kubeadmconfig_controller.go

@@ -112,18 +112,6 @@ func (r *KubeadmConfigReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, re
 	}
 	log = log.WithValues("machine-name", machine.Name)
 
-	// bail super early if it's already ready
-	if config.Status.Ready && machine.Status.InfrastructureReady {
-		log.Info("ignoring config for an already ready machine")
-		return ctrl.Result{}, nil
-	}
-
-	// Ignore machines that already have bootstrap data we didn't generate
-	if machine.Spec.Bootstrap.Data != nil && !config.Status.Ready {
-		// TODO: mark the config as ready?
-		return ctrl.Result{}, nil
-	}
-
 	// Lookup the cluster the machine is associated with
 	cluster, err := util.GetClusterFromMetadata(ctx, r.Client, machine.ObjectMeta)
 	if err != nil {
@@ -140,21 +128,19 @@ func (r *KubeadmConfigReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, re
 		return ctrl.Result{}, err
 	}
 
-	// Initialize the patch helper
-	patchHelper, err := patch.NewHelper(config, r)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-	// Attempt to Patch the KubeadmConfig object and status after each reconciliation if no error occurs.
-	defer func() {
-		if rerr == nil {
-			if rerr = patchHelper.Patch(ctx, config); rerr != nil {
-				log.Error(rerr, "failed to patch config")
-			}
-		}
-	}()
-
 	switch {
+	// Wait patiently for the infrastructure to be ready
+	case !cluster.Status.InfrastructureReady:
+		log.Info("Infrastructure is not ready, waiting until ready.")
+		return ctrl.Result{}, nil
+	// bail super early if it's already ready
+	case config.Status.Ready && machine.Status.InfrastructureReady:
+		log.Info("ignoring config for an already ready machine")
+		return ctrl.Result{}, nil
+	// Ignore machines that already have bootstrap data we didn't generate
+	case machine.Spec.Bootstrap.Data != nil && !config.Status.Ready:
+		// TODO: mark the config as ready?
+		return ctrl.Result{}, nil
 	// If we've already embedded a time-limited join token into a config, but are still waiting for the token to be used, refresh it
 	case config.Status.Ready:
 		token := config.Spec.JoinConfiguration.Discovery.BootstrapToken.Token
@@ -171,12 +157,27 @@ func (r *KubeadmConfigReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, re
 			// 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")
 		}
-		return ctrl.Result{}, nil
-	// Wait patiently for the infrastructure to be ready
-	case !cluster.Status.InfrastructureReady:
-		log.Info("Infrastructure is not ready, waiting until ready.")
-		return ctrl.Result{}, nil
-	case !cluster.Status.ControlPlaneInitialized:
+		// 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
+	}
+
+	// Initialize the patch helper
+	patchHelper, err := patch.NewHelper(config, r)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+	// Attempt to Patch the KubeadmConfig object and status after each reconciliation if no error occurs.
+	defer func() {
+		if rerr == nil {
+			if rerr = patchHelper.Patch(ctx, config); rerr != nil {
+				log.Error(rerr, "failed to patch config")
+			}
+		}
+	}()
+
+	if !cluster.Status.ControlPlaneInitialized {
 		// if it's NOT a control plane machine, requeue
 		if !util.IsControlPlaneMachine(machine) {
 			log.Info(fmt.Sprintf("Machine is not a control plane. If it should be a control plane, add `%s: true` as a label to the Machine", clusterv1.MachineControlPlaneLabelName))
@@ -268,102 +269,102 @@ func (r *KubeadmConfigReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, re
 		config.Status.Ready = true
 
 		return ctrl.Result{}, nil
-	// Every other case it's a join scenario
-	default:
-		// Nb. in this case ClusterConfiguration and JoinConfiguration should not be defined by users, but in case of misconfigurations, CABPK simply ignore them
+	}
 
-		// Unlock any locks that might have been set during init process
-		r.KubeadmInitLock.Unlock(ctx, cluster)
+	// Every other case it's a join scenario
+	// Nb. in this case ClusterConfiguration and JoinConfiguration should not be defined by users, but in case of misconfigurations, CABPK simply ignore them
 
-		if config.Spec.JoinConfiguration == nil {
-			return ctrl.Result{}, errors.New("Control plane already exists for the cluster, only KubeadmConfig objects with JoinConfiguration are allowed")
-		}
+	// Unlock any locks that might have been set during init process
+	r.KubeadmInitLock.Unlock(ctx, cluster)
 
-		certificates := internalcluster.NewCertificates()
-		if err := certificates.Lookup(ctx, r.Client, cluster); err != nil {
-			log.Error(err, "unable to lookup cluster certificates")
-			return ctrl.Result{}, err
-		}
-		if err := certificates.EnsureAllExist(); err != nil {
-			return ctrl.Result{}, err
-		}
+	if config.Spec.JoinConfiguration == nil {
+		return ctrl.Result{}, errors.New("Control plane already exists for the cluster, only KubeadmConfig objects with JoinConfiguration are allowed")
+	}
 
-		hashes, err := certificates.GetByPurpose(secret.ClusterCA).Hashes()
-		if err != nil {
-			log.Error(err, "Unable to generate Cluster CA certificate hashes")
-			return ctrl.Result{}, err
-		}
-		// TODO: move this into reconcile.Discovery so that defaults for the Discovery are all in the same place
-		if config.Spec.JoinConfiguration.Discovery.BootstrapToken == nil {
-			config.Spec.JoinConfiguration.Discovery.BootstrapToken = &kubeadmv1beta1.BootstrapTokenDiscovery{}
-		}
-		config.Spec.JoinConfiguration.Discovery.BootstrapToken.CACertHashes = hashes
+	certificates := internalcluster.NewCertificates()
+	if err := certificates.Lookup(ctx, r.Client, cluster); err != nil {
+		log.Error(err, "unable to lookup cluster certificates")
+		return ctrl.Result{}, err
+	}
+	if err := certificates.EnsureAllExist(); err != nil {
+		return ctrl.Result{}, err
+	}
 
-		// ensure that joinConfiguration.Discovery is properly set for joining node on the current cluster
-		if err := r.reconcileDiscovery(cluster, config); err != nil {
-			if requeueErr, ok := errors.Cause(err).(capierrors.HasRequeueAfterError); ok {
-				log.Info(err.Error())
-				return ctrl.Result{RequeueAfter: requeueErr.GetRequeueAfter()}, nil
-			}
-			return ctrl.Result{}, err
-		}
+	hashes, err := certificates.GetByPurpose(secret.ClusterCA).Hashes()
+	if err != nil {
+		log.Error(err, "Unable to generate Cluster CA certificate hashes")
+		return ctrl.Result{}, err
+	}
+	// TODO: move this into reconcile.Discovery so that defaults for the Discovery are all in the same place
+	if config.Spec.JoinConfiguration.Discovery.BootstrapToken == nil {
+		config.Spec.JoinConfiguration.Discovery.BootstrapToken = &kubeadmv1beta1.BootstrapTokenDiscovery{}
+	}
+	config.Spec.JoinConfiguration.Discovery.BootstrapToken.CACertHashes = hashes
 
-		joindata, err := kubeadmv1beta1.ConfigurationToYAML(config.Spec.JoinConfiguration)
-		if err != nil {
-			log.Error(err, "failed to marshal join configuration")
-			return ctrl.Result{}, err
+	// ensure that joinConfiguration.Discovery is properly set for joining node on the current cluster
+	if err := r.reconcileDiscovery(cluster, config); err != nil {
+		if requeueErr, ok := errors.Cause(err).(capierrors.HasRequeueAfterError); ok {
+			log.Info(err.Error())
+			return ctrl.Result{RequeueAfter: requeueErr.GetRequeueAfter()}, nil
 		}
+		return ctrl.Result{}, err
+	}
 
-		// it's a control plane join
-		if util.IsControlPlaneMachine(machine) {
-			if config.Spec.JoinConfiguration.ControlPlane == nil {
-				return ctrl.Result{}, errors.New("Machine is a ControlPlane, but JoinConfiguration.ControlPlane is not set in the KubeadmConfig object")
-			}
-
-			cloudJoinData, err := cloudinit.NewJoinControlPlane(&cloudinit.ControlPlaneJoinInput{
-				JoinConfiguration: joindata,
-				Certificates:      certificates,
-				BaseUserData: cloudinit.BaseUserData{
-					AdditionalFiles:     config.Spec.Files,
-					NTP:                 config.Spec.NTP,
-					PreKubeadmCommands:  config.Spec.PreKubeadmCommands,
-					PostKubeadmCommands: config.Spec.PostKubeadmCommands,
-					Users:               config.Spec.Users,
-				},
-			})
-			if err != nil {
-				log.Error(err, "failed to create a control plane join configuration")
-				return ctrl.Result{}, err
-			}
-
-			config.Status.BootstrapData = cloudJoinData
-			config.Status.Ready = true
-			return ctrl.Result{}, nil
-		}
+	joindata, err := kubeadmv1beta1.ConfigurationToYAML(config.Spec.JoinConfiguration)
+	if err != nil {
+		log.Error(err, "failed to marshal join configuration")
+		return ctrl.Result{}, err
+	}
 
-		// otherwise it is a node
-		if config.Spec.JoinConfiguration.ControlPlane != nil {
-			return ctrl.Result{}, errors.New("Machine is a Worker, but JoinConfiguration.ControlPlane is set in the KubeadmConfig object")
+	// it's a control plane join
+	if util.IsControlPlaneMachine(machine) {
+		if config.Spec.JoinConfiguration.ControlPlane == nil {
+			return ctrl.Result{}, errors.New("Machine is a ControlPlane, but JoinConfiguration.ControlPlane is not set in the KubeadmConfig object")
 		}
 
-		cloudJoinData, err := cloudinit.NewNode(&cloudinit.NodeInput{
+		cloudJoinData, err := cloudinit.NewJoinControlPlane(&cloudinit.ControlPlaneJoinInput{
+			JoinConfiguration: joindata,
+			Certificates:      certificates,
 			BaseUserData: cloudinit.BaseUserData{
 				AdditionalFiles:     config.Spec.Files,
 				NTP:                 config.Spec.NTP,
 				PreKubeadmCommands:  config.Spec.PreKubeadmCommands,
 				PostKubeadmCommands: config.Spec.PostKubeadmCommands,
 				Users:               config.Spec.Users,
 			},
-			JoinConfiguration: joindata,
 		})
 		if err != nil {
-			log.Error(err, "failed to create a worker join configuration")
+			log.Error(err, "failed to create a control plane join configuration")
 			return ctrl.Result{}, err
 		}
+
 		config.Status.BootstrapData = cloudJoinData
 		config.Status.Ready = true
 		return ctrl.Result{}, nil
 	}
+
+	// otherwise it is a node
+	if config.Spec.JoinConfiguration.ControlPlane != nil {
+		return ctrl.Result{}, errors.New("Machine is a Worker, but JoinConfiguration.ControlPlane is set in the KubeadmConfig object")
+	}
+
+	cloudJoinData, err := cloudinit.NewNode(&cloudinit.NodeInput{
+		BaseUserData: cloudinit.BaseUserData{
+			AdditionalFiles:     config.Spec.Files,
+			NTP:                 config.Spec.NTP,
+			PreKubeadmCommands:  config.Spec.PreKubeadmCommands,
+			PostKubeadmCommands: config.Spec.PostKubeadmCommands,
+			Users:               config.Spec.Users,
+		},
+		JoinConfiguration: joindata,
+	})
+	if err != nil {
+		log.Error(err, "failed to create a worker join configuration")
+		return ctrl.Result{}, err
+	}
+	config.Status.BootstrapData = cloudJoinData
+	config.Status.Ready = true
+	return ctrl.Result{}, nil
 }
 
 // ClusterToKubeadmConfigs is a handler.ToRequestsFunc to be used to enqeue

controllers/kubeadmconfig_controller_test.go

@@ -650,11 +650,8 @@ func TestReconcileIfJoinNodesAndControlPlaneIsReady(t *testing.T) {
 		if err != nil {
 			t.Fatalf("Failed to reconcile:\n %+v", err)
 		}
-		if result.Requeue == true {
-			t.Fatal("did not expect to requeue")
-		}
-		if result.RequeueAfter != time.Duration(0) {
-			t.Fatal("did not expect to requeue after")
+		if result.RequeueAfter >= DefaultTokenTTL {
+			t.Fatal("expected a requeue duration less than the token TTL")
 		}
 	}