diff --git a/internal/controllers/topology/cluster/cluster_controller.go b/internal/controllers/topology/cluster/cluster_controller.go index 2d50ceaf1700..b600e6e56dcb 100644 --- a/internal/controllers/topology/cluster/cluster_controller.go +++ b/internal/controllers/topology/cluster/cluster_controller.go @@ -25,7 +25,6 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" kerrors "k8s.io/apimachinery/pkg/util/errors" - "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" @@ -221,15 +220,13 @@ func (r *Reconciler) reconcile(ctx context.Context, s *scope.Scope) (ctrl.Result return ctrl.Result{}, nil } - // Default and Validate the Cluster based on information from the ClusterClass. + // Default and Validate the Cluster variables based on information from the ClusterClass. // This step is needed as if the ClusterClass does not exist at Cluster creation some fields may not be defaulted or // validated in the webhook. - if errs := webhooks.DefaultVariables(s.Current.Cluster, clusterClass); len(errs) > 0 { - return ctrl.Result{}, apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("Cluster").GroupKind(), s.Current.Cluster.Name, errs) - } - if errs := webhooks.ValidateClusterForClusterClass(s.Current.Cluster, clusterClass, field.NewPath("spec", "topology")); len(errs) > 0 { + if errs := webhooks.DefaultAndValidateVariables(s.Current.Cluster, clusterClass); len(errs) > 0 { return ctrl.Result{}, apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("Cluster").GroupKind(), s.Current.Cluster.Name, errs) } + // Gets the blueprint with the ClusterClass and the referenced templates // and store it in the request scope. s.Blueprint, err = r.getBlueprint(ctx, s.Current.Cluster, s.Blueprint.ClusterClass) diff --git a/internal/webhooks/cluster.go b/internal/webhooks/cluster.go index d3be0c82cdb1..0a3f255c1593 100644 --- a/internal/webhooks/cluster.go +++ b/internal/webhooks/cluster.go @@ -97,7 +97,10 @@ func (webhook *Cluster) Default(ctx context.Context, obj runtime.Object) error { return apierrors.NewInternalError(errors.Wrapf(err, "Cluster %s can't be defaulted. ClusterClass %s can not be retrieved", cluster.Name, cluster.Spec.Topology.Class)) } - allErrs = append(allErrs, DefaultVariables(cluster, clusterClass)...) + // Doing both defaulting and validating here prevents a race condition where the ClusterClass could be + // different in the defaulting and validating webhook. + allErrs = append(allErrs, DefaultAndValidateVariables(cluster, clusterClass)...) + if len(allErrs) > 0 { return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("Cluster").GroupKind(), cluster.Name, allErrs) } @@ -256,7 +259,7 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu } if clusterClassPollErr == nil { // If there's no error validate the Cluster based on the ClusterClass. - allErrs = append(allErrs, ValidateClusterForClusterClass(newCluster, clusterClass, fldPath)...) + allErrs = append(allErrs, ValidateClusterForClusterClass(newCluster, clusterClass)...) } if oldCluster != nil { // On update // The ClusterClass must exist to proceed with update validation. Return an error if the ClusterClass was @@ -453,6 +456,29 @@ func validateCIDRBlocks(fldPath *field.Path, cidrs []string) field.ErrorList { return allErrs } +// DefaultAndValidateVariables defaults and validates variables in the Cluster and MachineDeployment topologies based +// on the definitions in the ClusterClass. +func DefaultAndValidateVariables(cluster *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass) field.ErrorList { + var allErrs field.ErrorList + allErrs = append(allErrs, DefaultVariables(cluster, clusterClass)...) + + // Variables must be validated in the defaulting webhook. Variable definitions are stored in the ClusterClass status + // and are patched in the ClusterClass reconcile. + allErrs = append(allErrs, variables.ValidateClusterVariables(cluster.Spec.Topology.Variables, clusterClass.Status.Variables, + field.NewPath("spec", "topology", "variables"))...) + if cluster.Spec.Topology.Workers != nil { + for i, md := range cluster.Spec.Topology.Workers.MachineDeployments { + // Continue if there are no variable overrides. + if md.Variables == nil || len(md.Variables.Overrides) == 0 { + continue + } + allErrs = append(allErrs, variables.ValidateMachineDeploymentVariables(md.Variables.Overrides, clusterClass.Status.Variables, + field.NewPath("spec", "topology", "workers", "machineDeployments").Index(i).Child("variables", "overrides"))...) + } + } + return allErrs +} + // DefaultVariables defaults variables in the Cluster based on information in the ClusterClass. func DefaultVariables(cluster *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass) field.ErrorList { var allErrs field.ErrorList @@ -489,7 +515,7 @@ func DefaultVariables(cluster *clusterv1.Cluster, clusterClass *clusterv1.Cluste } // ValidateClusterForClusterClass uses information in the ClusterClass to validate the Cluster. -func ValidateClusterForClusterClass(cluster *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass, fldPath *field.Path) field.ErrorList { +func ValidateClusterForClusterClass(cluster *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass) field.ErrorList { var allErrs field.ErrorList if cluster == nil { return field.ErrorList{field.InternalError(field.NewPath(""), errors.New("Cluster can not be nil"))} @@ -499,24 +525,8 @@ func ValidateClusterForClusterClass(cluster *clusterv1.Cluster, clusterClass *cl } allErrs = append(allErrs, check.MachineDeploymentTopologiesAreValidAndDefinedInClusterClass(cluster, clusterClass)...) - // validate the MachineHealthChecks defined in the cluster topology + // Validate the MachineHealthChecks defined in the cluster topology. allErrs = append(allErrs, validateMachineHealthChecks(cluster, clusterClass)...) - - // Check if the variables defined in the ClusterClass are valid. - allErrs = append(allErrs, variables.ValidateClusterVariables(cluster.Spec.Topology.Variables, clusterClass.Status.Variables, - fldPath.Child("variables"))...) - - if cluster.Spec.Topology.Workers != nil { - for i, md := range cluster.Spec.Topology.Workers.MachineDeployments { - // Continue if there are no variable overrides. - if md.Variables == nil || len(md.Variables.Overrides) == 0 { - continue - } - allErrs = append(allErrs, variables.ValidateMachineDeploymentVariables(md.Variables.Overrides, clusterClass.Status.Variables, - fldPath.Child("workers", "machineDeployments").Index(i).Child("variables", "overrides"))...) - } - } - return allErrs } diff --git a/internal/webhooks/cluster_test.go b/internal/webhooks/cluster_test.go index 7f6059b43765..9e9a2f8fd214 100644 --- a/internal/webhooks/cluster_test.go +++ b/internal/webhooks/cluster_test.go @@ -57,8 +57,8 @@ func TestClusterDefaultNamespaces(t *testing.T) { g.Expect(c.Spec.ControlPlaneRef.Namespace).To(Equal(c.Namespace)) } -// TestClusterDefaultVariables cases where cluster.spec.topology.class is altered. -func TestClusterDefaultVariables(t *testing.T) { +// TestClusterDefaultAndValidateVariables cases where cluster.spec.topology.class is altered. +func TestClusterDefaultAndValidateVariables(t *testing.T) { defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.ClusterTopology, true)() tests := []struct { @@ -66,6 +66,7 @@ func TestClusterDefaultVariables(t *testing.T) { clusterClass *clusterv1.ClusterClass topology *clusterv1.Topology expect *clusterv1.Topology + wantErr bool }{ { name: "default a single variable to its correct values", @@ -526,6 +527,210 @@ func TestClusterDefaultVariables(t *testing.T) { }, }, }, + // Testing validation of variables. + { + name: "should fail when required variable is missing top-level", + clusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithStatusVariables(clusterv1.ClusterClassStatusVariable{ + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + }, + }, + }).Build(), + topology: builder.ClusterTopology().Build(), + expect: builder.ClusterTopology().Build(), + wantErr: true, + }, + { + name: "should fail when top-level variable is invalid", + clusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithStatusVariables(clusterv1.ClusterClassStatusVariable{ + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + }, + }}, + ).Build(), + topology: builder.ClusterTopology(). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`"text"`)}, + }). + Build(), + expect: builder.ClusterTopology().Build(), + wantErr: true, + }, + { + name: "should fail when variable override is invalid", + clusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithStatusVariables(clusterv1.ClusterClassStatusVariable{ + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + }, + }}).Build(), + topology: builder.ClusterTopology(). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`2`)}, + }). + WithMachineDeployment(builder.MachineDeploymentTopology("workers1"). + WithClass("aa"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`"text"`)}, + }). + Build()). + Build(), + expect: builder.ClusterTopology().Build(), + wantErr: true, + }, + { + name: "should pass when required variable exists top-level", + clusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithStatusVariables(clusterv1.ClusterClassStatusVariable{ + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + }, + }}).Build(), + topology: builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`2`)}, + }). + // Variable is not required in MachineDeployment topologies. + Build(), + expect: builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`2`)}, + }). + // Variable is not required in MachineDeployment topologies. + Build(), + }, + { + name: "should pass when top-level variable and override are valid", + clusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithWorkerMachineDeploymentClasses(*builder.MachineDeploymentClass("md1").Build()). + WithStatusVariables(clusterv1.ClusterClassStatusVariable{ + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + }, + }}).Build(), + topology: builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`2`)}, + }). + WithMachineDeployment(builder.MachineDeploymentTopology("workers1"). + WithClass("md1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`2`)}, + }). + Build()). + Build(), + expect: builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`2`)}, + }). + WithMachineDeployment(builder.MachineDeploymentTopology("workers1"). + WithClass("md1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`2`)}, + }). + Build()). + Build(), + }, + { + name: "should pass even when variable override is missing the corresponding top-level variable", + clusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithWorkerMachineDeploymentClasses(*builder.MachineDeploymentClass("md1").Build()). + WithStatusVariables(clusterv1.ClusterClassStatusVariable{ + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: false, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + }, + }}).Build(), + topology: builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithMachineDeployment(builder.MachineDeploymentTopology("workers1"). + WithClass("md1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`2`)}, + }). + Build()). + Build(), + expect: builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithVariables([]clusterv1.ClusterVariable{}...). + WithMachineDeployment(builder.MachineDeploymentTopology("workers1"). + WithClass("md1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`2`)}, + }). + Build()). + Build(), + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -548,14 +753,23 @@ func TestClusterDefaultVariables(t *testing.T) { // Create the webhook and add the fakeClient as its client. This is required because the test uses a Managed Topology. webhook := &Cluster{Client: fakeClient} - // Test if defaulting works in combination with validation. - util.CustomDefaultValidateTest(ctx, cluster, webhook)(t) // Test defaulting. t.Run("default", func(t *testing.T) { g := NewWithT(t) + if tt.wantErr { + g.Expect(webhook.Default(ctx, cluster)).To(Not(Succeed())) + return + } g.Expect(webhook.Default(ctx, cluster)).To(Succeed()) - g.Expect(cluster.Spec.Topology).To(Equal(tt.expect)) + g.Expect(cluster.Spec.Topology).To(BeEquivalentTo(tt.expect)) }) + + // Test if defaulting works in combination with validation. + // Note this test is not run for the case where the webhook should fail. + if tt.wantErr { + t.Skip("skipping test for combination of defaulting and validation (not supported by the test)") + } + util.CustomDefaultValidateTest(ctx, cluster, webhook)(t) }) } } @@ -992,202 +1206,7 @@ func TestClusterTopologyValidation(t *testing.T) { Build()). Build(), }, - { - name: "should pass when required variable exists top-level", - clusterClassStatusVariables: []clusterv1.ClusterClassStatusVariable{ - { - Name: "cpu", - Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ - { - Required: true, - From: clusterv1.VariableDefinitionFromInline, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "integer", - }, - }, - }, - }, - }, - }, - in: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.19.1"). - WithVariables(clusterv1.ClusterVariable{ - Name: "cpu", - Value: apiextensionsv1.JSON{Raw: []byte(`2`)}, - }). - // Variable is not required in MachineDeployment topologies. - Build()). - Build(), - expectErr: false, - }, - { - name: "should fail when required variable is missing top-level", - clusterClassStatusVariables: []clusterv1.ClusterClassStatusVariable{ - { - Name: "cpu", - Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ - { - Required: true, - From: clusterv1.VariableDefinitionFromInline, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "integer", - }, - }, - }, - }, - }, - }, - in: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.19.1"). - Build()). - Build(), - expectErr: true, - }, - { - name: "should fail when top-level variable is invalid", - clusterClassStatusVariables: []clusterv1.ClusterClassStatusVariable{ - { - Name: "cpu", - Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ - { - Required: true, - From: clusterv1.VariableDefinitionFromInline, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "integer", - }, - }, - }, - }, - }, - }, - in: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.19.1"). - WithVariables(clusterv1.ClusterVariable{ - Name: "cpu", - Value: apiextensionsv1.JSON{Raw: []byte(`"text"`)}, - }). - Build()). - Build(), - expectErr: true, - }, - { - name: "should pass when top-level variable and override are valid", - clusterClassStatusVariables: []clusterv1.ClusterClassStatusVariable{ - { - Name: "cpu", - Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ - { - Required: true, - From: clusterv1.VariableDefinitionFromInline, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "integer", - }, - }, - }, - }, - }, - }, - in: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.19.1"). - WithVariables(clusterv1.ClusterVariable{ - Name: "cpu", - Value: apiextensionsv1.JSON{Raw: []byte(`2`)}, - }). - WithMachineDeployment(builder.MachineDeploymentTopology("workers1"). - WithClass("aa"). - WithVariables(clusterv1.ClusterVariable{ - Name: "cpu", - Value: apiextensionsv1.JSON{Raw: []byte(`2`)}, - }). - Build()). - Build()). - Build(), - expectErr: false, - }, - { - name: "should fail when variable override is invalid", - clusterClassStatusVariables: []clusterv1.ClusterClassStatusVariable{ - { - Name: "cpu", - Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ - { - Required: true, - From: clusterv1.VariableDefinitionFromInline, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "integer", - }, - }, - }, - }, - }, - }, - in: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.19.1"). - WithVariables(clusterv1.ClusterVariable{ - Name: "cpu", - Value: apiextensionsv1.JSON{Raw: []byte(`2`)}, - }). - WithMachineDeployment(builder.MachineDeploymentTopology("workers1"). - WithClass("aa"). - WithVariables(clusterv1.ClusterVariable{ - Name: "cpu", - Value: apiextensionsv1.JSON{Raw: []byte(`"text"`)}, - }). - Build()). - Build()). - Build(), - expectErr: true, - }, - { - name: "should pass even when variable override is missing the corresponding top-level variable", - clusterClassStatusVariables: []clusterv1.ClusterClassStatusVariable{ - { - Name: "cpu", - Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ - { - Required: false, - From: clusterv1.VariableDefinitionFromInline, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "integer", - }, - }, - }, - }, - }, - }, - in: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.19.1"). - WithMachineDeployment(builder.MachineDeploymentTopology("workers1"). - WithClass("aa"). - WithVariables(clusterv1.ClusterVariable{ - Name: "cpu", - Value: apiextensionsv1.JSON{Raw: []byte(`2`)}, - }). - Build()). - Build()). - Build(), - expectErr: false, - }, } - for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t)