⚠️ Update CSR interfaces to support enhanced context and error handling (#333)

* Update CSRSign interface to support cluster and addon context

Changes CSRSignerFunc signature to include ManagedCluster and ManagedClusterAddOn parameters and return error, enabling CSR signers to make context-aware decisions based on cluster and addon information.

Migration guide:
- Old: CSRSign: func(csr *certificatesv1.CertificateSigningRequest) []byte
- New: CSRSign: func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn, csr *certificatesv1.CertificateSigningRequest) ([]byte, error)

Update your CSRSign implementations to:
1. Add cluster and addon parameters (can be ignored if not needed)
2. Return ([]byte, error) instead of just []byte
3. Handle errors properly in your signing logic

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

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

* Update CSRConfigurations interface to return error

Changes CSRConfigurationsFunc signature to return ([]RegistrationConfig, error) instead of just []RegistrationConfig, enabling better error handling in CSR configuration generation.

Migration guide:
- Old: CSRConfigurations: func(cluster *clusterv1.ManagedCluster) []addonapiv1alpha1.RegistrationConfig
- New: CSRConfigurations: func(cluster *clusterv1.ManagedCluster) ([]addonapiv1alpha1.RegistrationConfig, error)

Update your CSRConfigurations implementations to:
1. Return ([]RegistrationConfig, error) instead of just []RegistrationConfig
2. Return nil error for successful cases
3. Return appropriate errors when configuration generation fails

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

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

* Update CSRConfigurations to include addon parameter and improve error handling

Enhances CSRConfigurationsFunc to accept ManagedClusterAddOn parameter alongside ManagedCluster, enabling addon-specific CSR configuration decisions. Also improves error message formatting in CSR signing.

Migration guide:
- Old: CSRConfigurations: func(cluster *clusterv1.ManagedCluster) ([]RegistrationConfig, error)
- New: CSRConfigurations: func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn) ([]RegistrationConfig, error)

Update your CSRConfigurations implementations to:
1. Accept both cluster and addon parameters
2. Use addon parameter for addon-specific configuration logic when needed
3. Ignore addon parameter if not needed for your use case

🤖 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

pkg/addonmanager/controllers/certificate/csrsign.go

@@ -116,15 +116,15 @@ func (c *csrSignController) sync(ctx context.Context, syncCtx factory.SyncContex
 	}
 
 	// Get ManagedCluster
-	_, err = c.managedClusterLister.Get(clusterName)
+	cluster, err := c.managedClusterLister.Get(clusterName)
 	if errors.IsNotFound(err) {
 		return nil
 	}
 	if err != nil {
 		return err
 	}
 
-	_, err = c.managedClusterAddonLister.ManagedClusterAddOns(clusterName).Get(addonName)
+	addon, err := c.managedClusterAddonLister.ManagedClusterAddOns(clusterName).Get(addonName)
 	if errors.IsNotFound(err) {
 		return nil
 	}
@@ -136,7 +136,10 @@ func (c *csrSignController) sync(ctx context.Context, syncCtx factory.SyncContex
 		return nil
 	}
 
-	csr.Status.Certificate = registrationOption.CSRSign(csr)
+	csr.Status.Certificate, err = registrationOption.CSRSign(cluster, addon, csr)
+	if err != nil {
+		return fmt.Errorf("failed to sign addon csr %q: %v", csr.Name, err)
+	}
 	if len(csr.Status.Certificate) == 0 {
 		return fmt.Errorf("invalid client certificate generated for addon csr %q", csr.Name)
 	}

pkg/addonmanager/controllers/certificate/csrsign_test.go

@@ -34,8 +34,9 @@ func (t *testSignAgent) GetAgentAddonOptions() agent.AgentAddonOptions {
 	return agent.AgentAddonOptions{
 		AddonName: t.name,
 		Registration: &agent.RegistrationOption{
-			CSRSign: func(csr *certv1.CertificateSigningRequest) []byte {
-				return t.cert
+			CSRSign: func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn,
+				csr *certv1.CertificateSigningRequest) ([]byte, error) {
+				return t.cert, nil
 			},
 		},
 	}

pkg/addonmanager/controllers/registration/controller.go

@@ -163,7 +163,10 @@ func (c *addonRegistrationController) sync(ctx context.Context, syncCtx factory.
 		return err
 	}
 
-	configs := registrationOption.CSRConfigurations(managedCluster)
+	configs, err := registrationOption.CSRConfigurations(managedCluster, managedClusterAddonCopy)
+	if err != nil {
+		return fmt.Errorf("get csr configurations failed: %v", err)
+	}
 	managedClusterAddonCopy.Status.Registrations = configs
 
 	var agentInstallNamespace string

pkg/addonmanager/controllers/registration/controller_test.go

@@ -42,8 +42,8 @@ func (t *testAgent) GetAgentAddonOptions() agent.AgentAddonOptions {
 	agentOption := agent.AgentAddonOptions{
 		AddonName: t.name,
 		Registration: &agent.RegistrationOption{
-			CSRConfigurations: func(cluster *clusterv1.ManagedCluster) []addonapiv1alpha1.RegistrationConfig {
-				return t.registrations
+			CSRConfigurations: func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn) ([]addonapiv1alpha1.RegistrationConfig, error) {
+				return t.registrations, nil
 			},
 			PermissionConfig: func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn) error {
 				return nil

pkg/agent/inteface.go

@@ -98,12 +98,19 @@ type AgentAddonOptions struct {
 	ConfigCheckEnabled bool
 }
 
-type CSRSignerFunc func(csr *certificatesv1.CertificateSigningRequest) []byte
+type CSRConfigurationsFunc func(cluster *clusterv1.ManagedCluster,
+	addon *addonapiv1alpha1.ManagedClusterAddOn) ([]addonapiv1alpha1.RegistrationConfig, error)
 
-type CSRApproveFunc func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn, csr *certificatesv1.CertificateSigningRequest) bool
+type CSRSignerFunc func(cluster *clusterv1.ManagedCluster,
+	addon *addonapiv1alpha1.ManagedClusterAddOn, csr *certificatesv1.CertificateSigningRequest) ([]byte, error)
+
+type CSRApproveFunc func(cluster *clusterv1.ManagedCluster,
+	addon *addonapiv1alpha1.ManagedClusterAddOn, csr *certificatesv1.CertificateSigningRequest) bool
 
 type PermissionConfigFunc func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn) error
 
+type AgentInstallNamespaceFunc func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error)
+
 // RegistrationOption defines how agent is registered to the hub cluster. It needs to define:
 // 1. csr with what subject/signer should be created
 // 2. how csr is approved
@@ -113,7 +120,7 @@ type RegistrationOption struct {
 	// CSRConfigurations returns a list of csr configuration for the adddon agent in a managed cluster.
 	// A csr will be created from the managed cluster for addon agent with each CSRConfiguration.
 	// +required
-	CSRConfigurations func(cluster *clusterv1.ManagedCluster) []addonapiv1alpha1.RegistrationConfig
+	CSRConfigurations CSRConfigurationsFunc
 
 	// Namespace is the namespace where registraiton credential will be put on the managed cluster. It
 	// will be overridden by installNamespace on ManagedClusterAddon spec if set
@@ -125,7 +132,7 @@ type RegistrationOption struct {
 	// Note: Set this very carefully. If this is set, the addon agent must be deployed in the same namespace, which
 	// means when implementing Manifests function in AgentAddon interface, the namespace of the addon agent manifest
 	// must be set to the same value returned by this function.
-	AgentInstallNamespace func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error)
+	AgentInstallNamespace AgentInstallNamespaceFunc
 
 	// CSRApproveCheck checks whether the addon agent registration should be approved by the hub.
 	// Addon hub controller can implement this func to auto-approve all the CSRs. A better CSR check is
@@ -231,8 +238,9 @@ const (
 	HealthProberTypeWorkloadAvailability HealthProberType = "WorkloadAvailability"
 )
 
-func KubeClientSignerConfigurations(addonName, agentName string) func(cluster *clusterv1.ManagedCluster) []addonapiv1alpha1.RegistrationConfig {
-	return func(cluster *clusterv1.ManagedCluster) []addonapiv1alpha1.RegistrationConfig {
+func KubeClientSignerConfigurations(addonName, agentName string) CSRConfigurationsFunc {
+	return func(cluster *clusterv1.ManagedCluster,
+		addon *addonapiv1alpha1.ManagedClusterAddOn) ([]addonapiv1alpha1.RegistrationConfig, error) {
 		return []addonapiv1alpha1.RegistrationConfig{
 			{
 				SignerName: certificatesv1.KubeAPIServerClientSignerName,
@@ -241,7 +249,7 @@ func KubeClientSignerConfigurations(addonName, agentName string) func(cluster *c
 					Groups: DefaultGroups(cluster.Name, addonName),
 				},
 			},
-		}
+		}, nil
 	}
 }
 

pkg/utils/csr_helper_test.go

@@ -64,7 +64,10 @@ func TestDefaultSigner(t *testing.T) {
 
 	signer := DefaultSignerWithExpiry(key, ca, 24*time.Hour)
 
-	cert := signer(newCSR("test", "cluster1"))
+	cert, err := signer(nil, nil, newCSR("test", "cluster1"))
+	if err != nil {
+		t.Errorf("Failed to sign the csr, %v", err)
+	}
 	if cert == nil {
 		t.Errorf("Expect cert to be signed")
 	}

pkg/utils/csr_helpers.go

@@ -30,38 +30,34 @@ var serialNumberLimit = new(big.Int).Lsh(big.NewInt(1), 128)
 
 // DefaultSignerWithExpiry generates a signer func for addon agent to sign the csr using caKey and caData with expiry date.
 func DefaultSignerWithExpiry(caKey, caData []byte, duration time.Duration) agent.CSRSignerFunc {
-	return func(csr *certificatesv1.CertificateSigningRequest) []byte {
+	return func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn,
+		csr *certificatesv1.CertificateSigningRequest) ([]byte, error) {
 		blockTlsCrt, _ := pem.Decode(caData)
 		if blockTlsCrt == nil {
-			klog.Errorf("Failed to decode cert")
-			return nil
+			return nil, fmt.Errorf("failed to decode cert")
 		}
 		certs, err := x509.ParseCertificates(blockTlsCrt.Bytes)
 		if err != nil {
-			klog.Errorf("Failed to parse cert: %v", err)
-			return nil
+			return nil, fmt.Errorf("failed to parse cert: %v", err)
 		}
 
 		blockTlsKey, _ := pem.Decode(caKey)
 		if blockTlsKey == nil {
-			klog.Errorf("Failed to decode key")
-			return nil
+			return nil, fmt.Errorf("failed to decode key")
 		}
 
 		// For now only PKCS#1 is supported which assures the private key algorithm is RSA.
 		// TODO: Compatibility w/ PKCS#8 key e.g. EC algorithm
 		key, err := x509.ParsePKCS1PrivateKey(blockTlsKey.Bytes)
 		if err != nil {
-			klog.Errorf("Failed to parse key: %v", err)
-			return nil
+			return nil, fmt.Errorf("failed to parse key: %v", err)
 		}
 
 		data, err := signCSR(csr, certs[0], key, duration)
 		if err != nil {
-			klog.Errorf("Failed to sign csr: %v", err)
-			return nil
+			return nil, fmt.Errorf("failed to sign csr: %v", err)
 		}
-		return data
+		return data, nil
 	}
 }
 

test/integration/cloudevents/suite_test.go

@@ -3,13 +3,14 @@ package cloudevents
 import (
 	"context"
 	"fmt"
-	"open-cluster-management.io/sdk-go/pkg/cloudevents/clients/options"
 	"os"
 	"path"
 	"path/filepath"
 	"testing"
 	"time"
 
+	"open-cluster-management.io/sdk-go/pkg/cloudevents/clients/options"
+
 	mochimqtt "github.com/mochi-mqtt/server/v2"
 	"github.com/mochi-mqtt/server/v2/hooks/auth"
 	"github.com/mochi-mqtt/server/v2/listeners"
@@ -231,14 +232,15 @@ 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 {
 				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
 			},
 		}
 	}

test/integration/kube/suite_test.go

@@ -165,14 +165,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 {
 				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
 			},
 		}
 	}