From 1d6b58e0c74788ddf7bf6060d26a3a6bfb8f9a16 Mon Sep 17 00:00:00 2001 From: Vince Prignano Date: Wed, 29 May 2024 06:59:13 -0700 Subject: [PATCH] Add Cluster.GetClassKey() to retrieve a NamespacedName for classes Signed-off-by: Vince Prignano --- api/v1beta1/cluster_types.go | 9 +++++++++ api/v1beta1/index/cluster.go | 2 +- cmd/clusterctl/client/cluster/objectgraph.go | 2 +- cmd/clusterctl/client/cluster/topology.go | 3 ++- cmd/clusterctl/client/clusterclass.go | 2 +- .../topology/cluster/cluster_controller.go | 4 ++-- .../cluster/cluster_controller_test.go | 10 +++++----- .../cluster/patches/variables/variables.go | 2 +- internal/webhooks/cluster.go | 20 +++++++++---------- 9 files changed, 32 insertions(+), 22 deletions(-) diff --git a/api/v1beta1/cluster_types.go b/api/v1beta1/cluster_types.go index 265e358f93a8..3f785e618435 100644 --- a/api/v1beta1/cluster_types.go +++ b/api/v1beta1/cluster_types.go @@ -25,6 +25,7 @@ import ( corev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" capierrors "sigs.k8s.io/cluster-api/errors" @@ -509,6 +510,14 @@ type Cluster struct { Status ClusterStatus `json:"status,omitempty"` } +// GetClassKey returns the namespaced name for the class associated with this object. +func (c *Cluster) GetClassKey() types.NamespacedName { + if c.Spec.Topology == nil { + return types.NamespacedName{} + } + return types.NamespacedName{Namespace: c.GetNamespace(), Name: c.Spec.Topology.Class} +} + // GetConditions returns the set of conditions for this object. func (c *Cluster) GetConditions() Conditions { return c.Status.Conditions diff --git a/api/v1beta1/index/cluster.go b/api/v1beta1/index/cluster.go index 3937d09bbd49..d61d8c36f90d 100644 --- a/api/v1beta1/index/cluster.go +++ b/api/v1beta1/index/cluster.go @@ -51,7 +51,7 @@ func ClusterByClusterClassClassName(o client.Object) []string { panic(fmt.Sprintf("Expected Cluster but got a %T", o)) } if cluster.Spec.Topology != nil { - return []string{cluster.Spec.Topology.Class} + return []string{cluster.GetClassKey().Name} } return nil } diff --git a/cmd/clusterctl/client/cluster/objectgraph.go b/cmd/clusterctl/client/cluster/objectgraph.go index 90f072e20d19..73ec6450d742 100644 --- a/cmd/clusterctl/client/cluster/objectgraph.go +++ b/cmd/clusterctl/client/cluster/objectgraph.go @@ -148,7 +148,7 @@ func (n *node) captureAdditionalInformation(obj *unstructured.Unstructured) erro if n.additionalInfo == nil { n.additionalInfo = map[string]interface{}{} } - n.additionalInfo[clusterTopologyNameKey] = cluster.Spec.Topology.Class + n.additionalInfo[clusterTopologyNameKey] = cluster.GetClassKey().Name } } diff --git a/cmd/clusterctl/client/cluster/topology.go b/cmd/clusterctl/client/cluster/topology.go index 08e5d5d16471..64e1f21fb27f 100644 --- a/cmd/clusterctl/client/cluster/topology.go +++ b/cmd/clusterctl/client/cluster/topology.go @@ -697,7 +697,8 @@ func (t *topologyClient) affectedClusters(ctx context.Context, in *TopologyPlanI // Each of the Cluster that uses the ClusterClass in the input is an affected cluster. for _, cc := range affectedClusterClasses { for i := range clusterList.Items { - if clusterList.Items[i].Spec.Topology != nil && clusterList.Items[i].Spec.Topology.Class == cc.Name { + cluster := clusterList.Items[i] + if cluster.Spec.Topology != nil && cluster.GetClassKey().Name == cc.Name { affectedClusters[client.ObjectKeyFromObject(&clusterList.Items[i])] = true } } diff --git a/cmd/clusterctl/client/clusterclass.go b/cmd/clusterctl/client/clusterclass.go index ea0b8d965225..58123d83caeb 100644 --- a/cmd/clusterctl/client/clusterclass.go +++ b/cmd/clusterctl/client/clusterclass.go @@ -80,7 +80,7 @@ func clusterClassNamesFromTemplate(template Template) ([]string, error) { if cluster.Spec.Topology == nil { continue } - classes = append(classes, cluster.Spec.Topology.Class) + classes = append(classes, cluster.GetClassKey().Name) } return classes, nil } diff --git a/internal/controllers/topology/cluster/cluster_controller.go b/internal/controllers/topology/cluster/cluster_controller.go index 696c16a95c29..dc9c87b28409 100644 --- a/internal/controllers/topology/cluster/cluster_controller.go +++ b/internal/controllers/topology/cluster/cluster_controller.go @@ -221,9 +221,9 @@ func (r *Reconciler) reconcile(ctx context.Context, s *scope.Scope) (ctrl.Result // Get ClusterClass. clusterClass := &clusterv1.ClusterClass{} - key := client.ObjectKey{Name: s.Current.Cluster.Spec.Topology.Class, Namespace: s.Current.Cluster.Namespace} + key := s.Current.Cluster.GetClassKey() if err := r.Client.Get(ctx, key, clusterClass); err != nil { - return ctrl.Result{}, errors.Wrapf(err, "failed to retrieve ClusterClass %s", s.Current.Cluster.Spec.Topology.Class) + return ctrl.Result{}, errors.Wrapf(err, "failed to retrieve ClusterClass %s", key) } s.Blueprint.ClusterClass = clusterClass diff --git a/internal/controllers/topology/cluster/cluster_controller_test.go b/internal/controllers/topology/cluster/cluster_controller_test.go index ef5abd504378..972f3b42120a 100644 --- a/internal/controllers/topology/cluster/cluster_controller_test.go +++ b/internal/controllers/topology/cluster/cluster_controller_test.go @@ -303,7 +303,7 @@ func TestClusterReconciler_reconcileUpdatesOnClusterClass(t *testing.T) { // Get the clusterClass to update and check if clusterClass updates are being correctly reconciled. clusterClass := &clusterv1.ClusterClass{} - g.Expect(env.Get(ctx, client.ObjectKey{Namespace: ns.Name, Name: actualCluster.Spec.Topology.Class}, clusterClass)).To(Succeed()) + g.Expect(env.Get(ctx, actualCluster.GetClassKey(), clusterClass)).To(Succeed()) patchHelper, err := patch.NewHelper(clusterClass, env.Client) g.Expect(err).ToNot(HaveOccurred()) @@ -317,7 +317,7 @@ func TestClusterReconciler_reconcileUpdatesOnClusterClass(t *testing.T) { // Check that the clusterClass has been correctly updated to use the new infrastructure template. // This is necessary as sometimes the cache can take a little time to update. class := &clusterv1.ClusterClass{} - g.Expect(env.Get(ctx, client.ObjectKey{Namespace: ns.Name, Name: actualCluster.Spec.Topology.Class}, class)).To(Succeed()) + g.Expect(env.Get(ctx, actualCluster.GetClassKey(), class)).To(Succeed()) g.Expect(class.Spec.Workers.MachineDeployments[0].Template.Infrastructure.Ref.Name).To(Equal(infrastructureMachineTemplateName2)) g.Expect(class.Spec.Workers.MachinePools[0].Template.Infrastructure.Ref.Name).To(Equal(infrastructureMachinePoolTemplateName2)) @@ -412,7 +412,7 @@ func TestClusterReconciler_reconcileClusterClassRebase(t *testing.T) { return err } // Check to ensure the spec.topology.class has been successfully updated in the API server and cache. - g.Expect(updatedCluster.Spec.Topology.Class).To(Equal(clusterClassName2)) + g.Expect(updatedCluster.GetClassKey().Name).To(Equal(clusterClassName2)) // Check if Cluster has relevant Infrastructure and ControlPlane and labels and annotations. g.Expect(assertClusterReconcile(updatedCluster)).Should(Succeed()) @@ -633,7 +633,7 @@ func TestClusterReconciler_deleteClusterClass(t *testing.T) { // Ensure the clusterClass is available in the API server . clusterClass := &clusterv1.ClusterClass{} - g.Expect(env.Get(ctx, client.ObjectKey{Namespace: ns.Name, Name: actualCluster.Spec.Topology.Class}, clusterClass)).To(Succeed()) + g.Expect(env.Get(ctx, actualCluster.GetClassKey(), clusterClass)).To(Succeed()) // Attempt to delete the ClusterClass. Expect an error here as the ClusterClass deletion is blocked by the webhook. g.Expect(env.Delete(ctx, clusterClass)).NotTo(Succeed()) @@ -968,7 +968,7 @@ func assertControlPlaneReconcile(cluster *clusterv1.Cluster) error { } } clusterClass := &clusterv1.ClusterClass{} - if err := env.Get(ctx, client.ObjectKey{Namespace: cluster.Namespace, Name: cluster.Spec.Topology.Class}, clusterClass); err != nil { + if err := env.Get(ctx, cluster.GetClassKey(), clusterClass); err != nil { return err } // Check for the ControlPlaneInfrastructure if it's referenced in the clusterClass. diff --git a/internal/controllers/topology/cluster/patches/variables/variables.go b/internal/controllers/topology/cluster/patches/variables/variables.go index 61dbf7939d8f..9d3cd1ac7a68 100644 --- a/internal/controllers/topology/cluster/patches/variables/variables.go +++ b/internal/controllers/topology/cluster/patches/variables/variables.go @@ -63,7 +63,7 @@ func Global(clusterTopology *clusterv1.Topology, cluster *clusterv1.Cluster, def Namespace: cluster.Namespace, Topology: &runtimehooksv1.ClusterTopologyBuiltins{ Version: cluster.Spec.Topology.Version, - Class: cluster.Spec.Topology.Class, + Class: cluster.GetClassKey().Name, }, }, } diff --git a/internal/webhooks/cluster.go b/internal/webhooks/cluster.go index b70975893cd1..0f066af8c68b 100644 --- a/internal/webhooks/cluster.go +++ b/internal/webhooks/cluster.go @@ -102,7 +102,7 @@ func (webhook *Cluster) Default(ctx context.Context, obj runtime.Object) error { cluster.Spec.Topology.Version = "v" + cluster.Spec.Topology.Version } - if cluster.Spec.Topology.Class == "" { + if cluster.GetClassKey().Name == "" { allErrs = append( allErrs, field.Required( @@ -119,7 +119,7 @@ func (webhook *Cluster) Default(ctx context.Context, obj runtime.Object) error { if apierrors.IsNotFound(err) || errors.Is(err, errClusterClassNotReconciled) { return nil } - return apierrors.NewInternalError(errors.Wrapf(err, "Cluster %s can't be defaulted. 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.GetClassKey().Name)) } // Doing both defaulting and validating here prevents a race condition where the ClusterClass could be @@ -254,7 +254,7 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu var allErrs field.ErrorList // class should be defined. - if newCluster.Spec.Topology.Class == "" { + if newCluster.GetClassKey().Name == "" { allErrs = append( allErrs, field.Required( @@ -334,7 +334,7 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu } // Topology or Class can not be added on update unless ClusterTopologyUnsafeUpdateClassNameAnnotation is set. - if oldCluster.Spec.Topology == nil || oldCluster.Spec.Topology.Class == "" { + if oldCluster.Spec.Topology == nil || oldCluster.GetClassKey().Name == "" { if _, ok := newCluster.Annotations[clusterv1.ClusterTopologyUnsafeUpdateClassNameAnnotation]; ok { return allWarnings, allErrs } @@ -386,7 +386,7 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu } // If the ClusterClass referenced in the Topology has changed compatibility checks are needed. - if oldCluster.Spec.Topology.Class != newCluster.Spec.Topology.Class { + if oldCluster.GetClassKey() != newCluster.GetClassKey() { // Check to see if the ClusterClass referenced in the old version of the Cluster exists. oldClusterClass, err := webhook.pollClusterClassForCluster(ctx, oldCluster) if err != nil { @@ -394,7 +394,7 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu allErrs, field.Forbidden( fldPath.Child("class"), fmt.Sprintf("valid ClusterClass with name %q could not be retrieved, change from class %[1]q to class %q cannot be validated. Error: %s", - oldCluster.Spec.Topology.Class, newCluster.Spec.Topology.Class, err.Error()))) + oldCluster.GetClassKey(), newCluster.GetClassKey(), err.Error()))) // Return early with errors if the ClusterClass can't be retrieved. return allWarnings, allErrs @@ -854,20 +854,20 @@ func (webhook *Cluster) validateClusterClassExistsAndIsReconciled(ctx context.Co fmt.Sprintf( "Cluster refers to ClusterClass %s in the topology but it does not exist. "+ "Cluster topology has not been fully validated. "+ - "The ClusterClass must be created to reconcile the Cluster", newCluster.Spec.Topology.Class), + "The ClusterClass must be created to reconcile the Cluster", newCluster.GetClassKey()), ) case errors.Is(clusterClassPollErr, errClusterClassNotReconciled): allWarnings = append(allWarnings, fmt.Sprintf( "Cluster refers to ClusterClass %s but this object which hasn't yet been reconciled. "+ - "Cluster topology has not been fully validated. ", newCluster.Spec.Topology.Class), + "Cluster topology has not been fully validated. ", newCluster.GetClassKey()), ) // If there's any other error return a generic warning with the error message. default: allWarnings = append(allWarnings, fmt.Sprintf( "Cluster refers to ClusterClass %s in the topology but it could not be retrieved. "+ - "Cluster topology has not been fully validated: %s", newCluster.Spec.Topology.Class, clusterClassPollErr.Error()), + "Cluster topology has not been fully validated: %s", newCluster.GetClassKey(), clusterClassPollErr.Error()), ) } } @@ -879,7 +879,7 @@ func (webhook *Cluster) pollClusterClassForCluster(ctx context.Context, cluster clusterClass := &clusterv1.ClusterClass{} var clusterClassPollErr error _ = wait.PollUntilContextTimeout(ctx, 200*time.Millisecond, 2*time.Second, true, func(ctx context.Context) (bool, error) { - if clusterClassPollErr = webhook.Client.Get(ctx, client.ObjectKey{Namespace: cluster.Namespace, Name: cluster.Spec.Topology.Class}, clusterClass); clusterClassPollErr != nil { + if clusterClassPollErr = webhook.Client.Get(ctx, cluster.GetClassKey(), clusterClass); clusterClassPollErr != nil { return false, nil //nolint:nilerr }