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]>

Author: zhujian7

pkg/addon/templateagent/registration.go

@@ -116,7 +116,7 @@ func (a *CRDTemplateAgentAddon) TemplateCSRConfigurationsFunc() agent.CSRConfigu
 					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",
-							a.addonName, a.agentName, err)
+							cluster.Name, a.addonName, err)
 					}
 					registrationConfigs = append(registrationConfigs, configs...)
 				}
@@ -136,8 +136,8 @@ func (a *CRDTemplateAgentAddon) TemplateCSRConfigurationsFunc() agent.CSRConfigu
 				}
 
 			default:
-				a.logger.Info("CSRConfigurations unsupported registration type", "cluster", cluster.Name, "addon", a.addonName,
-					"type", registration.Type)
+				a.logger.Info("CSRConfigurations unsupported registration type",
+					"clusterName", cluster.Name, "addonName", a.addonName, "type", registration.Type)
 			}
 
 		}
@@ -178,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
 		}
 
@@ -202,8 +205,8 @@ func (a *CRDTemplateAgentAddon) TemplateCSRApproveCheckFunc() agent.CSRApproveFu
 				}
 
 			default:
-				a.logger.Info("CSRApproveCheck unsupported registration type", "cluster", cluster.Name, "addon", a.addonName,
-					"type", registration.Type)
+				a.logger.Info("CSRApproveCheck unsupported registration type",
+					"clusterName", cluster.Name, "addonName", a.addonName, "type", registration.Type)
 			}
 
 		}
@@ -247,11 +250,12 @@ func (a *CRDTemplateAgentAddon) TemplateCSRSignFunc() agent.CSRSignerFunc {
 		csr *certificatesv1.CertificateSigningRequest) ([]byte, error) {
 		template, err := a.GetDesiredAddOnTemplate(addon, cluster.Name, a.addonName)
 		if err != nil {
-			return nil, fmt.Errorf("CSRSign failed to get template for addon %s in cluster %s: %v",
-				a.addonName, cluster.Name, err)
+			return nil, fmt.Errorf("CSRSign failed to get template for addon %s/%s: %v",
+				cluster.Name, a.addonName, err)
 		}
 		if template == nil {
-			return nil, 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 {
@@ -268,8 +272,8 @@ func (a *CRDTemplateAgentAddon) TemplateCSRSignFunc() agent.CSRSignerFunc {
 				}
 
 			default:
-				a.logger.Info("CSRSign unsupported registration type", "cluster", cluster.Name, "addon", a.addonName,
-					"type", registration.Type)
+				a.logger.Info("CSRSign unsupported registration type",
+					"clusterName", cluster.Name, "addonName", a.addonName, "type", registration.Type)
 			}
 
 		}
@@ -286,7 +290,7 @@ func CustomSignerWithExpiry(
 	return func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn,
 		csr *certificatesv1.CertificateSigningRequest) ([]byte, error) {
 		if customSignerConfig == nil {
-			return nil, fmt.Errorf("custome signer config is nil")
+			return nil, fmt.Errorf("custom signer config is nil")
 		}
 
 		if csr.Spec.SignerName != customSignerConfig.SignerName {
@@ -300,13 +304,13 @@ func CustomSignerWithExpiry(
 		caSecret, err := kubeclient.CoreV1().Secrets(secretNamespace).Get(
 			context.TODO(), customSignerConfig.SigningCA.Name, metav1.GetOptions{})
 		if err != nil {
-			return nil, fmt.Errorf("get custome signer ca %s/%s failed, %v",
+			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 {
-			return nil, fmt.Errorf("get ca %s/%s data failed, %v",
+			return nil, fmt.Errorf("get ca %s/%s data failed: %w",
 				secretNamespace, customSignerConfig.SigningCA.Name, err)
 		}
 		return utils.DefaultSignerWithExpiry(caKey, caData, duration)(cluster, addon, csr)
@@ -353,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 {
@@ -376,8 +382,8 @@ func (a *CRDTemplateAgentAddon) TemplatePermissionConfigFunc() agent.PermissionC
 				continue
 
 			default:
-				a.logger.Info("PermissionConfig unsupported registration type", "cluster", cluster.Name, "addon", a.addonName,
-					"type", registration.Type)
+				a.logger.Info("PermissionConfig unsupported registration type",
+					"clusterName", cluster.Name, "addonName", a.addonName, "type", registration.Type)
 			}
 
 		}

pkg/addon/templateagent/registration_test.go

@@ -141,9 +141,16 @@ func TestTemplateCSRConfigurationsFunc(t *testing.T) {
 		agent := NewCRDTemplateAgentAddon(ctx, c.addon.Name, nil, addonClient, addonInformerFactory, nil, nil)
 		f := agent.TemplateCSRConfigurationsFunc()
 		registrationConfigs, err := f(c.cluster, c.addon)
-		if err != nil {
-			if !strings.Contains(c.expectedErr, err.Error()) {
-				t.Fatalf("case: %s, err: %v", c.name, err)
+		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) {
@@ -415,9 +422,16 @@ func TestTemplateCSRSignFunc(t *testing.T) {
 		agent := NewCRDTemplateAgentAddon(ctx, c.addon.Name, hubKubeClient, addonClient, addonInformerFactory, nil, nil)
 		f := agent.TemplateCSRSignFunc()
 		cert, err := f(c.cluster, c.addon, c.csr)
-		if err != nil {
-			if !strings.Contains(c.expectedErr, err.Error()) {
-				t.Fatalf("case: %s, err: %v", c.name, err)
+		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) {