diff --git a/pkg/addonmanager/controllers/addoninstall/controller.go b/pkg/addonmanager/controllers/addoninstall/controller.go index e4c2f9484..4c57759e7 100644 --- a/pkg/addonmanager/controllers/addoninstall/controller.go +++ b/pkg/addonmanager/controllers/addoninstall/controller.go @@ -16,7 +16,6 @@ import ( addonlisterv1alpha1 "open-cluster-management.io/api/client/addon/listers/addon/v1alpha1" clusterinformers "open-cluster-management.io/api/client/cluster/informers/externalversions/cluster/v1" clusterlister "open-cluster-management.io/api/client/cluster/listers/cluster/v1" - "open-cluster-management.io/sdk-go/pkg/patcher" "open-cluster-management.io/addon-framework/pkg/agent" "open-cluster-management.io/addon-framework/pkg/basecontroller/factory" @@ -24,32 +23,23 @@ import ( // managedClusterController reconciles instances of ManagedCluster on the hub. type addonInstallController struct { - addonClient addonv1alpha1client.Interface - managedClusterLister clusterlister.ManagedClusterLister - managedClusterAddonLister addonlisterv1alpha1.ManagedClusterAddOnLister - clusterManagementAddonLister addonlisterv1alpha1.ClusterManagementAddOnLister - agentAddons map[string]agent.AgentAddon - addonPatcher patcher.Patcher[*addonapiv1alpha1.ClusterManagementAddOn, - addonapiv1alpha1.ClusterManagementAddOnSpec, - addonapiv1alpha1.ClusterManagementAddOnStatus] + addonClient addonv1alpha1client.Interface + managedClusterLister clusterlister.ManagedClusterLister + managedClusterAddonLister addonlisterv1alpha1.ManagedClusterAddOnLister + agentAddons map[string]agent.AgentAddon } func NewAddonInstallController( addonClient addonv1alpha1client.Interface, clusterInformers clusterinformers.ManagedClusterInformer, addonInformers addoninformerv1alpha1.ManagedClusterAddOnInformer, - clusterManagementAddonInformers addoninformerv1alpha1.ClusterManagementAddOnInformer, agentAddons map[string]agent.AgentAddon, ) factory.Controller { c := &addonInstallController{ - addonClient: addonClient, - managedClusterLister: clusterInformers.Lister(), - managedClusterAddonLister: addonInformers.Lister(), - clusterManagementAddonLister: clusterManagementAddonInformers.Lister(), - agentAddons: agentAddons, - addonPatcher: patcher.NewPatcher[*addonapiv1alpha1.ClusterManagementAddOn, - addonapiv1alpha1.ClusterManagementAddOnSpec, - addonapiv1alpha1.ClusterManagementAddOnStatus](addonClient.AddonV1alpha1().ClusterManagementAddOns()), + addonClient: addonClient, + managedClusterLister: clusterInformers.Lister(), + managedClusterAddonLister: addonInformers.Lister(), + agentAddons: agentAddons, } return factory.New().WithFilteredEventsInformersQueueKeysFunc( @@ -108,27 +98,6 @@ func (c *addonInstallController) sync(ctx context.Context, syncCtx factory.SyncC continue } - cma, err := c.clusterManagementAddonLister.Get(addonName) - if err != nil { - if !errors.IsNotFound(err) { - return err - } - } else { - // 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.addonPatcher.PatchLabelAnnotations(ctx, cmaCopy, cmaCopy.ObjectMeta, cma.ObjectMeta) - if err != nil { - return err - } - } - managedClusterFilter := addon.GetAgentAddonOptions().InstallStrategy.GetManagedClusterFilter() if managedClusterFilter == nil { continue diff --git a/pkg/addonmanager/controllers/addoninstall/controller_test.go b/pkg/addonmanager/controllers/addoninstall/controller_test.go index fbba98c88..bffeb9478 100644 --- a/pkg/addonmanager/controllers/addoninstall/controller_test.go +++ b/pkg/addonmanager/controllers/addoninstall/controller_test.go @@ -2,7 +2,6 @@ package addoninstall import ( "context" - "encoding/json" "testing" "time" @@ -17,7 +16,6 @@ import ( fakecluster "open-cluster-management.io/api/client/cluster/clientset/versioned/fake" clusterv1informers "open-cluster-management.io/api/client/cluster/informers/externalversions" clusterv1 "open-cluster-management.io/api/cluster/v1" - "open-cluster-management.io/sdk-go/pkg/patcher" ) type testAgent struct { @@ -49,16 +47,9 @@ func newManagedClusterWithAnnotation(name, key, value string) *clusterv1.Managed return cluster } -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 addon []runtime.Object testaddons map[string]agent.AgentAddon cluster []runtime.Object @@ -202,90 +193,12 @@ func TestReconcile(t *testing.T) { })}, }, }, - { - name: "add annotation when uses install strategy", - cma: []runtime.Object{newClusterManagementAddonWithAnnotation("test", map[string]string{ - "test": "test", - })}, - addon: []runtime.Object{}, - cluster: []runtime.Object{addontesting.NewManagedCluster("cluster1")}, - validateAddonActions: func(t *testing.T, actions []clienttesting.Action) { - addontesting.AssertActions(t, actions, "patch", "create") - 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, - })}, - addon: []runtime.Object{}, - cluster: []runtime.Object{addontesting.NewManagedCluster("cluster1")}, - validateAddonActions: func(t *testing.T, actions []clienttesting.Action) { - addontesting.AssertActions(t, actions, "patch", "create") - 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, - })}, - addon: []runtime.Object{}, - cluster: []runtime.Object{addontesting.NewManagedCluster("cluster1")}, - validateAddonActions: func(t *testing.T, actions []clienttesting.Action) { - addontesting.AssertActions(t, actions, "create") - }, - 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, - })}, - addon: []runtime.Object{}, - cluster: []runtime.Object{addontesting.NewManagedCluster("cluster1")}, - validateAddonActions: addontesting.AssertNoActions, - testaddons: map[string]agent.AgentAddon{ - "test": &testAgent{name: "test"}, - }, - }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - obj := append(c.addon, c.cma...) fakeClusterClient := fakecluster.NewSimpleClientset(c.cluster...) - fakeAddonClient := fakeaddon.NewSimpleClientset(obj...) + fakeAddonClient := fakeaddon.NewSimpleClientset(c.addon...) addonInformers := addoninformers.NewSharedInformerFactory(fakeAddonClient, 10*time.Minute) clusterInformers := clusterv1informers.NewSharedInformerFactory(fakeClusterClient, 10*time.Minute) @@ -300,21 +213,12 @@ func TestReconcile(t *testing.T) { t.Fatal(err) } } - for _, obj := range c.cma { - if err := addonInformers.Addon().V1alpha1().ClusterManagementAddOns().Informer().GetStore().Add(obj); err != nil { - t.Fatal(err) - } - } controller := addonInstallController{ - addonClient: fakeAddonClient, - managedClusterLister: clusterInformers.Cluster().V1().ManagedClusters().Lister(), - managedClusterAddonLister: addonInformers.Addon().V1alpha1().ManagedClusterAddOns().Lister(), - clusterManagementAddonLister: addonInformers.Addon().V1alpha1().ClusterManagementAddOns().Lister(), - agentAddons: c.testaddons, - addonPatcher: patcher.NewPatcher[*addonapiv1alpha1.ClusterManagementAddOn, - addonapiv1alpha1.ClusterManagementAddOnSpec, - addonapiv1alpha1.ClusterManagementAddOnStatus](fakeAddonClient.AddonV1alpha1().ClusterManagementAddOns()), + addonClient: fakeAddonClient, + managedClusterLister: clusterInformers.Cluster().V1().ManagedClusters().Lister(), + managedClusterAddonLister: addonInformers.Addon().V1alpha1().ManagedClusterAddOns().Lister(), + agentAddons: c.testaddons, } for _, obj := range c.cluster { diff --git a/pkg/addonmanager/controllers/managementaddon/controller.go b/pkg/addonmanager/controllers/managementaddon/controller.go new file mode 100644 index 000000000..6c739b025 --- /dev/null +++ b/pkg/addonmanager/controllers/managementaddon/controller.go @@ -0,0 +1,97 @@ +package managementaddon + +import ( + "context" + + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/tools/cache" + 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/sdk-go/pkg/patcher" + + "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 + addonPatcher patcher.Patcher[*addonapiv1alpha1.ClusterManagementAddOn, + addonapiv1alpha1.ClusterManagementAddOnSpec, + addonapiv1alpha1.ClusterManagementAddOnStatus] +} + +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, + addonPatcher: patcher.NewPatcher[*addonapiv1alpha1.ClusterManagementAddOn, + addonapiv1alpha1.ClusterManagementAddOnSpec, + addonapiv1alpha1.ClusterManagementAddOnStatus](addonClient.AddonV1alpha1().ClusterManagementAddOns()), + } + + return factory.New(). + WithSyncContext(syncCtx). + WithInformersQueueKeysFunc(func(obj runtime.Object) []string { + key, _ := cache.MetaNamespaceKeyFunc(obj) + return []string{key} + }, 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 + } + + if !c.addonFilterFunc(cma) { + return nil + } + + 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.addonPatcher.PatchLabelAnnotations(ctx, cmaCopy, cmaCopy.ObjectMeta, cma.ObjectMeta) + 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..8a3c3ee96 --- /dev/null +++ b/pkg/addonmanager/controllers/managementaddon/controller_test.go @@ -0,0 +1,151 @@ +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" + "open-cluster-management.io/sdk-go/pkg/patcher" +) + +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), + addonPatcher: patcher.NewPatcher[*addonapiv1alpha1.ClusterManagementAddOn, + addonapiv1alpha1.ClusterManagementAddOnSpec, + addonapiv1alpha1.ClusterManagementAddOnStatus](fakeAddonClient.AddonV1alpha1().ClusterManagementAddOns()), + } + + 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 a608bca23..64616f512 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" @@ -244,8 +245,14 @@ func (a *addonManager) StartWithInformers(ctx context.Context, addonClient, clusterInformers.Cluster().V1().ManagedClusters(), addonInformers.Addon().V1alpha1().ManagedClusterAddOns(), + a.addonAgents, + ) + + 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 @@ -325,6 +332,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 {