🌱 Build a single hub manager command #1078
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16 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 |
WalkthroughThis change introduces new subcommands to the registration-operator CLI for running the hub manager and webhook server. It adds new modular options and setup logic for hub manager and webhook components, centralizes controller and webhook initialization, and provides integration tests for the hub manager. Minor import cleanups are also included. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.2.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/product/migration-guide for migration instructions Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ 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). (7)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1078 +/- ##
==========================================
- Coverage 57.82% 57.71% -0.11%
==========================================
Files 211 213 +2
Lines 20592 20728 +136
==========================================
+ Hits 11907 11964 +57
- Misses 7627 7696 +69
- Partials 1058 1068 +10
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:
|
![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 (2)
pkg/work/hub/manager.go (1)
120-139: Start the cloudevents informer factory consistentlyFor the cloudevents driver case, you create
ceInformersbut only start the individual informer. Consider starting the entire factory for consistency.ceInformers := workinformers.NewSharedInformerFactoryWithOptions(workClient, 30*time.Minute) manifestWorkInformer = ceInformers.Work().V1().ManifestWorks() watcherStore.SetInformer(manifestWorkInformer.Informer()) + go ceInformers.Start(ctx.Done()) } // ... controller creation ... go clusterInformers.Start(ctx.Done()) go replicaSetInformers.Start(ctx.Done()) go manifestWorkReplicaSetController.Run(ctx, 5) - go manifestWorkInformer.Informer().Run(ctx.Done())pkg/singleton/hub/manager.go (1)
69-72: Document the rationale for increased QPS/BurstThe metadata client configuration doubles the QPS and Burst values. Consider adding a comment explaining why the GC controller needs higher throughput.
// copy a separate config for gc controller and increase the gc controller's throughput. + // The GC controller requires higher throughput to efficiently handle resource cleanup + // across potentially large numbers of managed clusters. metadataKubeConfig := rest.CopyConfig(controllerContext.KubeConfig) metadataKubeConfig.QPS = controllerContext.KubeConfig.QPS * 2 metadataKubeConfig.Burst = controllerContext.KubeConfig.Burst * 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/registration-operator/main.go(1 hunks)pkg/cmd/hub/operator.go(2 hunks)pkg/singleton/hub/manager.go(1 hunks)pkg/work/hub/manager.go(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: zhujian7
PR: open-cluster-management-io/ocm#1070
File: SECURITY-INSIGHTS.yml:44-44
Timestamp: 2025-07-14T09:30:25.378Z
Learning: In the open-cluster-management-io/ocm repository, the team prefers to use commit SHAs instead of tags for GitHub Actions dependencies like dependency-review-action for security reasons, as commit SHAs are immutable while tags can be moved.
pkg/cmd/hub/operator.go (1)
Learnt from: skeeey
PR: #1058
File: pkg/server/services/work/work.go:39-49
Timestamp: 2025-07-02T05:42:41.749Z
Learning: In the OCM (Open Cluster Management) codebase, nil checks with panic statements in constructor functions for interface parameters are considered unnecessary, as the dependency injection/wiring is managed properly and such checks are not part of the established codebase patterns.
pkg/work/hub/manager.go (1)
Learnt from: skeeey
PR: #1071
File: pkg/server/grpc/clients.go:73-76
Timestamp: 2025-07-15T06:10:13.001Z
Learning: In OCM (Open Cluster Management) gRPC server informer setup, cache sync verification is not necessary when starting informers in the clients.Run() method. The current pattern of starting informers as goroutines without explicit cache sync waiting is the preferred approach for this codebase.
🧬 Code Graph Analysis (3)
cmd/registration-operator/main.go (1)
pkg/cmd/hub/operator.go (1)
NewHubManagerCmd(38-49)
pkg/work/hub/manager.go (1)
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controller.go (1)
NewManifestWorkReplicaSetController(76-121)
pkg/singleton/hub/manager.go (5)
pkg/registration/hub/manager.go (2)
HubManagerOptions(53-65)NewHubManagerOptions(68-75)pkg/work/hub/options.go (2)
WorkHubManagerOptions(8-13)NewWorkHubManagerOptions(15-19)pkg/addon/manager.go (1)
RunManager(33-88)pkg/features/feature.go (1)
HubMutableFeatureGate(11-11)pkg/work/hub/manager.go (1)
NewWorkHubManagerConfig(33-37)
⏰ 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). (7)
- GitHub Check: unit
- GitHub Check: integration
- GitHub Check: build
- GitHub Check: verify
- GitHub Check: e2e-hosted
- GitHub Check: e2e
- GitHub Check: e2e-singleton
🔇 Additional comments (3)
cmd/registration-operator/main.go (1)
50-50: LGTM!The new hub manager subcommand is properly integrated following the existing pattern.
pkg/cmd/hub/operator.go (1)
38-49: LGTM!The new hub manager command follows the established pattern and is properly structured.
pkg/work/hub/manager.go (1)
73-81: Ignore the duplicate-parameter warningThe call to
RunControllerManagerWithInformersinpkg/work/hub/manager.gocorrectly passes
replicaSetInformers- then
workInformersin line with the method’s signature. There is no duplicate
workInformersargument—please disregard this suggestion.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/singleton/hub/manager_test.go (1)
29-37: Consider making CRD paths more maintainable.The hardcoded relative paths for CRDs could be brittle if the project structure changes. Consider using build constraints or environment variables to make these paths more maintainable.
var CRDPaths = []string{ // hub - "../../../vendor/open-cluster-management.io/api/work/v1/0000_00_work.open-cluster-management.io_manifestworks.crd.yaml", - "../../../vendor/open-cluster-management.io/api/work/v1alpha1/0000_00_work.open-cluster-management.io_manifestworkreplicasets.crd.yaml", - filepath.Join("../../../", "vendor", "open-cluster-management.io", "api", "cluster", "v1"), - filepath.Join("../../../", "vendor", "open-cluster-management.io", "api", "cluster", "v1beta1"), - filepath.Join("../../../", "vendor", "open-cluster-management.io", "api", "cluster", "v1beta2"), - filepath.Join("../../../", "vendor", "open-cluster-management.io", "api", "addon", "v1alpha1"), + filepath.Join("..", "..", "..", "vendor", "open-cluster-management.io", "api", "work", "v1", "0000_00_work.open-cluster-management.io_manifestworks.crd.yaml"), + filepath.Join("..", "..", "..", "vendor", "open-cluster-management.io", "api", "work", "v1alpha1", "0000_00_work.open-cluster-management.io_manifestworkreplicasets.crd.yaml"), + filepath.Join("..", "..", "..", "vendor", "open-cluster-management.io", "api", "cluster", "v1"), + filepath.Join("..", "..", "..", "vendor", "open-cluster-management.io", "api", "cluster", "v1beta1"), + filepath.Join("..", "..", "..", "vendor", "open-cluster-management.io", "api", "cluster", "v1beta2"), + filepath.Join("..", "..", "..", "vendor", "open-cluster-management.io", "api", "addon", "v1alpha1"), }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/registration-operator/main.go(1 hunks)pkg/cmd/hub/operator.go(2 hunks)pkg/singleton/hub/manager.go(1 hunks)pkg/singleton/hub/manager_test.go(1 hunks)pkg/work/hub/manager.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/cmd/hub/operator.go
- cmd/registration-operator/main.go
- pkg/singleton/hub/manager.go
- pkg/work/hub/manager.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: zhujian7
PR: open-cluster-management-io/ocm#1070
File: SECURITY-INSIGHTS.yml:44-44
Timestamp: 2025-07-14T09:30:25.378Z
Learning: In the open-cluster-management-io/ocm repository, the team prefers to use commit SHAs instead of tags for GitHub Actions dependencies like dependency-review-action for security reasons, as commit SHAs are immutable while tags can be moved.
🪛 golangci-lint (1.64.8)
pkg/singleton/hub/manager_test.go
5-5: File is not properly formatted
(gci)
10-10: File is not properly formatted
(goimports)
⏰ 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). (7)
- GitHub Check: e2e
- GitHub Check: e2e-singleton
- GitHub Check: e2e-hosted
- GitHub Check: integration
- GitHub Check: build
- GitHub Check: unit
- GitHub Check: verify
🔇 Additional comments (1)
pkg/singleton/hub/manager_test.go (1)
61-82: Comprehensive feature gate setup looks good.The feature gate configuration is well-organized and covers all necessary functionality for hub manager testing. Error handling is appropriate for each gate setup.
| var _ = ginkgo.Describe("start hub manager", func() { | ||
| ginkgo.It("start hub manager", func() { | ||
| ctx, stopHub := context.WithCancel(context.Background()) | ||
| opts := NewHubOption() | ||
| opts.workOption.WorkDriver = "kube" | ||
| opts.workOption.WorkDriverConfig = sourceConfigFileName | ||
| opts.registrationOption.ClusterAutoApprovalUsers = []string{util.AutoApprovalBootstrapUser} | ||
|
|
||
| // start hub controller | ||
| go func() { | ||
| err := opts.RunManager(ctx, &controllercmd.ControllerContext{ | ||
| KubeConfig: cfg, | ||
| EventRecorder: util.NewIntegrationTestEventRecorder("hub"), | ||
| }) | ||
| gomega.Expect(err).NotTo(gomega.HaveOccurred()) | ||
| }() | ||
|
|
||
| time.Sleep(5 * time.Second) | ||
| stopHub() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test reliability and verification.
The test has several areas for improvement:
- Arbitrary sleep duration: The 5-second sleep is arbitrary and could make tests flaky in CI environments.
- Lack of startup verification: The test doesn't verify that the manager actually started successfully before stopping it.
- Minimal verification: Only checks that no error occurred during startup/shutdown.
Consider this improved approach:
var _ = ginkgo.Describe("start hub manager", func() {
- ginkgo.It("start hub manager", func() {
+ ginkgo.It("should start and stop hub manager successfully", func() {
ctx, stopHub := context.WithCancel(context.Background())
+ defer stopHub()
+
opts := NewHubOption()
opts.workOption.WorkDriver = "kube"
opts.workOption.WorkDriverConfig = sourceConfigFileName
opts.registrationOption.ClusterAutoApprovalUsers = []string{util.AutoApprovalBootstrapUser}
+ // Channel to capture startup errors
+ errChan := make(chan error, 1)
+
// start hub controller
go func() {
err := opts.RunManager(ctx, &controllercmd.ControllerContext{
KubeConfig: cfg,
EventRecorder: util.NewIntegrationTestEventRecorder("hub"),
})
- gomega.Expect(err).NotTo(gomega.HaveOccurred())
+ errChan <- err
}()
- time.Sleep(5 * time.Second)
- stopHub()
+ // Wait for either startup completion or timeout
+ select {
+ case err := <-errChan:
+ if err != nil {
+ ginkgo.Fail("Hub manager failed to start: " + err.Error())
+ }
+ case <-time.After(10 * time.Second):
+ // Manager started successfully, now stop it
+ stopHub()
+ // Wait for graceful shutdown
+ select {
+ case err := <-errChan:
+ gomega.Expect(err).To(gomega.Or(gomega.BeNil(), gomega.MatchError("context canceled")))
+ case <-time.After(5 * time.Second):
+ ginkgo.Fail("Hub manager did not shutdown gracefully")
+ }
+ }
})
})📝 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.
| var _ = ginkgo.Describe("start hub manager", func() { | |
| ginkgo.It("start hub manager", func() { | |
| ctx, stopHub := context.WithCancel(context.Background()) | |
| opts := NewHubOption() | |
| opts.workOption.WorkDriver = "kube" | |
| opts.workOption.WorkDriverConfig = sourceConfigFileName | |
| opts.registrationOption.ClusterAutoApprovalUsers = []string{util.AutoApprovalBootstrapUser} | |
| // start hub controller | |
| go func() { | |
| err := opts.RunManager(ctx, &controllercmd.ControllerContext{ | |
| KubeConfig: cfg, | |
| EventRecorder: util.NewIntegrationTestEventRecorder("hub"), | |
| }) | |
| gomega.Expect(err).NotTo(gomega.HaveOccurred()) | |
| }() | |
| time.Sleep(5 * time.Second) | |
| stopHub() | |
| }) | |
| }) | |
| var _ = ginkgo.Describe("start hub manager", func() { | |
| ginkgo.It("should start and stop hub manager successfully", func() { | |
| ctx, stopHub := context.WithCancel(context.Background()) | |
| defer stopHub() | |
| opts := NewHubOption() | |
| opts.workOption.WorkDriver = "kube" | |
| opts.workOption.WorkDriverConfig = sourceConfigFileName | |
| opts.registrationOption.ClusterAutoApprovalUsers = []string{util.AutoApprovalBootstrapUser} | |
| // Channel to capture startup errors | |
| errChan := make(chan error, 1) | |
| // start hub controller | |
| go func() { | |
| err := opts.RunManager(ctx, &controllercmd.ControllerContext{ | |
| KubeConfig: cfg, | |
| EventRecorder: util.NewIntegrationTestEventRecorder("hub"), | |
| }) | |
| errChan <- err | |
| }() | |
| // Wait for either startup completion or timeout | |
| select { | |
| case err := <-errChan: | |
| if err != nil { | |
| ginkgo.Fail("Hub manager failed to start: " + err.Error()) | |
| } | |
| case <-time.After(10 * time.Second): | |
| // Manager started successfully, now stop it | |
| stopHub() | |
| // Wait for graceful shutdown | |
| select { | |
| case err := <-errChan: | |
| gomega.Expect(err).To(gomega.Or(gomega.BeNil(), gomega.MatchError("context canceled"))) | |
| case <-time.After(5 * time.Second): | |
| ginkgo.Fail("Hub manager did not shutdown gracefully") | |
| } | |
| } | |
| }) | |
| }) |
🤖 Prompt for AI Agents
In pkg/singleton/hub/manager_test.go around lines 92 to 112, replace the
arbitrary 5-second sleep with a synchronization mechanism such as a channel or
wait group to confirm the manager has started successfully before proceeding.
Add explicit verification steps to ensure the manager is running as expected,
not just that no error occurred. This can include checking for expected side
effects or states after startup. Finally, enhance the test assertions to cover
both startup and shutdown phases more robustly to improve reliability and reduce
flakiness in CI.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/singleton/hub/manager.go (2)
37-41: Consider consistent field visibility in HubOption struct.The struct has mixed visibility with
CommonOptionexported whileregistrationOptionandworkOptionare unexported. Consider making the visibility consistent unless there's a specific encapsulation requirement.type HubOption struct { CommonOption *commonoptions.Options - registrationOption *registrationhub.HubManagerOptions - workOption *workhub.WorkHubManagerOptions + RegistrationOption *registrationhub.HubManagerOptions + WorkOption *workhub.WorkHubManagerOptions }
101-114: Update comment to match actual label selector logic.The comment states that resources "should not have the addon label," but the actual label selector only checks for the existence of the cluster label, not the absence of the addon label.
- func(listOptions *metav1.ListOptions) { - // Note all kube resources managed by registration should have the cluster label, and should not have - // the addon label. + func(listOptions *metav1.ListOptions) { + // Note all kube resources managed by registration should have the cluster label. selector := &metav1.LabelSelector{Or if the addon label exclusion is intended, add it to the selector:
selector := &metav1.LabelSelector{ MatchExpressions: []metav1.LabelSelectorRequirement{ { Key: clusterv1.ClusterNameLabelKey, Operator: metav1.LabelSelectorOpExists, }, + { + Key: addonv1alpha1.AddonLabelKey, + Operator: metav1.LabelSelectorOpDoesNotExist, + }, }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/registration-operator/main.go(1 hunks)pkg/cmd/hub/operator.go(2 hunks)pkg/registration/hub/manager.go(1 hunks)pkg/singleton/hub/manager.go(1 hunks)pkg/singleton/hub/manager_test.go(1 hunks)pkg/work/hub/manager.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/registration-operator/main.go
- pkg/cmd/hub/operator.go
- pkg/singleton/hub/manager_test.go
- pkg/work/hub/manager.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: zhujian7
PR: open-cluster-management-io/ocm#1070
File: SECURITY-INSIGHTS.yml:44-44
Timestamp: 2025-07-14T09:30:25.378Z
Learning: In the open-cluster-management-io/ocm repository, the team prefers to use commit SHAs instead of tags for GitHub Actions dependencies like dependency-review-action for security reasons, as commit SHAs are immutable while tags can be moved.
pkg/registration/hub/manager.go (1)
Learnt from: skeeey
PR: #1071
File: pkg/server/grpc/clients.go:73-76
Timestamp: 2025-07-15T06:10:13.001Z
Learning: In OCM (Open Cluster Management) gRPC server informer setup, cache sync verification is not necessary when starting informers in the clients.Run() method. The current pattern of starting informers as goroutines without explicit cache sync waiting is the preferred approach for this codebase.
pkg/singleton/hub/manager.go (2)
Learnt from: skeeey
PR: #1058
File: pkg/server/services/work/work.go:39-49
Timestamp: 2025-07-02T05:42:41.749Z
Learning: In the OCM (Open Cluster Management) codebase, nil checks with panic statements in constructor functions for interface parameters are considered unnecessary, as the dependency injection/wiring is managed properly and such checks are not part of the established codebase patterns.
Learnt from: skeeey
PR: #1071
File: pkg/server/grpc/clients.go:73-76
Timestamp: 2025-07-15T06:10:13.001Z
Learning: In OCM (Open Cluster Management) gRPC server informer setup, cache sync verification is not necessary when starting informers in the clients.Run() method. The current pattern of starting informers as goroutines without explicit cache sync waiting is the preferred approach for this codebase.
🧬 Code Graph Analysis (1)
pkg/registration/hub/manager.go (1)
pkg/features/feature.go (1)
HubMutableFeatureGate(11-11)
⏰ 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). (7)
- GitHub Check: build
- GitHub Check: unit
- GitHub Check: integration
- GitHub Check: verify
- GitHub Check: e2e-singleton
- GitHub Check: e2e-hosted
- GitHub Check: e2e
🔇 Additional comments (6)
pkg/registration/hub/manager.go (1)
352-352: LGTM! Improved feature gate consistency.Changing the feature gate from
DefaultClusterSettoClusterProfilecorrectly aligns the cluster profile informer startup with the controller that uses it (lines 370-372), ensuring consistent feature gating throughout the cluster profile functionality.pkg/singleton/hub/manager.go (5)
1-35: LGTM! Clean package structure and imports.The package declaration and imports are well-organized and appropriate for the hub manager functionality.
43-49: LGTM! Clean constructor implementation.The constructor properly initializes all option fields using their respective constructors.
51-55: LGTM! Proper flag delegation pattern.The method correctly delegates flag registration to each option type, maintaining proper separation of concerns.
57-96: LGTM! Proper client initialization pattern.The client initialization follows the standard pattern with appropriate error handling for each client type. The metadata client configuration with increased QPS/Burst for the GC controller is a good optimization.
118-175: LGTM! Excellent error handling and graceful shutdown implementation.The error channel approach provides much better error handling than the previous
klog.Fatalpattern. The implementation correctly:
- Uses a buffered channel to prevent goroutine blocking
- Logs errors before sending them to the channel
- Uses
selectto wait for either context cancellation or the first error- Allows for graceful shutdown of all controllers
The fix for the duplicate
workInformersparameter by introducingreplicaSetInformersalso correctly addresses the method signature requirements.
|
/hold |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/common/options/webhook.go (2)
42-44: Consider thread safety for webhook installationThe
InstallWebhookmethod appends to a slice without synchronization. While this is likely called during initialization before concurrent access, consider documenting this expectation or adding synchronization if webhooks might be installed concurrently.
91-91: Fix typo in error messageThere's a typo in the error message: "initilize" should be "initialize".
- logger.Error(err, "unable to initilize webhook server") + logger.Error(err, "unable to initialize webhook server")pkg/singleton/hub/webhook.go (1)
30-39: Good error handling with sequential setup.The method correctly sets up both webhook servers with proper error propagation. Consider removing the empty line before the return statement for consistency.
if err := c.workWebhookOptions.SetupWebhookServer(opts); err != nil { return err } - return nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/registration-operator/main.go(1 hunks)pkg/cmd/hub/operator.go(2 hunks)pkg/cmd/webhook/registration.go(1 hunks)pkg/cmd/webhook/work.go(1 hunks)pkg/common/options/webhook.go(1 hunks)pkg/registration/webhook/option.go(1 hunks)pkg/registration/webhook/start.go(1 hunks)pkg/singleton/hub/webhook.go(1 hunks)pkg/work/webhook/option.go(1 hunks)pkg/work/webhook/start.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/registration-operator/main.go
- pkg/cmd/hub/operator.go
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: zhujian7
PR: open-cluster-management-io/ocm#1070
File: SECURITY-INSIGHTS.yml:44-44
Timestamp: 2025-07-14T09:30:25.378Z
Learning: In the open-cluster-management-io/ocm repository, the team prefers to use commit SHAs instead of tags for GitHub Actions dependencies like dependency-review-action for security reasons, as commit SHAs are immutable while tags can be moved.
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.
pkg/work/webhook/option.go (1)
Learnt from: skeeey
PR: #1086
File: test/integration/util/grpc.go:146-146
Timestamp: 2025-07-23T10:10:42.066Z
Learning: In OCM codebase, there are two different GRPCServerOptions types: the local one in pkg/server/grpc/options.go (which only has GRPCServerConfig field) and the SDK one from open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options (which has ServerBindPort and other fields with default values). Test code uses the SDK version via grpcoptions import alias.
pkg/registration/webhook/start.go (1)
Learnt from: skeeey
PR: #1086
File: test/integration/util/grpc.go:146-146
Timestamp: 2025-07-23T10:10:42.066Z
Learning: In OCM codebase, there are two different GRPCServerOptions types: the local one in pkg/server/grpc/options.go (which only has GRPCServerConfig field) and the SDK one from open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options (which has ServerBindPort and other fields with default values). Test code uses the SDK version via grpcoptions import alias.
pkg/work/webhook/start.go (1)
Learnt from: skeeey
PR: #1086
File: test/integration/util/grpc.go:146-146
Timestamp: 2025-07-23T10:10:42.066Z
Learning: In OCM codebase, there are two different GRPCServerOptions types: the local one in pkg/server/grpc/options.go (which only has GRPCServerConfig field) and the SDK one from open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options (which has ServerBindPort and other fields with default values). Test code uses the SDK version via grpcoptions import alias.
pkg/registration/webhook/option.go (1)
Learnt from: skeeey
PR: #1086
File: test/integration/util/grpc.go:146-146
Timestamp: 2025-07-23T10:10:42.066Z
Learning: In OCM codebase, there are two different GRPCServerOptions types: the local one in pkg/server/grpc/options.go (which only has GRPCServerConfig field) and the SDK one from open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options (which has ServerBindPort and other fields with default values). Test code uses the SDK version via grpcoptions import alias.
pkg/cmd/webhook/registration.go (1)
Learnt from: skeeey
PR: #1086
File: test/integration/util/grpc.go:146-146
Timestamp: 2025-07-23T10:10:42.066Z
Learning: In OCM codebase, there are two different GRPCServerOptions types: the local one in pkg/server/grpc/options.go (which only has GRPCServerConfig field) and the SDK one from open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options (which has ServerBindPort and other fields with default values). Test code uses the SDK version via grpcoptions import alias.
pkg/cmd/webhook/work.go (1)
Learnt from: skeeey
PR: #1086
File: test/integration/util/grpc.go:146-146
Timestamp: 2025-07-23T10:10:42.066Z
Learning: In OCM codebase, there are two different GRPCServerOptions types: the local one in pkg/server/grpc/options.go (which only has GRPCServerConfig field) and the SDK one from open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options (which has ServerBindPort and other fields with default values). Test code uses the SDK version via grpcoptions import alias.
pkg/common/options/webhook.go (1)
Learnt from: skeeey
PR: #1086
File: test/integration/util/grpc.go:146-146
Timestamp: 2025-07-23T10:10:42.066Z
Learning: In OCM codebase, there are two different GRPCServerOptions types: the local one in pkg/server/grpc/options.go (which only has GRPCServerConfig field) and the SDK one from open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options (which has ServerBindPort and other fields with default values). Test code uses the SDK version via grpcoptions import alias.
pkg/singleton/hub/webhook.go (1)
Learnt from: skeeey
PR: #1086
File: test/integration/util/grpc.go:146-146
Timestamp: 2025-07-23T10:10:42.066Z
Learning: In OCM codebase, there are two different GRPCServerOptions types: the local one in pkg/server/grpc/options.go (which only has GRPCServerConfig field) and the SDK one from open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options (which has ServerBindPort and other fields with default values). Test code uses the SDK version via grpcoptions import alias.
⏰ 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). (7)
- GitHub Check: e2e-singleton
- GitHub Check: e2e-hosted
- GitHub Check: e2e
- GitHub Check: verify
- GitHub Check: integration
- GitHub Check: unit
- GitHub Check: build
🔇 Additional comments (26)
pkg/common/options/webhook.go (5)
16-21: LGTM: Well-structured webhook options abstractionThe
WebhookOptionsstruct provides a clean abstraction for webhook server configuration with appropriate separation of concerns. The private fields for scheme and webhooks maintain encapsulation while allowing controlled access through methods.
23-28: LGTM: Sensible defaults and initializationThe default port 9443 is the standard Kubernetes webhook port, and initializing with a new runtime scheme is appropriate.
30-36: LGTM: Clear flag documentationThe flag descriptions are comprehensive and explain the default behavior when certdir is not set.
66-70: LGTM: Strong TLS configurationEnforcing TLS 1.2 as the minimum version is a good security practice that aligns with modern security standards.
9-9: LGTM: Import all auth plugins for comprehensive authentication supportThe import of all Kubernetes client auth plugins is a common pattern that ensures the webhook server can authenticate with clusters using various authentication mechanisms (Azure, GCP, OIDC, etc.).
pkg/work/webhook/option.go (3)
3-5: LGTM: Clean import formattingThe grouped import format is consistent with Go conventions.
8-10: LGTM: Simplified options struct maintains work-specific configurationThe removal of
PortandCertDirfields aligns with the refactoring to centralize webhook server configuration. KeepingManifestLimithere is appropriate as it's work-specific configuration.
19-22: LGTM: Focused flag registrationThe
AddFlagsmethod now only registers work-specific flags, which is consistent with the refactoring pattern where common webhook flags are handled by the centralizedWebhookOptions.pkg/cmd/webhook/registration.go (5)
5-5: LGTM: Controller-runtime import for signal handlingAdding the controller-runtime import is necessary for the
SetupSignalHandler()call.
7-9: LGTM: Clean import organizationThe import of common options and registration webhook packages is well-organized.
12-13: LGTM: Proper initialization of both option typesCreating both
webhookOptionsfor common configuration andoptsfor registration-specific configuration follows the established pattern.
18-21: LGTM: Clean two-phase webhook setup and executionThe setup-then-run pattern is clean and follows good separation of concerns. Using
ctrl.SetupSignalHandler()ensures graceful shutdown handling.
26-27: LGTM: Comprehensive flag registrationRegistering flags from both option sets ensures all configuration options are available to users.
pkg/cmd/webhook/work.go (5)
5-5: LGTM: Controller-runtime import for signal handlingAdding the controller-runtime import is necessary for the
SetupSignalHandler()call.
7-7: LGTM: Import of common optionsThe import of common options package is necessary for the refactored webhook setup.
13-13: LGTM: Consistent pattern with registration webhookCreating
webhookOptionsusing the common options follows the same pattern as the registration webhook, ensuring consistency.
19-22: LGTM: Identical setup pattern to registration webhookThe two-phase setup (SetupWebhookServer then RunWebhookServer) with signal handling is consistent with the registration webhook implementation.
28-28: LGTM: Complete flag registrationAdding flags from both
opsandwebhookOptionsensures all configuration options are available.pkg/registration/webhook/option.go (4)
3-5: LGTM: Simplified importsThe imports are now minimal since only pflag is needed for the interface contract.
8-8: LGTM: Empty struct maintains interface contractThe empty
Optionsstruct maintains the expected interface while all configuration has been moved to the commonWebhookOptions. This is a clean way to handle the refactoring.
12-12: LGTM: Constructor returns empty instanceThe constructor now returns an empty instance, which is consistent with the empty struct.
15-15: LGTM: Clean no-op implementation with blank identifierUsing the blank identifier for the unused parameter is idiomatic Go. The no-op implementation maintains the interface contract while doing nothing, which is appropriate since all flags are now handled by the common
WebhookOptions.pkg/registration/webhook/start.go (1)
13-26: LGTM! Clean refactoring to centralized webhook setup.The delegation of scheme installation and webhook registration to
commonoptions.WebhookOptionssimplifies the code and improves modularity. The error handling forInstallSchemeis appropriate.pkg/work/webhook/start.go (1)
17-32: Well-structured webhook setup with proper feature gating.The implementation correctly:
- Configures the manifest validator before webhook setup
- Handles scheme installation errors appropriately
- Uses feature gates to conditionally enable the ManifestWorkReplicaSet webhook
pkg/singleton/hub/webhook.go (2)
17-23: LGTM! Proper initialization of aggregated options.The constructor correctly initializes both webhook options using their respective constructors.
25-28: LGTM! Proper delegation of flag registration.The method correctly delegates flag registration to both embedded webhook options.
| // Config contains the server (the webhook) cert and key. | ||
| type WebhookOptions struct { | ||
| registrationWebhookOptions *registrationwebhook.Options | ||
| workWebhookOptions *workwebhook.Options | ||
| } |
There was a problem hiding this comment.
Fix misleading struct comment.
The comment mentions "cert and key" but the struct actually aggregates registration and work webhook options. Update the comment to accurately describe the struct's purpose.
-// Config contains the server (the webhook) cert and key.
+// WebhookOptions aggregates configuration options for both registration and work webhooks.
type WebhookOptions struct {🤖 Prompt for AI Agents
In pkg/singleton/hub/webhook.go around lines 11 to 15, the struct comment
incorrectly states that the struct contains server cert and key, but it actually
holds registration and work webhook options. Update the comment to clearly
describe that WebhookOptions aggregates options for registration and work
webhooks, removing any mention of cert and key.
| cmd.AddCommand(hub.NewHubManagerCmd()) | ||
| cmd.AddCommand(hub.NewWebhookCmd()) |
There was a problem hiding this comment.
What commands are these replacing?
There was a problem hiding this comment.
I was thinking to introduce a singleton mode in the clustermanager also, so we do not deploy registration/placement /work controller and webhooks separately, but only two deployment: a single controller and a webhook.
There was a problem hiding this comment.
Oh gotcha. It would be helpful to have some comments or command descriptions that explain that. As a newcomer to the project, it is often difficult to tell which command/manager is which for spoke singleton vs. separate.
There was a problem hiding this comment.
yeah, that is a good suggestion.
Signed-off-by: Jian Qiu <[email protected]>
Signed-off-by: Jian Qiu <[email protected]>
|
PR needs rebase. 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. |
3 participants
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
New Features
Tests