🐛 Use specific addon template instead of default in CSR functions (#1180)

* Upgrade addon framework

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

* Use specific addon template instead of default in CSR functions

- Pass real ManagedClusterAddOn to GetDesiredAddOnTemplate instead of nil
- Enable per-addon template selection using addon.Status.ConfigReferences
- Replace utilruntime.HandleError with explicit error returns
- Update CSRConfigurationsFunc to return ([]RegistrationConfig, error)
- Update CSRSignerFunc to return ([]byte, error)
- Add addon parameter to CSR functions for better context
- Convert runtime errors to structured logging with cluster/addon context
- Update tests to verify error conditions

This allows each ManagedClusterAddOn instance to use its specific template
configuration rather than falling back to the ClusterManagementAddon default.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Signed-off-by: zhujian <[email protected]>

* Fix error assertion logic in registration tests and improve error handling

- Fix inverted error assertion logic in TestTemplateCSRConfigurationsFunc and TestTemplateCSRSignFunc
- Change tests to properly check if expectedErr is empty vs non-empty
- When no error expected, assert err == nil; when error expected, assert err != nil and contains substring
- Fix strings.Contains argument order to check if actual error contains expected substring
- Add nil template checks with proper error messages in CSRSign and PermissionConfig functions
- Improve logging consistency with clusterName/addonName format across CSR functions
- Guard against nil pointer access by checking err == nil before calling err.Error()

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Signed-off-by: zhujian <[email protected]>

---------

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

Author: zhujian7

go.mod

@@ -39,7 +39,7 @@ require (
 	k8s.io/kube-aggregator v0.33.4
 	k8s.io/kubectl v0.33.4
 	k8s.io/utils v0.0.0-20241210054802-24370beab758
-	open-cluster-management.io/addon-framework v1.0.1-0.20250811135502-4bae358d84c6
+	open-cluster-management.io/addon-framework v1.0.1-0.20250910091630-7f19b89a319b
 	open-cluster-management.io/api v1.0.1-0.20250903073454-c6702adf44cc
 	open-cluster-management.io/sdk-go v1.0.1-0.20250911065113-bff262df709b
 	sigs.k8s.io/about-api v0.0.0-20250131010323-518069c31c03

go.sum

@@ -555,8 +555,8 @@ k8s.io/kubectl v0.33.4 h1:nXEI6Vi+oB9hXxoAHyHisXolm/l1qutK3oZQMak4N98=
 k8s.io/kubectl v0.33.4/go.mod h1:Xe7P9X4DfILvKmlBsVqUtzktkI56lEj22SJW7cFy6nE=
 k8s.io/utils v0.0.0-20241210054802-24370beab758 h1:sdbE21q2nlQtFh65saZY+rRM6x6aJJI8IUa1AmH/qa0=
 k8s.io/utils v0.0.0-20241210054802-24370beab758/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
-open-cluster-management.io/addon-framework v1.0.1-0.20250811135502-4bae358d84c6 h1:l6XQEcd6lsvXw5NrKwd/jW+rqz19NRc/MT1gZ1cfBCw=
-open-cluster-management.io/addon-framework v1.0.1-0.20250811135502-4bae358d84c6/go.mod h1:fOPWaRyo6upgHFskcL18Al1kI2Ua9HzrS8uothWEe84=
+open-cluster-management.io/addon-framework v1.0.1-0.20250910091630-7f19b89a319b h1:/ZT+G/UyMa20gy2OnX30IByst0Ca3VV0lgyLt1miHjk=
+open-cluster-management.io/addon-framework v1.0.1-0.20250910091630-7f19b89a319b/go.mod h1:IrMjmd3dLjJtrP2Aqa0Sf/3lDysJHa4j5lNQQ13NxVs=
 open-cluster-management.io/api v1.0.1-0.20250903073454-c6702adf44cc h1:U8O6RhHjp088oWuQsGx6pwwFpOFgWo1gl9qhgIGgDpk=
 open-cluster-management.io/api v1.0.1-0.20250903073454-c6702adf44cc/go.mod h1:lEc5Wkc9ON5ym/qAtIqNgrE7NW7IEOCOC611iQMlnKM=
 open-cluster-management.io/sdk-go v1.0.1-0.20250911065113-bff262df709b h1:tzgcM+yJJBgMwYYbjfzW4kL8p7bsHnScE5lS/69lksE=

pkg/addon/templateagent/registration.go

@@ -16,7 +16,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/klog/v2"
 
@@ -86,16 +85,18 @@ func (a *CRDTemplateAgentAddon) GetDesiredAddOnTemplate(addon *addonapiv1alpha1.
 	return a.getDesiredAddOnTemplateInner(cma.Name, configReferences)
 }
 
-func (a *CRDTemplateAgentAddon) TemplateCSRConfigurationsFunc() func(cluster *clusterv1.ManagedCluster) []addonapiv1alpha1.RegistrationConfig {
+func (a *CRDTemplateAgentAddon) TemplateCSRConfigurationsFunc() agent.CSRConfigurationsFunc {
 
-	return func(cluster *clusterv1.ManagedCluster) []addonapiv1alpha1.RegistrationConfig {
-		template, err := a.GetDesiredAddOnTemplate(nil, cluster.Name, a.addonName)
+	return func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn,
+	) ([]addonapiv1alpha1.RegistrationConfig, error) {
+		template, err := a.GetDesiredAddOnTemplate(addon, cluster.Name, a.addonName)
 		if err != nil {
-			a.logger.Info("CSRConfigurations failed to get addon template", "addonName", a.addonName, "error", err)
-			return nil
+			return nil, fmt.Errorf("CSRConfigurations failed to get addon template for addon %s/%s: %v",
+				cluster.Name, a.addonName, err)
 		}
 		if template == nil {
-			return nil
+			return nil, fmt.Errorf("CSRConfigurations failed to get addon template for addon %s/%s, template is nil",
+				cluster.Name, a.addonName)
 		}
 
 		contain := func(rcs []addonapiv1alpha1.RegistrationConfig, signerName string) bool {
@@ -112,7 +113,11 @@ func (a *CRDTemplateAgentAddon) TemplateCSRConfigurationsFunc() func(cluster *cl
 			switch registration.Type {
 			case addonapiv1alpha1.RegistrationTypeKubeClient:
 				if !contain(registrationConfigs, certificatesv1.KubeAPIServerClientSignerName) {
-					configs := agent.KubeClientSignerConfigurations(a.addonName, a.agentName)(cluster)
+					configs, err := agent.KubeClientSignerConfigurations(a.addonName, a.agentName)(cluster, addon)
+					if err != nil {
+						return nil, fmt.Errorf("failed to get kube signer config for %s/%s: %v",
+							cluster.Name, a.addonName, err)
+					}
 					registrationConfigs = append(registrationConfigs, configs...)
 				}
 
@@ -121,29 +126,34 @@ func (a *CRDTemplateAgentAddon) TemplateCSRConfigurationsFunc() func(cluster *cl
 					continue
 				}
 				if !contain(registrationConfigs, registration.CustomSigner.SignerName) {
-					configs := CustomSignerConfigurations(
+					configs, err := CustomSignerConfigurations(
 						a.addonName, a.agentName, registration.CustomSigner)(cluster)
+					if err != nil {
+						return nil, fmt.Errorf("failed to get custom signer config for %s/%s: %v",
+							cluster.Name, a.addonName, err)
+					}
 					registrationConfigs = append(registrationConfigs, configs...)
 				}
 
 			default:
-				utilruntime.HandleError(fmt.Errorf("unsupported registration type %s", registration.Type))
+				a.logger.Info("CSRConfigurations unsupported registration type",
+					"clusterName", cluster.Name, "addonName", a.addonName, "type", registration.Type)
 			}
 
 		}
 
-		return registrationConfigs
+		return registrationConfigs, nil
 	}
 }
 
 // CustomSignerConfigurations returns a func that can generate RegistrationConfig
 // for CustomSigner type registration addon
 func CustomSignerConfigurations(addonName, agentName string,
 	customSignerConfig *addonapiv1alpha1.CustomSignerRegistrationConfig,
-) func(cluster *clusterv1.ManagedCluster) []addonapiv1alpha1.RegistrationConfig {
-	return func(cluster *clusterv1.ManagedCluster) []addonapiv1alpha1.RegistrationConfig {
+) func(cluster *clusterv1.ManagedCluster) ([]addonapiv1alpha1.RegistrationConfig, error) {
+	return func(cluster *clusterv1.ManagedCluster) ([]addonapiv1alpha1.RegistrationConfig, error) {
 		if customSignerConfig == nil {
-			utilruntime.HandleError(fmt.Errorf("custome signer is nil"))
+			return nil, fmt.Errorf("custom signer config is nil")
 		}
 		config := addonapiv1alpha1.RegistrationConfig{
 			SignerName: customSignerConfig.SignerName,
@@ -157,7 +167,7 @@ func CustomSignerConfigurations(addonName, agentName string,
 			config.Subject = *customSignerConfig.Subject
 		}
 
-		return []addonapiv1alpha1.RegistrationConfig{config}
+		return []addonapiv1alpha1.RegistrationConfig{config}, nil
 	}
 }
 
@@ -168,10 +178,13 @@ func (a *CRDTemplateAgentAddon) TemplateCSRApproveCheckFunc() agent.CSRApproveFu
 
 		template, err := a.GetDesiredAddOnTemplate(addon, cluster.Name, a.addonName)
 		if err != nil {
-			a.logger.Info("CSRApproveCheck failed to get addon template", "addonName", a.addonName, "error", err)
+			a.logger.Info("CSRApproveCheck failed to get addon template",
+				"clusterName", cluster.Name, "addonName", a.addonName, "error", err)
 			return false
 		}
 		if template == nil {
+			a.logger.Info("CSRApproveCheck failed to get addon template, template is nil",
+				"clusterName", cluster.Name, "addonName", a.addonName)
 			return false
 		}
 
@@ -192,7 +205,8 @@ func (a *CRDTemplateAgentAddon) TemplateCSRApproveCheckFunc() agent.CSRApproveFu
 				}
 
 			default:
-				utilruntime.HandleError(fmt.Errorf("unsupported registration type %s", registration.Type))
+				a.logger.Info("CSRApproveCheck unsupported registration type",
+					"clusterName", cluster.Name, "addonName", a.addonName, "type", registration.Type)
 			}
 
 		}
@@ -232,21 +246,16 @@ func CustomerSignerCSRApprover(logger klog.Logger, agentName string) agent.CSRAp
 
 func (a *CRDTemplateAgentAddon) TemplateCSRSignFunc() agent.CSRSignerFunc {
 
-	return func(csr *certificatesv1.CertificateSigningRequest) []byte {
-		// TODO: consider to change the agent.CSRSignerFun to accept parameter addon
-		getClusterName := func(userName string) string {
-			return csr.Labels[clusterv1.ClusterNameLabelKey]
-		}
-
-		clusterName := getClusterName(csr.Spec.Username)
-		template, err := a.GetDesiredAddOnTemplate(nil, clusterName, a.addonName)
+	return func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn,
+		csr *certificatesv1.CertificateSigningRequest) ([]byte, error) {
+		template, err := a.GetDesiredAddOnTemplate(addon, cluster.Name, a.addonName)
 		if err != nil {
-			utilruntime.HandleError(fmt.Errorf("failed to get template for addon %s in cluster %s: %v",
-				a.addonName, clusterName, err))
-			return nil
+			return nil, fmt.Errorf("CSRSign failed to get template for addon %s/%s: %v",
+				cluster.Name, a.addonName, err)
 		}
 		if template == nil {
-			return nil
+			return nil, fmt.Errorf("CSRSign failed to get addon template for addon %s/%s, template is nil",
+				cluster.Name, a.addonName)
 		}
 
 		for _, registration := range template.Spec.Registration {
@@ -259,31 +268,33 @@ func (a *CRDTemplateAgentAddon) TemplateCSRSignFunc() agent.CSRSignerFunc {
 					continue
 				}
 				if csr.Spec.SignerName == registration.CustomSigner.SignerName {
-					return CustomSignerWithExpiry(a.hubKubeClient, registration.CustomSigner, 24*time.Hour)(csr)
+					return CustomSignerWithExpiry(a.hubKubeClient, registration.CustomSigner, 24*time.Hour)(cluster, addon, csr)
 				}
 
 			default:
-				utilruntime.HandleError(fmt.Errorf("unsupported registration type %s", registration.Type))
+				a.logger.Info("CSRSign unsupported registration type",
+					"clusterName", cluster.Name, "addonName", a.addonName, "type", registration.Type)
 			}
 
 		}
 
-		return nil
+		return nil, nil
 	}
 }
 
 func CustomSignerWithExpiry(
 	kubeclient kubernetes.Interface,
 	customSignerConfig *addonapiv1alpha1.CustomSignerRegistrationConfig,
-	duration time.Duration) agent.CSRSignerFunc {
-	return func(csr *certificatesv1.CertificateSigningRequest) []byte {
+	duration time.Duration,
+) agent.CSRSignerFunc {
+	return func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn,
+		csr *certificatesv1.CertificateSigningRequest) ([]byte, error) {
 		if customSignerConfig == nil {
-			utilruntime.HandleError(fmt.Errorf("custome signer is nil"))
-			return nil
+			return nil, fmt.Errorf("custom signer config is nil")
 		}
 
 		if csr.Spec.SignerName != customSignerConfig.SignerName {
-			return nil
+			return nil, nil
 		}
 
 		secretNamespace := AddonManagerNamespace()
@@ -293,18 +304,16 @@ func CustomSignerWithExpiry(
 		caSecret, err := kubeclient.CoreV1().Secrets(secretNamespace).Get(
 			context.TODO(), customSignerConfig.SigningCA.Name, metav1.GetOptions{})
 		if err != nil {
-			utilruntime.HandleError(fmt.Errorf("get custome signer ca %s/%s failed, %v",
-				secretNamespace, customSignerConfig.SigningCA.Name, err))
-			return nil
+			return nil, fmt.Errorf("get custom signer ca %s/%s failed: %w",
+				secretNamespace, customSignerConfig.SigningCA.Name, err)
 		}
 
 		caData, caKey, err := extractCAdata(caSecret.Data[corev1.TLSCertKey], caSecret.Data[corev1.TLSPrivateKeyKey])
 		if err != nil {
-			utilruntime.HandleError(fmt.Errorf("get ca %s/%s data failed, %v",
-				secretNamespace, customSignerConfig.SigningCA.Name, err))
-			return nil
+			return nil, fmt.Errorf("get ca %s/%s data failed: %w",
+				secretNamespace, customSignerConfig.SigningCA.Name, err)
 		}
-		return utils.DefaultSignerWithExpiry(caKey, caData, duration)(csr)
+		return utils.DefaultSignerWithExpiry(caKey, caData, duration)(cluster, addon, csr)
 	}
 }
 
@@ -348,10 +357,12 @@ func (a *CRDTemplateAgentAddon) TemplatePermissionConfigFunc() agent.PermissionC
 	return func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn) error {
 		template, err := a.GetDesiredAddOnTemplate(addon, cluster.Name, a.addonName)
 		if err != nil {
-			return err
+			return fmt.Errorf("PermissionConfig failed to get addon template for addon %s/%s: %v",
+				cluster.Name, a.addonName, err)
 		}
 		if template == nil {
-			return nil
+			return fmt.Errorf("PermissionConfig failed to get addon template for addon %s/%s, template is nil",
+				cluster.Name, a.addonName)
 		}
 
 		for _, registration := range template.Spec.Registration {
@@ -371,7 +382,8 @@ func (a *CRDTemplateAgentAddon) TemplatePermissionConfigFunc() agent.PermissionC
 				continue
 
 			default:
-				utilruntime.HandleError(fmt.Errorf("unsupported registration type %s", registration.Type))
+				a.logger.Info("PermissionConfig unsupported registration type",
+					"clusterName", cluster.Name, "addonName", a.addonName, "type", registration.Type)
 			}
 
 		}

pkg/addon/templateagent/registration_test.go

@@ -37,13 +37,15 @@ func TestTemplateCSRConfigurationsFunc(t *testing.T) {
 		addon           *addonapiv1alpha1.ManagedClusterAddOn
 		template        *addonapiv1alpha1.AddOnTemplate
 		expectedConfigs []addonapiv1alpha1.RegistrationConfig
+		expectedErr     string
 	}{
 		{
 			name:            "empty",
 			cluster:         NewFakeManagedCluster("cluster1"),
 			addon:           NewFakeTemplateManagedClusterAddon("addon1", "cluster1", "", ""),
 			template:        NewFakeAddonTemplate("template1", []addonapiv1alpha1.RegistrationSpec{}),
 			expectedConfigs: []addonapiv1alpha1.RegistrationConfig{},
+			expectedErr:     "CSRConfigurations failed to get addon template for addon cluster1/addon1, template is nil",
 		},
 		{
 			name:    "kubeclient",
@@ -138,7 +140,19 @@ func TestTemplateCSRConfigurationsFunc(t *testing.T) {
 
 		agent := NewCRDTemplateAgentAddon(ctx, c.addon.Name, nil, addonClient, addonInformerFactory, nil, nil)
 		f := agent.TemplateCSRConfigurationsFunc()
-		registrationConfigs := f(c.cluster)
+		registrationConfigs, err := f(c.cluster, c.addon)
+		if c.expectedErr == "" {
+			if err != nil {
+				t.Fatalf("case: %s, expected no error but got: %v", c.name, err)
+			}
+		} else {
+			if err == nil {
+				t.Fatalf("case: %s, expected error but got none", c.name)
+			}
+			if !strings.Contains(err.Error(), c.expectedErr) {
+				t.Fatalf("case: %s, expected error containing %q but got: %v", c.name, c.expectedErr, err)
+			}
+		}
 		if !equality.Semantic.DeepEqual(registrationConfigs, c.expectedConfigs) {
 			t.Errorf("expected registrationConfigs %v, but got %v", c.expectedConfigs, registrationConfigs)
 		}
@@ -275,6 +289,7 @@ func TestTemplateCSRSignFunc(t *testing.T) {
 		casecret     *corev1.Secret
 		csr          *certificatesv1.CertificateSigningRequest
 		expectedCert []byte
+		expectedErr  string
 	}{
 		{
 			name:    "kubeclient",
@@ -344,6 +359,7 @@ func TestTemplateCSRSignFunc(t *testing.T) {
 				},
 			},
 			expectedCert: nil,
+			expectedErr:  `get custom signer ca open-cluster-management-hub/name1 failed: secrets "name1" not found`,
 		},
 		{
 			name:     "customsigner with ca secret",
@@ -383,6 +399,7 @@ func TestTemplateCSRSignFunc(t *testing.T) {
 				},
 			},
 			expectedCert: nil,
+			expectedErr:  "failed to sign csr: PEM block type must be CERTIFICATE REQUEST",
 		},
 	}
 	for _, c := range cases {
@@ -404,7 +421,19 @@ func TestTemplateCSRSignFunc(t *testing.T) {
 
 		agent := NewCRDTemplateAgentAddon(ctx, c.addon.Name, hubKubeClient, addonClient, addonInformerFactory, nil, nil)
 		f := agent.TemplateCSRSignFunc()
-		cert := f(c.csr)
+		cert, err := f(c.cluster, c.addon, c.csr)
+		if c.expectedErr == "" {
+			if err != nil {
+				t.Fatalf("case: %s, expected no error but got: %v", c.name, err)
+			}
+		} else {
+			if err == nil {
+				t.Fatalf("case: %s, expected error but got none", c.name)
+			}
+			if !strings.Contains(err.Error(), c.expectedErr) {
+				t.Fatalf("case: %s, expected error containing %q but got: %v", c.name, c.expectedErr, err)
+			}
+		}
 		if !bytes.Equal(cert, c.expectedCert) {
 			t.Errorf("expected cert %v, but got %v", c.expectedCert, cert)
 		}

test/integration/addon/suite_test.go

@@ -157,14 +157,16 @@ func (t *testAddon) GetAgentAddonOptions() agent.AgentAddonOptions {
 
 	if len(t.registrations) > 0 {
 		option.Registration = &agent.RegistrationOption{
-			CSRConfigurations: func(cluster *clusterv1.ManagedCluster) []addonapiv1alpha1.RegistrationConfig {
-				return t.registrations[cluster.Name]
+			CSRConfigurations: func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn,
+			) ([]addonapiv1alpha1.RegistrationConfig, error) {
+				return t.registrations[cluster.Name], nil
 			},
-			CSRApproveCheck: func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn, csr *certificatesv1.CertificateSigningRequest) bool {
+			CSRApproveCheck: func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn,
+				csr *certificatesv1.CertificateSigningRequest) bool {
 				return t.approveCSR
 			},
-			CSRSign: func(csr *certificatesv1.CertificateSigningRequest) []byte {
-				return t.cert
+			CSRSign: func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn, csr *certificatesv1.CertificateSigningRequest) ([]byte, error) {
+				return t.cert, nil
 			},
 		}
 	}

vendor/modules.txt

@@ -1729,7 +1729,7 @@ k8s.io/utils/path
 k8s.io/utils/pointer
 k8s.io/utils/ptr
 k8s.io/utils/trace
-# open-cluster-management.io/addon-framework v1.0.1-0.20250811135502-4bae358d84c6
+# open-cluster-management.io/addon-framework v1.0.1-0.20250910091630-7f19b89a319b
 ## explicit; go 1.24.0
 open-cluster-management.io/addon-framework/pkg/addonfactory
 open-cluster-management.io/addon-framework/pkg/addonmanager