Update addon rolebinding

Signed-off-by: zhujian <[email protected]>

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

@@ -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)
 }
@@ -196,6 +203,7 @@ func (c *addonTemplateController) runController(
 		c.addonClient,
 		c.addonInformers,
 		kubeInformers.Rbac().V1().RoleBindings().Lister(),
+		c.eventRecorder,
 		// image overrides from cluster annotation has lower priority than from the addonDeploymentConfig
 		getValuesClosure,
 		addonfactory.GetAddOnDeploymentConfigValues(

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{

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,15 +429,8 @@ 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 {
-	subjects := []rbacv1.Subject{
-		{
-			Kind:     rbacv1.GroupKind,
-			APIGroup: rbacv1.GroupName,
-			Name:     clusterAddonGroup(clusterName, addonName),
-		},
-	}
 
 	binding := &rbacv1.RoleBinding{
 		ObjectMeta: metav1.ObjectMeta{
@@ -451,28 +442,26 @@ func (a *CRDTemplateAgentAddon) createPermissionBinding(templateName, clusterNam
 				AddonTemplateLabelKey:          "",
 			},
 		},
-		RoleRef:  roleRef,
-		Subjects: subjects,
+		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
-		}
-		return nil
-	default:
-		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
 }
 
 // clusterAddonGroup returns the group that represents the addon for the cluster

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"
@@ -647,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,