Merge pull request k0rdent#371 from eromanova/template-ref

Support to reference ClusterTemplate from the ManagedClusters' namespace only
kylewuolle · Sep 23, 2024 · 5192d13 · 5192d13
2 parents 07398dc + a1f7403
commit 5192d13
Show file tree

Hide file tree

Showing 9 changed files with 63 additions and 58 deletions.
diff --git a/cmd/main.go b/cmd/main.go
@@ -206,11 +206,10 @@ func main() {
 		os.Exit(1)
 	}
 	if err = (&controller.ManagedClusterReconciler{
-		Client:          mgr.GetClient(),
-		Scheme:          mgr.GetScheme(),
-		Config:          mgr.GetConfig(),
-		DynamicClient:   dc,
-		SystemNamespace: currentNamespace,
+		Client:        mgr.GetClient(),
+		Scheme:        mgr.GetScheme(),
+		Config:        mgr.GetConfig(),
+		DynamicClient: dc,
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "ManagedCluster")
 		os.Exit(1)
@@ -267,9 +266,7 @@ func main() {
 	}
 
 	if enableWebhook {
-		if err := (&hmcwebhook.ManagedClusterValidator{
-			SystemNamespace: currentNamespace,
-		}).SetupWebhookWithManager(mgr); err != nil {
+		if err := (&hmcwebhook.ManagedClusterValidator{}).SetupWebhookWithManager(mgr); err != nil {
 			setupLog.Error(err, "unable to create webhook", "webhook", "ManagedCluster")
 			os.Exit(1)
 		}

diff --git a/internal/controller/managedcluster_controller.go b/internal/controller/managedcluster_controller.go
@@ -52,10 +52,9 @@ import (
 // ManagedClusterReconciler reconciles a ManagedCluster object
 type ManagedClusterReconciler struct {
 	client.Client
-	Scheme          *runtime.Scheme
-	Config          *rest.Config
-	DynamicClient   *dynamic.DynamicClient
-	SystemNamespace string
+	Scheme        *runtime.Scheme
+	Config        *rest.Config
+	DynamicClient *dynamic.DynamicClient
 }
 
 type providerSchema struct {
@@ -191,7 +190,7 @@ func (r *ManagedClusterReconciler) Update(ctx context.Context, l logr.Logger, ma
 	}()
 
 	template := &hmc.ClusterTemplate{}
-	templateRef := types.NamespacedName{Name: managedCluster.Spec.Template, Namespace: r.SystemNamespace}
+	templateRef := types.NamespacedName{Name: managedCluster.Spec.Template, Namespace: managedCluster.Namespace}
 	if err := r.Get(ctx, templateRef, template); err != nil {
 		l.Error(err, "Failed to get Template")
 		errMsg := fmt.Sprintf("failed to get provided template: %s", err)
@@ -425,7 +424,7 @@ func (r *ManagedClusterReconciler) Delete(ctx context.Context, l logr.Logger, ma
 }
 
 func (r *ManagedClusterReconciler) releaseCluster(ctx context.Context, namespace, name, templateName string) error {
-	providers, err := r.getProviders(ctx, templateName)
+	providers, err := r.getProviders(ctx, namespace, templateName)
 	if err != nil {
 		return err
 	}
@@ -460,11 +459,11 @@ func (r *ManagedClusterReconciler) releaseCluster(ctx context.Context, namespace
 	return nil
 }
 
-func (r *ManagedClusterReconciler) getProviders(ctx context.Context, templateName string) ([]string, error) {
+func (r *ManagedClusterReconciler) getProviders(ctx context.Context, templateNamespace, templateName string) ([]string, error) {
 	template := &hmc.ClusterTemplate{}
-	templateRef := types.NamespacedName{Name: templateName, Namespace: r.SystemNamespace}
+	templateRef := types.NamespacedName{Name: templateName, Namespace: templateNamespace}
 	if err := r.Get(ctx, templateRef, template); err != nil {
-		log.FromContext(ctx).Error(err, "Failed to get Template", "templateName", templateName)
+		log.FromContext(ctx).Error(err, "Failed to get ClusterTemplate", "namespace", templateNamespace, "name", templateName)
 		return nil, err
 	}
 	return template.Status.Providers.InfrastructureProviders, nil

diff --git a/internal/controller/managedcluster_controller_test.go b/internal/controller/managedcluster_controller_test.go
@@ -30,32 +30,35 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	hmc "github.com/Mirantis/hmc/api/v1alpha1"
-	"github.com/Mirantis/hmc/internal/utils"
 )
 
 var _ = Describe("ManagedCluster Controller", func() {
 	Context("When reconciling a resource", func() {
-		const managedClusterName = "test-managed-cluster"
-		const templateName = "test-template"
+		const (
+			managedClusterName      = "test-managed-cluster"
+			managedClusterNamespace = "test"
+
+			templateName = "test-template"
+		)
 
 		ctx := context.Background()
 
 		typeNamespacedName := types.NamespacedName{
 			Name:      managedClusterName,
-			Namespace: "default",
+			Namespace: managedClusterNamespace,
 		}
 		managedCluster := &hmc.ManagedCluster{}
 		template := &hmc.ClusterTemplate{}
 		management := &hmc.Management{}
 		namespace := &v1.Namespace{}
 
 		BeforeEach(func() {
-			By("creating hmc-system namespace")
-			err := k8sClient.Get(ctx, types.NamespacedName{Name: utils.DefaultSystemNamespace}, namespace)
+			By("creating ManagedCluster namespace")
+			err := k8sClient.Get(ctx, types.NamespacedName{Name: managedClusterNamespace}, namespace)
 			if err != nil && errors.IsNotFound(err) {
 				namespace = &v1.Namespace{
 					ObjectMeta: metav1.ObjectMeta{
-						Name: utils.DefaultSystemNamespace,
+						Name: managedClusterNamespace,
 					},
 				}
 				Expect(k8sClient.Create(ctx, namespace)).To(Succeed())
@@ -67,7 +70,7 @@ var _ = Describe("ManagedCluster Controller", func() {
 				template = &hmc.ClusterTemplate{
 					ObjectMeta: metav1.ObjectMeta{
 						Name:      templateName,
-						Namespace: utils.DefaultSystemNamespace,
+						Namespace: managedClusterNamespace,
 					},
 					Spec: hmc.ClusterTemplateSpec{
 						TemplateSpecMixin: hmc.TemplateSpecMixin{
@@ -112,7 +115,7 @@ var _ = Describe("ManagedCluster Controller", func() {
 				managedCluster = &hmc.ManagedCluster{
 					ObjectMeta: metav1.ObjectMeta{
 						Name:      managedClusterName,
-						Namespace: "default",
+						Namespace: managedClusterNamespace,
 					},
 					Spec: hmc.ManagedClusterSpec{
 						Template: templateName,
@@ -126,9 +129,8 @@ var _ = Describe("ManagedCluster Controller", func() {
 			By("Cleanup")
 
 			controllerReconciler := &ManagedClusterReconciler{
-				Client:          k8sClient,
-				Scheme:          k8sClient.Scheme(),
-				SystemNamespace: utils.DefaultSystemNamespace,
+				Client: k8sClient,
+				Scheme: k8sClient.Scheme(),
 			}
 
 			Expect(k8sClient.Delete(ctx, managedCluster)).To(Succeed())
@@ -145,10 +147,9 @@ var _ = Describe("ManagedCluster Controller", func() {
 		It("should successfully reconcile the resource", func() {
 			By("Reconciling the created resource")
 			controllerReconciler := &ManagedClusterReconciler{
-				Client:          k8sClient,
-				Scheme:          k8sClient.Scheme(),
-				Config:          &rest.Config{},
-				SystemNamespace: utils.DefaultSystemNamespace,
+				Client: k8sClient,
+				Scheme: k8sClient.Scheme(),
+				Config: &rest.Config{},
 			}
 
 			_, err := controllerReconciler.Reconcile(ctx, reconcile.Request{

diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go
@@ -44,7 +44,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 
 	hmcmirantiscomv1alpha1 "github.com/Mirantis/hmc/api/v1alpha1"
-	"github.com/Mirantis/hmc/internal/utils"
 	hmcwebhook "github.com/Mirantis/hmc/internal/webhook"
 	//+kubebuilder:scaffold:imports
 )
@@ -136,9 +135,7 @@ var _ = BeforeSuite(func() {
 	err = hmcmirantiscomv1alpha1.SetupIndexers(ctx, mgr)
 	Expect(err).NotTo(HaveOccurred())
 
-	err = (&hmcwebhook.ManagedClusterValidator{
-		SystemNamespace: utils.DefaultSystemNamespace,
-	}).SetupWebhookWithManager(mgr)
+	err = (&hmcwebhook.ManagedClusterValidator{}).SetupWebhookWithManager(mgr)
 	Expect(err).NotTo(HaveOccurred())
 
 	err = (&hmcwebhook.ManagementValidator{}).SetupWebhookWithManager(mgr)

diff --git a/internal/webhook/managedcluster_webhook.go b/internal/webhook/managedcluster_webhook.go
@@ -35,8 +35,6 @@ import (
 
 type ManagedClusterValidator struct {
 	client.Client
-
-	SystemNamespace string
 }
 
 var InvalidManagedClusterErr = errors.New("the ManagedCluster is invalid")
@@ -61,7 +59,7 @@ func (v *ManagedClusterValidator) ValidateCreate(ctx context.Context, obj runtim
 	if !ok {
 		return nil, apierrors.NewBadRequest(fmt.Sprintf("expected ManagedCluster but got a %T", obj))
 	}
-	template, err := v.getManagedClusterTemplate(ctx, managedCluster.Spec.Template)
+	template, err := v.getManagedClusterTemplate(ctx, managedCluster.Namespace, managedCluster.Spec.Template)
 	if err != nil {
 		return nil, fmt.Errorf("%s: %v", InvalidManagedClusterErr, err)
 	}
@@ -78,7 +76,7 @@ func (v *ManagedClusterValidator) ValidateUpdate(ctx context.Context, _ runtime.
 	if !ok {
 		return nil, apierrors.NewBadRequest(fmt.Sprintf("expected ManagedCluster but got a %T", newObj))
 	}
-	template, err := v.getManagedClusterTemplate(ctx, newManagedCluster.Spec.Template)
+	template, err := v.getManagedClusterTemplate(ctx, newManagedCluster.Namespace, newManagedCluster.Spec.Template)
 	if err != nil {
 		return nil, fmt.Errorf("%s: %v", InvalidManagedClusterErr, err)
 	}
@@ -105,7 +103,7 @@ func (v *ManagedClusterValidator) Default(ctx context.Context, obj runtime.Objec
 	if managedCluster.Spec.Config != nil {
 		return nil
 	}
-	template, err := v.getManagedClusterTemplate(ctx, managedCluster.Spec.Template)
+	template, err := v.getManagedClusterTemplate(ctx, managedCluster.Namespace, managedCluster.Spec.Template)
 	if err != nil {
 		return fmt.Errorf("could not get template for the managedcluster: %s", err)
 	}
@@ -121,9 +119,9 @@ func (v *ManagedClusterValidator) Default(ctx context.Context, obj runtime.Objec
 	return nil
 }
 
-func (v *ManagedClusterValidator) getManagedClusterTemplate(ctx context.Context, templateName string) (*v1alpha1.ClusterTemplate, error) {
+func (v *ManagedClusterValidator) getManagedClusterTemplate(ctx context.Context, templateNamespace, templateName string) (*v1alpha1.ClusterTemplate, error) {
 	template := &v1alpha1.ClusterTemplate{}
-	templateRef := types.NamespacedName{Name: templateName, Namespace: v.SystemNamespace}
+	templateRef := types.NamespacedName{Name: templateName, Namespace: templateNamespace}
 	if err := v.Get(ctx, templateRef, template); err != nil {
 		return nil, err
 	}

diff --git a/internal/webhook/managedcluster_webhook_test.go b/internal/webhook/managedcluster_webhook_test.go
@@ -25,7 +25,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/managedcluster"
 	"github.com/Mirantis/hmc/test/objects/management"
 	"github.com/Mirantis/hmc/test/objects/template"
@@ -34,6 +33,7 @@ import (
 
 var (
 	testTemplateName = "template-test"
+	testNamespace    = "test"
 
 	mgmt = management.NewManagement(
 		management.WithAvailableProviders(v1alpha1.Providers{
@@ -56,13 +56,13 @@ var (
 			err:            "the ManagedCluster is invalid: clustertemplates.hmc.mirantis.com \"\" not found",
 		},
 		{
-			name:           "should fail if the cluster template is not found in hmc-system namespace",
+			name:           "should fail if the ClusterTemplate is not found in the ManagedCluster's namespace",
 			managedCluster: managedcluster.NewManagedCluster(managedcluster.WithTemplate(testTemplateName)),
 			existingObjects: []runtime.Object{
 				mgmt,
 				template.NewClusterTemplate(
 					template.WithName(testTemplateName),
-					template.WithNamespace("default"),
+					template.WithNamespace(testNamespace),
 				),
 			},
 			err: fmt.Sprintf("the ManagedCluster is invalid: clustertemplates.hmc.mirantis.com \"%s\" not found", testTemplateName),
@@ -130,7 +130,7 @@ func TestManagedClusterValidateCreate(t *testing.T) {
 	for _, tt := range createAndUpdateTests {
 		t.Run(tt.name, func(t *testing.T) {
 			c := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(tt.existingObjects...).Build()
-			validator := &ManagedClusterValidator{Client: c, SystemNamespace: utils.DefaultSystemNamespace}
+			validator := &ManagedClusterValidator{Client: c}
 			warn, err := validator.ValidateCreate(ctx, tt.managedCluster)
 			if tt.err != "" {
 				g.Expect(err).To(HaveOccurred())
@@ -156,7 +156,7 @@ func TestManagedClusterValidateUpdate(t *testing.T) {
 	for _, tt := range createAndUpdateTests {
 		t.Run(tt.name, func(t *testing.T) {
 			c := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(tt.existingObjects...).Build()
-			validator := &ManagedClusterValidator{Client: c, SystemNamespace: utils.DefaultSystemNamespace}
+			validator := &ManagedClusterValidator{Client: c}
 			warn, err := validator.ValidateUpdate(ctx, managedcluster.NewManagedCluster(), tt.managedCluster)
 			if tt.err != "" {
 				g.Expect(err).To(HaveOccurred())
@@ -244,7 +244,7 @@ func TestManagedClusterDefault(t *testing.T) {
 	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, SystemNamespace: utils.DefaultSystemNamespace}
+			validator := &ManagedClusterValidator{Client: c}
 			err := validator.Default(ctx, tt.input)
 			if tt.err != "" {
 				g.Expect(err).To(HaveOccurred())

diff --git a/internal/webhook/template_webhook.go b/internal/webhook/template_webhook.go
@@ -73,6 +73,7 @@ func (v *ClusterTemplateValidator) ValidateDelete(ctx context.Context, obj runti
 	listOptions := client.ListOptions{
 		FieldSelector: fields.SelectorFromSet(fields.Set{v1alpha1.TemplateKey: template.Name}),
 		Limit:         1,
+		Namespace:     template.Namespace,
 	}
 	err := v.Client.List(ctx, managedClusters, &listOptions)
 	if err != nil {

diff --git a/internal/webhook/template_webhook_test.go b/internal/webhook/template_webhook_test.go
@@ -32,8 +32,9 @@ import (
 func TestClusterTemplateValidateDelete(t *testing.T) {
 	g := NewWithT(t)
 	ctx := context.Background()
-	tpl := template.NewClusterTemplate(template.WithName("testTemplateFail"))
-	tplTest := template.NewClusterTemplate(template.WithName("testTemplate"))
+	namespace := "test"
+	tpl := template.NewClusterTemplate(template.WithName("testTemplateFail"), template.WithNamespace(namespace))
+	tplTest := template.NewClusterTemplate(template.WithName("testTemplate"), template.WithNamespace(namespace))
 
 	tests := []struct {
 		name            string
@@ -43,11 +44,22 @@ func TestClusterTemplateValidateDelete(t *testing.T) {
 		warnings        admission.Warnings
 	}{
 		{
-			name:            "should fail if ManagedCluster objects exist",
-			template:        tpl,
-			existingObjects: []runtime.Object{managedcluster.NewManagedCluster(managedcluster.WithTemplate(tpl.Name))},
-			warnings:        admission.Warnings{"The ClusterTemplate object can't be removed if ManagedCluster objects referencing it still exist"},
-			err:             "template deletion is forbidden",
+			name:     "should fail if ManagedCluster objects exist in the same namespace",
+			template: tpl,
+			existingObjects: []runtime.Object{managedcluster.NewManagedCluster(
+				managedcluster.WithNamespace(namespace),
+				managedcluster.WithTemplate(tpl.Name),
+			)},
+			warnings: admission.Warnings{"The ClusterTemplate object can't be removed if ManagedCluster objects referencing it still exist"},
+			err:      "template deletion is forbidden",
+		},
+		{
+			name:     "should succeed if some ManagedCluster from another namespace references the template",
+			template: tpl,
+			existingObjects: []runtime.Object{managedcluster.NewManagedCluster(
+				managedcluster.WithNamespace("new"),
+				managedcluster.WithTemplate(tpl.Name),
+			)},
 		},
 		{
 			name:            "should be OK because of a different cluster",

diff --git a/test/objects/template/template.go b/test/objects/template/template.go
@@ -23,7 +23,7 @@ import (
 
 const (
 	DefaultName      = "template"
-	DefaultNamespace = "hmc-system"
+	DefaultNamespace = "default"
 )
 
 type Template struct {