diff --git a/manifests/cluster-manager/hub/cluster-manager-addon-manager-clusterrole.yaml b/manifests/cluster-manager/hub/cluster-manager-addon-manager-clusterrole.yaml index 49dbcd51c..3c21f0b30 100644 --- a/manifests/cluster-manager/hub/cluster-manager-addon-manager-clusterrole.yaml +++ b/manifests/cluster-manager/hub/cluster-manager-addon-manager-clusterrole.yaml @@ -61,4 +61,4 @@ rules: verbs: ["approve", "sign"] - apiGroups: ["rbac.authorization.k8s.io"] resources: ["rolebindings"] - verbs: ["get", "list", "watch", "create", "delete"] + verbs: ["get", "list", "watch", "create", "update", "delete"] diff --git a/pkg/addon/controllers/addontemplate/controller.go b/pkg/addon/controllers/addontemplate/controller.go index 569a22e48..ef68320d4 100644 --- a/pkg/addon/controllers/addontemplate/controller.go +++ b/pkg/addon/controllers/addontemplate/controller.go @@ -18,8 +18,8 @@ import ( "open-cluster-management.io/addon-framework/pkg/addonfactory" "open-cluster-management.io/addon-framework/pkg/addonmanager" + "open-cluster-management.io/addon-framework/pkg/index" "open-cluster-management.io/addon-framework/pkg/utils" - addonapiv1alpha1 "open-cluster-management.io/api/addon/v1alpha1" addonv1alpha1 "open-cluster-management.io/api/addon/v1alpha1" addonv1alpha1client "open-cluster-management.io/api/client/addon/clientset/versioned" addoninformers "open-cluster-management.io/api/client/addon/informers/externalversions" @@ -50,6 +50,7 @@ type addonTemplateController struct { dynamicInformers dynamicinformer.DynamicSharedInformerFactory workInformers workv1informers.SharedInformerFactory runControllerFunc runController + eventRecorder events.Recorder } type runController func(ctx context.Context, addonName string) error @@ -78,6 +79,7 @@ func NewAddonTemplateController( clusterInformers: clusterInformers, dynamicInformers: dynamicInformers, workInformers: workInformers, + eventRecorder: recorder, } if len(runController) > 0 { @@ -89,6 +91,11 @@ func NewAddonTemplateController( return factory.New().WithInformersQueueKeysFunc( queue.QueueKeyByMetaNamespaceName, addonInformers.Addon().V1alpha1().ClusterManagementAddOns().Informer()). + WithBareInformers( + // do not need to queue, just make sure the controller reconciles after the addonTemplate cache is synced + // otherwise, there will be "xx-addon-template" not found" errors in the log as the controller uses the + // addonTemplate lister to get the template object + addonInformers.Addon().V1alpha1().AddOnTemplates().Informer()). WithSync(c.sync). ToController("addon-template-controller", recorder) } @@ -186,7 +193,7 @@ func (c *addonTemplateController) runController( listOptions.LabelSelector = metav1.FormatLabelSelector(selector) }), ) - getValuesClosure := func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn) (addonfactory.Values, error) { + getValuesClosure := func(cluster *clusterv1.ManagedCluster, addon *addonv1alpha1.ManagedClusterAddOn) (addonfactory.Values, error) { return templateagent.GetAddOnRegistriesPrivateValuesFromClusterAnnotation(klog.FromContext(ctx), cluster, addon) } agentAddon := templateagent.NewCRDTemplateAgentAddon( @@ -194,8 +201,9 @@ func (c *addonTemplateController) runController( addonName, c.kubeClient, c.addonClient, - c.addonInformers, + c.addonInformers, // use the shared informers, whose cache is synced already kubeInformers.Rbac().V1().RoleBindings().Lister(), + c.eventRecorder, // image overrides from cluster annotation has lower priority than from the addonDeploymentConfig getValuesClosure, addonfactory.GetAddOnDeploymentConfigValues( @@ -211,13 +219,34 @@ func (c *addonTemplateController) runController( return err } + // not share the addon informers + addonInformerFactory := addoninformers.NewSharedInformerFactory(c.addonClient, 10*time.Minute) + err = addonInformerFactory.Addon().V1alpha1().ManagedClusterAddOns().Informer().AddIndexers( + cache.Indexers{ + index.ManagedClusterAddonByNamespace: index.IndexManagedClusterAddonByNamespace, // agentDeployController + index.AddonByConfig: index.IndexAddonByConfig, // addonConfigController + }, + ) + if err != nil { + return err + } + + // managementAddonConfigController + err = addonInformerFactory.Addon().V1alpha1().ClusterManagementAddOns().Informer().AddIndexers( + cache.Indexers{ + index.ClusterManagementAddonByConfig: index.IndexClusterManagementAddonByConfig, // cmaConfigController + }) + if err != nil { + return err + } err = mgr.StartWithInformers(ctx, c.workClient, c.workInformers.Work().V1().ManifestWorks(), - kubeInformers, c.addonInformers, c.clusterInformers, c.dynamicInformers) + kubeInformers, addonInformerFactory, c.clusterInformers, c.dynamicInformers) if err != nil { return err } kubeInformers.Start(ctx.Done()) + addonInformerFactory.Start(ctx.Done()) return nil } diff --git a/pkg/addon/manager.go b/pkg/addon/manager.go index 8f20d1283..7aa7f93a4 100644 --- a/pkg/addon/manager.go +++ b/pkg/addon/manager.go @@ -58,7 +58,7 @@ func RunManager(ctx context.Context, controllerContext *controllercmd.Controller } clusterInformerFactory := clusterinformers.NewSharedInformerFactory(hubClusterClient, 30*time.Minute) - addonInformerFactory := addoninformers.NewSharedInformerFactory(addonClient, 30*time.Minute) + addonInformerFactory := addoninformers.NewSharedInformerFactory(addonClient, 10*time.Minute) workInformers := workv1informers.NewSharedInformerFactoryWithOptions(workClient, 10*time.Minute, workv1informers.WithTweakListOptions(func(listOptions *metav1.ListOptions) { selector := &metav1.LabelSelector{ @@ -112,9 +112,7 @@ func RunControllerManagerWithInformers( err = addonInformers.Addon().V1alpha1().ManagedClusterAddOns().Informer().AddIndexers( cache.Indexers{ - index.ManagedClusterAddonByNamespace: index.IndexManagedClusterAddonByNamespace, // addonDeployController - index.ManagedClusterAddonByName: index.IndexManagedClusterAddonByName, // addonConfigController - index.AddonByConfig: index.IndexAddonByConfig, // addonConfigController + index.ManagedClusterAddonByName: index.IndexManagedClusterAddonByName, // addonConfigurationController, addonManagementController }, ) if err != nil { @@ -124,8 +122,7 @@ func RunControllerManagerWithInformers( // managementAddonConfigController err = addonInformers.Addon().V1alpha1().ClusterManagementAddOns().Informer().AddIndexers( cache.Indexers{ - index.ClusterManagementAddonByConfig: index.IndexClusterManagementAddonByConfig, - index.ClusterManagementAddonByPlacement: index.IndexClusterManagementAddonByPlacement, + index.ClusterManagementAddonByPlacement: index.IndexClusterManagementAddonByPlacement, // addonConfigurationController, addonManagementController }) if err != nil { return err diff --git a/pkg/addon/templateagent/registration.go b/pkg/addon/templateagent/registration.go index 3836d8e67..85ab1008f 100644 --- a/pkg/addon/templateagent/registration.go +++ b/pkg/addon/templateagent/registration.go @@ -10,11 +10,11 @@ import ( "time" openshiftcrypto "github.com/openshift/library-go/pkg/crypto" + "github.com/openshift/library-go/pkg/operator/resource/resourceapply" "github.com/pkg/errors" certificatesv1 "k8s.io/api/certificates/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/client-go/kubernetes" @@ -357,7 +357,7 @@ func (a *CRDTemplateAgentAddon) TemplatePermissionConfigFunc() agent.PermissionC continue } - err := a.createKubeClientPermissions(template.Name, kcrc, cluster, addon) + err := a.createKubeClientPermissions(kcrc, cluster, addon) if err != nil { return err } @@ -376,7 +376,6 @@ func (a *CRDTemplateAgentAddon) TemplatePermissionConfigFunc() agent.PermissionC } func (a *CRDTemplateAgentAddon) createKubeClientPermissions( - templateName string, kcrc *addonapiv1alpha1.KubeClientRegistrationConfig, cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn, @@ -409,8 +408,7 @@ func (a *CRDTemplateAgentAddon) createKubeClientPermissions( APIGroup: rbacv1.GroupName, Name: pc.CurrentCluster.ClusterRoleName, } - err := a.createPermissionBinding(templateName, - cluster.Name, addon.Name, cluster.Name, roleRef, &owner) + err := a.createPermissionBinding(cluster.Name, addon.Name, cluster.Name, roleRef, &owner) if err != nil { return err } @@ -421,8 +419,8 @@ func (a *CRDTemplateAgentAddon) createKubeClientPermissions( // set owner reference nil since the rolebinding has different namespace with the ManagedClusterAddon // TODO: cleanup the rolebinding when the addon is deleted - err := a.createPermissionBinding(templateName, - cluster.Name, addon.Name, pc.SingleNamespace.Namespace, pc.SingleNamespace.RoleRef, nil) + err := a.createPermissionBinding(cluster.Name, addon.Name, + pc.SingleNamespace.Namespace, pc.SingleNamespace.RoleRef, nil) if err != nil { return err } @@ -431,16 +429,9 @@ func (a *CRDTemplateAgentAddon) createKubeClientPermissions( return nil } -func (a *CRDTemplateAgentAddon) createPermissionBinding(templateName, clusterName, addonName, namespace string, +func (a *CRDTemplateAgentAddon) createPermissionBinding(clusterName, addonName, namespace string, roleRef rbacv1.RoleRef, owner *metav1.OwnerReference) error { - // TODO: confirm the group - groups := agent.DefaultGroups(clusterName, addonName) - subject := []rbacv1.Subject{} - for _, group := range groups { - subject = append(subject, rbacv1.Subject{ - Kind: rbacv1.GroupKind, APIGroup: rbacv1.GroupName, Name: group, - }) - } + binding := &rbacv1.RoleBinding{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("open-cluster-management:%s:%s:agent", @@ -451,27 +442,29 @@ func (a *CRDTemplateAgentAddon) createPermissionBinding(templateName, clusterNam AddonTemplateLabelKey: "", }, }, - RoleRef: roleRef, - Subjects: subject, + RoleRef: roleRef, + Subjects: []rbacv1.Subject{ + { + Kind: rbacv1.GroupKind, + APIGroup: rbacv1.GroupName, + Name: clusterAddonGroup(clusterName, addonName), + }, + }, } if owner != nil { binding.OwnerReferences = []metav1.OwnerReference{*owner} } - _, err := a.rolebindingLister.RoleBindings(namespace).Get(binding.Name) - switch { - case err == nil: - // TODO: update the rolebinding if it is not the same - a.logger.Info("Rolebinding already exists", "rolebindingName", binding.Name) - return nil - case apierrors.IsNotFound(err): - _, createErr := a.hubKubeClient.RbacV1().RoleBindings(namespace).Create( - context.TODO(), binding, metav1.CreateOptions{}) - if createErr != nil && !apierrors.IsAlreadyExists(createErr) { - return createErr - } - case err != nil: - return err - } - return nil + _, modified, err := resourceapply.ApplyRoleBinding(context.TODO(), + a.hubKubeClient.RbacV1(), a.eventRecorder, binding) + if err == nil && modified { + a.logger.Info("Rolebinding for addon updated", "namespace", binding.Namespace, "name", binding.Name, + "clusterName", clusterName, "addonName", addonName) + } + return err +} + +// clusterAddonGroup returns the group that represents the addon for the cluster +func clusterAddonGroup(clusterName, addonName string) string { + return fmt.Sprintf("system:open-cluster-management:cluster:%s:addon:%s", clusterName, addonName) } diff --git a/pkg/addon/templateagent/registration_test.go b/pkg/addon/templateagent/registration_test.go index d4038eb00..5ed6855c1 100644 --- a/pkg/addon/templateagent/registration_test.go +++ b/pkg/addon/templateagent/registration_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/openshift/library-go/pkg/operator/events/eventstesting" "github.com/stretchr/testify/assert" certificatesv1 "k8s.io/api/certificates/v1" certificates "k8s.io/api/certificates/v1beta1" @@ -586,6 +587,13 @@ func TestTemplatePermissionConfigFunc(t *testing.T) { if len(rb.OwnerReferences) != 0 { t.Errorf("expected rolebinding to have 0 owner reference, got %d", len(rb.OwnerReferences)) } + if len(rb.Subjects) != 1 { + t.Errorf("expected rolebinding to have 1 subject, got %d", len(rb.Subjects)) + } + if rb.Subjects[0].Name != "system:open-cluster-management:cluster:cluster1:addon:addon1" { + t.Errorf("expected rolebinding subject name to be system:open-cluster-management:cluster:cluster1:addon:addon1, got %s", + rb.Subjects[0].Name) + } }, }, { @@ -640,7 +648,7 @@ func TestTemplatePermissionConfigFunc(t *testing.T) { } agent := NewCRDTemplateAgentAddon(ctx, c.addon.Name, hubKubeClient, addonClient, addonInformerFactory, - kubeInformers.Rbac().V1().RoleBindings().Lister(), nil) + kubeInformers.Rbac().V1().RoleBindings().Lister(), eventstesting.NewTestingEventRecorder(t)) f := agent.TemplatePermissionConfigFunc() err := f(c.cluster, c.addon) if err != c.expectedErr { diff --git a/pkg/addon/templateagent/template_agent.go b/pkg/addon/templateagent/template_agent.go index d9b60ed4b..8e89ba64c 100644 --- a/pkg/addon/templateagent/template_agent.go +++ b/pkg/addon/templateagent/template_agent.go @@ -4,6 +4,7 @@ import ( "context" "fmt" + "github.com/openshift/library-go/pkg/operator/events" "github.com/valyala/fasttemplate" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -61,6 +62,7 @@ type CRDTemplateAgentAddon struct { rolebindingLister rbacv1lister.RoleBindingLister addonName string agentName string + eventRecorder events.Recorder } // NewCRDTemplateAgentAddon creates a CRDTemplateAgentAddon instance @@ -71,6 +73,7 @@ func NewCRDTemplateAgentAddon( addonClient addonv1alpha1client.Interface, addonInformers addoninformers.SharedInformerFactory, rolebindingLister rbacv1lister.RoleBindingLister, + recorder events.Recorder, getValuesFuncs ...addonfactory.GetValuesFunc, ) *CRDTemplateAgentAddon { @@ -87,6 +90,7 @@ func NewCRDTemplateAgentAddon( rolebindingLister: rolebindingLister, addonName: addonName, agentName: fmt.Sprintf("%s-agent", addonName), + eventRecorder: recorder, } return a diff --git a/pkg/addon/templateagent/template_agent_test.go b/pkg/addon/templateagent/template_agent_test.go index 7a3d43b26..150618b00 100644 --- a/pkg/addon/templateagent/template_agent_test.go +++ b/pkg/addon/templateagent/template_agent_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/openshift/library-go/pkg/operator/events/eventstesting" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -518,6 +519,7 @@ func TestAddonTemplateAgentManifests(t *testing.T) { addonClient, addonInformerFactory, kubeInformers.Rbac().V1().RoleBindings().Lister(), + eventstesting.NewTestingEventRecorder(t), addonfactory.GetAddOnDeploymentConfigValues( utils.NewAddOnDeploymentConfigGetter(addonClient), addonfactory.ToAddOnCustomizedVariableValues, @@ -744,6 +746,7 @@ func TestAgentInstallNamespace(t *testing.T) { addonClient, addonInformerFactory, kubeInformers.Rbac().V1().RoleBindings().Lister(), + eventstesting.NewTestingEventRecorder(t), addonfactory.GetAddOnDeploymentConfigValues( utils.NewAddOnDeploymentConfigGetter(addonClient), addonfactory.ToAddOnCustomizedVariableValues, @@ -913,6 +916,7 @@ func TestAgentManifestConfigs(t *testing.T) { addonClient, addonInformerFactory, kubeInformers.Rbac().V1().RoleBindings().Lister(), + eventstesting.NewTestingEventRecorder(t), addonfactory.GetAddOnDeploymentConfigValues( utils.NewAddOnDeploymentConfigGetter(addonClient), addonfactory.ToAddOnCustomizedVariableValues,