🐛 Use specific addon template instead of default in CSR functions #1180
Conversation
WalkthroughRefactors CSR-related template agent APIs to accept ManagedClusterAddOn context and return errors; updates signer signatures to include cluster/addon and return ([]byte, error); passes actual addon to template lookup; improves error propagation/logging; updates unit and integration tests; bumps addon-framework dependency. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Pre-merge checks (3 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1180 +/- ##
==========================================
+ Coverage 58.17% 59.26% +1.08%
==========================================
Files 211 211
Lines 20819 20848 +29
==========================================
+ Hits 12111 12355 +244
+ Misses 7637 7414 -223
- Partials 1071 1079 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: zhujian <[email protected]>
- 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]>
zhujian7
force-pushed
the
addon-todo
branch
from
36fe003 to
545b060
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/addon/templateagent/registration.go (1)
331-336: Compile-time bug: wrong error variable checked.
erris undefined; should checkerrPkcs1. This currently won’t compile and also masks both-parse-failures.- caKey, errPkcs8 = x509.ParsePKCS8PrivateKey(keyBlock.Bytes) - if errPkcs8 != nil { - caKey, errPkcs1 = x509.ParsePKCS1PrivateKey(keyBlock.Bytes) - if err != nil { - return nil, nil, fmt.Errorf("failed to parse ca key with pkcs8: %v and pkcs1: %v", errPkcs8, errPkcs1) - } - } + caKey, errPkcs8 = x509.ParsePKCS8PrivateKey(keyBlock.Bytes) + if errPkcs8 != nil { + caKey, errPkcs1 = x509.ParsePKCS1PrivateKey(keyBlock.Bytes) + if errPkcs1 != nil { + return nil, nil, fmt.Errorf("failed to parse CA key with pkcs8: %v and pkcs1: %v", errPkcs8, errPkcs1) + } + }
🧹 Nitpick comments (10)
pkg/addon/templateagent/registration.go (6)
116-121: Tighten error text and wrap underlying error.Use precise context and %w.
- configs, err := agent.KubeClientSignerConfigurations(a.addonName, a.agentName)(cluster, addon) + 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) + return nil, fmt.Errorf("CSRConfigurations: failed to get kube-client signer config for addon %q agent %q: %w", + a.addonName, a.agentName, err) }
129-135: Consistent error wording and wrapping.Align wording and wrap root cause.
- configs, err := CustomSignerConfigurations( - a.addonName, a.agentName, registration.CustomSigner)(cluster) + 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) + return nil, fmt.Errorf("CSRConfigurations: failed to get custom signer config for cluster %q addon %q: %w", + cluster.Name, a.addonName, err) }
139-141: Standardize log keys.Use a consistent key, e.g., "addonName" (not "addon"), across logs for easier querying.
- a.logger.Info("CSRConfigurations unsupported registration type", "cluster", cluster.Name, "addon", a.addonName, + a.logger.Info("CSRConfigurations unsupported registration type", "cluster", cluster.Name, "addonName", a.addonName, "type", registration.Type) - a.logger.Info("CSRApproveCheck unsupported registration type", "cluster", cluster.Name, "addon", a.addonName, + a.logger.Info("CSRApproveCheck unsupported registration type", "cluster", cluster.Name, "addonName", a.addonName, "type", registration.Type) - a.logger.Info("CSRSign unsupported registration type", "cluster", cluster.Name, "addon", a.addonName, + a.logger.Info("CSRSign unsupported registration type", "cluster", cluster.Name, "addonName", a.addonName, "type", registration.Type) - a.logger.Info("PermissionConfig unsupported registration type", "cluster", cluster.Name, "addon", a.addonName, + a.logger.Info("PermissionConfig unsupported registration type", "cluster", cluster.Name, "addonName", a.addonName, "type", registration.Type)Also applies to: 205-207, 271-273, 379-381
153-157: Fix typos in user-facing errors ("custome" → "custom") and clarify messages.These show up in logs/events; polish them.
- return func(cluster *clusterv1.ManagedCluster) ([]addonapiv1alpha1.RegistrationConfig, error) { - if customSignerConfig == nil { - return nil, fmt.Errorf("custome signer is nil") - } + return func(cluster *clusterv1.ManagedCluster) ([]addonapiv1alpha1.RegistrationConfig, error) { + if customSignerConfig == nil { + return nil, fmt.Errorf("custom signer config is nil") + } ... - if customSignerConfig == nil { - return nil, fmt.Errorf("custome signer config is nil") - } + if customSignerConfig == nil { + return nil, fmt.Errorf("custom signer config is nil") + } ... - caSecret, err := kubeclient.CoreV1().Secrets(secretNamespace).Get( + 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("failed to get custom signer CA %s/%s: %w", secretNamespace, customSignerConfig.SigningCA.Name, err) } ... - caData, caKey, err := extractCAdata(caSecret.Data[corev1.TLSCertKey], caSecret.Data[corev1.TLSPrivateKeyKey]) + 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", - secretNamespace, customSignerConfig.SigningCA.Name, err) + return nil, fmt.Errorf("failed to read CA data %s/%s: %w", + secretNamespace, customSignerConfig.SigningCA.Name, err) }Also applies to: 170-171, 285-290, 300-305, 309-314
246-252: Improve error wrapping and parameter order in message.Minor clarity: mention cluster then addon; wrap with %w.
- if err != nil { - return nil, fmt.Errorf("CSRSign failed to get template for addon %s in cluster %s: %v", - a.addonName, cluster.Name, err) - } + if err != nil { + return nil, fmt.Errorf("CSRSign: failed to get template for cluster %q addon %q: %w", + cluster.Name, a.addonName, err) + }
267-268: Optional: make signer expiry configurable.If the API supports a TTL/expiry on CustomSignerRegistrationConfig, prefer that over a hardcoded 24h default, with fallback to 24h.
pkg/addon/templateagent/registration_test.go (4)
15-16: Drop deprecated v1beta1 certificates import; use v1 everywhere.Prefer certificatesv1 for constants like KubeAPIServerClientSignerName and remove the v1beta1 import to avoid drift and deprecation warnings.
Example:
// use: certificatesv1.KubeAPIServerClientSignerName // and remove the v1beta1 import alias entirely.
40-41: Make error expectation explicit.Document that an empty expectedErr means “no error expected”, or rename to wantErrContains to reflect substring matching. This avoids ambiguity across cases.
285-286: Apply the same explicit error expectation pattern here.Mirror the “no error vs. expected substring” contract in CSR signing tests too for consistency.
355-356: Typo in expected error text (“custome”).If production code indeed says “custome”, consider fixing the user-facing error to “custom signer” and update this expectation accordingly.
Would become:
get custom signer ca open-cluster-management-hub/name1 failed, secrets "name1" not found
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/addon-framework/pkg/addonmanager/controllers/certificate/csrsign.gois excluded by!vendor/**vendor/open-cluster-management.io/addon-framework/pkg/addonmanager/controllers/registration/controller.gois excluded by!vendor/**vendor/open-cluster-management.io/addon-framework/pkg/agent/inteface.gois excluded by!vendor/**vendor/open-cluster-management.io/addon-framework/pkg/utils/csr_helpers.gois excluded by!vendor/**
📒 Files selected for processing (4)
go.mod(1 hunks)pkg/addon/templateagent/registration.go(9 hunks)pkg/addon/templateagent/registration_test.go(6 hunks)test/integration/addon/suite_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-25T01:21:08.891Z
Learnt from: skeeey
PR: open-cluster-management-io/ocm#1077
File: test/integration/registration/spokecluster_grpc_test.go:0-0
Timestamp: 2025-07-25T01:21:08.891Z
Learning: In OCM integration tests, gRPC and kube authentication mechanisms require different CSR handling approaches: gRPC authentication uses util.ApproveCSR since the hub controller signs client certificates, while kube authentication uses authn.ApproveSpokeClusterCSR to simulate the kube-controller-manager signing client certificates.
Applied to files:
test/integration/addon/suite_test.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-singleton
- GitHub Check: e2e-hosted
- GitHub Check: e2e
- GitHub Check: unit
- GitHub Check: integration
- GitHub Check: verify
🔇 Additional comments (4)
go.mod (1)
42-42: Verify addon-framework bump and update sums.Confirm APIs align with the new CSR function signatures across the repo, run
go mod tidyto refresh go.sum, and ensure no implicit replaces are needed.test/integration/addon/suite_test.go (3)
160-163: LGTM: CSRConfigurations callback updated to new signature.Matches framework changes and preserves behavior.
164-166: LGTM: CSRApproveCheck signature alignment.No behavioral change; compiles against new interface.
169-170: LGTM: CSRSign now returns ([]byte, error).Aligned with new agent.CSRSignerFunc.
| 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) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent nil-template handling + clearer error context.
CSRSign/Approve treat a missing template as “no-op”, but CSRConfigurations returns an error. Recommend returning an empty slice (no registrations) for consistency and to avoid needless failures. Also improve error text and wrap with %w.
Apply:
- template, err := a.GetDesiredAddOnTemplate(addon, cluster.Name, a.addonName)
+ template, err := a.GetDesiredAddOnTemplate(addon, cluster.Name, a.addonName)
if err != nil {
- return nil, fmt.Errorf("CSRConfigurations failed to get addon template for addon %s/%s: %v",
- cluster.Name, a.addonName, err)
+ return nil, fmt.Errorf("CSRConfigurations: failed to get addon template for cluster %q addon %q: %w",
+ cluster.Name, a.addonName, err)
}
if template == nil {
- return nil, fmt.Errorf("CSRConfigurations failed to get addon template for addon %s/%s, template is nil",
- cluster.Name, a.addonName)
+ a.logger.V(4).Info("CSRConfigurations: no addon template found; returning no registrations",
+ "cluster", cluster.Name, "addonName", a.addonName)
+ return nil, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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) | |
| } | |
| func (a *CRDTemplateAgentAddon) TemplateCSRConfigurationsFunc() agent.CSRConfigurationsFunc { | |
| return func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn, | |
| ) ([]addonapiv1alpha1.RegistrationConfig, error) { | |
| template, err := a.GetDesiredAddOnTemplate(addon, cluster.Name, a.addonName) | |
| if err != nil { | |
| return nil, fmt.Errorf("CSRConfigurations: failed to get addon template for cluster %q addon %q: %w", | |
| cluster.Name, a.addonName, err) | |
| } | |
| if template == nil { | |
| a.logger.V(4).Info("CSRConfigurations: no addon template found; returning no registrations", | |
| "cluster", cluster.Name, "addonName", a.addonName) | |
| return nil, nil | |
| } |
🤖 Prompt for AI Agents
In pkg/addon/templateagent/registration.go around lines 88 to 100, the function
treats a missing template inconsistently (CSRSign/Approve are no-ops) and
currently returns errors; change behavior to return an empty
[]addonapiv1alpha1.RegistrationConfig slice (no registrations) when template is
nil, and update error handling to wrap the underlying error using %w and provide
clearer context when GetDesiredAddOnTemplate returns an error (e.g., "failed to
get addon template for addon <addonName> on cluster <clusterName>"); ensure the
returned types remain ([]addonapiv1alpha1.RegistrationConfig, error).
e9e1005 to
a899f1b
Compare
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/addon/templateagent/registration.go (1)
333-341: Bug: wrong error identifier causes compile-time error (undefined: err).Use errPkcs1 in the check.
- if errPkcs8 != nil { - caKey, errPkcs1 = x509.ParsePKCS1PrivateKey(keyBlock.Bytes) - if err != nil { - return nil, nil, fmt.Errorf("failed to parse ca key with pkcs8: %v and pkcs1: %v", errPkcs8, errPkcs1) - } - } + if errPkcs8 != nil { + caKey, errPkcs1 = x509.ParsePKCS1PrivateKey(keyBlock.Bytes) + if errPkcs1 != nil { + return nil, nil, fmt.Errorf("failed to parse ca key with pkcs8: %v and pkcs1: %v", errPkcs8, errPkcs1) + } + }
♻️ Duplicate comments (2)
pkg/addon/templateagent/registration.go (2)
94-100: Use %w and make nil-template a no-op for consistency with Approve/Sign.
- Wrap underlying errors with %w and clearer context.
- Treat missing template as “no registrations” instead of error to align behavior with Approve/Sign flows. This mirrors the earlier suggestion on this hunk.
- if err != 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, fmt.Errorf("CSRConfigurations failed to get addon template for addon %s/%s, template is nil", - cluster.Name, a.addonName) - } + if err != nil { + return nil, fmt.Errorf("CSRConfigurations: failed to get addon template for cluster %q addon %q: %w", + cluster.Name, a.addonName, err) + } + if template == nil { + a.logger.V(4).Info("CSRConfigurations: no addon template found; returning no registrations", + "cluster", cluster.Name, "addonName", a.addonName) + return nil, nil + } @@ - if err != nil { - return nil, fmt.Errorf("failed to get kube signer config for %s/%s: %v", - cluster.Name, a.addonName, err) - } + if err != nil { + return nil, fmt.Errorf("CSRConfigurations: failed to get kube signer config for cluster %q addon %q: %w", + cluster.Name, a.addonName, err) + } @@ - if err != nil { - return nil, fmt.Errorf("failed to get custom signer config for %s/%s: %v", - cluster.Name, a.addonName, err) - } + if err != nil { + return nil, fmt.Errorf("CSRConfigurations: failed to get custom signer config for cluster %q addon %q: %w", + cluster.Name, a.addonName, err) + } @@ - return registrationConfigs, nil + return registrationConfigs, nilAlso applies to: 118-121, 132-135, 145-146
249-255: CSRSign: wrap errors with %w; make nil-template a no-op.Keep behavior consistent with “no template” meaning “do nothing”.
- if err != 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, fmt.Errorf("CSRSign failed to get addon template for addon %s/%s, template is nil", - cluster.Name, a.addonName) - } + if err != nil { + return nil, fmt.Errorf("CSRSign: failed to get addon template for cluster %q addon %q: %w", + cluster.Name, a.addonName, err) + } + if template == nil { + a.logger.V(4).Info("CSRSign: no addon template found; signing no-op", + "cluster", cluster.Name, "addonName", a.addonName) + return nil, nil + } @@ - return nil, nil + return nil, nilAlso applies to: 257-259, 281-282
🧹 Nitpick comments (4)
pkg/addon/templateagent/registration.go (4)
102-111: Micro: O(1) de-dupe instead of scanning.Use a set (map[string]struct{}) for signer names to avoid repeated linear scans in contain().
153-157: Typos in error text; clarify message.“custome” → “custom”; specify “custom signer config”.
- if customSignerConfig == nil { - return nil, fmt.Errorf("custome signer is nil") - } + if customSignerConfig == nil { + return nil, fmt.Errorf("custom signer config is nil") + }Also applies to: 170-171
181-183: Standardize log key names.Use consistent keys like "cluster" and "addonName" across logs.
- a.logger.Info("CSRApproveCheck failed to get addon template", - "clusterName", cluster.Name, "addonName", a.addonName, "error", err) + a.logger.Info("CSRApproveCheck failed to get addon template", + "cluster", cluster.Name, "addonName", a.addonName, "error", err) @@ - a.logger.Info("CSRApproveCheck failed to get addon template, template is nil", - "clusterName", cluster.Name, "addonName", a.addonName) + a.logger.Info("CSRApproveCheck failed to get addon template, template is nil", + "cluster", cluster.Name, "addonName", a.addonName) @@ - a.logger.Info("CSRApproveCheck unsupported registration type", - "clusterName", cluster.Name, "addonName", a.addonName, "type", registration.Type) + a.logger.Info("CSRApproveCheck unsupported registration type", + "cluster", cluster.Name, "addonName", a.addonName, "type", registration.Type)Also applies to: 186-187, 208-210
288-294: Typos and error wrapping in CustomSignerWithExpiry.Fix “custome” typos; wrap with %w; improve messages.
- if customSignerConfig == nil { - return nil, fmt.Errorf("custome signer config is nil") - } + if customSignerConfig == nil { + return nil, fmt.Errorf("custom signer config is nil") + } @@ - if csr.Spec.SignerName != customSignerConfig.SignerName { - return nil, nil - } + if csr.Spec.SignerName != customSignerConfig.SignerName { + return nil, nil + } @@ - return nil, fmt.Errorf("get custome signer ca %s/%s failed, %v", - secretNamespace, customSignerConfig.SigningCA.Name, err) + return nil, fmt.Errorf("CustomSignerWithExpiry: get custom signer CA %s/%s failed: %w", + secretNamespace, customSignerConfig.SigningCA.Name, err) @@ - if err != nil { - return nil, fmt.Errorf("get ca %s/%s data failed, %v", - secretNamespace, customSignerConfig.SigningCA.Name, err) - } + if err != nil { + return nil, fmt.Errorf("CustomSignerWithExpiry: get CA %s/%s data failed: %w", + secretNamespace, customSignerConfig.SigningCA.Name, err) + }Also applies to: 297-298, 307-309, 314-317
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/addon/templateagent/registration.go(11 hunks)pkg/addon/templateagent/registration_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/addon/templateagent/registration_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/addon/templateagent/registration.go (1)
pkg/addon/templateagent/template_agent.go (1)
CRDTemplateAgentAddon(63-77)
⏰ 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). (4)
- GitHub Check: integration
- GitHub Check: e2e-hosted
- GitHub Check: e2e
- GitHub Check: e2e-singleton
🔇 Additional comments (1)
pkg/addon/templateagent/registration.go (1)
90-93: Passing addon into GetDesiredAddOnTemplate — aligns with #1179.This fixes the root cause (default CMA template fallback) by providing addon context. Good change.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/addon/templateagent/registration.go (1)
335-341: Bug: wrong variable checked after PKCS1 parse fallback.This condition checks a stale “err”; it should check errPkcs1. As written, a PKCS1 parse failure won’t be detected.
- caKey, errPkcs1 = x509.ParsePKCS1PrivateKey(keyBlock.Bytes) - if err != nil { - return nil, nil, fmt.Errorf("failed to parse ca key with pkcs8: %v and pkcs1: %v", errPkcs8, errPkcs1) - } + caKey, errPkcs1 = x509.ParsePKCS1PrivateKey(keyBlock.Bytes) + if errPkcs1 != nil { + return nil, nil, fmt.Errorf("failed to parse ca key with pkcs8: %v and pkcs1: %v", errPkcs8, errPkcs1) + }pkg/addon/templateagent/registration_test.go (1)
1-1: Fix 'custome' typos & migrate certificates/v1beta1 imports
- Correct "custome" → "custom" in error strings/code and update tests that assert the typo: pkg/addon/templateagent/registration.go:156,293,307 and pkg/addon/templateagent/registration_test.go:362.
- Replace k8s.io/api/certificates/v1beta1 imports with k8s.io/api/certificates/v1 and adapt usages; occurrences found in: test/e2e/managedcluster_loopback_test.go:10, pkg/addon/templateagent/registration_test.go:16, pkg/registration/register/csr/{approver_beta_test.go,hub_driver.go,approver_test.go,certficate_beta.go,approve_reconciler.go,certificate_beta_test.go}, pkg/registration/helpers/{testing/testinghelpers.go,testing/assertion.go,helpers.go}.
- The "CSRConfigurations failed to get addon template... template is nil" string currently matches code and test (pkg/addon/templateagent/registration.go:98 and pkg/addon/templateagent/registration_test.go:48) — update tests only if you intend to change that message.
♻️ Duplicate comments (2)
pkg/addon/templateagent/registration.go (1)
88-100: Return no-op when template is missing; wrap underlying error.To keep behavior consistent with CSRApprove and CSRSign no-op semantics, return an empty registration set (and nil error) when template is nil. Also wrap the upstream error with %w for context.
- template, err := a.GetDesiredAddOnTemplate(addon, cluster.Name, a.addonName) + template, err := a.GetDesiredAddOnTemplate(addon, cluster.Name, a.addonName) if err != nil { - return nil, fmt.Errorf("CSRConfigurations failed to get addon template for addon %s/%s: %v", - cluster.Name, a.addonName, err) + return nil, fmt.Errorf("CSRConfigurations: failed to get addon template for cluster %q addon %q: %w", + cluster.Name, a.addonName, err) } if template == nil { - return nil, fmt.Errorf("CSRConfigurations failed to get addon template for addon %s/%s, template is nil", - cluster.Name, a.addonName) + a.logger.V(4).Info("CSRConfigurations: no addon template found; returning no registrations", + "cluster", cluster.Name, "addonName", a.addonName) + return nil, nil }pkg/addon/templateagent/registration_test.go (1)
41-49: If CSRConfigurations adopts no-op on nil template, update this expectedErr.Set expectedErr to empty and keep expectedConfigs empty.
- expectedErr: "CSRConfigurations failed to get addon template for addon cluster1/addon1, template is nil", + expectedErr: "",
🧹 Nitpick comments (5)
pkg/addon/templateagent/registration.go (3)
116-121: Use %w to wrap errors from signer config helpers.Preserve the original error for callers and tests while adding context.
- configs, err := agent.KubeClientSignerConfigurations(a.addonName, a.agentName)(cluster, addon) + 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) + return nil, fmt.Errorf("failed to get kube signer config for %s/%s: %w", + cluster.Name, a.addonName, err) } ... - configs, err := CustomSignerConfigurations( - a.addonName, a.agentName, registration.CustomSigner)(cluster) + 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) + return nil, fmt.Errorf("failed to get custom signer config for %s/%s: %w", + cluster.Name, a.addonName, err) }Also applies to: 129-137
249-259: Align nil-template handling with no-op; wrap errors.CSRSign should treat a missing template as a no-op (return nil, nil) like Approve; also wrap errors with %w.
- template, err := a.GetDesiredAddOnTemplate(addon, cluster.Name, a.addonName) + template, err := a.GetDesiredAddOnTemplate(addon, cluster.Name, a.addonName) if err != nil { - return nil, fmt.Errorf("CSRSign failed to get template for addon %s/%s: %v", - cluster.Name, a.addonName, err) + return nil, fmt.Errorf("CSRSign: failed to get addon template for cluster %q addon %q: %w", + cluster.Name, a.addonName, err) } if template == nil { - return nil, fmt.Errorf("CSRSign failed to get addon template for addon %s/%s, template is nil", - cluster.Name, a.addonName) + a.logger.V(4).Info("CSRSign: no addon template found; returning no cert", + "clusterName", cluster.Name, "addonName", a.addonName) + return nil, nil } ... - return CustomSignerWithExpiry(a.hubKubeClient, registration.CustomSigner, 24*time.Hour)(cluster, addon, csr) + return CustomSignerWithExpiry(a.hubKubeClient, registration.CustomSigner, 24*time.Hour)(cluster, addon, csr) ... - return nil, nil + return nil, nilIf you adopt this, please adjust tests that assert on the previous “template is nil” error string. I can send a patch.
Also applies to: 271-272, 275-277, 281-281
360-366: Prefer %w for error wrapping in PermissionConfig.Slightly improves debuggability.
- return fmt.Errorf("PermissionConfig failed to get addon template for addon %s/%s: %v", - cluster.Name, a.addonName, err) + return fmt.Errorf("PermissionConfig: failed to get addon template for cluster %q addon %q: %w", + cluster.Name, a.addonName, err) ... - return fmt.Errorf("PermissionConfig failed to get addon template for addon %s/%s, template is nil", - cluster.Name, a.addonName) + return fmt.Errorf("PermissionConfig: addon template is nil for cluster %q addon %q", + cluster.Name, a.addonName)Also applies to: 385-387
pkg/addon/templateagent/registration_test.go (2)
15-17: Use v1 SignerName constant; drop v1beta1 import.Modernize tests and avoid v1beta1 dependency.
- certificates "k8s.io/api/certificates/v1beta1" + // v1beta1 not needed; use v1 constants @@ - SignerName: certificates.KubeAPIServerClientSignerName, + SignerName: certificatesv1.KubeAPIServerClientSignerName, @@ - SignerName: certificates.KubeAPIServerClientSignerName, + SignerName: certificatesv1.KubeAPIServerClientSignerName, @@ - SignerName: certificates.KubeAPIServerClientSignerName, + SignerName: certificatesv1.KubeAPIServerClientSignerName,Also applies to: 74-88, 203-209, 318-326
745-747: Nit: error message text is misleading.This asserts on an error, not “registrationConfigs”.
- t.Errorf("expected registrationConfigs %v, but got %v", c.expectedErr, err) + t.Errorf("expected error %v, but got %v", c.expectedErr, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/addon/templateagent/registration.go(11 hunks)pkg/addon/templateagent/registration_test.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/addon/templateagent/registration.go (1)
pkg/addon/templateagent/template_agent.go (1)
CRDTemplateAgentAddon(63-77)
⏰ 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). (4)
- GitHub Check: integration
- GitHub Check: e2e-hosted
- GitHub Check: e2e
- GitHub Check: e2e-singleton
🔇 Additional comments (3)
pkg/addon/templateagent/registration.go (1)
232-245: Add spelled-correct alias CustomSignerCSRApprover and deprecate CustomerSignerCSRApprover.Only internal references found in this repo: pkg/addon/templateagent/registration.go — call at line 204; definition at lines 232–245. Add an exported alias and deprecate the old name to avoid breaking external consumers:
+// CustomSignerCSRApprover is a spelled-correct alias for CustomerSignerCSRApprover. +// Deprecated: use CustomSignerCSRApprover; CustomerSignerCSRApprover will be removed in a future release. +func CustomSignerCSRApprover(logger klog.Logger, agentName string) agent.CSRApproveFunc { + return CustomerSignerCSRApprover(logger, agentName) +}pkg/addon/templateagent/registration_test.go (2)
143-155: Nice cleanup on error assertions.The nil/non-nil contract and substring direction are now correct.
362-363: Sync expectedErr with corrected "custom" spelling.- expectedErr: `get custome signer ca open-cluster-management-hub/name1 failed, secrets "name1" not found`, + expectedErr: `get custom signer ca open-cluster-management-hub/name1 failed: secrets "name1" not found`,
zhujian7
force-pushed
the
addon-todo
branch
2 times, most recently
from
c134757 to
66f2e2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/addon/templateagent/registration.go (2)
333-341: Bug: wrong error variable checked in PKCS#1 fallback.This checks a stale ‘err’ (from ParseCertificate) instead of ‘errPkcs1’, masking parse failures.
Apply:
caKey, errPkcs8 = x509.ParsePKCS8PrivateKey(keyBlock.Bytes) if errPkcs8 != nil { caKey, errPkcs1 = x509.ParsePKCS1PrivateKey(keyBlock.Bytes) - if err != nil { - return nil, nil, fmt.Errorf("failed to parse ca key with pkcs8: %v and pkcs1: %v", errPkcs8, errPkcs1) + if errPkcs1 != nil { + return nil, nil, fmt.Errorf("failed to parse ca key with pkcs8: %v and pkcs1: %v", errPkcs8, errPkcs1) } }
204-205: Rename CustomerSignerCSRApprover → CustomSignerCSRApproverTypo in function name and log; update call site and function definition in pkg/addon/templateagent/registration.go (occurrences at ~lines 204 and 232-245).
- return CustomerSignerCSRApprover(a.logger, a.addonName)(cluster, addon, csr) + return CustomSignerCSRApprover(a.logger, a.addonName)(cluster, addon, csr)-func CustomerSignerCSRApprover(logger klog.Logger, agentName string) agent.CSRApproveFunc { +func CustomSignerCSRApprover(logger klog.Logger, agentName string) agent.CSRApproveFunc { return func( cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn, csr *certificatesv1.CertificateSigningRequest) bool { - logger.Info("Customer signer CSR is approved", + logger.Info("Custom signer CSR is approved", "clusterName", cluster.Name, "addonName", addon.Name, "requester", csr.Spec.Username) return true } }
🧹 Nitpick comments (4)
pkg/addon/templateagent/registration_test.go (1)
401-403: Reduce brittleness of error matching.Upstream message can change; assert on the stable fragment.
Apply:
- expectedErr: "failed to sign csr: PEM block type must be CERTIFICATE REQUEST", + expectedErr: "CERTIFICATE REQUEST",pkg/addon/templateagent/registration.go (3)
94-96: Wrap underlying errors with %w for better causality.Use
%winstead of%vto enableerrors.Is/Asand improve debugging.Apply:
- return nil, fmt.Errorf("CSRConfigurations failed to get addon template for addon %s/%s: %v", + return nil, fmt.Errorf("CSRConfigurations failed to get addon template for addon %s/%s: %w", cluster.Name, a.addonName, err) - return nil, fmt.Errorf("failed to get kube signer config for %s/%s: %v", + return nil, fmt.Errorf("failed to get kube signer config for %s/%s: %w", cluster.Name, a.addonName, err) - return nil, fmt.Errorf("failed to get custom signer config for %s/%s: %v", + return nil, fmt.Errorf("failed to get custom signer config for %s/%s: %w", cluster.Name, a.addonName, err) - return nil, fmt.Errorf("CSRSign failed to get template for addon %s/%s: %v", + return nil, fmt.Errorf("CSRSign failed to get template for addon %s/%s: %w", cluster.Name, a.addonName, err) - return fmt.Errorf("PermissionConfig failed to get addon template for addon %s/%s: %v", + return fmt.Errorf("PermissionConfig failed to get addon template for addon %s/%s: %w", cluster.Name, a.addonName, err)Also applies to: 118-120, 132-134, 253-255, 360-362
102-111: Minor: avoid O(n^2) contains scans.For many registrations, use a set for signerName de-dup. Low impact here, but cleaner.
Apply:
- contain := func(rcs []addonapiv1alpha1.RegistrationConfig, signerName string) bool { - for _, rc := range rcs { - if rc.SignerName == signerName { - return true - } - } - return false - } + seen := map[string]struct{}{} + contain := func(signerName string) bool { + _, ok := seen[signerName] + return ok + }And after each append:
- registrationConfigs = append(registrationConfigs, configs...) + registrationConfigs = append(registrationConfigs, configs...) + for _, c := range configs { + seen[c.SignerName] = struct{}{} + }
458-461: Label value for AddonTemplate is empty.If feasible, set AddonTemplateLabelKey to the template name to aid discovery/GC; otherwise consider omitting the label.
Would you like a patch that threads the template name into createPermissionBinding?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/addon/templateagent/registration.go(11 hunks)pkg/addon/templateagent/registration_test.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/addon/templateagent/registration.go (1)
pkg/addon/templateagent/template_agent.go (1)
CRDTemplateAgentAddon(63-77)
⏰ 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). (4)
- GitHub Check: e2e-singleton
- GitHub Check: integration
- GitHub Check: e2e-hosted
- GitHub Check: e2e
🔇 Additional comments (1)
pkg/addon/templateagent/registration_test.go (1)
143-155: Good fix: robust error assertion logic.The nil vs non-nil contract and substring direction are now correct and prevent false passes.
…dling - 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]>
zhujian7
force-pushed
the
addon-todo
branch
from
66f2e2a to
ecc0f09
Compare
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/addon/templateagent/registration.go (1)
333-341: Bug: wrong error variable checked when parsing CA key (can mask failures).The code checks
if err != nilinstead ofif errPkcs1 != nil, potentially allowing a nil/invalid key through if PKCS#1 parse fails while earliererris nil.- var errPkcs8, errPkcs1 error - var caKey any - caKey, errPkcs8 = x509.ParsePKCS8PrivateKey(keyBlock.Bytes) - if errPkcs8 != nil { - caKey, errPkcs1 = x509.ParsePKCS1PrivateKey(keyBlock.Bytes) - if err != nil { - return nil, nil, fmt.Errorf("failed to parse ca key with pkcs8: %v and pkcs1: %v", errPkcs8, errPkcs1) - } - } + var caKey any + caKey, errPkcs8 := x509.ParsePKCS8PrivateKey(keyBlock.Bytes) + if errPkcs8 != nil { + caKeyPKCS1, errPkcs1 := x509.ParsePKCS1PrivateKey(keyBlock.Bytes) + if errPkcs1 != nil { + return nil, nil, fmt.Errorf("failed to parse ca key with pkcs8: %w and pkcs1: %w", errPkcs8, errPkcs1) + } + caKey = caKeyPKCS1 + }
♻️ Duplicate comments (1)
pkg/addon/templateagent/registration.go (1)
88-100: Make nil-template a no-op and wrap underlying error with %w.Align behavior with Approve/Sign paths: return no registrations when template is absent, and wrap errors for unwrapping.
- return func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn, - ) ([]addonapiv1alpha1.RegistrationConfig, error) { + return func(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn, + ) ([]addonapiv1alpha1.RegistrationConfig, error) { template, err := a.GetDesiredAddOnTemplate(addon, cluster.Name, a.addonName) if err != nil { - return nil, fmt.Errorf("CSRConfigurations failed to get addon template for addon %s/%s: %v", - cluster.Name, a.addonName, err) + return nil, fmt.Errorf("CSRConfigurations: failed to get addon template for cluster %q addon %q: %w", + cluster.Name, a.addonName, err) } if template == nil { - return nil, fmt.Errorf("CSRConfigurations failed to get addon template for addon %s/%s, template is nil", - cluster.Name, a.addonName) + a.logger.V(4).Info("CSRConfigurations: no addon template found; returning no registrations", + "cluster", cluster.Name, "addonName", a.addonName) + return nil, nil }
🧹 Nitpick comments (4)
pkg/addon/templateagent/registration.go (4)
116-121: Use %w for error wrapping on kube signer config.- configs, err := agent.KubeClientSignerConfigurations(a.addonName, a.agentName)(cluster, addon) + 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) + return nil, fmt.Errorf("failed to get kube signer config for cluster %q addon %q: %w", + cluster.Name, a.addonName, err) }
129-135: Use %w for error wrapping on custom signer config.- configs, err := CustomSignerConfigurations( - a.addonName, a.agentName, registration.CustomSigner)(cluster) + 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) + return nil, fmt.Errorf("failed to get custom signer config for cluster %q addon %q: %w", + cluster.Name, a.addonName, err) }
249-259: Prefer no-op on missing template; wrap errors.For consistency with Approve and suggested CSRConfigurations behavior, treat nil template as “no cert to sign” instead of an error; wrap the retrieval error with %w.
- template, err := a.GetDesiredAddOnTemplate(addon, cluster.Name, a.addonName) + template, err := a.GetDesiredAddOnTemplate(addon, cluster.Name, a.addonName) if err != nil { - return nil, fmt.Errorf("CSRSign failed to get template for addon %s/%s: %v", - cluster.Name, a.addonName, err) + return nil, fmt.Errorf("CSRSign: failed to get addon template for cluster %q addon %q: %w", + cluster.Name, a.addonName, err) } if template == nil { - return nil, fmt.Errorf("CSRSign failed to get addon template for addon %s/%s, template is nil", - cluster.Name, a.addonName) + a.logger.V(4).Info("CSRSign: no addon template found; returning no certificate", + "cluster", cluster.Name, "addonName", a.addonName) + return nil, nil }
360-366: Wrap errors and consider no-op on nil template in PermissionConfig.Use %w for error chaining. Optionally treat nil template as no-op since there’s nothing to bind.
- return fmt.Errorf("PermissionConfig failed to get addon template for addon %s/%s: %v", - cluster.Name, a.addonName, err) + return fmt.Errorf("PermissionConfig: failed to get addon template for cluster %q addon %q: %w", + cluster.Name, a.addonName, err)Optional:
- if template == nil { - return fmt.Errorf("PermissionConfig failed to get addon template for addon %s/%s, template is nil", - cluster.Name, a.addonName) - } + if template == nil { + a.logger.V(4).Info("PermissionConfig: no addon template found; nothing to bind", + "cluster", cluster.Name, "addonName", a.addonName) + return nil + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/addon/templateagent/registration.go(11 hunks)pkg/addon/templateagent/registration_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/addon/templateagent/registration_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/addon/templateagent/registration.go (1)
pkg/addon/templateagent/template_agent.go (1)
CRDTemplateAgentAddon(63-77)
⏰ 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: unit
- GitHub Check: integration
- GitHub Check: verify
- GitHub Check: e2e-hosted
- GitHub Check: e2e
- GitHub Check: e2e-singleton
🔇 Additional comments (3)
pkg/addon/templateagent/registration.go (3)
153-157: Good: clear validation error.
181-188: LGTM on switching to contextual logs for template retrieval failures.
307-316: Nice: switched to %w and clarified messages in CustomSignerWithExpiry.
|
[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 ab41b86
into
open-cluster-management-io:main
Summary
Related issue(s)
Fixes #1179
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores