Validation for MultiClusterService (#597)

* Validation for MultiClusterService and some additional testing
* Remove use of hardcoded system namespace

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,24 @@ func ExtractServiceTemplateName(rawObj client.Object) []string {
 	return templates
 }
 
+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
+}
+
 const SupportedTemplateKey = ".spec.supportedTemplates[].Name"
 
 func SetupClusterTemplateChainIndexer(ctx context.Context, mgr ctrl.Manager) error {

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"`

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,6 +332,10 @@ func setupWebhooks(mgr ctrl.Manager, currentNamespace string) error {
 		setupLog.Error(err, "unable to create webhook", "webhook", "ManagedCluster")
 		return err
 	}
+	if err := (&hmcwebhook.MultiClusterServiceValidator{SystemNamespace: currentNamespace}).SetupWebhookWithManager(mgr); err != nil {
+		setupLog.Error(err, "unable to create webhook", "webhook", "MultiClusterService")
+		return err
+	}
 	if err := (&hmcwebhook.ManagementValidator{}).SetupWebhookWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create webhook", "webhook", "Management")
 		return err
@@ -351,7 +356,7 @@ func setupWebhooks(mgr ctrl.Manager, currentNamespace string) error {
 		setupLog.Error(err, "unable to create webhook", "webhook", "ClusterTemplate")
 		return err
 	}
-	if err := (&hmcwebhook.ServiceTemplateValidator{}).SetupWebhookWithManager(mgr); err != nil {
+	if err := (&hmcwebhook.ServiceTemplateValidator{SystemNamespace: currentNamespace}).SetupWebhookWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create webhook", "webhook", "ServiceTemplate")
 		return err
 	}

internal/controller/managedcluster_controller_test.go

@@ -27,6 +27,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/rest"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	hmc "github.com/Mirantis/hmc/api/v1alpha1"
@@ -38,8 +39,9 @@ var _ = Describe("ManagedCluster Controller", func() {
 			managedClusterName      = "test-managed-cluster"
 			managedClusterNamespace = "test"
 
-			templateName   = "test-template"
-			credentialName = "test-credential"
+			templateName    = "test-template"
+			svcTemplateName = "test-svc-template"
+			credentialName  = "test-credential"
 		)
 
 		ctx := context.Background()
@@ -50,6 +52,7 @@ var _ = Describe("ManagedCluster Controller", func() {
 		}
 		managedCluster := &hmc.ManagedCluster{}
 		template := &hmc.ClusterTemplate{}
+		svcTemplate := &hmc.ServiceTemplate{}
 		management := &hmc.Management{}
 		credential := &hmc.Credential{}
 		namespace := &corev1.Namespace{}
@@ -66,7 +69,7 @@ var _ = Describe("ManagedCluster Controller", func() {
 				Expect(k8sClient.Create(ctx, namespace)).To(Succeed())
 			}
 
-			By("creating the custom resource for the Kind Template")
+			By("creating the custom resource for the Kind ClusterTemplate")
 			err = k8sClient.Get(ctx, typeNamespacedName, template)
 			if err != nil && errors.IsNotFound(err) {
 				template = &hmc.ClusterTemplate{
@@ -99,6 +102,35 @@ var _ = Describe("ManagedCluster Controller", func() {
 				Expect(k8sClient.Status().Update(ctx, template)).To(Succeed())
 			}
 
+			By("creating the custom resource for the Kind ServiceTemplate")
+			err = k8sClient.Get(ctx, client.ObjectKey{Namespace: managedClusterNamespace, Name: svcTemplateName}, svcTemplate)
+			if err != nil && errors.IsNotFound(err) {
+				svcTemplate = &hmc.ServiceTemplate{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      svcTemplateName,
+						Namespace: managedClusterNamespace,
+					},
+					Spec: hmc.ServiceTemplateSpec{
+						Helm: hmc.HelmSpec{
+							ChartRef: &hcv2.CrossNamespaceSourceReference{
+								Kind:      "HelmChart",
+								Name:      "ref-test",
+								Namespace: "default",
+							},
+						},
+					},
+				}
+				Expect(k8sClient.Create(ctx, svcTemplate)).To(Succeed())
+				svcTemplate.Status = hmc.ServiceTemplateStatus{
+					TemplateStatusCommon: hmc.TemplateStatusCommon{
+						TemplateValidationStatus: hmc.TemplateValidationStatus{
+							Valid: true,
+						},
+					},
+				}
+				Expect(k8sClient.Status().Update(ctx, svcTemplate)).To(Succeed())
+			}
+
 			By("creating the custom resource for the Kind Management")
 			err = k8sClient.Get(ctx, typeNamespacedName, management)
 			if err != nil && errors.IsNotFound(err) {
@@ -148,6 +180,12 @@ var _ = Describe("ManagedCluster Controller", func() {
 					Spec: hmc.ManagedClusterSpec{
 						Template:   templateName,
 						Credential: credentialName,
+						Services: []hmc.ServiceSpec{
+							{
+								Template: svcTemplateName,
+								Name:     "test-svc-name",
+							},
+						},
 					},
 				}
 				Expect(k8sClient.Create(ctx, managedCluster)).To(Succeed())

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.
@@ -114,9 +115,9 @@ func (r *MultiClusterServiceReconciler) reconcileUpdate(ctx context.Context, mcs
 		return ctrl.Result{Requeue: true}, nil
 	}
 
-	// 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)
+	// We are enforcing that MultiClusterService 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{}
 	}
 

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,14 +140,28 @@ var _ = Describe("MultiClusterService Controller", func() {
 							ChartRef: &helmcontrollerv2.CrossNamespaceSourceReference{
 								Kind:      "HelmChart",
 								Name:      helmChartName,
-								Namespace: testNamespace,
+								Namespace: testSystemNamespace,
 							},
 						},
 					},
 				}
 			}
 			Expect(k8sClient.Create(ctx, serviceTemplate)).To(Succeed())
 
+			By("reconciling ServiceTemplate used by MultiClusterService")
+			templateReconciler := TemplateReconciler{
+				Client:                k8sClient,
+				downloadHelmChartFunc: fakeDownloadHelmChartFunc,
+			}
+			serviceTemplateReconciler := &ServiceTemplateReconciler{TemplateReconciler: templateReconciler}
+			_, err = serviceTemplateReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: serviceTemplateRef})
+			Expect(err).NotTo(HaveOccurred())
+
+			By("having the valid service template status")
+			Expect(k8sClient.Get(ctx, serviceTemplateRef, serviceTemplate)).To(Succeed())
+			Expect(serviceTemplate.Status.Valid).To(BeTrue())
+			Expect(serviceTemplate.Status.ValidationError).To(BeEmpty())
+
 			By("creating MultiClusterService")
 			err = k8sClient.Get(ctx, multiClusterServiceRef, multiClusterService)
 			if err != nil && apierrors.IsNotFound(err) {
@@ -181,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})
@@ -204,19 +216,10 @@ var _ = Describe("MultiClusterService Controller", func() {
 		})
 
 		It("should successfully reconcile the resource", func() {
-			By("reconciling ServiceTemplate used by MultiClusterService")
-			templateReconciler := TemplateReconciler{
-				Client:                k8sClient,
-				downloadHelmChartFunc: fakeDownloadHelmChartFunc,
-			}
-			serviceTemplateReconciler := &ServiceTemplateReconciler{TemplateReconciler: templateReconciler}
-			_, err := serviceTemplateReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: serviceTemplateRef})
-			Expect(err).NotTo(HaveOccurred())
-
 			By("reconciling MultiClusterService")
-			multiClusterServiceReconciler := &MultiClusterServiceReconciler{Client: k8sClient}
+			multiClusterServiceReconciler := &MultiClusterServiceReconciler{Client: k8sClient, SystemNamespace: testSystemNamespace}
 
-			_, err = multiClusterServiceReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: multiClusterServiceRef})
+			_, err := multiClusterServiceReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: multiClusterServiceRef})
 			Expect(err).NotTo(HaveOccurred())
 
 			Eventually(k8sClient.Get, 1*time.Minute, 5*time.Second).WithArguments(ctx, clusterProfileRef, clusterProfile).ShouldNot(HaveOccurred())

internal/controller/suite_test.go

@@ -54,6 +54,7 @@ import (
 const (
 	mutatingWebhookKind   = "MutatingWebhookConfiguration"
 	validatingWebhookKind = "ValidatingWebhookConfiguration"
+	testSystemNamespace   = "test-system-namespace"
 )
 
 var (
@@ -150,6 +151,9 @@ var _ = BeforeSuite(func() {
 	err = (&hmcwebhook.ManagedClusterValidator{}).SetupWebhookWithManager(mgr)
 	Expect(err).NotTo(HaveOccurred())
 
+	err = (&hmcwebhook.MultiClusterServiceValidator{SystemNamespace: testSystemNamespace}).SetupWebhookWithManager(mgr)
+	Expect(err).NotTo(HaveOccurred())
+
 	err = (&hmcwebhook.ManagementValidator{}).SetupWebhookWithManager(mgr)
 	Expect(err).NotTo(HaveOccurred())
 
@@ -165,7 +169,7 @@ var _ = BeforeSuite(func() {
 	err = (&hmcwebhook.ClusterTemplateValidator{}).SetupWebhookWithManager(mgr)
 	Expect(err).NotTo(HaveOccurred())
 
-	err = (&hmcwebhook.ServiceTemplateValidator{}).SetupWebhookWithManager(mgr)
+	err = (&hmcwebhook.ServiceTemplateValidator{SystemNamespace: testSystemNamespace}).SetupWebhookWithManager(mgr)
 	Expect(err).NotTo(HaveOccurred())
 
 	err = (&hmcwebhook.ProviderTemplateValidator{}).SetupWebhookWithManager(mgr)

internal/controller/template_controller_test.go

@@ -312,7 +312,8 @@ var _ = Describe("Template Controller", func() {
 
 			By("Having the valid cluster template status")
 			Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(clusterTemplate), clusterTemplate)).To(Succeed())
-			Expect(clusterTemplate.Status.Valid && clusterTemplate.Status.ValidationError == "").To(BeTrue())
+			Expect(clusterTemplate.Status.Valid).To(BeTrue())
+			Expect(clusterTemplate.Status.ValidationError).To(BeEmpty())
 			Expect(clusterTemplate.Status.Providers).To(HaveLen(2))
 			Expect(clusterTemplate.Status.ProviderContracts).To(HaveLen(2))
 			Expect(clusterTemplate.Status.Providers[0]).To(Equal(someProviderName))