From b5468981b57970b4b2ff893cdeeaf198198157a1 Mon Sep 17 00:00:00 2001 From: killianmuldoon Date: Fri, 12 Nov 2021 10:17:24 +0000 Subject: [PATCH] add client based checks to clusterclass webhook Signed-off-by: killianmuldoon --- internal/builder/builders.go | 16 +- internal/topology/check/compatibility.go | 14 - internal/topology/check/compatibility_test.go | 78 ---- internal/webhooks/cluster_test.go | 30 -- internal/webhooks/clusterclass.go | 137 +++++- internal/webhooks/clusterclass_test.go | 433 ++++++++++++++++-- 6 files changed, 548 insertions(+), 160 deletions(-) diff --git a/internal/builder/builders.go b/internal/builder/builders.go index e0a70e229e54..ed5b71080df9 100644 --- a/internal/builder/builders.go +++ b/internal/builder/builders.go @@ -31,6 +31,7 @@ import ( type ClusterBuilder struct { namespace string name string + labels map[string]string topology *clusterv1.Topology infrastructureCluster *unstructured.Unstructured controlPlane *unstructured.Unstructured @@ -44,6 +45,12 @@ func Cluster(namespace, name string) *ClusterBuilder { } } +// WithLabels sets the labels for the MachineDeploymentClassBuilder. +func (c *ClusterBuilder) WithLabels(labels map[string]string) *ClusterBuilder { + c.labels = labels + return c +} + // WithInfrastructureCluster adds the passed InfrastructureCluster to the ClusterBuilder. func (c *ClusterBuilder) WithInfrastructureCluster(t *unstructured.Unstructured) *ClusterBuilder { c.infrastructureCluster = t @@ -72,6 +79,7 @@ func (c *ClusterBuilder) Build() *clusterv1.Cluster { ObjectMeta: metav1.ObjectMeta{ Name: c.name, Namespace: c.namespace, + Labels: c.labels, }, Spec: clusterv1.ClusterSpec{ Topology: c.topology, @@ -314,14 +322,10 @@ func (m *MachineDeploymentClassBuilder) Build() *clusterv1.MachineDeploymentClas }, } if m.bootstrapTemplate != nil { - obj.Template.Bootstrap = clusterv1.LocalObjectTemplate{ - Ref: objToRef(m.bootstrapTemplate), - } + obj.Template.Bootstrap.Ref = objToRef(m.bootstrapTemplate) } if m.infrastructureMachineTemplate != nil { - obj.Template.Infrastructure = clusterv1.LocalObjectTemplate{ - Ref: objToRef(m.infrastructureMachineTemplate), - } + obj.Template.Infrastructure.Ref = objToRef(m.infrastructureMachineTemplate) } return obj } diff --git a/internal/topology/check/compatibility.go b/internal/topology/check/compatibility.go index 1160f0e4c786..26524450bcfd 100644 --- a/internal/topology/check/compatibility.go +++ b/internal/topology/check/compatibility.go @@ -226,20 +226,6 @@ func ClusterClassesAreCompatible(current, desired *clusterv1.ClusterClass) field func MachineDeploymentClassesAreCompatible(current, desired *clusterv1.ClusterClass) field.ErrorList { var allErrs field.ErrorList - // Ensure no MachineDeployment class was removed. - classes := classNamesFromWorkerClass(desired.Spec.Workers) - for i, oldClass := range current.Spec.Workers.MachineDeployments { - if !classes.Has(oldClass.Class) { - allErrs = append(allErrs, - field.Invalid( - field.NewPath("spec", "workers", "machineDeployments").Index(i), - desired.Spec.Workers.MachineDeployments, - fmt.Sprintf("The %q MachineDeployment class can't be removed.", oldClass.Class), - ), - ) - } - } - // Ensure previous MachineDeployment class was modified in a compatible way. for _, class := range desired.Spec.Workers.MachineDeployments { for i, oldClass := range current.Spec.Workers.MachineDeployments { diff --git a/internal/topology/check/compatibility_test.go b/internal/topology/check/compatibility_test.go index e09386d775d6..e0a8594abdce 100644 --- a/internal/topology/check/compatibility_test.go +++ b/internal/topology/check/compatibility_test.go @@ -518,45 +518,6 @@ func TestClusterClassesAreCompatible(t *testing.T) { Build(), wantErr: false, }, - { - name: "error if machineDeploymentClass is removed from ClusterClass", - current: builder.ClusterClass(metav1.NamespaceDefault, "class1"). - WithInfrastructureClusterTemplate( - builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithControlPlaneTemplate( - refToUnstructured(ref)). - WithControlPlaneInfrastructureMachineTemplate( - refToUnstructured(ref)). - WithWorkerMachineDeploymentClasses( - *builder.MachineDeploymentClass("aa"). - WithInfrastructureTemplate( - builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithBootstrapTemplate( - builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()). - Build(), - *builder.MachineDeploymentClass("bb"). - WithInfrastructureTemplate( - builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithBootstrapTemplate( - builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()). - Build()). - Build(), - desired: builder.ClusterClass(metav1.NamespaceDefault, "class1"). - WithInfrastructureClusterTemplate( - builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithControlPlaneTemplate( - refToUnstructured(ref)). - WithControlPlaneInfrastructureMachineTemplate( - refToUnstructured(ref)). - WithWorkerMachineDeploymentClasses( - *builder.MachineDeploymentClass("aa"). - WithInfrastructureTemplate( - builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithBootstrapTemplate( - refToUnstructured(incompatibleRef)).Build()). - Build(), - wantErr: true, - }, } for _, tt := range tests { g := NewWithT(t) @@ -629,45 +590,6 @@ func TestMachineDeploymentClassesAreCompatible(t *testing.T) { Build(), wantErr: false, }, - { - name: "error if machineDeploymentClass is removed from ClusterClass", - current: builder.ClusterClass(metav1.NamespaceDefault, "class1"). - WithInfrastructureClusterTemplate( - builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithControlPlaneTemplate( - refToUnstructured(ref)). - WithControlPlaneInfrastructureMachineTemplate( - refToUnstructured(ref)). - WithWorkerMachineDeploymentClasses( - *builder.MachineDeploymentClass("aa"). - WithInfrastructureTemplate( - builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithBootstrapTemplate( - builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()). - Build(), - *builder.MachineDeploymentClass("bb"). - WithInfrastructureTemplate( - builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithBootstrapTemplate( - builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()). - Build()). - Build(), - desired: builder.ClusterClass(metav1.NamespaceDefault, "class1"). - WithInfrastructureClusterTemplate( - builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithControlPlaneTemplate( - refToUnstructured(ref)). - WithControlPlaneInfrastructureMachineTemplate( - refToUnstructured(ref)). - WithWorkerMachineDeploymentClasses( - *builder.MachineDeploymentClass("aa"). - WithInfrastructureTemplate( - builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithBootstrapTemplate( - refToUnstructured(incompatibleRef)).Build()). - Build(), - wantErr: true, - }, { name: "error if machineDeploymentClass has multiple incompatible references", current: builder.ClusterClass(metav1.NamespaceDefault, "class1"). diff --git a/internal/webhooks/cluster_test.go b/internal/webhooks/cluster_test.go index 6a5a8a8509e0..000988e6d84b 100644 --- a/internal/webhooks/cluster_test.go +++ b/internal/webhooks/cluster_test.go @@ -696,36 +696,6 @@ func TestClusterTopologyValidationForTopologyClassChange(t *testing.T) { Build(), wantErr: false, }, - { - name: "Reject cluster.topology.class change with a deleted MachineDeploymentClass", - firstClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). - WithInfrastructureClusterTemplate(refToUnstructured(ref)). - WithControlPlaneTemplate(refToUnstructured(ref)). - WithControlPlaneInfrastructureMachineTemplate(refToUnstructured(ref)). - WithWorkerMachineDeploymentClasses( - *builder.MachineDeploymentClass("aa"). - WithInfrastructureTemplate(refToUnstructured(ref)). - WithBootstrapTemplate(refToUnstructured(ref)). - Build(), - *builder.MachineDeploymentClass("bb"). - WithInfrastructureTemplate(refToUnstructured(ref)). - WithBootstrapTemplate(refToUnstructured(ref)). - Build(), - ). - Build(), - secondClass: builder.ClusterClass(metav1.NamespaceDefault, "class2"). - WithInfrastructureClusterTemplate(refToUnstructured(ref)). - WithControlPlaneTemplate(refToUnstructured(ref)). - WithControlPlaneInfrastructureMachineTemplate(refToUnstructured(ref)). - WithWorkerMachineDeploymentClasses( - *builder.MachineDeploymentClass("aa"). - WithInfrastructureTemplate(refToUnstructured(ref)). - WithBootstrapTemplate(refToUnstructured(ref)). - Build(), - ). - Build(), - wantErr: true, - }, { name: "Accept cluster.topology.class change with an added MachineDeploymentClass", firstClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). diff --git a/internal/webhooks/clusterclass.go b/internal/webhooks/clusterclass.go index 036db8ddd27d..da7a9f76c810 100644 --- a/internal/webhooks/clusterclass.go +++ b/internal/webhooks/clusterclass.go @@ -23,11 +23,13 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/feature" "sigs.k8s.io/cluster-api/internal/topology/check" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook" ) @@ -43,7 +45,9 @@ func (webhook *ClusterClass) SetupWebhookWithManager(mgr ctrl.Manager) error { // +kubebuilder:webhook:verbs=create;update,path=/mutate-cluster-x-k8s-io-v1beta1-clusterclass,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=clusterclasses,versions=v1beta1,name=default.clusterclass.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 // ClusterClass implements a validation and defaulting webhook for ClusterClass. -type ClusterClass struct{} +type ClusterClass struct { + Client client.Reader +} var _ webhook.CustomDefaulter = &ClusterClass{} var _ webhook.CustomValidator = &ClusterClass{} @@ -99,6 +103,19 @@ func (webhook *ClusterClass) ValidateUpdate(_ context.Context, oldObj, newObj ru // ValidateDelete implements validation for ClusterClass delete. func (webhook *ClusterClass) ValidateDelete(ctx context.Context, obj runtime.Object) error { + clusterClass, ok := obj.(*clusterv1.ClusterClass) + if !ok { + return apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterClass but got a %T", obj)) + } + clusters, err := webhook.clustersUsingClusterClass(clusterClass) + if err != nil { + return err.ToAggregate() + } + if len(clusters) != 0 { + // TODO: Improve error here to include the names of some clusters using the clusterClass. + return field.Forbidden(field.NewPath("clusterClass"), + fmt.Sprintf("clusterClass %v can not be deleted as it is still in use by some clusters", clusterClass.Name)) + } return nil } @@ -123,8 +140,126 @@ func (webhook *ClusterClass) validate(old, new *clusterv1.ClusterClass) error { // Ensure spec changes are compatible. allErrs = append(allErrs, check.ClusterClassesAreCompatible(old, new)...) + // Ensure no MachineDeploymentClass currently in use has been removed from the ClusterClass. + allErrs = append(allErrs, webhook.validateLivingMachineDeploymentClassesNotRemoved(old, new)...) + if len(allErrs) > 0 { return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("ClusterClass").GroupKind(), new.Name, allErrs) } return nil } + +func (webhook *ClusterClass) validateLivingMachineDeploymentClassesNotRemoved(old, new *clusterv1.ClusterClass) field.ErrorList { + var allErrs field.ErrorList + if old == nil { + return allErrs + } + missingClasses := webhook.removedMachineClasses(old, new) + + // If no classes have been removed return early as no further checks are needed. + if len(missingClasses) == 0 { + return allErrs + } + + // TODO: The first part of this method will need to be broken out to properly account for changes when also validating variables. + // Retrieve all clusters using the ClusterClass. + clusters, errs := webhook.clustersUsingClusterClass(old) + if len(errs) > 0 { + allErrs = append(allErrs, errs...) + return allErrs + } + + // Retrieve all the MachineDeployments in the clusterClass namespace using a managed topology. + machineDeployments, errs := webhook.topologyManagedMachineDeployments(new.Namespace) + if len(errs) > 0 { + allErrs = append(allErrs, errs...) + return allErrs + } + + // Create a set of records of machineDeploymentClass information where the name is a fully qualified name for the + // MachineDeployment topology in the form cluster.Name/machineDeploymentTopology.Name and links to the machineDeploymentClass Name. + mdcRecords := map[string]string{} + for _, c := range clusters { + for _, machineDeploymentClass := range c.Spec.Topology.Workers.MachineDeployments { + if missingClasses.Has(machineDeploymentClass.Class) { + mdcRecords[fmt.Sprintf("%v/%v", c.Name, machineDeploymentClass.Name)] = machineDeploymentClass.Class + } + } + } + + // For each machineDeployment using a managed topology return an error if it is using a class that has been removed in the ClusterClass change. + for _, md := range machineDeployments { + mdName := fmt.Sprintf("%v/%v", md.Labels[clusterv1.ClusterLabelName], md.Labels[clusterv1.ClusterTopologyMachineDeploymentLabelName]) + if _, ok := mdcRecords[mdName]; ok { + allErrs = append(allErrs, field.Forbidden(field.NewPath(""), fmt.Sprintf("MachineDeploymentClass %v is in use in MachineDeployment %v in Cluster %v. ClusterClass %v modification not allowed", + mdcRecords[mdName], md.Labels[clusterv1.ClusterTopologyMachineDeploymentLabelName], md.Labels[clusterv1.ClusterLabelName], old.Name), + )) + } + } + + return allErrs +} + +func (webhook *ClusterClass) topologyManagedMachineDeployments(namespace string) ([]clusterv1.MachineDeployment, field.ErrorList) { + var allErrs field.ErrorList + + // List all the MachineDeployments in the current cluster using a managed topology. + machineDeployments := &clusterv1.MachineDeploymentList{} + err := webhook.Client.List(context.Background(), machineDeployments, + client.MatchingLabels{ + clusterv1.ClusterTopologyOwnedLabel: "", + }, + // TODO: Should this be namespaced in future? + client.InNamespace(namespace), + ) + if err != nil { + allErrs = append(allErrs, field.InternalError(field.NewPath(""), err)) + return nil, allErrs + } + return machineDeployments.Items, nil +} + +func (webhook *ClusterClass) removedMachineClasses(old, new *clusterv1.ClusterClass) sets.String { + missingClasses := sets.NewString() + + // Ensure no MachineDeploymentClass in use has been removed. + classes := webhook.classNamesFromWorkerClass(new.Spec.Workers) + for _, oldClass := range old.Spec.Workers.MachineDeployments { + if !classes.Has(oldClass.Class) { + missingClasses.Insert(oldClass.Class) + } + } + return missingClasses +} + +// classNames returns the set of MachineDeployment class names. +func (webhook *ClusterClass) classNamesFromWorkerClass(w clusterv1.WorkersClass) sets.String { + classes := sets.NewString() + for _, class := range w.MachineDeployments { + classes.Insert(class.Class) + } + return classes +} + +func (webhook *ClusterClass) clustersUsingClusterClass(clusterClass *clusterv1.ClusterClass) ([]clusterv1.Cluster, field.ErrorList) { + var allErrs field.ErrorList + clusters := &clusterv1.ClusterList{} + clustersUsingClusterClass := []clusterv1.Cluster{} + err := webhook.Client.List(context.Background(), clusters, + client.MatchingLabels{ + clusterv1.ClusterTopologyOwnedLabel: "", + }, + //TODO: This will be an issue when doing clusterClass across namespaces. + client.InNamespace(clusterClass.Namespace), + ) + if err != nil { + allErrs = append(allErrs, field.InternalError(field.NewPath(""), err)) + return nil, allErrs + } + for _, c := range clusters.Items { + if c.Spec.Topology.Class == clusterClass.Name { + clustersUsingClusterClass = append(clustersUsingClusterClass, c) + } + } + return clustersUsingClusterClass, nil +} diff --git a/internal/webhooks/clusterclass_test.go b/internal/webhooks/clusterclass_test.go index baa91a577c6a..6bd714e3cb6b 100644 --- a/internal/webhooks/clusterclass_test.go +++ b/internal/webhooks/clusterclass_test.go @@ -28,6 +28,8 @@ import ( "sigs.k8s.io/cluster-api/feature" "sigs.k8s.io/cluster-api/internal/builder" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" ) var ( @@ -64,7 +66,12 @@ func TestClusterClassDefaultNamespaces(t *testing.T) { Build()). Build() - webhook := &ClusterClass{} + fakeClient := fake.NewClientBuilder(). + WithScheme(fakeScheme). + Build() + + // Create the webhook and add the fakeClient as its client. + webhook := &ClusterClass{Client: fakeClient} t.Run("for ClusterClass", customDefaultValidateTest(ctx, in, webhook)) g := NewWithT(t) @@ -994,45 +1001,273 @@ func TestClusterClassValidation(t *testing.T) { Build(), expectErr: true, }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + // Sets up the fakeClient for the test case. + fakeClient := fake.NewClientBuilder(). + WithScheme(fakeScheme). + Build() + + webhook := &ClusterClass{Client: fakeClient} + + // Create the webhook and add the fakeClient as its client. + if tt.expectErr { + g.Expect(webhook.validate(tt.old, tt.in)).NotTo(Succeed()) + } else { + g.Expect(webhook.validate(tt.old, tt.in)).To(Succeed()) + } + }) + } +} + +func TestClusterClass_ValidateMachineDeploymentClassChanges(t *testing.T) { + tests := []struct { + name string + oldClusterClass *clusterv1.ClusterClass + newClusterClass *clusterv1.ClusterClass + machineDeployments []client.Object + clusters []client.Object + expectErr bool + }{ { - name: "update fails if a machine deployment class gets removed", - old: builder.ClusterClass(metav1.NamespaceDefault, "class1"). - WithInfrastructureClusterTemplate( - builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithControlPlaneTemplate( - builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). + name: "pass if a MachineDeploymentClass not in use gets removed", + clusters: []client.Object{ + builder.Cluster(metav1.NamespaceDefault, "cluster1"). + WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}). + WithTopology( + builder.ClusterTopology(). + WithClass("class1"). + WithMachineDeployment( + builder.MachineDeploymentTopology("workers1"). + WithClass("bb"). + Build(), + ). + Build()). + Build(), + }, + machineDeployments: []client.Object{ + builder.MachineDeployment(metav1.NamespaceDefault, "md1"). + WithLabels(map[string]string{ + clusterv1.ClusterTopologyOwnedLabel: "", + clusterv1.ClusterLabelName: "cluster1", + clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers1"}, + ). + Build(), + }, + oldClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithWorkerMachineDeploymentClasses( + *builder.MachineDeploymentClass("aa"). + Build(), + *builder.MachineDeploymentClass("bb"). Build()). - WithControlPlaneInfrastructureMachineTemplate( - builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "cpInfra1"). + Build(), + newClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithWorkerMachineDeploymentClasses( + *builder.MachineDeploymentClass("bb"). Build()). + Build(), + expectErr: false, + }, + { + name: "error fails if a MachineDeploymentClass in use gets removed", + clusters: []client.Object{ + builder.Cluster(metav1.NamespaceDefault, "cluster1"). + WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}). + WithTopology( + builder.ClusterTopology(). + WithClass("class1"). + WithMachineDeployment( + builder.MachineDeploymentTopology("workers1"). + WithClass("bb"). + Build(), + ). + Build()). + Build(), + }, + machineDeployments: []client.Object{ + builder.MachineDeployment(metav1.NamespaceDefault, "md1"). + WithLabels(map[string]string{ + clusterv1.ClusterTopologyOwnedLabel: "", + clusterv1.ClusterLabelName: "cluster1", + clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers1"}, + ). + Build(), + }, + oldClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). WithWorkerMachineDeploymentClasses( *builder.MachineDeploymentClass("aa"). - WithInfrastructureTemplate( - builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithBootstrapTemplate( - builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).Build(), + Build(), *builder.MachineDeploymentClass("bb"). - WithInfrastructureTemplate( - builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithBootstrapTemplate( - builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()). Build()). Build(), - in: builder.ClusterClass(metav1.NamespaceDefault, "class1"). - WithInfrastructureClusterTemplate( - builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithControlPlaneTemplate( - builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). + newClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithWorkerMachineDeploymentClasses( + *builder.MachineDeploymentClass("aa"). Build()). - WithControlPlaneInfrastructureMachineTemplate( - builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "cpInfra1"). + Build(), + expectErr: true, + }, + { + name: "pass if MachineDeploymentClass with the same Class name is used on a Cluster which does not use the modified ClusterClass", + clusters: []client.Object{ + builder.Cluster(metav1.NamespaceDefault, "cluster1"). + WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}). + WithTopology( + builder.ClusterTopology(). + WithClass("class1"). + WithMachineDeployment( + builder.MachineDeploymentTopology("workers1"). + WithClass("bb"). + Build(), + ). + Build()). + Build(), + + builder.Cluster(metav1.NamespaceDefault, "cluster2"). + WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}). + WithTopology( + builder.ClusterTopology(). + WithClass("class2"). + WithMachineDeployment( + builder.MachineDeploymentTopology("workers1"). + WithClass("aa"). + Build(), + ). + Build()). + Build(), + }, + machineDeployments: []client.Object{ + builder.MachineDeployment(metav1.NamespaceDefault, "md1"). + WithLabels(map[string]string{ + clusterv1.ClusterTopologyOwnedLabel: "", + clusterv1.ClusterLabelName: "cluster1", + clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers1"}, + ). + Build(), + builder.MachineDeployment(metav1.NamespaceDefault, "md2"). + WithLabels(map[string]string{ + clusterv1.ClusterTopologyOwnedLabel: "", + clusterv1.ClusterLabelName: "cluster2", + clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers1"}, + ). + Build(), + }, + oldClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithWorkerMachineDeploymentClasses( + *builder.MachineDeploymentClass("aa"). + Build(), + *builder.MachineDeploymentClass("bb"). + Build()). + Build(), + newClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithWorkerMachineDeploymentClasses( + *builder.MachineDeploymentClass("bb"). Build()). + Build(), + expectErr: false, + }, + { + name: "fail if many MachineDeploymentClasses, used in multiple Clusters using the modified ClusterClass, are removed", + clusters: []client.Object{ + builder.Cluster(metav1.NamespaceDefault, "cluster1"). + WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}). + WithTopology( + builder.ClusterTopology(). + WithClass("class1"). + WithMachineDeployment( + builder.MachineDeploymentTopology("workers1"). + WithClass("bb"). + Build(), + ). + WithMachineDeployment( + builder.MachineDeploymentTopology("workers2"). + WithClass("aa"). + Build(), + ). + Build()). + Build(), + builder.Cluster(metav1.NamespaceDefault, "cluster2"). + WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}). + WithTopology( + builder.ClusterTopology(). + WithClass("class1"). + WithMachineDeployment( + builder.MachineDeploymentTopology("workers1"). + WithClass("aa"). + Build(), + ). + WithMachineDeployment( + builder.MachineDeploymentTopology("workers2"). + WithClass("aa"). + Build(), + ). + Build()). + Build(), + builder.Cluster(metav1.NamespaceDefault, "cluster3"). + WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}). + WithTopology( + builder.ClusterTopology(). + WithClass("class1"). + WithMachineDeployment( + builder.MachineDeploymentTopology("workers1"). + WithClass("bb"). + Build(), + ). + Build()). + Build(), + }, + machineDeployments: []client.Object{ + builder.MachineDeployment(metav1.NamespaceDefault, "md1"). + WithLabels(map[string]string{ + clusterv1.ClusterTopologyOwnedLabel: "", + clusterv1.ClusterLabelName: "cluster1", + clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers1"}, + ). + Build(), + builder.MachineDeployment(metav1.NamespaceDefault, "md2"). + WithLabels(map[string]string{ + clusterv1.ClusterTopologyOwnedLabel: "", + clusterv1.ClusterLabelName: "cluster1", + clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers2"}, + ). + Build(), + + builder.MachineDeployment(metav1.NamespaceDefault, "md3"). + WithLabels(map[string]string{ + clusterv1.ClusterTopologyOwnedLabel: "", + clusterv1.ClusterLabelName: "cluster2", + clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers1"}, + ). + Build(), + builder.MachineDeployment(metav1.NamespaceDefault, "md4"). + WithLabels(map[string]string{ + clusterv1.ClusterTopologyOwnedLabel: "", + clusterv1.ClusterLabelName: "cluster2", + clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers2"}, + ). + Build(), + builder.MachineDeployment(metav1.NamespaceDefault, "md5"). + WithLabels(map[string]string{ + clusterv1.ClusterTopologyOwnedLabel: "", + clusterv1.ClusterLabelName: "cluster3", + clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers1"}, + ). + Build(), + }, + oldClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). WithWorkerMachineDeploymentClasses( *builder.MachineDeploymentClass("aa"). - WithInfrastructureTemplate( - builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithBootstrapTemplate( - builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()). + Build(), + *builder.MachineDeploymentClass("bb"). + Build()). + Build(), + newClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithWorkerMachineDeploymentClasses( + *builder.MachineDeploymentClass("bb"). Build()). Build(), expectErr: true, @@ -1042,11 +1277,147 @@ func TestClusterClassValidation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - webhook := &ClusterClass{} + // Sets up the fakeClient for the test case. + fakeClient := fake.NewClientBuilder(). + WithObjects(tt.clusters...). + WithObjects(tt.machineDeployments...). + WithScheme(fakeScheme). + Build() + + // Create the webhook and add the fakeClient as its client. + webhook := &ClusterClass{Client: fakeClient} + if tt.expectErr { - g.Expect(webhook.validate(tt.old, tt.in)).NotTo(Succeed()) + g.Expect(webhook.validateLivingMachineDeploymentClassesNotRemoved(tt.oldClusterClass, tt.newClusterClass).ToAggregate()).NotTo(Succeed()) } else { - g.Expect(webhook.validate(tt.old, tt.in)).To(Succeed()) + g.Expect(webhook.validateLivingMachineDeploymentClassesNotRemoved(tt.oldClusterClass, tt.newClusterClass).ToAggregate()).To(Succeed()) + } + }) + } +} + +func TestClusterClass_ValidateDelete(t *testing.T) { + // NOTE: ClusterTopology feature flag is disabled by default, thus preventing to create or update ClusterClasses. + class := builder.ClusterClass(metav1.NamespaceDefault, "class1").Build() + + tests := []struct { + name string + clusters []client.Object + expectErr bool + }{ + { + name: "allow deletion if a cluster exists but does not reference the ClusterClass for deletion", + clusters: []client.Object{ + builder.Cluster(metav1.NamespaceDefault, "cluster1"). + WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}). + WithTopology( + builder.ClusterTopology(). + WithClass("class2"). + Build()). + Build(), + }, + expectErr: false, + }, + { + name: "error if cluster exists with a reference to the ClusterClass for deletion", + clusters: []client.Object{ + builder.Cluster(metav1.NamespaceDefault, "cluster1"). + WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}). + WithTopology( + builder.ClusterTopology(). + WithClass("class1"). + Build()). + Build(), + }, + expectErr: true, + }, + { + name: "error if multiple clusters exist and at least one references to the ClusterClass for deletion", + clusters: []client.Object{ + builder.Cluster(metav1.NamespaceDefault, "cluster1"). + WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}). + WithTopology( + builder.ClusterTopology(). + WithClass("class1"). + Build()). + Build(), + builder.Cluster(metav1.NamespaceDefault, "cluster2"). + WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}). + WithTopology( + builder.ClusterTopology(). + WithClass("class2"). + Build()). + Build(), + builder.Cluster(metav1.NamespaceDefault, "cluster3"). + WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}). + WithTopology( + builder.ClusterTopology(). + WithClass("class3"). + Build()). + Build(), + builder.Cluster(metav1.NamespaceDefault, "cluster4"). + WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}). + WithTopology( + builder.ClusterTopology(). + WithClass("class4"). + Build()). + Build(), + }, + + expectErr: true, + }, + { + name: "allow deletion if multiple clusters exist and at none references to the ClusterClass for deletion", + clusters: []client.Object{ + builder.Cluster(metav1.NamespaceDefault, "cluster1"). + WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}). + WithTopology( + builder.ClusterTopology(). + WithClass("class5"). + Build()). + Build(), + builder.Cluster(metav1.NamespaceDefault, "cluster2"). + WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}). + WithTopology( + builder.ClusterTopology(). + WithClass("class2"). + Build()). + Build(), + builder.Cluster(metav1.NamespaceDefault, "cluster3"). + WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}). + WithTopology( + builder.ClusterTopology(). + WithClass("class3"). + Build()). + Build(), + builder.Cluster(metav1.NamespaceDefault, "cluster4"). + WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}). + WithTopology( + builder.ClusterTopology(). + WithClass("class4"). + Build()). + Build(), + }, + expectErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + // Sets up the fakeClient for the test case. + fakeClient := fake.NewClientBuilder(). + WithObjects(tt.clusters...). + WithScheme(fakeScheme). + Build() + + // Create the webhook and add the fakeClient as its client. + webhook := &ClusterClass{Client: fakeClient} + + if tt.expectErr { + g.Expect(webhook.ValidateDelete(ctx, class)).NotTo(Succeed()) + } else { + g.Expect(webhook.ValidateDelete(ctx, class)).To(Succeed()) } }) }