From 9a6189b8c292bd245b9dda731286334151140dcf Mon Sep 17 00:00:00 2001 From: zhujian Date: Tue, 26 Nov 2024 17:03:27 +0800 Subject: [PATCH] Update addon rolebinding Signed-off-by: zhujian --- ...ter-manager-addon-manager-clusterrole.yaml | 2 +- .../controllers/addontemplate/controller.go | 8 +++ pkg/addon/manager.go | 2 +- pkg/addon/templateagent/registration.go | 53 ++++++++----------- pkg/addon/templateagent/registration_test.go | 3 +- pkg/addon/templateagent/template_agent.go | 4 ++ .../templateagent/template_agent_test.go | 4 ++ 7 files changed, 41 insertions(+), 35 deletions(-) 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..267836e44 100644 --- a/pkg/addon/controllers/addontemplate/controller.go +++ b/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( diff --git a/pkg/addon/manager.go b/pkg/addon/manager.go index 8f20d1283..7bbcee489 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{ diff --git a/pkg/addon/templateagent/registration.go b/pkg/addon/templateagent/registration.go index 50c3b60e5..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,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 diff --git a/pkg/addon/templateagent/registration_test.go b/pkg/addon/templateagent/registration_test.go index 238be76b3..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" @@ -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 { 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,