🐛 Remove old ns when klusterlet ns is changed (#442)

* Remove old ns when klusterlet ns is changed

Signed-off-by: Jian Qiu <jqiu@redhat.com>

* Resolve comments

Signed-off-by: Jian Qiu <jqiu@redhat.com>

---------

Signed-off-by: Jian Qiu <jqiu@redhat.com>

pkg/operator/operators/klusterlet/controllers/klusterletcontroller/client_builder.go

@@ -8,7 +8,10 @@ import (
 	"context"
 
 	"github.com/openshift/library-go/pkg/operator/events"
+	"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
+	corev1 "k8s.io/api/core/v1"
 	apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
 
@@ -83,7 +86,15 @@ func (m *managedClusterClientsBuilder) build(ctx context.Context) (*managedClust
 	// Ensure the agent namespace for users to create the external-managed-kubeconfig secret in this
 	// namespace, so that in the next reconcile loop the controller can get the secret successfully after
 	// the secret was created.
-	if err := ensureAgentNamespace(ctx, m.kubeClient, m.secretNamespace, m.recorder); err != nil {
+	_, _, err := resourceapply.ApplyNamespace(ctx, m.kubeClient.CoreV1(), m.recorder, &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: m.secretNamespace,
+			Annotations: map[string]string{
+				"workload.openshift.io/allowed": "management",
+			},
+		},
+	})
+	if err != nil {
 		return nil, err
 	}
 

pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller.go

@@ -40,6 +40,8 @@ const (
 	klusterletHostedFinalizer             = "operator.open-cluster-management.io/klusterlet-hosted-cleanup"
 	klusterletFinalizer                   = "operator.open-cluster-management.io/klusterlet-cleanup"
 	managedResourcesEvictionTimestampAnno = "operator.open-cluster-management.io/managed-resources-eviction-timestamp"
+	klusterletNamespaceLabelKey           = "operator.open-cluster-management.io/klusterlet"
+	hostedKlusterletLabelKey              = "operator.open-cluster-management.io/hosted-klusterlet"
 )
 
 type klusterletController struct {
@@ -311,6 +313,9 @@ func (n *klusterletController) sync(ctx context.Context, controllerContext facto
 			kubeClient:            n.kubeClient,
 			recorder:              controllerContext.Recorder(),
 			cache:                 n.cache},
+		&namespaceReconcile{
+			managedClusterClients: managedClusterClients,
+		},
 	}
 
 	var errs []error
@@ -368,19 +373,6 @@ func getManagedKubeConfig(ctx context.Context, kubeClient kubernetes.Interface,
 	return helpers.LoadClientConfigFromSecret(managedKubeconfigSecret)
 }
 
-// ensureAgentNamespace create agent namespace if it is not exist
-func ensureAgentNamespace(ctx context.Context, kubeClient kubernetes.Interface, namespace string, recorder events.Recorder) error {
-	_, _, err := resourceapply.ApplyNamespace(ctx, kubeClient.CoreV1(), recorder, &corev1.Namespace{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: namespace,
-			Annotations: map[string]string{
-				"workload.openshift.io/allowed": "management",
-			},
-		},
-	})
-	return err
-}
-
 // syncPullSecret will sync pull secret from the sourceClient cluster to the targetClient cluster in desired namespace.
 func syncPullSecret(ctx context.Context, sourceClient, targetClient kubernetes.Interface,
 	klusterlet *operatorapiv1.Klusterlet, operatorNamespace, namespace string, recorder events.Recorder) error {
@@ -405,12 +397,28 @@ func syncPullSecret(ctx context.Context, sourceClient, targetClient kubernetes.I
 	return nil
 }
 
-func ensureNamespace(ctx context.Context, kubeClient kubernetes.Interface, klusterlet *operatorapiv1.Klusterlet,
-	namespace string, recorder events.Recorder) error {
-	if err := ensureAgentNamespace(ctx, kubeClient, namespace, recorder); err != nil {
+// ensureNamespace is to apply the namespace defined in klusterlet spec to the managed cluster. The namespace
+// will have a klusterlet label.
+func ensureNamespace(
+	ctx context.Context,
+	kubeClient kubernetes.Interface,
+	klusterlet *operatorapiv1.Klusterlet,
+	namespace string, labels map[string]string, recorder events.Recorder) error {
+	_, _, err := resourceapply.ApplyNamespace(ctx, kubeClient.CoreV1(), recorder, &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: namespace,
+			Annotations: map[string]string{
+				"workload.openshift.io/allowed": "management",
+			},
+			Labels: labels,
+		},
+	})
+	if err != nil {
 		meta.SetStatusCondition(&klusterlet.Status.Conditions, metav1.Condition{
-			Type: operatorapiv1.ConditionKlusterletApplied, Status: metav1.ConditionFalse, Reason: operatorapiv1.ReasonKlusterletApplyFailed,
-			Message: fmt.Sprintf("Failed to ensure namespace %q: %v", namespace, err)})
+			Type:    operatorapiv1.ConditionKlusterletApplied,
+			Status:  metav1.ConditionFalse,
+			Reason:  operatorapiv1.ReasonKlusterletApplyFailed,
+			Message: fmt.Sprintf("Failed to ensure namespace of klusterlet %q: %v", namespace, err)})
 		return err
 	}
 	return nil

pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller_test.go

@@ -23,6 +23,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/rand"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/version"
 	fakekube "k8s.io/client-go/kubernetes/fake"
 	"k8s.io/client-go/rest"
@@ -745,6 +746,62 @@ func TestSyncDeployHostedCreateAgentNamespace(t *testing.T) {
 	}
 }
 
+func TestRemoveOldNamespace(t *testing.T) {
+	klusterlet := newKlusterlet("klusterlet", "testns", "cluster1")
+	bootStrapSecret := newSecret(helpers.BootstrapHubKubeConfig, "testns")
+	hubKubeConfigSecret := newSecret(helpers.HubKubeConfig, "testns")
+	hubKubeConfigSecret.Data["kubeconfig"] = []byte("dummuykubeconnfig")
+	namespace := newNamespace("testns")
+	oldNamespace := newNamespace("oldns")
+	oldNamespace.Labels = map[string]string{
+		klusterletNamespaceLabelKey: "klusterlet",
+	}
+	syncContext := testingcommon.NewFakeSyncContext(t, "klusterlet")
+	controller := newTestController(t, klusterlet, syncContext.Recorder(), nil, bootStrapSecret, hubKubeConfigSecret, namespace, oldNamespace)
+
+	err := controller.controller.sync(context.TODO(), syncContext)
+	if err != nil {
+		t.Errorf("Expected non error when sync, %v", err)
+	}
+
+	var deleteNamespaces []string
+	kubeActions := controller.kubeClient.Actions()
+	for _, action := range kubeActions {
+		if action.GetVerb() == deleteVerb && action.GetResource().Resource == "namespaces" {
+			ns := action.(clienttesting.DeleteActionImpl).Name
+			deleteNamespaces = append(deleteNamespaces, ns)
+		}
+	}
+	if len(deleteNamespaces) != 0 {
+		t.Errorf("expect no namespace deleted before applied")
+	}
+
+	controller.kubeClient.ClearActions()
+	meta.SetStatusCondition(&klusterlet.Status.Conditions, metav1.Condition{
+		Type:   operatorapiv1.ConditionKlusterletApplied,
+		Status: metav1.ConditionTrue,
+	})
+	controller.operatorStore.Update(klusterlet)
+	err = controller.controller.sync(context.TODO(), syncContext)
+	if err != nil {
+		t.Errorf("Expected non error when sync, %v", err)
+	}
+
+	kubeActions = controller.kubeClient.Actions()
+	for _, action := range kubeActions {
+		if action.GetVerb() == deleteVerb && action.GetResource().Resource == "namespaces" {
+			ns := action.(clienttesting.DeleteActionImpl).Name
+			deleteNamespaces = append(deleteNamespaces, ns)
+		}
+	}
+
+	expectedSet := sets.New[string]("oldns")
+	actualSet := sets.New[string](deleteNamespaces...)
+	if !expectedSet.Equal(actualSet) {
+		t.Errorf("ns deletion not correct, got %v", actualSet)
+	}
+}
+
 // TestGetServersFromKlusterlet tests getServersFromKlusterlet func
 func TestGetServersFromKlusterlet(t *testing.T) {
 	cases := []struct {

pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_managed_reconcile.go

@@ -68,7 +68,7 @@ func (r *managedReconcile) reconcile(ctx context.Context, klusterlet *operatorap
 	// TODO(zhujian7): In the future, we may consider deploy addons on the management cluster in Hosted mode.
 	addonNamespace := fmt.Sprintf("%s-addon", config.KlusterletNamespace)
 	// Ensure the addon namespace on the managed cluster
-	err := ensureNamespace(ctx, r.managedClusterClients.kubeClient, klusterlet, addonNamespace, r.recorder)
+	err := ensureNamespace(ctx, r.managedClusterClients.kubeClient, klusterlet, addonNamespace, nil, r.recorder)
 	if err != nil {
 		return klusterlet, reconcileStop, err
 	}
@@ -82,13 +82,12 @@ func (r *managedReconcile) reconcile(ctx context.Context, klusterlet *operatorap
 		return klusterlet, reconcileStop, err
 	}
 
-	if helpers.IsHosted(config.InstallMode) {
-		// In hosted mode, we should ensure the namespace on the managed cluster since
-		// some resources(eg:service account) are still deployed on managed cluster.
-		err := ensureNamespace(ctx, r.managedClusterClients.kubeClient, klusterlet, config.KlusterletNamespace, r.recorder)
-		if err != nil {
-			return klusterlet, reconcileStop, err
-		}
+	err = ensureNamespace(
+		ctx, r.managedClusterClients.kubeClient, klusterlet, config.KlusterletNamespace, map[string]string{
+			klusterletNamespaceLabelKey: klusterlet.Name,
+		}, r.recorder)
+	if err != nil {
+		return klusterlet, reconcileStop, err
 	}
 
 	managedResource := managedStaticResourceFiles

pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_management_recocile.go

@@ -48,7 +48,7 @@ type managementReconcile struct {
 
 func (r *managementReconcile) reconcile(ctx context.Context, klusterlet *operatorapiv1.Klusterlet,
 	config klusterletConfig) (*operatorapiv1.Klusterlet, reconcileState, error) {
-	err := ensureNamespace(ctx, r.kubeClient, klusterlet, config.AgentNamespace, r.recorder)
+	err := ensureNamespace(ctx, r.kubeClient, klusterlet, config.AgentNamespace, nil, r.recorder)
 	if err != nil {
 		return klusterlet, reconcileStop, err
 	}

pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_namespace_reconcile.go

@@ -0,0 +1,55 @@
+package klusterletcontroller
+
+import (
+	"context"
+	"fmt"
+
+	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	operatorapiv1 "open-cluster-management.io/api/operator/v1"
+)
+
+// namespaceReconcile is the reconciler to handle the case when spec.namespace is changed, and we need to
+// clean the old namespace. The reconciler should be run as the last reconciler, and only operate when
+// apply status is true, which ensures that old namespace should only be deleted after the agent in new
+// namespace has been deployed.
+type namespaceReconcile struct {
+	managedClusterClients *managedClusterClients
+}
+
+func (r *namespaceReconcile) reconcile(
+	ctx context.Context,
+	klusterlet *operatorapiv1.Klusterlet,
+	config klusterletConfig) (*operatorapiv1.Klusterlet, reconcileState, error) {
+	cond := meta.FindStatusCondition(klusterlet.Status.Conditions, operatorapiv1.ConditionKlusterletApplied)
+	if cond == nil || cond.Status == metav1.ConditionFalse || klusterlet.Generation != klusterlet.Status.ObservedGeneration {
+		return klusterlet, reconcileContinue, nil
+	}
+	// filters namespace for klusterlet and not in hosted mode
+	namespaces, err := r.managedClusterClients.kubeClient.CoreV1().Namespaces().List(ctx, metav1.ListOptions{
+		LabelSelector: fmt.Sprintf(
+			"%s=%s",
+			klusterletNamespaceLabelKey, klusterlet.Name),
+	})
+	if err != nil {
+		return klusterlet, reconcileStop, err
+	}
+	for _, ns := range namespaces.Items {
+		if ns.Name == config.KlusterletNamespace || ns.Name == config.KlusterletNamespace+"-addon" {
+			continue
+		}
+		if err := r.managedClusterClients.kubeClient.CoreV1().Namespaces().Delete(
+			ctx, ns.Name, metav1.DeleteOptions{}); err != nil && !errors.IsNotFound(err) {
+			return klusterlet, reconcileStop, err
+		}
+	}
+	return klusterlet, reconcileContinue, nil
+}
+
+func (r *namespaceReconcile) clean(_ context.Context, klusterlet *operatorapiv1.Klusterlet,
+	_ klusterletConfig) (*operatorapiv1.Klusterlet, reconcileState, error) {
+	// noop
+	return klusterlet, reconcileContinue, nil
+}

test/e2e/klusterlet_test.go

@@ -1,10 +1,12 @@
 package e2e
 
 import (
+	"context"
 	"fmt"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/rand"
 
@@ -148,4 +150,76 @@ var _ = Describe("Create klusterlet CR", func() {
 			return err
 		}, t.EventuallyTimeout*5, t.EventuallyInterval*5).Should(Succeed())
 	})
+
+	It("Update klusterlet CR namespace", func() {
+		By(fmt.Sprintf("create klusterlet %v with managed cluster name %v", klusterletName, clusterName))
+		_, err := t.CreateKlusterlet(klusterletName, clusterName, klusterletNamespace, operatorapiv1.InstallMode(klusterletDeployMode))
+		Expect(err).ToNot(HaveOccurred())
+
+		By(fmt.Sprintf("waiting for the managed cluster %v to be created", clusterName))
+		Eventually(func() error {
+			_, err := t.GetCreatedManagedCluster(clusterName)
+			return err
+		}, t.EventuallyTimeout*5, t.EventuallyInterval*5).Should(Succeed())
+
+		By(fmt.Sprintf("approve the created managed cluster %v", clusterName))
+		Eventually(func() error {
+			return t.ApproveCSR(clusterName)
+		}, t.EventuallyTimeout, t.EventuallyInterval).Should(Succeed())
+
+		By(fmt.Sprintf("accept the created managed cluster %v", clusterName))
+		Eventually(func() error {
+			return t.AcceptsClient(clusterName)
+		}, t.EventuallyTimeout, t.EventuallyInterval).Should(Succeed())
+
+		By(fmt.Sprintf("waiting for the managed cluster %v to be ready", clusterName))
+		Eventually(func() error {
+			return t.CheckManagedClusterStatus(clusterName)
+		}, t.EventuallyTimeout*5, t.EventuallyInterval*5).Should(Succeed())
+
+		By("update klusterlet namespace")
+		newNamespace := "open-cluster-management-agent-another"
+		Eventually(func() error {
+			klusterlet, err := t.OperatorClient.OperatorV1().Klusterlets().Get(context.TODO(), klusterletName, metav1.GetOptions{})
+			if err != nil {
+				return err
+			}
+			klusterlet.Spec.Namespace = newNamespace
+			_, err = t.OperatorClient.OperatorV1().Klusterlets().Update(context.TODO(), klusterlet, metav1.UpdateOptions{})
+			return err
+		}, t.EventuallyTimeout*5, t.EventuallyInterval*5).Should(Succeed())
+
+		By("copy bootstrap secret to the new namespace")
+		Eventually(func() error {
+			secret := t.bootstrapHubSecret.DeepCopy()
+			_, err = t.SpokeKubeClient.CoreV1().Secrets(newNamespace).Create(context.TODO(), secret, metav1.CreateOptions{})
+			if errors.IsAlreadyExists(err) {
+				return nil
+			}
+			return err
+		}, t.EventuallyTimeout*5, t.EventuallyInterval*5).Should(Succeed())
+
+		By("old namespace should be removed")
+		Eventually(func() error {
+			ns, err := t.SpokeKubeClient.CoreV1().Namespaces().Get(context.TODO(), klusterletNamespace, metav1.GetOptions{})
+			if errors.IsNotFound(err) {
+				return nil
+			}
+			if err == nil {
+				By(fmt.Sprintf("ns is %s, %v", ns.Name, ns.Labels))
+			}
+			return fmt.Errorf("namespace still exists")
+		}, t.EventuallyTimeout*5, t.EventuallyInterval*5).Should(Succeed())
+
+		By(fmt.Sprintf("approve the managed cluster %v since it is registered in the new namespace", clusterName))
+		Eventually(func() error {
+			return t.ApproveCSR(clusterName)
+		}, t.EventuallyTimeout, t.EventuallyInterval).Should(Succeed())
+
+		By("klusterlet status should be ok")
+		Eventually(func() error {
+			err := t.checkKlusterletStatus(klusterletName, "HubConnectionDegraded", "HubConnectionFunctional", metav1.ConditionFalse)
+			return err
+		}, t.EventuallyTimeout*5, t.EventuallyInterval*5).Should(Succeed())
+	})
 })