From 718b43733a203fa94baece5b25d9ad50a29aec92 Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Wed, 8 Sep 2021 22:15:17 +0200 Subject: [PATCH] allow cluster class compatible changes --- api/v1alpha4/clusterclass_webhook.go | 148 +++++++--- api/v1alpha4/clusterclass_webhook_test.go | 252 ++++++++++++++++-- controllers/topology/reconcile_state.go | 10 +- ...56-cluster-class-and-managed-topologies.md | 1 + 4 files changed, 340 insertions(+), 71 deletions(-) diff --git a/api/v1alpha4/clusterclass_webhook.go b/api/v1alpha4/clusterclass_webhook.go index e447289266d6..db83b5ff1fd7 100644 --- a/api/v1alpha4/clusterclass_webhook.go +++ b/api/v1alpha4/clusterclass_webhook.go @@ -18,7 +18,6 @@ package v1alpha4 import ( "fmt" - "reflect" "strings" corev1 "k8s.io/api/core/v1" @@ -112,24 +111,24 @@ func (in *ClusterClass) validate(old *ClusterClass) error { return nil } -func (in ClusterClass) validateAllRefs() field.ErrorList { +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"))...) + allErrs = append(allErrs, in.Spec.Infrastructure.isValid(in.Namespace, field.NewPath("spec", "infrastructure"))...) + allErrs = append(allErrs, in.Spec.ControlPlane.LocalObjectTemplate.isValid(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"))...) + allErrs = append(allErrs, in.Spec.ControlPlane.MachineInfrastructure.isValid(in.Namespace, field.NewPath("spec", "controlPlane", "machineInfrastructure"))...) } 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"))...) + allErrs = append(allErrs, class.Template.Bootstrap.isValid(in.Namespace, field.NewPath("spec", "workers", fmt.Sprintf("machineDeployments[%v]", i), "template", "bootstrap"))...) + allErrs = append(allErrs, class.Template.Infrastructure.isValid(in.Namespace, field.NewPath("spec", "workers", fmt.Sprintf("machineDeployments[%v]", i), "template", "infrastructure"))...) } return allErrs } -func (in ClusterClass) validateCompatibleSpecChanges(old *ClusterClass) field.ErrorList { +func (in *ClusterClass) validateCompatibleSpecChanges(old *ClusterClass) field.ErrorList { var allErrs field.ErrorList // in case of create, no changes to verify @@ -138,33 +137,32 @@ func (in ClusterClass) validateCompatibleSpecChanges(old *ClusterClass) field.Er return nil } - // Ensure that the old MachineDeployments still exist. - allErrs = append(allErrs, in.validateMachineDeploymentsChanges(old)...) - - if !reflect.DeepEqual(in.Spec.Infrastructure, old.Spec.Infrastructure) { - allErrs = append(allErrs, - field.Invalid( - field.NewPath("spec", "infrastructure"), - in.Spec.Infrastructure, - "cannot be changed.", - ), - ) - } - - if !reflect.DeepEqual(in.Spec.ControlPlane, old.Spec.ControlPlane) { - allErrs = append(allErrs, - field.Invalid( - field.NewPath("spec", "controlPlane"), - in.Spec.Infrastructure, - "cannot be changed.", - ), - ) + // Validate changes to MachineDeployments. + allErrs = append(allErrs, in.validateMachineDeploymentsCompatibleChanges(old)...) + + // Validate InfrastructureClusterTemplate changes in a compatible way. + allErrs = append(allErrs, in.Spec.Infrastructure.isCompatibleWith( + old.Spec.Infrastructure, + field.NewPath("spec", "infrastructure"), + )...) + + // Validate control plane changes in a compatible way. + allErrs = append(allErrs, in.Spec.ControlPlane.LocalObjectTemplate.isCompatibleWith( + old.Spec.ControlPlane.LocalObjectTemplate, + field.NewPath("spec", "controlPlane"), + )...) + + if in.Spec.ControlPlane.MachineInfrastructure != nil && old.Spec.ControlPlane.MachineInfrastructure != nil { + allErrs = append(allErrs, in.Spec.ControlPlane.MachineInfrastructure.isCompatibleWith( + *old.Spec.ControlPlane.MachineInfrastructure, + field.NewPath("spec", "controlPlane", "machineInfrastructure"), + )...) } return allErrs } -func (in ClusterClass) validateMachineDeploymentsChanges(old *ClusterClass) field.ErrorList { +func (in *ClusterClass) validateMachineDeploymentsCompatibleChanges(old *ClusterClass) field.ErrorList { var allErrs field.ErrorList // Ensure no MachineDeployment class was removed. @@ -181,17 +179,18 @@ func (in ClusterClass) validateMachineDeploymentsChanges(old *ClusterClass) fiel } } - // Ensure no previous MachineDeployment class was modified. - for _, class := range in.Spec.Workers.MachineDeployments { + // Ensure previous MachineDeployment class was modified in a compatible way. + for i, class := range in.Spec.Workers.MachineDeployments { for _, oldClass := range old.Spec.Workers.MachineDeployments { - if class.Class == oldClass.Class && !reflect.DeepEqual(class, oldClass) { - allErrs = append(allErrs, - field.Invalid( - field.NewPath("spec", "workers", "machineDeployments"), - class, - "cannot be changed.", - ), - ) + if class.Class == oldClass.Class { + // NOTE: class.Template.Metadata and class.Template.Bootstrap are allowed to change; + // class.Template.Bootstrap are ensured syntactically correct by validateAllRefs. + + // Validates class.Template.Infrastructure template changes in a compatible way + allErrs = append(allErrs, class.Template.Infrastructure.isCompatibleWith( + oldClass.Template.Infrastructure, + field.NewPath("spec", "workers", "machineDeployments").Index(i).Child("template", "infrastructure"), + )...) } } } @@ -199,7 +198,7 @@ func (in ClusterClass) validateMachineDeploymentsChanges(old *ClusterClass) fiel return allErrs } -func (r LocalObjectTemplate) validate(namespace string, pathPrefix *field.Path) field.ErrorList { +func (r *LocalObjectTemplate) isValid(namespace string, pathPrefix *field.Path) field.ErrorList { var allErrs field.ErrorList // check if ref is not nil. @@ -261,7 +260,68 @@ func (r LocalObjectTemplate) validate(namespace string, pathPrefix *field.Path) field.Invalid( pathPrefix.Child("ref", "apiVersion"), r.Ref.APIVersion, - "cannot be empty", + "value cannot be empty", + ), + ) + } + + return allErrs +} + +// isCompatibleWith checks if a reference is compatible with the old one. +// NOTE: this func assumes that r.isValid() is called before, thus both ref are defined and syntactically valid; +// also namespace are enforced to be the same of the ClusterClass. +func (r *LocalObjectTemplate) isCompatibleWith(old LocalObjectTemplate, pathPrefix *field.Path) field.ErrorList { + var allErrs field.ErrorList + + // Check for nil Ref here to avoid panic. + if r.Ref == nil || old.Ref == nil { + return allErrs + } + + // Ensure that the API Group and Kind does not change, while instead we allow version to change. + // TODO: In the future we might want to relax this requirement with some sort of opt-in behavior (e.g. annotation). + + gv, err := schema.ParseGroupVersion(r.Ref.APIVersion) + if err != nil { + // NOTE this should never happen. + allErrs = append(allErrs, + field.Invalid( + pathPrefix.Child("ref", "apiVersion"), + r.Ref.APIVersion, + fmt.Sprintf("must be a valid apiVersion: %v", err), + ), + ) + } + + oldGV, err := schema.ParseGroupVersion(old.Ref.APIVersion) + if err != nil { + // NOTE this should never happen. + allErrs = append(allErrs, + field.Invalid( + pathPrefix.Child("ref", "apiVersion"), + old.Ref.APIVersion, + fmt.Sprintf("must be a valid apiVersion: %v", err), + ), + ) + } + + if gv.Group != oldGV.Group { + allErrs = append(allErrs, + field.Invalid( + pathPrefix.Child("ref", "apiVersion"), + r.Ref.APIVersion, + "apiGroup cannot be changed", + ), + ) + } + + if r.Ref.Kind != old.Ref.Kind { + allErrs = append(allErrs, + field.Invalid( + pathPrefix.Child("ref", "kind"), + r.Ref.Kind, + "value cannot be changed", ), ) } @@ -270,7 +330,7 @@ func (r LocalObjectTemplate) validate(namespace string, pathPrefix *field.Path) } // classNames returns the set of MachineDeployment class names. -func (w WorkersClass) classNames() sets.String { +func (w *WorkersClass) classNames() sets.String { classes := sets.NewString() for _, class := range w.MachineDeployments { classes.Insert(class.Class) @@ -278,7 +338,7 @@ func (w WorkersClass) classNames() sets.String { return classes } -func (w WorkersClass) validateUniqueClasses(pathPrefix *field.Path) field.ErrorList { +func (w *WorkersClass) validateUniqueClasses(pathPrefix *field.Path) field.ErrorList { var allErrs field.ErrorList classes := sets.NewString() diff --git a/api/v1alpha4/clusterclass_webhook_test.go b/api/v1alpha4/clusterclass_webhook_test.go index fd0ec3ed911d..f81997463634 100644 --- a/api/v1alpha4/clusterclass_webhook_test.go +++ b/api/v1alpha4/clusterclass_webhook_test.go @@ -215,6 +215,18 @@ func TestClusterClassValidation(t *testing.T) { Namespace: "default", Kind: "barTemplate", } + incompatibleRef := &corev1.ObjectReference{ + APIVersion: "group.test.io/foo", + Kind: "another-barTemplate", + Name: "baz", + Namespace: "default", + } + compatibleRef := &corev1.ObjectReference{ + APIVersion: "group.test.io/another-foo", + Kind: "barTemplate", + Name: "another-baz", + Namespace: "default", + } tests := []struct { name string @@ -892,7 +904,57 @@ func TestClusterClassValidation(t *testing.T) { expectErr: false, }, { - name: "update fails if infrastructure changes", + name: "update pass if infrastructure changes in a compatible way", + old: &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{ + Metadata: ObjectMeta{}, + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: compatibleRef}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Metadata: ObjectMeta{}, + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + expectErr: false, + }, + { + name: "update fails if infrastructure changes in an incompatible way", old: &ClusterClass{ ObjectMeta: metav1.ObjectMeta{ Namespace: "default", @@ -921,12 +983,7 @@ func TestClusterClassValidation(t *testing.T) { Namespace: "default", }, Spec: ClusterClassSpec{ - Infrastructure: LocalObjectTemplate{Ref: &corev1.ObjectReference{ - APIVersion: "foox", - Kind: "barx", - Name: "bazx", - Namespace: "default", - }}, + Infrastructure: LocalObjectTemplate{Ref: incompatibleRef}, ControlPlane: ControlPlaneClass{ LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, }, @@ -947,7 +1004,63 @@ func TestClusterClassValidation(t *testing.T) { expectErr: true, }, { - name: "update fails if controlPlane changes", + name: "update pass if controlPlane changes in a compatible way", + old: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: ref}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, + MachineInfrastructure: &LocalObjectTemplate{Ref: ref}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Metadata: ObjectMeta{}, + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: ref}, + ControlPlane: ControlPlaneClass{ + Metadata: ObjectMeta{ + Labels: map[string]string{"foo": "bar"}, + Annotations: map[string]string{"foo": "bar"}, + }, + LocalObjectTemplate: LocalObjectTemplate{Ref: compatibleRef}, + MachineInfrastructure: &LocalObjectTemplate{Ref: compatibleRef}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Metadata: ObjectMeta{}, + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + expectErr: false, + }, + { + name: "update fails if controlPlane changes in an incompatible way (control plane template)", old: &ClusterClass{ ObjectMeta: metav1.ObjectMeta{ Namespace: "default", @@ -978,12 +1091,59 @@ func TestClusterClassValidation(t *testing.T) { Spec: ClusterClassSpec{ Infrastructure: LocalObjectTemplate{Ref: ref}, ControlPlane: ControlPlaneClass{ - LocalObjectTemplate: LocalObjectTemplate{Ref: &corev1.ObjectReference{ - APIVersion: "foox", - Kind: "barx", - Name: "bazx", - Namespace: "default", - }}, + LocalObjectTemplate: LocalObjectTemplate{Ref: incompatibleRef}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Metadata: ObjectMeta{}, + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + expectErr: true, + }, + { + name: "update fails if controlPlane changes in an incompatible way (control plane infrastructure machine template)", + old: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: ref}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, + MachineInfrastructure: &LocalObjectTemplate{Ref: ref}, + }, + Workers: WorkersClass{ + MachineDeployments: []MachineDeploymentClass{ + { + Class: "aa", + Template: MachineDeploymentClassTemplate{ + Metadata: ObjectMeta{}, + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: ref}, + }, + }, + }, + }, + }, + }, + in: &ClusterClass{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ClusterClassSpec{ + Infrastructure: LocalObjectTemplate{Ref: ref}, + ControlPlane: ControlPlaneClass{ + LocalObjectTemplate: LocalObjectTemplate{Ref: ref}, + MachineInfrastructure: &LocalObjectTemplate{Ref: incompatibleRef}, }, Workers: WorkersClass{ MachineDeployments: []MachineDeploymentClass{ @@ -1002,7 +1162,7 @@ func TestClusterClassValidation(t *testing.T) { expectErr: true, }, { - name: "update fails a machine deployment changes", + name: "update pass if a machine deployment changes in a compatible way", old: &ClusterClass{ ObjectMeta: metav1.ObjectMeta{ Namespace: "default", @@ -1040,13 +1200,38 @@ func TestClusterClassValidation(t *testing.T) { { Class: "aa", Template: MachineDeploymentClassTemplate{ - Metadata: ObjectMeta{}, - Bootstrap: LocalObjectTemplate{Ref: &corev1.ObjectReference{ - APIVersion: "foox", - Kind: "barx", - Name: "bazx", - Namespace: "default", - }}, + Metadata: ObjectMeta{ + Labels: map[string]string{"foo": "bar"}, + Annotations: map[string]string{"foo": "bar"}, + }, + Bootstrap: LocalObjectTemplate{Ref: incompatibleRef}, // NOTE: this should be tolerated + Infrastructure: LocalObjectTemplate{Ref: compatibleRef}, + }, + }, + }, + }, + }, + }, + expectErr: false, + }, + { + name: "update fails a machine deployment changes in an incompatible way", + old: &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{ + Metadata: ObjectMeta{}, + Bootstrap: LocalObjectTemplate{Ref: ref}, Infrastructure: LocalObjectTemplate{Ref: ref}, }, }, @@ -1054,6 +1239,29 @@ func TestClusterClassValidation(t *testing.T) { }, }, }, + 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{ + Metadata: ObjectMeta{}, + Bootstrap: LocalObjectTemplate{Ref: ref}, + Infrastructure: LocalObjectTemplate{Ref: incompatibleRef}, + }, + }, + }, + }, + }, + }, expectErr: true, }, { diff --git a/controllers/topology/reconcile_state.go b/controllers/topology/reconcile_state.go index 29a03098de79..6ebb03c9622c 100644 --- a/controllers/topology/reconcile_state.go +++ b/controllers/topology/reconcile_state.go @@ -201,7 +201,7 @@ func (r *ClusterReconciler) updateMachineDeployment(ctx context.Context, cluster log := loggerFrom(ctx).WithMachineDeployment(desiredMD.Object) ctx, _ = log.WithObject(desiredMD.InfrastructureMachineTemplate).Into(ctx) - cleanupOldInfrastructureTemplate, err := r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{ + _, err := r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{ ref: &desiredMD.Object.Spec.Template.Spec.InfrastructureRef, current: currentMD.InfrastructureMachineTemplate, desired: desiredMD.InfrastructureMachineTemplate, @@ -215,7 +215,7 @@ func (r *ClusterReconciler) updateMachineDeployment(ctx context.Context, cluster } ctx, _ = log.WithObject(desiredMD.BootstrapTemplate).Into(ctx) - cleanupOldBootstrapTemplate, err := r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{ + _, err = r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{ ref: desiredMD.Object.Spec.Template.Spec.Bootstrap.ConfigRef, current: currentMD.BootstrapTemplate, desired: desiredMD.BootstrapTemplate, @@ -236,7 +236,7 @@ func (r *ClusterReconciler) updateMachineDeployment(ctx context.Context, cluster } if !patchHelper.HasChanges() { log.V(3).Infof("No changes for %s", KRef{Obj: currentMD.Object}) - return kerrors.NewAggregate([]error{cleanupOldInfrastructureTemplate(), cleanupOldBootstrapTemplate()}) + return nil } log.Infof("Patching %s", KRef{Obj: currentMD.Object}) @@ -245,7 +245,7 @@ func (r *ClusterReconciler) updateMachineDeployment(ctx context.Context, cluster } // We want to call both cleanup functions even if one of them fails to clean up as much as possible. - return kerrors.NewAggregate([]error{cleanupOldInfrastructureTemplate(), cleanupOldBootstrapTemplate()}) + return nil } // deleteMachineDeployment deletes a MachineDeployment. @@ -380,7 +380,7 @@ func (r *ClusterReconciler) reconcileReferencedTemplate(ctx context.Context, in newName := names.SimpleNameGenerator.GenerateName(in.templateNamer()) in.desired.SetName(newName) - log.Infof("Rotating %s, new name %s", KRef{Obj: in.desired}, newName) + log.Infof("Rotating %s, new name %s", KRef{Obj: in.current}, newName) log.Infof("Creating %s", KRef{Obj: in.desired}) if err := r.Client.Create(ctx, in.desired.DeepCopy()); err != nil { return nil, errors.Wrapf(err, "failed to create %s/%s", in.desired.GroupVersionKind(), in.desired.GetName()) diff --git a/docs/proposals/202105256-cluster-class-and-managed-topologies.md b/docs/proposals/202105256-cluster-class-and-managed-topologies.md index 3916b3d8f1aa..14bbe6029cc2 100644 --- a/docs/proposals/202105256-cluster-class-and-managed-topologies.md +++ b/docs/proposals/202105256-cluster-class-and-managed-topologies.md @@ -324,6 +324,7 @@ type LocalObjectTemplate struct { - all the reference must be in the same namespace of `metadata.Namespace` - `spec.workers.machineDeployments[i].class` field must be unique within a ClusterClass. - `spec.workers.machineDeployments` supports adding new deployment classes. + - changes should be compliant with the compatibility rules defined in this doc. ##### Cluster - For object creation: