🐛 Only bind the agent role for the addon group (#721)

* Only bind the agent role for the addon group

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

* Update addon rolebinding

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

* Tigger registration reconcile after start

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

* Address code review comments

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

---------

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

go.mod

@@ -32,7 +32,7 @@ require (
 	k8s.io/klog/v2 v2.130.1
 	k8s.io/kube-aggregator v0.31.3
 	k8s.io/utils v0.0.0-20240921022957-49e7df575cb6
-	open-cluster-management.io/addon-framework v0.11.1-0.20241129073230-a3a80961df5a
+	open-cluster-management.io/addon-framework v0.11.1-0.20241129080247-57b1d2859f50
 	open-cluster-management.io/api v0.15.1-0.20241120090202-cb7ce98ab874
 	open-cluster-management.io/sdk-go v0.15.1-0.20241125015855-1536c3970f8f
 	sigs.k8s.io/cluster-inventory-api v0.0.0-20240730014211-ef0154379848

go.sum

@@ -451,8 +451,8 @@ k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 h1:BZqlfIlq5YbRMFko6/PM7F
 k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340/go.mod h1:yD4MZYeKMBwQKVht279WycxKyM84kkAx2DPrTXaeb98=
 k8s.io/utils v0.0.0-20240921022957-49e7df575cb6 h1:MDF6h2H/h4tbzmtIKTuctcwZmY0tY9mD9fNT47QO6HI=
 k8s.io/utils v0.0.0-20240921022957-49e7df575cb6/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
-open-cluster-management.io/addon-framework v0.11.1-0.20241129073230-a3a80961df5a h1:fMeHyfC/0R/7ucub/RF2m7kFtBSG3v6KFzjGn1he5lw=
-open-cluster-management.io/addon-framework v0.11.1-0.20241129073230-a3a80961df5a/go.mod h1:tsBSNs9mGfVQQjXBnjgpiX6r0UM+G3iNfmzQgKhEfw4=
+open-cluster-management.io/addon-framework v0.11.1-0.20241129080247-57b1d2859f50 h1:TXRd6OdGjArh6cwlCYOqlIcyx21k81oUIYj4rmHlYx0=
+open-cluster-management.io/addon-framework v0.11.1-0.20241129080247-57b1d2859f50/go.mod h1:tsBSNs9mGfVQQjXBnjgpiX6r0UM+G3iNfmzQgKhEfw4=
 open-cluster-management.io/api v0.15.1-0.20241120090202-cb7ce98ab874 h1:WgkuYXTbJV7EK+qtiMq3soa21faGUKeTG5w0C8Mn1Ok=
 open-cluster-management.io/api v0.15.1-0.20241120090202-cb7ce98ab874/go.mod h1:9erZEWEn4bEqh0nIX2wA7f/s3KCuFycQdBrPrRzi0QM=
 open-cluster-management.io/sdk-go v0.15.1-0.20241125015855-1536c3970f8f h1:zeC7QrFNarfK2zY6jGtd+mX+yDrQQmnH/J8A7n5Nh38=

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

@@ -8,6 +8,7 @@ import (
 	"github.com/openshift/library-go/pkg/operator/events"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/client-go/dynamic/dynamicinformer"
 	kubeinformers "k8s.io/client-go/informers"
@@ -49,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
@@ -77,6 +79,7 @@ func NewAddonTemplateController(
 		clusterInformers: clusterInformers,
 		dynamicInformers: dynamicInformers,
 		workInformers:    workInformers,
+		eventRecorder:    recorder,
 	}
 
 	if len(runController) > 0 {
@@ -88,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)
 }
@@ -159,8 +167,8 @@ func (c *addonTemplateController) startManager(
 	return stopFunc
 }
 
-func (c *addonTemplateController) runController(
-	ctx context.Context, addonName string) error {
+func (c *addonTemplateController) runController(ctx context.Context, addonName string) error {
+	logger := klog.FromContext(ctx)
 	mgr, err := addonmanager.New(c.kubeConfig)
 	if err != nil {
 		return err
@@ -188,8 +196,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(
@@ -210,8 +219,19 @@ func (c *addonTemplateController) runController(
 	if err != nil {
 		return err
 	}
-
 	kubeInformers.Start(ctx.Done())
 
+	// trigger the manager to reconcile for the existing managed cluster addons
+	mcas, err := c.addonInformers.Addon().V1alpha1().ManagedClusterAddOns().Lister().List(labels.Everything())
+	if err != nil {
+		logger.Info("Failed to list ManagedClusterAddOns", "error", err)
+	} else {
+		for _, mca := range mcas {
+			if mca.Name == addonName {
+				mgr.Trigger(mca.Namespace, addonName)
+			}
+		}
+	}
+
 	return nil
 }

pkg/addon/manager.go

@@ -59,7 +59,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{
@@ -193,6 +193,8 @@ func RunControllerManagerWithInformers(
 		hubWorkClient,
 		addonInformers,
 		clusterInformers,
+		// can share the same dynamic informers for different template type addons since
+		// these addons only support addontemplate and addondeploymentconfig
 		dynamicInformers,
 		workinformers,
 		controllerContext.EventRecorder,

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)
 }

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,

vendor/modules.txt

@@ -1568,7 +1568,7 @@ k8s.io/utils/pointer
 k8s.io/utils/ptr
 k8s.io/utils/strings/slices
 k8s.io/utils/trace
-# open-cluster-management.io/addon-framework v0.11.1-0.20241129073230-a3a80961df5a
+# open-cluster-management.io/addon-framework v0.11.1-0.20241129080247-57b1d2859f50
 ## explicit; go 1.22.0
 open-cluster-management.io/addon-framework/pkg/addonfactory
 open-cluster-management.io/addon-framework/pkg/addonmanager

vendor/open-cluster-management.io/addon-framework/pkg/addonmanager/controllers/addonconfig/controller.go

@@ -71,6 +71,8 @@ func NewAddonConfigController(
 			return []string{key}
 		}, addonInformers.Informer()).
 		WithBareInformers(configInformers...).
+		// clusterManagementAddonLister is used, so wait for cache sync
+		WithBareInformers(clusterManagementAddonInformers.Informer()).
 		WithSync(c.sync).ToController(controllerName)
 }
 

vendor/open-cluster-management.io/addon-framework/pkg/addonmanager/controllers/agentdeploy/controller.go

@@ -76,6 +76,8 @@ func NewAddonDeployController(
 		agentAddons:                agentAddons,
 	}
 
+	c.setClusterInformerHandler(clusterInformers)
+
 	f := factory.New().WithSyncContext(syncCtx).
 		WithFilteredEventsInformersQueueKeysFunc(
 			func(obj runtime.Object) []string {
@@ -126,15 +128,13 @@ func NewAddonDeployController(
 			},
 			workInformers.Informer(),
 		).
+		WithBareInformers(clusterInformers.Informer()).
 		WithSync(c.sync)
 
-	if c.watchManagedCluster(clusterInformers) {
-		f.WithBareInformers(clusterInformers.Informer())
-	}
 	return f.ToController(controllerName)
 }
 
-func (c addonDeployController) watchManagedCluster(clusterInformers clusterinformers.ManagedClusterInformer) bool {
+func (c addonDeployController) setClusterInformerHandler(clusterInformers clusterinformers.ManagedClusterInformer) {
 	var filters []func(old, new *clusterv1.ManagedCluster) bool
 	for _, addon := range c.agentAddons {
 		if addon.GetAgentAddonOptions().AgentDeployTriggerClusterFilter != nil {
@@ -142,7 +142,7 @@ func (c addonDeployController) watchManagedCluster(clusterInformers clusterinfor
 		}
 	}
 	if len(filters) == 0 {
-		return false
+		return
 	}
 
 	_, err := clusterInformers.Informer().AddEventHandler(
@@ -169,8 +169,8 @@ func (c addonDeployController) watchManagedCluster(clusterInformers clusterinfor
 	if err != nil {
 		utilruntime.HandleError(err)
 	}
-	return true
 }
+
 func (c *addonDeployController) enqueueAddOnsByCluster() func(obj interface{}) {
 	return func(obj interface{}) {
 		accessor, _ := meta.Accessor(obj)

vendor/open-cluster-management.io/addon-framework/pkg/addonmanager/controllers/certificate/csrapprove.go

@@ -109,6 +109,8 @@ func NewCSRApprovingController(
 				return true
 			},
 			csrInformer).
+		// clusterLister and addonLister are used, so wait for cache sync
+		WithBareInformers(clusterInformers.Informer(), addonInformers.Informer()).
 		WithSync(c.sync).
 		ToController("CSRApprovingController")
 }

vendor/open-cluster-management.io/addon-framework/pkg/addonmanager/controllers/certificate/csrsign.go

@@ -70,6 +70,8 @@ func NewCSRSignController(
 				return true
 			},
 			csrInformer.Informer()).
+		// clusterLister and addonLister are used, so wait for cache sync
+		WithBareInformers(clusterInformers.Informer(), addonInformers.Informer()).
 		WithSync(c.sync).
 		ToController("CSRSignController")
 }

vendor/open-cluster-management.io/addon-framework/pkg/addonmanager/controllers/registration/controller.go

@@ -58,6 +58,8 @@ func NewAddonRegistrationController(
 			return true
 		},
 		addonInformers.Informer()).
+		// clusterLister is used, so wait for cache sync
+		WithBareInformers(clusterInformers.Informer()).
 		WithSync(c.sync).ToController("addon-registration-controller")
 }