From 7323d2047ab4ee8c9e478ded4f9c035f37cea702 Mon Sep 17 00:00:00 2001 From: Jian Zhu Date: Fri, 30 Jan 2026 19:44:53 +0800 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Support=20token-based=20authenticat?= =?UTF-8?q?ion=20for=20template=20addons=20(#1363)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ✨ Support token-based authentication for template addons This change enables template type addons to work with both CSR-based and token-based authentication through dynamic subject binding. Changes: - Modified createPermissionBinding() to extract dynamic subjects from addon.Status.Registrations instead of using hardcoded groups - Added buildSubjectsFromRegistration() helper to extract user/groups from registration status - Returns SubjectNotReadyError when subjects not ready (enables retry) - Removed clusterAddonGroup() function (no longer needed) - Updated addon-framework dependency to v1.2.0 for SubjectNotReadyError - Added comprehensive tests for buildSubjectsFromRegistration - Updated test helpers to include registration status with proper subjects The implementation now supports: - CSR-based authentication (existing) - Token-based authentication (new) - Any future authentication method that populates Status.Registrations Related: https://github.com/elgnay/ocm-enhancements/blob/14af2a2eeb527d30b2c7f196a51e6b7e270be8c8/enhancements/sig-architecture/167-token-based-addon-registration/README.md 🤖 Generated with Claude Code https://claude.com/claude-code Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: zhujian * test: add unit test for system:authenticated group filtering Add a test case to verify that buildSubjectsFromRegistration correctly filters out the system:authenticated group from the list of groups when building RBAC subjects. This covers the filtering logic in registration.go lines 560-562. Also update the expected groups in TestTemplateCSRConfigurationsFunc to match the implementation that includes both cluster-specific and addon-wide groups for token-based authentication. Signed-off-by: Claude Signed-off-by: zhujian * feat: add addon-wide group and filter system:authenticated Add support for addon-wide group in defaultGroups() to support token-based authentication for template addons. This adds the system:open-cluster-management:addon:{addonName} group in addition to the cluster-specific group. Also add filtering logic in buildSubjectsFromRegistration() to exclude the system:authenticated group from RBAC subjects, as this is a special Kubernetes group automatically added to all authenticated users and should not be explicitly included in RoleBindings. Signed-off-by: Claude Signed-off-by: zhujian * refactor: implement custom CSR approver with flexible org validation Replace addon-framework's DefaultCSRApprover with a custom implementation that supports both legacy and new CSR organization structures. Key changes: - Implement defaultCSRApprover function that accepts 2 or 3 organization units - 3 orgs: legacy behavior including system:authenticated group in CSRs - 2 orgs: new behavior where system:authenticated is filtered out - Add support for gRPC-based CSR requests by checking CSRUsernameAnnotation - Validate all required default addon groups are present in CSR - Add necessary imports: k8s.io/apimachinery/pkg/util/sets and operatorapiv1 This enables backward compatibility while supporting the new token-based authentication flow where system:authenticated is excluded from CSR orgs but included in registration configs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: zhujian * refactor: use addon-framework's updated KubeClientSignerConfigurations Remove custom implementations and use addon-framework's native functions which now include system:authenticated group by default. Changes: - Remove custom kubeClientSignerConfigurations function - Remove custom defaultGroups function - Remove custom defaultCSRApprover function - Use agent.KubeClientSignerConfigurations from addon-framework - Use utils.DefaultCSRApprover from addon-framework - Remove unused imports: k8s.io/apimachinery/pkg/util/sets and operatorapiv1 The addon-framework has been updated to include system:authenticated in DefaultGroups(), eliminating the need for custom implementations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: zhujian --------- Signed-off-by: zhujian Signed-off-by: Claude Co-authored-by: Claude Sonnet 4.5 --- pkg/addon/templateagent/registration.go | 79 ++++- pkg/addon/templateagent/registration_test.go | 291 ++++++++++++++++++- 2 files changed, 351 insertions(+), 19 deletions(-) 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) + } + }) + } +}