From 5fc1dbdce72a3ad192fbb48495e303ab210429d9 Mon Sep 17 00:00:00 2001 From: Jian Qiu Date: Mon, 6 May 2024 11:15:48 +0800 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20=20Remove=20old=20ns=20when=20kl?= =?UTF-8?q?usterlet=20ns=20is=20changed=20(#442)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Remove old ns when klusterlet ns is changed Signed-off-by: Jian Qiu * Resolve comments Signed-off-by: Jian Qiu --------- Signed-off-by: Jian Qiu --- .../klusterletcontroller/client_builder.go | 13 +++- .../klusterlet_controller.go | 44 ++++++----- .../klusterlet_controller_test.go | 57 ++++++++++++++ .../klusterlet_managed_reconcile.go | 15 ++-- .../klusterlet_management_recocile.go | 2 +- .../klusterlet_namespace_reconcile.go | 55 ++++++++++++++ test/e2e/klusterlet_test.go | 74 +++++++++++++++++++ 7 files changed, 232 insertions(+), 28 deletions(-) create mode 100644 pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_namespace_reconcile.go diff --git a/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/client_builder.go b/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/client_builder.go index dd003ad70..a6aa8fb8f 100644 --- a/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/client_builder.go +++ b/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 } diff --git a/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller.go b/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller.go index 2343f8599..b8b108cd5 100644 --- a/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller.go +++ b/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 diff --git a/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller_test.go b/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller_test.go index 33f23cfc3..52623ece3 100644 --- a/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller_test.go +++ b/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 { diff --git a/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_managed_reconcile.go b/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_managed_reconcile.go index 3ad52ca38..eb8e36c09 100644 --- a/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_managed_reconcile.go +++ b/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 diff --git a/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_management_recocile.go b/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_management_recocile.go index 91c8aaab5..0824ee08b 100644 --- a/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_management_recocile.go +++ b/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 } diff --git a/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_namespace_reconcile.go b/pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_namespace_reconcile.go new file mode 100644 index 000000000..9522c1bde --- /dev/null +++ b/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 +} diff --git a/test/e2e/klusterlet_test.go b/test/e2e/klusterlet_test.go index 05b4f8468..2b2f54109 100644 --- a/test/e2e/klusterlet_test.go +++ b/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()) + }) })