From 48b899f11bf3ceebc0011377dab464406fc12907 Mon Sep 17 00:00:00 2001 From: killianmuldoon Date: Mon, 20 Feb 2023 13:36:21 +0000 Subject: [PATCH] Make Cluster webhook less strict for out of date ClusterClasses --- cmd/clusterctl/client/cluster/mover.go | 15 ++++++++------ internal/webhooks/cluster.go | 27 +++++++++++++------------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/cmd/clusterctl/client/cluster/mover.go b/cmd/clusterctl/client/cluster/mover.go index 6c8b01c4c867..6862955b969b 100644 --- a/cmd/clusterctl/client/cluster/mover.go +++ b/cmd/clusterctl/client/cluster/mover.go @@ -637,20 +637,23 @@ 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) + currentAnnotations := clusterClass.GetAnnotations() + + // Create a copy of the current annotations to avoid modifying the original map. + wantAnnotations := map[string]string{} + for k, v := range currentAnnotations { + wantAnnotations[k] = v } if pause { // Set the pause annotation. - ccAnnotations[clusterv1.PausedAnnotation] = "" + wantAnnotations[clusterv1.PausedAnnotation] = "" } else { // Delete the pause annotation. - delete(ccAnnotations, clusterv1.PausedAnnotation) + delete(wantAnnotations, clusterv1.PausedAnnotation) } // If the ClusterClass is already at desired state return early. - if !annotations.AddAnnotations(clusterClass, ccAnnotations) { + if !annotations.AddAnnotations(clusterClass, wantAnnotations) { return nil } diff --git a/internal/webhooks/cluster.go b/internal/webhooks/cluster.go index 2b4ba8f67cae..8fe7e9d8aac3 100644 --- a/internal/webhooks/cluster.go +++ b/internal/webhooks/cluster.go @@ -38,7 +38,6 @@ import ( "sigs.k8s.io/cluster-api/internal/topology/check" "sigs.k8s.io/cluster-api/internal/topology/variables" "sigs.k8s.io/cluster-api/util" - "sigs.k8s.io/cluster-api/util/conditions" "sigs.k8s.io/cluster-api/util/version" ) @@ -62,6 +61,8 @@ type Cluster struct { var _ webhook.CustomDefaulter = &Cluster{} var _ webhook.CustomValidator = &Cluster{} +var clusterClassNotReconciledError = 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,13 +89,12 @@ 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, clusterClassNotReconciledError) { 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)...) if len(allErrs) > 0 { return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("Cluster").GroupKind(), cluster.Name, allErrs) @@ -243,8 +243,10 @@ 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. + if clusterClassGetErr != nil && + !apierrors.IsNotFound(clusterClassGetErr) && + !errors.Is(clusterClassGetErr, clusterClassNotReconciledError) { + // If the error is anything other than "NotFound" or "NotReconciled" return all errors at this point. allErrs = append( allErrs, field.InternalError( fldPath.Child("class"), @@ -530,18 +532,15 @@ func ValidateClusterForClusterClass(cluster *clusterv1.Cluster, clusterClass *cl func (webhook *Cluster) pollClusterClassForCluster(ctx context.Context, cluster *clusterv1.Cluster) (*clusterv1.ClusterClass, error) { clusterClass := &clusterv1.ClusterClass{} var clusterClassGetErr 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 { 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) || - conditions.IsFalse(clusterClass, clusterv1.ClusterClassVariablesReconciledCondition) { - clusterClassGetErr = errors.New("ClusterClass is not up to date. If this persists check ClusterClass status") + // Return a clusterClassNotReconciledError error if the ClusterClass has not successfully reconciled. + if clusterClass.Generation != clusterClass.Status.ObservedGeneration { + clusterClassGetErr = clusterClassNotReconciledError return false, nil } return true, nil