Make Cluster webhook less strict for out of date ClusterClasses

kubernetes-sigs · Feb 20, 2023 · 049b8fe · 049b8fe
1 parent cf6f77a
commit 049b8fe
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 28 deletions.
diff --git a/cmd/clusterctl/client/cluster/mover.go b/cmd/clusterctl/client/cluster/mover.go
@@ -36,7 +36,6 @@ import (
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	logf "sigs.k8s.io/cluster-api/cmd/clusterctl/log"
-	"sigs.k8s.io/cluster-api/util/annotations"
 	"sigs.k8s.io/cluster-api/util/conditions"
 	"sigs.k8s.io/cluster-api/util/patch"
 	"sigs.k8s.io/cluster-api/util/yaml"
@@ -639,7 +638,7 @@ func pauseClusterClass(proxy Proxy, n *node, pause bool) error {
 	// Update the annotation to the desired state
 	ccAnnotations := clusterClass.GetAnnotations()
 	if ccAnnotations == nil {
-		ccAnnotations = make(map[string]string)
+		ccAnnotations = map[string]string{}
 	}
 	if pause {
 		// Set the pause annotation.
@@ -649,12 +648,8 @@ func pauseClusterClass(proxy Proxy, n *node, pause bool) error {
 		delete(ccAnnotations, clusterv1.PausedAnnotation)
 	}
 
-	// If the ClusterClass is already at desired state return early.
-	if !annotations.AddAnnotations(clusterClass, ccAnnotations) {
-		return nil
-	}
-
-	// Update the cluster class with the new annotations.
+	// Update the ClusterClass with the new annotations.
+	clusterClass.SetAnnotations(ccAnnotations)
 	if err := patchHelper.Patch(ctx, clusterClass); err != nil {
 		return errors.Wrapf(err, "error patching ClusterClass %s/%s", n.identity.Namespace, n.identity.Name)
 	}

diff --git a/internal/webhooks/cluster.go b/internal/webhooks/cluster.go
@@ -62,6 +62,8 @@ type Cluster struct {
 var _ webhook.CustomDefaulter = &Cluster{}
 var _ webhook.CustomValidator = &Cluster{}
 
+var errClusterClassNotReconciled = errors.New("ClusterClass is not up to date")
+
 // Default satisfies the defaulting webhook interface.
 func (webhook *Cluster) Default(ctx context.Context, obj runtime.Object) error {
 	// We gather all defaulting errors and return them together.
@@ -88,11 +90,11 @@ func (webhook *Cluster) Default(ctx context.Context, obj runtime.Object) error {
 		}
 		clusterClass, err := webhook.pollClusterClassForCluster(ctx, cluster)
 		if err != nil {
-			// If the ClusterClass can't be found ignore the error.
-			if apierrors.IsNotFound(err) {
+			// If the ClusterClass can't be found or is not up to date ignore the error.
+			if apierrors.IsNotFound(err) || errors.Is(err, errClusterClassNotReconciled) {
 				return nil
 			}
-			return apierrors.NewInternalError(errors.Wrapf(err, "Cluster %s can't be defaulted. Valid ClusterClass %s can not be retrieved", cluster.Name, cluster.Spec.Topology.Class))
+			return apierrors.NewInternalError(errors.Wrapf(err, "Cluster %s can't be defaulted. ClusterClass %s can not be retrieved", cluster.Name, cluster.Spec.Topology.Class))
 		}
 
 		allErrs = append(allErrs, DefaultVariables(cluster, clusterClass)...)
@@ -242,27 +244,28 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu
 	}
 
 	// Get the ClusterClass referenced in the Cluster.
-	clusterClass, clusterClassGetErr := webhook.pollClusterClassForCluster(ctx, newCluster)
-	if clusterClassGetErr != nil && !apierrors.IsNotFound(clusterClassGetErr) {
-		// If the error is anything other than "Not Found" return all errors at this point.
+	clusterClass, clusterClassPollErr := webhook.pollClusterClassForCluster(ctx, newCluster)
+	if clusterClassPollErr != nil &&
+		// If the error is anything other than "NotFound" or "NotReconciled" return all errors at this point.
+		!(apierrors.IsNotFound(clusterClassPollErr) || errors.Is(clusterClassPollErr, errClusterClassNotReconciled)) {
 		allErrs = append(
 			allErrs, field.InternalError(
 				fldPath.Child("class"),
-				clusterClassGetErr))
+				clusterClassPollErr))
 		return allErrs
 	}
-	if clusterClassGetErr == nil {
+	if clusterClassPollErr == nil {
 		// If there's no error validate the Cluster based on the ClusterClass.
 		allErrs = append(allErrs, ValidateClusterForClusterClass(newCluster, clusterClass, fldPath)...)
 	}
 	if oldCluster != nil { // On update
 		// The ClusterClass must exist to proceed with update validation. Return an error if the ClusterClass was
 		// not found.
-		if clusterClassGetErr != nil {
+		if apierrors.IsNotFound(clusterClassPollErr) {
 			allErrs = append(
 				allErrs, field.InternalError(
 					fldPath.Child("class"),
-					clusterClassGetErr))
+					clusterClassPollErr))
 			return allErrs
 		}
 
@@ -341,7 +344,7 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu
 				allErrs = append(
 					allErrs, field.Forbidden(
 						fldPath.Child("class"),
-						fmt.Sprintf("ClusterClass with name %q could not be found, change from class %[1]q to class %q cannot be validated",
+						fmt.Sprintf("valid ClusterClass with name %q could not be found, change from class %[1]q to class %q cannot be validated",
 							oldCluster.Spec.Topology.Class, newCluster.Spec.Topology.Class)))
 
 				// Return early with errors if the ClusterClass can't be retrieved.
@@ -529,24 +532,25 @@ func ValidateClusterForClusterClass(cluster *clusterv1.Cluster, clusterClass *cl
 // pollClusterClassForCluster will retry getting the ClusterClass referenced in the Cluster for two seconds.
 func (webhook *Cluster) pollClusterClassForCluster(ctx context.Context, cluster *clusterv1.Cluster) (*clusterv1.ClusterClass, error) {
 	clusterClass := &clusterv1.ClusterClass{}
-	var clusterClassGetErr error
+	var clusterClassPollErr error
+	// TODO: Add a webhook warning if the ClusterClass is not up to date or not found.
 	_ = util.PollImmediate(200*time.Millisecond, 2*time.Second, func() (bool, error) {
-		if clusterClassGetErr = webhook.Client.Get(ctx, client.ObjectKey{Namespace: cluster.Namespace, Name: cluster.Spec.Topology.Class}, clusterClass); clusterClassGetErr != nil {
+		if clusterClassPollErr = webhook.Client.Get(ctx, client.ObjectKey{Namespace: cluster.Namespace, Name: cluster.Spec.Topology.Class}, clusterClass); clusterClassPollErr != nil {
 			return false, nil //nolint:nilerr
 		}
 
-		// Return an error if the ClusterClass has not successfully reconciled because variables aren't correctly
-		// reconciled.
-		// TODO: Decide whether to check generation here. This requires creating templates before creating the Cluster and
-		// may interfere with the way clusterctl move works.
-		if !conditions.Has(clusterClass, clusterv1.ClusterClassVariablesReconciledCondition) ||
+		// Return errClusterClassNotReconciled if the ClusterClass has not successfully reconciled or if the
+		// ClusterClass variables have not been successfully reconciled.:
+		if clusterClass.Generation != clusterClass.Status.ObservedGeneration ||
+			!conditions.Has(clusterClass, clusterv1.ClusterClassVariablesReconciledCondition) ||
 			conditions.IsFalse(clusterClass, clusterv1.ClusterClassVariablesReconciledCondition) {
-			clusterClassGetErr = errors.New("ClusterClass is not up to date. If this persists check ClusterClass status")
+			clusterClassPollErr = errClusterClassNotReconciled
 			return false, nil
 		}
+		clusterClassPollErr = nil
 		return true, nil
 	})
-	return clusterClass, clusterClassGetErr
+	return clusterClass, clusterClassPollErr
 }
 
 // TODO: This function will not be needed once per-definition defaulting and validation is implemented.