-
Notifications
You must be signed in to change notification settings - Fork 67
✨ support join a cluster via grpc #515
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ support join a cluster via grpc #515
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⛔ Files ignored due to path filters (151)
📒 Files selected for processing (8)
WalkthroughAdds gRPC-based registration support: new CLI flags and Options fields for gRPC server/CA, CA data sourcing and injection into klusterlet chart, CSR approval extended to accept gRPC signer identities/groups, a new config constant, indirect go.mod dependencies, and a new e2e gRPC join test. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
dd8be6e to
f515086
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Nitpick comments (1)
pkg/cmd/join/exec.go (1)
51-53: Minor: import alias for clarityEven with the type fix, aliasing the cloudevents grpc options import improves readability and avoids future name conflicts:
- "open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc" + cegrpc "open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (151)
go.sumis excluded by!**/*.sumvendor/cloud.google.com/go/compute/metadata/CHANGES.mdis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/LICENSEis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/README.mdis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/metadata.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/retry.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/retry_linux.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck_linux.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck_windows.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/LICENSEis excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/alias.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/binary_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/encoding.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/event_message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/finish_message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/format/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/format/format.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/attributes.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/match_exact_version.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/spec.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/structured_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/to_event.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/transformer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/write.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client_http.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client_observed.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/defaulters.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/http_receiver.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/invoker.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/observability.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/options.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/receiver.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/context.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/delegating.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/logger.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/retry.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/content_type.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/data_content_encoding.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/codec.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/json/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/json/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/text/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/text/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/xml/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/xml/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_interface.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_marshal.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_unmarshal.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_validation.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/extensions.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/error.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/abuse_protection.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/context.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/headers.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/options.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_lifecycle.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_rate.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_retry.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/retries_result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/utility.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/write_request.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/write_responsewriter.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/inbound.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/lifecycle.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/outbound.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/staticcheck.confis excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/allocate.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/timestamp.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/uri.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/uriref.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/value.gois excluded by!vendor/**vendor/github.com/golang/protobuf/AUTHORSis excluded by!vendor/**vendor/github.com/golang/protobuf/CONTRIBUTORSis excluded by!vendor/**vendor/github.com/golang/protobuf/LICENSEis excluded by!vendor/**vendor/github.com/golang/protobuf/ptypes/any/any.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/github.com/golang/protobuf/ptypes/empty/empty.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/github.com/golang/protobuf/ptypes/timestamp/timestamp.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/golang.org/x/oauth2/authhandler/authhandler.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/appengine.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/default.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/doc.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/error.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/aws.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/basecredentials.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/executablecredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/filecredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/header.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/programmaticrefreshcredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/urlcredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/google.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/externalaccountauthorizeduser/externalaccountauthorizeduser.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/impersonate/impersonate.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/stsexchange/clientauth.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/stsexchange/sts_exchange.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/jwt.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/sdk.gois excluded by!vendor/**vendor/golang.org/x/oauth2/jws/jws.gois excluded by!vendor/**vendor/golang.org/x/oauth2/jwt/jwt.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/key.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/mksyscall.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/syscall.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/value.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/zsyscall_windows.gois excluded by!vendor/**vendor/google.golang.org/grpc/credentials/oauth/oauth.gois excluded by!vendor/**vendor/google.golang.org/protobuf/types/known/emptypb/empty.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/config.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/rotation.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/README.mdis excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent.protois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/gen.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/message.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/option.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/write_message.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/heartbeat/healthcheck.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/heartbeat/heartbeat.gois excluded by!vendor/**
📒 Files selected for processing (7)
go.mod(3 hunks)pkg/cmd/accept/exec.go(2 hunks)pkg/cmd/join/cmd.go(1 hunks)pkg/cmd/join/exec.go(10 hunks)pkg/cmd/join/options.go(2 hunks)pkg/config/env.go(1 hunks)test/e2e/clusteradm/joinhubscenario_grpc_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/e2e/clusteradm/joinhubscenario_grpc_test.go (4)
vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (7)
ClusterManager(18-29)RegistrationHubConfiguration(99-123)RegistrationDriverHub(144-164)GRPCAuthType(131-131)GRPCRegistrationConfig(254-258)ServerConfiguration(166-176)EndpointExposure(178-195)vendor/open-cluster-management.io/api/operator/v1/types_klusterlet.go (3)
RegistrationConfiguration(133-189)Klusterlet(19-28)RegistrationDriver(207-217)test/e2e/util/helper.go (1)
WaitClusterManagerApplied(168-208)vendor/open-cluster-management.io/api/cluster/v1/types.go (1)
ManagedClusterConditionAvailable(232-232)
pkg/cmd/join/exec.go (7)
vendor/open-cluster-management.io/ocm/pkg/operator/helpers/chart/render.go (1)
RenderKlusterletChart(56-62)pkg/cmd/join/options.go (1)
Options(13-117)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.go (1)
GRPCConfig(122-139)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/config.go (1)
CertConfig(29-44)vendor/open-cluster-management.io/api/operator/v1/types_klusterlet.go (2)
Klusterlet(19-28)RegistrationDriver(207-217)vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (1)
GRPCAuthType(131-131)pkg/config/env.go (2)
HubClusterNamespace(15-15)CABundleConfigmap(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-test
🔇 Additional comments (6)
go.mod (1)
43-43: Review comment contains incorrect assumptions about the dependencies; no fixes required.The script output confirms that the flagged dependencies are legitimate:
cloud.google.com/go/compute/metadatais a transitive dependency fromgolang.org/x/oauth2, not a direct GCP-specific addition. It appears only in the OAuth2 Google provider code, which is standard authentication infrastructure for any cloud platform.
cloudevents/sdk-go/v2is actively used in the project code (pkg/cmd/join/exec.goimports it viaopen-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc). It's core infrastructure for the gRPC protocol implementation, not over-scoped bloat.
golang/protobufv1.5.4 coexisting with the modern version is normal transitive dependency behavior.All three dependencies serve legitimate purposes and should remain in
go.mod.Likely an incorrect or invalid review comment.
test/e2e/clusteradm/joinhubscenario_grpc_test.go (1)
4-16: Ignore this review comment; the code does not have the described issues.The variables
e2e,operatorClient, andclusterClientare package-level globals declared ine2e_suite_test.go(lines 26, 35, and 32 respectively) and initialized in theBeforeSuite()hook. They are accessible throughout all tests in theclusteradme2epackage without any import or re-declaration.The proposed import path
"open-cluster-management.io/clusteradm/test/e2e"does not correspond to an actual package—nopackage e2eexists in the codebase. The test uses the suite-level setup pattern where globals are initialized once and reused across tests.The minor redundancy at line 98 (creating
managedClusterOperatorClient) is unnecessary but does not constitute a breaking issue.Likely an incorrect or invalid review comment.
pkg/cmd/join/exec.go (4)
410-410: Namespace constant usage LGTMConsistent use of OperatorNamespace for chart rendering, Deployment lookup, and Pod watch is correct and improves maintainability.
Also applies to: 472-472, 565-565
216-221: GRPC CA fetch placement LGTM; verify RBAC in typical joinsFetching the GRPC CA using the insecure bootstrap client is reasonable. With the resilience fix above, joins won’t fail if the ConfigMap is unreadable. Please confirm your bootstrap token has list/get on the target ConfigMap in common setups, or expect the fallback path.
679-682: HubCAData usage LGTMDirectly setting CertificateAuthorityData when --ca-file is provided is correct and matches the earlier flow.
294-298: Verification confirmed—no issues foundThe code changes are sound:
- KlusterletChartConfig includes a GRPCConfig string field (line 70)
- setGRPCConfig() properly marshals the gRPC configuration and populates o.klusterletChartConfig.GRPCConfig on line 746
- The field is consumed as part of the chart configuration object used for rendering
The wiring is correct and the call sequence is appropriate.
f515086 to
7885b42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/e2e/clusteradm/joinhubscenario_grpc_test.go (1)
70-70: Consider using constants for the auto-approved identity and gRPC server address.The hardcoded service account identity and gRPC server address work for the current test but could be fragile if service naming changes. Consider defining these as test constants or deriving them from the namespace constants already in use.
Also applies to: 97-97
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (151)
go.sumis excluded by!**/*.sumvendor/cloud.google.com/go/compute/metadata/CHANGES.mdis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/LICENSEis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/README.mdis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/metadata.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/retry.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/retry_linux.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck_linux.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck_windows.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/LICENSEis excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/alias.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/binary_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/encoding.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/event_message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/finish_message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/format/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/format/format.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/attributes.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/match_exact_version.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/spec.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/structured_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/to_event.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/transformer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/write.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client_http.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client_observed.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/defaulters.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/http_receiver.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/invoker.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/observability.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/options.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/receiver.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/context.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/delegating.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/logger.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/retry.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/content_type.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/data_content_encoding.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/codec.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/json/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/json/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/text/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/text/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/xml/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/xml/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_interface.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_marshal.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_unmarshal.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_validation.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/extensions.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/error.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/abuse_protection.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/context.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/headers.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/options.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_lifecycle.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_rate.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_retry.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/retries_result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/utility.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/write_request.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/write_responsewriter.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/inbound.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/lifecycle.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/outbound.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/staticcheck.confis excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/allocate.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/timestamp.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/uri.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/uriref.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/value.gois excluded by!vendor/**vendor/github.com/golang/protobuf/AUTHORSis excluded by!vendor/**vendor/github.com/golang/protobuf/CONTRIBUTORSis excluded by!vendor/**vendor/github.com/golang/protobuf/LICENSEis excluded by!vendor/**vendor/github.com/golang/protobuf/ptypes/any/any.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/github.com/golang/protobuf/ptypes/empty/empty.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/github.com/golang/protobuf/ptypes/timestamp/timestamp.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/golang.org/x/oauth2/authhandler/authhandler.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/appengine.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/default.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/doc.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/error.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/aws.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/basecredentials.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/executablecredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/filecredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/header.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/programmaticrefreshcredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/urlcredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/google.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/externalaccountauthorizeduser/externalaccountauthorizeduser.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/impersonate/impersonate.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/stsexchange/clientauth.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/stsexchange/sts_exchange.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/jwt.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/sdk.gois excluded by!vendor/**vendor/golang.org/x/oauth2/jws/jws.gois excluded by!vendor/**vendor/golang.org/x/oauth2/jwt/jwt.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/key.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/mksyscall.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/syscall.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/value.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/zsyscall_windows.gois excluded by!vendor/**vendor/google.golang.org/grpc/credentials/oauth/oauth.gois excluded by!vendor/**vendor/google.golang.org/protobuf/types/known/emptypb/empty.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/config.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/rotation.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/README.mdis excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent.protois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/gen.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/message.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/option.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/write_message.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/heartbeat/healthcheck.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/heartbeat/heartbeat.gois excluded by!vendor/**
📒 Files selected for processing (7)
go.mod(3 hunks)pkg/cmd/accept/exec.go(2 hunks)pkg/cmd/join/cmd.go(1 hunks)pkg/cmd/join/exec.go(10 hunks)pkg/cmd/join/options.go(2 hunks)pkg/config/env.go(1 hunks)test/e2e/clusteradm/joinhubscenario_grpc_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/config/env.go
- go.mod
- pkg/cmd/accept/exec.go
- pkg/cmd/join/cmd.go
- pkg/cmd/join/options.go
🧰 Additional context used
🧬 Code graph analysis (2)
test/e2e/clusteradm/joinhubscenario_grpc_test.go (4)
vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (7)
ClusterManager(18-29)RegistrationHubConfiguration(99-123)RegistrationDriverHub(144-164)GRPCAuthType(131-131)GRPCRegistrationConfig(254-258)ServerConfiguration(166-176)EndpointExposure(178-195)vendor/open-cluster-management.io/api/operator/v1/types_klusterlet.go (3)
RegistrationConfiguration(133-189)Klusterlet(19-28)RegistrationDriver(207-217)test/e2e/util/helper.go (1)
WaitClusterManagerApplied(168-208)vendor/open-cluster-management.io/api/cluster/v1/types.go (1)
ManagedClusterConditionAvailable(232-232)
pkg/cmd/join/exec.go (6)
vendor/open-cluster-management.io/ocm/pkg/operator/helpers/chart/render.go (1)
RenderKlusterletChart(56-62)pkg/cmd/join/options.go (1)
Options(13-117)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.go (1)
GRPCConfig(122-139)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/config.go (2)
CertConfig(29-44)Bytes(9-9)vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (1)
GRPCAuthType(131-131)pkg/config/env.go (2)
HubClusterNamespace(15-15)CABundleConfigMap(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: e2e-test
- GitHub Check: unit-test
- GitHub Check: verify
🔇 Additional comments (6)
test/e2e/clusteradm/joinhubscenario_grpc_test.go (1)
18-139: LGTM! Comprehensive e2e test for gRPC join flow.The test properly validates the complete gRPC registration flow: hub initialization, feature gate enablement, gRPC registration driver configuration, join command execution, and managed cluster availability verification. The test structure follows BDD best practices with proper setup/teardown.
pkg/cmd/join/exec.go (5)
40-40: LGTM! Import additions and typo fix.The new imports support gRPC configuration functionality, and the alias
sdkgrpcproperly avoids potential naming conflicts. The constant rename fromOperatorNamesapcetoOperatorNamespacefixes a typo.Also applies to: 51-52, 58-58
728-751: LGTM! GRPC configuration properly marshaled and injected.The method correctly constructs the gRPC configuration with CA data, URL, and token, marshals it to YAML, and injects it into the klusterlet chart configuration. The type conversion
cert.Bytes(o.grpcCAData)properly handles the CA data type mismatch noted in previous reviews.
195-195: LGTM! Field rename from HubCADate to HubCAData.The typo fix is consistently applied across all usages of the hub CA data field.
Also applies to: 679-681
410-410: LGTM! Consistent usage of corrected OperatorNamespace constant.All references properly use the renamed constant.
Also applies to: 472-472, 565-565
766-774: Missing ConfigMap key validation is a valid concern, but the resilience requirement needs verification.The code directly accesses
cm.Data["ca-bundle.crt"]without checking if the key exists. While Go won't panic (returns empty string), this is poor defensive programming. However, the core claim in the review—that a "previous review suggested non-fatal handling"—cannot be verified in the codebase (no such comment or evidence found).What is confirmed:
- The
mergeCertificateData()function (line 776+) explicitly handles empty CA bundles, suggesting the system architecture supports graceful degradation with missing/empty CA data- The caller at line 217 treats errors as fatal (returns immediately)
- gRPC itself is optional (guarded by
if o.grpcServer != "")What needs clarification:
- Whether the CA bundle ConfigMap is guaranteed to exist in your environment
- Whether the join operation should fail hard or proceed with empty CA data when the ConfigMap is unavailable or missing the key
- Whether the suggested diff aligns with intended failure semantics
4190492 to
a3fac9f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/cmd/join/options.go (1)
28-30: Consider referencing the constant in the comment.The comment mentions "configmap ca-bundle-configmap" but the actual constant
config.CABundleConfigMap(defined inpkg/config/env.goand used inexec.go:771) should be referenced for maintainability. This prevents drift if the ConfigMap name changes.Apply this diff:
- // The grpc ca file which can be found in the configmap ca-bundle-configmap in open-cluster-management-hub ns + // The gRPC CA file (can be found in the ConfigMap config.CABundleConfigMap in the hub namespace) grpcCAFile string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (151)
go.sumis excluded by!**/*.sumvendor/cloud.google.com/go/compute/metadata/CHANGES.mdis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/LICENSEis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/README.mdis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/metadata.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/retry.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/retry_linux.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck_linux.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck_windows.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/LICENSEis excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/alias.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/binary_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/encoding.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/event_message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/finish_message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/format/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/format/format.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/attributes.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/match_exact_version.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/spec.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/structured_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/to_event.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/transformer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/write.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client_http.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client_observed.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/defaulters.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/http_receiver.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/invoker.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/observability.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/options.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/receiver.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/context.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/delegating.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/logger.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/retry.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/content_type.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/data_content_encoding.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/codec.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/json/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/json/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/text/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/text/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/xml/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/xml/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_interface.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_marshal.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_unmarshal.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_validation.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/extensions.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/error.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/abuse_protection.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/context.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/headers.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/options.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_lifecycle.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_rate.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_retry.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/retries_result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/utility.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/write_request.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/write_responsewriter.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/inbound.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/lifecycle.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/outbound.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/staticcheck.confis excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/allocate.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/timestamp.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/uri.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/uriref.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/value.gois excluded by!vendor/**vendor/github.com/golang/protobuf/AUTHORSis excluded by!vendor/**vendor/github.com/golang/protobuf/CONTRIBUTORSis excluded by!vendor/**vendor/github.com/golang/protobuf/LICENSEis excluded by!vendor/**vendor/github.com/golang/protobuf/ptypes/any/any.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/github.com/golang/protobuf/ptypes/empty/empty.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/github.com/golang/protobuf/ptypes/timestamp/timestamp.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/golang.org/x/oauth2/authhandler/authhandler.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/appengine.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/default.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/doc.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/error.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/aws.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/basecredentials.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/executablecredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/filecredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/header.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/programmaticrefreshcredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/urlcredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/google.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/externalaccountauthorizeduser/externalaccountauthorizeduser.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/impersonate/impersonate.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/stsexchange/clientauth.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/stsexchange/sts_exchange.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/jwt.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/sdk.gois excluded by!vendor/**vendor/golang.org/x/oauth2/jws/jws.gois excluded by!vendor/**vendor/golang.org/x/oauth2/jwt/jwt.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/key.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/mksyscall.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/syscall.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/value.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/zsyscall_windows.gois excluded by!vendor/**vendor/google.golang.org/grpc/credentials/oauth/oauth.gois excluded by!vendor/**vendor/google.golang.org/protobuf/types/known/emptypb/empty.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/config.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/rotation.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/README.mdis excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent.protois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/gen.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/message.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/option.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/write_message.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/heartbeat/healthcheck.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/heartbeat/heartbeat.gois excluded by!vendor/**
📒 Files selected for processing (7)
go.mod(3 hunks)pkg/cmd/accept/exec.go(2 hunks)pkg/cmd/join/cmd.go(1 hunks)pkg/cmd/join/exec.go(11 hunks)pkg/cmd/join/options.go(2 hunks)pkg/config/env.go(1 hunks)test/e2e/clusteradm/joinhubscenario_grpc_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- go.mod
- pkg/cmd/accept/exec.go
- pkg/config/env.go
- pkg/cmd/join/cmd.go
- test/e2e/clusteradm/joinhubscenario_grpc_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/cmd/join/exec.go (6)
pkg/cmd/join/options.go (1)
Options(13-117)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.go (1)
GRPCConfig(122-139)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/config.go (2)
CertConfig(29-44)Bytes(9-9)vendor/open-cluster-management.io/api/operator/v1/types_klusterlet.go (2)
Klusterlet(19-28)RegistrationDriver(207-217)vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (1)
GRPCAuthType(131-131)pkg/config/env.go (2)
HubClusterNamespace(15-15)CABundleConfigMap(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: e2e-test
- GitHub Check: verify
- GitHub Check: unit-test
🔇 Additional comments (2)
pkg/cmd/join/exec.go (2)
338-340: LGTM!The validation correctly prevents conflicting registration methods. The check ensures users cannot configure both
--grpc-serverand--registration-auth=awsirsasimultaneously, and the error message clearly explains the constraint.
732-755: LGTM!The gRPC configuration logic is well-structured:
- Early return when gRPC is not configured
- Proper type conversion using
cert.Bytes()- Clear error messages on marshal failure
- Correctly sets the registration driver to
GRPCAuthType
|
/assign @qiujian16 |
a3fac9f to
8de6ad7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/cmd/join/exec.go (2)
189-196: Add empty-file guard for --ca-file (parity with gRPC CA checks)Mirror the robustness you added for --grpc-ca-file by rejecting an empty --ca-file to prevent confusing TLS failures.
if o.caFile != "" { cabytes, err := os.ReadFile(o.caFile) if err != nil { return err } + if len(cabytes) == 0 { + return fmt.Errorf("--ca-file %q is empty", o.caFile) + } o.HubCAData = cabytes }
745-749: Prefer error wrapping with %wWrap the marshal error to preserve cause.
- return fmt.Errorf("failed to marshal GRPC server configuration. %v", err) + return fmt.Errorf("failed to marshal GRPC server configuration: %w", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (151)
go.sumis excluded by!**/*.sumvendor/cloud.google.com/go/compute/metadata/CHANGES.mdis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/LICENSEis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/README.mdis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/metadata.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/retry.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/retry_linux.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck_linux.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck_windows.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/LICENSEis excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/alias.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/binary_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/encoding.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/event_message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/finish_message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/format/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/format/format.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/attributes.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/match_exact_version.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/spec.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/structured_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/to_event.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/transformer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/write.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client_http.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client_observed.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/defaulters.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/http_receiver.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/invoker.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/observability.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/options.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/receiver.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/context.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/delegating.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/logger.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/retry.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/content_type.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/data_content_encoding.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/codec.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/json/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/json/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/text/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/text/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/xml/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/xml/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_interface.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_marshal.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_unmarshal.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_validation.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/extensions.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/error.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/abuse_protection.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/context.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/headers.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/options.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_lifecycle.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_rate.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_retry.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/retries_result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/utility.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/write_request.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/write_responsewriter.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/inbound.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/lifecycle.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/outbound.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/staticcheck.confis excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/allocate.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/timestamp.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/uri.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/uriref.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/value.gois excluded by!vendor/**vendor/github.com/golang/protobuf/AUTHORSis excluded by!vendor/**vendor/github.com/golang/protobuf/CONTRIBUTORSis excluded by!vendor/**vendor/github.com/golang/protobuf/LICENSEis excluded by!vendor/**vendor/github.com/golang/protobuf/ptypes/any/any.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/github.com/golang/protobuf/ptypes/empty/empty.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/github.com/golang/protobuf/ptypes/timestamp/timestamp.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/golang.org/x/oauth2/authhandler/authhandler.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/appengine.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/default.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/doc.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/error.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/aws.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/basecredentials.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/executablecredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/filecredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/header.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/programmaticrefreshcredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/urlcredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/google.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/externalaccountauthorizeduser/externalaccountauthorizeduser.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/impersonate/impersonate.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/stsexchange/clientauth.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/stsexchange/sts_exchange.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/jwt.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/sdk.gois excluded by!vendor/**vendor/golang.org/x/oauth2/jws/jws.gois excluded by!vendor/**vendor/golang.org/x/oauth2/jwt/jwt.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/key.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/mksyscall.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/syscall.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/value.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/zsyscall_windows.gois excluded by!vendor/**vendor/google.golang.org/grpc/credentials/oauth/oauth.gois excluded by!vendor/**vendor/google.golang.org/protobuf/types/known/emptypb/empty.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/config.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/rotation.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/README.mdis excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent.protois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/gen.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/message.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/option.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/write_message.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/heartbeat/healthcheck.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/heartbeat/heartbeat.gois excluded by!vendor/**
📒 Files selected for processing (7)
go.mod(3 hunks)pkg/cmd/accept/exec.go(2 hunks)pkg/cmd/join/cmd.go(1 hunks)pkg/cmd/join/exec.go(11 hunks)pkg/cmd/join/options.go(2 hunks)pkg/config/env.go(1 hunks)test/e2e/clusteradm/joinhubscenario_grpc_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/cmd/accept/exec.go
- pkg/cmd/join/options.go
- pkg/cmd/join/cmd.go
🧰 Additional context used
🧬 Code graph analysis (2)
test/e2e/clusteradm/joinhubscenario_grpc_test.go (4)
test/e2e/util/helper.go (2)
WaitClustersDeleted(103-127)WaitClusterManagerApplied(168-208)vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (8)
ClusterManager(18-29)RegistrationHubConfiguration(99-123)FeatureGateModeTypeEnable(330-330)RegistrationDriverHub(144-164)GRPCAuthType(131-131)GRPCRegistrationConfig(254-258)ServerConfiguration(166-176)EndpointExposure(178-195)vendor/open-cluster-management.io/api/operator/v1/types_klusterlet.go (3)
RegistrationConfiguration(133-189)Klusterlet(19-28)RegistrationDriver(207-217)vendor/open-cluster-management.io/api/cluster/v1/types.go (1)
ManagedClusterConditionAvailable(232-232)
pkg/cmd/join/exec.go (7)
vendor/open-cluster-management.io/ocm/pkg/operator/helpers/chart/render.go (1)
RenderKlusterletChart(56-62)pkg/cmd/join/options.go (1)
Options(13-117)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.go (1)
GRPCConfig(122-139)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/config.go (2)
CertConfig(29-44)Bytes(9-9)vendor/open-cluster-management.io/api/operator/v1/types_klusterlet.go (2)
Klusterlet(19-28)RegistrationDriver(207-217)vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (1)
GRPCAuthType(131-131)pkg/config/env.go (2)
HubClusterNamespace(15-15)CABundleConfigMap(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: e2e-test
- GitHub Check: verify
- GitHub Check: unit-test
🔇 Additional comments (11)
pkg/config/env.go (1)
19-19: LGTM: new ConfigMap constantCABundleConfigMap reads well and follows Go/Kubernetes casing. Callers now use a single source of truth.
pkg/cmd/join/exec.go (9)
40-40: Good: cohesive imports and grpc aliasingImporting config and aliasing the SDK gRPC options avoids collisions with google.golang.org/grpc and keeps env constants centralized.
Also applies to: 51-53
57-59: Namespace constants cleanup looks goodAgentNamespacePrefix and OperatorNamespace remove prior misspellings and magic strings.
216-220: Gating GRPC CA lookup is correctOnly fetching CA when --grpc-server is set keeps the path minimal for non‑gRPC joins.
294-297: LGTM: GRPC config wiring in validate()Populating chart values during validate keeps run() lean.
338-340: Good guard against conflicting registration methodsEarly validation avoids silent overrides between awsirsa and grpc.
414-414: Consistent use of OperatorNamespaceUsing the constant in chart rendering, deployment and pod watches improves portability.
Also applies to: 476-477, 569-570
683-686: Direct HubCAData assignment is fineHonors explicit --ca-file before attempting discovery.
757-783: GRPC CA retrieval is solidClear errors for unreadable files; validates ConfigMap key presence and non-empty bundle. Nice.
732-753: Token correctly stored in Secret and not logged—verification successfulThe token in GRPCConfig is properly handled:
- Stored in a Kubernetes Secret resource at
data.config.yamlwith base64 encoding (bootstrap_kubeconfig_secret.yaml)- Not logged or printed in the code path (no logging detected around the marshaling and chart config assignment)
- Token is only included in the YAML string assigned to the chart configuration, which is then base64-encoded by the Helm template before storage in the Secret
go.mod (1)
43-43: No action needed—all indirects are verified as necessary.The
go mod whyoutput confirms these are legitimate, non-transient dependencies:
cloud.google.com/go/compute/metadatais required bygoogle.golang.org/grpc/credentials/oauthgithub.com/cloudevents/sdk-go/v2is required byopen-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpcgithub.com/golang/protobufis required byopen-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1The
go mod tidycommand completed successfully and retained all three, confirming they are not dead code or experimental cruft.Likely an incorrect or invalid review comment.
8de6ad7 to
2139461
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/cmd/accept/exec.go (1)
146-151: Tighten CSR requester checks: use equality for SA usernames (security)Using HasPrefix for SA usernames can approve unintended SAs. Keep prefix check only for system:bootstrap:, and use exact equality for SA names.
Apply:
- if !strings.HasPrefix(item.Spec.Username, userNameSignatureBootstrapPrefix) && - !strings.HasPrefix(item.Spec.Username, userNameSignatureSA) && - !strings.HasPrefix(item.Spec.Username, userNameGRPCSignatureSA) { + if !(strings.HasPrefix(item.Spec.Username, userNameSignatureBootstrapPrefix) || + item.Spec.Username == userNameSignatureSA || + item.Spec.Username == userNameGRPCSignatureSA) { continue }
🧹 Nitpick comments (2)
pkg/cmd/join/options.go (1)
22-23: Comment nits: prefer “gRPC”, reference constants to avoid driftUse “gRPC” casing and refer to config constants in comments.
- // The grpc server of the hub cluster + // The gRPC server of the hub cluster @@ - // The grpc ca file which can be found in the configmap ca-bundle-configmap in open-cluster-management-hub ns + // The gRPC CA file (can be sourced from ConfigMap config.CABundleConfigMap in config.HubClusterNamespace)Also applies to: 28-30
pkg/cmd/join/exec.go (1)
737-747: Minor naming polish: use grpcConfig for local varPure readability; no behavior change.
- gRPCConfig := sdkgrpc.GRPCConfig{ + grpcConfig := sdkgrpc.GRPCConfig{ @@ - configStr, err := yaml.Marshal(gRPCConfig) + configStr, err := yaml.Marshal(grpcConfig)Also applies to: 750-751
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (151)
go.sumis excluded by!**/*.sumvendor/cloud.google.com/go/compute/metadata/CHANGES.mdis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/LICENSEis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/README.mdis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/metadata.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/retry.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/retry_linux.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck_linux.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck_windows.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/LICENSEis excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/alias.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/binary_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/encoding.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/event_message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/finish_message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/format/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/format/format.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/attributes.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/match_exact_version.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/spec.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/structured_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/to_event.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/transformer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/write.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client_http.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client_observed.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/defaulters.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/http_receiver.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/invoker.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/observability.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/options.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/receiver.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/context.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/delegating.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/logger.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/retry.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/content_type.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/data_content_encoding.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/codec.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/json/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/json/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/text/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/text/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/xml/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/xml/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_interface.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_marshal.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_unmarshal.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_validation.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/extensions.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/error.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/abuse_protection.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/context.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/headers.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/options.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_lifecycle.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_rate.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_retry.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/retries_result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/utility.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/write_request.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/write_responsewriter.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/inbound.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/lifecycle.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/outbound.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/staticcheck.confis excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/allocate.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/timestamp.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/uri.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/uriref.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/value.gois excluded by!vendor/**vendor/github.com/golang/protobuf/AUTHORSis excluded by!vendor/**vendor/github.com/golang/protobuf/CONTRIBUTORSis excluded by!vendor/**vendor/github.com/golang/protobuf/LICENSEis excluded by!vendor/**vendor/github.com/golang/protobuf/ptypes/any/any.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/github.com/golang/protobuf/ptypes/empty/empty.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/github.com/golang/protobuf/ptypes/timestamp/timestamp.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/golang.org/x/oauth2/authhandler/authhandler.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/appengine.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/default.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/doc.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/error.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/aws.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/basecredentials.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/executablecredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/filecredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/header.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/programmaticrefreshcredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/urlcredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/google.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/externalaccountauthorizeduser/externalaccountauthorizeduser.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/impersonate/impersonate.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/stsexchange/clientauth.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/stsexchange/sts_exchange.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/jwt.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/sdk.gois excluded by!vendor/**vendor/golang.org/x/oauth2/jws/jws.gois excluded by!vendor/**vendor/golang.org/x/oauth2/jwt/jwt.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/key.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/mksyscall.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/syscall.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/value.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/zsyscall_windows.gois excluded by!vendor/**vendor/google.golang.org/grpc/credentials/oauth/oauth.gois excluded by!vendor/**vendor/google.golang.org/protobuf/types/known/emptypb/empty.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/config.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/rotation.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/README.mdis excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent.protois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/gen.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/message.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/option.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/write_message.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/heartbeat/healthcheck.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/heartbeat/heartbeat.gois excluded by!vendor/**
📒 Files selected for processing (7)
go.mod(3 hunks)pkg/cmd/accept/exec.go(2 hunks)pkg/cmd/join/cmd.go(1 hunks)pkg/cmd/join/exec.go(11 hunks)pkg/cmd/join/options.go(2 hunks)pkg/config/env.go(1 hunks)test/e2e/clusteradm/joinhubscenario_grpc_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- pkg/cmd/join/cmd.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/cmd/join/exec.go (6)
vendor/open-cluster-management.io/ocm/pkg/operator/helpers/chart/render.go (1)
RenderKlusterletChart(56-62)pkg/cmd/join/options.go (1)
Options(13-117)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.go (1)
GRPCConfig(122-139)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/config.go (2)
CertConfig(29-44)Bytes(9-9)vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (1)
GRPCAuthType(131-131)pkg/config/env.go (2)
HubClusterNamespace(15-15)CABundleConfigMap(19-19)
test/e2e/clusteradm/joinhubscenario_grpc_test.go (4)
test/e2e/util/helper.go (2)
WaitClustersDeleted(103-127)WaitClusterManagerApplied(168-208)vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (7)
ClusterManager(18-29)RegistrationHubConfiguration(99-123)RegistrationDriverHub(144-164)GRPCAuthType(131-131)GRPCRegistrationConfig(254-258)ServerConfiguration(166-176)EndpointExposure(178-195)vendor/open-cluster-management.io/api/operator/v1/types_klusterlet.go (3)
RegistrationConfiguration(133-189)Klusterlet(19-28)RegistrationDriver(207-217)vendor/open-cluster-management.io/api/cluster/v1/types.go (1)
ManagedClusterConditionAvailable(232-232)
🔇 Additional comments (4)
pkg/config/env.go (1)
19-19: LGTM: well-named constantCABundleConfigMap matches Go/Kubernetes naming and is referenced correctly elsewhere.
pkg/cmd/accept/exec.go (1)
31-34: LGTM: GRPC SA and group constantsNew identifiers align with namespace and RBAC conventions.
pkg/cmd/join/exec.go (1)
51-53: LGTM: GRPC config injection, CA handling, and OperatorNamespace integration
- Correct aliasing and type conversion (cert.Bytes) prevent type mismatch.
- getGRPCCAData properly validates file read and ConfigMap key presence.
- OperatorNamespace use centralizes lookups and rendering.
Also applies to: 56-59, 732-755, 757-783
pkg/cmd/join/options.go (1)
22-30: Flag bindings verified—no issues foundThe shell script output confirms that both
grpcServerandgrpcCAFilefields are properly bound to their corresponding CLI flags (cmd.go:91-92) and actively used throughout exec.go. The implementation is correct.
pkg/cmd/join/exec.go
Outdated
| if o.grpcServer != "" && o.registrationAuth == AwsIrsaAuthentication { | ||
| return gherrors.New("cannot use both --grpc-server and --registration-auth=awsirsa; choose one registration method") | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when using grpc, we may check if (registrationAuth == "grpc" && o.grpcServer == "") ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and grpc ca is also required, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only need grpcServer=<grpc server>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this is the join process, the ca is not need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the ca is retrieved from the ca bundle configmap on the hub cluster if --grpc-ca-file is not set .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need the --registration-auth=grpc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will set Klusterlet.RegistrationConfiguration.RegistrationDriver.AuthType = grpc when --grpc-server is set.
looks like--registration-auth=grpcis not needed .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should --registration-auth=grpc and validate --grpc-server when driver is grpc. This could have a consistent behavior comparing to --registration-auth=aws-irsa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added grpc to --registration-auth .
2139461 to
339daf8
Compare
|
/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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (3)
test/e2e/clusteradm/joinhubscenario_grpc_test.go (3)
44-91: Critical: Initialize operatorClient before use.
operatorClientis used at lines 47 and 91 but never defined or initialized. This will cause a compilation failure.Initialize the client after line 42:
gomega.Expect(err).NotTo(gomega.HaveOccurred(), "clusteradm init error") + operatorClient, err := operatorclient.NewForConfig(e2e.Cluster().Hub().KubeConfig()) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + ginkgo.By("wait for cluster-manager CR to be created and update cluster-manager to enable grpc") var clusterManager *operatorv1.ClusterManager gomega.Eventually(func() error {
9-15: Critical: Add missing clientset imports.The test references
operatorClientandclusterClientbut the required imports are missing.Add these imports:
import ( "context" "fmt" "time" "github.com/onsi/ginkgo/v2" "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + clusterclient "open-cluster-management.io/api/client/cluster/clientset/versioned" + operatorclient "open-cluster-management.io/api/client/operator/clientset/versioned" clusterapiv1 "open-cluster-management.io/api/cluster/v1" operatorv1 "open-cluster-management.io/api/operator/v1" "open-cluster-management.io/clusteradm/test/e2e/util" )
121-135: Critical: Initialize clusterClient before use.
clusterClientis used at line 123 but never defined or initialized.Initialize the client before line 122:
ginkgo.By(fmt.Sprintf("wait for cluster %s to become available", e2e.Cluster().Hub().Name())) + clusterClient, err := clusterclient.NewForConfig(e2e.Cluster().Hub().KubeConfig()) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + gomega.Eventually(func() bool { managedCluster, err := clusterClient.ClusterV1().ManagedClusters().Get(
🧹 Nitpick comments (2)
pkg/cmd/join/exec.go (2)
216-220: Consider optimizing CA fetch to only when GRPC auth is used.Currently, GRPC CA data is fetched whenever
grpcServeris set, even ifregistrationAuthmight be configured for a different auth type later. While not harmful, fetching the CA only whenregistrationAuth == operatorv1.GRPCAuthTypewould be more efficient.Consider moving this check into
validate()after the registration auth type is confirmed, or add an early check:- if o.grpcServer != "" { + if o.registrationAuth == operatorv1.GRPCAuthType { if err = o.getGRPCCAData(externalClientUnSecure); err != nil { return err } }Note: This requires that
registrationAuthbe set before callingcomplete().
338-351: Consider explicit validation for conflicting auth configuration flags.While the current switch validates each auth type's requirements, a user could set
--grpc-serverwith--registration-auth=awsirsa, causing the GRPC flag to be silently ignored. An explicit mutual exclusivity check would improve UX by failing fast with a clear error.Add validation before the switch:
+ // Validate mutual exclusivity of auth configuration + if o.registrationAuth == operatorv1.AwsIrsaAuthType && o.grpcServer != "" { + return gherrors.New("cannot specify --grpc-server when --registration-auth=awsirsa") + } + if o.registrationAuth == operatorv1.GRPCAuthType && o.hubClusterArn != "" { + return gherrors.New("cannot specify --hub-cluster-arn when --registration-auth=grpc") + } + switch o.registrationAuth {This provides clearer error messages when incompatible flags are combined.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (151)
go.sumis excluded by!**/*.sumvendor/cloud.google.com/go/compute/metadata/CHANGES.mdis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/LICENSEis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/README.mdis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/metadata.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/retry.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/retry_linux.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck_linux.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck_windows.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/LICENSEis excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/alias.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/binary_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/encoding.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/event_message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/finish_message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/format/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/format/format.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/attributes.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/match_exact_version.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/spec/spec.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/structured_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/to_event.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/transformer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/binding/write.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client_http.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/client_observed.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/defaulters.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/http_receiver.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/invoker.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/observability.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/options.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/client/receiver.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/context.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/delegating.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/logger.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/context/retry.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/content_type.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/data_content_encoding.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/codec.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/json/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/json/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/text/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/text/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/xml/data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/datacodec/xml/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_data.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_interface.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_marshal.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_unmarshal.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_validation.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/event_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v03_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1_reader.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/eventcontext_v1_writer.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/event/extensions.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/error.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/abuse_protection.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/context.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/headers.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/options.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_lifecycle.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_rate.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/protocol_retry.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/retries_result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/utility.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/write_request.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/http/write_responsewriter.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/inbound.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/lifecycle.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/outbound.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/protocol/result.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/staticcheck.confis excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/allocate.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/doc.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/timestamp.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/uri.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/uriref.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/v2/types/value.gois excluded by!vendor/**vendor/github.com/golang/protobuf/AUTHORSis excluded by!vendor/**vendor/github.com/golang/protobuf/CONTRIBUTORSis excluded by!vendor/**vendor/github.com/golang/protobuf/LICENSEis excluded by!vendor/**vendor/github.com/golang/protobuf/ptypes/any/any.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/github.com/golang/protobuf/ptypes/empty/empty.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/github.com/golang/protobuf/ptypes/timestamp/timestamp.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/golang.org/x/oauth2/authhandler/authhandler.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/appengine.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/default.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/doc.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/error.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/aws.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/basecredentials.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/executablecredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/filecredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/header.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/programmaticrefreshcredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/externalaccount/urlcredsource.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/google.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/externalaccountauthorizeduser/externalaccountauthorizeduser.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/impersonate/impersonate.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/stsexchange/clientauth.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/internal/stsexchange/sts_exchange.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/jwt.gois excluded by!vendor/**vendor/golang.org/x/oauth2/google/sdk.gois excluded by!vendor/**vendor/golang.org/x/oauth2/jws/jws.gois excluded by!vendor/**vendor/golang.org/x/oauth2/jwt/jwt.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/key.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/mksyscall.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/syscall.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/value.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/registry/zsyscall_windows.gois excluded by!vendor/**vendor/google.golang.org/grpc/credentials/oauth/oauth.gois excluded by!vendor/**vendor/google.golang.org/protobuf/types/known/emptypb/empty.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/config.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/rotation.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/README.mdis excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent.protois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/cloudevent_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protobuf/v1/gen.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/message.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/option.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/write_message.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/heartbeat/healthcheck.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/heartbeat/heartbeat.gois excluded by!vendor/**
📒 Files selected for processing (7)
go.mod(3 hunks)pkg/cmd/accept/exec.go(2 hunks)pkg/cmd/join/cmd.go(2 hunks)pkg/cmd/join/exec.go(11 hunks)pkg/cmd/join/options.go(2 hunks)pkg/config/env.go(1 hunks)test/e2e/clusteradm/joinhubscenario_grpc_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/cmd/join/cmd.go
- pkg/cmd/join/options.go
- pkg/config/env.go
🧰 Additional context used
🧬 Code graph analysis (2)
test/e2e/clusteradm/joinhubscenario_grpc_test.go (4)
test/e2e/util/helper.go (2)
WaitClustersDeleted(103-127)WaitClusterManagerApplied(168-208)vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (8)
ClusterManager(18-29)RegistrationHubConfiguration(99-123)FeatureGateModeTypeEnable(330-330)RegistrationDriverHub(144-164)GRPCAuthType(131-131)GRPCRegistrationConfig(254-258)ServerConfiguration(166-176)EndpointExposure(178-195)vendor/open-cluster-management.io/api/operator/v1/types_klusterlet.go (3)
RegistrationConfiguration(133-189)Klusterlet(19-28)RegistrationDriver(207-217)vendor/open-cluster-management.io/api/cluster/v1/types.go (1)
ManagedClusterConditionAvailable(232-232)
pkg/cmd/join/exec.go (7)
vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (3)
AwsIrsaAuthType(127-127)GRPCAuthType(131-131)CSRAuthType(129-129)vendor/open-cluster-management.io/ocm/pkg/operator/helpers/chart/render.go (1)
RenderKlusterletChart(56-62)pkg/cmd/join/options.go (1)
Options(13-117)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.go (1)
GRPCConfig(122-139)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/cert/config.go (2)
CertConfig(29-44)Bytes(9-9)vendor/open-cluster-management.io/api/operator/v1/types_klusterlet.go (3)
Klusterlet(19-28)RegistrationConfiguration(133-189)RegistrationDriver(207-217)pkg/config/env.go (2)
HubClusterNamespace(15-15)CABundleConfigMap(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-test
🔇 Additional comments (7)
go.mod (1)
43-43: LGTM! Indirect dependencies align with GRPC support.The three new indirect dependencies (
cloud.google.com/go/compute/metadata,cloudevents/sdk-go,golang/protobuf) are appropriate transitive dependencies for the GRPC-based registration flow introduced in this PR.Also applies to: 57-57, 80-80
pkg/cmd/accept/exec.go (2)
31-33: GRPC service account and group constants added correctly.The new GRPC-specific constants align with the service account and namespace used by the GRPC server on the hub, enabling CSR approval for GRPC-based registrations.
147-157: CSR approval extended to support GRPC authentication.The approval logic now accepts CSRs from GRPC-based registrations by checking for the GRPC service account username prefix and group membership, complementing the existing bootstrap and service account checks. The group validation at lines 154-157 provides an additional authorization layer.
pkg/cmd/join/exec.go (4)
51-52: Good practice: GRPC import aliased to avoid conflicts.The
sdkgrpcalias prevents potential naming collisions withgoogle.golang.org/grpc.
57-58: Typo fix: OperatorNamesapce → OperatorNamespace.Corrects the constant name and updates all usages throughout the file (lines 421, 483, 576).
739-762: GRPC configuration marshaling and injection looks correct.The function properly:
- Returns early when GRPC auth is not selected
- Constructs GRPCConfig with CA data and credentials
- Marshals to YAML for chart injection
- Sets the RegistrationDriver to GRPC auth type
764-790: GRPC CA data retrieval is robust and well-validated.The implementation correctly:
- Reads from --grpc-ca-file if provided, with validation for empty files
- Falls back to fetching from the Hub ConfigMap
- Validates the ConfigMap key exists and contains non-empty data, returning clear errors otherwise
This aligns with the maintainer's requirement to return an error if CA data is missing or empty.
962a206 to
ed46075
Compare
Signed-off-by: Zhiwei Yin <[email protected]>
ed46075 to
c23a4b3
Compare
|
/lgtm |
0dc0a93
into
open-cluster-management-io:main
|
/cherry-pick release-1.1 |
|
@zhiweiyin318: new pull request created: #518 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Summary
the join command example :
clusteradm join --hub-token <token> --hub-apiserver <hub apiserver> --cluster-name cluster1 --registration-auth=grpc --grpc-server <grpc server>
the managed cluster and hub are the same Kind cluster:
clusteradm join --hub-token <token> --hub-apiserver https://127.0.0.1:35229 --cluster-name cluster1 --registration-auth=grpc --grpc-server cluster-manager-grpc-server.open-cluster-management-hub.svc:8090 --force-internal-endpoint-lookup
Related issue(s)
Fixes #
Summary by CodeRabbit