✨ support loadBalancer for grpc endpoint type #1255
Conversation
WalkthroughAdds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1255 +/- ##
==========================================
- Coverage 62.21% 62.20% -0.01%
==========================================
Files 209 209
Lines 16997 17037 +40
==========================================
+ Hits 10574 10598 +24
- Misses 5305 5319 +14
- Partials 1118 1120 +2
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 (1)
pkg/operator/helpers/helpers_test.go (1)
1881-2214: Add route coverage to prevent regressionsOnce you wire the route host handling, please extend
TestGRPCServerHostNameswith aEndpointTypeRoutecase so we fail fast if that path regresses again.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.gois excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/zz_generated.deepcopy.gois excluded by!vendor/**
📒 Files selected for processing (14)
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml(2 hunks)deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml(2 hunks)deploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yaml(1 hunks)deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml(2 hunks)go.mod(1 hunks)manifests/cluster-manager/hub/grpc-server/clusterrole.yaml(1 hunks)manifests/cluster-manager/hub/grpc-server/service.yaml(1 hunks)manifests/config.go(1 hunks)pkg/operator/helpers/helpers.go(1 hunks)pkg/operator/helpers/helpers_test.go(1 hunks)pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go(1 hunks)pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go(4 hunks)pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go(1 hunks)pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2025-08-06T06:00:53.508Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1107
File: deploy/cluster-manager/config/rbac/cluster_role.yaml:165-168
Timestamp: 2025-08-06T06:00:53.508Z
Learning: In OCM gRPC deployments, both the cluster-manager operator ClusterRole and the gRPC server ClusterRole need "create" permission on "managedclustersets/join" resources for proper bootstrapping of managed clusters.
Applied to files:
manifests/cluster-manager/hub/grpc-server/clusterrole.yaml
📚 Learning: 2025-08-04T08:58:41.865Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1107
File: manifests/klusterlet/management/klusterlet-registration-deployment.yaml:111-115
Timestamp: 2025-08-04T08:58:41.865Z
Learning: In OCM klusterlet deployments, gRPC authentication uses different file naming conventions than CSR/kube authentication: gRPC auth expects config.yaml files (/spoke/bootstrap/config.yaml and /spoke/hub-kubeconfig/config.yaml) while CSR/kube auth uses kubeconfig files. The gRPC driver explicitly creates config.yaml files in the secret data via additionalSecretData["config.yaml"] = d.configTemplate.
Applied to files:
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go
📚 Learning: 2025-07-23T10:10:42.066Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm 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.
Applied to files:
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.gopkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.gopkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.gopkg/operator/helpers/helpers.gopkg/operator/helpers/helpers_test.go
📚 Learning: 2025-07-25T01:21:08.891Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1077
File: test/integration/registration/spokecluster_grpc_test.go:0-0
Timestamp: 2025-07-25T01:21:08.891Z
Learning: In OCM integration tests, gRPC and kube authentication mechanisms require different CSR handling approaches: gRPC authentication uses util.ApproveCSR since the hub controller signs client certificates, while kube authentication uses authn.ApproveSpokeClusterCSR to simulate the kube-controller-manager signing client certificates.
Applied to files:
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.gopkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go
📚 Learning: 2025-09-24T00:18:33.339Z
Learnt from: zhiweiyin318
Repo: open-cluster-management-io/ocm PR: 1194
File: deploy/klusterlet/chart/klusterlet/templates/bootstrap_kubeconfig_secret.yaml:25-27
Timestamp: 2025-09-24T00:18:33.339Z
Learning: gRPC config in OCM klusterlet bootstrap secrets does not support multiHubBootstrapHubKubeConfigs scenarios - it is intentionally designed only for single hub bootstrap configurations.
Applied to files:
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go
📚 Learning: 2025-08-28T01:59:04.611Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml:94-176
Timestamp: 2025-08-28T01:59:04.611Z
Learning: The file deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml is copied from vendor and should not be modified directly as changes would be overwritten during vendor updates.
Applied to files:
go.moddeploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamldeploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yamldeploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yamldeploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:58:05.882Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:128-135
Timestamp: 2025-08-28T01:58:05.882Z
Learning: Files in deploy/cluster-manager/chart/cluster-manager/crds/ and similar CRD directories are often copied from vendor/upstream sources and should not be modified directly to avoid conflicts during updates.
Applied to files:
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamldeploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yamldeploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:58:37.933Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:247-280
Timestamp: 2025-08-28T01:58:37.933Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ directory are copied from vendor and should not be modified locally as changes may be overwritten during vendor updates.
Applied to files:
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamldeploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yamldeploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:58:23.958Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:192-225
Timestamp: 2025-08-28T01:58:23.958Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ and deploy/cluster-manager/config/crds/ directories are copied from vendor (open-cluster-management.io/api dependency) and should not be modified locally.
Applied to files:
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamldeploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-07-01T05:27:25.998Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1058
File: pkg/server/services/cluster/cluster.go:48-64
Timestamp: 2025-07-01T05:27:25.998Z
Learning: The ClusterService struct in pkg/server/services/cluster/cluster.go implements the server.Service interface, so method names like List() cannot be renamed as they must match the interface definition exactly.
Applied to files:
pkg/operator/helpers/helpers.go
📚 Learning: 2025-08-28T04:09:12.357Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:94-176
Timestamp: 2025-08-28T04:09:12.357Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ directory are copied from vendor/upstream sources and should not be modified directly.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:58:50.021Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:168-175
Timestamp: 2025-08-28T01:58:50.021Z
Learning: In the OCM project, CRD schemas come from the open-cluster-management.io/api dependency. Changes to CRD schemas should be made upstream in that API package, not in the local CRD YAML files which are generated/copied from the API types.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-09-16T03:04:38.251Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1158
File: deploy/klusterlet/chart/klusterlet/crds/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml:48-48
Timestamp: 2025-09-16T03:04:38.251Z
Learning: CRD files in deploy/klusterlet/chart/klusterlet/crds/ and similar directories are properly updated via "make update" when vendor dependencies change (like open-cluster-management.io/api updates) and validated by "make verify". The issue is only with local hand-edits, not legitimate vendor update processes.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-10-10T02:50:14.188Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1204
File: manifests/cluster-manager/hub/crds/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml:88-91
Timestamp: 2025-10-10T02:50:14.188Z
Learning: Kubernetes CustomResourceDefinition (CRD) OpenAPI v3 schema does not support the `deprecated: true` property for individual fields. Field-level deprecation must be documented in the description field only. The formal `deprecated` property is not supported by Kubernetes CRD tooling.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
🧬 Code graph analysis (5)
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go (1)
pkg/operator/helpers/helpers.go (2)
GRPCAuthEnabled(922-933)GRPCServerEndpointType(983-998)
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.go (2)
pkg/operator/helpers/queuekey.go (1)
ClusterManagerNamespace(51-56)pkg/common/testing/fake_sync_context.go (1)
NewFakeSyncContext(21-27)
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go (2)
pkg/operator/helpers/queuekey.go (6)
GRPCServerSecret(48-48)ClusterManagerNamespace(51-56)SignerSecret(45-45)RegistrationWebhookSecret(40-40)WorkWebhookSecret(42-42)CaBundleConfigmap(46-46)pkg/common/testing/fake_sync_context.go (1)
NewFakeSyncContext(21-27)
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go (2)
pkg/operator/helpers/helpers.go (2)
GRPCAuthEnabled(922-933)GRPCServerHostNames(935-981)pkg/operator/helpers/queuekey.go (1)
GRPCServerSecret(48-48)
pkg/operator/helpers/helpers_test.go (1)
pkg/operator/helpers/helpers.go (2)
GRPCServerHostNames(935-981)GRPCServerEndpointType(983-998)
⏰ 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: build
- GitHub Check: verify
- GitHub Check: integration
- GitHub Check: unit
🔇 Additional comments (1)
manifests/cluster-manager/hub/grpc-server/clusterrole.yaml (1)
18-20: Verify that replicasets access is required for loadBalancer endpoint discovery.The new RBAC rule grants
geton replicasets in theappsAPI group, which follows the existing pattern and appears syntactically sound. However, clarification on its necessity would strengthen the review.Please confirm:
- Why the gRPC server needs to query replicasets when resolving LoadBalancer endpoints. Is this for pod topology discovery, health checks, or another aspect of endpoint resolution?
- Whether
getis the minimal verb needed, or if additional verbs (list, watch) are required during operation.- Whether any other RBAC changes are needed in other ClusterRoles or related manifests to fully support the loadBalancer feature.
...rator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go
Outdated
Show resolved
Hide resolved
zhiweiyin318
force-pushed
the
grpc-server-lb
branch
from
248125b to
7b665af
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/operator/helpers/helpers.go (1)
976-978: Route endpoint type remains unimplemented.The route case still only contains a TODO comment without appending the route host to the SAN list. When users configure
endpoint.GRPC.Type = route, the TLS certificate won't include the route hostname, causing handshake failures.Apply this diff to handle the route case:
case operatorapiv1.EndpointTypeRoute: - // TODO: append route.host to the hostName + if endpoint.GRPC.Route != nil && strings.TrimSpace(endpoint.GRPC.Route.Host) != "" { + hostNames = append(hostNames, endpoint.GRPC.Route.Host) + }
🧹 Nitpick comments (3)
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml (1)
569-611: CRD schema changes look structurally correct; verify they are vendor-syncedThe additions of
loadBalancerandrouteunder bothgrpcandhttps(withcaBundleandhostfields) and the extension of thetypeenum to includeloadBalancerandrouteare structurally consistent with the existinghostnameblock and with how the code uses these endpoint types.However, per prior project notes, CRDs under
deploy/cluster-manager/config/crds/are typically copied/generated from the upstreamopen-cluster-management.io/apitypes and not edited manually, otherwise they can be overwritten or drift from the source. Please confirm these changes came from updating/regenerating the API dependency (and that the chart/OLM copies are in sync), rather than hand-editing this file.Based on learnings
Also applies to: 616-620, 628-683, 676-679
deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml (1)
569-611: Ensure OLM CRD manifest remains in sync with upstream-generated schemaThe
loadBalancer/routeadditions and enum updates here mirror the main CRD schema and look consistent. Given this OLM manifest is normally generated/copied from upstream (and can be overwritten on vendor bumps), please confirm it was updated via the same generation/sync process as the main CRD and not manually edited, so we avoid schema drift between operator, chart CRDs, and OLM metadata.Based on learnings
Also applies to: 616-620, 628-683, 676-679
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go (1)
362-716: Comprehensive GRPC hostname rotation tests; consider also asserting success cases return no error
TestCertRotationGRPCServerHostNamesgives solid coverage of GRPC hostname handling for:
- LoadBalancer endpoints with IP vs hostname.
- Hostname endpoints with custom host.
- Error cases where the service is missing or has no ingress.
Checking
rotationMapfor the GRPC rotation and verifying the hostnames slice matches expectations is exactly what we need.One small robustness improvement: in success cases (
expectedErrorSubstr == ""), the test currently ignores theerrreturned fromcontroller.sync. If future changes causesyncto start returning a non-nil error while still partially populatingrotationMap, these tests might still pass. You could tighten the contract by explicitly assertingerr == nilfor those cases, e.g.:- // Sync the controller - err := controller.sync(context.TODO(), syncContext) - - // Check if we expect an error - if c.expectedErrorSubstr != "" { + // Sync the controller + err := controller.sync(context.TODO(), syncContext) + + // Check if we expect an error + if c.expectedErrorSubstr != "" { ... - } + } else if err != nil { + t.Fatalf("expected no error, got: %v", err) + }Otherwise, the structure and coverage of the test look good.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.gois excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/zz_generated.deepcopy.gois excluded by!vendor/**
📒 Files selected for processing (14)
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml(2 hunks)deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml(2 hunks)deploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yaml(1 hunks)deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml(2 hunks)go.mod(1 hunks)manifests/cluster-manager/hub/grpc-server/clusterrole.yaml(1 hunks)manifests/cluster-manager/hub/grpc-server/service.yaml(1 hunks)manifests/config.go(1 hunks)pkg/operator/helpers/helpers.go(1 hunks)pkg/operator/helpers/helpers_test.go(1 hunks)pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go(1 hunks)pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go(4 hunks)pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go(1 hunks)pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yaml
- go.mod
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2025-07-23T10:10:42.066Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm 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.
Applied to files:
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.gomanifests/cluster-manager/hub/grpc-server/service.yamlpkg/operator/helpers/helpers_test.gopkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.gopkg/operator/helpers/helpers.gopkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.go
📚 Learning: 2025-08-04T08:58:41.865Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1107
File: manifests/klusterlet/management/klusterlet-registration-deployment.yaml:111-115
Timestamp: 2025-08-04T08:58:41.865Z
Learning: In OCM klusterlet deployments, gRPC authentication uses different file naming conventions than CSR/kube authentication: gRPC auth expects config.yaml files (/spoke/bootstrap/config.yaml and /spoke/hub-kubeconfig/config.yaml) while CSR/kube auth uses kubeconfig files. The gRPC driver explicitly creates config.yaml files in the secret data via additionalSecretData["config.yaml"] = d.configTemplate.
Applied to files:
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.gomanifests/cluster-manager/hub/grpc-server/service.yaml
📚 Learning: 2025-09-24T00:18:33.339Z
Learnt from: zhiweiyin318
Repo: open-cluster-management-io/ocm PR: 1194
File: deploy/klusterlet/chart/klusterlet/templates/bootstrap_kubeconfig_secret.yaml:25-27
Timestamp: 2025-09-24T00:18:33.339Z
Learning: gRPC config in OCM klusterlet bootstrap secrets does not support multiHubBootstrapHubKubeConfigs scenarios - it is intentionally designed only for single hub bootstrap configurations.
Applied to files:
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go
📚 Learning: 2025-07-25T01:21:08.891Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1077
File: test/integration/registration/spokecluster_grpc_test.go:0-0
Timestamp: 2025-07-25T01:21:08.891Z
Learning: In OCM integration tests, gRPC and kube authentication mechanisms require different CSR handling approaches: gRPC authentication uses util.ApproveCSR since the hub controller signs client certificates, while kube authentication uses authn.ApproveSpokeClusterCSR to simulate the kube-controller-manager signing client certificates.
Applied to files:
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.gopkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go
📚 Learning: 2025-08-28T01:58:37.933Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:247-280
Timestamp: 2025-08-28T01:58:37.933Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ directory are copied from vendor and should not be modified locally as changes may be overwritten during vendor updates.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamldeploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:58:05.882Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:128-135
Timestamp: 2025-08-28T01:58:05.882Z
Learning: Files in deploy/cluster-manager/chart/cluster-manager/crds/ and similar CRD directories are often copied from vendor/upstream sources and should not be modified directly to avoid conflicts during updates.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamldeploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:58:23.958Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:192-225
Timestamp: 2025-08-28T01:58:23.958Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ and deploy/cluster-manager/config/crds/ directories are copied from vendor (open-cluster-management.io/api dependency) and should not be modified locally.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamldeploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T04:09:12.357Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:94-176
Timestamp: 2025-08-28T04:09:12.357Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ directory are copied from vendor/upstream sources and should not be modified directly.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamldeploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:59:04.611Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml:94-176
Timestamp: 2025-08-28T01:59:04.611Z
Learning: The file deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml is copied from vendor and should not be modified directly as changes would be overwritten during vendor updates.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamldeploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yamldeploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:58:50.021Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:168-175
Timestamp: 2025-08-28T01:58:50.021Z
Learning: In the OCM project, CRD schemas come from the open-cluster-management.io/api dependency. Changes to CRD schemas should be made upstream in that API package, not in the local CRD YAML files which are generated/copied from the API types.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamldeploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-09-16T03:04:38.251Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1158
File: deploy/klusterlet/chart/klusterlet/crds/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml:48-48
Timestamp: 2025-09-16T03:04:38.251Z
Learning: CRD files in deploy/klusterlet/chart/klusterlet/crds/ and similar directories are properly updated via "make update" when vendor dependencies change (like open-cluster-management.io/api updates) and validated by "make verify". The issue is only with local hand-edits, not legitimate vendor update processes.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T02:00:03.385Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/klusterlet/chart/klusterlet/crds/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml:278-280
Timestamp: 2025-08-28T02:00:03.385Z
Learning: CRD files in deploy/klusterlet/chart/klusterlet/crds/ and similar directories are copied from vendor code and should not be modified locally. Grammar or other issues in these files should be reported upstream to the vendor instead.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-06T06:00:53.508Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1107
File: deploy/cluster-manager/config/rbac/cluster_role.yaml:165-168
Timestamp: 2025-08-06T06:00:53.508Z
Learning: In OCM gRPC deployments, both the cluster-manager operator ClusterRole and the gRPC server ClusterRole need "create" permission on "managedclustersets/join" resources for proper bootstrapping of managed clusters.
Applied to files:
manifests/cluster-manager/hub/grpc-server/clusterrole.yaml
🧬 Code graph analysis (5)
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go (2)
pkg/operator/helpers/helpers.go (2)
GRPCAuthEnabled(922-933)GRPCServerHostNames(935-983)pkg/operator/helpers/queuekey.go (1)
GRPCServerSecret(48-48)
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go (1)
pkg/operator/helpers/helpers.go (2)
GRPCAuthEnabled(922-933)GRPCServerEndpointType(985-1000)
pkg/operator/helpers/helpers_test.go (1)
pkg/operator/helpers/helpers.go (2)
GRPCServerHostNames(935-983)GRPCServerEndpointType(985-1000)
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go (1)
pkg/operator/helpers/queuekey.go (6)
GRPCServerSecret(48-48)ClusterManagerNamespace(51-56)SignerSecret(45-45)RegistrationWebhookSecret(40-40)WorkWebhookSecret(42-42)CaBundleConfigmap(46-46)
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.go (2)
pkg/operator/helpers/queuekey.go (1)
ClusterManagerNamespace(51-56)pkg/common/testing/fake_sync_context.go (1)
NewFakeSyncContext(21-27)
⏰ 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: e2e-singleton
- GitHub Check: verify
- GitHub Check: integration
- GitHub Check: e2e
- GitHub Check: build
- GitHub Check: e2e-hosted
🔇 Additional comments (12)
manifests/cluster-manager/hub/grpc-server/clusterrole.yaml (1)
18-20: Verify whether the replicasets permission is actually required for LoadBalancer endpoint discovery.My codebase search found no evidence that the gRPC server implementation accesses native Kubernetes ReplicaSets (from the
appsAPI group). Extensive searches across controller code returned only references to the customManifestWorkReplicaSetresource, not native replicasets.While the RBAC rule is syntactically correct and properly formatted, I cannot confirm from the available code that the gRPC server actually needs this permission for LoadBalancer service support. The PR adds support for LoadBalancer endpoint type, but the endpoint discovery mechanism that would require replicaset access is not evident in the searchable code paths.
Additionally, verify whether other RBAC rules are needed for complete LoadBalancer support (e.g.,
services,services/status, orendpointsresources), as the current rule only grants access to replicasets.Suggested actions:
- Confirm the gRPC server controller code actually queries native Kubernetes ReplicaSets for LoadBalancer endpoint discovery
- Verify all necessary RBAC permissions are included for the complete LoadBalancer feature (check if services/endpoints access is also needed)
- If this permission is not actively used, consider removing it or documenting why it is included as forward-compatibility
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go (1)
233-239: Correct wiring of GRPC auth flag and endpoint typeSetting
GRPCAuthEnabledviahelpers.GRPCAuthEnabledand only derivingGRPCEndpointTypewhen it is true is consistent with the helper semantics and avoids unnecessary endpoint processing when gRPC auth is off. Looks good.manifests/config.go (1)
43-47: HubConfig extended cleanly for GRPC endpoint typeAdding
GRPCEndpointTypealongside other gRPC fields inHubConfigis straightforward and aligns with how templates and controllers consume it. No issues from this change alone.manifests/cluster-manager/hub/grpc-server/service.yaml (1)
17-27: Service template logic matches GRPC endpoint type semanticsConditionally switching to
port: 443andtype: LoadBalancerwhen.GRPCEndpointType == "loadBalancer"and defaulting toport: 8090/ClusterIPotherwise is consistent with the new endpoint type and with the controller tests. The templating/indentation is valid.pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.go (1)
825-958: Good coverage of GRPC service type/port variantsThe new
TestGRPCServiceLoadBalancerTypeexercises the GRPC service behavior for LoadBalancer, hostname, and default configurations by inspecting the created Service objects across hub and management clients. Assertions onServiceTypeandPortalign with the templating logic and help prevent regressions.pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go (2)
5-7: New imports are appropriate and usedThe added
fmt,strings, andcertrotationimports are all used in the new tests and keep dependencies localized to test code. No issues here.Also applies to: 25-26
240-254: Stronger validation of GRPC server secret contentsExtending
TestCertRotationGRPCAuthto assert the GRPC server secret exists and contains bothtls.crtandtls.keymaterially improves the test: it now confirms actual TLS material is written, not just that a Secret object was created. This is a good tightening of expectations.pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go (1)
242-291: LGTM! Certificate SANs will now stay in sync with load balancer changes.The implementation correctly addresses the previous concern about stale certificate SANs. By resolving hostnames on every sync (line 249) and updating the existing rotation's HostNames (line 255), the certificate will reflect current load balancer endpoints rather than caching the initial values.
The error aggregation pattern (lines 242, 251, 287, 291) ensures that hostname resolution failures and target rotation errors are properly collected and reported without short-circuiting the sync process.
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml (1)
583-679: CRD schema extensions look consistent.The new loadBalancer and route endpoint configuration blocks are symmetrically defined for both grpc (lines 583-620) and https (lines 642-679) endpoints, following the same structure as the existing hostname configuration.
Based on learnings, CRD files in this directory are copied from the open-cluster-management.io/api vendor dependency. These changes should originate from that upstream API package rather than being hand-edited locally.
Based on learnings
pkg/operator/helpers/helpers.go (2)
951-975: LoadBalancer endpoint resolution is correctly implemented.The logic properly:
- Appends the custom host if configured (lines 952-954)
- Fetches the gRPC service using the correct naming convention (lines 956-958)
- Returns informative errors when the service or ingress is missing (lines 959-967)
- Extracts both IP and hostname from the first ingress entry (lines 969-975)
This allows certificates to include all relevant SANs for LoadBalancer-exposed gRPC endpoints.
985-1000: Endpoint type detection provides safe defaults.
GRPCServerEndpointTypecorrectly:
- Returns hostname as the default when ServerConfiguration is nil or no gRPC endpoint exists
- Returns hostname when the gRPC endpoint config is nil
- Otherwise returns the configured endpoint type
This defensive approach ensures the system can operate with sensible defaults while respecting explicit configuration.
pkg/operator/helpers/helpers_test.go (1)
1880-2384: LGTM! Comprehensive test coverage for GRPC endpoint resolution.The test suites thoroughly exercise both
GRPCServerHostNamesandGRPCServerEndpointType:TestGRPCServerHostNames covers:
- Default behaviors with nil/empty configurations (lines 1890-1959)
- Hostname endpoints with valid and empty hosts (lines 1961-2011)
- LoadBalancer endpoints with IP, hostname, and both (lines 2013-2135)
- Error conditions: service not found and missing ingress (lines 2137-2195)
TestGRPCServerEndpointType validates:
- Safe defaults when configuration is missing or incomplete (lines 2224-2285)
- Correct type detection for hostname and loadBalancer (lines 2287-2327)
- Edge cases like nil GRPC config and multiple endpoints (lines 2329-2373)
The use of fake Kubernetes clients with pre-populated Service fixtures (e.g., lines 2036-2050) properly simulates real cluster behavior.
|
/assign @skeeey |
|
|
||
| if len(gRPCService.Status.LoadBalancer.Ingress[0].Hostname) != 0 { | ||
| hostNames = append(hostNames, gRPCService.Status.LoadBalancer.Ingress[0].Hostname) | ||
| } |
There was a problem hiding this comment.
do we need to check both of ip and hostname are empty at the same time?
There was a problem hiding this comment.
zhiweiyin318
force-pushed
the
grpc-server-lb
branch
from
7b665af to
9fed584
Compare
| - apiGroups: [""] | ||
| resources: ["pods"] | ||
| verbs: ["get"] | ||
| - apiGroups: ["apps"] |
There was a problem hiding this comment.
why we need to get replicasets?
There was a problem hiding this comment.
there is a warning in grpc-server pod
W1117 03:24:35.902536 1 builder.go:272] unable to get owner reference (falling back to namespace): replicasets.apps "cluster-manager-grpc-server-7fb9c4b846" is forbidden: User "system:serviceaccount:open-cluster-management-hub:grpc-server-sa" cannot get resource "replicasets" in API group "apps" in the namespace "open-cluster-management-hub"
There was a problem hiding this comment.
I think it is because the server is intending to get the podName from envvar? https://github.com/openshift/library-go/blob/master/pkg/operator/events/recorder.go#L59. We do not need this permission of podName envvar is set.
zhiweiyin318
force-pushed
the
grpc-server-lb
branch
from
9fed584 to
6be3212
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.go (1)
825-958: New GRPC service LoadBalancer/ClusterIP test covers key scenariosThe table‑driven
TestGRPCServiceLoadBalancerTypenicely validates serviceTypeandPortfor loadBalancer, hostname, and default configurations against the rendered manifests, and the action scan across hub/management clients is consistent with existing tests.One small nit: in the first two cases you set
Protocol: "grpc". To avoid drift withGRPCServerEndpointType, consider usingProtocol: operatorapiv1.GRPCAuthTypeinstead so the tests track the enum constant rather than a magic string.pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go (1)
362-716: GRPC hostname rotation tests look good; consider relaxing order sensitivity
TestCertRotationGRPCServerHostNamesexercises the main GRPC endpoint variants (LoadBalancer with IP, LoadBalancer with hostname, explicit hostname) plus the key error paths (service missing, ingress missing). Validating that the GRPC target rotation exists only when hostname discovery succeeds, and that itsHostNamescontain the default service DNS plus the LB/IP or custom hostname, gives strong coverage of the new helper behavior.Two minor nits:
- The checks for
grpcRotation.HostNamescompare entries by index; ifGRPCServerHostNamesever changes the ordering while preserving the same set of names, these tests will fail. You could make the assertions order‑insensitive (e.g., sort both slices or compare via a map) to make them more robust.- When locating the GRPC rotation you take
&rotationon the range variable. It’s safe here because youbreakimmediately, but if this is ever refactored to collect multiple pointers, copying to a local (r := rotation; grpcRotation = &r) would avoid the common Go gotcha with range variables.Otherwise the structure and use of
certrotation.TargetRotationandhelpers.GRPCServerSecretlook consistent with the intended design.pkg/operator/helpers/helpers.go (1)
935-990: GRPCServerHostNames / GRPCServerEndpointType are sound; consider preserving the underlying Service errorThe hostname resolution and endpoint‑type selection match the GRPC endpoint semantics and the new CRD enums: you seed with the internal
*-grpc-serverDNS, skip non‑gRPC endpoints, handle hostname and loadBalancer types (including the configured host plus LB IP/hostname), and default tohostnamewhen configuration is absent or incomplete. This aligns with the cert‑rotation controller’s expectations.One small improvement: when the Service
Getfails, you currently drop the underlying error. Keeping it helps debugging cluster‑side issues:- gRPCService, err := kubeClient.CoreV1().Services(clusterManagerNamespace). - Get(context.TODO(), serviceName, metav1.GetOptions{}) - if err != nil { - return hostNames, fmt.Errorf("failed to find service %s in namespace %s", - serviceName, clusterManagerNamespace) - } + gRPCService, err := kubeClient.CoreV1().Services(clusterManagerNamespace). + Get(context.TODO(), serviceName, metav1.GetOptions{}) + if err != nil { + return hostNames, fmt.Errorf("failed to find service %s in namespace %s: %w", + serviceName, clusterManagerNamespace, err) + }Otherwise, the functions look correct and well‑covered by tests.
Also applies to: 992-1007
pkg/operator/helpers/helpers_test.go (1)
1880-2215: GRPC helper tests are thorough; add one case for the “empty ingress fields” error pathThe new
TestGRPCServerHostNamesandTestGRPCServerEndpointTypecases exercise the main success and failure scenarios (nil/empty config, non‑gRPC endpoints, hostname and loadBalancer variants, and most LB error conditions) and line up well with the helper logic.For complete coverage of the LB error handling, consider adding one more
TestGRPCServerHostNamescase whereLoadBalancer.Ingresshas a single entry but bothIPandHostnameare empty, assertingexpectError: true. That would exercise thefailed to find ip or hostname in the ingressbranch.Also applies to: 2217-2381
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.gois excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/zz_generated.deepcopy.gois excluded by!vendor/**
📒 Files selected for processing (13)
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml(2 hunks)deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml(2 hunks)deploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yaml(1 hunks)deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml(2 hunks)go.mod(1 hunks)manifests/cluster-manager/hub/grpc-server/service.yaml(1 hunks)manifests/config.go(1 hunks)pkg/operator/helpers/helpers.go(1 hunks)pkg/operator/helpers/helpers_test.go(1 hunks)pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go(1 hunks)pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go(4 hunks)pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go(1 hunks)pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- go.mod
- deploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yaml
- deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2025-07-23T10:10:42.066Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm 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.
Applied to files:
manifests/cluster-manager/hub/grpc-server/service.yamlpkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.gopkg/operator/helpers/helpers_test.gopkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.gopkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.gopkg/operator/helpers/helpers.go
📚 Learning: 2025-08-28T01:59:04.611Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml:94-176
Timestamp: 2025-08-28T01:59:04.611Z
Learning: The file deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml is copied from vendor and should not be modified directly as changes would be overwritten during vendor updates.
Applied to files:
deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yamldeploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-07-25T01:21:08.891Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1077
File: test/integration/registration/spokecluster_grpc_test.go:0-0
Timestamp: 2025-07-25T01:21:08.891Z
Learning: In OCM integration tests, gRPC and kube authentication mechanisms require different CSR handling approaches: gRPC authentication uses util.ApproveCSR since the hub controller signs client certificates, while kube authentication uses authn.ApproveSpokeClusterCSR to simulate the kube-controller-manager signing client certificates.
Applied to files:
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.gopkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go
📚 Learning: 2025-08-28T01:58:05.882Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:128-135
Timestamp: 2025-08-28T01:58:05.882Z
Learning: Files in deploy/cluster-manager/chart/cluster-manager/crds/ and similar CRD directories are often copied from vendor/upstream sources and should not be modified directly to avoid conflicts during updates.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:58:37.933Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:247-280
Timestamp: 2025-08-28T01:58:37.933Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ directory are copied from vendor and should not be modified locally as changes may be overwritten during vendor updates.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:58:23.958Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:192-225
Timestamp: 2025-08-28T01:58:23.958Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ and deploy/cluster-manager/config/crds/ directories are copied from vendor (open-cluster-management.io/api dependency) and should not be modified locally.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T04:09:12.357Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:94-176
Timestamp: 2025-08-28T04:09:12.357Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ directory are copied from vendor/upstream sources and should not be modified directly.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:58:50.021Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:168-175
Timestamp: 2025-08-28T01:58:50.021Z
Learning: In the OCM project, CRD schemas come from the open-cluster-management.io/api dependency. Changes to CRD schemas should be made upstream in that API package, not in the local CRD YAML files which are generated/copied from the API types.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamlpkg/operator/helpers/helpers.go
📚 Learning: 2025-09-16T03:04:38.251Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1158
File: deploy/klusterlet/chart/klusterlet/crds/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml:48-48
Timestamp: 2025-09-16T03:04:38.251Z
Learning: CRD files in deploy/klusterlet/chart/klusterlet/crds/ and similar directories are properly updated via "make update" when vendor dependencies change (like open-cluster-management.io/api updates) and validated by "make verify". The issue is only with local hand-edits, not legitimate vendor update processes.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-04T08:58:41.865Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1107
File: manifests/klusterlet/management/klusterlet-registration-deployment.yaml:111-115
Timestamp: 2025-08-04T08:58:41.865Z
Learning: In OCM klusterlet deployments, gRPC authentication uses different file naming conventions than CSR/kube authentication: gRPC auth expects config.yaml files (/spoke/bootstrap/config.yaml and /spoke/hub-kubeconfig/config.yaml) while CSR/kube auth uses kubeconfig files. The gRPC driver explicitly creates config.yaml files in the secret data via additionalSecretData["config.yaml"] = d.configTemplate.
Applied to files:
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go
📚 Learning: 2025-09-24T00:18:33.339Z
Learnt from: zhiweiyin318
Repo: open-cluster-management-io/ocm PR: 1194
File: deploy/klusterlet/chart/klusterlet/templates/bootstrap_kubeconfig_secret.yaml:25-27
Timestamp: 2025-09-24T00:18:33.339Z
Learning: gRPC config in OCM klusterlet bootstrap secrets does not support multiHubBootstrapHubKubeConfigs scenarios - it is intentionally designed only for single hub bootstrap configurations.
Applied to files:
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go
📚 Learning: 2025-07-14T09:30:25.378Z
Learnt from: zhujian7
Repo: open-cluster-management-io/ocm PR: 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.
Applied to files:
pkg/operator/helpers/helpers.go
📚 Learning: 2025-07-01T05:27:25.998Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1058
File: pkg/server/services/cluster/cluster.go:48-64
Timestamp: 2025-07-01T05:27:25.998Z
Learning: The ClusterService struct in pkg/server/services/cluster/cluster.go implements the server.Service interface, so method names like List() cannot be renamed as they must match the interface definition exactly.
Applied to files:
pkg/operator/helpers/helpers.go
🧬 Code graph analysis (5)
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go (2)
pkg/operator/helpers/queuekey.go (6)
GRPCServerSecret(48-48)ClusterManagerNamespace(51-56)SignerSecret(45-45)RegistrationWebhookSecret(40-40)WorkWebhookSecret(42-42)CaBundleConfigmap(46-46)pkg/common/testing/fake_sync_context.go (1)
NewFakeSyncContext(21-27)
pkg/operator/helpers/helpers_test.go (1)
pkg/operator/helpers/helpers.go (2)
GRPCServerHostNames(935-990)GRPCServerEndpointType(992-1007)
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go (1)
pkg/operator/helpers/helpers.go (2)
GRPCAuthEnabled(922-933)GRPCServerEndpointType(992-1007)
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.go (2)
pkg/operator/helpers/queuekey.go (1)
ClusterManagerNamespace(51-56)pkg/common/testing/fake_sync_context.go (1)
NewFakeSyncContext(21-27)
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go (2)
pkg/operator/helpers/helpers.go (2)
GRPCAuthEnabled(922-933)GRPCServerHostNames(935-990)pkg/operator/helpers/queuekey.go (1)
GRPCServerSecret(48-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: e2e-hosted
- GitHub Check: e2e-singleton
- GitHub Check: e2e
- GitHub Check: integration
- GitHub Check: unit
- GitHub Check: verify
🔇 Additional comments (9)
manifests/config.go (1)
46-46: GRPCEndpointType addition is consistent with helper usageAdding
GRPCEndpointType stringtoHubConfigmatcheshelpers.GRPCServerEndpointType’s return type and safely defaults to""when gRPC auth is disabled.pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go (1)
233-239: gRPC auth and endpoint-type wiring into HubConfig looks correctUsing
helpers.GRPCAuthEnabledand, when true,helpers.GRPCServerEndpointTypeto populateconfig.GRPCEndpointTypecleanly exposes the endpoint choice to manifests without affecting non‑gRPC setups.manifests/cluster-manager/hub/grpc-server/service.yaml (1)
17-27: Conditional port/type based on GRPCEndpointType is soundThe template correctly switches the service to
type: LoadBalancerwithport: 443when.GRPCEndpointType == "loadBalancer", and otherwise keepsClusterIPon8090while still targeting container port8090. Indentation keeps the rendered YAML valid.pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go (3)
3-29: New imports are aligned with added GRPC rotation tests
fmt,strings, and thecertrotationpackage are all used in the new GRPC hostname/error‑path assertions and TLS rotation checks; the import changes are consistent with the added tests.
174-179: Including GRPCServerSecret in secret informers is appropriateAdding
helpers.GRPCServerSecretto thesecretInformersmap ensures the cert‑rotation controller can watch and drive rotation for the GRPC server TLS secret alongside the existing signer/webhook secrets.
241-254: Extra validation of GRPC server TLS secret improves coverageExtending the “Enable GRPC” case in
TestCertRotationGRPCAuthto fetchhelpers.GRPCServerSecretand assert presence oftls.crtandtls.keyis a good sanity check that rotation produced a structurally valid TLS secret.deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml (1)
583-620: GRPC/HTTPS loadBalancer & route schema extensions look consistentThe new
loadBalancer/routeobjects and the extendedtypeenums under bothgrpcandhttpsmatch the existinghostnameshape and look API‑compatible. Assuming this manifest was regenerated from the updatedopen-cluster-management.io/apitypes (viamake update) rather than hand‑edited, I don’t see issues here.Based on learnings
Also applies to: 642-679
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go (1)
242-291: Dynamic GRPC hostname handling and aggregated error reporting look correctResolving GRPC hostnames on every sync, updating existing rotations when present, and aggregating both hostname‑resolution and target‑rotation errors via
NewMultiLineAggregategives up‑to‑date SANs without regressing the other cert paths. The slice mutation androtationMapupdates are safe with the current struct/slice usage.deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml (1)
583-620: CRD endpoint exposure schema matches the new GRPC/HTTPS endpoint typesThe added
loadBalancer/routeblocks and extendedtypeenums underserverConfiguration.endpointsExposure.grpc/httpsare aligned with the new endpoint types used in code and tests. As long as this CRD came from the updatedopen-cluster-management.io/api(e.g., viamake update) rather than manual edits, it looks good.Based on learnings
Also applies to: 642-679
zhiweiyin318
force-pushed
the
grpc-server-lb
branch
from
6be3212 to
2164a5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go (1)
362-716: Good coverage of GRPC server hostnames; consider reducing order coupling
TestCertRotationGRPCServerHostNamesdoes a nice job covering:
- LoadBalancer with IP and hostname
- Custom hostname endpoint type
- Error paths (service missing, ingress missing) and their impact on
rotationMap.One minor suggestion: the assertions currently rely on the exact order of
grpcRotation.HostNames. To make the tests more resilient to benign implementation changes, you could compare sorted slices or treat the hostnames as a set instead of checkingHostNames[i]by index.deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml (1)
569-620: CRD extension for grpc/https endpoint types looks consistent; ensure it stays in sync with vendor‑generated CRDsThe new
loadBalancerandrouteblocks under bothgrpcandhttps, plus the extendedtypeenums, line up with the new endpoint exposure types introduced in the API and controllers.However, per prior guidance, this OLM catalog CRD is copied from the API/vendor source and is prone to drift if edited manually. Please ensure that:
- These changes are generated from the updated
open-cluster-management.io/apiCRD definitions (or via the usual sync script), not hand‑maintained.- The CRD here matches the copies under
deploy/cluster-manager/config/crds/...and the Helm chart CRDs so all three stay consistent.[based on learnings]
Also applies to: 628-679
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.gois excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/zz_generated.deepcopy.gois excluded by!vendor/**
📒 Files selected for processing (13)
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml(2 hunks)deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml(2 hunks)deploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yaml(1 hunks)deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml(2 hunks)go.mod(1 hunks)manifests/cluster-manager/hub/grpc-server/service.yaml(1 hunks)manifests/config.go(1 hunks)pkg/operator/helpers/helpers.go(1 hunks)pkg/operator/helpers/helpers_test.go(1 hunks)pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go(1 hunks)pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go(4 hunks)pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go(1 hunks)pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- deploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.go
- manifests/cluster-manager/hub/grpc-server/service.yaml
- deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-07-23T10:10:42.066Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm 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.
Applied to files:
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.gopkg/operator/helpers/helpers.gopkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.gopkg/operator/helpers/helpers_test.go
📚 Learning: 2025-07-25T01:21:08.891Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1077
File: test/integration/registration/spokecluster_grpc_test.go:0-0
Timestamp: 2025-07-25T01:21:08.891Z
Learning: In OCM integration tests, gRPC and kube authentication mechanisms require different CSR handling approaches: gRPC authentication uses util.ApproveCSR since the hub controller signs client certificates, while kube authentication uses authn.ApproveSpokeClusterCSR to simulate the kube-controller-manager signing client certificates.
Applied to files:
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.gopkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go
📚 Learning: 2025-08-04T08:58:41.865Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1107
File: manifests/klusterlet/management/klusterlet-registration-deployment.yaml:111-115
Timestamp: 2025-08-04T08:58:41.865Z
Learning: In OCM klusterlet deployments, gRPC authentication uses different file naming conventions than CSR/kube authentication: gRPC auth expects config.yaml files (/spoke/bootstrap/config.yaml and /spoke/hub-kubeconfig/config.yaml) while CSR/kube auth uses kubeconfig files. The gRPC driver explicitly creates config.yaml files in the secret data via additionalSecretData["config.yaml"] = d.configTemplate.
Applied to files:
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go
📚 Learning: 2025-08-28T01:59:04.611Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml:94-176
Timestamp: 2025-08-28T01:59:04.611Z
Learning: The file deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml is copied from vendor and should not be modified directly as changes would be overwritten during vendor updates.
Applied to files:
go.moddeploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamldeploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml
📚 Learning: 2025-07-14T09:30:25.378Z
Learnt from: zhujian7
Repo: open-cluster-management-io/ocm PR: 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.
Applied to files:
pkg/operator/helpers/helpers.go
📚 Learning: 2025-08-28T01:58:50.021Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:168-175
Timestamp: 2025-08-28T01:58:50.021Z
Learning: In the OCM project, CRD schemas come from the open-cluster-management.io/api dependency. Changes to CRD schemas should be made upstream in that API package, not in the local CRD YAML files which are generated/copied from the API types.
Applied to files:
pkg/operator/helpers/helpers.godeploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-07-01T05:27:25.998Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1058
File: pkg/server/services/cluster/cluster.go:48-64
Timestamp: 2025-07-01T05:27:25.998Z
Learning: The ClusterService struct in pkg/server/services/cluster/cluster.go implements the server.Service interface, so method names like List() cannot be renamed as they must match the interface definition exactly.
Applied to files:
pkg/operator/helpers/helpers.go
📚 Learning: 2025-08-28T01:58:37.933Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:247-280
Timestamp: 2025-08-28T01:58:37.933Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ directory are copied from vendor and should not be modified locally as changes may be overwritten during vendor updates.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:58:05.882Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:128-135
Timestamp: 2025-08-28T01:58:05.882Z
Learning: Files in deploy/cluster-manager/chart/cluster-manager/crds/ and similar CRD directories are often copied from vendor/upstream sources and should not be modified directly to avoid conflicts during updates.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:58:23.958Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:192-225
Timestamp: 2025-08-28T01:58:23.958Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ and deploy/cluster-manager/config/crds/ directories are copied from vendor (open-cluster-management.io/api dependency) and should not be modified locally.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T04:09:12.357Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:94-176
Timestamp: 2025-08-28T04:09:12.357Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ directory are copied from vendor/upstream sources and should not be modified directly.
Applied to files:
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
🧬 Code graph analysis (4)
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go (1)
pkg/operator/helpers/helpers.go (2)
GRPCAuthEnabled(922-933)GRPCServerEndpointType(992-1007)
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go (2)
pkg/operator/helpers/helpers.go (2)
GRPCAuthEnabled(922-933)GRPCServerHostNames(935-990)pkg/operator/helpers/queuekey.go (1)
GRPCServerSecret(48-48)
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go (2)
pkg/operator/helpers/queuekey.go (6)
GRPCServerSecret(48-48)ClusterManagerNamespace(51-56)SignerSecret(45-45)RegistrationWebhookSecret(40-40)WorkWebhookSecret(42-42)CaBundleConfigmap(46-46)pkg/common/testing/fake_sync_context.go (1)
NewFakeSyncContext(21-27)
pkg/operator/helpers/helpers_test.go (1)
pkg/operator/helpers/helpers.go (2)
GRPCServerHostNames(935-990)GRPCServerEndpointType(992-1007)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: e2e
- GitHub Check: e2e-singleton
- GitHub Check: e2e-hosted
- GitHub Check: unit
- GitHub Check: integration
- GitHub Check: verify
🔇 Additional comments (9)
go.mod (1)
43-43: API module bump looks consistent with CRD/schema changesUpdating
open-cluster-management.io/apihere aligns with the new endpoint exposure fields introduced elsewhere in the PR. Please double‑check that:
- There are no breaking API changes for existing callers.
go mod tidyand vendoring/regeneration steps have been run so generated CRDs and clients match this version.pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go (1)
233-239: GRPC auth and endpoint type wiring looks correctPopulating
config.GRPCAuthEnabledandconfig.GRPCEndpointTypevia the helpers centralizes the GRPC server behavior in the HubConfig and matches the new helper semantics (defaulting tohostnamewhen unset). No issues spotted here; downstream reconcilers and templates should now have all the information they need.manifests/config.go (1)
45-47: HubConfig extension for GRPC endpoint type is reasonableAdding
GRPCEndpointType stringalongside the existing GRPC fields matches the controller wiring and enables templates to branch on endpoint type (hostname vs loadBalancer/route). The field is non‑intrusive and safe when GRPC auth is disabled.pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go (1)
242-254: Stronger validation of GRPC server secret contentsThe updated GRPC auth test now actually fetches the GRPC server secret and asserts
tls.crt/tls.keypresence, not just rotationMap state. This closes an important gap and ensures the rotation logic results in a viable serving secret.deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml (1)
583-620: CRD schema additions align with the loadBalancer feature.The new
loadBalancerandrouteendpoint exposure types are properly structured withcaBundleandhostfields matching the existinghostnamepattern. The schema changes are consistent with the feature objectives.Based on learnings: These CRD files are copied from the
open-cluster-management.io/apidependency. If any schema adjustments are needed, they should be made upstream in that API package rather than here.pkg/operator/helpers/helpers.go (2)
935-990: LGTM! Comprehensive LoadBalancer endpoint handling.The updated
GRPCServerHostNamesimplementation correctly:
- Filters for GRPC protocol endpoints
- Handles the LoadBalancer type with thorough validation (service lookup, ingress presence, IP/hostname availability)
- Appends both custom LoadBalancer.Host and discovered ingress IP/hostname to the SANs list
- Returns descriptive errors for each failure scenario
The Route case TODO is appropriately deferred, with follow-up issue #1260 already created per the PR comments.
992-1007: LGTM! Clean endpoint type resolver.The
GRPCServerEndpointTypefunction correctly defaults toHostnamewhen:
- Server configuration is nil
- No GRPC endpoints are configured
- GRPC config is nil
And properly returns the configured type otherwise.
pkg/operator/helpers/helpers_test.go (2)
1880-2215: LGTM! Comprehensive test coverage for hostname resolution.The
TestGRPCServerHostNamestest suite thoroughly validates:
- Default behavior (nil/empty server configuration)
- Protocol filtering (non-GRPC endpoints ignored)
- Hostname endpoints (valid hosts and empty/whitespace-only hosts)
- LoadBalancer endpoints across multiple scenarios:
- Custom host plus ingress IP
- Hostname-only ingress
- Both IP and hostname in ingress
- Error cases: service not found, empty ingress list
The test cases properly use fake Kubernetes clients with
existingObjectsto simulate API responses and verify error handling.
2217-2384: LGTM! Complete coverage for endpoint type resolution.The
TestGRPCServerEndpointTypetest suite validates all scenarios:
- Default fallback to
Hostnamewhen config is nil/empty- Protocol filtering (non-GRPC returns default)
- Nil GRPC config handling
- Correct type returned for both Hostname and LoadBalancer configurations
- Mixed endpoint lists (GRPC identified among other protocols)
...rator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go
Show resolved
Hide resolved
zhiweiyin318
force-pushed
the
grpc-server-lb
branch
from
2164a5c to
d8d5aa0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pkg/operator/helpers/helpers.go (1)
983-985: Route case still TODO; ensure it’s tracked and tested.Implement route host inclusion (e.g., append endpoint.GRPC.Route.Host) and add tests. I see a follow‑up issue was mentioned earlier; confirm linkage and priority.
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go (1)
246-269: Critical: hostname refresh isn’t persisted back to rotationMap.After updating rotations.targetRotations[i].HostNames, you must save rotations back to c.rotationMap; otherwise the change is lost. Same issue was flagged earlier.
} else if hasRotation(rotations.targetRotations, helpers.GRPCServerSecret) { for i := range rotations.targetRotations { if rotations.targetRotations[i].Name == helpers.GRPCServerSecret { rotations.targetRotations[i].HostNames = hostNames break } } + c.rotationMap[clustermanagerName] = rotations } else { rotations.targetRotations = append(rotations.targetRotations, certrotation.TargetRotation{ Namespace: clustermanagerNamespace, Name: helpers.GRPCServerSecret, Validity: TargetCertValidity, HostNames: hostNames, Lister: c.secretInformers[helpers.GRPCServerSecret].Lister(), Client: c.kubeClient.CoreV1(), }) c.rotationMap[clustermanagerName] = rotations }
🧹 Nitpick comments (4)
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.go (1)
825-959: Add targetPort assertion to prevent regressions.Also assert service.Spec.Ports[0].TargetPort == 8090 in all cases, since port switches to 443 for LB while targetPort remains 8090.
- actualServicePort = service.Spec.Ports[0].Port + actualServicePort = service.Spec.Ports[0].Port + if service.Spec.Ports[0].TargetPort.IntVal != 8090 { + t.Errorf("Expected targetPort 8090, got %d", service.Spec.Ports[0].TargetPort.IntVal) + }pkg/operator/helpers/helpers_test.go (1)
1882-2196: Nice coverage; add a route-case test when implemented.Once route handling is added, include a case for grpc.type=route to assert host inclusion and error behavior.
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go (1)
362-716: Add a refresh test to detect hostname updates across syncs.Simulate LB ingress change and assert the rotation hostnames update after a second sync. This would have caught the missing map write-back.
// Add near existing tests func TestCertRotationGRPCHostnamesRefreshOnChange(t *testing.T) { ns := helpers.ClusterManagerNamespace(testClusterManagerNameDefault, operatorapiv1.InstallModeDefault) cm := newClusterManager(testClusterManagerNameDefault, operatorapiv1.InstallModeDefault) cm.Spec.ServerConfiguration = &operatorapiv1.ServerConfiguration{ EndpointsExposure: []operatorapiv1.EndpointExposure{{ Protocol: operatorapiv1.GRPCAuthType, GRPC: &operatorapiv1.Endpoint{Type: operatorapiv1.EndpointTypeLoadBalancer}, }}, } svcName := testClusterManagerNameDefault + "-grpc-server" kubeClient := fakekube.NewSimpleClientset( &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}, &corev1.Service{ ObjectMeta: metav1.ObjectMeta{Name: svcName, Namespace: ns}, Status: corev1.ServiceStatus{LoadBalancer: corev1.LoadBalancerStatus{ Ingress: []corev1.LoadBalancerIngress{{IP: "1.1.1.1"}}, }}, }, ) // wire informers/controller like in other tests... // first sync // mutate Service to new ingress (e.g., 2.2.2.2) and update in fake client // second sync // assert rotationMap entry for GRPCServerSecret hostnames now contains 2.2.2.2 (not 1.1.1.1) }pkg/operator/helpers/helpers.go (1)
935-990: Improve LB resolution: include err, handle all ingress entries, de‑dupe.
- Wrap the underlying Get error for easier debugging.
- Append all ingress IPs/hostnames (not just index 0).
- Avoid duplicates in hostNames.
- gRPCService, err := kubeClient.CoreV1().Services(clusterManagerNamespace). - Get(context.TODO(), serviceName, metav1.GetOptions{}) - if err != nil { - return hostNames, fmt.Errorf("failed to find service %s in namespace %s", - serviceName, clusterManagerNamespace) - } + gRPCService, err := kubeClient.CoreV1().Services(clusterManagerNamespace). + Get(context.TODO(), serviceName, metav1.GetOptions{}) + if err != nil { + return hostNames, fmt.Errorf("failed to get service %s in %s: %w", + serviceName, clusterManagerNamespace, err) + } - if len(gRPCService.Status.LoadBalancer.Ingress) == 0 { + if len(gRPCService.Status.LoadBalancer.Ingress) == 0 { return hostNames, fmt.Errorf("failed to find ingress in the status of the service %s in namespace %s", serviceName, clusterManagerNamespace) } - - if len(gRPCService.Status.LoadBalancer.Ingress[0].IP) == 0 && - len(gRPCService.Status.LoadBalancer.Ingress[0].Hostname) == 0 { - return hostNames, fmt.Errorf("failed to find ip or hostname in the ingress "+ - "in the status of the service %s in namespace %s", serviceName, clusterManagerNamespace) - } - - if len(gRPCService.Status.LoadBalancer.Ingress[0].IP) != 0 { - hostNames = append(hostNames, gRPCService.Status.LoadBalancer.Ingress[0].IP) - } - - if len(gRPCService.Status.LoadBalancer.Ingress[0].Hostname) != 0 { - hostNames = append(hostNames, gRPCService.Status.LoadBalancer.Ingress[0].Hostname) - } + added := map[string]struct{}{} + for _, h := range hostNames { added[h] = struct{}{} } + hasAny := false + for _, ing := range gRPCService.Status.LoadBalancer.Ingress { + if ip := strings.TrimSpace(ing.IP); ip != "" { + hasAny = true + if _, ok := added[ip]; !ok { + hostNames = append(hostNames, ip) + added[ip] = struct{}{} + } + } + if hn := strings.TrimSpace(ing.Hostname); hn != "" { + hasAny = true + if _, ok := added[hn]; !ok { + hostNames = append(hostNames, hn) + added[hn] = struct{}{} + } + } + } + if !hasAny { + return hostNames, fmt.Errorf("failed to find ip or hostname in service %s/%s ingress entries", + clusterManagerNamespace, serviceName) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.gois excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/zz_generated.deepcopy.gois excluded by!vendor/**
📒 Files selected for processing (13)
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml(2 hunks)deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml(2 hunks)deploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yaml(1 hunks)deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml(2 hunks)go.mod(1 hunks)manifests/cluster-manager/hub/grpc-server/service.yaml(1 hunks)manifests/config.go(1 hunks)pkg/operator/helpers/helpers.go(1 hunks)pkg/operator/helpers/helpers_test.go(1 hunks)pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go(1 hunks)pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go(4 hunks)pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go(1 hunks)pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go
- deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml
- deploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yaml
- deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
- manifests/config.go
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml:94-176
Timestamp: 2025-08-28T01:59:04.611Z
Learning: The file deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml is copied from vendor and should not be modified directly as changes would be overwritten during vendor updates.
📚 Learning: 2025-08-04T08:58:41.865Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1107
File: manifests/klusterlet/management/klusterlet-registration-deployment.yaml:111-115
Timestamp: 2025-08-04T08:58:41.865Z
Learning: In OCM klusterlet deployments, gRPC authentication uses different file naming conventions than CSR/kube authentication: gRPC auth expects config.yaml files (/spoke/bootstrap/config.yaml and /spoke/hub-kubeconfig/config.yaml) while CSR/kube auth uses kubeconfig files. The gRPC driver explicitly creates config.yaml files in the secret data via additionalSecretData["config.yaml"] = d.configTemplate.
Applied to files:
manifests/cluster-manager/hub/grpc-server/service.yaml
📚 Learning: 2025-07-23T10:10:42.066Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm 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.
Applied to files:
manifests/cluster-manager/hub/grpc-server/service.yamlpkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.gopkg/operator/helpers/helpers_test.gopkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.gopkg/operator/helpers/helpers.go
📚 Learning: 2025-08-28T01:59:04.611Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml:94-176
Timestamp: 2025-08-28T01:59:04.611Z
Learning: The file deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml is copied from vendor and should not be modified directly as changes would be overwritten during vendor updates.
Applied to files:
go.moddeploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-07-25T01:21:08.891Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1077
File: test/integration/registration/spokecluster_grpc_test.go:0-0
Timestamp: 2025-07-25T01:21:08.891Z
Learning: In OCM integration tests, gRPC and kube authentication mechanisms require different CSR handling approaches: gRPC authentication uses util.ApproveCSR since the hub controller signs client certificates, while kube authentication uses authn.ApproveSpokeClusterCSR to simulate the kube-controller-manager signing client certificates.
Applied to files:
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go
📚 Learning: 2025-07-14T09:30:25.378Z
Learnt from: zhujian7
Repo: open-cluster-management-io/ocm PR: 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.
Applied to files:
pkg/operator/helpers/helpers.go
📚 Learning: 2025-08-28T01:58:50.021Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:168-175
Timestamp: 2025-08-28T01:58:50.021Z
Learning: In the OCM project, CRD schemas come from the open-cluster-management.io/api dependency. Changes to CRD schemas should be made upstream in that API package, not in the local CRD YAML files which are generated/copied from the API types.
Applied to files:
pkg/operator/helpers/helpers.godeploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-07-01T05:27:25.998Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1058
File: pkg/server/services/cluster/cluster.go:48-64
Timestamp: 2025-07-01T05:27:25.998Z
Learning: The ClusterService struct in pkg/server/services/cluster/cluster.go implements the server.Service interface, so method names like List() cannot be renamed as they must match the interface definition exactly.
Applied to files:
pkg/operator/helpers/helpers.go
📚 Learning: 2025-08-28T01:58:23.958Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:192-225
Timestamp: 2025-08-28T01:58:23.958Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ and deploy/cluster-manager/config/crds/ directories are copied from vendor (open-cluster-management.io/api dependency) and should not be modified locally.
Applied to files:
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:58:37.933Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:247-280
Timestamp: 2025-08-28T01:58:37.933Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ directory are copied from vendor and should not be modified locally as changes may be overwritten during vendor updates.
Applied to files:
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:58:05.882Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:128-135
Timestamp: 2025-08-28T01:58:05.882Z
Learning: Files in deploy/cluster-manager/chart/cluster-manager/crds/ and similar CRD directories are often copied from vendor/upstream sources and should not be modified directly to avoid conflicts during updates.
Applied to files:
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T04:09:12.357Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:94-176
Timestamp: 2025-08-28T04:09:12.357Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ directory are copied from vendor/upstream sources and should not be modified directly.
Applied to files:
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-09-16T03:04:38.251Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1158
File: deploy/klusterlet/chart/klusterlet/crds/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml:48-48
Timestamp: 2025-09-16T03:04:38.251Z
Learning: CRD files in deploy/klusterlet/chart/klusterlet/crds/ and similar directories are properly updated via "make update" when vendor dependencies change (like open-cluster-management.io/api updates) and validated by "make verify". The issue is only with local hand-edits, not legitimate vendor update processes.
Applied to files:
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-10-10T02:50:14.188Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1204
File: manifests/cluster-manager/hub/crds/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml:88-91
Timestamp: 2025-10-10T02:50:14.188Z
Learning: Kubernetes CustomResourceDefinition (CRD) OpenAPI v3 schema does not support the `deprecated: true` property for individual fields. Field-level deprecation must be documented in the description field only. The formal `deprecated` property is not supported by Kubernetes CRD tooling.
Applied to files:
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T02:00:03.385Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/klusterlet/chart/klusterlet/crds/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml:278-280
Timestamp: 2025-08-28T02:00:03.385Z
Learning: CRD files in deploy/klusterlet/chart/klusterlet/crds/ and similar directories are copied from vendor code and should not be modified locally. Grammar or other issues in these files should be reported upstream to the vendor instead.
Applied to files:
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-06T06:00:53.508Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1107
File: deploy/cluster-manager/config/rbac/cluster_role.yaml:165-168
Timestamp: 2025-08-06T06:00:53.508Z
Learning: In OCM gRPC deployments, both the cluster-manager operator ClusterRole and the gRPC server ClusterRole need "create" permission on "managedclustersets/join" resources for proper bootstrapping of managed clusters.
Applied to files:
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
🧬 Code graph analysis (4)
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.go (2)
pkg/operator/helpers/queuekey.go (1)
ClusterManagerNamespace(51-56)pkg/common/testing/fake_sync_context.go (1)
NewFakeSyncContext(21-27)
pkg/operator/helpers/helpers_test.go (1)
pkg/operator/helpers/helpers.go (2)
GRPCServerHostNames(935-990)GRPCServerEndpointType(992-1007)
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go (2)
pkg/operator/helpers/helpers.go (2)
GRPCAuthEnabled(922-933)GRPCServerHostNames(935-990)pkg/operator/helpers/queuekey.go (1)
GRPCServerSecret(48-48)
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go (2)
pkg/operator/helpers/queuekey.go (6)
GRPCServerSecret(48-48)ClusterManagerNamespace(51-56)SignerSecret(45-45)RegistrationWebhookSecret(40-40)WorkWebhookSecret(42-42)CaBundleConfigmap(46-46)pkg/common/testing/fake_sync_context.go (1)
NewFakeSyncContext(21-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: integration
- GitHub Check: unit
- GitHub Check: e2e-hosted
- GitHub Check: verify
- GitHub Check: e2e-singleton
- GitHub Check: e2e
🔇 Additional comments (5)
go.mod (1)
43-43: Verify API bump drove CRD/manifests updates (no hand edits).Please confirm CRD and catalog changes were produced by updating open-cluster-management.io/api and running the standard update targets (e.g., make update/verify). Hand edits in CRD YAMLs are discouraged and get overwritten. Based on learnings.
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml (1)
583-620: Confirm CRD schema updates are vendor-generated.CRD files here should reflect upstream open-cluster-management.io/api types. Please confirm these loadBalancer/route additions were generated via the normal vendor/update flow, not hand-edited. Based on learnings.
Also applies to: 642-670, 676-679
pkg/operator/helpers/helpers_test.go (1)
2217-2384: LGTM on endpoint type tests.Covers nil/empty/non‑grpc and specific types cleanly.
manifests/cluster-manager/hub/grpc-server/service.yaml (1)
17-26: Template change looks correct.Port/type switch matches GRPCEndpointType; targetPort remains 8090.
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go (1)
242-253: Good: verify tls.crt and tls.key exist.This guards secret content, not just presence.
| hostNames, grpcErr := helpers.GRPCServerHostNames(c.kubeClient, clustermanagerNamespace, clustermanager) | ||
| if grpcErr != nil { | ||
| errs = append(errs, grpcErr) | ||
| } else if hasRotation(rotations.targetRotations, helpers.GRPCServerSecret) { |
There was a problem hiding this comment.
could this hasRotation return the rotation so we do not need another for loop?
There was a problem hiding this comment.
looking at the code, I think changing targetRotations to a map will make it much simpler.
There was a problem hiding this comment.
good suggestion. done.
| } else if hasRotation(rotations.targetRotations, helpers.GRPCServerSecret) { | ||
| for i := range rotations.targetRotations { | ||
| if rotations.targetRotations[i].Name == helpers.GRPCServerSecret { | ||
| rotations.targetRotations[i].HostNames = hostNames |
There was a problem hiding this comment.
will grpc server restart if the hostNames changes? having a log output might be helpful here, because here indicates the hostnames is changed.
There was a problem hiding this comment.
no, I think we need doc this . log is added.
zhiweiyin318
force-pushed
the
grpc-server-lb
branch
from
d8d5aa0 to
ceed65c
Compare
Signed-off-by: Zhiwei Yin <[email protected]>
zhiweiyin318
force-pushed
the
grpc-server-lb
branch
from
ceed65c to
da8c83d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.go (1)
825-958: Good coverage of GRPC Service type/port variants; minor optional cleanupThe new
TestGRPCServiceLoadBalancerTypecleanly validates that:
- LB endpoint ⇒ Service
Type=LoadBalancer,port=443.- Hostname / default configs ⇒
Type=ClusterIP,port=8090.Behavior matches the manifest template and helper wiring.
If you want to trim duplication, consider extracting the “find
<name>-grpc-serverService from actions and return itsType/Port” logic into a small helper used by the cases, but that’s purely cosmetic.pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go (1)
175-179: GRPC server secret rotation tests are solid and align with new behavior
- Adding
helpers.GRPCServerSecretto thesecretInformersmaps ensures the controller can watch and manage the new secret alongside the existing ones.- In
TestCertRotationGRPCAuth, the extra checks that:
- the GRPC server secret exists (when enabling GRPC) and contains
tls.crt/tls.key, androtationMap[cmName].targetRotations[helpers.GRPCServerSecret]is added/removed appropriately on enable/disable,
provide good coverage for the new rotation path.If you’d like broader reuse, you could optionally add
helpers.GRPCServerSecretto thesecretNamesslice so thatassertResourcesExistAndValidandassertResourcesNotExistvalidate this secret as well, but the explicit tests you’ve added already cover the critical behavior.Also applies to: 242-262, 296-302
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.gois excluded by!vendor/**vendor/open-cluster-management.io/api/operator/v1/zz_generated.deepcopy.gois excluded by!vendor/**
📒 Files selected for processing (13)
deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml(2 hunks)deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml(2 hunks)deploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yaml(1 hunks)deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml(2 hunks)go.mod(1 hunks)manifests/cluster-manager/hub/grpc-server/service.yaml(1 hunks)manifests/config.go(1 hunks)pkg/operator/helpers/helpers.go(2 hunks)pkg/operator/helpers/helpers_test.go(1 hunks)pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go(5 hunks)pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go(5 hunks)pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go(1 hunks)pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- manifests/config.go
- go.mod
- deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml:94-176
Timestamp: 2025-08-28T01:59:04.611Z
Learning: The file deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml is copied from vendor and should not be modified directly as changes would be overwritten during vendor updates.
📚 Learning: 2025-07-23T10:10:42.066Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm 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.
Applied to files:
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.gopkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.gopkg/operator/helpers/helpers_test.gomanifests/cluster-manager/hub/grpc-server/service.yamlpkg/operator/helpers/helpers.gopkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go
📚 Learning: 2025-08-04T08:58:41.865Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1107
File: manifests/klusterlet/management/klusterlet-registration-deployment.yaml:111-115
Timestamp: 2025-08-04T08:58:41.865Z
Learning: In OCM klusterlet deployments, gRPC authentication uses different file naming conventions than CSR/kube authentication: gRPC auth expects config.yaml files (/spoke/bootstrap/config.yaml and /spoke/hub-kubeconfig/config.yaml) while CSR/kube auth uses kubeconfig files. The gRPC driver explicitly creates config.yaml files in the secret data via additionalSecretData["config.yaml"] = d.configTemplate.
Applied to files:
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.gopkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go
📚 Learning: 2025-07-25T01:21:08.891Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1077
File: test/integration/registration/spokecluster_grpc_test.go:0-0
Timestamp: 2025-07-25T01:21:08.891Z
Learning: In OCM integration tests, gRPC and kube authentication mechanisms require different CSR handling approaches: gRPC authentication uses util.ApproveCSR since the hub controller signs client certificates, while kube authentication uses authn.ApproveSpokeClusterCSR to simulate the kube-controller-manager signing client certificates.
Applied to files:
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.gopkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go
📚 Learning: 2025-07-15T06:10:13.001Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm 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.
Applied to files:
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go
📚 Learning: 2025-08-28T01:59:04.611Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml:94-176
Timestamp: 2025-08-28T01:59:04.611Z
Learning: The file deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml is copied from vendor and should not be modified directly as changes would be overwritten during vendor updates.
Applied to files:
deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yamldeploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yamldeploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:58:50.021Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:168-175
Timestamp: 2025-08-28T01:58:50.021Z
Learning: In the OCM project, CRD schemas come from the open-cluster-management.io/api dependency. Changes to CRD schemas should be made upstream in that API package, not in the local CRD YAML files which are generated/copied from the API types.
Applied to files:
deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yamlpkg/operator/helpers/helpers.godeploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-07-14T09:30:25.378Z
Learnt from: zhujian7
Repo: open-cluster-management-io/ocm PR: 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.
Applied to files:
pkg/operator/helpers/helpers.go
📚 Learning: 2025-07-01T05:27:25.998Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1058
File: pkg/server/services/cluster/cluster.go:48-64
Timestamp: 2025-07-01T05:27:25.998Z
Learning: The ClusterService struct in pkg/server/services/cluster/cluster.go implements the server.Service interface, so method names like List() cannot be renamed as they must match the interface definition exactly.
Applied to files:
pkg/operator/helpers/helpers.go
📚 Learning: 2025-10-14T09:37:12.472Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1196
File: pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go:231-242
Timestamp: 2025-10-14T09:37:12.472Z
Learning: In the ocm repository, there are two different factory packages with different SyncContext.Queue() return types:
1. `github.com/openshift/library-go/pkg/controller/factory` returns `workqueue.RateLimitingInterface` (non-typed)
2. `open-cluster-management.io/sdk-go/pkg/basecontroller/factory` returns `workqueue.TypedRateLimitingInterface[string]` (typed)
When reviewing controller code, check which factory import is used to determine the correct queue interface type.
Applied to files:
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go
📚 Learning: 2025-08-28T01:58:05.882Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:128-135
Timestamp: 2025-08-28T01:58:05.882Z
Learning: Files in deploy/cluster-manager/chart/cluster-manager/crds/ and similar CRD directories are often copied from vendor/upstream sources and should not be modified directly to avoid conflicts during updates.
Applied to files:
deploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yamldeploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:58:37.933Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:247-280
Timestamp: 2025-08-28T01:58:37.933Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ directory are copied from vendor and should not be modified locally as changes may be overwritten during vendor updates.
Applied to files:
deploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yamldeploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T04:09:12.357Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:94-176
Timestamp: 2025-08-28T04:09:12.357Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ directory are copied from vendor/upstream sources and should not be modified directly.
Applied to files:
deploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yamldeploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T01:58:23.958Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:192-225
Timestamp: 2025-08-28T01:58:23.958Z
Learning: CRD files in deploy/cluster-manager/chart/cluster-manager/crds/ and deploy/cluster-manager/config/crds/ directories are copied from vendor (open-cluster-management.io/api dependency) and should not be modified locally.
Applied to files:
deploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yamldeploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-09-16T03:04:38.251Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1158
File: deploy/klusterlet/chart/klusterlet/crds/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml:48-48
Timestamp: 2025-09-16T03:04:38.251Z
Learning: CRD files in deploy/klusterlet/chart/klusterlet/crds/ and similar directories are properly updated via "make update" when vendor dependencies change (like open-cluster-management.io/api updates) and validated by "make verify". The issue is only with local hand-edits, not legitimate vendor update processes.
Applied to files:
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-06T06:00:53.508Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1107
File: deploy/cluster-manager/config/rbac/cluster_role.yaml:165-168
Timestamp: 2025-08-06T06:00:53.508Z
Learning: In OCM gRPC deployments, both the cluster-manager operator ClusterRole and the gRPC server ClusterRole need "create" permission on "managedclustersets/join" resources for proper bootstrapping of managed clusters.
Applied to files:
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-10-10T02:50:14.188Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1204
File: manifests/cluster-manager/hub/crds/0000_01_addon.open-cluster-management.io_managedclusteraddons.crd.yaml:88-91
Timestamp: 2025-10-10T02:50:14.188Z
Learning: Kubernetes CustomResourceDefinition (CRD) OpenAPI v3 schema does not support the `deprecated: true` property for individual fields. Field-level deprecation must be documented in the description field only. The formal `deprecated` property is not supported by Kubernetes CRD tooling.
Applied to files:
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
📚 Learning: 2025-08-28T02:00:03.385Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1146
File: deploy/klusterlet/chart/klusterlet/crds/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml:278-280
Timestamp: 2025-08-28T02:00:03.385Z
Learning: CRD files in deploy/klusterlet/chart/klusterlet/crds/ and similar directories are copied from vendor code and should not be modified locally. Grammar or other issues in these files should be reported upstream to the vendor instead.
Applied to files:
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml
🧬 Code graph analysis (5)
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go (1)
pkg/operator/helpers/helpers.go (2)
GRPCAuthEnabled(923-934)GRPCServerEndpointType(999-1014)
pkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller_test.go (2)
pkg/operator/helpers/queuekey.go (1)
ClusterManagerNamespace(51-56)pkg/common/testing/fake_sync_context.go (1)
NewFakeSyncContext(21-27)
pkg/operator/helpers/helpers_test.go (1)
pkg/operator/helpers/helpers.go (2)
GRPCServerHostNames(936-997)GRPCServerEndpointType(999-1014)
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go (2)
pkg/operator/helpers/queuekey.go (4)
GRPCServerSecret(48-48)RegistrationWebhookSecret(40-40)RegistrationWebhookService(41-41)WorkWebhookSecret(42-42)pkg/operator/helpers/helpers.go (2)
GRPCAuthEnabled(923-934)GRPCServerHostNames(936-997)
pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go (2)
pkg/operator/helpers/queuekey.go (6)
GRPCServerSecret(48-48)ClusterManagerNamespace(51-56)SignerSecret(45-45)RegistrationWebhookSecret(40-40)WorkWebhookSecret(42-42)CaBundleConfigmap(46-46)pkg/common/testing/fake_sync_context.go (1)
NewFakeSyncContext(21-27)
⏰ 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). (5)
- GitHub Check: e2e-singleton
- GitHub Check: e2e-hosted
- GitHub Check: e2e
- GitHub Check: integration
- GitHub Check: unit
🔇 Additional comments (10)
deploy/cluster-manager/config/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml (1)
560-682: CRD schema for new GRPC/HTTPS endpoint types looks consistent; confirm it’s generated, not hand‑editedThe added
loadBalancer/routeblocks under bothgrpcandhttps, and the enum extension oftypeto includehostname,loadBalancer,route, are structurally consistent with the existinghostnameschema (same caBundle/host pattern, optional fields, and string enums). I don’t see OpenAPI/CRD issues here.Given these CRDs are expected to be generated from
open-cluster-management.io/api, please double‑check that this file came from a vendor/api bump +make update/make verify, not from manual editing. Otherwise it may be overwritten on the next vendor update. Based on learningspkg/operator/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go (1)
233-239: GRPCAuthEnabled / GRPCEndpointType wiring in HubConfig looks correctThe new fields are derived via helpers and only populated when GRPC auth is enabled, which keeps existing behavior for non‑GRPC setups while driving the updated Service template correctly. No functional issues spotted here.
deploy/cluster-manager/olm-catalog/latest/manifests/cluster-manager.clusterserviceversion.yaml (1)
62-62: Metadata timestamp update is fineThe
createdAtannotation bump is metadata‑only and doesn’t affect operator behavior. Assuming this CSV is managed by your usual release/bundle tooling, this looks good.manifests/cluster-manager/hub/grpc-server/service.yaml (1)
15-27: Conditional GRPC Service port/type behavior matches the new endpoint typesDriving
portandspec.typeoff.GRPCEndpointTypegives the expected behavior:LoadBalancerwith port 443 for LB exposure, andClusterIPwith 8090 for hostname/route/default. KeepingtargetPort: 8090is consistent with the pod listener. No issues from a templating or semantics standpoint.deploy/cluster-manager/olm-catalog/latest/manifests/operator.open-cluster-management.io_clustermanagers.yaml (1)
560-682: OLM CRD schema updates are consistent; verify they’re generated from the APIThe added
grpc.loadBalancer/grpc.routeandhttps.loadBalancer/https.routeblocks, plus the extendedtypeenums, line up with the main CRD file and the intended new endpoint exposure types. Structurally this looks correct.Given this OLM CRD manifest is copied from upstream/vendor, please confirm it was regenerated via the
open-cluster-management.io/apibump +make update(or equivalent) rather than hand‑edited, so it won’t be lost on the next vendor sync. Based on learningspkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go (1)
6-6: LGTM! Solid refactoring to support dynamic LoadBalancer hostnames.The switch from slice-based to map-based
targetRotationsimproves lookup efficiency and makes the hostname update logic cleaner. The GRPC hostname resolution now properly detects when LoadBalancer ingress IPs/hostnames change (line 253) and triggers certificate rotation with a helpful warning log (line 256). Error aggregation ensures partial failures don't block other certificate operations.Also applies to: 61-61, 190-191, 216-269, 272-290
pkg/operator/helpers/helpers.go (2)
936-997: LGTM! Well-structured hostname resolution with proper error handling.The function correctly resolves gRPC server hostnames from different endpoint types:
- Default service DNS name (line 937)
- User-provided hostname (lines 948-952)
- LoadBalancer host + ingress IPs/hostnames (lines 955-988)
Returning partial
hostNamesalongside errors (lines 965, 970, 976) is good design—it allows cert rotation to proceed with whatever was successfully resolved. The guards against nil pointers and the deduplication logic usingslices.Containsare appropriate.
999-1014: LGTM! Clean endpoint type resolution.The function provides a straightforward way to determine the gRPC endpoint type, defaulting to
Hostnamewhen configuration is missing or incomplete. This aligns with the default behavior inGRPCServerHostNames.pkg/operator/helpers/helpers_test.go (2)
1880-2258: LGTM! Excellent test coverage for hostname resolution.The test suite comprehensively covers:
- Configuration edge cases (nil, empty, non-GRPC endpoints)
- Hostname endpoint with valid/empty hosts
- LoadBalancer with IP, hostname, both, and duplicates
- Error paths (service not found, missing ingress, empty ingress)
The table-driven approach with clear test names makes this maintainable and easy to extend when Route support is added.
2260-2427: LGTM! Thorough endpoint type detection tests.Tests validate the endpoint type resolution logic across all configuration scenarios, including proper defaulting to
Hostnamewhen configuration is absent or incomplete.
.../operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go
Show resolved
Hide resolved
| signingRotation certrotation.SigningRotation | ||
| caBundleRotation certrotation.CABundleRotation | ||
| targetRotations []certrotation.TargetRotation | ||
| targetRotations map[string]certrotation.TargetRotation |
There was a problem hiding this comment.
the key should be namespace+name
There was a problem hiding this comment.
each clusterManager has its own set of rotations , and the namespace is fixed to open-cluster-management-hub in default mode and clustermanager.Name in hosted mode. so I think it is ok to use name as the key .
not sure if there is any other concern about the key ?
|
/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 |
openshift-merge-bot
bot
merged commit 76449f8
into
open-cluster-management-io:main
3 participants
Summary
support to create a loadBalancer service for grpc server.
Related issue(s)
#1250
Fixes #
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores