From 59131c22fe0741e05a6487bbfc44343b80a97322 Mon Sep 17 00:00:00 2001 From: Wahab Ali Date: Tue, 5 Nov 2024 09:50:12 -0500 Subject: [PATCH] Validation for MultiClusterService --- api/v1alpha1/common.go | 32 ++++- internal/webhook/managedcluster_webhook.go | 36 +++++- .../webhook/managedcluster_webhook_test.go | 68 ++++++++++- .../webhook/multiclusterservice_webhook.go | 111 ++++++++++++++++++ .../multiclusterservice_webhook_test.go | 108 +++++++++++++++++ internal/webhook/template_webhook.go | 15 +++ internal/webhook/template_webhook_test.go | 17 ++- .../multiclusterservice.go | 54 +++++++++ 8 files changed, 429 insertions(+), 12 deletions(-) create mode 100644 internal/webhook/multiclusterservice_webhook.go create mode 100644 internal/webhook/multiclusterservice_webhook_test.go create mode 100644 test/objects/multiclusterservice/multiclusterservice.go diff --git a/api/v1alpha1/common.go b/api/v1alpha1/common.go index ddd02be9a..4f23547da 100644 --- a/api/v1alpha1/common.go +++ b/api/v1alpha1/common.go @@ -79,6 +79,10 @@ func SetupIndexers(ctx context.Context, mgr ctrl.Manager) error { return err } + if err := SetupMultiClusterServiceServicesIndexer(ctx, mgr); err != nil { + return err + } + if err := SetupClusterTemplateChainIndexer(ctx, mgr); err != nil { return err } @@ -131,10 +135,10 @@ func ExtractReleaseTemplates(rawObj client.Object) []string { const ServicesTemplateKey = ".spec.services[].Template" func SetupManagedClusterServicesIndexer(ctx context.Context, mgr ctrl.Manager) error { - return mgr.GetFieldIndexer().IndexField(ctx, &ManagedCluster{}, ServicesTemplateKey, ExtractServiceTemplateName) + return mgr.GetFieldIndexer().IndexField(ctx, &ManagedCluster{}, ServicesTemplateKey, ExtractServiceTemplateFromManagedCluster) } -func ExtractServiceTemplateName(rawObj client.Object) []string { +func ExtractServiceTemplateFromManagedCluster(rawObj client.Object) []string { cluster, ok := rawObj.(*ManagedCluster) if !ok { return nil @@ -148,6 +152,30 @@ func ExtractServiceTemplateName(rawObj client.Object) []string { return templates } +// const ServicesTemplateKey2 = ".spec.services[].Template2" + +func SetupMultiClusterServiceServicesIndexer(ctx context.Context, mgr ctrl.Manager) error { + return mgr.GetFieldIndexer().IndexField(ctx, &MultiClusterService{}, ServicesTemplateKey, ExtractServiceTemplateFromMultiClusterService) +} + +func ExtractServiceTemplateFromMultiClusterService(rawObj client.Object) []string { + cluster, ok := rawObj.(*MultiClusterService) + if !ok { + return nil + } + + templates := []string{} + for _, s := range cluster.Spec.Services { + templates = append(templates, s.Template) + } + + return templates +} + +// func SetupMultiClusterSerivceServicesIndexer(ctx context.Context, mgr ctrl.Manager) error { +// return mgr.GetFieldIndexer().IndexField(ctx, &MultiClusterService{}, ServicesTemplateKey, ExtractServiceTemplateName) +// } + const SupportedTemplateKey = ".spec.supportedTemplates[].Name" func SetupClusterTemplateChainIndexer(ctx context.Context, mgr ctrl.Manager) error { diff --git a/internal/webhook/managedcluster_webhook.go b/internal/webhook/managedcluster_webhook.go index 25970c588..b2c2b2be4 100644 --- a/internal/webhook/managedcluster_webhook.go +++ b/internal/webhook/managedcluster_webhook.go @@ -67,7 +67,7 @@ func (v *ManagedClusterValidator) ValidateCreate(ctx context.Context, obj runtim return nil, fmt.Errorf("%s: %v", invalidManagedClusterMsg, err) } - if err := isTemplateValid(template); err != nil { + if err := isTemplateValid(template.GetCommonStatus()); err != nil { return nil, fmt.Errorf("%s: %v", invalidManagedClusterMsg, err) } @@ -79,6 +79,18 @@ func (v *ManagedClusterValidator) ValidateCreate(ctx context.Context, obj runtim return nil, fmt.Errorf("%s: %v", invalidManagedClusterMsg, err) } + // validate services + for _, svc := range managedCluster.Spec.Services { + tpl, err := getServiceTemplate(ctx, v.Client, managedCluster.Namespace, svc.Template) + if err != nil { + return nil, fmt.Errorf("%s: %v", invalidManagedClusterMsg, err) + } + + if err := isTemplateValid(tpl.GetCommonStatus()); err != nil { + return nil, fmt.Errorf("%s: %v", invalidManagedClusterMsg, err) + } + } + return nil, nil } @@ -106,7 +118,7 @@ func (v *ManagedClusterValidator) ValidateUpdate(ctx context.Context, oldObj, ne return admission.Warnings{msg}, errClusterUpgradeForbidden } - if err := isTemplateValid(template); err != nil { + if err := isTemplateValid(template.GetCommonStatus()); err != nil { return nil, fmt.Errorf("%s: %v", invalidManagedClusterMsg, err) } @@ -119,6 +131,18 @@ func (v *ManagedClusterValidator) ValidateUpdate(ctx context.Context, oldObj, ne return nil, fmt.Errorf("%s: %v", invalidManagedClusterMsg, err) } + // // validate services + // for _, svc := range newManagedCluster.Spec.Services { + // tpl, err := getServiceTemplate(ctx, v.Client, newManagedCluster.Namespace, svc.Template) + // if err != nil { + // return nil, fmt.Errorf("%s: %v", invalidManagedClusterMsg, err) + // } + + // if err := isTemplateValid(tpl.GetCommonStatus()); err != nil { + // return nil, fmt.Errorf("%s: %v", invalidManagedClusterMsg, err) + // } + // } + return nil, nil } @@ -185,7 +209,7 @@ func (v *ManagedClusterValidator) Default(ctx context.Context, obj runtime.Objec return fmt.Errorf("could not get template for the managedcluster: %v", err) } - if err := isTemplateValid(template); err != nil { + if err := isTemplateValid(template.GetCommonStatus()); err != nil { return fmt.Errorf("template is invalid: %v", err) } @@ -216,9 +240,9 @@ func (v *ManagedClusterValidator) getManagedClusterCredential(ctx context.Contex return cred, nil } -func isTemplateValid(template *hmcv1alpha1.ClusterTemplate) error { - if !template.Status.Valid { - return fmt.Errorf("the template is not valid: %s", template.Status.ValidationError) +func isTemplateValid(status *hmcv1alpha1.TemplateStatusCommon) error { + if !status.Valid { + return fmt.Errorf("the template is not valid: %s", status.ValidationError) } return nil diff --git a/internal/webhook/managedcluster_webhook_test.go b/internal/webhook/managedcluster_webhook_test.go index 4d8cc84a5..398b3e9c9 100644 --- a/internal/webhook/managedcluster_webhook_test.go +++ b/internal/webhook/managedcluster_webhook_test.go @@ -35,9 +35,10 @@ import ( ) var ( - testTemplateName = "template-test" - testCredentialName = "cred-test" - newTemplateName = "new-template-name" + testTemplateName = "template-test" + testSvcTemplateName = "svc-template-test" + testCredentialName = "cred-test" + newTemplateName = "new-template-name" testNamespace = "test" @@ -97,6 +98,32 @@ func TestManagedClusterValidateCreate(t *testing.T) { }, err: fmt.Sprintf("the ManagedCluster is invalid: clustertemplates.hmc.mirantis.com \"%s\" not found", testTemplateName), }, + { + name: "should fail if the ServiceTemplates are not found in same namespace", + managedCluster: managedcluster.NewManagedCluster( + managedcluster.WithClusterTemplate(testTemplateName), + managedcluster.WithCredential(testCredentialName), + managedcluster.WithServiceTemplate(testSvcTemplateName), + ), + existingObjects: []runtime.Object{ + mgmt, + cred, + template.NewClusterTemplate( + template.WithName(testTemplateName), + template.WithProvidersStatus(v1alpha1.Providers{ + "infrastructure-aws", + "control-plane-k0smotron", + "bootstrap-k0smotron", + }), + template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), + ), + template.NewServiceTemplate( + template.WithName(testSvcTemplateName), + template.WithNamespace("othernamespace"), + ), + }, + err: fmt.Sprintf("the ManagedCluster is invalid: servicetemplates.hmc.mirantis.com \"%s\" not found", testSvcTemplateName), + }, { name: "should fail if the cluster template was found but is invalid (some validation error)", managedCluster: managedcluster.NewManagedCluster( @@ -116,11 +143,41 @@ func TestManagedClusterValidateCreate(t *testing.T) { }, err: "the ManagedCluster is invalid: the template is not valid: validation error example", }, + { + name: "should fail if the service templates were found but are invalid (some validation error)", + managedCluster: managedcluster.NewManagedCluster( + managedcluster.WithClusterTemplate(testTemplateName), + managedcluster.WithCredential(testCredentialName), + managedcluster.WithServiceTemplate(testSvcTemplateName), + ), + existingObjects: []runtime.Object{ + mgmt, + cred, + template.NewClusterTemplate( + template.WithName(testTemplateName), + template.WithProvidersStatus(v1alpha1.Providers{ + "infrastructure-aws", + "control-plane-k0smotron", + "bootstrap-k0smotron", + }), + template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), + ), + template.NewServiceTemplate( + template.WithName(testSvcTemplateName), + 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", managedCluster: managedcluster.NewManagedCluster( managedcluster.WithClusterTemplate(testTemplateName), managedcluster.WithCredential(testCredentialName), + managedcluster.WithServiceTemplate(testSvcTemplateName), ), existingObjects: []runtime.Object{ mgmt, @@ -134,6 +191,11 @@ func TestManagedClusterValidateCreate(t *testing.T) { }), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), ), + template.NewServiceTemplate( + template.WithName(testSvcTemplateName), + // template.WithNamespace(testNamespace), + template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), + ), }, }, { diff --git a/internal/webhook/multiclusterservice_webhook.go b/internal/webhook/multiclusterservice_webhook.go new file mode 100644 index 000000000..70f872769 --- /dev/null +++ b/internal/webhook/multiclusterservice_webhook.go @@ -0,0 +1,111 @@ +// Copyright 2024 +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package webhook + +import ( + "context" + "fmt" + + "github.com/Mirantis/hmc/api/v1alpha1" + "github.com/Mirantis/hmc/internal/utils" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +type MultiClusterServiceValidator struct { + client.Client +} + +const invalidMultiClusterServiceMsg = "the MultiClusterService is invalid" + +// SetupWebhookWithManager will setup the manager to manage the webhooks +func (v *MultiClusterServiceValidator) SetupWebhookWithManager(mgr ctrl.Manager) error { + v.Client = mgr.GetClient() + return ctrl.NewWebhookManagedBy(mgr). + For(&v1alpha1.MultiClusterService{}). + WithValidator(v). + WithDefaulter(v). + Complete() +} + +var ( + _ webhook.CustomValidator = &MultiClusterServiceValidator{} + _ webhook.CustomDefaulter = &MultiClusterServiceValidator{} +) + +// Default implements webhook.Defaulter so a webhook will be registered for the type. +func (*MultiClusterServiceValidator) Default(_ context.Context, _ runtime.Object) error { + return nil +} + +// ValidateCreate implements webhook.Validator so a webhook will be registered for the type. +func (v *MultiClusterServiceValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + mcs, ok := obj.(*v1alpha1.MultiClusterService) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected MultiClusterService but got a %T", obj)) + } + + for _, svc := range mcs.Spec.Services { + tpl, err := getServiceTemplate(ctx, v.Client, utils.DefaultSystemNamespace, svc.Template) + if err != nil { + return nil, fmt.Errorf("%s: %v", invalidMultiClusterServiceMsg, err) + } + + if err := isTemplateValid(tpl.GetCommonStatus()); err != nil { + return nil, fmt.Errorf("%s: %v", invalidMultiClusterServiceMsg, err) + } + } + + return nil, nil +} + +// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. +func (v *MultiClusterServiceValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + _, ok := oldObj.(*v1alpha1.MultiClusterService) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected MultiClusterService but got a %T", oldObj)) + } + mcs, ok := newObj.(*v1alpha1.MultiClusterService) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected MultiClusterService but got a %T", newObj)) + } + + for _, svc := range mcs.Spec.Services { + tpl, err := getServiceTemplate(ctx, v.Client, utils.DefaultSystemNamespace, svc.Template) + if err != nil { + return nil, fmt.Errorf("%s: %v", invalidMultiClusterServiceMsg, err) + } + + if err := isTemplateValid(tpl.GetCommonStatus()); err != nil { + return nil, fmt.Errorf("%s: %v", invalidMultiClusterServiceMsg, err) + } + } + + return nil, nil +} + +// ValidateDelete implements webhook.Validator so a webhook will be registered for the type. +func (*MultiClusterServiceValidator) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { + return nil, nil +} + +func getServiceTemplate(ctx context.Context, c client.Client, templateNamespace, templateName string) (tpl *v1alpha1.ServiceTemplate, err error) { + tpl = new(v1alpha1.ServiceTemplate) + return tpl, c.Get(ctx, client.ObjectKey{Namespace: templateNamespace, Name: templateName}, tpl) +} diff --git a/internal/webhook/multiclusterservice_webhook_test.go b/internal/webhook/multiclusterservice_webhook_test.go new file mode 100644 index 000000000..f22bc8e4c --- /dev/null +++ b/internal/webhook/multiclusterservice_webhook_test.go @@ -0,0 +1,108 @@ +package webhook + +import ( + "context" + "fmt" + "testing" + + "github.com/Mirantis/hmc/api/v1alpha1" + "github.com/Mirantis/hmc/internal/utils" + "github.com/Mirantis/hmc/test/objects/multiclusterservice" + "github.com/Mirantis/hmc/test/objects/template" + "github.com/Mirantis/hmc/test/scheme" + + . "github.com/onsi/gomega" + admissionv1 "k8s.io/api/admission/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +func TestMultiClusterServiceValidateCreate(t *testing.T) { + ctx := admission.NewContextWithRequest(context.Background(), admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + }, + }) + + testMCSName := "testmcs" + testTplName := "testtemplate" + + tests := []struct { + name string + mcs *v1alpha1.MultiClusterService + existingObjects []runtime.Object + err string + warnings admission.Warnings + }{ + { + name: "should fail if the ServiceTemplates are not found in hmc-system namespace", + mcs: multiclusterservice.NewMultiClusterService( + multiclusterservice.WithName(testMCSName), + multiclusterservice.WithServiceTemplate(testTplName), + ), + existingObjects: []runtime.Object{ + template.NewServiceTemplate( + template.WithName(testTplName), + template.WithNamespace("othernamespace"), + ), + }, + err: fmt.Sprintf("the MultiClusterService is invalid: servicetemplates.hmc.mirantis.com \"%s\" not found", testTplName), + }, + { + name: "should fail if the ServiceTemplates were found but are invalid", + mcs: multiclusterservice.NewMultiClusterService( + multiclusterservice.WithName(testMCSName), + multiclusterservice.WithServiceTemplate(testTplName), + ), + existingObjects: []runtime.Object{ + template.NewServiceTemplate( + template.WithName(testTplName), + template.WithNamespace(utils.DefaultSystemNamespace), + template.WithValidationStatus(v1alpha1.TemplateValidationStatus{ + Valid: false, + ValidationError: "validation error example", + }), + ), + }, + err: "the MultiClusterService is invalid: the template is not valid: validation error example", + }, + + { + name: "should succeed", + mcs: multiclusterservice.NewMultiClusterService( + multiclusterservice.WithName(testMCSName), + multiclusterservice.WithServiceTemplate(testTplName), + ), + existingObjects: []runtime.Object{ + template.NewServiceTemplate( + template.WithName(testTplName), + template.WithNamespace(utils.DefaultSystemNamespace), + template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), + ), + }, + }, + } + + for _, tt := range tests { + g := NewWithT(t) + t.Run(tt.name, func(t *testing.T) { + c := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(tt.existingObjects...).Build() + validator := &MultiClusterServiceValidator{Client: c} + warn, err := validator.ValidateCreate(ctx, tt.mcs) + // fmt.Printf(">>>>> warn=%s\n", warn) + // fmt.Printf(">>>>> err=%s\n", err) + if tt.err != "" { + g.Expect(err).To(MatchError(tt.err)) + } else { + g.Expect(err).To(Succeed()) + } + + if len(tt.warnings) > 0 { + g.Expect(warn).To(Equal(tt.warnings)) + } else { + g.Expect(warn).To(BeEmpty()) + } + }) + } +} diff --git a/internal/webhook/template_webhook.go b/internal/webhook/template_webhook.go index b2a0dacd8..6666c2575 100644 --- a/internal/webhook/template_webhook.go +++ b/internal/webhook/template_webhook.go @@ -27,6 +27,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "github.com/Mirantis/hmc/api/v1alpha1" + "github.com/Mirantis/hmc/internal/utils" ) type ClusterTemplateValidator struct { @@ -133,6 +134,20 @@ func (v *ServiceTemplateValidator) ValidateDelete(ctx context.Context, obj runti return admission.Warnings{"The ServiceTemplate object can't be removed if ManagedCluster objects referencing it still exist"}, errTemplateDeletionForbidden } + // MultiClusterServices can only refer to serviceTemplates in hmc-system namespace. + if tmpl.Namespace == utils.DefaultSystemNamespace { + multiSvcClusters := &v1alpha1.MultiClusterServiceList{} + if err := v.Client.List(ctx, multiSvcClusters, + client.MatchingFields{v1alpha1.ServicesTemplateKey: tmpl.Name}, + client.Limit(1)); err != nil { + return nil, err + } + + if len(multiSvcClusters.Items) > 0 { + return admission.Warnings{"The ServiceTemplate object can't be removed if MultiClusterService objects referencing it still exist"}, errTemplateDeletionForbidden + } + } + return nil, nil } diff --git a/internal/webhook/template_webhook_test.go b/internal/webhook/template_webhook_test.go index 44938a4cc..962a97560 100644 --- a/internal/webhook/template_webhook_test.go +++ b/internal/webhook/template_webhook_test.go @@ -24,7 +24,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "github.com/Mirantis/hmc/api/v1alpha1" + "github.com/Mirantis/hmc/internal/utils" "github.com/Mirantis/hmc/test/objects/managedcluster" + "github.com/Mirantis/hmc/test/objects/multiclusterservice" "github.com/Mirantis/hmc/test/objects/template" "github.com/Mirantis/hmc/test/scheme" ) @@ -136,6 +138,18 @@ func TestServiceTemplateValidateDelete(t *testing.T) { template: tmpl, existingObjects: []runtime.Object{managedcluster.NewManagedCluster()}, }, + { + title: "should fail if a MultiClusterService is referencing serviceTemplate in hmc-system namespace", + template: template.NewServiceTemplate(template.WithNamespace(utils.DefaultSystemNamespace), template.WithName(tmpl.Name)), + existingObjects: []runtime.Object{ + multiclusterservice.NewMultiClusterService( + multiclusterservice.WithName("mymulticlusterservice"), + multiclusterservice.WithServiceTemplate(tmpl.Name), + ), + }, + warnings: admission.Warnings{"The ServiceTemplate object can't be removed if MultiClusterService objects referencing it still exist"}, + err: errTemplateDeletionForbidden.Error(), + }, } for _, tt := range tests { @@ -146,7 +160,8 @@ func TestServiceTemplateValidateDelete(t *testing.T) { NewClientBuilder(). WithScheme(scheme.Scheme). WithRuntimeObjects(tt.existingObjects...). - WithIndex(tt.existingObjects[0], v1alpha1.ServicesTemplateKey, v1alpha1.ExtractServiceTemplateName). + WithIndex(&v1alpha1.ManagedCluster{}, v1alpha1.ServicesTemplateKey, v1alpha1.ExtractServiceTemplateFromManagedCluster). + WithIndex(&v1alpha1.MultiClusterService{}, v1alpha1.ServicesTemplateKey, v1alpha1.ExtractServiceTemplateFromMultiClusterService). Build() validator := &ServiceTemplateValidator{Client: c} warn, err := validator.ValidateDelete(ctx, tt.template) diff --git a/test/objects/multiclusterservice/multiclusterservice.go b/test/objects/multiclusterservice/multiclusterservice.go new file mode 100644 index 000000000..cc6aec977 --- /dev/null +++ b/test/objects/multiclusterservice/multiclusterservice.go @@ -0,0 +1,54 @@ +// Copyright 2024 +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package multiclusterservice + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/Mirantis/hmc/api/v1alpha1" +) + +const ( + DefaultName = "multiclusterservice" +) + +type Opt func(multiClusterService *v1alpha1.MultiClusterService) + +func NewMultiClusterService(opts ...Opt) *v1alpha1.MultiClusterService { + p := &v1alpha1.MultiClusterService{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultName, + }, + } + + for _, opt := range opts { + opt(p) + } + return p +} + +func WithName(name string) Opt { + return func(p *v1alpha1.MultiClusterService) { + p.Name = name + } +} + +func WithServiceTemplate(templateName string) Opt { + return func(p *v1alpha1.MultiClusterService) { + p.Spec.Services = append(p.Spec.Services, v1alpha1.ServiceSpec{ + Template: templateName, + }) + } +}