From 022114946c8152d45f435cfa0423f76cdfdfe5ba Mon Sep 17 00:00:00 2001 From: Qing Hao Date: Sat, 3 Feb 2024 09:10:53 +0800 Subject: [PATCH] add annotation if addon uses InstallStrategy (#238) Signed-off-by: haoqing0110 --- .../resources/cluster_role.yaml | 2 +- .../helloworld/resources/cluster_role.yaml | 2 +- pkg/addonfactory/addonfactory.go | 3 + .../controllers/managementaddon/controller.go | 131 ++++++++++++++++ .../managementaddon/controller_test.go | 147 ++++++++++++++++++ pkg/addonmanager/manager.go | 12 ++ pkg/agent/inteface.go | 1 + test/e2e/helloworld_helm_test.go | 14 ++ test/e2e/helloworld_test.go | 14 ++ 9 files changed, 324 insertions(+), 2 deletions(-) create mode 100644 pkg/addonmanager/controllers/managementaddon/controller.go create mode 100644 pkg/addonmanager/controllers/managementaddon/controller_test.go diff --git a/examples/deploy/addon/helloworld-helm/resources/cluster_role.yaml b/examples/deploy/addon/helloworld-helm/resources/cluster_role.yaml index fcdeb0b4c..c33c31c4e 100644 --- a/examples/deploy/addon/helloworld-helm/resources/cluster_role.yaml +++ b/examples/deploy/addon/helloworld-helm/resources/cluster_role.yaml @@ -38,7 +38,7 @@ verbs: ["update", "patch"] - apiGroups: ["addon.open-cluster-management.io"] resources: ["clustermanagementaddons"] - verbs: ["get", "list", "watch"] + verbs: ["get", "list", "watch", "patch"] - apiGroups: ["addon.open-cluster-management.io"] resources: ["managedclusteraddons"] verbs: ["get", "list", "watch", "create", "update", "delete"] diff --git a/examples/deploy/addon/helloworld/resources/cluster_role.yaml b/examples/deploy/addon/helloworld/resources/cluster_role.yaml index 8097b2ed9..5a9642ee1 100644 --- a/examples/deploy/addon/helloworld/resources/cluster_role.yaml +++ b/examples/deploy/addon/helloworld/resources/cluster_role.yaml @@ -38,7 +38,7 @@ verbs: ["update", "patch"] - apiGroups: ["addon.open-cluster-management.io"] resources: ["clustermanagementaddons"] - verbs: ["get", "list", "watch"] + verbs: ["get", "list", "watch", "patch"] - apiGroups: ["addon.open-cluster-management.io"] resources: ["managedclusteraddons"] verbs: ["get", "list", "watch", "create", "update", "delete"] diff --git a/pkg/addonfactory/addonfactory.go b/pkg/addonfactory/addonfactory.go index 21d6de39f..6a6e8f7e8 100644 --- a/pkg/addonfactory/addonfactory.go +++ b/pkg/addonfactory/addonfactory.go @@ -78,6 +78,9 @@ func (f *AgentAddonFactory) WithGetValuesFuncs(getValuesFuncs ...GetValuesFunc) } // WithInstallStrategy defines the installation strategy of the manifests prescribed by Manifests(..). +// Deprecated: add annotation "addon.open-cluster-management.io/lifecycle: addon-manager" to ClusterManagementAddon +// and define install strategy in ClusterManagementAddon spec.installStrategy instead. +// The migration plan refer to https://github.com/open-cluster-management-io/ocm/issues/355. func (f *AgentAddonFactory) WithInstallStrategy(strategy *agent.InstallStrategy) *AgentAddonFactory { if strategy.InstallNamespace == "" { strategy.InstallNamespace = AddonDefaultInstallNamespace diff --git a/pkg/addonmanager/controllers/managementaddon/controller.go b/pkg/addonmanager/controllers/managementaddon/controller.go new file mode 100644 index 000000000..9ed14dbc5 --- /dev/null +++ b/pkg/addonmanager/controllers/managementaddon/controller.go @@ -0,0 +1,131 @@ +package managementaddon + +import ( + "context" + "encoding/json" + "fmt" + + jsonpatch "github.com/evanphx/json-patch" + "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/cache" + "k8s.io/klog/v2" + addonapiv1alpha1 "open-cluster-management.io/api/addon/v1alpha1" + addonv1alpha1client "open-cluster-management.io/api/client/addon/clientset/versioned" + addoninformerv1alpha1 "open-cluster-management.io/api/client/addon/informers/externalversions/addon/v1alpha1" + addonlisterv1alpha1 "open-cluster-management.io/api/client/addon/listers/addon/v1alpha1" + + "open-cluster-management.io/addon-framework/pkg/agent" + "open-cluster-management.io/addon-framework/pkg/basecontroller/factory" +) + +const ( + controllerName = "management-addon-controller" +) + +// clusterManagementAddonController reconciles cma on the hub. +type clusterManagementAddonController struct { + addonClient addonv1alpha1client.Interface + clusterManagementAddonLister addonlisterv1alpha1.ClusterManagementAddOnLister + agentAddons map[string]agent.AgentAddon + addonFilterFunc factory.EventFilterFunc +} + +func NewManagementAddonController( + addonClient addonv1alpha1client.Interface, + clusterManagementAddonInformers addoninformerv1alpha1.ClusterManagementAddOnInformer, + agentAddons map[string]agent.AgentAddon, + addonFilterFunc factory.EventFilterFunc, +) factory.Controller { + syncCtx := factory.NewSyncContext(controllerName) + + c := &clusterManagementAddonController{ + addonClient: addonClient, + clusterManagementAddonLister: clusterManagementAddonInformers.Lister(), + agentAddons: agentAddons, + addonFilterFunc: addonFilterFunc, + } + + return factory.New(). + WithSyncContext(syncCtx). + WithFilteredEventsInformersQueueKeysFunc( + func(obj runtime.Object) []string { + key, _ := cache.DeletionHandlingMetaNamespaceKeyFunc(obj) + return []string{key} + }, + c.addonFilterFunc, clusterManagementAddonInformers.Informer()). + WithSync(c.sync).ToController(controllerName) +} + +func (c *clusterManagementAddonController) sync(ctx context.Context, syncCtx factory.SyncContext, key string) error { + _, addonName, err := cache.SplitMetaNamespaceKey(key) + if err != nil { + // ignore addon whose key is invalid + return nil + } + + cma, err := c.clusterManagementAddonLister.Get(addonName) + if errors.IsNotFound(err) { + // addon cloud be deleted, ignore + return nil + } + if err != nil { + return err + } + + addon := c.agentAddons[cma.GetName()] + if addon.GetAgentAddonOptions().InstallStrategy == nil { + return nil + } + + // If the addon defines install strategy via WithInstallStrategy(), force add annotation "addon.open-cluster-management.io/lifecycle: self" to cma. + // The annotation with value "self" will be removed when remove WithInstallStrategy() in addon-framework. + // The migration plan refer to https://github.com/open-cluster-management-io/ocm/issues/355. + cmaCopy := cma.DeepCopy() + if cmaCopy.Annotations == nil { + cmaCopy.Annotations = map[string]string{} + } + cmaCopy.Annotations[addonapiv1alpha1.AddonLifecycleAnnotationKey] = addonapiv1alpha1.AddonLifecycleSelfManageAnnotationValue + + err = c.patchMgmtAddonAnnotations(ctx, cmaCopy, cma) + return err +} + +func (c *clusterManagementAddonController) patchMgmtAddonAnnotations(ctx context.Context, new, old *addonapiv1alpha1.ClusterManagementAddOn) error { + if equality.Semantic.DeepEqual(new.Annotations, old.Annotations) { + return nil + } + + oldData, err := json.Marshal(&addonapiv1alpha1.ClusterManagementAddOn{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: old.Annotations, + }, + }) + if err != nil { + return err + } + + newData, err := json.Marshal(&addonapiv1alpha1.ClusterManagementAddOn{ + ObjectMeta: metav1.ObjectMeta{ + UID: new.UID, + ResourceVersion: new.ResourceVersion, + Annotations: new.Annotations, + }, + }) + if err != nil { + return err + } + + patchBytes, err := jsonpatch.CreateMergePatch(oldData, newData) + if err != nil { + return fmt.Errorf("failed to create patch for addon %s: %w", new.Name, err) + } + + klog.V(2).Infof("Patching clustermanagementaddon %s annotations with %s", new.Name, string(patchBytes)) + _, err = c.addonClient.AddonV1alpha1().ClusterManagementAddOns().Patch( + ctx, new.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{}) + return err +} diff --git a/pkg/addonmanager/controllers/managementaddon/controller_test.go b/pkg/addonmanager/controllers/managementaddon/controller_test.go new file mode 100644 index 000000000..1ec450e54 --- /dev/null +++ b/pkg/addonmanager/controllers/managementaddon/controller_test.go @@ -0,0 +1,147 @@ +package managementaddon + +import ( + "context" + "encoding/json" + "testing" + "time" + + "k8s.io/apimachinery/pkg/runtime" + clienttesting "k8s.io/client-go/testing" + "open-cluster-management.io/addon-framework/pkg/addonmanager/addontesting" + "open-cluster-management.io/addon-framework/pkg/agent" + "open-cluster-management.io/addon-framework/pkg/utils" + addonapiv1alpha1 "open-cluster-management.io/api/addon/v1alpha1" + fakeaddon "open-cluster-management.io/api/client/addon/clientset/versioned/fake" + addoninformers "open-cluster-management.io/api/client/addon/informers/externalversions" + clusterv1 "open-cluster-management.io/api/cluster/v1" +) + +type testAgent struct { + name string + strategy *agent.InstallStrategy +} + +func (t *testAgent) Manifests(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn) ([]runtime.Object, error) { + return nil, nil +} + +func (t *testAgent) GetAgentAddonOptions() agent.AgentAddonOptions { + return agent.AgentAddonOptions{ + AddonName: t.name, + InstallStrategy: t.strategy, + } +} + +func newClusterManagementAddonWithAnnotation(name string, annotations map[string]string) *addonapiv1alpha1.ClusterManagementAddOn { + cma := addontesting.NewClusterManagementAddon(name, "", "").Build() + cma.Annotations = annotations + return cma +} + +func TestReconcile(t *testing.T) { + cases := []struct { + name string + cma []runtime.Object + testaddons map[string]agent.AgentAddon + validateAddonActions func(t *testing.T, actions []clienttesting.Action) + }{ + { + name: "add annotation when uses install strategy", + cma: []runtime.Object{newClusterManagementAddonWithAnnotation("test", map[string]string{ + "test": "test", + })}, + validateAddonActions: func(t *testing.T, actions []clienttesting.Action) { + addontesting.AssertActions(t, actions, "patch") + patch := actions[0].(clienttesting.PatchActionImpl).Patch + cma := &addonapiv1alpha1.ClusterManagementAddOn{} + err := json.Unmarshal(patch, cma) + if err != nil { + t.Fatal(err) + } + + if len(cma.Annotations) != 1 || cma.Annotations[addonapiv1alpha1.AddonLifecycleAnnotationKey] != addonapiv1alpha1.AddonLifecycleSelfManageAnnotationValue { + t.Errorf("cma annotation is not correct, expected self but got %s", cma.Annotations[addonapiv1alpha1.AddonLifecycleAnnotationKey]) + } + }, + testaddons: map[string]agent.AgentAddon{ + "test": &testAgent{name: "test", strategy: agent.InstallAllStrategy("test")}, + }, + }, + { + name: "override annotation when uses install strategy", + cma: []runtime.Object{newClusterManagementAddonWithAnnotation("test", map[string]string{ + "test": "test", + addonapiv1alpha1.AddonLifecycleAnnotationKey: addonapiv1alpha1.AddonLifecycleAddonManagerAnnotationValue, + })}, + validateAddonActions: func(t *testing.T, actions []clienttesting.Action) { + addontesting.AssertActions(t, actions, "patch") + patch := actions[0].(clienttesting.PatchActionImpl).Patch + cma := &addonapiv1alpha1.ClusterManagementAddOn{} + err := json.Unmarshal(patch, cma) + if err != nil { + t.Fatal(err) + } + + if len(cma.Annotations) != 1 || cma.Annotations[addonapiv1alpha1.AddonLifecycleAnnotationKey] != addonapiv1alpha1.AddonLifecycleSelfManageAnnotationValue { + t.Errorf("cma annotation is not correct, expected self but got %s", cma.Annotations[addonapiv1alpha1.AddonLifecycleAnnotationKey]) + } + }, + testaddons: map[string]agent.AgentAddon{ + "test": &testAgent{name: "test", strategy: agent.InstallAllStrategy("test")}, + }, + }, + { + name: "no patch annotation if managed by self", + cma: []runtime.Object{newClusterManagementAddonWithAnnotation("test", map[string]string{ + "test": "test", + addonapiv1alpha1.AddonLifecycleAnnotationKey: addonapiv1alpha1.AddonLifecycleSelfManageAnnotationValue, + })}, + validateAddonActions: addontesting.AssertNoActions, + testaddons: map[string]agent.AgentAddon{ + "test": &testAgent{name: "test", strategy: agent.InstallAllStrategy("test")}, + }, + }, + { + name: "no patch annotation if no install strategy", + cma: []runtime.Object{newClusterManagementAddonWithAnnotation("test", map[string]string{ + "test": "test", + addonapiv1alpha1.AddonLifecycleAnnotationKey: addonapiv1alpha1.AddonLifecycleAddonManagerAnnotationValue, + })}, + validateAddonActions: addontesting.AssertNoActions, + testaddons: map[string]agent.AgentAddon{ + "test": &testAgent{name: "test"}, + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + fakeAddonClient := fakeaddon.NewSimpleClientset(c.cma...) + addonInformers := addoninformers.NewSharedInformerFactory(fakeAddonClient, 10*time.Minute) + + for _, obj := range c.cma { + if err := addonInformers.Addon().V1alpha1().ClusterManagementAddOns().Informer().GetStore().Add(obj); err != nil { + t.Fatal(err) + } + } + + controller := clusterManagementAddonController{ + addonClient: fakeAddonClient, + clusterManagementAddonLister: addonInformers.Addon().V1alpha1().ClusterManagementAddOns().Lister(), + agentAddons: c.testaddons, + addonFilterFunc: utils.FilterByAddonName(c.testaddons), + } + + for _, obj := range c.cma { + cma := obj.(*addonapiv1alpha1.ClusterManagementAddOn) + syncContext := addontesting.NewFakeSyncContext(t) + err := controller.sync(context.TODO(), syncContext, cma.Name) + if err != nil { + t.Errorf("expected no error when sync: %v", err) + } + } + c.validateAddonActions(t, fakeAddonClient.Actions()) + }) + } +} diff --git a/pkg/addonmanager/manager.go b/pkg/addonmanager/manager.go index 28292695a..b2f3575e5 100644 --- a/pkg/addonmanager/manager.go +++ b/pkg/addonmanager/manager.go @@ -25,6 +25,7 @@ import ( "open-cluster-management.io/addon-framework/pkg/addonmanager/controllers/addoninstall" "open-cluster-management.io/addon-framework/pkg/addonmanager/controllers/agentdeploy" "open-cluster-management.io/addon-framework/pkg/addonmanager/controllers/certificate" + "open-cluster-management.io/addon-framework/pkg/addonmanager/controllers/managementaddon" "open-cluster-management.io/addon-framework/pkg/addonmanager/controllers/managementaddonconfig" "open-cluster-management.io/addon-framework/pkg/addonmanager/controllers/registration" "open-cluster-management.io/addon-framework/pkg/agent" @@ -247,6 +248,16 @@ func (a *addonManager) StartWithInformers(ctx context.Context, a.addonAgents, ) + // This controller is used during migrating addons to be managed by addon-manager. + // This should be removed when the migration is done. + // The migration plan refer to https://github.com/open-cluster-management-io/ocm/issues/355. + managementAddonController := managementaddon.NewManagementAddonController( + addonClient, + addonInformers.Addon().V1alpha1().ClusterManagementAddOns(), + a.addonAgents, + utils.FilterByAddonName(a.addonAgents), + ) + // This is a duplicate controller in general addon-manager. This should be removed when we // alway enable the addon-manager addonOwnerController := addonowner.NewAddonOwnerController( @@ -324,6 +335,7 @@ func (a *addonManager) StartWithInformers(ctx context.Context, go deployController.Run(ctx, 1) go registrationController.Run(ctx, 1) go addonInstallController.Run(ctx, 1) + go managementAddonController.Run(ctx, 1) go addonOwnerController.Run(ctx, 1) if addonConfigController != nil { diff --git a/pkg/agent/inteface.go b/pkg/agent/inteface.go index 32514684f..e6ce7ac04 100644 --- a/pkg/agent/inteface.go +++ b/pkg/agent/inteface.go @@ -53,6 +53,7 @@ type AgentAddonOptions struct { // Addon will not be installed automatically until a ManagedClusterAddon is applied to the cluster's // namespace if InstallStrategy is nil. // Deprecated: use installStrategy config in ClusterManagementAddOn API instead + // The migration plan refer to https://github.com/open-cluster-management-io/ocm/issues/355. // +optional InstallStrategy *InstallStrategy diff --git a/test/e2e/helloworld_helm_test.go b/test/e2e/helloworld_helm_test.go index 77b823a8e..ebfadd3a1 100644 --- a/test/e2e/helloworld_helm_test.go +++ b/test/e2e/helloworld_helm_test.go @@ -109,6 +109,20 @@ var _ = ginkgo.Describe("install/uninstall helloworld helm addons", func() { }) ginkgo.It("addon should be available", func() { + ginkgo.By("Make sure cma annotation is not added since no install strategy defined") + gomega.Eventually(func() error { + cma, err := hubAddOnClient.AddonV1alpha1().ClusterManagementAddOns().Get(context.Background(), helloWorldHelmAddonName, metav1.GetOptions{}) + if err != nil { + return err + } + + if _, exist := cma.Annotations[addonapiv1alpha1.AddonLifecycleAnnotationKey]; exist { + return fmt.Errorf("addon should not have annotation, but get %v", cma.Annotations) + } + + return nil + }, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred()) + ginkgo.By("Make sure addon is available and has pre-delete finalizer") gomega.Eventually(func() error { addon, err := hubAddOnClient.AddonV1alpha1().ManagedClusterAddOns(managedClusterName).Get(context.Background(), helloWorldHelmAddonName, metav1.GetOptions{}) diff --git a/test/e2e/helloworld_test.go b/test/e2e/helloworld_test.go index cb6c39fee..0ab44841a 100644 --- a/test/e2e/helloworld_test.go +++ b/test/e2e/helloworld_test.go @@ -100,6 +100,20 @@ var _ = ginkgo.Describe("install/uninstall helloworld addons", func() { }) ginkgo.It("addon should be worked", func() { + ginkgo.By("Make sure cma annotation managed by self is added") + gomega.Eventually(func() error { + cma, err := hubAddOnClient.AddonV1alpha1().ClusterManagementAddOns().Get(context.Background(), addonName, metav1.GetOptions{}) + if err != nil { + return err + } + + if cma.Annotations[addonapiv1alpha1.AddonLifecycleAnnotationKey] != addonapiv1alpha1.AddonLifecycleSelfManageAnnotationValue { + return fmt.Errorf("addon should have annotation, but get %v", cma.Annotations) + } + + return nil + }, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred()) + ginkgo.By("Make sure addon is available") gomega.Eventually(func() error { addon, err := hubAddOnClient.AddonV1alpha1().ManagedClusterAddOns(managedClusterName).Get(context.Background(), addonName, metav1.GetOptions{})