⚠️ Update CSR interfaces to support enhanced context and error handling #333
Conversation
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]>
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]>
WalkthroughCallback signatures for CSR operations were expanded to accept cluster and addon context and to return errors. Call sites across controllers, agent interfaces, utilities, and tests were updated to the new signatures and now propagate or handle errors accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… 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]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (20)
pkg/addonmanager/controllers/certificate/csrsign.go (3)
139-142: Wrap and enrich signer errors with %w and context (cluster/addon).This preserves the original error and makes triage easier.
- return fmt.Errorf("failed to sign addon csr %q: %v", csr.Name, err) + return fmt.Errorf( + "failed to sign addon csr %q for cluster %q addon %q: %w", + csr.Name, clusterName, addonName, err, + )
98-101: Fix typo in comment ("sigh" → "sign").- // Do not sigh apiserver cert + // Do not sign apiserver cert
119-126: Consider logging NotFound cases for observability.Silent returns on missing cluster/addon can be hard to debug in production. A low-verbosity klog would help trace why a CSR wasn’t processed.
cluster, err := c.managedClusterLister.Get(clusterName) if errors.IsNotFound(err) { - return nil + klog.V(2).Infof("ManagedCluster %q not found for CSR %q; skipping", clusterName, csr.Name) + return nil } @@ addon, err := c.managedClusterAddonLister.ManagedClusterAddOns(clusterName).Get(addonName) if errors.IsNotFound(err) { - return nil + klog.V(2).Infof("ManagedClusterAddOn %q/%q not found for CSR %q; skipping", clusterName, addonName, csr.Name) + return nil }Also applies to: 127-134
pkg/addonmanager/controllers/certificate/csrsign_test.go (3)
87-101: Rename duplicated test case for clarity.This case signs and updates status; the name duplicates an earlier “csr with cert already” (which skips).
- name: "csr with cert already", + name: "approved csr - sign and update status",
95-97: Prefer bytes.Equal for byte slice comparison.More idiomatic than reflect.DeepEqual for []byte.
+ "bytes" @@ - if !reflect.DeepEqual(csr.Status.Certificate, []byte("test")) { + if !bytes.Equal(csr.Status.Certificate, []byte("test")) { t.Errorf("Expect certificate to be updated, actual %v", csr.Status.Certificate) }
45-53: Add an error-path test case.Cover signer returning an error or empty cert to validate controller behavior and error propagation.
I can add a case where CSRSign returns (nil, fmt.Errorf("…")) and assert no update occurs; want me to draft it?
pkg/utils/csr_helpers.go (2)
39-61: Use %w for error wrapping and guard certs slice length.Preserve original errors and avoid a potential panic on certs[0] if ever empty.
- certs, err := x509.ParseCertificates(blockTlsCrt.Bytes) + certs, err := x509.ParseCertificates(blockTlsCrt.Bytes) if err != nil { - return nil, fmt.Errorf("failed to parse cert: %v", err) + return nil, fmt.Errorf("failed to parse cert: %w", err) } + if len(certs) == 0 { + return nil, fmt.Errorf("no certificates parsed from CA data") + } @@ - key, err := x509.ParsePKCS1PrivateKey(blockTlsKey.Bytes) + key, err := x509.ParsePKCS1PrivateKey(blockTlsKey.Bytes) if err != nil { - return nil, fmt.Errorf("failed to parse key: %v", err) + return nil, fmt.Errorf("failed to parse key: %w", err) } @@ - data, err := signCSR(csr, certs[0], key, duration) + data, err := signCSR(csr, certs[0], key, duration) if err != nil { - return nil, fmt.Errorf("failed to sign csr: %v", err) + return nil, fmt.Errorf("failed to sign csr: %w", err) }
31-62: Optionally include cluster/addon in signer error messages.You already accept context; threading cluster/addon names into errors improves debuggability, especially when multiple addons sign concurrently.
pkg/utils/csr_helper_test.go (1)
67-70: Fail fast on signer error to avoid cascading test failuresUse assert/require instead of t.Errorf so the test stops before parsing a bad cert.
- cert, err := signer(nil, nil, newCSR("test", "cluster1")) - if err != nil { - t.Errorf("Failed to sign the csr, %v", err) - } + cert, err := signer(nil, nil, newCSR("test", "cluster1")) + assert.NoError(t, err, "Failed to sign the CSR") + if err != nil { // guard to prevent follow-up panics if assertions are disabled + return + }pkg/agent/inteface.go (4)
238-241: Guard against nil/empty cluster to prevent panicThe closure dereferences cluster.Name; add a defensive check and return a typed error so callers can surface a meaningful status.
Do we guarantee non-nil cluster everywhere this function is used? If not, apply:
func KubeClientSignerConfigurations(addonName, agentName string) CSRConfigurationsFunc { return func(cluster *clusterv1.ManagedCluster, - addon *addonapiv1alpha1.ManagedClusterAddOn) ([]addonapiv1alpha1.RegistrationConfig, error) { + addon *addonapiv1alpha1.ManagedClusterAddOn) ([]addonapiv1alpha1.RegistrationConfig, error) { + if cluster == nil || cluster.Name == "" { + return nil, fmt.Errorf("cluster must be non-nil with a valid name") + } return []addonapiv1alpha1.RegistrationConfig{
101-104: Document new function type contracts (inputs, zero values, error semantics)Add GoDoc to clarify expected behavior and nil/empty handling for implementers.
+// CSRConfigurationsFunc returns registration configurations for an addon on a given cluster. +// Implementations should: +// - Return (nil, nil) to indicate no registrations, or (configs, nil) for one or more. +// - Return a non-nil error when inputs are invalid or configs cannot be constructed. type CSRConfigurationsFunc func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn) ([]addonapiv1alpha1.RegistrationConfig, error) +// CSRSignerFunc signs the provided CSR and returns a PEM-encoded certificate. +// Implementations must not panic; return a non-nil error on failure. type CSRSignerFunc func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn, csr *certificatesv1.CertificateSigningRequest) ([]byte, error)
109-110: Clarify AgentInstallNamespaceFunc return contractExplicitly state that empty string means “use default resolution,” and when to return an error.
- type AgentInstallNamespaceFunc func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error) +// AgentInstallNamespaceFunc returns the namespace to place credentials/agents. +// Return "" to fall back to default resolution logic; return an error for unrecoverable conditions. +type AgentInstallNamespaceFunc func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error)
101-101: Typo in filenameConsider renaming file from “inteface.go” to “interface.go” for consistency and discoverability.
pkg/addonmanager/controllers/registration/controller.go (1)
166-169: Surface CSR configuration errors via status condition before returningMatch the error-handling pattern used for PermissionConfig: set a Condition with Reason and patch status before returning. Also wrap the error with %w.
- configs, err := registrationOption.CSRConfigurations(managedCluster, managedClusterAddonCopy) - if err != nil { - return fmt.Errorf("get csr configurations failed: %v", err) - } + configs, err := registrationOption.CSRConfigurations(managedCluster, managedClusterAddonCopy) + if err != nil { + meta.SetStatusCondition(&managedClusterAddonCopy.Status.Conditions, metav1.Condition{ + Type: addonapiv1alpha1.ManagedClusterAddOnRegistrationApplied, + Status: metav1.ConditionFalse, + Reason: "RegistrationAppliedGetCSRConfigurationsFailed", + Message: fmt.Sprintf("Failed to get CSR configurations: %v", err), + }) + if _, patchErr := addonPatcher.PatchStatus(ctx, managedClusterAddonCopy, managedClusterAddonCopy.Status, managedClusterAddon.Status); patchErr != nil { + return patchErr + } + return fmt.Errorf("failed to get CSR configurations: %w", err) + }pkg/addonmanager/controllers/registration/controller_test.go (2)
45-46: Prefer returning an empty slice instead of nil to avoid nulls in status.
Can help keep status.Registrations consistently [] rather than null.Apply this diff:
- CSRConfigurations: func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn) ([]addonapiv1alpha1.RegistrationConfig, error) { - return t.registrations, nil - }, + CSRConfigurations: func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn) ([]addonapiv1alpha1.RegistrationConfig, error) { + cfgs := t.registrations + if cfgs == nil { + cfgs = []addonapiv1alpha1.RegistrationConfig{} + } + return cfgs, nil + },
61-102: Add an error-path test for CSRConfigurations.
Validate controller behavior when CSRConfigurations returns an error (condition set, no registrations patched).Happy to draft a focused test case if you want.
test/integration/cloudevents/suite_test.go (2)
235-237: OK on signature; coalesce nil to empty for stability.
Prevents null slices surfacing in status/patch assertions.Apply this diff:
- CSRConfigurations: func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn) ([]addonapiv1alpha1.RegistrationConfig, error) { - return t.registrations[cluster.Name], nil - }, + CSRConfigurations: func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn) ([]addonapiv1alpha1.RegistrationConfig, error) { + cfgs := t.registrations[cluster.Name] + if cfgs == nil { + cfgs = []addonapiv1alpha1.RegistrationConfig{} + } + return cfgs, nil + },
241-244: CSRSign update LGTM; consider adding a failing-sign test.
Add a variant returning (nil, err) to assert error propagation and condition updates.I can provide a minimal negative-path test snippet if helpful.
test/integration/kube/suite_test.go (2)
168-171: Signature change aligns with new interface; normalize nil to empty.
Minor guard avoids nil slices in downstream logic.Apply this diff:
- CSRConfigurations: func(cluster *clusterv1.ManagedCluster, - addon *addonapiv1alpha1.ManagedClusterAddOn) ([]addonapiv1alpha1.RegistrationConfig, error) { - return t.registrations[cluster.Name], nil - }, + CSRConfigurations: func(cluster *clusterv1.ManagedCluster, + addon *addonapiv1alpha1.ManagedClusterAddOn) ([]addonapiv1alpha1.RegistrationConfig, error) { + cfgs := t.registrations[cluster.Name] + if cfgs == nil { + cfgs = []addonapiv1alpha1.RegistrationConfig{} + } + return cfgs, nil + },
175-178: CSRSign update LGTM; add a negative-path test.
Exercise a signer that returns an error to verify controller/config flows handle it.I can draft the test case wiring if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pkg/addonmanager/controllers/certificate/csrsign.go(2 hunks)pkg/addonmanager/controllers/certificate/csrsign_test.go(1 hunks)pkg/addonmanager/controllers/registration/controller.go(1 hunks)pkg/addonmanager/controllers/registration/controller_test.go(1 hunks)pkg/agent/inteface.go(5 hunks)pkg/utils/csr_helper_test.go(1 hunks)pkg/utils/csr_helpers.go(1 hunks)test/integration/cloudevents/suite_test.go(2 hunks)test/integration/kube/suite_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: e2e-cloudevents
- GitHub Check: integration
- GitHub Check: e2e
- GitHub Check: unit
- GitHub Check: e2e-hosted
- GitHub Check: cloudevents-integration
- GitHub Check: build
- GitHub Check: verify
🔇 Additional comments (4)
pkg/addonmanager/controllers/certificate/csrsign_test.go (1)
37-40: LGTM on updated CSRSign test stub signature and return.Matches the new public API and keeps tests aligned.
pkg/utils/csr_helpers.go (1)
33-35: LGTM: signer now carries cluster/addon context and returns (data, error).Signature change looks correct and consistent with callers.
pkg/addonmanager/controllers/registration/controller_test.go (1)
45-46: Signature update looks correct and consistent with PR intent.
Matches new CSRConfigurations(cluster, addon) ([]RegistrationConfig, error) and returns (regs, nil).test/integration/cloudevents/suite_test.go (1)
12-13: New options import is correct and used below.
Import aligns with options.NewGenericClientOptions usage.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/agent/inteface.go (1)
241-253: Validate inputs in KubeClientSignerConfigurations to avoid nil deref on cluster.The closure dereferences cluster.Name without checks. Add a guard and return an error—new signature supports it.
func KubeClientSignerConfigurations(addonName, agentName string) CSRConfigurationsFunc { - return func(cluster *clusterv1.ManagedCluster, - addon *addonapiv1alpha1.ManagedClusterAddOn) ([]addonapiv1alpha1.RegistrationConfig, error) { + return func(cluster *clusterv1.ManagedCluster, + addon *addonapiv1alpha1.ManagedClusterAddOn) ([]addonapiv1alpha1.RegistrationConfig, error) { + if cluster == nil || cluster.Name == "" { + return nil, fmt.Errorf("invalid cluster: nil or empty name") + } return []addonapiv1alpha1.RegistrationConfig{ { SignerName: certificatesv1.KubeAPIServerClientSignerName, Subject: addonapiv1alpha1.Subject{ User: DefaultUser(cluster.Name, addonName, agentName), Groups: DefaultGroups(cluster.Name, addonName), }, }, - }, nil + }, nil } }
🧹 Nitpick comments (4)
pkg/agent/inteface.go (4)
1-1: Rename file to interface.go (typo).Spelling “inteface.go” hinders discoverability and grepping. Rename to “interface.go”.
101-106: Document the new function type contracts (nil-safety and error semantics).Add brief GoDoc so implementers know to return errors (not panic) on bad inputs.
+// CSRConfigurationsFunc computes CSR RegistrationConfigs for the given cluster/addon. +// Implementations must NOT panic; return (nil, err) if inputs are invalid or +// configs cannot be generated. type CSRConfigurationsFunc func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn) ([]addonapiv1alpha1.RegistrationConfig, error) +// CSRSignerFunc signs the CSR for the addon agent. On success, return a non-empty +// PEM-encoded X.509 certificate; otherwise return a non-nil error. Do not panic. type CSRSignerFunc func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn, csr *certificatesv1.CertificateSigningRequest) ([]byte, error)
112-113: Add GoDoc for AgentInstallNamespaceFunc.Clarify empty-string vs error behavior.
+// AgentInstallNamespaceFunc returns the managed-cluster namespace to store +// registration credentials. Return "" to use the default; return an error +// if it cannot be determined. type AgentInstallNamespaceFunc func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error)
130-136: Clarify AgentInstallNamespace behavior and fix “registraiton” typo.Mention error path explicitly and keep “empty string = fallback” behavior.
- // AgentInstallNamespace returns the namespace where registraiton credential will be put on the managed cluster. - // It will override the installNamespace on ManagedClusterAddon spec if set and the returned value is not empty. + // AgentInstallNamespace returns the namespace where registration credentials will be put on the managed cluster. + // It overrides ManagedClusterAddOn.spec.installNamespace if set and the returned value is not empty. + // Return an error if the namespace cannot be determined. // 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 AgentInstallNamespaceFunc
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/addonmanager/controllers/certificate/csrsign.go(2 hunks)pkg/addonmanager/controllers/registration/controller.go(1 hunks)pkg/addonmanager/controllers/registration/controller_test.go(1 hunks)pkg/agent/inteface.go(5 hunks)test/integration/cloudevents/suite_test.go(2 hunks)test/integration/kube/suite_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/addonmanager/controllers/registration/controller_test.go
- test/integration/cloudevents/suite_test.go
- test/integration/kube/suite_test.go
- pkg/addonmanager/controllers/registration/controller.go
- pkg/addonmanager/controllers/certificate/csrsign.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: e2e-cloudevents
- GitHub Check: e2e
- GitHub Check: integration
- GitHub Check: e2e-hosted
- GitHub Check: verify
- GitHub Check: unit
🔇 Additional comments (2)
pkg/agent/inteface.go (2)
107-108: LGTM: contextual CSRApproveFunc.Signature extension to include cluster/addon context is appropriate.
120-123: Update CSRConfigurations docs for error semantics and fix typo- // 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. + // CSRConfigurations returns CSR configurations for the addon agent in a managed cluster. + // A CSR will be created for each RegistrationConfig. Implementations should return + // a non-nil error when configurations cannot be generated (e.g., missing context).
| 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, |
There was a problem hiding this comment.
this will change the signature of the function, how do we resolve compatibility problem
There was a problem hiding this comment.
This is a breaking change; it is easy to migrate to the new version. The added cluster and addon parameters can be ignored if not needed. However, the developer still needs to make some slight changes.
There was a problem hiding this comment.
let's ensure we add this to the release note.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, zhujian7 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 7f19b89
into
open-cluster-management-io:main
2 participants
Summary
Why do we need this
The template-type addons need to read the CSR configurations and signing information from the AddonTemplate. Currently, it finds the template from the ClusterManagementAddon, which is not 100% accurate, as the addontemplate could be overridden in the ManagedClusterAddon config. So with this change, we can find the accurate AddonTemplate from the managedclusteraddon status.
Fix this TODO in ocm
Changes
CSRSign Interface
CSRSign(csr *CertificateSigningRequest) []byteCSRSign(cluster *ManagedCluster, addon *ManagedClusterAddOn, csr *CertificateSigningRequest) ([]byte, error)CSRConfigurations Interface
CSRConfigurations(cluster *ManagedCluster) []RegistrationConfigCSRConfigurations(cluster *ManagedCluster, addon *ManagedClusterAddOn) ([]RegistrationConfig, error)Migration Guide
CSRSign implementations:
CSRConfigurations implementations:
Related issue(s)
open-cluster-management-io/ocm#1179
Fixes #
Summary by CodeRabbit
New Features
Refactor
Tests