From b3f40371321146c5baf0b8df2cbd74b0ed89d8d7 Mon Sep 17 00:00:00 2001 From: Wahab Ali Date: Thu, 7 Nov 2024 20:58:48 -0500 Subject: [PATCH] Don't use hardcoded system namespace in MultiClusterService --- api/v1alpha1/multiclusterservice_types.go | 2 +- cmd/main.go | 5 ++-- .../multiclusterservice_controller.go | 9 +++--- .../multiclusterservice_controller_test.go | 28 +++++++++--------- internal/controller/suite_test.go | 3 +- .../webhook/multiclusterservice_webhook.go | 18 ++++++------ .../multiclusterservice_webhook_test.go | 29 +++++++++---------- ...hmc.mirantis.com_multiclusterservices.yaml | 2 +- 8 files changed, 48 insertions(+), 48 deletions(-) diff --git a/api/v1alpha1/multiclusterservice_types.go b/api/v1alpha1/multiclusterservice_types.go index 31be9c0db..fe575294f 100644 --- a/api/v1alpha1/multiclusterservice_types.go +++ b/api/v1alpha1/multiclusterservice_types.go @@ -100,7 +100,7 @@ type ServiceStatus struct { type MultiClusterServiceStatus struct { // Services contains details for the state of services. Services []ServiceStatus `json:"services,omitempty"` - // Conditions contains details for the current state of the ManagedCluster + // Conditions contains details for the current state of the MultiClusterService. Conditions []metav1.Condition `json:"conditions,omitempty"` // ObservedGeneration is the last observed generation. ObservedGeneration int64 `json:"observedGeneration,omitempty"` diff --git a/cmd/main.go b/cmd/main.go index cc7eb777a..3bb9fa0da 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -296,7 +296,8 @@ func main() { } if err = (&controller.MultiClusterServiceReconciler{ - Client: mgr.GetClient(), + Client: mgr.GetClient(), + SystemNamespace: currentNamespace, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "MultiClusterService") os.Exit(1) @@ -331,7 +332,7 @@ func setupWebhooks(mgr ctrl.Manager, currentNamespace string) error { setupLog.Error(err, "unable to create webhook", "webhook", "ManagedCluster") return err } - if err := (&hmcwebhook.MultiClusterServiceValidator{}).SetupWebhookWithManager(mgr); err != nil { + if err := (&hmcwebhook.MultiClusterServiceValidator{SystemNamespace: currentNamespace}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "MultiClusterService") return err } diff --git a/internal/controller/multiclusterservice_controller.go b/internal/controller/multiclusterservice_controller.go index 38136cf99..9334efe86 100644 --- a/internal/controller/multiclusterservice_controller.go +++ b/internal/controller/multiclusterservice_controller.go @@ -44,6 +44,7 @@ import ( // MultiClusterServiceReconciler reconciles a MultiClusterService object type MultiClusterServiceReconciler struct { client.Client + SystemNamespace string } // Reconcile reconciles a MultiClusterService object. @@ -115,8 +116,8 @@ func (r *MultiClusterServiceReconciler) reconcileUpdate(ctx context.Context, mcs } // By using DefaultSystemNamespace we are enforcing that MultiClusterService - // may only use ServiceTemplates that are present in the hmc-system namespace. - opts, err := helmChartOpts(ctx, r.Client, utils.DefaultSystemNamespace, mcs.Spec.Services) + // may only use ServiceTemplates that are present in the system namespace. + opts, err := helmChartOpts(ctx, r.Client, r.SystemNamespace, mcs.Spec.Services) if err != nil { return ctrl.Result{}, err } @@ -179,7 +180,7 @@ func helmChartOpts(ctx context.Context, c client.Client, namespace string, servi // Here we can use the same namespace for all services // because if the services slice is part of: // 1. ManagedCluster: Then the referred template must be in its own namespace. - // 2. MultiClusterService: Then the referred template must be in hmc-system namespace. + // 2. MultiClusterService: Then the referred template must be in system namespace. tmplRef := client.ObjectKey{Name: svc.Template, Namespace: namespace} if err := c.Get(ctx, tmplRef, tmpl); err != nil { return nil, fmt.Errorf("failed to get ServiceTemplate %s: %w", tmplRef.String(), err) @@ -356,7 +357,7 @@ func requeueSveltosProfileForClusterSummary(ctx context.Context, obj client.Obje cs, ok := obj.(*sveltosv1beta1.ClusterSummary) if !ok { - l.Error(errors.New("request is not for a ClusterSummary object"), msg, "ClusterSummary.Name", obj.GetName(), "ClusterSummary.Namespace", obj.GetNamespace()) + l.Error(errors.New("request is not for a ClusterSummary object"), msg, "Requested.Name", obj.GetName(), "Requested.Namespace", obj.GetNamespace()) return []ctrl.Request{} } diff --git a/internal/controller/multiclusterservice_controller_test.go b/internal/controller/multiclusterservice_controller_test.go index 9dc480164..817eba6f5 100644 --- a/internal/controller/multiclusterservice_controller_test.go +++ b/internal/controller/multiclusterservice_controller_test.go @@ -31,13 +31,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" hmc "github.com/Mirantis/hmc/api/v1alpha1" - "github.com/Mirantis/hmc/internal/utils" ) var _ = Describe("MultiClusterService Controller", func() { Context("When reconciling a resource", func() { const ( - testNamespace = utils.DefaultSystemNamespace serviceTemplateName = "test-service-0-1-0" helmRepoName = "test-helmrepo" helmChartName = "test-helmchart" @@ -66,31 +64,31 @@ var _ = Describe("MultiClusterService Controller", func() { multiClusterService := &hmc.MultiClusterService{} clusterProfile := &sveltosv1beta1.ClusterProfile{} - helmRepositoryRef := types.NamespacedName{Namespace: testNamespace, Name: helmRepoName} - helmChartRef := types.NamespacedName{Namespace: testNamespace, Name: helmChartName} - serviceTemplateRef := types.NamespacedName{Namespace: testNamespace, Name: serviceTemplateName} + helmRepositoryRef := types.NamespacedName{Namespace: testSystemNamespace, Name: helmRepoName} + helmChartRef := types.NamespacedName{Namespace: testSystemNamespace, Name: helmChartName} + serviceTemplateRef := types.NamespacedName{Namespace: testSystemNamespace, Name: serviceTemplateName} multiClusterServiceRef := types.NamespacedName{Name: multiClusterServiceName} clusterProfileRef := types.NamespacedName{Name: multiClusterServiceName} BeforeEach(func() { By("creating Namespace") - err := k8sClient.Get(ctx, types.NamespacedName{Name: testNamespace}, namespace) + err := k8sClient.Get(ctx, types.NamespacedName{Name: testSystemNamespace}, namespace) if err != nil && apierrors.IsNotFound(err) { namespace = &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: testNamespace, + Name: testSystemNamespace, }, } Expect(k8sClient.Create(ctx, namespace)).To(Succeed()) } By("creating HelmRepository") - err = k8sClient.Get(ctx, types.NamespacedName{Name: helmRepoName, Namespace: testNamespace}, helmRepo) + err = k8sClient.Get(ctx, types.NamespacedName{Name: helmRepoName, Namespace: testSystemNamespace}, helmRepo) if err != nil && apierrors.IsNotFound(err) { helmRepo = &sourcev1.HelmRepository{ ObjectMeta: metav1.ObjectMeta{ Name: helmRepoName, - Namespace: testNamespace, + Namespace: testSystemNamespace, }, Spec: sourcev1.HelmRepositorySpec{ URL: "oci://test/helmrepo", @@ -100,12 +98,12 @@ var _ = Describe("MultiClusterService Controller", func() { } By("creating HelmChart") - err = k8sClient.Get(ctx, types.NamespacedName{Name: helmChartName, Namespace: testNamespace}, helmChart) + err = k8sClient.Get(ctx, types.NamespacedName{Name: helmChartName, Namespace: testSystemNamespace}, helmChart) if err != nil && apierrors.IsNotFound(err) { helmChart = &sourcev1.HelmChart{ ObjectMeta: metav1.ObjectMeta{ Name: helmChartName, - Namespace: testNamespace, + Namespace: testSystemNamespace, }, Spec: sourcev1.HelmChartSpec{ SourceRef: sourcev1.LocalHelmChartSourceReference{ @@ -131,7 +129,7 @@ var _ = Describe("MultiClusterService Controller", func() { serviceTemplate = &hmc.ServiceTemplate{ ObjectMeta: metav1.ObjectMeta{ Name: serviceTemplateName, - Namespace: testNamespace, + Namespace: testSystemNamespace, Labels: map[string]string{ hmc.HMCManagedLabelKey: "true", }, @@ -142,7 +140,7 @@ var _ = Describe("MultiClusterService Controller", func() { ChartRef: &helmcontrollerv2.CrossNamespaceSourceReference{ Kind: "HelmChart", Name: helmChartName, - Namespace: testNamespace, + Namespace: testSystemNamespace, }, }, }, @@ -195,7 +193,7 @@ var _ = Describe("MultiClusterService Controller", func() { multiClusterServiceResource := &hmc.MultiClusterService{} Expect(k8sClient.Get(ctx, multiClusterServiceRef, multiClusterServiceResource)).NotTo(HaveOccurred()) - reconciler := &MultiClusterServiceReconciler{Client: k8sClient} + reconciler := &MultiClusterServiceReconciler{Client: k8sClient, SystemNamespace: testSystemNamespace} Expect(k8sClient.Delete(ctx, multiClusterService)).To(Succeed()) // Running reconcile to remove the finalizer and delete the MultiClusterService _, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: multiClusterServiceRef}) @@ -219,7 +217,7 @@ var _ = Describe("MultiClusterService Controller", func() { It("should successfully reconcile the resource", func() { By("reconciling MultiClusterService") - multiClusterServiceReconciler := &MultiClusterServiceReconciler{Client: k8sClient} + multiClusterServiceReconciler := &MultiClusterServiceReconciler{Client: k8sClient, SystemNamespace: testSystemNamespace} _, err := multiClusterServiceReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: multiClusterServiceRef}) Expect(err).NotTo(HaveOccurred()) diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 769835950..a372f77b3 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -54,6 +54,7 @@ import ( const ( mutatingWebhookKind = "MutatingWebhookConfiguration" validatingWebhookKind = "ValidatingWebhookConfiguration" + testSystemNamespace = "test-system-namespace" ) var ( @@ -150,7 +151,7 @@ var _ = BeforeSuite(func() { err = (&hmcwebhook.ManagedClusterValidator{}).SetupWebhookWithManager(mgr) Expect(err).NotTo(HaveOccurred()) - err = (&hmcwebhook.MultiClusterServiceValidator{}).SetupWebhookWithManager(mgr) + err = (&hmcwebhook.MultiClusterServiceValidator{SystemNamespace: testSystemNamespace}).SetupWebhookWithManager(mgr) Expect(err).NotTo(HaveOccurred()) err = (&hmcwebhook.ManagementValidator{}).SetupWebhookWithManager(mgr) diff --git a/internal/webhook/multiclusterservice_webhook.go b/internal/webhook/multiclusterservice_webhook.go index ddbb78c2e..fd84a77b0 100644 --- a/internal/webhook/multiclusterservice_webhook.go +++ b/internal/webhook/multiclusterservice_webhook.go @@ -16,6 +16,7 @@ package webhook import ( "context" + "errors" "fmt" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -26,11 +27,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "github.com/Mirantis/hmc/api/v1alpha1" - "github.com/Mirantis/hmc/internal/utils" ) type MultiClusterServiceValidator struct { client.Client + SystemNamespace string } const invalidMultiClusterServiceMsg = "the MultiClusterService is invalid" @@ -62,7 +63,7 @@ func (v *MultiClusterServiceValidator) ValidateCreate(ctx context.Context, obj r return nil, apierrors.NewBadRequest(fmt.Sprintf("expected MultiClusterService but got a %T", obj)) } - if err := validateServices(ctx, v.Client, utils.DefaultSystemNamespace, mcs.Spec.Services); err != nil { + if err := validateServices(ctx, v.Client, v.SystemNamespace, mcs.Spec.Services); err != nil { return nil, fmt.Errorf("%s: %w", invalidMultiClusterServiceMsg, err) } @@ -76,7 +77,7 @@ func (v *MultiClusterServiceValidator) ValidateUpdate(ctx context.Context, _, ne return nil, apierrors.NewBadRequest(fmt.Sprintf("expected MultiClusterService but got a %T", newObj)) } - if err := validateServices(ctx, v.Client, utils.DefaultSystemNamespace, mcs.Spec.Services); err != nil { + if err := validateServices(ctx, v.Client, v.SystemNamespace, mcs.Spec.Services); err != nil { return nil, fmt.Errorf("%s: %w", invalidMultiClusterServiceMsg, err) } @@ -93,17 +94,16 @@ func getServiceTemplate(ctx context.Context, c client.Client, templateNamespace, return tpl, c.Get(ctx, client.ObjectKey{Namespace: templateNamespace, Name: templateName}, tpl) } -func validateServices(ctx context.Context, c client.Client, namespace string, services []v1alpha1.ServiceSpec) error { +func validateServices(ctx context.Context, c client.Client, namespace string, services []v1alpha1.ServiceSpec) (errs error) { for _, svc := range services { tpl, err := getServiceTemplate(ctx, c, namespace, svc.Template) if err != nil { - return err + errs = errors.Join(errs, err) + continue } - if err := isTemplateValid(tpl.GetCommonStatus()); err != nil { - return err - } + errs = errors.Join(errs, isTemplateValid(tpl.GetCommonStatus())) } - return nil + return errs } diff --git a/internal/webhook/multiclusterservice_webhook_test.go b/internal/webhook/multiclusterservice_webhook_test.go index 600b83f3e..38905431c 100644 --- a/internal/webhook/multiclusterservice_webhook_test.go +++ b/internal/webhook/multiclusterservice_webhook_test.go @@ -26,7 +26,6 @@ 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/multiclusterservice" "github.com/Mirantis/hmc/test/objects/template" "github.com/Mirantis/hmc/test/scheme" @@ -36,6 +35,7 @@ const ( testMCSName = "testmcs" testSvcTemplate1Name = "test-servicetemplate-1" testSvcTemplate2Name = "test-servicetemplate-2" + testSystemNamespace = "test-system-namespace" ) func TestMultiClusterServiceValidateCreate(t *testing.T) { @@ -53,7 +53,7 @@ func TestMultiClusterServiceValidateCreate(t *testing.T) { warnings admission.Warnings }{ { - name: "should fail if the ServiceTemplates are not found in hmc-system namespace", + name: "should fail if the ServiceTemplates are not found in system namespace", mcs: multiclusterservice.NewMultiClusterService( multiclusterservice.WithName(testMCSName), multiclusterservice.WithServiceTemplate(testSvcTemplate1Name), @@ -76,7 +76,7 @@ func TestMultiClusterServiceValidateCreate(t *testing.T) { existingObjects: []runtime.Object{ template.NewServiceTemplate( template.WithName(testSvcTemplate1Name), - template.WithNamespace(utils.DefaultSystemNamespace), + template.WithNamespace(testSystemNamespace), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{ Valid: false, ValidationError: "validation error example", @@ -94,7 +94,7 @@ func TestMultiClusterServiceValidateCreate(t *testing.T) { existingObjects: []runtime.Object{ template.NewServiceTemplate( template.WithName(testSvcTemplate1Name), - template.WithNamespace(utils.DefaultSystemNamespace), + template.WithNamespace(testSystemNamespace), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), ), }, @@ -109,12 +109,12 @@ func TestMultiClusterServiceValidateCreate(t *testing.T) { existingObjects: []runtime.Object{ template.NewServiceTemplate( template.WithName(testSvcTemplate1Name), - template.WithNamespace(utils.DefaultSystemNamespace), + template.WithNamespace(testSystemNamespace), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), ), template.NewServiceTemplate( template.WithName(testSvcTemplate2Name), - template.WithNamespace(utils.DefaultSystemNamespace), + template.WithNamespace(testSystemNamespace), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), ), }, @@ -130,9 +130,8 @@ func TestMultiClusterServiceValidateCreate(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - c := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(tt.existingObjects...).Build() - validator := &MultiClusterServiceValidator{Client: c} + validator := &MultiClusterServiceValidator{Client: c, SystemNamespace: testSystemNamespace} warn, err := validator.ValidateCreate(ctx, tt.mcs) if tt.err != "" { g.Expect(err).To(MatchError(tt.err)) @@ -169,7 +168,7 @@ func TestMultiClusterServiceValidateUpdate(t *testing.T) { warnings admission.Warnings }{ { - name: "should fail if the ServiceTemplates are not found in hmc-system namespace", + name: "should fail if the ServiceTemplates are not found in system namespace", newMCS: multiclusterservice.NewMultiClusterService( multiclusterservice.WithName(testMCSName), multiclusterservice.WithServiceTemplate(testSvcTemplate1Name), @@ -178,7 +177,7 @@ func TestMultiClusterServiceValidateUpdate(t *testing.T) { existingObjects: []runtime.Object{ template.NewServiceTemplate( template.WithName(testSvcTemplate1Name), - template.WithNamespace(utils.DefaultSystemNamespace), + template.WithNamespace(testSystemNamespace), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), ), template.NewServiceTemplate( @@ -199,12 +198,12 @@ func TestMultiClusterServiceValidateUpdate(t *testing.T) { existingObjects: []runtime.Object{ template.NewServiceTemplate( template.WithName(testSvcTemplate1Name), - template.WithNamespace(utils.DefaultSystemNamespace), + template.WithNamespace(testSystemNamespace), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), ), template.NewServiceTemplate( template.WithName(testSvcTemplate2Name), - template.WithNamespace(utils.DefaultSystemNamespace), + template.WithNamespace(testSystemNamespace), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{ Valid: false, ValidationError: "validation error example", @@ -223,12 +222,12 @@ func TestMultiClusterServiceValidateUpdate(t *testing.T) { existingObjects: []runtime.Object{ template.NewServiceTemplate( template.WithName(testSvcTemplate1Name), - template.WithNamespace(utils.DefaultSystemNamespace), + template.WithNamespace(testSystemNamespace), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), ), template.NewServiceTemplate( template.WithName(testSvcTemplate2Name), - template.WithNamespace(utils.DefaultSystemNamespace), + template.WithNamespace(testSystemNamespace), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), ), }, @@ -246,7 +245,7 @@ func TestMultiClusterServiceValidateUpdate(t *testing.T) { g := NewWithT(t) c := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(tt.existingObjects...).Build() - validator := &MultiClusterServiceValidator{Client: c} + validator := &MultiClusterServiceValidator{Client: c, SystemNamespace: testSystemNamespace} warn, err := validator.ValidateUpdate(ctx, oldMCS, tt.newMCS) if tt.err != "" { g.Expect(err).To(MatchError(tt.err)) diff --git a/templates/provider/hmc/templates/crds/hmc.mirantis.com_multiclusterservices.yaml b/templates/provider/hmc/templates/crds/hmc.mirantis.com_multiclusterservices.yaml index 1cbbc3440..e15a1e115 100644 --- a/templates/provider/hmc/templates/crds/hmc.mirantis.com_multiclusterservices.yaml +++ b/templates/provider/hmc/templates/crds/hmc.mirantis.com_multiclusterservices.yaml @@ -145,7 +145,7 @@ spec: properties: conditions: description: Conditions contains details for the current state of - the ManagedCluster + the MultiClusterService. items: description: Condition contains details for one aspect of the current state of this API Resource.