✨ support enable grpc registration when clusteradm init #516
Conversation
WalkthroughAdds gRPC as a first-class registration driver: dependency bump, new init CLI flags and Options fields for gRPC, extended registration-driver validation and assembly, moves gRPC test setup to init-time, and minor e2e helper message tweaks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
pkg/cmd/init/exec.go (1)
181-181: Improve error message formatting.The error message lacks proper punctuation between the driver names for better readability.
Apply this diff to improve the formatting:
- return fmt.Errorf("only csr,awsirsa and grpc are valid drivers") + return fmt.Errorf("only csr, awsirsa, and grpc are valid drivers")
📜 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/ocm/deploy/cluster-manager/chart/cluster-manager/templates/cluster_manager.yamlis excluded by!vendor/**vendor/open-cluster-management.io/ocm/deploy/cluster-manager/chart/cluster-manager/values.yamlis excluded by!vendor/**vendor/open-cluster-management.io/ocm/deploy/klusterlet/chart/klusterlet/values.yamlis excluded by!vendor/**vendor/open-cluster-management.io/ocm/pkg/operator/helpers/chart/config.gois excluded by!vendor/**
📒 Files selected for processing (5)
go.mod(1 hunks)pkg/cmd/init/cmd.go(3 hunks)pkg/cmd/init/exec.go(4 hunks)pkg/cmd/init/options.go(1 hunks)test/e2e/clusteradm/joinhubscenario_grpc_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/cmd/init/exec.go (1)
vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (12)
CSRAuthType(129-129)AwsIrsaAuthType(127-127)GRPCAuthType(131-131)ServerConfiguration(166-176)EndpointExposure(178-195)Endpoint(197-209)EndpointTypeHostname(227-227)HostnameConfig(212-220)RegistrationDriverHub(144-164)CSRConfig(230-234)AwsIrsaConfig(236-251)GRPCRegistrationConfig(254-258)
⏰ 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: unit-test
- GitHub Check: e2e-test
- GitHub Check: integration-test
- GitHub Check: verify
🔇 Additional comments (9)
pkg/cmd/init/options.go (1)
71-75: LGTM!The new fields for gRPC configuration are well-documented and properly integrated into the Options struct.
pkg/cmd/init/cmd.go (3)
19-19: LGTM!The example usage clearly demonstrates the new gRPC registration driver option alongside other drivers.
92-92: LGTM!The help text accurately documents gRPC as a valid registration driver option.
103-105: LGTM!The new flags for gRPC configuration are properly defined with clear descriptions.
test/e2e/clusteradm/joinhubscenario_grpc_test.go (1)
41-44: LGTM!The test now properly configures gRPC registration at initialization time, which is cleaner than the previous runtime reconfiguration approach.
pkg/cmd/init/exec.go (4)
43-43: LGTM!The global
validRegistrationDriverset properly defines all supported registration drivers including the new gRPC option.
100-118: LGTM!The gRPC server configuration correctly validates that
grpcServeris provided when gRPC registration is enabled, and properly configures the endpoint exposure.
188-195: LGTM!The validation correctly ensures that auto-approved identities are only provided when the corresponding registration driver is enabled.
424-449: LGTM!The refactored
getRegistrationDriversfunction using a switch statement is cleaner and properly handles each registration driver type (CSR, AWS IRSA, and gRPC) with their respective configurations. The default case appropriately catches unknown driver types.
zhiweiyin318
force-pushed
the
grpc-init
branch
from
28a79c3 to
9b53cf5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/cmd/init/exec.go (1)
197-199: Typo in error message (already flagged).This was already noted in previous reviews.
📜 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/ocm/deploy/cluster-manager/chart/cluster-manager/templates/cluster_manager.yamlis excluded by!vendor/**vendor/open-cluster-management.io/ocm/deploy/cluster-manager/chart/cluster-manager/values.yamlis excluded by!vendor/**vendor/open-cluster-management.io/ocm/deploy/klusterlet/chart/klusterlet/values.yamlis excluded by!vendor/**vendor/open-cluster-management.io/ocm/pkg/operator/helpers/chart/config.gois excluded by!vendor/**
📒 Files selected for processing (7)
go.mod(1 hunks)pkg/cmd/init/cmd.go(3 hunks)pkg/cmd/init/exec.go(4 hunks)pkg/cmd/init/options.go(1 hunks)test/e2e/clusteradm/joinhubscenario_grpc_test.go(1 hunks)test/e2e/util/helper.go(1 hunks)test/e2e/util/util.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/e2e/util/util.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/cmd/init/options.go
- pkg/cmd/init/cmd.go
🧰 Additional context used
🧬 Code graph analysis (2)
test/e2e/clusteradm/joinhubscenario_grpc_test.go (3)
test/e2e/util/helper.go (1)
WaitClusterManagerApplied(170-210)vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (2)
ClusterManager(18-29)ServerConfiguration(166-176)vendor/open-cluster-management.io/api/operator/v1/types_klusterlet.go (1)
RegistrationConfiguration(133-189)
pkg/cmd/init/exec.go (1)
vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (11)
CSRAuthType(129-129)AwsIrsaAuthType(127-127)GRPCAuthType(131-131)ServerConfiguration(166-176)EndpointExposure(178-195)Endpoint(197-209)HostnameConfig(212-220)RegistrationDriverHub(144-164)CSRConfig(230-234)AwsIrsaConfig(236-251)GRPCRegistrationConfig(254-258)
🔇 Additional comments (8)
test/e2e/util/helper.go (1)
123-125: LGTM! Improved error message handling.The separation of logging (for debugging) and error message (for control flow) improves code clarity.
pkg/cmd/init/exec.go (4)
43-43: LGTM! Good use of constants and sets for validation.Centralizing valid registration driver types improves maintainability.
100-118: LGTM! Proper GRPC server configuration and validation.The logic correctly validates the GRPC server requirement and configures the endpoint exposure when GRPC is selected as a registration driver.
179-182: LGTM! Validation uses the centralized driver set.The validation logic correctly uses the
validRegistrationDriverset defined at the package level.
420-455: LGTM! Well-structured refactor using switch statement.The refactored function clearly handles each registration driver type with appropriate configuration. The addition of a default case properly catches unknown driver types.
test/e2e/clusteradm/joinhubscenario_grpc_test.go (3)
41-47: LGTM! Init command properly configures GRPC at initialization.The parameters correctly enable GRPC registration with appropriate server address and auto-approved identities.
51-55: LGTM! Appropriate Eventually pattern for async operation.The 60-second timeout with 2-second polling interval is reasonable for waiting on the ClusterManager resource.
63-90: LGTM! Join command and verification properly use GRPC authentication.The test correctly verifies that the klusterlet is configured with GRPC authentication type after joining.
Signed-off-by: Zhiwei Yin <[email protected]>
zhiweiyin318
force-pushed
the
grpc-init
branch
from
9b53cf5 to
40713b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/e2e/clusteradm/joinhubscenario_grpc_test.go (1)
50-61: LGTM: Assertions correctly verify GRPC configuration.The test properly validates the cluster manager state:
- Eventually block allows time for configuration to be applied
- Correctly asserts 2 registration drivers (csr + grpc)
- Protocol assertion directly compares the string value (past len() issue has been resolved)
For extra defensive programming, consider adding a bounds check before accessing
EndpointsExposure[0]:+ gomega.Expect(len(clusterManager.Spec.ServerConfiguration.EndpointsExposure)).To( + gomega.BeNumerically(">", 0), "should have at least one endpoint exposure") gomega.Expect(clusterManager.Spec.ServerConfiguration.EndpointsExposure[0].Protocol).To( gomega.Equal("grpc"), "server config endpoint exposure protocol should be grpc")However, this is optional since the init command guarantees the slice is populated when GRPC is configured.
📜 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/ocm/deploy/cluster-manager/chart/cluster-manager/templates/cluster_manager.yamlis excluded by!vendor/**vendor/open-cluster-management.io/ocm/deploy/cluster-manager/chart/cluster-manager/values.yamlis excluded by!vendor/**vendor/open-cluster-management.io/ocm/deploy/klusterlet/chart/klusterlet/values.yamlis excluded by!vendor/**vendor/open-cluster-management.io/ocm/pkg/operator/helpers/chart/config.gois excluded by!vendor/**
📒 Files selected for processing (7)
go.mod(1 hunks)pkg/cmd/init/cmd.go(3 hunks)pkg/cmd/init/exec.go(4 hunks)pkg/cmd/init/options.go(1 hunks)test/e2e/clusteradm/joinhubscenario_grpc_test.go(1 hunks)test/e2e/util/helper.go(1 hunks)test/e2e/util/util.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (3)
- test/e2e/util/helper.go
- test/e2e/util/util.go
- pkg/cmd/init/options.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: zhiweiyin318
Repo: open-cluster-management-io/clusteradm PR: 516
File: go.mod:34-34
Timestamp: 2025-11-05T15:25:15.177Z
Learning: In the clusteradm repository, the dependency upgrade of open-cluster-management.io/ocm to v1.1.1-0.20251105064423-d80ec55608e7 is intentional and required to support the gRPC registration feature being added.
📚 Learning: 2025-11-05T15:25:15.177Z
Learnt from: zhiweiyin318
Repo: open-cluster-management-io/clusteradm PR: 516
File: go.mod:34-34
Timestamp: 2025-11-05T15:25:15.177Z
Learning: In the clusteradm repository, the dependency upgrade of open-cluster-management.io/ocm to v1.1.1-0.20251105064423-d80ec55608e7 is intentional and required to support the gRPC registration feature being added.
Applied to files:
test/e2e/clusteradm/joinhubscenario_grpc_test.gopkg/cmd/init/exec.go
🧬 Code graph analysis (2)
test/e2e/clusteradm/joinhubscenario_grpc_test.go (3)
test/e2e/util/helper.go (1)
WaitClusterManagerApplied(170-210)vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (2)
ClusterManager(18-29)ServerConfiguration(166-176)vendor/open-cluster-management.io/api/operator/v1/types_klusterlet.go (1)
RegistrationConfiguration(133-189)
pkg/cmd/init/exec.go (1)
vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (12)
CSRAuthType(129-129)AwsIrsaAuthType(127-127)GRPCAuthType(131-131)ServerConfiguration(166-176)EndpointExposure(178-195)Endpoint(197-209)EndpointTypeHostname(227-227)HostnameConfig(212-220)RegistrationDriverHub(144-164)CSRConfig(230-234)AwsIrsaConfig(236-251)GRPCRegistrationConfig(254-258)
⏰ 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). (3)
- GitHub Check: e2e-test
- GitHub Check: verify
- GitHub Check: unit-test
🔇 Additional comments (8)
pkg/cmd/init/exec.go (4)
43-43: LGTM: Centralized validation set.Good use of a global set to centralize valid registration driver types, making validation consistent across the codebase.
100-118: LGTM: Proper GRPC server configuration.The GRPC server endpoint configuration is correctly structured:
- Validates grpcServer is non-empty when GRPC driver is enabled
- Properly configures EndpointsExposure with hostname-based GRPC endpoint
- Uses appropriate operatorv1 constants throughout
179-203: LGTM: Consistent validation patterns.The validation logic properly ensures:
- Only valid registration drivers are accepted
- Auto-approval identities are only allowed when the corresponding driver is enabled
- All three driver types (CSR, AWS IRSA, GRPC) follow consistent validation patterns
420-455: LGTM: Well-structured registration driver assembly.The refactored switch-based approach cleanly handles all three registration driver types:
- Each case properly constructs the driver config with appropriate identity lists
- Default case provides clear error handling for unknown types
- Logic is maintainable and consistent
pkg/cmd/init/cmd.go (2)
19-24: LGTM: Example usage updated.The example correctly demonstrates GRPC as a registration driver option alongside CSR and AWS IRSA.
91-105: LGTM: CLI flags properly exposed.The new GRPC-related flags are well-integrated:
- Help text correctly documents grpc as a valid registration driver
- Flag naming is consistent with existing patterns (e.g., auto-approved-csr-identities)
- Flags are properly bound to Options fields
test/e2e/clusteradm/joinhubscenario_grpc_test.go (2)
38-48: LGTM: Proper init-time GRPC configuration.The test correctly initializes the hub with GRPC support:
- Specifies both csr and grpc registration drivers
- Configures the GRPC server endpoint
- Enables ManagedClusterAutoApproval and sets auto-approved identities
- Properly synchronizes with WaitClusterManagerApplied
63-106: LGTM: Join and verification flow is correct.The test properly:
- Joins the hub with GRPC registration auth type
- Verifies the klusterlet has GRPC auth type configured
- Waits for the managed cluster to become available with appropriate timeouts
|
/assign @qiujian16 |
|
LGTM |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, zhiweiyin318 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 |
|
/lgtm |
openshift-merge-bot
bot
merged commit 38ca957
into
open-cluster-management-io:main
|
/cherry-pick release-1.1 |
|
@zhiweiyin318: #516 failed to apply on top of branch "release-1.1": In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cherry-pick release-1.1 |
|
@zhiweiyin318: new pull request created: #519 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
4 participants
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
New Features
Tests
Chores
Style