🐛 [mce-2.7] Only bind the agent role for the addon group (#157) (#734)

* Only bind the agent role for the addon group



* Update addon rolebinding



* Start an addon informor for each addon



---------

Signed-off-by: zhujian <jiazhu@redhat.com>

Author: zhujian7

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"]

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,16 +193,17 @@ 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(
 		ctx,
 		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
 }

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

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
+
+	_, 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
+}
 
-	return nil
+// 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)
 }

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 {

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

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,