From 8f2caaa3c572bde5c9653b252f02eb60fa649f7d Mon Sep 17 00:00:00 2001
From: Ekaterina Kazakova <eromanova@mirantis.com>
Date: Wed, 4 Sep 2024 15:54:58 +0400
Subject: [PATCH] Check ManagedCluster template validness only in case it was
 changed

Closes #244
---
 internal/webhook/managedcluster_webhook.go    | 22 ++++---
 .../webhook/managedcluster_webhook_test.go    | 57 +++++++++++++++++++
 2 files changed, 71 insertions(+), 8 deletions(-)

diff --git a/internal/webhook/managedcluster_webhook.go b/internal/webhook/managedcluster_webhook.go
index 857a9a92..27d826df 100644
--- a/internal/webhook/managedcluster_webhook.go
+++ b/internal/webhook/managedcluster_webhook.go
@@ -77,18 +77,24 @@ 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.(*v1alpha1.ManagedCluster)
+	if !ok {
+		return nil, apierrors.NewBadRequest(fmt.Sprintf("expected ManagedCluster but got a %T", oldObj))
+	}
 	newManagedCluster, ok := newObj.(*v1alpha1.ManagedCluster)
 	if !ok {
 		return nil, apierrors.NewBadRequest(fmt.Sprintf("expected ManagedCluster but got a %T", newObj))
 	}
-	template, err := v.getTemplate(ctx, newManagedCluster.Spec.Template)
-	if err != nil {
-		return nil, fmt.Errorf("%s: %v", InvalidManagedClusterErr, err)
-	}
-	err = v.isTemplateValid(template)
-	if err != nil {
-		return nil, fmt.Errorf("%s: %v", InvalidManagedClusterErr, err)
+	if oldManagedCluster.Spec.Template != newManagedCluster.Spec.Template {
+		template, err := v.getTemplate(ctx, newManagedCluster.Spec.Template)
+		if err != nil {
+			return nil, fmt.Errorf("%s: %v", InvalidManagedClusterErr, err)
+		}
+		err = v.isTemplateValid(template)
+		if err != nil {
+			return nil, fmt.Errorf("%s: %v", InvalidManagedClusterErr, err)
+		}
 	}
 	return nil, nil
 }
diff --git a/internal/webhook/managedcluster_webhook_test.go b/internal/webhook/managedcluster_webhook_test.go
index bc41cf61..ed3c508f 100644
--- a/internal/webhook/managedcluster_webhook_test.go
+++ b/internal/webhook/managedcluster_webhook_test.go
@@ -275,6 +275,63 @@ cluster-api installation failed: cluster-api template is invalid`,
 			},
 			err: "the ManagedCluster is invalid: the template is not valid: validation error example",
 		},
+		{
+			name:      "update - should succeed if another field than the `spec.template` was changed even if the old template is not valid",
+			operation: admissionv1.Update,
+			oldManagedCluster: managedcluster.NewManagedCluster(
+				managedcluster.WithTemplate(testTemplateName),
+				managedcluster.WithConfig(`{"foo":"foo"}`),
+			),
+			newManagedCluster: managedcluster.NewManagedCluster(
+				managedcluster.WithTemplate(testTemplateName),
+				managedcluster.WithConfig(`{"foo":"bar"}`),
+			),
+			existingObjects: []runtime.Object{
+				management.NewManagement(
+					management.WithAvailableProviders(v1alpha1.Providers{
+						BootstrapProviders:    []string{"k0s"},
+						ControlPlaneProviders: []string{"k0s"},
+					}),
+					management.WithComponentsStatus(map[string]v1alpha1.ComponentStatus{
+						v1alpha1.DefaultCoreHMCTemplate:  {Success: false},
+						v1alpha1.DefaultCoreCAPITemplate: {Success: true},
+					}),
+				),
+				template.NewTemplate(
+					template.WithName(capzTemplateName),
+					template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: false}),
+				),
+				template.NewTemplate(
+					template.WithName(newTemplateName),
+					template.WithTypeStatus(v1alpha1.TemplateTypeDeployment),
+					template.WithProvidersStatus(v1alpha1.Providers{
+						InfrastructureProviders: []string{"aws"},
+						BootstrapProviders:      []string{"k0s"},
+						ControlPlaneProviders:   []string{"k0s"},
+					}),
+					template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: false}),
+				),
+			},
+		},
+		{
+			name:              "update - should succeed",
+			operation:         admissionv1.Update,
+			oldManagedCluster: managedcluster.NewManagedCluster(managedcluster.WithTemplate(testTemplateName)),
+			newManagedCluster: managedcluster.NewManagedCluster(managedcluster.WithTemplate(newTemplateName)),
+			existingObjects: []runtime.Object{
+				mgmt,
+				template.NewTemplate(
+					template.WithName(newTemplateName),
+					template.WithTypeStatus(v1alpha1.TemplateTypeDeployment),
+					template.WithProvidersStatus(v1alpha1.Providers{
+						InfrastructureProviders: []string{"aws"},
+						BootstrapProviders:      []string{"k0s"},
+						ControlPlaneProviders:   []string{"k0s"},
+					}),
+					template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}),
+				),
+			},
+		},
 	}
 
 	for _, tt := range tests {