From 989b6f9f42314b578375547d00501428f21adcbc Mon Sep 17 00:00:00 2001 From: Ekaterina Kazakova Date: Fri, 18 Oct 2024 16:29:29 +0400 Subject: [PATCH] Improve ManagedCluster update validation Validate template only in case it was updated in the cluster's spec Closes #244 --- internal/webhook/managedcluster_webhook.go | 19 ++-- .../webhook/managedcluster_webhook_test.go | 87 +++++++++++++++---- 2 files changed, 84 insertions(+), 22 deletions(-) diff --git a/internal/webhook/managedcluster_webhook.go b/internal/webhook/managedcluster_webhook.go index f095d8769..18b66b0eb 100644 --- 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 index 1dc4589b8..4bd961898 100644 --- 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 {