From ac639c25f3238e9a7f4a89c4ccf07fb2a16b4322 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/envtest/environment.go | 2 +- internal/topology/check/compatibility.go | 14 - internal/topology/check/compatibility_test.go | 8 +- internal/webhooks/cluster_test.go | 4 +- internal/webhooks/clusterclass.go | 116 ++++- internal/webhooks/clusterclass_test.go | 422 +++++++++++++++++- main.go | 2 +- webhooks/alias.go | 8 +- 9 files changed, 536 insertions(+), 56 deletions(-) diff --git a/internal/builder/builders.go b/internal/builder/builders.go index e0a70e229e54..015e9fba0e43 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 ClusterBuilder. +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/envtest/environment.go b/internal/envtest/environment.go index 35673f68923c..033f5eaee87e 100644 --- a/internal/envtest/environment.go +++ b/internal/envtest/environment.go @@ -219,7 +219,7 @@ func newEnvironment(uncachedObjs ...client.Object) *Environment { if err := (&webhooks.Cluster{Client: mgr.GetClient()}).SetupWebhookWithManager(mgr); err != nil { klog.Fatalf("unable to create webhook: %+v", err) } - if err := (&webhooks.ClusterClass{}).SetupWebhookWithManager(mgr); err != nil { + if err := (&webhooks.ClusterClass{Client: mgr.GetClient()}).SetupWebhookWithManager(mgr); err != nil { klog.Fatalf("unable to create webhook: %+v", err) } if err := (&clusterv1.Machine{}).SetupWebhookWithManager(mgr); err != nil { 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..4923e83e4577 100644 --- a/internal/topology/check/compatibility_test.go +++ b/internal/topology/check/compatibility_test.go @@ -519,7 +519,7 @@ func TestClusterClassesAreCompatible(t *testing.T) { wantErr: false, }, { - name: "error if machineDeploymentClass is removed from ClusterClass", + name: "pass if machineDeploymentClass is removed from ClusterClass", current: builder.ClusterClass(metav1.NamespaceDefault, "class1"). WithInfrastructureClusterTemplate( builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). @@ -555,7 +555,7 @@ func TestClusterClassesAreCompatible(t *testing.T) { WithBootstrapTemplate( refToUnstructured(incompatibleRef)).Build()). Build(), - wantErr: true, + wantErr: false, }, } for _, tt := range tests { @@ -630,7 +630,7 @@ func TestMachineDeploymentClassesAreCompatible(t *testing.T) { wantErr: false, }, { - name: "error if machineDeploymentClass is removed from ClusterClass", + name: "pass if machineDeploymentClass is removed from ClusterClass", current: builder.ClusterClass(metav1.NamespaceDefault, "class1"). WithInfrastructureClusterTemplate( builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). @@ -666,7 +666,7 @@ func TestMachineDeploymentClassesAreCompatible(t *testing.T) { WithBootstrapTemplate( refToUnstructured(incompatibleRef)).Build()). Build(), - wantErr: true, + wantErr: false, }, { name: "error if machineDeploymentClass has multiple incompatible references", diff --git a/internal/webhooks/cluster_test.go b/internal/webhooks/cluster_test.go index 6a5a8a8509e0..7e9d31b17dea 100644 --- a/internal/webhooks/cluster_test.go +++ b/internal/webhooks/cluster_test.go @@ -697,7 +697,7 @@ func TestClusterTopologyValidationForTopologyClassChange(t *testing.T) { wantErr: false, }, { - name: "Reject cluster.topology.class change with a deleted MachineDeploymentClass", + name: "Accept cluster.topology.class change with a deleted MachineDeploymentClass", firstClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). WithInfrastructureClusterTemplate(refToUnstructured(ref)). WithControlPlaneTemplate(refToUnstructured(ref)). @@ -724,7 +724,7 @@ func TestClusterTopologyValidationForTopologyClassChange(t *testing.T) { Build(), ). Build(), - wantErr: true, + wantErr: false, }, { name: "Accept cluster.topology.class change with an added MachineDeploymentClass", diff --git a/internal/webhooks/clusterclass.go b/internal/webhooks/clusterclass.go index 036db8ddd27d..98e6ca112c64 100644 --- a/internal/webhooks/clusterclass.go +++ b/internal/webhooks/clusterclass.go @@ -20,14 +20,17 @@ import ( "context" "fmt" + "github.com/pkg/errors" 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 +46,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{} @@ -76,16 +81,16 @@ func defaultNamespace(ref *corev1.ObjectReference, namespace string) { } // ValidateCreate implements validation for ClusterClass create. -func (webhook *ClusterClass) ValidateCreate(_ context.Context, obj runtime.Object) error { +func (webhook *ClusterClass) ValidateCreate(ctx context.Context, obj runtime.Object) error { in, ok := obj.(*clusterv1.ClusterClass) if !ok { return apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterClass but got a %T", obj)) } - return webhook.validate(nil, in) + return webhook.validate(ctx, nil, in) } // ValidateUpdate implements validation for ClusterClass update. -func (webhook *ClusterClass) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) error { +func (webhook *ClusterClass) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) error { newClusterClass, ok := newObj.(*clusterv1.ClusterClass) if !ok { return apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterClass but got a %T", newObj)) @@ -94,15 +99,30 @@ func (webhook *ClusterClass) ValidateUpdate(_ context.Context, oldObj, newObj ru if !ok { return apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterClass but got a %T", oldObj)) } - return webhook.validate(oldClusterClass, newClusterClass) + return webhook.validate(ctx, oldClusterClass, newClusterClass) } // 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.getClustersUsingClusterClass(ctx, clusterClass) + if err != nil { + return apierrors.NewInternalError(errors.Wrapf(err, "could not retrieve Clusters using ClusterClass")) + } + + if len(clusters) > 0 { + // TODO(killianmuldoon): Improve error here to include the names of some clusters using the clusterClass. + return apierrors.NewForbidden(clusterv1.GroupVersion.WithResource("ClusterClass").GroupResource(), clusterClass.Name, + fmt.Errorf("cannot be deleted. %d clusters still using the ClusterClass", len(clusters))) + } return nil } -func (webhook *ClusterClass) validate(old, new *clusterv1.ClusterClass) error { +func (webhook *ClusterClass) validate(ctx context.Context, old, new *clusterv1.ClusterClass) error { // NOTE: ClusterClass and managed topologies are behind ClusterTopology feature gate flag; the web hook // must prevent creating new objects new case the feature flag is disabled. if !feature.Gates.Enabled(feature.ClusterTopology) { @@ -111,7 +131,6 @@ func (webhook *ClusterClass) validate(old, new *clusterv1.ClusterClass) error { "can be set only if the ClusterTopology feature flag is enabled", ) } - var allErrs field.ErrorList // Ensure all references are valid. @@ -120,11 +139,88 @@ func (webhook *ClusterClass) validate(old, new *clusterv1.ClusterClass) error { // Ensure all MachineDeployment classes are unique. allErrs = append(allErrs, check.MachineDeploymentClassesAreUnique(new)...) - // Ensure spec changes are compatible. - allErrs = append(allErrs, check.ClusterClassesAreCompatible(old, new)...) - + // If this is an update run additional validation. + if old != nil { + // Ensure spec changes are compatible. + allErrs = append(allErrs, check.ClusterClassesAreCompatible(old, new)...) + + // Retrieve all clusters using the ClusterClass. + clusters, err := webhook.getClustersUsingClusterClass(ctx, old) + if err != nil { + allErrs = append(allErrs, field.InternalError(field.NewPath(""), + errors.Wrapf(err, "Clusters using ClusterClass %v can not be retrieved", old.Name))) + return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("ClusterClass").GroupKind(), new.Name, allErrs) + } + + // Ensure no MachineDeploymentClass currently in use has been removed from the ClusterClass. + allErrs = append(allErrs, webhook.validateRemovedMachineDeploymentClassesAreNotUsed(clusters, old, new)...) + } if len(allErrs) > 0 { return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("ClusterClass").GroupKind(), new.Name, allErrs) } return nil } + +func (webhook *ClusterClass) validateRemovedMachineDeploymentClassesAreNotUsed(clusters []clusterv1.Cluster, old, new *clusterv1.ClusterClass) field.ErrorList { + var allErrs field.ErrorList + + removedClasses := webhook.removedMachineClasses(old, new) + // If no classes have been removed return early as no further checks are needed. + if len(removedClasses) == 0 { + return nil + } + // Error if any Cluster using the ClusterClass uses a MachineDeploymentClass that has been removed. + for _, c := range clusters { + for _, machineDeploymentTopology := range c.Spec.Topology.Workers.MachineDeployments { + if removedClasses.Has(machineDeploymentTopology.Class) { + // TODO(killianmuldoon): Improve error printing here so large scale changes don't flood the error log e.g. deduplication, only example usages given. + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "workers", "machineDeployments"), + fmt.Sprintf("MachineDeploymentClass %v is in use in MachineDeploymentTopology %v in Cluster %v. ClusterClass %v modification not allowed", + machineDeploymentTopology.Class, machineDeploymentTopology.Name, c.Name, old.Name), + )) + } + } + } + return allErrs +} + +func (webhook *ClusterClass) removedMachineClasses(old, new *clusterv1.ClusterClass) sets.String { + removedClasses := sets.NewString() + + classes := webhook.classNamesFromWorkerClass(new.Spec.Workers) + for _, oldClass := range old.Spec.Workers.MachineDeployments { + if !classes.Has(oldClass.Class) { + removedClasses.Insert(oldClass.Class) + } + } + return removedClasses +} + +// classNamesFromWorkerClass 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) getClustersUsingClusterClass(ctx context.Context, clusterClass *clusterv1.ClusterClass) ([]clusterv1.Cluster, error) { + clusters := &clusterv1.ClusterList{} + clustersUsingClusterClass := []clusterv1.Cluster{} + err := webhook.Client.List(ctx, clusters, + client.MatchingLabels{ + clusterv1.ClusterTopologyOwnedLabel: "", + }, + client.InNamespace(clusterClass.Namespace), + ) + if err != nil { + return nil, err + } + 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..c82cf0a7de47 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) @@ -155,9 +162,9 @@ func TestClusterClassValidationFeatureGated(t *testing.T) { g := NewWithT(t) webhook := &ClusterClass{} if tt.expectErr { - g.Expect(webhook.validate(tt.old, tt.in)).NotTo(Succeed()) + g.Expect(webhook.validate(ctx, tt.old, tt.in)).NotTo(Succeed()) } else { - g.Expect(webhook.validate(tt.old, tt.in)).To(Succeed()) + g.Expect(webhook.validate(ctx, tt.old, tt.in)).To(Succeed()) } }) } @@ -994,23 +1001,156 @@ 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(ctx, tt.old, tt.in)).NotTo(Succeed()) + } else { + g.Expect(webhook.validate(ctx, tt.old, tt.in)).To(Succeed()) + } + }) + } +} + +func TestClusterClassValidationWithClusterAwareChecks(t *testing.T) { + // NOTE: ClusterTopology feature flag is disabled by default, thus preventing to create or update ClusterClasses. + // Enabling the feature flag temporarily for this test. + defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.ClusterTopology, true)() + + tests := []struct { + name string + oldClusterClass *clusterv1.ClusterClass + newClusterClass *clusterv1.ClusterClass + clusters []client.Object + expectErr bool + }{ { - name: "update fails if a machine deployment class gets removed", - old: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + name: "error 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(), + }, + oldClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). WithInfrastructureClusterTemplate( - builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). + builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "inf").Build()). WithControlPlaneTemplate( builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). Build()). - WithControlPlaneInfrastructureMachineTemplate( - builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "cpInfra1"). + 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(), + newClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithInfrastructureClusterTemplate( + builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "inf").Build()). + WithControlPlaneTemplate( + builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). Build()). WithWorkerMachineDeploymentClasses( *builder.MachineDeploymentClass("aa"). WithInfrastructureTemplate( builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()). WithBootstrapTemplate( - builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).Build(), + builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()). + Build()). + Build(), + expectErr: true, + }, + { + name: "error 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(), + }, + oldClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithInfrastructureClusterTemplate( + builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "inf").Build()). + WithControlPlaneTemplate( + builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). + Build()). + 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()). @@ -1018,35 +1158,285 @@ func TestClusterClassValidation(t *testing.T) { builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()). Build()). Build(), - in: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + newClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). WithInfrastructureClusterTemplate( - builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). + builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "inf").Build()). WithControlPlaneTemplate( builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). Build()). - WithControlPlaneInfrastructureMachineTemplate( - builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "cpInfra1"). + WithWorkerMachineDeploymentClasses( + *builder.MachineDeploymentClass("bb"). + WithInfrastructureTemplate( + builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()). + WithBootstrapTemplate( + builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()). + Build()). + Build(), + expectErr: true, + }, + { + name: "pass if a similar MachineDeploymentClass is deleted when it is only used in Clusters not belonging to the 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"). + + // A MachineDeploymentClass with the same name is in ClusterClass "class1" but + // this cluster is based on ClusterClass "class2" and does not impact deletion. + WithClass("aa"). + Build(), + ). + Build()). + Build(), + }, + oldClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithInfrastructureClusterTemplate( + builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "inf").Build()). + WithControlPlaneTemplate( + builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). Build()). 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(), + newClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithInfrastructureClusterTemplate( + builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "inf").Build()). + WithControlPlaneTemplate( + builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). + Build()). + WithWorkerMachineDeploymentClasses( + *builder.MachineDeploymentClass("bb"). + WithInfrastructureTemplate( + builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()). + WithBootstrapTemplate( + builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()). + Build()). + Build(), + expectErr: false, + }, + { + 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(), + }, + oldClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithInfrastructureClusterTemplate( + builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "inf").Build()). + WithControlPlaneTemplate( + builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). + Build()). + 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(), + newClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithInfrastructureClusterTemplate( + builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "inf").Build()). + WithControlPlaneTemplate( + builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). + Build()). + WithWorkerMachineDeploymentClasses( + *builder.MachineDeploymentClass("bb"). + WithInfrastructureTemplate( + builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()). + WithBootstrapTemplate( + builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()). + 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(). + WithScheme(fakeScheme). + WithObjects(tt.clusters...). + Build() + + // Create the webhook and add the fakeClient as its client. + webhook := &ClusterClass{Client: fakeClient} + + if tt.expectErr { + g.Expect(webhook.validate(ctx, tt.oldClusterClass, tt.newClusterClass)).NotTo(Succeed()) + } else { + g.Expect(webhook.validate(ctx, tt.oldClusterClass, tt.newClusterClass)).To(Succeed()) + } + }) + } +} + +func TestClusterClass_ValidateDelete(t *testing.T) { + 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 none of them 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) - webhook := &ClusterClass{} + // 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.validate(tt.old, tt.in)).NotTo(Succeed()) + g.Expect(webhook.ValidateDelete(ctx, class)).NotTo(Succeed()) } else { - g.Expect(webhook.validate(tt.old, tt.in)).To(Succeed()) + g.Expect(webhook.ValidateDelete(ctx, class)).To(Succeed()) } }) } diff --git a/main.go b/main.go index 1ad28c307652..15c19b7e475b 100644 --- a/main.go +++ b/main.go @@ -403,7 +403,7 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) { func setupWebhooks(mgr ctrl.Manager) { // NOTE: ClusterClass and managed topologies are behind ClusterTopology feature gate flag; the webhook // is going to prevent creating or updating new objects in case the feature flag is disabled. - if err := (&webhooks.ClusterClass{}).SetupWebhookWithManager(mgr); err != nil { + if err := (&webhooks.ClusterClass{Client: mgr.GetClient()}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "ClusterClass") os.Exit(1) } diff --git a/webhooks/alias.go b/webhooks/alias.go index d2818a197acf..e83f55ace3c2 100644 --- a/webhooks/alias.go +++ b/webhooks/alias.go @@ -35,9 +35,13 @@ func (webhook *Cluster) SetupWebhookWithManager(mgr ctrl.Manager) error { } // ClusterClass implements a validation and defaulting webhook for ClusterClass. -type ClusterClass struct{} +type ClusterClass struct { + Client client.Reader +} // SetupWebhookWithManager sets up ClusterClass webhooks. func (webhook *ClusterClass) SetupWebhookWithManager(mgr ctrl.Manager) error { - return (&webhooks.ClusterClass{}).SetupWebhookWithManager(mgr) + return (&webhooks.ClusterClass{ + Client: webhook.Client, + }).SetupWebhookWithManager(mgr) }