diff --git a/api/v1alpha4/clusterclass_webhook.go b/api/v1alpha4/clusterclass_webhook.go index 4a120f0dedf1..86e647dc6832 100644 --- a/api/v1alpha4/clusterclass_webhook.go +++ b/api/v1alpha4/clusterclass_webhook.go @@ -19,9 +19,11 @@ package v1alpha4 import ( "fmt" "reflect" + "strings" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" "sigs.k8s.io/cluster-api/feature" @@ -44,17 +46,19 @@ var _ webhook.Defaulter = &ClusterClass{} // Default satisfies the defaulting webhook interface. func (in *ClusterClass) Default() { // Default all namespaces in the references to the object namespace. - if len(in.Spec.Infrastructure.Ref.Namespace) == 0 { + if in.Spec.Infrastructure.Ref != nil && len(in.Spec.Infrastructure.Ref.Namespace) == 0 { in.Spec.Infrastructure.Ref.Namespace = in.Namespace } - if len(in.Spec.ControlPlane.Ref.Namespace) == 0 { + if in.Spec.ControlPlane.Ref != nil && len(in.Spec.ControlPlane.Ref.Namespace) == 0 { in.Spec.ControlPlane.Ref.Namespace = in.Namespace } for i := range in.Spec.Workers.MachineDeployments { - if len(in.Spec.Workers.MachineDeployments[i].Template.Bootstrap.Ref.Namespace) == 0 { + if in.Spec.Workers.MachineDeployments[i].Template.Bootstrap.Ref != nil && + len(in.Spec.Workers.MachineDeployments[i].Template.Bootstrap.Ref.Namespace) == 0 { in.Spec.Workers.MachineDeployments[i].Template.Bootstrap.Ref.Namespace = in.Namespace } - if len(in.Spec.Workers.MachineDeployments[i].Template.Infrastructure.Ref.Namespace) == 0 { + if in.Spec.Workers.MachineDeployments[i].Template.Infrastructure.Ref != nil && + len(in.Spec.Workers.MachineDeployments[i].Template.Infrastructure.Ref.Namespace) == 0 { in.Spec.Workers.MachineDeployments[i].Template.Infrastructure.Ref.Namespace = in.Namespace } } @@ -91,89 +95,50 @@ func (in *ClusterClass) validate(old *ClusterClass) error { var allErrs field.ErrorList - // ensure all the references are within the same namespace - if in.Spec.Infrastructure.Ref != nil && in.Spec.Infrastructure.Ref.Namespace != in.Namespace { - allErrs = append( - allErrs, - field.Invalid( - field.NewPath("spec", "infrastructure", "ref", "namespace"), - in.Spec.Infrastructure.Ref.Namespace, - "must match metadata.namespace", - ), - ) - } - if in.Spec.ControlPlane.Ref != nil && in.Spec.ControlPlane.Ref.Namespace != in.Namespace { - allErrs = append( - allErrs, - field.Invalid( - field.NewPath("spec", "controlPlane", "ref", "namespace"), - in.Spec.ControlPlane.Ref.Namespace, - "must match metadata.namespace", - ), - ) + // Ensure all references are valid. + allErrs = append(allErrs, in.validateAllRefs()...) + + // Ensure all MachineDeployment classes are unique. + allErrs = append(allErrs, in.Spec.Workers.validateUniqueClasses(field.NewPath("spec", "workers"))...) + + // Ensure spec changes are compatible. + allErrs = append(allErrs, in.validateCompatibleSpecChanges(old)...) + + if len(allErrs) > 0 { + return apierrors.NewInvalid(GroupVersion.WithKind("ClusterClass").GroupKind(), in.Name, allErrs) } - for _, class := range in.Spec.Workers.MachineDeployments { - if class.Template.Bootstrap.Ref != nil && class.Template.Bootstrap.Ref.Namespace != in.Namespace { - allErrs = append(allErrs, - field.Invalid( - field.NewPath("spec", "workers", "machineDeployments", "template", "bootstrap", "ref", "namespace"), - class.Template.Bootstrap.Ref.Namespace, - "must match metadata.namespace", - ), - ) - } - if class.Template.Infrastructure.Ref != nil && class.Template.Infrastructure.Ref.Namespace != in.Namespace { - allErrs = append(allErrs, - field.Invalid( - field.NewPath("spec", "workers", "machineDeployments", "template", "infrastructure", "ref", "namespace"), - class.Template.Infrastructure.Ref.Namespace, - "must match metadata.namespace", - ), - ) - } + return nil +} + +func (in ClusterClass) validateAllRefs() field.ErrorList { + var allErrs field.ErrorList + + allErrs = append(allErrs, in.Spec.Infrastructure.validate(in.Namespace, field.NewPath("spec", "infrastructure"))...) + allErrs = append(allErrs, in.Spec.ControlPlane.LocalObjectTemplate.validate(in.Namespace, field.NewPath("spec", "controlPlane"))...) + if in.Spec.ControlPlane.MachineInfrastructure != nil { + allErrs = append(allErrs, in.Spec.ControlPlane.MachineInfrastructure.validate(in.Namespace, field.NewPath("spec", "controlPlane", "machineInfrastructure"))...) } - // Ensure MachineDeployment class are unique. - classNames := sets.String{} - for _, class := range in.Spec.Workers.MachineDeployments { - if classNames.Has(class.Class) { - allErrs = append(allErrs, - field.Invalid( - field.NewPath("spec", "workers", "machineDeployments"), - class, - fmt.Sprintf("MachineDeployment class should be unique. MachineDeployment with class %q is defined more than once.", class.Class), - ), - ) - } - classNames.Insert(class.Class) + for i, class := range in.Spec.Workers.MachineDeployments { + allErrs = append(allErrs, class.Template.Bootstrap.validate(in.Namespace, field.NewPath("spec", "workers", fmt.Sprintf("machineDeployments[%v]", i), "template", "bootstrap"))...) + allErrs = append(allErrs, class.Template.Infrastructure.validate(in.Namespace, field.NewPath("spec", "workers", fmt.Sprintf("machineDeployments[%v]", i), "template", "infrastructure"))...) } - // in case of create, we are done. + return allErrs +} + +func (in ClusterClass) validateCompatibleSpecChanges(old *ClusterClass) field.ErrorList { + var allErrs field.ErrorList + + // in case of create, no changes to verify + // return early. if old == nil { - if len(allErrs) > 0 { - return apierrors.NewInvalid(GroupVersion.WithKind("ClusterClass").GroupKind(), in.Name, allErrs) - } return nil } - // Otherwise, In case of updates: - - // Makes sure all the old MachineDeployment classes are still there (only MachineDeployment class addition are allowed). - oldClassNames := sets.String{} - for _, oldClass := range old.Spec.Workers.MachineDeployments { - if !classNames.Has(oldClass.Class) { - allErrs = append(allErrs, - field.Invalid( - field.NewPath("spec", "workers", "machineDeployments"), - in.Spec.Workers.MachineDeployments, - fmt.Sprintf("The %q MachineDeployment class can't be removed.", oldClass.Class), - ), - ) - } - oldClassNames.Insert(oldClass.Class) - } + // Ensure that the old MachineDeployments still exist. + allErrs = append(allErrs, in.validateMachineDeploymentsChanges(old)...) - // Makes sure no additional changes were applied. if !reflect.DeepEqual(in.Spec.Infrastructure, old.Spec.Infrastructure) { allErrs = append(allErrs, field.Invalid( @@ -194,6 +159,27 @@ func (in *ClusterClass) validate(old *ClusterClass) error { ) } + return allErrs +} + +func (in ClusterClass) validateMachineDeploymentsChanges(old *ClusterClass) field.ErrorList { + var allErrs field.ErrorList + + // Ensure no MachineDeployment class was removed. + classes := in.Spec.Workers.classNames() + for _, oldClass := range old.Spec.Workers.MachineDeployments { + if !classes.Has(oldClass.Class) { + allErrs = append(allErrs, + field.Invalid( + field.NewPath("spec", "workers", "machineDeployments"), + in.Spec.Workers.MachineDeployments, + fmt.Sprintf("The %q MachineDeployment class can't be removed.", oldClass.Class), + ), + ) + } + } + + // Ensure no previous MachineDeployment class was modified. for _, class := range in.Spec.Workers.MachineDeployments { for _, oldClass := range old.Spec.Workers.MachineDeployments { if class.Class == oldClass.Class && !reflect.DeepEqual(class, oldClass) { @@ -208,8 +194,95 @@ func (in *ClusterClass) validate(old *ClusterClass) error { } } - if len(allErrs) > 0 { - return apierrors.NewInvalid(GroupVersion.WithKind("ClusterClass").GroupKind(), in.Name, allErrs) + return allErrs +} + +func (r LocalObjectTemplate) validate(namespace string, pathPrefix *field.Path) field.ErrorList { + var allErrs field.ErrorList + + // check if a name is provided + if r.Ref.Name == "" { + allErrs = append(allErrs, + field.Invalid( + pathPrefix.Child("ref", "name"), + r.Ref.Name, + "cannot be empty", + ), + ) } - return nil + + // validate if namespace matches the provided namespace + if namespace != "" && r.Ref.Namespace != namespace { + allErrs = append( + allErrs, + field.Invalid( + pathPrefix.Child("ref", "namespace"), + r.Ref.Namespace, + fmt.Sprintf("must be '%s'", namespace), + ), + ) + } + + // check if kind is a template + if len(r.Ref.Kind) <= len(TemplateSuffix) || !strings.HasSuffix(r.Ref.Kind, TemplateSuffix) { + allErrs = append(allErrs, + field.Invalid( + pathPrefix.Child("ref", "kind"), + r.Ref.Kind, + fmt.Sprintf("kind must be of form '%s'", TemplateSuffix), + ), + ) + } + + // check if apiVersion is valid + gv, err := schema.ParseGroupVersion(r.Ref.APIVersion) + if err != nil { + allErrs = append(allErrs, + field.Invalid( + pathPrefix.Child("ref", "apiVersion"), + r.Ref.APIVersion, + fmt.Sprintf("must be a valid apiVersion: %v", err), + ), + ) + } + if err == nil && gv.Empty() { + allErrs = append(allErrs, + field.Invalid( + pathPrefix.Child("ref", "apiVersion"), + r.Ref.APIVersion, + "cannot be empty", + ), + ) + } + + return allErrs +} + +// classNames returns the set of MachineDeployment class names. +func (w WorkersClass) classNames() sets.String { + classes := sets.NewString() + for _, class := range w.MachineDeployments { + classes.Insert(class.Class) + } + return classes +} + +func (w WorkersClass) validateUniqueClasses(pathPrefix *field.Path) field.ErrorList { + var allErrs field.ErrorList + + classes := sets.NewString() + for i, class := range w.MachineDeployments { + if classes.Has(class.Class) { + allErrs = append(allErrs, + field.Invalid( + pathPrefix.Child(fmt.Sprintf("machineDeployments[%v]", i), "class"), + class.Class, + fmt.Sprintf("MachineDeployment class should be unique. MachineDeployment with class %q is defined more than once.", class.Class), + ), + ) + } + classes.Insert(class.Class) + } + + return allErrs } diff --git a/api/v1alpha4/clusterclass_webhook_test.go b/api/v1alpha4/clusterclass_webhook_test.go index a969ff2fc406..0272622dd00c 100644 --- a/api/v1alpha4/clusterclass_webhook_test.go +++ b/api/v1alpha4/clusterclass_webhook_test.go @@ -36,7 +36,7 @@ func TestClusterClassDefaultNamespaces(t *testing.T) { namespace := "default" ref := &corev1.ObjectReference{ APIVersion: "foo", - Kind: "bar", + Kind: "barTemplate", Name: "baz", } in := &ClusterClass{ @@ -80,7 +80,7 @@ func TestClusterClassValidationFeatureGated(t *testing.T) { ref := &corev1.ObjectReference{ APIVersion: "foo", - Kind: "bar", + Kind: "barTemplate", Name: "baz", Namespace: "default", } @@ -185,23 +185,46 @@ func TestClusterClassValidation(t *testing.T) { defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.ClusterTopology, true)() ref := &corev1.ObjectReference{ - APIVersion: "foo", - Kind: "bar", + APIVersion: "group.test.io/foo", + Kind: "barTemplate", Name: "baz", Namespace: "default", } refInAnotherNamespace := &corev1.ObjectReference{ - APIVersion: "foo", - Kind: "bar", + APIVersion: "group.test.io/foo", + Kind: "barTemplate", Name: "baz", Namespace: "another-namespace", } + refBadTemplate := &corev1.ObjectReference{ + APIVersion: "group.test.io/foo", + Kind: "bar", + Name: "baz", + Namespace: "default", + } + refBadAPIVersion := &corev1.ObjectReference{ + APIVersion: "group/test.io/v1/foo", + Kind: "barTemplate", + Name: "baz", + Namespace: "default", + } + refEmptyName := &corev1.ObjectReference{ + APIVersion: "group.test.io/foo", + Namespace: "default", + Kind: "barTemplate", + } + tests := []struct { name string in *ClusterClass old *ClusterClass expectErr bool }{ + + /* + CREATE Tests + */ + { name: "create pass", in: &ClusterClass{ @@ -235,8 +258,143 @@ func TestClusterClassValidation(t *testing.T) { }, expectErr: false, }, + + // empty name in ref tests + { + name: "create fail infrastructure has empty name", + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: refEmptyName}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + expectErr: true, + }, { - name: "create fail in infrastructure has inconsistent namespace", + name: "create fail control plane class has empty name", + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: ref}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: refEmptyName}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + expectErr: true, + }, + { + name: "create fail control plane class machineinfrastructure has empty name", + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: ref}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, + MachineInfrastructure: &LocalObjectTemplate{Ref: refEmptyName}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + expectErr: true, + }, + { + name: "create fail machine deployment bootstrap has empty name", + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: ref}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Bootstrap: LocalObjectTemplate{Ref: refEmptyName}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + expectErr: true, + }, + { + name: "create fail machine deployment infrastructure has empty name", + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: ref}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: refEmptyName}, + }, + }, + }, + }, + }, + }, + expectErr: true, + }, + + // inconsistent namespace in ref tests + { + name: "create fail if infrastructure has inconsistent namespace", in: &ClusterClass{ ObjectMeta: metav1.ObjectMeta{ Namespace: "default", @@ -262,7 +420,7 @@ func TestClusterClassValidation(t *testing.T) { expectErr: true, }, { - name: "create fail in control plane has inconsistent namespace", + name: "create fail if control plane has inconsistent namespace", in: &ClusterClass{ ObjectMeta: metav1.ObjectMeta{ Namespace: "default", @@ -288,7 +446,34 @@ func TestClusterClassValidation(t *testing.T) { expectErr: true, }, { - name: "create fail in machine deployment / bootstrap has inconsistent namespace", + name: "create fail if control plane class machineinfrastructure has inconsistent namespace", + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: ref}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, + MachineInfrastructure: &LocalObjectTemplate{Ref: refInAnotherNamespace}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + expectErr: true, + }, + { + name: "create fail if machine deployment / bootstrap has inconsistent namespace", in: &ClusterClass{ ObjectMeta: metav1.ObjectMeta{ Namespace: "default", @@ -314,7 +499,7 @@ func TestClusterClassValidation(t *testing.T) { expectErr: true, }, { - name: "create fail in machine deployment / infrastructure has inconsistent namespace", + name: "create fail if machine deployment / infrastructure has inconsistent namespace", in: &ClusterClass{ ObjectMeta: metav1.ObjectMeta{ Namespace: "default", @@ -339,6 +524,284 @@ func TestClusterClassValidation(t *testing.T) { }, expectErr: true, }, + + // bad template in ref tests + { + name: "create fail if bad template in control plane", + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: refBadTemplate}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + old: nil, + expectErr: true, + }, + { + name: "create fail if bad template in control plane class machineinfrastructure", + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: ref}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, + MachineInfrastructure: &LocalObjectTemplate{Ref: refBadTemplate}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + old: nil, + expectErr: true, + }, + { + name: "create fail if bad template in infrastructure", + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: ref}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: refBadTemplate}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + old: nil, + expectErr: true, + }, + { + name: "create fail if bad template in machine deployment bootstrap", + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: ref}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Bootstrap: LocalObjectTemplate{Ref: refBadTemplate}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + old: nil, + expectErr: true, + }, + { + name: "create fail if bad template in machine deployment infrastructure", + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: ref}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: refBadTemplate}, + }, + }, + }, + }, + }, + }, + old: nil, + expectErr: true, + }, + + // bad apiVersion in ref tests + { + name: "create fail if bad apiVersion in control plane", + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: ref}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: refBadAPIVersion}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + old: nil, + expectErr: true, + }, + { + name: "create fail if bad apiVersion in control plane class machineinfrastructure", + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: ref}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, + MachineInfrastructure: &LocalObjectTemplate{Ref: refBadAPIVersion}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + old: nil, + expectErr: true, + }, + { + name: "create fail if bad apiVersion in infrastructure", + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: refBadAPIVersion}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + old: nil, + expectErr: true, + }, + { + name: "create fail if bad apiVersion in machine deployment bootstrap", + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: ref}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Bootstrap: LocalObjectTemplate{Ref: refBadAPIVersion}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + old: nil, + expectErr: true, + }, + { + name: "create fail if bad apiVersion in machine deployment infrastructure", + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: ref}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: refBadAPIVersion}, + }, + }, + }, + }, + }, + }, + old: nil, + expectErr: true, + }, + + // create test { name: "create fail if duplicated DeploymentClasses", in: &ClusterClass{ @@ -372,6 +835,11 @@ func TestClusterClassValidation(t *testing.T) { }, expectErr: true, }, + + /* + UPDATE Tests + */ + { name: "update pass in case of no changes", old: &ClusterClass{ diff --git a/api/v1alpha4/common_types.go b/api/v1alpha4/common_types.go index 60c86f55548d..fc2dabf94810 100644 --- a/api/v1alpha4/common_types.go +++ b/api/v1alpha4/common_types.go @@ -94,6 +94,11 @@ const ( ManagedByAnnotation = "cluster.x-k8s.io/managed-by" ) +const ( + // TemplateSuffix is the object kind suffix used by template types. + TemplateSuffix = "Template" +) + var ( // ZeroDuration is a zero value of the metav1.Duration type. ZeroDuration = metav1.Duration{} diff --git a/controllers/external/util.go b/controllers/external/util.go index ddb12d726590..7509d32dff54 100644 --- a/controllers/external/util.go +++ b/controllers/external/util.go @@ -30,12 +30,6 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4" ) -const ( - // TemplateSuffix is the object kind suffix used by infrastructure references associated - // with MachineSet or MachineDeployments. - TemplateSuffix = "Template" -) - // Get uses the client and reference to get an external, unstructured object. func Get(ctx context.Context, c client.Client, ref *corev1.ObjectReference, namespace string) (*unstructured.Unstructured, error) { obj := new(unstructured.Unstructured) @@ -192,7 +186,7 @@ func GenerateTemplate(in *GenerateTemplateInput) (*unstructured.Unstructured, er // Set the object Kind and strip the word "Template" if it's a suffix. if to.GetKind() == "" { - to.SetKind(strings.TrimSuffix(in.Template.GetKind(), TemplateSuffix)) + to.SetKind(strings.TrimSuffix(in.Template.GetKind(), clusterv1.TemplateSuffix)) } return to, nil } diff --git a/controllers/machinehealthcheck_controller.go b/controllers/machinehealthcheck_controller.go index 6747cf203b3b..0ba10ee21d6b 100644 --- a/controllers/machinehealthcheck_controller.go +++ b/controllers/machinehealthcheck_controller.go @@ -586,7 +586,7 @@ func unhealthyMachineCount(mhc *clusterv1.MachineHealthCheck) int { func (r *MachineHealthCheckReconciler) getExternalRemediationRequest(ctx context.Context, m *clusterv1.MachineHealthCheck, machineName string) (*unstructured.Unstructured, error) { remediationRef := &corev1.ObjectReference{ APIVersion: m.Spec.RemediationTemplate.APIVersion, - Kind: strings.TrimSuffix(m.Spec.RemediationTemplate.Kind, external.TemplateSuffix), + Kind: strings.TrimSuffix(m.Spec.RemediationTemplate.Kind, clusterv1.TemplateSuffix), Name: machineName, } remediationReq, err := external.Get(ctx, r.Client, remediationRef, m.Namespace) diff --git a/controllers/machineset_controller.go b/controllers/machineset_controller.go index 7cdd666775b7..795fead89dbb 100644 --- a/controllers/machineset_controller.go +++ b/controllers/machineset_controller.go @@ -667,7 +667,7 @@ func (r *MachineSetReconciler) getMachineNode(ctx context.Context, cluster *clus } func reconcileExternalTemplateReference(ctx context.Context, c client.Client, restConfig *rest.Config, cluster *clusterv1.Cluster, ref *corev1.ObjectReference) error { - if !strings.HasSuffix(ref.Kind, external.TemplateSuffix) { + if !strings.HasSuffix(ref.Kind, clusterv1.TemplateSuffix) { return nil } diff --git a/controlplane/kubeadm/api/v1alpha4/kubeadm_control_plane_webhook.go b/controlplane/kubeadm/api/v1alpha4/kubeadm_control_plane_webhook.go index 890225c61d21..26e244de51a7 100644 --- a/controlplane/kubeadm/api/v1alpha4/kubeadm_control_plane_webhook.go +++ b/controlplane/kubeadm/api/v1alpha4/kubeadm_control_plane_webhook.go @@ -260,6 +260,16 @@ func validateKubeadmControlPlaneSpec(s KubeadmControlPlaneSpec, namespace string ), ) } + if s.MachineTemplate.InfrastructureRef.Name == "" { + allErrs = append( + allErrs, + field.Invalid( + pathPrefix.Child("machineTemplate", "infrastructure", "name"), + s.MachineTemplate.InfrastructureRef.Name, + "cannot be empty", + ), + ) + } if s.MachineTemplate.InfrastructureRef.Namespace != namespace { allErrs = append( allErrs, diff --git a/controlplane/kubeadm/controllers/helpers.go b/controlplane/kubeadm/controllers/helpers.go index 20fa7b319147..d61a1d2ca0f4 100644 --- a/controlplane/kubeadm/controllers/helpers.go +++ b/controlplane/kubeadm/controllers/helpers.go @@ -121,7 +121,7 @@ func (r *KubeadmControlPlaneReconciler) adoptKubeconfigSecret(ctx context.Contex } func (r *KubeadmControlPlaneReconciler) reconcileExternalReference(ctx context.Context, cluster *clusterv1.Cluster, ref *corev1.ObjectReference) error { - if !strings.HasSuffix(ref.Kind, external.TemplateSuffix) { + if !strings.HasSuffix(ref.Kind, clusterv1.TemplateSuffix) { return nil }