Improve ManagedCluster update validation

Validate template only in case it was updated in the cluster's spec Closes Mirantis#244
eromanova · Oct 18, 2024 · 22afeb5 · 22afeb5
1 parent 1a8841f
commit 22afeb5
Show file tree

Hide file tree

Showing 2 changed files with 84 additions and 22 deletions.
diff --git a/internal/webhook/managedcluster_webhook.go b/internal/webhook/managedcluster_webhook.go
@@ -78,23 +78,28 @@ func (v *ManagedClusterValidator) ValidateCreate(ctx context.Context, obj runtim
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
-func (v *ManagedClusterValidator) ValidateUpdate(ctx context.Context, _ runtime.Object, newObj runtime.Object) (admission.Warnings, error) {
+func (v *ManagedClusterValidator) ValidateUpdate(ctx context.Context, oldObj runtime.Object, newObj runtime.Object) (admission.Warnings, error) {
+	oldManagedCluster, ok := oldObj.(*hmcv1alpha1.ManagedCluster)
+	if !ok {
+		return nil, apierrors.NewBadRequest(fmt.Sprintf("expected ManagedCluster but got a %T", oldObj))
+	}
 	newManagedCluster, ok := newObj.(*hmcv1alpha1.ManagedCluster)
 	if !ok {
 		return nil, apierrors.NewBadRequest(fmt.Sprintf("expected ManagedCluster but got a %T", newObj))
 	}
-
 	template, err := v.getManagedClusterTemplate(ctx, newManagedCluster.Namespace, newManagedCluster.Spec.Template)
 	if err != nil {
 		return nil, fmt.Errorf("%s: %v", invalidManagedClusterMsg, err)
 	}
 
-	if err := isTemplateValid(template); err != nil {
-		return nil, fmt.Errorf("%s: %v", invalidManagedClusterMsg, err)
-	}
+	if oldManagedCluster.Spec.Template != newManagedCluster.Spec.Template {
+		if err := isTemplateValid(template); err != nil {
+			return nil, fmt.Errorf("%s: %v", invalidManagedClusterMsg, err)
+		}
 
-	if err := validateK8sCompatibility(ctx, v.Client, template, newManagedCluster); err != nil {
-		return admission.Warnings{"Failed to validate k8s version compatibility with ServiceTemplates"}, fmt.Errorf("failed to validate k8s compatibility: %v", err)
+		if err := validateK8sCompatibility(ctx, v.Client, template, newManagedCluster); err != nil {
+			return admission.Warnings{"Failed to validate k8s version compatibility with ServiceTemplates"}, fmt.Errorf("failed to validate k8s compatibility: %v", err)
+		}
 	}
 
 	if err := v.validateCredential(ctx, newManagedCluster, template); err != nil {

diff --git a/internal/webhook/managedcluster_webhook_test.go b/internal/webhook/managedcluster_webhook_test.go
@@ -37,7 +37,9 @@ import (
 var (
 	testTemplateName   = "template-test"
 	testCredentialName = "cred-test"
-	testNamespace      = "test"
+	newTemplateName    = "new-template-name"
+
+	testNamespace = "test"
 
 	mgmt = management.NewManagement(
 		management.WithAvailableProviders(v1alpha1.ProvidersTupled{
@@ -56,8 +58,18 @@ var (
 				Name: "awsclid",
 			}),
 	)
+)
+
+func TestManagedClusterValidateCreate(t *testing.T) {
+	g := NewWithT(t)
+
+	ctx := admission.NewContextWithRequest(context.Background(), admission.Request{
+		AdmissionRequest: admissionv1.AdmissionRequest{
+			Operation: admissionv1.Create,
+		},
+	})
 
-	createAndUpdateTests = []struct {
+	tests := []struct {
 		name            string
 		managedCluster  *v1alpha1.ManagedCluster
 		existingObjects []runtime.Object
@@ -225,17 +237,7 @@ var (
 			err: "the ManagedCluster is invalid: wrong kind of the ClusterIdentity \"AWSClusterStaticIdentity\" for provider \"azure\"",
 		},
 	}
-)
-
-func TestManagedClusterValidateCreate(t *testing.T) {
-	g := NewWithT(t)
-
-	ctx := admission.NewContextWithRequest(context.Background(), admission.Request{
-		AdmissionRequest: admissionv1.AdmissionRequest{
-			Operation: admissionv1.Create,
-		},
-	})
-	for _, tt := range createAndUpdateTests {
+	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			c := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(tt.existingObjects...).Build()
 			validator := &ManagedClusterValidator{Client: c}
@@ -262,11 +264,66 @@ func TestManagedClusterValidateUpdate(t *testing.T) {
 			Operation: admissionv1.Update,
 		},
 	})
-	for _, tt := range createAndUpdateTests {
+
+	tests := []struct {
+		name              string
+		oldManagedCluster *v1alpha1.ManagedCluster
+		newManagedCluster *v1alpha1.ManagedCluster
+		existingObjects   []runtime.Object
+		err               string
+		warnings          admission.Warnings
+	}{
+		{
+			name:              "should fail if the new cluster template was found but is invalid (some validation error)",
+			oldManagedCluster: managedcluster.NewManagedCluster(managedcluster.WithClusterTemplate(testTemplateName)),
+			newManagedCluster: managedcluster.NewManagedCluster(managedcluster.WithClusterTemplate(newTemplateName)),
+			existingObjects: []runtime.Object{
+				mgmt,
+				template.NewClusterTemplate(
+					template.WithName(newTemplateName),
+					template.WithValidationStatus(v1alpha1.TemplateValidationStatus{
+						Valid:           false,
+						ValidationError: "validation error example",
+					}),
+				),
+			},
+			err: "the ManagedCluster is invalid: the template is not valid: validation error example",
+		},
+		{
+			name: "should succeed if template is not changed",
+			oldManagedCluster: managedcluster.NewManagedCluster(
+				managedcluster.WithClusterTemplate(testTemplateName),
+				managedcluster.WithConfig(`{"foo":"bar"}`),
+				managedcluster.WithCredential(testCredentialName),
+			),
+			newManagedCluster: managedcluster.NewManagedCluster(
+				managedcluster.WithClusterTemplate(testTemplateName),
+				managedcluster.WithConfig(`{"a":"b"}`),
+				managedcluster.WithCredential(testCredentialName),
+			),
+			existingObjects: []runtime.Object{
+				mgmt,
+				cred,
+				template.NewClusterTemplate(
+					template.WithName(testTemplateName),
+					template.WithValidationStatus(v1alpha1.TemplateValidationStatus{
+						Valid:           false,
+						ValidationError: "validation error example",
+					}),
+					template.WithProvidersStatus(v1alpha1.ProvidersTupled{
+						InfrastructureProviders: []v1alpha1.ProviderTuple{{Name: "aws"}},
+						BootstrapProviders:      []v1alpha1.ProviderTuple{{Name: "k0s"}},
+						ControlPlaneProviders:   []v1alpha1.ProviderTuple{{Name: "k0s"}},
+					}),
+				),
+			},
+		},
+	}
+	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			c := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(tt.existingObjects...).Build()
 			validator := &ManagedClusterValidator{Client: c}
-			warn, err := validator.ValidateUpdate(ctx, managedcluster.NewManagedCluster(), tt.managedCluster)
+			warn, err := validator.ValidateUpdate(ctx, tt.oldManagedCluster, tt.newManagedCluster)
 			if tt.err != "" {
 				g.Expect(err).To(HaveOccurred())
 				if err.Error() != tt.err {