diff --git a/pkg/addon/templateagent/registration.go b/pkg/addon/templateagent/registration.go index 70299399b..34834bafd 100644 --- a/pkg/addon/templateagent/registration.go +++ b/pkg/addon/templateagent/registration.go @@ -15,6 +15,7 @@ import ( certificatesv1 "k8s.io/api/certificates/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" "k8s.io/klog/v2" @@ -353,7 +354,8 @@ func extractCAdata(caCertData, caKeyData []byte) ([]byte, []byte, error) { // that is deployed by addon template. // the returned func will create a rolebinding to bind the clusterRole/role which is // specified by the user, so the user is required to make sure the existence of the -// clusterRole/role +// clusterRole/role. +// It uses dynamic subject binding which works with both CSR-based and token-based authentication. func (a *CRDTemplateAgentAddon) TemplatePermissionConfigFunc() agent.PermissionConfigFunc { return func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn) error { @@ -451,6 +453,22 @@ func (a *CRDTemplateAgentAddon) createKubeClientPermissions( func (a *CRDTemplateAgentAddon) createPermissionBinding(clusterName, addonName, namespace string, roleRef rbacv1.RoleRef, owner *metav1.OwnerReference) error { + // Get the ManagedClusterAddOn to extract dynamic subjects from Status.Registrations + addon, err := a.addonLister.ManagedClusterAddOns(clusterName).Get(addonName) + if err != nil { + return fmt.Errorf("failed to get ManagedClusterAddOn %s/%s: %w", clusterName, addonName, err) + } + + // Build subjects dynamically from addon.Status.Registrations for KubeClient signer + // This works with both CSR-based and token-based authentication + subjects := buildSubjectsFromRegistration(addon, certificatesv1.KubeAPIServerClientSignerName) + + // If no subjects found, return pending error to retry later + // This can happen when the addon is first created and registrations are not yet populated + if len(subjects) == 0 { + return &agent.SubjectNotReadyError{} + } + binding := &rbacv1.RoleBinding{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("open-cluster-management:%s:%s:agent", @@ -461,14 +479,8 @@ func (a *CRDTemplateAgentAddon) createPermissionBinding(clusterName, addonName, AddonTemplateLabelKey: "", }, }, - RoleRef: roleRef, - Subjects: []rbacv1.Subject{ - { - Kind: rbacv1.GroupKind, - APIGroup: rbacv1.GroupName, - Name: clusterAddonGroup(clusterName, addonName), - }, - }, + RoleRef: roleRef, + Subjects: subjects, } if owner != nil { binding.OwnerReferences = []metav1.OwnerReference{*owner} @@ -484,12 +496,53 @@ func (a *CRDTemplateAgentAddon) createPermissionBinding(clusterName, addonName, a.hubKubeClient.RbacV1(), recorderWrapper, binding) if err == nil && modified { a.logger.Info("Rolebinding for addon updated", "namespace", binding.Namespace, "name", binding.Name, - "clusterName", clusterName, "addonName", addonName) + "clusterName", clusterName, "addonName", addonName, "subjects", subjects) } 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) +// buildSubjectsFromRegistration extracts and builds RBAC subjects from addon registration status. +// Returns nil if no matching registration is found or if the subject is empty. +// This function is based on the addon-framework implementation to support dynamic subject binding +// for both CSR-based and token-based authentication. +func buildSubjectsFromRegistration(addon *addonapiv1alpha1.ManagedClusterAddOn, signerName string) []rbacv1.Subject { + // Find the registration config for the specified signer + var subject *addonapiv1alpha1.Subject + for _, registration := range addon.Status.Registrations { + if registration.SignerName == signerName { + subject = ®istration.Subject + break + } + } + + // If no registration config found or subject is empty, return nil + if subject == nil || equality.Semantic.DeepEqual(*subject, addonapiv1alpha1.Subject{}) { + return nil + } + + // Build subjects from the registration config subject + subjects := []rbacv1.Subject{} + + // Include user if set + if subject.User != "" { + subjects = append(subjects, rbacv1.Subject{ + Kind: rbacv1.UserKind, + APIGroup: rbacv1.GroupName, + Name: subject.User, + }) + } + + // Include groups + for _, group := range subject.Groups { + if group == "system:authenticated" { + continue + } + subjects = append(subjects, rbacv1.Subject{ + Kind: rbacv1.GroupKind, + APIGroup: rbacv1.GroupName, + Name: group, + }) + } + + return subjects } diff --git a/pkg/addon/templateagent/registration_test.go b/pkg/addon/templateagent/registration_test.go index 2109bc66b..c1dcecc9a 100644 --- a/pkg/addon/templateagent/registration_test.go +++ b/pkg/addon/templateagent/registration_test.go @@ -458,9 +458,25 @@ func NewFakeTemplateManagedClusterAddon(name, clusterName, addonTemplateName, ad ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: clusterName, + UID: "fakeuid", + }, + Spec: addonapiv1alpha1.ManagedClusterAddOnSpec{}, + Status: addonapiv1alpha1.ManagedClusterAddOnStatus{ + // Add default registration status with subjects for dynamic RBAC binding + Registrations: []addonapiv1alpha1.RegistrationConfig{ + { + SignerName: certificatesv1.KubeAPIServerClientSignerName, + Subject: addonapiv1alpha1.Subject{ + User: fmt.Sprintf("system:open-cluster-management:cluster:%s:addon:%s:agent:%s-agent", + clusterName, name, name), + Groups: []string{ + fmt.Sprintf("system:open-cluster-management:cluster:%s:addon:%s", clusterName, name), + fmt.Sprintf("system:open-cluster-management:addon:%s", name), + }, + }, + }, + }, }, - Spec: addonapiv1alpha1.ManagedClusterAddOnSpec{}, - Status: addonapiv1alpha1.ManagedClusterAddOnStatus{}, } if addonTemplateName != "" { @@ -677,13 +693,25 @@ 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)) + // Dynamic subjects: 1 user + 2 groups + if len(rb.Subjects) != 3 { + t.Errorf("expected rolebinding to have 3 subjects, 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", + // Verify user subject + if rb.Subjects[0].Kind != rbacv1.UserKind { + t.Errorf("expected first subject to be User, got %s", rb.Subjects[0].Kind) + } + if rb.Subjects[0].Name != "system:open-cluster-management:cluster:cluster1:addon:addon1:agent:addon1-agent" { + t.Errorf("expected user subject name to be system:open-cluster-management:cluster:cluster1:addon:addon1:agent:addon1-agent, got %s", rb.Subjects[0].Name) } + // Verify group subjects + if rb.Subjects[1].Kind != rbacv1.GroupKind { + t.Errorf("expected second subject to be Group, got %s", rb.Subjects[1].Kind) + } + if rb.Subjects[2].Kind != rbacv1.GroupKind { + t.Errorf("expected third subject to be Group, got %s", rb.Subjects[2].Kind) + } }, }, { @@ -791,3 +819,254 @@ func TestAddonManagerNamespace(t *testing.T) { os.Setenv("POD_NAMESPACE", "") } } + +func TestBuildSubjectsFromRegistration(t *testing.T) { + cases := []struct { + name string + addon *addonapiv1alpha1.ManagedClusterAddOn + signerName string + expectedSubjects []rbacv1.Subject + }{ + { + name: "no registrations", + addon: &addonapiv1alpha1.ManagedClusterAddOn{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-addon", + Namespace: "cluster1", + }, + Status: addonapiv1alpha1.ManagedClusterAddOnStatus{}, + }, + signerName: certificatesv1.KubeAPIServerClientSignerName, + expectedSubjects: nil, + }, + { + name: "registration with user and groups", + addon: &addonapiv1alpha1.ManagedClusterAddOn{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-addon", + Namespace: "cluster1", + }, + Status: addonapiv1alpha1.ManagedClusterAddOnStatus{ + Registrations: []addonapiv1alpha1.RegistrationConfig{ + { + SignerName: certificatesv1.KubeAPIServerClientSignerName, + Subject: addonapiv1alpha1.Subject{ + User: "system:open-cluster-management:cluster:cluster1:addon:test-addon:agent:test-addon-agent", + Groups: []string{ + "system:open-cluster-management:cluster:cluster1:addon:test-addon", + "system:open-cluster-management:addon:test-addon", + }, + }, + }, + }, + }, + }, + signerName: certificatesv1.KubeAPIServerClientSignerName, + expectedSubjects: []rbacv1.Subject{ + { + Kind: rbacv1.UserKind, + APIGroup: rbacv1.GroupName, + Name: "system:open-cluster-management:cluster:cluster1:addon:test-addon:agent:test-addon-agent", + }, + { + Kind: rbacv1.GroupKind, + APIGroup: rbacv1.GroupName, + Name: "system:open-cluster-management:cluster:cluster1:addon:test-addon", + }, + { + Kind: rbacv1.GroupKind, + APIGroup: rbacv1.GroupName, + Name: "system:open-cluster-management:addon:test-addon", + }, + }, + }, + { + name: "registration with only groups", + addon: &addonapiv1alpha1.ManagedClusterAddOn{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-addon", + Namespace: "cluster1", + }, + Status: addonapiv1alpha1.ManagedClusterAddOnStatus{ + Registrations: []addonapiv1alpha1.RegistrationConfig{ + { + SignerName: certificatesv1.KubeAPIServerClientSignerName, + Subject: addonapiv1alpha1.Subject{ + Groups: []string{ + "system:open-cluster-management:cluster:cluster1:addon:test-addon", + }, + }, + }, + }, + }, + }, + signerName: certificatesv1.KubeAPIServerClientSignerName, + expectedSubjects: []rbacv1.Subject{ + { + Kind: rbacv1.GroupKind, + APIGroup: rbacv1.GroupName, + Name: "system:open-cluster-management:cluster:cluster1:addon:test-addon", + }, + }, + }, + { + name: "registration with only user", + addon: &addonapiv1alpha1.ManagedClusterAddOn{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-addon", + Namespace: "cluster1", + }, + Status: addonapiv1alpha1.ManagedClusterAddOnStatus{ + Registrations: []addonapiv1alpha1.RegistrationConfig{ + { + SignerName: certificatesv1.KubeAPIServerClientSignerName, + Subject: addonapiv1alpha1.Subject{ + User: "system:serviceaccount:cluster1:test-addon-sa", + }, + }, + }, + }, + }, + signerName: certificatesv1.KubeAPIServerClientSignerName, + expectedSubjects: []rbacv1.Subject{ + { + Kind: rbacv1.UserKind, + APIGroup: rbacv1.GroupName, + Name: "system:serviceaccount:cluster1:test-addon-sa", + }, + }, + }, + { + name: "empty subject", + addon: &addonapiv1alpha1.ManagedClusterAddOn{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-addon", + Namespace: "cluster1", + }, + Status: addonapiv1alpha1.ManagedClusterAddOnStatus{ + Registrations: []addonapiv1alpha1.RegistrationConfig{ + { + SignerName: certificatesv1.KubeAPIServerClientSignerName, + Subject: addonapiv1alpha1.Subject{}, + }, + }, + }, + }, + signerName: certificatesv1.KubeAPIServerClientSignerName, + expectedSubjects: nil, + }, + { + name: "signer name not found", + addon: &addonapiv1alpha1.ManagedClusterAddOn{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-addon", + Namespace: "cluster1", + }, + Status: addonapiv1alpha1.ManagedClusterAddOnStatus{ + Registrations: []addonapiv1alpha1.RegistrationConfig{ + { + SignerName: "custom.signer/name", + Subject: addonapiv1alpha1.Subject{ + User: "test-user", + }, + }, + }, + }, + }, + signerName: certificatesv1.KubeAPIServerClientSignerName, + expectedSubjects: nil, + }, + { + name: "multiple registrations - finds correct one", + addon: &addonapiv1alpha1.ManagedClusterAddOn{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-addon", + Namespace: "cluster1", + }, + Status: addonapiv1alpha1.ManagedClusterAddOnStatus{ + Registrations: []addonapiv1alpha1.RegistrationConfig{ + { + SignerName: "custom.signer/name", + Subject: addonapiv1alpha1.Subject{ + User: "custom-user", + }, + }, + { + SignerName: certificatesv1.KubeAPIServerClientSignerName, + Subject: addonapiv1alpha1.Subject{ + User: "kube-user", + Groups: []string{ + "kube-group", + }, + }, + }, + }, + }, + }, + signerName: certificatesv1.KubeAPIServerClientSignerName, + expectedSubjects: []rbacv1.Subject{ + { + Kind: rbacv1.UserKind, + APIGroup: rbacv1.GroupName, + Name: "kube-user", + }, + { + Kind: rbacv1.GroupKind, + APIGroup: rbacv1.GroupName, + Name: "kube-group", + }, + }, + }, + { + name: "groups with system:authenticated should be filtered out", + addon: &addonapiv1alpha1.ManagedClusterAddOn{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-addon", + Namespace: "cluster1", + }, + Status: addonapiv1alpha1.ManagedClusterAddOnStatus{ + Registrations: []addonapiv1alpha1.RegistrationConfig{ + { + SignerName: certificatesv1.KubeAPIServerClientSignerName, + Subject: addonapiv1alpha1.Subject{ + User: "system:serviceaccount:cluster1:test-addon-sa", + Groups: []string{ + "system:authenticated", + "system:open-cluster-management:cluster:cluster1:addon:test-addon", + "system:serviceaccounts", + }, + }, + }, + }, + }, + }, + signerName: certificatesv1.KubeAPIServerClientSignerName, + expectedSubjects: []rbacv1.Subject{ + { + Kind: rbacv1.UserKind, + APIGroup: rbacv1.GroupName, + Name: "system:serviceaccount:cluster1:test-addon-sa", + }, + { + Kind: rbacv1.GroupKind, + APIGroup: rbacv1.GroupName, + Name: "system:open-cluster-management:cluster:cluster1:addon:test-addon", + }, + { + Kind: rbacv1.GroupKind, + APIGroup: rbacv1.GroupName, + Name: "system:serviceaccounts", + }, + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + subjects := buildSubjectsFromRegistration(c.addon, c.signerName) + if !equality.Semantic.DeepEqual(subjects, c.expectedSubjects) { + t.Errorf("expected subjects %v, got %v", c.expectedSubjects, subjects) + } + }) + } +}