✨ support add-on registration #412
Conversation
|
/hold |
WalkthroughRenames CSR concepts to CustomSigner in addon v1beta1, adds Driver (csr|token) to kubeClient and v1alpha1 registration configs, introduces AddOnKubeClientRegistrationDriver (authType + token.expirationSeconds) in operator API, and updates CRDs, conversions, deepcopies, and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-12-01T01:10:01.745ZApplied to files:
🧬 Code graph analysis (4)addon/v1beta1/zz_generated.conversion.go (2)
operator/v1/zz_generated.deepcopy.go (1)
addon/v1beta1/types_managedclusteraddon.go (1)
addon/v1beta1/conversion_test.go (1)
⏰ 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). (2)
🔇 Additional comments (28)
✏️ Tip: You can disable this entire section by setting 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 |
![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 (2)
operator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml (1)
188-216: Consider adding validation for token expiration minimum value.The CRD schema correctly reflects the Go type definitions. However, the documentation states "The minimum valid value for production use is 3600 (1 hour)", but there's no schema validation to enforce this minimum. While smaller values are mentioned as allowed for testing, adding a minimum constraint with a warning would help prevent misconfiguration in production environments.
🔎 Consider adding minimum validation
expirationSeconds: description: |- ExpirationSeconds represents the seconds of a token to expire. If it is not set or 0, the default duration will be used, which is the same as the certificate expiration set by the hub cluster's kube-controller-manager (typically 1 year). The minimum valid value for production use is 3600 (1 hour), though smaller values are allowed for testing. format: int64 + minimum: 0 type: integerNote: A minimum of 0 allows testing while preventing negative values. Runtime validation could still warn about values below 3600 in production.
operator/v1/types_klusterlet.go (1)
239-261: Consider adding validation for token expiration and field relationships.The new types are well-structured and documented. However, there are two validation gaps:
Production minimum not enforced: The comment mentions "minimum valid value for production use is 3600 (1 hour)", but there's no kubebuilder validation tag to enforce this. While runtime validation may handle this, schema-level validation would prevent misconfiguration earlier.
Field relationship: The
Tokenfield should logically only be set whenAuthTypeis "token". Consider adding validation or documentation to clarify this relationship.🔎 Consider adding validation constraints
For the minimum value:
type TokenConfig struct { // ExpirationSeconds represents the seconds of a token to expire. // If it is not set or 0, the default duration will be used, which is // the same as the certificate expiration set by the hub cluster's // kube-controller-manager (typically 1 year). // The minimum valid value for production use is 3600 (1 hour), though smaller values are allowed for testing. // +optional + // +kubebuilder:validation:Minimum=0 ExpirationSeconds int64 `json:"expirationSeconds,omitempty"` }For field relationship documentation:
// Token contains the configuration for token-based registration. + // This field should only be set when AuthType is "token". // +optional Token *TokenConfig `json:"token,omitempty"`
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1beta1/types_managedclusteraddon.gooperator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yamloperator/v1/types_klusterlet.gooperator/v1/zz_generated.deepcopy.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: haoqing0110
Repo: open-cluster-management-io/api PR: 407
File: addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml:17-778
Timestamp: 2025-12-01T01:10:01.745Z
Learning: In the open-cluster-management-io/api repository, CRD files for addon resources (like ManagedClusterAddOn and ClusterManagementAddOn) intentionally omit conversion webhook configuration in their base definitions. The registration-operator patches these CRDs at deployment time with the appropriate webhook clientConfig and CA bundle. This is a deliberate architectural pattern to separate schema definitions from deployment-specific webhook configuration.
📚 Learning: 2025-12-01T01:10:01.745Z
Learnt from: haoqing0110
Repo: open-cluster-management-io/api PR: 407
File: addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml:17-778
Timestamp: 2025-12-01T01:10:01.745Z
Learning: In the open-cluster-management-io/api repository, CRD files for addon resources (like ManagedClusterAddOn and ClusterManagementAddOn) intentionally omit conversion webhook configuration in their base definitions. The registration-operator patches these CRDs at deployment time with the appropriate webhook clientConfig and CA bundle. This is a deliberate architectural pattern to separate schema definitions from deployment-specific webhook configuration.
Applied to files:
addon/v1beta1/types_managedclusteraddon.goaddon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamloperator/v1/types_klusterlet.gooperator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml
🧬 Code graph analysis (1)
operator/v1/zz_generated.deepcopy.go (1)
operator/v1/types_klusterlet.go (2)
AddOnRegistrationDriver(239-251)TokenConfig(253-261)
⏰ 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). (1)
- GitHub Check: verify
🔇 Additional comments (5)
addon/v1beta1/types_managedclusteraddon.go (1)
80-85: LGTM! Clean addition of driver field.The new
Driverfield is well-documented and properly validated with enum constraints. The field being optional and set by the agent to declare which driver it's using is a good design pattern.addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml (1)
684-691: LGTM! CRD schema correctly reflects the Go type.The schema definition for the
driverfield is consistent with the Go type, including proper enum validation and description.operator/v1/zz_generated.deepcopy.go (1)
36-55: LGTM! Auto-generated deep copy methods are correct.The generated DeepCopy implementations properly handle:
- Pointer fields with nil checks (Token field in AddOnRegistrationDriver)
- Nested struct copying
- Integration with RegistrationConfiguration
Also applies to: 833-833, 1044-1058
operator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml (1)
356-356: Minor description improvement noted.The description update from "with hub" to "klusterlet agent with hub" adds clarity.
operator/v1/types_klusterlet.go (1)
181-187: LGTM! Clear extension of registration configuration.The addition of
AddOnKubeClientRegistrationDriveris well-documented and clearly separates add-on registration concerns from klusterlet agent registration.
elgnay
force-pushed
the
support-addon-registration
branch
from
b6b1c72 to
23323d3
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)
addon/v1beta1/types_managedclusteraddon.go (1)
59-63: Validation enum mismatch with constant value.The kubebuilder validation annotation specifies
Enum=kubeClient;csr, but theCustomSignerconstant is defined as"customSigner"(line 55). This means the API will reject"customSigner"as an invalid value while accepting"csr".Either:
- Update the enum to
Enum=kubeClient;customSigner, or- Keep backward compatibility by accepting both values
🔎 Suggested fix if customSigner should be the only accepted value
// type specifies the type of registration configuration. -// +kubebuilder:validation:Enum=kubeClient;csr +// +kubebuilder:validation:Enum=kubeClient;customSigner // +required Type RegistrationType `json:"type"`
🧹 Nitpick comments (1)
addon/v1beta1/conversion_test.go (1)
635-646: Minor: Error message still references "CSR".The error message on line 637 says
"Registrations[%d].CSR is nil"but should say"CustomSigner is nil"to match the renamed field.🔎 Suggested fix
if got.Status.Registrations[i].Type == CustomSigner { if got.Status.Registrations[i].CustomSigner == nil { - t.Errorf("Registrations[%d].CSR is nil", i) + t.Errorf("Registrations[%d].CustomSigner is nil", i) } else {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1alpha1/types_managedclusteraddon.goaddon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1beta1/conversion.goaddon/v1beta1/conversion_test.goaddon/v1beta1/types_managedclusteraddon.goaddon/v1beta1/zz_generated.conversion.goaddon/v1beta1/zz_generated.deepcopy.gooperator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yamloperator/v1/types_klusterlet.gooperator/v1/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (2)
- operator/v1/types_klusterlet.go
- operator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: haoqing0110
Repo: open-cluster-management-io/api PR: 407
File: addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml:17-778
Timestamp: 2025-12-01T01:10:01.745Z
Learning: In the open-cluster-management-io/api repository, CRD files for addon resources (like ManagedClusterAddOn and ClusterManagementAddOn) intentionally omit conversion webhook configuration in their base definitions. The registration-operator patches these CRDs at deployment time with the appropriate webhook clientConfig and CA bundle. This is a deliberate architectural pattern to separate schema definitions from deployment-specific webhook configuration.
📚 Learning: 2025-12-01T01:10:01.745Z
Learnt from: haoqing0110
Repo: open-cluster-management-io/api PR: 407
File: addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml:17-778
Timestamp: 2025-12-01T01:10:01.745Z
Learning: In the open-cluster-management-io/api repository, CRD files for addon resources (like ManagedClusterAddOn and ClusterManagementAddOn) intentionally omit conversion webhook configuration in their base definitions. The registration-operator patches these CRDs at deployment time with the appropriate webhook clientConfig and CA bundle. This is a deliberate architectural pattern to separate schema definitions from deployment-specific webhook configuration.
Applied to files:
addon/v1alpha1/types_managedclusteraddon.goaddon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1beta1/conversion_test.goaddon/v1beta1/types_managedclusteraddon.goaddon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1beta1/zz_generated.conversion.goaddon/v1beta1/conversion.go
🧬 Code graph analysis (5)
addon/v1beta1/conversion_test.go (2)
addon/v1beta1/types_managedclusteraddon.go (12)
CustomSigner(55-55)CustomSignerConfig(88-99)ManagedClusterAddOn(19-32)ManagedClusterAddOnSpec(36-43)AddOnConfig(101-107)ManagedClusterAddOnStatus(140-195)RegistrationConfig(59-74)Subject(130-136)KubeClient(51-51)KubeClientConfig(76-86)KubeClientSubject(119-121)BaseSubject(110-117)addon/v1alpha1/types_managedclusteraddon.go (6)
ManagedClusterAddOn(19-32)ManagedClusterAddOnSpec(36-52)AddOnConfig(89-95)ManagedClusterAddOnStatus(113-174)RegistrationConfig(56-87)Subject(98-109)
addon/v1beta1/types_managedclusteraddon.go (1)
addon/v1alpha1/types_managedclusteraddon.go (1)
Subject(98-109)
addon/v1beta1/zz_generated.conversion.go (2)
addon/v1alpha1/types_managedclusteraddon.go (1)
RegistrationConfig(56-87)addon/v1beta1/types_managedclusteraddon.go (1)
RegistrationConfig(59-74)
operator/v1/zz_generated.deepcopy.go (1)
operator/v1/types_klusterlet.go (2)
AddOnRegistrationDriver(239-251)TokenConfig(253-261)
addon/v1beta1/zz_generated.deepcopy.go (1)
addon/v1beta1/types_managedclusteraddon.go (3)
CustomSignerConfig(88-99)Subject(130-136)CustomSigner(55-55)
⏰ 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). (1)
- GitHub Check: verify
🔇 Additional comments (20)
operator/v1/zz_generated.deepcopy.go (3)
36-55: LGTM - DeepCopy for AddOnRegistrationDriver follows standard patterns.The deep copy correctly handles the optional
Tokenpointer field by nil-checking and allocating a newTokenConfigbefore copying.
1044-1058: LGTM - TokenConfig DeepCopy is correct.Since
TokenConfigonly contains theExpirationSeconds int64field (per the relevant code snippets), a simple value copy (*out = *in) is sufficient.
833-833: AddOnKubeClientRegistrationDriver is correctly defined as a value type.The field in
RegistrationConfiguration(operator/v1/types_klusterlet.go, line 187) is declared asAddOnKubeClientRegistrationDriver AddOnRegistrationDriver, confirming it is a value type (not a pointer). TheDeepCopyIntocall in the generated deepcopy code is consistent with this type definition.addon/v1beta1/types_managedclusteraddon.go (4)
53-55: LGTM - Constant rename from CSR to CustomSigner.The rename aligns with the broader refactoring to clarify that this registration type is for custom signers, not just CSRs.
70-73: LGTM - CustomSigner field replaces CSR.The field rename and type change to
CustomSignerConfigis consistent with the broader refactoring.
80-85: LGTM - Driver field added to KubeClientConfig.The new
Driverfield withEnum=csr;tokenallows agents to declare which authentication mechanism they use for kubeClient registration.
88-99: LGTM - CustomSignerConfig struct is well-defined.The struct includes appropriate validation constraints on
SignerName(length, pattern) and an optionalSubjectfield.addon/v1beta1/zz_generated.conversion.go (1)
905-916: LGTM - Generated conversion warnings correctly updated.The warning comments accurately reflect that:
in.CustomSigner(v1beta1 → v1alpha1) requires manual conversionin.Driver(v1alpha1 → v1beta1) requires manual conversionThe actual conversion logic is properly implemented in
addon/v1beta1/conversion.go.addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml (1)
303-315: LGTM - Driver field added to v1alpha1 CRD.The field is properly:
- Marked as deprecated with clear migration path to
kubeClient.driverin v1beta1- Scoped to apply only when
signerNameiskubernetes.io/kube-apiserver-client- Constrained with enum validation (
csr,token)Based on learnings, the absence of conversion webhook configuration is intentional - it's patched by registration-operator at deployment time.
addon/v1alpha1/types_managedclusteraddon.go (1)
76-86: LGTM - Driver field added to RegistrationConfig.The field is properly:
- Marked as deprecated with clear migration path
- Documented to explain its scope (only applies for
kubernetes.io/kube-apiserver-client)- Validated with enum constraint
This aligns with the corresponding CRD and conversion logic changes.
addon/v1beta1/conversion_test.go (3)
452-468: LGTM - Test updated to use CustomSigner.The test case correctly validates the conversion of custom signer registrations, using the new
CustomSignertype and field names.
469-568: LGTM - Good test coverage for Driver field conversions.These test cases comprehensively cover:
v1alpha1.Driver="csr"→v1beta1.KubeClient.Driver="csr"v1alpha1.Driver="token"→v1beta1.KubeClient.Driver="token"This ensures the Driver field is correctly propagated during conversion.
784-883: LGTM - Comprehensive driver conversion tests (v1beta1 → v1alpha1).These test cases verify the reverse conversion path, ensuring
v1beta1.KubeClient.Drivercorrectly propagates tov1alpha1.Driver.addon/v1beta1/conversion.go (2)
111-133: LGTM - v1beta1 → v1alpha1 conversion correctly handles Driver and CustomSigner.The conversion logic correctly:
- Copies
in.KubeClient.Drivertoout.Driver(line 121)- Uses
in.CustomSignerinstead of the oldin.CSRpath- Properly converts the Subject via
Convert_v1beta1_Subject_To_v1alpha1_Subject
136-163: LGTM - v1alpha1 → v1beta1 conversion correctly handles Driver and CustomSigner.The conversion logic correctly:
- Passes
in.DrivertoKubeClientConfig.Driver(line 146)- Uses
out.CustomSignerandCustomSignerConfigfor non-KubeClient registrations- Properly maps
OrganizationUnitsto the CustomSigner Subjectaddon/v1beta1/zz_generated.deepcopy.go (2)
259-274: LGTM! Auto-generated deepcopy methods for CustomSignerConfig.The deepcopy implementation correctly handles the nested Subject field, which contains slices that require deep copying. The generated code follows standard patterns for Kubernetes deepcopy-gen.
628-632: LGTM! Updated to use CustomSigner instead of CSR.The change from CSR to CustomSigner in RegistrationConfig.DeepCopyInto is consistent with the API refactoring across the codebase.
addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml (3)
658-691: LGTM! CustomSigner correctly replaces CSR.The field rename from
csrtocustomSignerin v1beta1 is consistent with the API refactoring. The schema preserves the same structure (signerName + subject with organizationUnits) with improved naming clarity.
697-704: LGTM! Driver field properly scoped under kubeClient.The driver field is correctly positioned under the kubeClient registration type with appropriate enum constraints (csr, token). The description clearly indicates this is set by the agent to declare which authentication driver it uses.
303-315: LGTM! Clear deprecation path for driver field with proper conversion handling.The v1alpha1 driver field is properly marked as deprecated with clear guidance that it will be replaced by
kubeClient.driverin v1beta1. The enum constraints (csr, token) and scope limitation tokubernetes.io/kube-apiserver-clientsigner are appropriate.The conversion webhook correctly handles the bidirectional migration:
- v1alpha1 → v1beta1: driver field is mapped to
registrations[].kubeClient.driver- v1beta1 → v1alpha1:
kubeClient.driveris converted back to the driver fieldThe conversion logic is properly tested in the conversion test suite, which verifies the driver field values are preserved during version migrations. Since v1alpha1 remains the storage version, this approach ensures backward compatibility.
elgnay
force-pushed
the
support-addon-registration
branch
2 times, most recently
from
c9c0ac3 to
0ce554f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @addon/v1beta1/conversion_test.go:
- Around line 635-641: Update the stale test error messages that reference "CSR"
to use "CustomSigner" so they match the checked fields; specifically, in the
block that checks got.Status.Registrations[i].Type == CustomSigner and inspects
got.Status.Registrations[i].CustomSigner, change the t.Errorf strings that say
"Registrations[%d].CSR is nil" and "Registrations[%d].CSR.SignerName..." to
"Registrations[%d].CustomSigner is nil" and
"Registrations[%d].CustomSigner.SignerName..." (and any similar occurrences) so
the messages accurately reflect the CustomSigner checks.
🧹 Nitpick comments (3)
operator/v1/types_klusterlet.go (1)
239-261: LGTM - AddOnRegistrationDriver and TokenConfig types.The new types are well-designed:
AuthTypeenum (csr,token) is consistent with the addon APITokenConfig.ExpirationSecondsdocumentation is clear about defaults and minimum values- The comment noting that customSigner always uses CSR driver clarifies the scope
Consider adding minimum validation for ExpirationSeconds.
The documentation mentions "minimum valid value for production use is 3600" but there's no kubebuilder validation marker. While allowing smaller values for testing is intentional, consider if a validation warning or marker would help prevent misconfiguration.
test/integration/api/managedclusteraddon_test.go (2)
198-203: Consider asserting the specific error type for clarity.The test correctly expects an error for an invalid driver value, but asserting the specific error type would make the test more robust and explicit about the expected failure mode, consistent with other validation tests in this file (e.g., line 335).
🔎 Proposed improvement
- gomega.Expect(err).To(gomega.HaveOccurred()) + gomega.Expect(errors.IsInvalid(err)).To(gomega.BeTrue())
686-692: Consider asserting the specific error type for consistency.Similar to the v1alpha1 invalid driver test, asserting
errors.IsInvalid(err)would make the test more explicit about the expected validation failure.🔎 Proposed improvement
- gomega.Expect(err).To(gomega.HaveOccurred()) + gomega.Expect(errors.IsInvalid(err)).To(gomega.BeTrue())
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1alpha1/types_managedclusteraddon.goaddon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1beta1/conversion.goaddon/v1beta1/conversion_test.goaddon/v1beta1/types_managedclusteraddon.goaddon/v1beta1/zz_generated.conversion.goaddon/v1beta1/zz_generated.deepcopy.gooperator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yamloperator/v1/types_klusterlet.gooperator/v1/zz_generated.deepcopy.gotest/integration/api/managedclusteraddon_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- addon/v1beta1/zz_generated.deepcopy.go
- addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml
- operator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: haoqing0110
Repo: open-cluster-management-io/api PR: 407
File: addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml:17-778
Timestamp: 2025-12-01T01:10:01.745Z
Learning: In the open-cluster-management-io/api repository, CRD files for addon resources (like ManagedClusterAddOn and ClusterManagementAddOn) intentionally omit conversion webhook configuration in their base definitions. The registration-operator patches these CRDs at deployment time with the appropriate webhook clientConfig and CA bundle. This is a deliberate architectural pattern to separate schema definitions from deployment-specific webhook configuration.
📚 Learning: 2025-12-01T01:10:01.745Z
Learnt from: haoqing0110
Repo: open-cluster-management-io/api PR: 407
File: addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml:17-778
Timestamp: 2025-12-01T01:10:01.745Z
Learning: In the open-cluster-management-io/api repository, CRD files for addon resources (like ManagedClusterAddOn and ClusterManagementAddOn) intentionally omit conversion webhook configuration in their base definitions. The registration-operator patches these CRDs at deployment time with the appropriate webhook clientConfig and CA bundle. This is a deliberate architectural pattern to separate schema definitions from deployment-specific webhook configuration.
Applied to files:
addon/v1beta1/zz_generated.conversion.goaddon/v1beta1/types_managedclusteraddon.goaddon/v1beta1/conversion.goaddon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1alpha1/types_managedclusteraddon.gooperator/v1/types_klusterlet.goaddon/v1beta1/conversion_test.gotest/integration/api/managedclusteraddon_test.go
📚 Learning: 2025-09-02T11:59:50.703Z
Learnt from: qiujian16
Repo: open-cluster-management-io/api PR: 392
File: operator/v1/types_clustermanager.go:178-195
Timestamp: 2025-09-02T11:59:50.703Z
Learning: XValidation markers in kubebuilder require Kubernetes 1.25+ and should not be added to the open-cluster-management-io/api project until minimum Kubernetes version requirements are established to avoid breaking compatibility with older clusters.
Applied to files:
addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml
🧬 Code graph analysis (6)
addon/v1beta1/zz_generated.conversion.go (2)
addon/v1alpha1/types_managedclusteraddon.go (1)
RegistrationConfig(56-87)addon/v1beta1/types_managedclusteraddon.go (1)
RegistrationConfig(59-74)
addon/v1beta1/types_managedclusteraddon.go (1)
addon/v1alpha1/types_managedclusteraddon.go (1)
Subject(98-109)
operator/v1/zz_generated.deepcopy.go (1)
operator/v1/types_klusterlet.go (2)
AddOnRegistrationDriver(239-251)TokenConfig(253-261)
addon/v1beta1/conversion.go (2)
addon/v1beta1/types_managedclusteraddon.go (4)
KubeClient(51-51)CustomSigner(55-55)Subject(130-136)CustomSignerConfig(88-99)addon/v1alpha1/types_managedclusteraddon.go (1)
Subject(98-109)
addon/v1beta1/conversion_test.go (1)
addon/v1beta1/types_managedclusteraddon.go (6)
CustomSigner(55-55)CustomSignerConfig(88-99)RegistrationConfig(59-74)Subject(130-136)KubeClient(51-51)KubeClientConfig(76-86)
test/integration/api/managedclusteraddon_test.go (2)
addon/v1alpha1/types_managedclusteraddon.go (4)
ManagedClusterAddOn(19-32)ManagedClusterAddOnSpec(36-52)RegistrationConfig(56-87)Subject(98-109)addon/v1beta1/types_managedclusteraddon.go (10)
ManagedClusterAddOn(19-32)ManagedClusterAddOnSpec(36-43)RegistrationConfig(59-74)Subject(130-136)CustomSigner(55-55)CustomSignerConfig(88-99)KubeClient(51-51)KubeClientConfig(76-86)KubeClientSubject(119-121)BaseSubject(110-117)
⏰ 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). (1)
- GitHub Check: verify
🔇 Additional comments (22)
addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml (2)
303-315: LGTM - v1alpha1 driver field addition.The new
driverfield is appropriately marked as deprecated with a clear migration path tokubeClient.driverin v1beta1. The enum validation (csr,token) is consistent with the v1beta1 counterpart.
658-661: LGTM - v1beta1 customSigner and driver additions.The renaming from CSR to customSigner is cleanly implemented:
customSignerfield properly encapsulates signerName and subject configurationdriverfield added tokubeClientwith matching enum validation- Type enum updated to include
customSignerAlso applies to: 697-704, 723-723
addon/v1beta1/zz_generated.conversion.go (1)
905-917: LGTM - Generated conversion warnings updated.The auto-generated conversion functions correctly identify the fields requiring manual conversion:
Type,KubeClient,CustomSigner(v1beta1→v1alpha1) andSignerName,Subject,Driver(v1alpha1→v1beta1). These are handled in the manualconversion.gofile.addon/v1alpha1/types_managedclusteraddon.go (1)
76-87: LGTM - Driver field addition to v1alpha1 RegistrationConfig.The new
Driverfield is properly:
- Marked as deprecated with clear migration path to v1beta1
- Documented with its semantic (only applies when signerName is
kubernetes.io/kube-apiserver-client)- Validated with enum constraint (
csr,token)- Tagged as optional with
omitemptyoperator/v1/zz_generated.deepcopy.go (3)
36-55: LGTM - DeepCopy for AddOnRegistrationDriver.The generated deep copy correctly handles the
Tokenpointer field by creating a newTokenConfigand copying the value.
833-833: LGTM - RegistrationConfiguration deepcopy updated.The
AddOnKubeClientRegistrationDriverfield is properly deep-copied into the output struct.
1044-1058: LGTM - DeepCopy for TokenConfig.Since
TokenConfigonly contains a primitiveint64field, the simple value copy is correct.addon/v1beta1/conversion.go (2)
111-133: LGTM - v1beta1 to v1alpha1 RegistrationConfig conversion.The conversion correctly:
- Propagates
DriverfromKubeClient.Driverto the top-levelDriverfield- Maps
CustomSigner.SignerNameandCustomSigner.Subjectto the flat v1alpha1 structure- Includes proper nil checks with error returns
136-163: LGTM - v1alpha1 to v1beta1 RegistrationConfig conversion.The conversion correctly:
- Routes kube-apiserver-client signer to
KubeClienttype withDriverpropagation- Routes other signers to
CustomSignertype withOrganizationUnitspreservation- Properly initializes nested structs with the correct field mappings
addon/v1beta1/types_managedclusteraddon.go (3)
53-55: LGTM - CustomSigner constant.The constant is appropriately renamed from
CSRtoCustomSignerwith updated documentation reflecting the non-kube endpoint use case.
76-86: LGTM - KubeClientConfig with Driver field.The
Driverfield is properly added with:
- Clear documentation explaining its purpose
- Enum validation for
csrandtokenvalues- Optional marker with
omitempty
88-99: LGTM - CustomSignerConfig type.The type (formerly CSRConfig) properly encapsulates:
signerNamewith appropriate validation (length, pattern)subjectwith the fullSubjectstruct includingOrganizationUnitsoperator/v1/types_klusterlet.go (1)
185-188: LGTM - AddOnKubeClientRegistrationDriver field.The new field is appropriately placed in
RegistrationConfigurationand clearly documented as being specific to add-on registration (separate from klusterlet's ownRegistrationDriver).test/integration/api/managedclusteraddon_test.go (4)
124-163: LGTM!The test properly validates the v1alpha1 API's ability to accept the new
Driverfield with a valid "csr" value alongside the kube-apiserver-client signer configuration.
494-501: LGTM!The test correctly uses the new
CustomSignertype andCustomSignerConfigstruct for v1beta1 registration configuration, properly reflecting the API migration from CSR-based to CustomSigner-based paths.
556-646: LGTM!Both tests correctly validate the v1beta1 API's ability to accept KubeClient registrations with "csr" and "token" driver values. The tests properly set the Driver field within the KubeClientConfig struct as defined in the API types.
716-723: LGTM!The test correctly uses
CustomSignertype andCustomSignerConfigstruct to test the validation of invalid signer names in v1beta1.addon/v1beta1/conversion_test.go (5)
450-468: LGTM!The test correctly validates that v1alpha1 registrations with custom signers (non-kube-apiserver-client) are converted to
CustomSignertype with properCustomSignerConfigin v1beta1, including preservation of OrganizationUnits.
469-568: LGTM!The new test cases comprehensively cover the conversion of v1alpha1 registrations with Driver fields ("csr" and "token") to v1beta1 KubeClient configurations, properly validating that the Driver value is preserved in the nested
KubeClientConfig.Driverfield.
744-783: LGTM!The test correctly validates the reverse conversion from v1beta1
CustomSignertype back to v1alpha1's flat structure withSignerNameandSubject.OrganizationUnitsproperly preserved.
784-883: LGTM!The test cases correctly validate the reverse conversion from v1beta1 KubeClient configurations with Driver fields back to v1alpha1 format, where the Driver field is mapped to the top-level
RegistrationConfig.Driverfield.
941-943: LGTM!The validation correctly verifies that the Driver field is properly converted and preserved when converting from v1beta1 to v1alpha1 format.
elgnay
force-pushed
the
support-addon-registration
branch
from
0ce554f to
dbcc428
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
addon/v1beta1/conversion_test.go (1)
635-645: Stale error messages reference "CSR" instead of "CustomSigner".The test assertions on lines 637, 640, and 643 still reference "CSR" in error messages but are checking CustomSigner fields. The messages should be updated to say "CustomSigner" for clarity during debugging.
🧹 Nitpick comments (1)
addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml (1)
303-315: LGTM! Driver field is well-structured.The new
driverfield is properly defined with appropriate enum values and a clear deprecation path to v1beta1, where it becomeskubeClient.driver. Making it optional ensures backward compatibility with agents that don't yet report this field.Optional documentation enhancement: Consider clarifying in the description what the expected behavior is when
driveris not set (e.g., "If not set, the authentication driver is unknown or assumed to be 'csr' for backward compatibility").
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1alpha1/types_managedclusteraddon.goaddon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1beta1/conversion.goaddon/v1beta1/conversion_test.goaddon/v1beta1/types_managedclusteraddon.goaddon/v1beta1/zz_generated.conversion.goaddon/v1beta1/zz_generated.deepcopy.gooperator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yamloperator/v1/types_klusterlet.gooperator/v1/zz_generated.deepcopy.gotest/integration/api/managedclusteraddon_test.go
✅ Files skipped from review due to trivial changes (1)
- addon/v1beta1/zz_generated.conversion.go
🚧 Files skipped from review as they are similar to previous changes (1)
- operator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: haoqing0110
Repo: open-cluster-management-io/api PR: 407
File: addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml:17-778
Timestamp: 2025-12-01T01:10:01.745Z
Learning: In the open-cluster-management-io/api repository, CRD files for addon resources (like ManagedClusterAddOn and ClusterManagementAddOn) intentionally omit conversion webhook configuration in their base definitions. The registration-operator patches these CRDs at deployment time with the appropriate webhook clientConfig and CA bundle. This is a deliberate architectural pattern to separate schema definitions from deployment-specific webhook configuration.
📚 Learning: 2025-12-01T01:10:01.745Z
Learnt from: haoqing0110
Repo: open-cluster-management-io/api PR: 407
File: addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml:17-778
Timestamp: 2025-12-01T01:10:01.745Z
Learning: In the open-cluster-management-io/api repository, CRD files for addon resources (like ManagedClusterAddOn and ClusterManagementAddOn) intentionally omit conversion webhook configuration in their base definitions. The registration-operator patches these CRDs at deployment time with the appropriate webhook clientConfig and CA bundle. This is a deliberate architectural pattern to separate schema definitions from deployment-specific webhook configuration.
Applied to files:
addon/v1alpha1/types_managedclusteraddon.goaddon/v1beta1/conversion_test.gotest/integration/api/managedclusteraddon_test.goaddon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1beta1/types_managedclusteraddon.gooperator/v1/types_klusterlet.goaddon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1beta1/conversion.go
📚 Learning: 2025-09-02T11:59:50.703Z
Learnt from: qiujian16
Repo: open-cluster-management-io/api PR: 392
File: operator/v1/types_clustermanager.go:178-195
Timestamp: 2025-09-02T11:59:50.703Z
Learning: XValidation markers in kubebuilder require Kubernetes 1.25+ and should not be added to the open-cluster-management-io/api project until minimum Kubernetes version requirements are established to avoid breaking compatibility with older clusters.
Applied to files:
addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml
🧬 Code graph analysis (5)
addon/v1beta1/conversion_test.go (2)
addon/v1beta1/types_managedclusteraddon.go (12)
CustomSigner(55-55)CustomSignerConfig(88-99)ManagedClusterAddOn(19-32)ManagedClusterAddOnSpec(36-43)AddOnConfig(101-107)ManagedClusterAddOnStatus(140-195)RegistrationConfig(59-74)Subject(130-136)KubeClient(51-51)KubeClientConfig(76-86)KubeClientSubject(119-121)BaseSubject(110-117)addon/v1alpha1/types_managedclusteraddon.go (6)
ManagedClusterAddOn(19-32)ManagedClusterAddOnSpec(36-52)AddOnConfig(89-95)ManagedClusterAddOnStatus(113-174)RegistrationConfig(56-87)Subject(98-109)
addon/v1beta1/types_managedclusteraddon.go (1)
addon/v1alpha1/types_managedclusteraddon.go (1)
Subject(98-109)
addon/v1beta1/conversion.go (2)
addon/v1beta1/types_managedclusteraddon.go (4)
KubeClient(51-51)CustomSigner(55-55)Subject(130-136)CustomSignerConfig(88-99)addon/v1alpha1/types_managedclusteraddon.go (1)
Subject(98-109)
operator/v1/zz_generated.deepcopy.go (1)
operator/v1/types_klusterlet.go (2)
AddOnRegistrationDriver(239-251)TokenConfig(253-261)
addon/v1beta1/zz_generated.deepcopy.go (1)
addon/v1beta1/types_managedclusteraddon.go (3)
CustomSignerConfig(88-99)Subject(130-136)CustomSigner(55-55)
⏰ 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). (2)
- GitHub Check: integration
- GitHub Check: verify
🔇 Additional comments (30)
addon/v1alpha1/types_managedclusteraddon.go (1)
77-86: LGTM! Well-structured backward-compatible addition.The new
Driverfield is properly designed with:
- Clear deprecation notice pointing to the v1beta1 replacement
- Appropriate enum validation restricting values to "csr" and "token"
- Good documentation explaining when the field is relevant and who sets it
addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml (2)
303-315: LGTM! CRD schema correctly reflects the v1alpha1 API changes.The
driverfield addition in the v1alpha1 CRD schema properly:
- Mirrors the Go type definition with matching enum values
- Includes appropriate deprecation notice
- Maintains backward compatibility as an optional field
658-723: LGTM! CustomSigner rename and driver addition are correctly implemented.The v1beta1 schema changes properly:
- Replace
csrwithcustomSignerin the type enum (line 723)- Rename the field from
csrtocustomSignerwith updated description (line 658-691)- Add
driverfield underkubeClientproperties with consistent enum validation (lines 697-704)These changes align with the broader API migration from CSR to CustomSigner terminology.
operator/v1/zz_generated.deepcopy.go (3)
36-55: LGTM! Auto-generated deepcopy implementation is correct.The
AddOnRegistrationDriver.DeepCopyIntomethod properly handles the nestedTokenfield with appropriate nil checking and allocation.
1044-1058: LGTM! TokenConfig deepcopy implementation is correct.The simple struct deepcopy follows standard patterns and correctly copies the
ExpirationSecondsfield.
833-833: LGTM! RegistrationConfiguration deepcopy properly includes the new field.The addition of
in.AddOnKubeClientRegistrationDriver.DeepCopyInto(&out.AddOnKubeClientRegistrationDriver)ensures the new nested struct is recursively copied during deep cloning.addon/v1beta1/zz_generated.deepcopy.go (2)
259-274: LGTM! CustomSignerConfig deepcopy correctly replaces CSRConfig.The new deepcopy methods for
CustomSignerConfigproperly handle the nestedSubjectfield by calling itsDeepCopyIntomethod, consistent with the CSR → CustomSigner rename across the API.
628-631: LGTM! RegistrationConfig deepcopy correctly updated for CustomSigner.The change from checking
in.CSR != niltoin.CustomSigner != niland allocatingnew(CustomSignerConfig)properly reflects the API rename.addon/v1beta1/conversion.go (4)
121-121: LGTM! Driver field properly propagated during conversion.The assignment
out.Driver = in.KubeClient.Drivercorrectly carries the authentication driver information from v1beta1's nested KubeClient structure to v1alpha1's top-level Driver field, ensuring round-trip conversion fidelity for KubeClient registrations.
123-129: LGTM! CustomSigner conversion correctly replaces CSR logic.The changes properly:
- Check for
nil CustomSignerinstead ofnil CSR- Map
SignerNamefromin.CustomSigner.SignerName- Convert the
Subjectfield from the CustomSigner structureThis aligns with the API-wide rename from CSR to CustomSigner.
146-146: LGTM! Driver properly carried from v1alpha1 to v1beta1.The assignment
Driver: in.Drivercorrectly propagates the authentication driver from v1alpha1's top-level field to v1beta1's nested KubeClient.Driver field, completing the bidirectional conversion chain.
149-159: LGTM! CustomSigner population correctly implements the CSR rename.The conversion now:
- Sets
Type = CustomSignerinstead of referencing CSR- Populates
out.CustomSignerinstead ofout.CSR- Correctly maps
SignerName,User,Groups, andOrganizationUnitsfrom the v1alpha1 structureThis completes the CSR → CustomSigner migration in the conversion layer.
test/integration/api/managedclusteraddon_test.go (7)
124-163: LGTM! Driver field validation test added for v1alpha1.This test properly validates that the new Driver field can be set to "csr" in v1alpha1 RegistrationConfig and that status updates succeed with valid driver values.
165-204: LGTM! Negative test case for invalid driver value.This test correctly validates that the enum validation rejects invalid driver values in v1alpha1, ensuring the API enforces the csr/token constraint.
496-497: LGTM! Test updated to reflect CustomSigner migration.The test correctly reflects the API change from CSR to CustomSigner in v1beta1.
556-600: LGTM! KubeClient registration with csr driver test added.This test validates the KubeClient registration type with the new Driver field set to "csr" in v1beta1.
602-646: LGTM! KubeClient registration with token driver test added.This test validates the KubeClient registration type with Driver set to "token", providing coverage for the alternative driver option.
648-692: LGTM! Negative test case for invalid driver in v1beta1 KubeClient.This test correctly validates that enum validation rejects invalid driver values in v1beta1 KubeClient registrations.
718-719: LGTM! Test updated to reflect CustomSigner migration.The test correctly reflects the API change from CSR to CustomSigner in v1beta1.
addon/v1beta1/types_managedclusteraddon.go (3)
53-73: LGTM! CustomSigner migration properly implemented.The renaming from CSR to CustomSigner is consistent across:
- The RegistrationType constant (line 55)
- The validation enum (line 61)
- The field name and type in RegistrationConfig (lines 70-73)
The changes maintain backward compatibility through conversion logic in other files.
80-86: LGTM! Driver field properly added to KubeClientConfig.The new Driver field is correctly defined with:
- Appropriate JSON tag with omitempty
- Enum validation restricting values to "csr" and "token"
- Clear documentation indicating it's set by the agent
This enables the agent to declare which authentication driver it's using for KubeClient registrations.
88-99: LGTM! CustomSignerConfig type properly renamed.The type rename from CSRConfig to CustomSignerConfig is consistent with the overall CustomSigner migration, while preserving all existing validations and field structure.
operator/v1/types_klusterlet.go (2)
181-187: LGTM! Add-on registration driver configuration added to RegistrationConfiguration.The new AddOnKubeClientRegistrationDriver field properly extends RegistrationConfiguration to support configuring the authentication driver for add-on kubeClient registrations. This complements the existing RegistrationDriver field which configures the klusterlet agent's own registration.
The field is appropriately optional with omitempty, allowing it to be unset when not needed.
239-261: LGTM! AddOnRegistrationDriver and TokenConfig types properly defined.The new types correctly support token-based add-on registration:
- AddOnRegistrationDriver: Provides AuthType selection (csr/token) with optional Token configuration
- TokenConfig: Holds ExpirationSeconds with clear documentation about defaults and minimum values
The comments appropriately explain:
- That this currently affects only kubeClient type add-ons
- Default behavior when ExpirationSeconds is 0
- Production minimum of 3600 seconds (1 hour)
This design allows future extension to customize authentication for other registration types.
addon/v1beta1/conversion_test.go (6)
452-453: LGTM! Test expectations updated for CustomSigner migration.The expected output correctly reflects the v1beta1 API change from CSR to CustomSigner.
469-568: LGTM! Conversion tests added for Driver field (v1alpha1→v1beta1).These two test cases properly validate that the Driver field is preserved when converting from v1alpha1 to v1beta1:
- Test with Driver: "csr" (lines 469-517)
- Test with Driver: "token" (lines 519-567)
The tests ensure the new Driver field correctly round-trips through the conversion logic.
626-633: LGTM! Assertion added for KubeClient.Driver field.The test now properly validates that the Driver field is preserved in KubeClient registrations after conversion from v1alpha1 to v1beta1.
746-747: LGTM! Test expectations updated for CustomSigner migration.The expected output correctly reflects the v1beta1 API change from CSR to CustomSigner.
784-883: LGTM! Conversion tests added for Driver field (v1beta1→v1alpha1).These test cases properly validate that the Driver field is preserved when converting from v1beta1 back to v1alpha1:
- Test with Driver: "csr" (lines 784-832)
- Test with Driver: "token" (lines 834-882)
The bidirectional conversion coverage ensures the Driver field correctly round-trips in both directions.
941-943: LGTM! Assertion added for Driver field in reverse conversion.The test now properly validates that the Driver field is preserved when converting from v1beta1 back to v1alpha1, ensuring bidirectional conversion correctness.
| // +optional | ||
| Subject Subject `json:"subject,omitempty"` | ||
|
|
||
| // Deprecated: Will be replaced with kubeClient.driver in v1beta1. |
There was a problem hiding this comment.
It does not need to mark as deprecated I think?
| Subject Subject `json:"subject,omitempty"` | ||
|
|
||
| // Deprecated: Will be replaced with kubeClient.driver in v1beta1. | ||
| // Driver specifies the authentication driver used by the ManagedClusterAddOn |
elgnay
force-pushed
the
support-addon-registration
branch
2 times, most recently
from
018184b to
43f66d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @addon/v1beta1/conversion_test.go:
- Around line 626-646: The test's nil-check error message is stale: when
validating a CustomSigner entry in Registrations (check using
got.Status.Registrations[i].Type == CustomSigner and CustomSigner field), change
the t.Errorf call that currently says "Registrations[%d].CSR is nil" to
"Registrations[%d].CustomSigner is nil" so the message accurately references the
CustomSigner field being nil; update the error string in the test where
CustomSigner is verified.
🧹 Nitpick comments (1)
operator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml (1)
206-214: Consider adding OpenAPI validation for minimum expirationSeconds.The description states "The minimum valid value for production use is 3600 (1 hour)", but there's no OpenAPI
minimumconstraint enforcing this. Without validation, users could accidentally configure very small values in production environments, leading to frequent token rotation and potential stability issues.🔎 Proposed fix to add minimum validation
expirationSeconds: description: |- ExpirationSeconds represents the seconds of a token to expire. If it is not set or 0, the default duration will be used, which is the same as the certificate expiration set by the hub cluster's kube-controller-manager (typically 1 year). The minimum valid value for production use is 3600 (1 hour), though smaller values are allowed for testing. format: int64 + minimum: 1 type: integerNote: This sets
minimum: 1rather thanminimum: 3600to allow testing with smaller values as mentioned in the description, while preventing negative or zero values that could cause runtime errors.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1alpha1/types_managedclusteraddon.goaddon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1beta1/conversion.goaddon/v1beta1/conversion_test.goaddon/v1beta1/types_managedclusteraddon.goaddon/v1beta1/zz_generated.conversion.goaddon/v1beta1/zz_generated.deepcopy.gooperator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yamloperator/v1/types_klusterlet.gooperator/v1/zz_generated.deepcopy.gotest/integration/api/managedclusteraddon_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- operator/v1/types_klusterlet.go
- addon/v1beta1/types_managedclusteraddon.go
- addon/v1alpha1/types_managedclusteraddon.go
- operator/v1/zz_generated.deepcopy.go
- addon/v1beta1/conversion.go
- addon/v1alpha1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: haoqing0110
Repo: open-cluster-management-io/api PR: 407
File: addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml:17-778
Timestamp: 2025-12-01T01:10:01.745Z
Learning: In the open-cluster-management-io/api repository, CRD files for addon resources (like ManagedClusterAddOn and ClusterManagementAddOn) intentionally omit conversion webhook configuration in their base definitions. The registration-operator patches these CRDs at deployment time with the appropriate webhook clientConfig and CA bundle. This is a deliberate architectural pattern to separate schema definitions from deployment-specific webhook configuration.
📚 Learning: 2025-12-01T01:10:01.745Z
Learnt from: haoqing0110
Repo: open-cluster-management-io/api PR: 407
File: addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml:17-778
Timestamp: 2025-12-01T01:10:01.745Z
Learning: In the open-cluster-management-io/api repository, CRD files for addon resources (like ManagedClusterAddOn and ClusterManagementAddOn) intentionally omit conversion webhook configuration in their base definitions. The registration-operator patches these CRDs at deployment time with the appropriate webhook clientConfig and CA bundle. This is a deliberate architectural pattern to separate schema definitions from deployment-specific webhook configuration.
Applied to files:
addon/v1beta1/zz_generated.conversion.goaddon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yamladdon/v1beta1/conversion_test.gooperator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yamltest/integration/api/managedclusteraddon_test.go
🧬 Code graph analysis (2)
addon/v1beta1/zz_generated.conversion.go (2)
addon/v1alpha1/types_managedclusteraddon.go (1)
RegistrationConfig(56-86)addon/v1beta1/types_managedclusteraddon.go (1)
RegistrationConfig(59-74)
addon/v1beta1/conversion_test.go (2)
addon/v1beta1/types_managedclusteraddon.go (9)
CustomSigner(55-55)CustomSignerConfig(88-99)ManagedClusterAddOn(19-32)RegistrationConfig(59-74)Subject(130-136)KubeClient(51-51)KubeClientConfig(76-86)KubeClientSubject(119-121)BaseSubject(110-117)addon/v1alpha1/types_managedclusteraddon.go (3)
ManagedClusterAddOn(19-32)RegistrationConfig(56-86)Subject(97-108)
⏰ 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). (1)
- GitHub Check: verify
🔇 Additional comments (19)
operator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml (2)
192-201: Verify whether authType should have a default value.The
authTypefield has no default value specified, requiring explicit user configuration. For comparison, theregistrationDriver.authTypefield at line 359 defaults tocsr.Please confirm whether this is intentional or if a default should be added for better usability (e.g.,
default: csr).
354-356: LGTM! Improved description clarity.The updated description better specifies that it's the "klusterlet agent" that registers with the hub, improving clarity.
addon/v1beta1/zz_generated.conversion.go (1)
905-917: Autogenerated conversion warnings align with manual conversion requirements.The warnings correctly identify that
Type,KubeClient, andCustomSignerin v1beta1 andSignerName,Subject, andDriverin v1alpha1 require manual conversion since they don't have direct peer-type equivalents. The actual field mappings are implemented inconversion.go.addon/v1beta1/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml (3)
303-314: v1alpha1 driver field addition looks correct.The new
driverfield in v1alpha1 registrations with enum[csr, token]correctly supports the authentication driver selection. The description clearly states it's only relevant forkubernetes.io/kube-apiserver-clientsigner and is agent-declared.
657-690: CustomSigner field correctly replaces CSR in v1beta1.The schema update properly renames
csrtocustomSignerand the nested structure withsignerName(required) andsubject(optional) is consistent with theCustomSignerConfigtype definition.
691-723: v1beta1 kubeClient.driver field and type enum update look correct.The
driverfield underkubeClientwith enum[csr, token]and the updatedtypeenum replacingcsrwithcustomSignerare consistent with the API type changes.addon/v1beta1/zz_generated.deepcopy.go (2)
259-274: CustomSignerConfig deepcopy implementation is correct.The generated deepcopy correctly uses
in.Subject.DeepCopyInto(&out.Subject)sinceSubjectcontains slice fields (Groups,OrganizationUnits) that require deep copying.
620-634: RegistrationConfig deepcopy correctly handles both KubeClient and CustomSigner.The nil-check pattern for optional pointer fields (
KubeClient,CustomSigner) follows the standard generated deepcopy approach.test/integration/api/managedclusteraddon_test.go (6)
124-163: Good test coverage for v1alpha1 driver field.The test correctly validates that the
csrdriver value is accepted when used with thekubernetes.io/kube-apiserver-clientsigner name, which aligns with the CRD schema.
165-204: Negative test for invalid driver validation is valuable.Testing that an invalid driver value is rejected helps ensure the CRD enum validation is working correctly.
494-501: v1beta1 test correctly updated to use CustomSigner.The migration from CSR to CustomSigner type is properly reflected in this test case.
556-646: Comprehensive v1beta1 KubeClient driver tests.Good coverage of both
csrandtokendriver values under theKubeClientregistration type.
648-692: v1beta1 invalid driver test complements the positive cases.Validates that the CRD rejects invalid driver values for the v1beta1 API as well.
716-723: Signer name validation test correctly updated to use CustomSigner.The test maintains its purpose (validating signer name format) while using the new CustomSigner type.
addon/v1beta1/conversion_test.go (5)
452-468: CSR registration test correctly updated to use CustomSigner.The test data and expectations properly reflect the CSR→CustomSigner rename, with
Type: CustomSignerandCustomSigner: &CustomSignerConfig{...}structure.
469-568: New KubeClient driver conversion tests provide good coverage.Tests for both
csrandtokendrivers ensure the Driver field is correctly propagated during v1alpha1→v1beta1 conversion.
746-757: v1beta1→v1alpha1 CustomSigner conversion test data is correct.The test properly validates that CustomSigner fields map back to the flat v1alpha1 structure with SignerName and Subject.
784-883: v1beta1→v1alpha1 KubeClient driver conversion tests are comprehensive.Both
csrandtokendriver values are tested to ensure proper round-trip conversion, verifying the Driver field appears in the v1alpha1 output.
941-943: Driver field verification correctly added to v1beta1→v1alpha1 test validation.This ensures the Driver field is preserved during conversion back to v1alpha1.
elgnay
force-pushed
the
support-addon-registration
branch
2 times, most recently
from
f3a8efe to
d22cb1f
Compare
Signed-off-by: Yang Le <[email protected]>
elgnay
force-pushed
the
support-addon-registration
branch
from
d22cb1f to
0a1b069
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elgnay, qiujian16 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
|
/unhold |
openshift-merge-bot
bot
merged commit 8307845
into
open-cluster-management-io:main
3 participants
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.