[WIP] ✨ grpc server #1053
[WIP] ✨ grpc server #1053
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: skeeey The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughThis update introduces a comprehensive gRPC-based CloudEvents control plane for cluster registration and resource synchronization. It adds new gRPC server and client drivers, integrates authentication and authorization using Kubernetes tokens and mTLS, and implements CloudEvents-based services for clusters, add-ons, CSRs, events, leases, and work resources. Extensive testing and RBAC adjustments are included. Changes
Sequence Diagram(s)sequenceDiagram
participant Operator
participant gRPC_Server
participant Authenticator
participant Authorizer
participant CloudEvents_Service
participant Kubernetes
Operator->>gRPC_Server: Start with CLI flags
gRPC_Server->>Authenticator: Authenticate (token/mTLS)
Authenticator-->>gRPC_Server: Auth result (user/groups)
gRPC_Server->>Authorizer: Authorize (SAR) for CloudEvents type
Authorizer-->>gRPC_Server: Authorization result
gRPC_Server->>CloudEvents_Service: Handle CloudEvent (e.g., cluster, addon, CSR, event, lease, work)
CloudEvents_Service->>Kubernetes: Get/List/Create/Update resource
Kubernetes-->>CloudEvents_Service: Resource result
CloudEvents_Service-->>gRPC_Server: CloudEvent response
gRPC_Server-->>Operator: Operation result
sequenceDiagram
participant ManagedCluster
participant RegistrationCLI
participant gRPC_Server
participant CloudEvents_Service
ManagedCluster->>RegistrationCLI: Start registration (gRPC mode)
RegistrationCLI->>gRPC_Server: Connect via gRPC
gRPC_Server->>CloudEvents_Service: Publish/Subscribe CloudEvents (CSR, cluster, etc.)
CloudEvents_Service->>gRPC_Server: Resource status/updates
gRPC_Server-->>RegistrationCLI: Registration result
RegistrationCLI-->>ManagedCluster: Registration complete
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1053 +/- ##
==========================================
- Coverage 64.03% 63.07% -0.96%
==========================================
Files 198 208 +10
Lines 19647 20546 +899
==========================================
+ Hits 12580 12960 +380
- Misses 6018 6478 +460
- Partials 1049 1108 +59
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🧹 Nitpick comments (31)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/interface.go (1)
9-11: Add documentation for the Authorizer interface.Consider adding godoc comments to document the expected behavior, error types, and usage patterns for the
Authorizemethod.+// Authorizer provides authorization decisions for CloudEvents operations. type Authorizer interface { + // Authorize determines whether the given cluster is authorized to perform + // the specified CloudEvents operation. Returns nil if authorized, or an + // error describing the authorization failure. Authorize(ctx context.Context, cluster string, eventsType types.CloudEventsType) error }vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/interface.go (1)
19-22: Consider making the helper function exported for external use.The
newContextWithIdentityfunction is currently unexported but provides useful functionality for enriching contexts with authentication data. Consider making it exported (e.g.,NewContextWithIdentity) so that external authentication implementations can reuse this functionality.-func newContextWithIdentity(ctx context.Context, user string, groups []string) context.Context { +func NewContextWithIdentity(ctx context.Context, user string, groups []string) context.Context { ctx = context.WithValue(ctx, ContextUserKey, user) return context.WithValue(ctx, ContextGroupsKey, groups) }vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/mtls.go (1)
38-41: Consider input sanitization for certificate fields.While the certificate fields (CommonName and Organization) are extracted directly from the validated certificate, consider adding basic sanitization or validation if these values will be used in security-sensitive contexts downstream.
user := tlsAuth.State.VerifiedChains[0][0].Subject.CommonName groups := tlsAuth.State.VerifiedChains[0][0].Subject.Organization + + // Basic validation - ensure user is not empty + if user == "" { + return ctx, status.Error(codes.Unauthenticated, "certificate must have a valid CommonName") + } + newCtx := newContextWithIdentity(ctx, user, groups) return newCtx, nilvendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/options.go (1)
44-58: Fix typo in flag descriptions.Multiple flag descriptions contain "gPRC" instead of "gRPC". This typo appears consistently across several flag descriptions.
- flags.StringVar(&o.ServerBindPort, "grpc-server-bindport", o.ServerBindPort, "gPRC server bind port") - flags.Uint32Var(&o.MaxConcurrentStreams, "grpc-max-concurrent-streams", o.MaxConcurrentStreams, "gPRC max concurrent streams") - flags.IntVar(&o.MaxReceiveMessageSize, "grpc-max-receive-message-size", o.MaxReceiveMessageSize, "gPRC max receive message size") - flags.IntVar(&o.MaxSendMessageSize, "grpc-max-send-message-size", o.MaxSendMessageSize, "gPRC max send message size") - flags.DurationVar(&o.ConnectionTimeout, "grpc-connection-timeout", o.ConnectionTimeout, "gPRC connection timeout") + flags.StringVar(&o.ServerBindPort, "grpc-server-bindport", o.ServerBindPort, "gRPC server bind port") + flags.Uint32Var(&o.MaxConcurrentStreams, "grpc-max-concurrent-streams", o.MaxConcurrentStreams, "gRPC max concurrent streams") + flags.IntVar(&o.MaxReceiveMessageSize, "grpc-max-receive-message-size", o.MaxReceiveMessageSize, "gRPC max receive message size") + flags.IntVar(&o.MaxSendMessageSize, "grpc-max-send-message-size", o.MaxSendMessageSize, "gRPC max send message size") + flags.DurationVar(&o.ConnectionTimeout, "grpc-connection-timeout", o.ConnectionTimeout, "gRPC connection timeout")Also apply the same fix to remaining flags on lines 54-55.
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/token.go (1)
38-38: Consider more robust token extraction.The current token extraction assumes the "Bearer " prefix but doesn't validate the token format. Consider adding basic validation to ensure the token is not empty after prefix removal.
token := strings.TrimPrefix(authorization[0], "Bearer ") + if token == "" || token == authorization[0] { + return ctx, status.Error(codes.Unauthenticated, "invalid token format") + }pkg/registration/register/factory/options.go (1)
40-42: Consider defining a constant for the "grpc" authentication typeThe hardcoded "grpc" string is used in multiple places (lines 40 and 51). Consider defining it as a constant to improve maintainability and reduce the risk of typos.
+const ( + GrpcAuthType = "grpc" +) + func (s *Options) Validate() error { switch s.RegistrationAuth { case helpers.AwsIrsaAuthType: return s.AWSISRAOption.Validate() - case "grpc": + case GrpcAuthType: return s.GRPCOption.Validate()Apply the same change to line 51 in the
Drivermethod.vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/wrapper.go (1)
49-55: Verify the type assertion pattern and document the behavior.The type assertion pattern is safe but the behavior should be documented more clearly regarding when nil is returned.
func (c *ClusterV1ClientWrapper) ManagedClusters() clusterv1client.ManagedClusterInterface { + // Return the client only if it's the expected concrete type if agentManagedClusterClient, ok := c.ManagedClusterClient.(*ManagedClusterClient); ok { return agentManagedClusterClient } - + // Return nil if the wrapped client is not the expected type return nil }vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/interface.go (1)
38-38: Consider creating an issue for the TODO item.The TODO comment suggests a planned SourceEventServer interface. Consider creating an issue to track this future enhancement.
Do you want me to create an issue to track the implementation of the SourceEventServer interface mentioned in the TODO comment?
pkg/registration/register/csr/csr.go (1)
462-462: Fix typo in event message.There's a grammatical error in the event message.
- recorder.Eventf("AdditionalSecretDataChanged", "The additional secret data is changed for %v. Re-create the client certificate for %s", err, controllerName) + recorder.Eventf("AdditionalSecretDataChanged", "The additional secret data changed: %v. Re-creating the client certificate for %s", err, controllerName)pkg/server/grpc/server.go (2)
3-36: Fix import formatting.The imports are not properly grouped according to Go conventions.
Apply this diff to fix the import grouping:
import ( "context" "time" "github.com/openshift/library-go/pkg/controller/controllercmd" "github.com/spf13/cobra" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" kubeinformers "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" "k8s.io/utils/clock" addonv1alpha1client "open-cluster-management.io/api/client/addon/clientset/versioned" addoninformers "open-cluster-management.io/api/client/addon/informers/externalversions" clusterv1client "open-cluster-management.io/api/client/cluster/clientset/versioned" clusterv1informers "open-cluster-management.io/api/client/cluster/informers/externalversions" workclientset "open-cluster-management.io/api/client/work/clientset/versioned" workinformers "open-cluster-management.io/api/client/work/informers/externalversions" clusterv1 "open-cluster-management.io/api/cluster/v1" - + commonoptions "open-cluster-management.io/ocm/pkg/common/options" + "open-cluster-management.io/ocm/pkg/server/services" + "open-cluster-management.io/ocm/pkg/version" addonce "open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon" "open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster" "open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr" eventce "open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event" leasece "open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease" "open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/payload" grpcauthn "open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn" grpcauthz "open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/kube" grpcoptions "open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options" - - commonoptions "open-cluster-management.io/ocm/pkg/common/options" - "open-cluster-management.io/ocm/pkg/server/services" - "open-cluster-management.io/ocm/pkg/version" )
123-137: Consider making the informer resync period configurable.The 30-minute resync period is hardcoded for all informers. Consider making this configurable via command-line flags for better operational flexibility.
Would you like me to generate the code to add a configurable resync period option?
pkg/server/services/work.go (2)
3-28: Fix import formatting.The imports should be properly grouped according to Go conventions.
Apply this diff to fix the import grouping:
import ( "context" "fmt" cloudevents "github.com/cloudevents/sdk-go/v2" cloudeventstypes "github.com/cloudevents/sdk-go/v2/types" - "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/labels" + kubetypes "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/cache" "k8s.io/klog/v2" - kubetypes "k8s.io/apimachinery/pkg/types" workclient "open-cluster-management.io/api/client/work/clientset/versioned" workinformers "open-cluster-management.io/api/client/work/informers/externalversions/work/v1" worklisters "open-cluster-management.io/api/client/work/listers/work/v1" workv1 "open-cluster-management.io/api/work/v1" - "open-cluster-management.io/sdk-go/pkg/cloudevents/clients/common" "open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/payload" "open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/source/codec" "open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types" "open-cluster-management.io/sdk-go/pkg/cloudevents/server" "open-cluster-management.io/sdk-go/pkg/patcher" )
180-193: Consider performance implications of UID-based lookup.The current implementation lists all ManifestWorks in a namespace and iterates through them to find by UID. This could become a performance bottleneck with a large number of works.
Consider adding a comment documenting this performance characteristic or explore using field selectors if UID-based queries become frequent.
pkg/server/services/csr.go (3)
57-57: Misleading comment about prefix matchingThe comment mentions "prefix matching" but the implementation uses exact label matching.
- // list csrs with prefix matching cluster name + // list csrs with cluster name label
76-76: Clarify or remove the question commentThis comment appears to be a question that should be resolved or converted to a proper TODO.
Either remove this comment or convert it to a proper TODO with context about what needs to be investigated.
88-95: Consider supporting CSR update operationsThe service only supports create operations for CSRs. Consider whether update operations should be supported for CSR approval/denial status updates.
Would you like me to help implement support for update operations?
pkg/server/services/cluster.go (1)
55-55: Fix comment formatting- //only get single cluster + // only get single clusterpkg/server/services/lease.go (1)
3-16: Fix import formattingRemove the empty line between imports to follow Go conventions.
cloudevents "github.com/cloudevents/sdk-go/v2" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"pkg/server/services/event.go (2)
3-13: Fix import formattingRemove the empty line between imports.
cloudevents "github.com/cloudevents/sdk-go/v2" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
81-81: Remove redundant return statementfunc (e EventService) RegisterHandler(handler server.EventHandler) { - return }vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/client.go (3)
64-64: Address the TODO comment for CSR validation.The validation step is currently skipped but is crucial for security. CSR validation should include checking the certificate request format, subject fields, and ensuring it meets security requirements.
Would you like me to help implement the CSR validation logic? I can provide a secure validation implementation that checks common security requirements.
45-48: Consider more robust name generation logic.The current name generation concatenates
GenerateNamewith a 5-character random string. This approach could potentially lead to name collisions in high-throughput scenarios.Consider using a more robust approach:
-if csr.Name == "" && csr.GenerateName != "" { - csr.Name = csr.GenerateName + utilrand.String(5) - csr.GenerateName = "" -} +if csr.Name == "" && csr.GenerateName != "" { + csr.Name = csr.GenerateName + utilrand.String(8) // Longer random suffix + csr.GenerateName = "" +}
32-41: Unused clusterName parameter in constructor.The
clusterNameparameter is accepted but never used in theNewCSRClientfunction.Either utilize the parameter or remove it:
func NewCSRClient( cloudEventsClient *generic.CloudEventAgentClient[*certificatev1.CertificateSigningRequest], watcherStore store.ClientWatcherStore[*certificatev1.CertificateSigningRequest], - clusterName string, ) *CSRClient {pkg/server/services/addon.go (2)
54-54: Fix comment formatting.Add a space between
//and the comment text as suggested by the static analysis tool.- //only get single cluster + // only get single cluster
70-70: Clarify or remove the unclear comment.The comment "q if there is resourceVersion, this will return directly to the agent as conflict?" is unclear and seems incomplete.
Please clarify this comment or remove it if it's not relevant to the current implementation.
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/wrapper.go (1)
24-24: Consider more graceful error handling instead of panic.While panic is clear about unsupported operations, returning proper errors would be more robust and easier to handle by callers.
Consider returning errors instead of panicking:
func (a AddonClientSetWrapper) Discovery() discovery.DiscoveryInterface { - panic("Discovery is unsupported") + return nil // or implement a stub that returns ErrNotSupported } func (c *AddonV1Alpha1ClientWrapper) AddOnDeploymentConfigs(namespace string) addonv1alpha1client.AddOnDeploymentConfigInterface { - panic("AddOnDeploymentConfigs is unsupported") + return nil // or implement a stub that returns ErrNotSupported for all operations }However, if panic is the intended behavior to catch programming errors early, the current approach is acceptable.
Also applies to: 39-39, 43-43, 47-47, 51-51
pkg/registration/register/grpc/spoke_driver.go (1)
63-196: Consider refactoring the BuildClients methodThe
BuildClientsmethod is quite long (130+ lines) and handles multiple responsibilities. Consider breaking it down into smaller, focused methods for better maintainability.Consider extracting the following helper methods:
loadConfig(bootstrapped bool) (any, string, error)- for config loading logiccreateClusterClient(ctx, config, secretOption) (*clusterClient, *clusterInformer, error)createLeaseClient(ctx, config, secretOption) (*leaseClient, error)createEventClient(ctx, config, secretOption) (*eventClient, error)createAddonClient(ctx, config, secretOption) (*addonClient, *addonInformer, error)createCSRControl(ctx, config, secretOption) (*csrControl, error)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.go (1)
60-60: Address TODO: Add ManagedCluster validationThe TODO comment indicates missing validation for the ManagedCluster resource.
Would you like me to implement the validation logic or create an issue to track this task?
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/server.go (1)
86-90: Consider making TLS version configurableThe server is hardcoded to use only TLS 1.3, which might cause compatibility issues with older clients.
Consider making the TLS version configurable:
+ minTLSVersion := tls.VersionTLS12 + if s.options.MinTLSVersion != 0 { + minTLSVersion = s.options.MinTLSVersion + } tlsConfig := &tls.Config{ Certificates: []tls.Certificate{serverCerts}, - MinVersion: tls.VersionTLS13, - MaxVersion: tls.VersionTLS13, + MinVersion: minTLSVersion, }vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go (1)
276-276: Use structured logging instead of printf-style formatting.The logging statement uses printf-style formatting with structured logging API, which will produce incorrect output.
- log.V(4).Info("ignore the obj %v since it has a invalid resourceVersion, %v", obj, err) + log.V(4).Info("ignore the obj since it has an invalid resourceVersion", "obj", obj, "error", err)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.go (1)
69-80: Fix error message format for consistency.The current implementation concatenates namespace and name for the NotFound error, which may not align with standard Kubernetes error formats.
if !exists { - return nil, errors.NewNotFound(common.ManagedClusterAddOnGR, c.namespace+"/"+name) + return nil, errors.NewNotFound(common.ManagedClusterAddOnGR, name) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (48)
cmd/registration/main.go(2 hunks)go.mod(3 hunks)pkg/registration/hub/manager.go(4 hunks)pkg/registration/hub/manifests/rbac/managedcluster-registration-clusterrole.yaml(2 hunks)pkg/registration/hub/manifests/rbac/managedcluster-work-clusterrole.yaml(1 hunks)pkg/registration/register/csr/csr.go(7 hunks)pkg/registration/register/factory/options.go(3 hunks)pkg/registration/register/grpc/hub_driver.go(1 hunks)pkg/registration/register/grpc/options.go(1 hunks)pkg/registration/register/grpc/spoke_driver.go(1 hunks)pkg/server/grpc/server.go(1 hunks)pkg/server/services/addon.go(1 hunks)pkg/server/services/cluster.go(1 hunks)pkg/server/services/csr.go(1 hunks)pkg/server/services/event.go(1 hunks)pkg/server/services/lease.go(1 hunks)pkg/server/services/work.go(1 hunks)vendor/modules.txt(3 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/wrapper.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/clientholder.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/wrapper.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/common/common.go(0 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/clientholder.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/wrapper.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/client/manifestwork.go(2 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/source/client/manifestwork.go(3 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.go(2 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/interface.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/mtls.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/token.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/interface.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/kube/sar.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/options.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/server.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/interface.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/store.go(1 hunks)
💤 Files with no reviewable changes (1)
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/common/common.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
pkg/server/grpc/server.go
9-9: File is not properly formatted
(gci)
pkg/server/services/work.go
155-155: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandler is not checked
(errcheck)
9-9: File is not properly formatted
(gci)
pkg/server/services/event.go
8-8: File is not properly formatted
(gci)
81-81: S1023: redundant return statement
(gosimple)
pkg/server/services/cluster.go
101-101: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandler is not checked
(errcheck)
55-55: commentFormatting: put a space between // and comment text
(gocritic)
pkg/server/services/csr.go
98-98: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandler is not checked
(errcheck)
pkg/server/services/addon.go
98-98: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandler is not checked
(errcheck)
54-54: commentFormatting: put a space between // and comment text
(gocritic)
pkg/server/services/lease.go
88-88: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandler is not checked
(errcheck)
8-8: File is not properly formatted
(gci)
🔇 Additional comments (63)
go.mod (1)
11-11: ```shell
#!/bin/bashInspect the version of golang.org/x/net in go.mod
echo "golang.org/x/net entries in go.mod:"
grep -n "golang.org/x/net" go.mod || echo "No direct golang.org/x/net dependency found."Check for security advisories on cloudevents/sdk-go
echo -e "\nSecurity advisories for github.com/cloudevents/sdk-go:"
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/cloudevents/sdk-go") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'</details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/client/manifestwork.go (1)</summary> `166-166`: **LGTM: Correct constant migration for status updates.** The migration from `common.UpdateRequestAction` to `types.UpdateRequestAction` is correct for status patching operations. </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.go (2)</summary> `10-11`: **LGTM: Adding necessary imports for gRPC status handling.** The imports are required for the improved error handling in the subscription client. --- `128-133`: **Excellent error handling improvement for gRPC cancellation.** The code now properly distinguishes between normal context cancellation and actual errors. This prevents unnecessary error logs when gRPC streams are canceled as part of normal shutdown procedures. </details> <details> <summary>pkg/registration/hub/manifests/rbac/managedcluster-work-clusterrole.yaml (1)</summary> `16-16`: **LGTM: Appropriate RBAC expansion for gRPC event handling.** Adding "list" and "watch" verbs for events aligns with the new gRPC server's event handling requirements while maintaining appropriate security boundaries. </details> <details> <summary>cmd/registration/main.go (2)</summary> `20-20`: **LGTM: Adding necessary import for gRPC server command.** The import follows the established pattern for other command packages in the CLI. --- `66-66`: **LGTM: Proper integration of gRPC server subcommand.** The gRPC server command is added consistently with the existing hub, spoke, and webhook commands, providing a clean CLI interface for the new functionality. </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/source/client/manifestwork.go (3)</summary> `80-84`: **LGTM! Consistent constant refactoring.** The update from `common.CreateRequestAction` to `types.CreateRequestAction` aligns with the broader refactoring to move CloudEvents action constants to the types package. --- `141-145`: **LGTM! Consistent constant refactoring.** The update from `common.DeleteRequestAction` to `types.DeleteRequestAction` maintains consistency with the package refactoring. --- `265-269`: **LGTM! Consistent constant refactoring.** The update from `common.UpdateRequestAction` to `types.UpdateRequestAction` completes the consistent refactoring of CloudEvents action constants. </details> <details> <summary>pkg/registration/hub/manifests/rbac/managedcluster-registration-clusterrole.yaml (2)</summary> `14-19`: **Consider scoping down the lease permissions.** Adding "list" and "watch" permissions to all leases in the cluster may be overly broad. If the gRPC driver only needs to monitor specific leases (e.g., managed cluster leases), consider using resourceNames to restrict access to only the necessary resources. --- `29-31`: **Consider scoping down the events permissions.** Adding "list" and "watch" permissions to all events in the cluster may grant broader access than necessary. Consider whether these permissions can be scoped to specific namespaces or event types required by the gRPC functionality. </details> <details> <summary>vendor/modules.txt (1)</summary> `1789-1837`: **LGTM! Module update supports new gRPC functionality.** The SDK module version update and addition of gRPC server-related packages aligns with the introduction of the new gRPC server capabilities in this PR. </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/store.go (1)</summary> `9-25`: **LGTM! Well-designed Service interface.** The Service interface provides a clean abstraction for CloudEvents backend operations. The TODO comment appropriately notes a future enhancement for event deduplication, which is good planning for production use. </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/interface.go (1)</summary> `5-12`: **Well-designed context key pattern.** Good use of a custom type for context keys to avoid collisions, following the recommended pattern from the Go documentation. The constants are appropriately defined and will help maintain consistency across the authentication system. </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/mtls.go (1)</summary> `30-36`: **Robust certificate chain validation.** Good defensive programming with multiple checks for certificate chain validity. The nested checks ensure that both the verified chains exist and contain valid certificates before proceeding with authentication. </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/options.go (1)</summary> `27-41`: **Well-structured options with sensible defaults.** Good separation of concerns with a dedicated options struct and constructor. Most default values are appropriate for typical gRPC server configurations, with reasonable timeouts and buffer sizes. </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/clientholder.go (2)</summary> `37-39`: **Appropriate informer configuration.** The 30-second resync period and namespace indexing are reasonable defaults for CSR resources. The configuration supports efficient lookups and regular synchronization. --- `14-26`: **Clean encapsulation design.** Good use of the holder pattern to encapsulate related components (client and informer). The accessor methods provide clear, type-safe access to the encapsulated resources. </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/token.go (2)</summary> `19-19`: **Good interface compliance verification.** Excellent use of a compile-time interface compliance check. This ensures that `TokenAuthenticator` properly implements the `Authenticator` interface and will catch interface changes at compile time. --- `25-54`: **Secure token authentication implementation.** The implementation correctly uses the Kubernetes TokenReview API for token validation and properly extracts user identity and groups from the response. The error handling covers the main failure scenarios with appropriate gRPC status codes. </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/codec.go (1)</summary> `1-69`: **LGTM: Well-implemented CloudEvents codec for ManagedCluster** The codec implementation follows CloudEvents best practices with proper: - Event type validation in the `Encode` method - Deep copying of resources before modification to avoid side effects - Proper TypeMeta setting for Kubernetes API compatibility - Clean error handling and JSON marshaling </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/clientholder.go (1)</summary> `1-47`: **LGTM: Clean adapter pattern implementation** The `ClientHolder` correctly implements the adapter pattern to bridge CloudEvents-based clients with standard Kubernetes client interfaces. The implementation properly: - Delegates to wrapped client implementations - Handles error cases in the constructor - Maintains proper interface contracts </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/wrapper.go (2)</summary> `17-19`: **LGTM: Appropriate handling of unsupported operation** The panic in `RESTClient()` is the correct approach for operations that are intentionally unsupported in this wrapper implementation. This follows the established pattern for CloudEvents-based client wrappers. --- `27-38`: **LGTM: Clean wrapper implementation** The constructor properly initializes the CloudEvents client and wraps it in the appropriate interface. Error handling and client initialization follow the established patterns. </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go (1)</summary> `40-54`: **LGTM: Well-designed event action constants** The comment correction and new `EventAction` constants are well-implemented: - Consistent naming convention with existing constants - Covers essential CRUD operations (create, update, delete) plus watch - Follows the established underscore-separated value format - Improves API completeness for resource event handling </details> <details> <summary>pkg/registration/hub/manager.go (3)</summary> `49-49`: **LGTM! Clean import addition.** The gRPC package import is correctly added to support the new registration driver. --- `63-64`: **LGTM! Appropriate field additions for gRPC CA configuration.** The new fields for CA file and key file paths are well-named and follow the existing naming conventions. --- `99-102`: **LGTM! Clear and descriptive CLI flags.** The flag descriptions are clear and follow the existing pattern for configuration options. </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/codec.go (4)</summary> `16-20`: **LGTM! Well-defined CloudEvents data type constant.** The data type constant correctly uses the Kubernetes certificates API group and version. --- `25-32`: **LGTM! Simple and correct codec constructor and accessor.** The constructor and EventDataType method follow standard patterns. --- `35-47`: **LGTM! Proper validation of required fields.** The validation ensures both event type compatibility and required cluster name label presence with clear error messages. --- `72-79`: **LGTM! Clean decoding implementation with proper error handling.** The decoding method correctly unmarshals the CloudEvent data into a CSR object with appropriate error messaging. </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/wrapper.go (2)</summary> `14-18`: **LGTM! Well-documented wrapper struct with clear purpose.** The comment clearly explains the wrapper's purpose for building informer factories. --- `26-40`: **LGTM! Consistent nil returns for unsupported interfaces.** Returning nil for unsupported client interfaces is the correct pattern for partial interface implementations. </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/interface.go (2)</summary> `11-25`: **LGTM! Well-designed AgentEventServer interface.** The interface composition and method signatures are clean and follow Go conventions. The service registration by data type is a good design pattern. --- `27-36`: **LGTM! Consistent EventHandler interface design.** The CRUD operation callbacks are well-named and consistently use context and typed parameters. </details> <details> <summary>pkg/registration/register/csr/csr.go (7)</summary> `19-19`: **LGTM! Appropriate import for semantic equality.** The addition of the equality package supports the improved secret data comparison logic. --- `83-83`: **LGTM! Enhanced structured logging.** The structured logging with key-value pairs improves observability and debugging capabilities. --- `236-237`: **LGTM! Consistent logging enhancement.** The additional logging for CSR name setting maintains consistency with the logging improvements. --- `301-303`: **LGTM! Proper default signer name handling.** Setting a default signer name when none is provided follows good defensive programming practices. --- `357-362`: **LGTM! Good refactoring to extract CSR control setup.** The delegation to SetCSRControl improves modularity and reusability of the CSR control setup logic. --- `364-381`: **LGTM! Well-designed CSR control setup method.** The extracted method properly handles indexer setup and halt function assignment with appropriate error handling. --- `496-496`: **LGTM! Improved data comparison with semantic equality.** Using semantic equality instead of basic comparison provides more robust comparison for byte slices and handles edge cases better. </details> <details> <summary>pkg/server/grpc/server.go (1)</summary> `38-88`: **Well-structured gRPC server implementation!** The command setup properly initializes all necessary components with appropriate authentication, authorization, and service registration. </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/codec.go (1)</summary> `1-68`: **LGTM!** The lease codec implementation is correct and follows the standard pattern. </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/codec.go (1)</summary> `1-67`: **LGTM!** The addon codec implementation is correct and follows the established pattern. </details> <details> <summary>pkg/server/services/work.go (1)</summary> `154-178`: **Consider error handling for AddEventHandler.** While it's common practice to ignore the error from `AddEventHandler` as it rarely fails, the static analysis tool has flagged this. The current approach of logging errors within the handlers is acceptable. </details> <details> <summary>pkg/registration/register/grpc/hub_driver.go (1)</summary> `74-117`: **Well-structured controller implementation** The CSR signing controller is well-implemented with proper filtering, event handling, and factory pattern usage. </details> <details> <summary>pkg/server/services/cluster.go (2)</summary> `21-37`: **Good implementation of source constant and service structure** The service properly defines the source constant and has a well-structured constructor with codec initialization. --- `53-67`: **List method returns only a single cluster** The List method implementation only returns a single cluster by name, which doesn't align with typical list semantics that return multiple items. Is this intentional design? Consider either: 1. Renaming the method to better reflect its behavior 2. Implementing proper list functionality with filtering support </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/client.go (1)</summary> `86-98`: **LGTM! Clean list implementation.** The list method properly handles the conversion from the watcher store format to the standard Kubernetes list format with appropriate error handling. </details> <details> <summary>pkg/registration/register/grpc/options.go (2)</summary> `49-63`: **LGTM! Proper CSR approval status checking.** The approval status logic correctly handles both denied and approved conditions, following Kubernetes CSR patterns. --- `73-95`: **LGTM! Well-structured CSR creation.** The CSR creation method properly sets required fields including client authentication usage and handles event recording appropriately. </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/kube/sar.go (2)</summary> `78-122`: **LGTM! Comprehensive resource mapping for authorization.** The resource mapping correctly handles all supported resource types and properly maps cloud event actions to Kubernetes verbs. The special handling for manifest works and subresources is appropriate. --- `33-54`: **LGTM! Proper SAR-based authorization flow.** The authorization logic correctly extracts user info, creates SAR requests, and properly handles authorization responses with detailed error messages. </details> <details> <summary>vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/wrapper.go (2)</summary> `59-68`: **LGTM! Clean wrapper construction.** The `ManagedClusterAddOnInterface` function properly constructs the wrapper hierarchy with appropriate error propagation from the cloud events client creation. --- `54-56`: **LGTM! Proper delegation to the wrapped client.** The `ManagedClusterAddOns` method correctly delegates to the underlying client's `Namespace` method, maintaining the expected interface contract. </details> <details> <summary>pkg/registration/register/grpc/spoke_driver.go (1)</summary> `52-52`: ```shell #!/bin/bash # Find the definition of Option across the repository rg -n "type Option" -C3 --type go # Find the definition of SecretOption in the register package rg -n "type SecretOption" -C3 pkg/registration/registervendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/client.go (1)
1-126: LGTM!The EventClient implementation follows the expected pattern for a CloudEvents-based client with appropriate error handling and limited operation support.
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.go (1)
1-160: LGTM!The ManagedClusterClient implementation correctly handles CloudEvents-based operations with appropriate restrictions on supported methods.
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/client.go (1)
1-99: LGTM!The LeaseClient implementation correctly provides minimal lease management functionality through CloudEvents with appropriate method restrictions.
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.go (1)
107-143: Well-implemented status patch handling.The patch method correctly validates that only status subresource updates are allowed and properly publishes the update event via CloudEvents.
| @@ -234,7 +234,7 @@ func (c *ManifestWorkAgentClient) Patch(ctx context.Context, name string, pt kub | |||
| Message: fmt.Sprintf("The manifests are deleted from the cluster %s", newWork.Namespace), | |||
| }) | |||
|
|
|||
| eventType.Action = common.DeleteRequestAction | |||
| eventType.Action = types.UpdateRequestAction | |||
There was a problem hiding this comment.
Fix incorrect action type for deletion event.
The code is handling manifest work deletion (setting ResourceDeleted condition and calling watcherStore.Delete()), but it's using types.UpdateRequestAction instead of types.DeleteRequestAction.
- eventType.Action = types.UpdateRequestAction
+ eventType.Action = types.DeleteRequestAction📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| eventType.Action = types.UpdateRequestAction | |
| - eventType.Action = types.UpdateRequestAction | |
| + eventType.Action = types.DeleteRequestAction |
🤖 Prompt for AI Agents
In
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/client/manifestwork.go
at line 237, the action type for a deletion event is incorrectly set to
types.UpdateRequestAction. Change this to types.DeleteRequestAction to correctly
represent the deletion operation when setting the eventType.Action.
| MaxConcurrentStreams: math.MaxUint32, | ||
| MaxReceiveMessageSize: 1024 * 1024 * 4, | ||
| MaxSendMessageSize: math.MaxInt32, | ||
| ConnectionTimeout: 120 * time.Second, | ||
| MaxConnectionAge: time.Duration(math.MaxInt64), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider extreme default values for production safety.
Using math.MaxUint32 and math.MaxInt32 as defaults for concurrent streams and message sizes could lead to resource exhaustion in production environments. Consider more conservative defaults that can be increased as needed.
- MaxConcurrentStreams: math.MaxUint32,
+ MaxConcurrentStreams: 1000, // Reasonable default, can be increased if needed
- MaxSendMessageSize: math.MaxInt32,
+ MaxSendMessageSize: 1024 * 1024 * 16, // 16MB, matching common gRPC defaults📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| MaxConcurrentStreams: math.MaxUint32, | |
| MaxReceiveMessageSize: 1024 * 1024 * 4, | |
| MaxSendMessageSize: math.MaxInt32, | |
| ConnectionTimeout: 120 * time.Second, | |
| MaxConnectionAge: time.Duration(math.MaxInt64), | |
| MaxConcurrentStreams: 1000, // Reasonable default, can be increased if needed | |
| MaxReceiveMessageSize: 1024 * 1024 * 4, | |
| MaxSendMessageSize: 1024 * 1024 * 16, // 16MB, matching common gRPC defaults | |
| ConnectionTimeout: 120 * time.Second, | |
| MaxConnectionAge: time.Duration(math.MaxInt64), |
🤖 Prompt for AI Agents
In
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/options.go
around lines 30 to 34, the default values for MaxConcurrentStreams and
MaxSendMessageSize are set to extremely high limits (math.MaxUint32 and
math.MaxInt32), which can cause resource exhaustion in production. Adjust these
defaults to more conservative, reasonable limits that balance performance and
safety, allowing them to be increased explicitly if needed.
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/clientholder.go
Outdated
Show resolved
Hide resolved
| if err != nil { | ||
| return ctx, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for TokenReview API calls.
The current implementation passes through the raw error from the TokenReview API call, which might expose internal details. Consider wrapping the error with an appropriate gRPC status.
if err != nil {
- return ctx, err
+ return ctx, status.Error(codes.Internal, "failed to validate token")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil { | |
| return ctx, err | |
| } | |
| if err != nil { | |
| return ctx, status.Error(codes.Internal, "failed to validate token") | |
| } |
🤖 Prompt for AI Agents
In
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/token.go
around lines 44 to 46, the error returned from the TokenReview API call is
passed through directly, potentially exposing internal details. Modify the error
handling to wrap the original error using an appropriate gRPC status error, such
as using status.Errorf with a relevant gRPC code, to provide a cleaner and more
controlled error message.
pkg/registration/hub/manager.go
Outdated
| case "grpc": | ||
| grpcHubDriver, err := grpc.NewGRPCHubDriver( | ||
| kubeClient, kubeInformers, m.GRPCCAKeyFile, m.GRPCCAFile, 720*time.Hour, controllerContext.EventRecorder) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| drivers = append(drivers, grpcHubDriver) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Verification correct
Consider using a constant for the driver type and verify CA file requirements.
The hardcoded "grpc" string should be replaced with a constant for consistency. Also verify that proper validation exists for the CA files.
- case "grpc":
+ case commonhelpers.GRPCAuthType:Let me verify if the constant exists and check CA file validation:
🏁 Script executed:
#!/bin/bash
# Check if GRPCAuthType constant exists in commonhelpers
rg -A 2 -B 2 "GRPCAuthType"
# Check for CA file validation in grpc package
rg -A 10 -B 5 "NewGRPCHubDriver"Length of output: 2199
Consider using a constant for the driver type and verify CA file requirements.
The hardcoded "grpc" string should be replaced with a constant for consistency. Also verify that proper validation exists for the CA files.
- case "grpc":
+ case commonhelpers.GRPCAuthType:Let me verify if the constant exists and check CA file validation:
#!/bin/bash
# Check if GRPCAuthType constant exists in commonhelpers
rg -A 2 -B 2 "GRPCAuthType"
# Check for CA file validation in grpc package
rg -A 10 -B 5 "NewGRPCHubDriver"🤖 Prompt for AI Agents
In pkg/registration/hub/manager.go around lines 205 to 211, replace the
hardcoded "grpc" string with the appropriate constant defined for the driver
type to ensure consistency. Additionally, review the grpc.NewGRPCHubDriver
function to confirm that it properly validates the CA key and CA certificate
files; if validation is missing or insufficient, add checks to verify the
presence and correctness of these files before proceeding.
| var err error | ||
| for _, authenticator := range authenticators { | ||
| ctx, err = authenticator.Authenticate(ctx) | ||
| if err == nil { | ||
| return handler(ctx, req) | ||
| } | ||
| } | ||
|
|
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return handler(ctx, req) | ||
| } |
There was a problem hiding this comment.
Improve error handling in authentication interceptor
The current implementation might return a nil error if no authenticators are configured, allowing unauthenticated requests.
Apply this diff to ensure authentication is required:
var err error
for _, authenticator := range authenticators {
ctx, err = authenticator.Authenticate(ctx)
if err == nil {
return handler(ctx, req)
}
}
- if err != nil {
- return nil, err
- }
-
- return handler(ctx, req)
+ if err == nil && len(authenticators) == 0 {
+ return nil, fmt.Errorf("no authenticators configured")
+ }
+ return nil, err📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var err error | |
| for _, authenticator := range authenticators { | |
| ctx, err = authenticator.Authenticate(ctx) | |
| if err == nil { | |
| return handler(ctx, req) | |
| } | |
| } | |
| if err != nil { | |
| return nil, err | |
| } | |
| return handler(ctx, req) | |
| } | |
| var err error | |
| for _, authenticator := range authenticators { | |
| ctx, err = authenticator.Authenticate(ctx) | |
| if err == nil { | |
| return handler(ctx, req) | |
| } | |
| } | |
| if err == nil && len(authenticators) == 0 { | |
| return nil, fmt.Errorf("no authenticators configured") | |
| } | |
| return nil, err | |
| } |
🤖 Prompt for AI Agents
In
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/server.go
around lines 142 to 155, the authentication interceptor may allow
unauthenticated requests if no authenticators are configured because it returns
nil error by default. Modify the code to check if any authenticators exist and
if none succeed, return an explicit authentication error instead of nil. This
ensures that authentication is always required and unauthenticated requests are
rejected properly.
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go
Outdated
Show resolved
Hide resolved
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go
Outdated
Show resolved
Hide resolved
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
pkg/server/services/event.go (2)
85-89: Missing codec initialization will cause nil pointer dereference.The codec field is not initialized in the constructor, which will cause nil pointer dereferences when encoding/decoding events.
79-81: RegisterHandler has empty implementation.The RegisterHandler method is empty, which means event changes won't be propagated to handlers.
pkg/server/services/lease.go (1)
104-110: Missing codec initialization will cause nil pointer dereference.The codec field is not initialized in the constructor, which will cause nil pointer dereferences when encoding/decoding lease objects.
🧹 Nitpick comments (2)
pkg/server/services/work.go (1)
153-153: Consider handling AddEventHandler error return value.The static analysis correctly identifies that
AddEventHandlerreturns an error that should be checked.- w.workInformer.Informer().AddEventHandler(&cache.ResourceEventHandlerFuncs{ + if err := w.workInformer.Informer().AddEventHandler(&cache.ResourceEventHandlerFuncs{However, since this is typically called during initialization and failures are rare, the current approach may be acceptable depending on your error handling strategy.
pkg/server/services/lease.go (1)
87-87: Consider handling AddEventHandler error return value.The static analysis correctly identifies that
AddEventHandlerreturns an error that should be checked, similar to the WorkService.- l.informer.Informer().AddEventHandler(&cache.ResourceEventHandlerFuncs{ + if err := l.informer.Informer().AddEventHandler(&cache.ResourceEventHandlerFuncs{
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/registration/register/grpc/spoke_driver.go(1 hunks)pkg/server/grpc/server.go(1 hunks)pkg/server/services/event.go(1 hunks)pkg/server/services/lease.go(1 hunks)pkg/server/services/work.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/server/grpc/server.go
- pkg/registration/register/grpc/spoke_driver.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
pkg/server/services/event.go
80-80: S1023: redundant return statement
(gosimple)
pkg/server/services/lease.go
87-87: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandler is not checked
(errcheck)
pkg/server/services/work.go
153-153: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandler is not checked
(errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e-singleton
- GitHub Check: e2e-hosted
- GitHub Check: e2e
- GitHub Check: verify
- GitHub Check: integration
- GitHub Check: unit
🔇 Additional comments (1)
pkg/server/services/work.go (1)
37-47: LGTM: Well-structured service implementation.The WorkService implementation follows good patterns with proper error handling, resource management, and CloudEvents integration. The status update logic with finalizer handling is particularly well-implemented.
Also applies to: 86-150, 178-191
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go (3)
102-142: Consider refactoring the Publish method to improve maintainability.The
Publishmethod handles multiple responsibilities (protobuf conversion, event type parsing, resync handling, and status updates) in a single 40-line function. This makes it harder to test and maintain.Consider extracting the resync handling logic into a separate method:
func (bkr *GRPCBroker) Publish(ctx context.Context, pubReq *pbv1.PublishRequest) (*emptypb.Empty, error) { logger := klog.FromContext(ctx) // WARNING: don't use "evt, err := pb.FromProto(pubReq.Event)" to convert protobuf to cloudevent evt, err := binding.ToEvent(ctx, grpcprotocol.NewMessage(pubReq.Event)) if err != nil { return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("failed to convert protobuf to cloudevent: %v", err)) } eventType, err := types.ParseCloudEventsType(evt.Type()) if err != nil { return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("failed to parse cloud event type %s, %v", evt.Type(), err)) } logger.V(4).Info("receive the event with grpc broker", "event", evt.Context) // handler resync request if eventType.Action == types.ResyncRequestAction { - err := bkr.respondResyncSpecRequest(ctx, eventType.CloudEventsDataType, evt) - if err != nil { - return nil, status.Error(codes.FailedPrecondition, fmt.Sprintf("failed to respond resync spec request: %v", err)) - } - return &emptypb.Empty{}, nil + return bkr.handleResyncRequest(ctx, eventType.CloudEventsDataType, evt) } + return bkr.handleStatusUpdate(ctx, eventType, evt) +} +func (bkr *GRPCBroker) handleResyncRequest(ctx context.Context, dataType types.CloudEventsDataType, evt *cloudevents.Event) (*emptypb.Empty, error) { + err := bkr.respondResyncSpecRequest(ctx, dataType, evt) + if err != nil { + return nil, status.Error(codes.FailedPrecondition, fmt.Sprintf("failed to respond resync spec request: %v", err)) + } + return &emptypb.Empty{}, nil +} +func (bkr *GRPCBroker) handleStatusUpdate(ctx context.Context, eventType *types.CloudEventsType, evt *cloudevents.Event) (*emptypb.Empty, error) { service, ok := bkr.services[eventType.CloudEventsDataType] if !ok { return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("failed to find service for event type %s", eventType.CloudEventsDataType)) } // handle the resource status update according status update type if err := service.HandleStatusUpdate(ctx, evt); err != nil { errStr, marshalErr := json.Marshal(err) if marshalErr != nil { return nil, status.Error(codes.FailedPrecondition, err.Error()) } return nil, status.Error(codes.FailedPrecondition, string(errStr)) } return &emptypb.Empty{}, nil +}
262-262: Fix grammatical error in log message.The log message contains a grammatical error: "there are is no objs" should be "there are no objs".
- log.V(4).Info("there are is no objs from the list, do nothing") + log.V(4).Info("there are no objs from the list, do nothing")
180-227: Improve error handling specificity in the Subscribe method.The current implementation treats all errors from the
handleras potential reasons to unregister a subscriber. However, only certain types of errors (like connection failures) should trigger unregistration, while others might be transient and recoverable.Consider implementing more granular error handling:
func (bkr *GRPCBroker) Subscribe(subReq *pbv1.SubscriptionRequest, subServer pbv1.CloudEventService_SubscribeServer) error { // ... existing code ... subscriberID, errChan := bkr.register(subReq.ClusterName, *dataType, func(evt *cloudevents.Event) error { // ... existing protobuf conversion code ... // send the cloudevent to the subscriber klog.V(4).Infof("sending the event to spec subscribers, %s", evt.Context) if err := subServer.Send(pbEvt); err != nil { klog.Errorf("failed to send grpc event, %v", err) - // Return the error without wrapping, as it includes the gRPC error code and message for further handling. - // For unrecoverable errors, such as a connection closed by an intermediate proxy, push the error to subscriber's - // error channel to unregister the subscriber. + // Check if this is an unrecoverable error that should trigger unregistration + if st, ok := status.FromError(err); ok { + switch st.Code() { + case codes.Unavailable, codes.Canceled, codes.DeadlineExceeded: + // These are typically unrecoverable connection issues + select { + case errChan <- err: + default: + // Channel might be closed, ignore + } + } + } return err } return nil }) select { case err := <-errChan: // When reaching this point, an unrecoverable error occurred while sending the event, // such as the connection being closed. Unregister the subscriber to trigger agent reconnection. klog.Errorf("unregister subscriber %s, error= %v", subscriberID, err) bkr.unregister(subscriberID) return err case <-subServer.Context().Done(): // ... existing code ... } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
go.mod(3 hunks)pkg/common/helpers/constants.go(1 hunks)pkg/registration/hub/manager.go(4 hunks)pkg/registration/register/grpc/hub_driver.go(1 hunks)pkg/registration/register/grpc/options.go(1 hunks)pkg/registration/register/grpc/spoke_driver.go(1 hunks)pkg/server/services/addon.go(1 hunks)pkg/server/services/cluster.go(1 hunks)pkg/server/services/csr.go(1 hunks)pkg/server/services/event.go(1 hunks)pkg/server/services/lease.go(1 hunks)pkg/server/services/work.go(1 hunks)vendor/modules.txt(3 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/clientholder.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/kube/sar.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/common/helpers/constants.go
🚧 Files skipped from review as they are similar to previous changes (16)
- go.mod
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/clientholder.go
- vendor/modules.txt
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/codec.go
- pkg/server/services/lease.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/kube/sar.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/codec.go
- pkg/server/services/event.go
- pkg/registration/register/grpc/spoke_driver.go
- pkg/server/services/cluster.go
- pkg/registration/register/grpc/hub_driver.go
- pkg/server/services/addon.go
- pkg/server/services/csr.go
- pkg/registration/hub/manager.go
- pkg/server/services/work.go
- pkg/registration/register/grpc/options.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: integration
- GitHub Check: e2e-singleton
- GitHub Check: e2e
- GitHub Check: e2e-hosted
| func (bkr *GRPCBroker) OnCreate(ctx context.Context, t types.CloudEventsDataType, id string) error { | ||
| service, ok := bkr.services[t] | ||
| if !ok { | ||
| return fmt.Errorf("failed to find service for event type %s", t) | ||
| } | ||
|
|
||
| resource, err := service.Get(ctx, id) | ||
| // if the resource is not found, it indicates the resource has been processed. | ||
| if errors.IsNotFound(err) { | ||
| return nil | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return bkr.handleRes(ctx, resource, t, "create_request") | ||
| } | ||
|
|
||
| // OnUpdate is called by the controller when a resource is updated on the maestro server. | ||
| func (bkr *GRPCBroker) OnUpdate(ctx context.Context, t types.CloudEventsDataType, id string) error { | ||
| service, ok := bkr.services[t] | ||
| if !ok { | ||
| return fmt.Errorf("failed to find service for event type %s", t) | ||
| } | ||
|
|
||
| resource, err := service.Get(ctx, id) | ||
| // if the resource is not found, it indicates the resource has been processed. | ||
| if errors.IsNotFound(err) { | ||
| return nil | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return bkr.handleRes(ctx, resource, t, "update_request") | ||
| } | ||
|
|
||
| // OnDelete is called by the controller when a resource is deleted from the maestro server. | ||
| func (bkr *GRPCBroker) OnDelete(ctx context.Context, t types.CloudEventsDataType, id string) error { | ||
| service, ok := bkr.services[t] | ||
| if !ok { | ||
| return fmt.Errorf("failed to find service for event type %s", t) | ||
| } | ||
|
|
||
| resource, err := service.Get(ctx, id) | ||
| // if the resource is not found, it indicates the resource has been processed. | ||
| if errors.IsNotFound(err) { | ||
| return nil | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return bkr.handleRes(ctx, resource, t, "delete_request") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating the OnCreate, OnUpdate, and OnDelete methods to reduce code duplication.
These three methods follow an identical pattern: find service, get resource, handle not-found case, and call handleRes with different actions. This duplication makes maintenance harder and increases the risk of inconsistent behavior.
Consider consolidating them into a single method:
-// OnCreate is called by the controller when a resource is created on the maestro server.
-func (bkr *GRPCBroker) OnCreate(ctx context.Context, t types.CloudEventsDataType, id string) error {
- service, ok := bkr.services[t]
- if !ok {
- return fmt.Errorf("failed to find service for event type %s", t)
- }
-
- resource, err := service.Get(ctx, id)
- // if the resource is not found, it indicates the resource has been processed.
- if errors.IsNotFound(err) {
- return nil
- }
- if err != nil {
- return err
- }
-
- return bkr.handleRes(ctx, resource, t, "create_request")
-}
-
-// OnUpdate is called by the controller when a resource is updated on the maestro server.
-func (bkr *GRPCBroker) OnUpdate(ctx context.Context, t types.CloudEventsDataType, id string) error {
- service, ok := bkr.services[t]
- if !ok {
- return fmt.Errorf("failed to find service for event type %s", t)
- }
-
- resource, err := service.Get(ctx, id)
- // if the resource is not found, it indicates the resource has been processed.
- if errors.IsNotFound(err) {
- return nil
- }
- if err != nil {
- return err
- }
-
- return bkr.handleRes(ctx, resource, t, "update_request")
-}
-
-// OnDelete is called by the controller when a resource is deleted from the maestro server.
-func (bkr *GRPCBroker) OnDelete(ctx context.Context, t types.CloudEventsDataType, id string) error {
- service, ok := bkr.services[t]
- if !ok {
- return fmt.Errorf("failed to find service for event type %s", t)
- }
-
- resource, err := service.Get(ctx, id)
- // if the resource is not found, it indicates the resource has been processed.
- if errors.IsNotFound(err) {
- return nil
- }
- if err != nil {
- return err
- }
-
- return bkr.handleRes(ctx, resource, t, "delete_request")
-}
+// OnCreate is called by the controller when a resource is created on the maestro server.
+func (bkr *GRPCBroker) OnCreate(ctx context.Context, t types.CloudEventsDataType, id string) error {
+ return bkr.handleResourceEvent(ctx, t, id, "create_request")
+}
+
+// OnUpdate is called by the controller when a resource is updated on the maestro server.
+func (bkr *GRPCBroker) OnUpdate(ctx context.Context, t types.CloudEventsDataType, id string) error {
+ return bkr.handleResourceEvent(ctx, t, id, "update_request")
+}
+
+// OnDelete is called by the controller when a resource is deleted from the maestro server.
+func (bkr *GRPCBroker) OnDelete(ctx context.Context, t types.CloudEventsDataType, id string) error {
+ return bkr.handleResourceEvent(ctx, t, id, "delete_request")
+}
+
+// handleResourceEvent is a common method for handling resource lifecycle events.
+func (bkr *GRPCBroker) handleResourceEvent(ctx context.Context, t types.CloudEventsDataType, id string, action types.EventAction) error {
+ service, ok := bkr.services[t]
+ if !ok {
+ return fmt.Errorf("failed to find service for event type %s", t)
+ }
+
+ resource, err := service.Get(ctx, id)
+ // if the resource is not found, it indicates the resource has been processed.
+ if errors.IsNotFound(err) {
+ return nil
+ }
+ if err != nil {
+ return err
+ }
+
+ return bkr.handleRes(ctx, resource, t, action)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (bkr *GRPCBroker) OnCreate(ctx context.Context, t types.CloudEventsDataType, id string) error { | |
| service, ok := bkr.services[t] | |
| if !ok { | |
| return fmt.Errorf("failed to find service for event type %s", t) | |
| } | |
| resource, err := service.Get(ctx, id) | |
| // if the resource is not found, it indicates the resource has been processed. | |
| if errors.IsNotFound(err) { | |
| return nil | |
| } | |
| if err != nil { | |
| return err | |
| } | |
| return bkr.handleRes(ctx, resource, t, "create_request") | |
| } | |
| // OnUpdate is called by the controller when a resource is updated on the maestro server. | |
| func (bkr *GRPCBroker) OnUpdate(ctx context.Context, t types.CloudEventsDataType, id string) error { | |
| service, ok := bkr.services[t] | |
| if !ok { | |
| return fmt.Errorf("failed to find service for event type %s", t) | |
| } | |
| resource, err := service.Get(ctx, id) | |
| // if the resource is not found, it indicates the resource has been processed. | |
| if errors.IsNotFound(err) { | |
| return nil | |
| } | |
| if err != nil { | |
| return err | |
| } | |
| return bkr.handleRes(ctx, resource, t, "update_request") | |
| } | |
| // OnDelete is called by the controller when a resource is deleted from the maestro server. | |
| func (bkr *GRPCBroker) OnDelete(ctx context.Context, t types.CloudEventsDataType, id string) error { | |
| service, ok := bkr.services[t] | |
| if !ok { | |
| return fmt.Errorf("failed to find service for event type %s", t) | |
| } | |
| resource, err := service.Get(ctx, id) | |
| // if the resource is not found, it indicates the resource has been processed. | |
| if errors.IsNotFound(err) { | |
| return nil | |
| } | |
| if err != nil { | |
| return err | |
| } | |
| return bkr.handleRes(ctx, resource, t, "delete_request") | |
| } | |
| // OnCreate is called by the controller when a resource is created on the maestro server. | |
| func (bkr *GRPCBroker) OnCreate(ctx context.Context, t types.CloudEventsDataType, id string) error { | |
| return bkr.handleResourceEvent(ctx, t, id, "create_request") | |
| } | |
| // OnUpdate is called by the controller when a resource is updated on the maestro server. | |
| func (bkr *GRPCBroker) OnUpdate(ctx context.Context, t types.CloudEventsDataType, id string) error { | |
| return bkr.handleResourceEvent(ctx, t, id, "update_request") | |
| } | |
| // OnDelete is called by the controller when a resource is deleted from the maestro server. | |
| func (bkr *GRPCBroker) OnDelete(ctx context.Context, t types.CloudEventsDataType, id string) error { | |
| return bkr.handleResourceEvent(ctx, t, id, "delete_request") | |
| } | |
| // handleResourceEvent is a common method for handling resource lifecycle events. | |
| func (bkr *GRPCBroker) handleResourceEvent(ctx context.Context, t types.CloudEventsDataType, id string, action types.EventAction) error { | |
| service, ok := bkr.services[t] | |
| if !ok { | |
| return fmt.Errorf("failed to find service for event type %s", t) | |
| } | |
| resource, err := service.Get(ctx, id) | |
| // if the resource is not found, it indicates the resource has been processed. | |
| if errors.IsNotFound(err) { | |
| return nil | |
| } | |
| if err != nil { | |
| return err | |
| } | |
| return bkr.handleRes(ctx, resource, t, action) | |
| } |
🤖 Prompt for AI Agents
In
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go
between lines 372 and 426, the OnCreate, OnUpdate, and OnDelete methods have
duplicated code that fetches the service, retrieves the resource, handles
not-found errors, and calls handleRes with different action strings. Refactor by
creating a single helper method that accepts the context, event type, resource
ID, and action string as parameters, performs the shared logic, and returns the
error. Then update OnCreate, OnUpdate, and OnDelete to call this helper method
with the appropriate action string to eliminate duplication and improve
maintainability.
| func (bkr *GRPCBroker) respondResyncSpecRequest(ctx context.Context, eventDataType types.CloudEventsDataType, evt *cloudevents.Event) error { | ||
| log := klog.FromContext(ctx) | ||
|
|
||
| resourceVersions, err := payload.DecodeSpecResyncRequest(*evt) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| clusterNameValue, err := evt.Context.GetExtension(types.ExtensionClusterName) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| clusterName := fmt.Sprintf("%s", clusterNameValue) | ||
|
|
||
| service, ok := bkr.services[eventDataType] | ||
| if !ok { | ||
| return fmt.Errorf("failed to find service for event type %s", eventDataType) | ||
| } | ||
|
|
||
| objs, err := service.List(types.ListOptions{ClusterName: clusterName, CloudEventsDataType: eventDataType}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if len(objs) == 0 { | ||
| log.V(4).Info("there are is no objs from the list, do nothing") | ||
| return nil | ||
| } | ||
|
|
||
| for _, obj := range objs { | ||
| // respond with the deleting resource regardless of the resource version | ||
| if _, ok := obj.Extensions()[types.ExtensionDeletionTimestamp]; ok { | ||
| err = bkr.handleRes(ctx, obj, eventDataType, "delete_request") | ||
| if err != nil { | ||
| log.Error(err, "failed to handle resync spec request") | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| lastResourceVersion := findResourceVersion(obj.ID(), resourceVersions.Versions) | ||
| currentResourceVersion, err := cloudeventstypes.ToInteger(obj.Extensions()[types.ExtensionResourceVersion]) | ||
| if err != nil { | ||
| log.V(4).Info("ignore the obj %v since it has a invalid resourceVersion, %v", obj, err) | ||
| continue | ||
| } | ||
|
|
||
| // the version of the work is not maintained on source or the source's work is newer than agent, send | ||
| // the newer work to agent | ||
| if currentResourceVersion == 0 || int64(currentResourceVersion) > lastResourceVersion { | ||
| err := bkr.handleRes(ctx, obj, eventDataType, "update_request") | ||
| if err != nil { | ||
| log.Error(err, "failed to handle resync spec request") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // the resources do not exist on the source, but exist on the agent, delete them | ||
| for _, rv := range resourceVersions.Versions { | ||
| _, exists := getObj(rv.ResourceID, objs) | ||
| if exists { | ||
| continue | ||
| } | ||
|
|
||
| deleteEventTypes := types.CloudEventsType{ | ||
| CloudEventsDataType: eventDataType, | ||
| SubResource: types.SubResourceSpec, | ||
| } | ||
| obj := types.NewEventBuilder("source", deleteEventTypes). | ||
| WithResourceID(rv.ResourceID). | ||
| WithResourceVersion(rv.ResourceVersion). | ||
| WithClusterName(clusterName). | ||
| WithDeletionTimestamp(time.Now()). | ||
| NewEvent() | ||
|
|
||
| // send a delete event for the current resource | ||
| err := bkr.handleRes(ctx, &obj, eventDataType, "delete_request") | ||
| if err != nil { | ||
| log.Error(err, "failed to handle delete request") | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the respondResyncSpecRequest method to improve readability.
This 82-line method handles complex resync logic and would benefit from being broken down into smaller, more focused functions. The method has multiple responsibilities: decoding requests, listing resources, handling version comparisons, and managing deletions.
Consider extracting the resource processing logic into helper methods:
func (bkr *GRPCBroker) respondResyncSpecRequest(ctx context.Context, eventDataType types.CloudEventsDataType, evt *cloudevents.Event) error {
log := klog.FromContext(ctx)
resourceVersions, err := payload.DecodeSpecResyncRequest(*evt)
if err != nil {
return err
}
clusterNameValue, err := evt.Context.GetExtension(types.ExtensionClusterName)
if err != nil {
return err
}
clusterName := fmt.Sprintf("%s", clusterNameValue)
service, ok := bkr.services[eventDataType]
if !ok {
return fmt.Errorf("failed to find service for event type %s", eventDataType)
}
objs, err := service.List(types.ListOptions{ClusterName: clusterName, CloudEventsDataType: eventDataType})
if err != nil {
return err
}
if len(objs) == 0 {
log.V(4).Info("there are no objs from the list, do nothing")
return nil
}
+ // Process existing resources
+ if err := bkr.processExistingResources(ctx, objs, resourceVersions.Versions, eventDataType); err != nil {
+ return err
+ }
+
+ // Process deleted resources
+ return bkr.processDeletedResources(ctx, resourceVersions.Versions, objs, eventDataType, clusterName)
+}
+
+func (bkr *GRPCBroker) processExistingResources(ctx context.Context, objs []*cloudevents.Event, versions []payload.ResourceVersion, eventDataType types.CloudEventsDataType) error {
+ log := klog.FromContext(ctx)
for _, obj := range objs {
// respond with the deleting resource regardless of the resource version
if _, ok := obj.Extensions()[types.ExtensionDeletionTimestamp]; ok {
err := bkr.handleRes(ctx, obj, eventDataType, "delete_request")
if err != nil {
log.Error(err, "failed to handle resync spec request")
}
continue
}
- lastResourceVersion := findResourceVersion(obj.ID(), resourceVersions.Versions)
+ lastResourceVersion := findResourceVersion(obj.ID(), versions)
currentResourceVersion, err := cloudeventstypes.ToInteger(obj.Extensions()[types.ExtensionResourceVersion])
if err != nil {
log.V(4).Info("ignore the obj %v since it has a invalid resourceVersion, %v", obj, err)
continue
}
// the version of the work is not maintained on source or the source's work is newer than agent, send
// the newer work to agent
if currentResourceVersion == 0 || int64(currentResourceVersion) > lastResourceVersion {
err := bkr.handleRes(ctx, obj, eventDataType, "update_request")
if err != nil {
log.Error(err, "failed to handle resync spec request")
}
}
}
+ return nil
+}
+
+func (bkr *GRPCBroker) processDeletedResources(ctx context.Context, versions []payload.ResourceVersion, objs []*cloudevents.Event, eventDataType types.CloudEventsDataType, clusterName string) error {
+ log := klog.FromContext(ctx)
// the resources do not exist on the source, but exist on the agent, delete them
- for _, rv := range resourceVersions.Versions {
+ for _, rv := range versions {
_, exists := getObj(rv.ResourceID, objs)
if exists {
continue
}
deleteEventTypes := types.CloudEventsType{
CloudEventsDataType: eventDataType,
SubResource: types.SubResourceSpec,
}
obj := types.NewEventBuilder("source", deleteEventTypes).
WithResourceID(rv.ResourceID).
WithResourceVersion(rv.ResourceVersion).
WithClusterName(clusterName).
WithDeletionTimestamp(time.Now()).
NewEvent()
// send a delete event for the current resource
err := bkr.handleRes(ctx, &obj, eventDataType, "delete_request")
if err != nil {
log.Error(err, "failed to handle delete request")
}
}
return nil
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go
between lines 237 and 319, the respondResyncSpecRequest method is too long and
handles multiple responsibilities such as decoding requests, listing resources,
processing resource versions, and managing deletions. Refactor this method by
extracting distinct parts of the logic into smaller helper functions, for
example, separate functions for processing resource updates, handling deletions,
and validating resource versions. This will improve readability and
maintainability by isolating concerns and making the main method more concise.
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (1)
pkg/registration/register/grpc/spoke_driver.go (1)
211-217: The log message parameters are now correctly implemented.The current implementation properly distinguishes between the certificate's issued identity (
clusterNameInCert,agentNameInCert) and the expected identity (secretOption.ClusterName,secretOption.AgentName). This addresses the concern raised in the previous review.
🧹 Nitpick comments (3)
pkg/server/services/work/work.go (1)
192-205: Consider optimizing work lookup by UID.The current implementation uses a linear search which could be inefficient with a large number of works.
For better performance at scale, consider:
- Using a UID-based indexer in the informer
- Maintaining a separate UID to work mapping cache
- Using field selectors if the API server supports filtering by UID
This would reduce lookup complexity from O(n) to O(1) or O(log n).
pkg/registration/register/grpc/spoke_driver.go (2)
65-158: Consider breaking down the large BuildClients method for better maintainability.The
BuildClientsmethod is quite long (93 lines) and handles multiple client initializations. While functionally correct, it could benefit from being broken into smaller, focused methods.Consider extracting client creation logic into separate methods:
func (d *GRPCDriver) BuildClients(ctx context.Context, secretOption register.SecretOption, bootstrapped bool) (*register.Clients, error) { config, configData, err := d.loadConfig(secretOption, bootstrapped) if err != nil { return nil, err } d.configTemplate = configData + clusterClient, clusterInformers, err := d.createClusterClient(ctx, config, secretOption) + if err != nil { + return nil, err + } + + leaseClient, err := d.createLeaseClient(ctx, config, secretOption) + if err != nil { + return nil, err + } + + // Similar extractions for event, addon, and CSR clients...This would improve readability and make the method easier to test and maintain.
222-224: Consider documenting the no-op behavior or implementing meaningful decoration.The
ManagedClusterDecoratormethod currently returns the cluster unchanged. If this is intentional, consider adding a comment explaining why no decoration is needed for GRPC clusters.func (d *GRPCDriver) ManagedClusterDecorator(cluster *clusterv1.ManagedCluster) *clusterv1.ManagedCluster { + // No additional decoration needed for GRPC-based managed clusters return cluster }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
pkg/registration/helpers/helpers.go(1 hunks)pkg/registration/helpers/helpers_test.go(2 hunks)pkg/registration/register/csr/certficate_beta.go(2 hunks)pkg/registration/register/csr/certificate.go(1 hunks)pkg/registration/register/csr/csr.go(6 hunks)pkg/registration/register/factory/options.go(3 hunks)pkg/registration/register/factory/options_test.go(2 hunks)pkg/registration/register/grpc/hub_driver.go(1 hunks)pkg/registration/register/grpc/hub_driver_test.go(1 hunks)pkg/registration/register/grpc/options.go(1 hunks)pkg/registration/register/grpc/options_test.go(1 hunks)pkg/registration/register/grpc/spoke_deriver_test.go(1 hunks)pkg/registration/register/grpc/spoke_driver.go(1 hunks)pkg/server/grpc/server.go(1 hunks)pkg/server/services/addon/addon.go(1 hunks)pkg/server/services/addon/addon_test.go(1 hunks)pkg/server/services/cluster/cluster.go(1 hunks)pkg/server/services/cluster/cluster_test.go(1 hunks)pkg/server/services/csr/csr.go(1 hunks)pkg/server/services/csr/csr_test.go(1 hunks)pkg/server/services/event/event.go(1 hunks)pkg/server/services/event/event_test.go(1 hunks)pkg/server/services/lease/lease.go(1 hunks)pkg/server/services/lease/lease_test.go(1 hunks)pkg/server/services/work/work.go(1 hunks)pkg/server/services/work/work_test.go(1 hunks)test/framework/managedcluster.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/registration/register/grpc/spoke_deriver_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/registration/register/grpc/options.go
- pkg/server/grpc/server.go
- pkg/registration/register/factory/options.go
- pkg/registration/register/grpc/hub_driver.go
- pkg/registration/register/csr/csr.go
🧰 Additional context used
🧬 Code Graph Analysis (8)
test/framework/managedcluster.go (1)
pkg/registration/helpers/helpers.go (1)
IsCSRInTerminalState(20-30)
pkg/registration/register/factory/options_test.go (2)
pkg/registration/register/factory/options.go (1)
Options(13-18)pkg/registration/register/grpc/options.go (1)
Option(12-15)
pkg/registration/register/csr/certficate_beta.go (1)
pkg/registration/helpers/helpers.go (1)
Isv1beta1CSRApproved(54-61)
pkg/registration/register/csr/certificate.go (1)
pkg/registration/helpers/helpers.go (1)
IsCSRApproved(45-52)
pkg/registration/helpers/helpers_test.go (1)
pkg/registration/helpers/helpers.go (2)
IsCSRApproved(45-52)Isv1beta1CSRApproved(54-61)
pkg/server/services/csr/csr.go (6)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/codec.go (3)
CSRCodec(23-23)NewCSRCodec(25-27)CSREventDataType(16-20)pkg/common/helpers/constants.go (1)
CloudEventsKubeSource(10-10)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go (4)
CloudEventsType(170-179)CloudEventsDataType(159-163)ParseCloudEventsType(203-224)CreateRequestAction(44-44)vendor/open-cluster-management.io/api/cluster/v1/types.go (1)
ClusterNameLabelKey(237-237)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/interface.go (1)
EventHandler(27-36)vendor/k8s.io/client-go/tools/cache/controller.go (1)
ResourceEventHandlerFuncs(237-241)
pkg/server/services/work/work.go (10)
vendor/open-cluster-management.io/api/client/work/informers/externalversions/work/v1/manifestwork.go (1)
ManifestWorkInformer(21-24)vendor/open-cluster-management.io/api/client/work/listers/work/v1/manifestwork.go (1)
ManifestWorkLister(14-21)pkg/common/helpers/constants.go (1)
CloudEventsKubeSource(10-10)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go (6)
CloudEventsType(170-179)CloudEventsDataType(159-163)ParseCloudEventsType(203-224)ExtensionClusterName(71-71)ExtensionResourceVersion(61-61)UpdateRequestAction(47-47)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/payload/manifestbundle.go (1)
ManifestBundleEventDataType(11-15)vendor/open-cluster-management.io/sdk-go/pkg/patcher/patcher.go (1)
NewPatcher(51-56)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/common/common.go (3)
ResourceDeleted(31-31)ResourceFinalizer(33-33)ManifestWorkGR(39-39)vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllerutil/controllerutil.go (2)
RemoveFinalizer(484-498)AddFinalizer(471-480)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/interface.go (1)
EventHandler(27-36)vendor/k8s.io/client-go/tools/cache/controller.go (1)
ResourceEventHandlerFuncs(237-241)
pkg/server/services/event/event.go (4)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/codec.go (3)
EventCodec(20-20)NewEventCodec(22-24)EventEventDataType(13-17)pkg/common/helpers/constants.go (1)
CloudEventsKubeSource(10-10)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go (5)
CloudEventsType(170-179)CloudEventsDataType(159-163)ParseCloudEventsType(203-224)CreateRequestAction(44-44)UpdateRequestAction(47-47)vendor/k8s.io/klog/v2/klog.go (2)
V(1353-1355)Infof(1512-1514)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit
- GitHub Check: integration
- GitHub Check: e2e-hosted
- GitHub Check: verify
- GitHub Check: e2e-singleton
- GitHub Check: e2e
🔇 Additional comments (20)
pkg/registration/register/grpc/options_test.go (1)
18-27: Clarify the expected validation logic for configuration files.The test cases suggest that having only
BootstrapConfigFileor onlyConfigFileshould fail validation. However, this seems counterintuitive - typically, either a bootstrap config or a main config file should be sufficient for validation to pass.Please verify if the validation logic is correct or if the test expectations need to be updated.
#!/bin/bash # Description: Check the actual validation implementation to understand expected behavior ast-grep --pattern 'func ($_ *Option) Validate() error { $$$ }'pkg/registration/register/factory/options_test.go (1)
8-8: LGTM! Well-structured test coverage for GRPC validation.The new test cases properly cover the GRPC validation scenarios:
- Empty GRPCOption correctly expects validation failure
- GRPCOption with either BootstrapConfigFile or ConfigFile correctly expects validation success
The test structure is consistent with existing patterns and provides good coverage.
Also applies to: 55-82
test/framework/managedcluster.go (1)
16-16: LGTM! Good refactoring to use centralized helper function.This change eliminates code duplication by using the centralized
helpers.IsCSRInTerminalStatefunction instead of maintaining local CSR state checking logic. This improves maintainability and consistency across the codebase.Also applies to: 133-133
pkg/registration/register/csr/certificate.go (1)
175-175: LGTM! Good refactoring to use centralized CSR approval logic.This change replaces manual condition checking with the centralized
helpers.IsCSRApprovedfunction, improving code consistency and maintainability while preserving the same functionality.pkg/registration/register/csr/certficate_beta.go (1)
15-15: LGTM: Clean refactoring to centralize CSR approval logic.The refactoring successfully delegates CSR approval checking to the helper function
helpers.Isv1beta1CSRApproved, replacing the previous inline iteration logic. This improves code maintainability and ensures consistent CSR approval status determination across the codebase.Also applies to: 32-32
pkg/registration/helpers/helpers.go (1)
45-61: LGTM: Well-implemented helper functions for CSR approval status.Both
IsCSRApprovedandIsv1beta1CSRApprovedfunctions correctly implement the logic to check for CSR approval status by iterating through conditions and looking for theCertificateApprovedtype. The separation for different API versions (v1 and v1beta1) is appropriate and maintains compatibility across Kubernetes versions.pkg/registration/register/grpc/hub_driver_test.go (1)
22-152: LGTM: Comprehensive test coverage for CSR signing functionality.The test thoroughly validates the CSR signing controller behavior across multiple scenarios:
- Edge cases (no CSR, unapproved CSR, existing certificate)
- Error conditions (wrong signer name)
- Happy path (valid CSR signing)
The test structure is well-organized with proper use of fake clients, self-signed CA generation for testing, and clear validation of expected client actions.
pkg/registration/helpers/helpers_test.go (1)
252-295: LGTM: Well-structured test for CSR approval logic.The test covers all important scenarios (no condition, approved, denied) with clear assertions and follows good table-driven test patterns.
pkg/server/services/event/event_test.go (1)
21-207: LGTM: Comprehensive test coverage for event service functionality.The test suite thoroughly validates all core event service methods:
Get: Proper error handling for missing events and successful retrievalList: Correct filtering by cluster nameHandleStatusUpdate: Comprehensive CloudEvent processing with proper validation of invalid event types, unsupported actions, and successful create/update operationsThe test structure is well-organized with clear assertions and appropriate use of fake clients for unit testing.
pkg/server/services/work/work_test.go (2)
1-402: LGTM! Comprehensive test coverage with good patterns.The test file demonstrates excellent testing practices with table-driven tests, proper use of fake clients and informers, and thorough coverage of various scenarios including error cases, resource version mismatches, and finalizer handling.
340-362: Add test coverage for OnDelete handler method.The
TestEventHandlerFuncsonly testsOnCreateandOnUpdatemethods but doesn't test theOnDeletemethod, even though the mock handler implements it (lines 392-401). This creates incomplete test coverage for the event handler functionality.Add test coverage for the delete handler:
eventHandlerFuncs.UpdateFunc(nil, work) if !handler.onUpdateCalled { t.Errorf("onUpdate not called") } + + eventHandlerFuncs.DeleteFunc(work) + if !handler.onDeleteCalled { + t.Errorf("onDelete not called") + }Likely an incorrect or invalid review comment.
pkg/server/services/addon/addon_test.go (1)
1-251: LGTM! Well-structured test suite with good coverage.The test file follows excellent testing patterns with comprehensive coverage of the AddonService functionality, proper use of fake clients, and thorough validation of various scenarios.
pkg/server/services/addon/addon.go (1)
1-123: LGTM! Clean implementation following consistent service patterns.The AddonService implementation demonstrates good software engineering practices:
- Clear separation of concerns with proper dependency injection
- Consistent error handling and logging
- Proper use of Kubernetes client-go patterns
- CloudEvents integration with appropriate encoding/decoding
- Standard informer event handling
The code follows the established patterns seen in other service implementations, which promotes maintainability and consistency across the codebase.
pkg/server/services/cluster/cluster.go (2)
49-65: Verify the List method behavior is intentional.The
Listmethod returns only a single cluster by name rather than a list of clusters, which differs from the pattern used in other services (addon, work, etc.) whereListreturns multiple resources. This might be correct for the cluster use case, but please verify this behavior is intentional.If this is the intended behavior, consider adding a comment to clarify why clusters are handled differently from other resource types.
1-126: LGTM! Well-implemented service with proper cluster-scoped resource handling.The ClusterService implementation correctly handles cluster-scoped resources by using
meta.Accessorinstead of namespace-based key functions. The support for both create and update actions with subresource handling is appropriate for cluster management scenarios.pkg/server/services/csr/csr.go (3)
51-73: LGTM! Correct label-based filtering for CSR listing.The CSR listing implementation correctly uses label selectors to filter CSRs by cluster name using the
open-cluster-management.io/cluster-namelabel. This is the appropriate approach for CSR management in a multi-cluster environment.
75-92: LGTM! Appropriate action support for CSR lifecycle.The
HandleStatusUpdatemethod correctly supports only thecreateaction, which aligns with the typical CSR workflow where agents create CSRs and the hub approves/denies them rather than updating their status.
1-124: LGTM! Well-implemented CSR service with proper resource management.The CSRService implementation demonstrates good practices for managing cluster-scoped Kubernetes resources with appropriate label-based filtering and CloudEvents integration.
pkg/server/services/cluster/cluster_test.go (1)
1-296: Well-structured and comprehensive test coverage!The test file properly uses
ManagedClusterobjects throughout and covers all major scenarios including error cases, CRUD operations, and event handling.pkg/registration/register/grpc/spoke_driver.go (1)
271-320: CSR control implementation looks solid.The
csrControlstruct properly delegates CSR operations to the CloudEvents CSR client holder, and the implementation correctly uses the helper functions for CSR approval checking.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return e.codec.Encode(helpers.CloudEventsKubeSource, types.CloudEventsType{CloudEventsDataType: eventce.EventEventDataType}, evt) |
There was a problem hiding this comment.
Complete the CloudEventsType construction.
The CloudEventsType construction is incomplete - it's missing the SubResource and Action fields that are required according to the type definition in vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go.
-return e.codec.Encode(helpers.CloudEventsKubeSource, types.CloudEventsType{CloudEventsDataType: eventce.EventEventDataType}, evt)
+return e.codec.Encode(helpers.CloudEventsKubeSource, types.CloudEventsType{
+ CloudEventsDataType: eventce.EventEventDataType,
+ SubResource: types.SubResourceSpec, // or types.SubResourceStatus as appropriate
+ Action: types.CreateRequestAction, // or appropriate action
+}, evt)The same fix should be applied to line 51 in the List method.
Also applies to: 51-51
🤖 Prompt for AI Agents
In pkg/server/services/event/event.go at lines 41 and 51, the CloudEventsType
struct is constructed incompletely, missing the required SubResource and Action
fields. Update the construction of CloudEventsType in both the return statements
to include appropriate values for SubResource and Action fields as per the type
definition in
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go.
| func (e *EventService) List(listOpts types.ListOptions) ([]*cloudevents.Event, error) { | ||
| evts, err := e.client.EventsV1().Events(listOpts.ClusterName).List(context.Background(), metav1.ListOptions{}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use context parameter instead of context.Background().
The List method should accept a context parameter and use it for the Kubernetes API call instead of context.Background(). This allows proper request cancellation and timeout handling.
-func (e *EventService) List(listOpts types.ListOptions) ([]*cloudevents.Event, error) {
- evts, err := e.client.EventsV1().Events(listOpts.ClusterName).List(context.Background(), metav1.ListOptions{})
+func (e *EventService) List(ctx context.Context, listOpts types.ListOptions) ([]*cloudevents.Event, error) {
+ evts, err := e.client.EventsV1().Events(listOpts.ClusterName).List(ctx, metav1.ListOptions{})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (e *EventService) List(listOpts types.ListOptions) ([]*cloudevents.Event, error) { | |
| evts, err := e.client.EventsV1().Events(listOpts.ClusterName).List(context.Background(), metav1.ListOptions{}) | |
| func (e *EventService) List(ctx context.Context, listOpts types.ListOptions) ([]*cloudevents.Event, error) { | |
| evts, err := e.client.EventsV1().Events(listOpts.ClusterName).List(ctx, metav1.ListOptions{}) |
🤖 Prompt for AI Agents
In pkg/server/services/event/event.go around lines 44 to 45, the List method
currently uses context.Background() for the Kubernetes API call, which prevents
proper cancellation and timeout handling. Modify the List method signature to
accept a context.Context parameter and replace context.Background() with this
passed context in the call to e.client.EventsV1().Events(...).List. This change
enables better control over request lifecycle and cancellation.
| func TestIsv1beta1CSRDenied(t *testing.T) { | ||
| cases := []struct { | ||
| name string | ||
| status *certificatesv1beta1.CertificateSigningRequestStatus | ||
| expected bool | ||
| }{ | ||
| { | ||
| name: "no condition", | ||
| status: &certificatesv1beta1.CertificateSigningRequestStatus{}, | ||
| expected: false, | ||
| }, | ||
| { | ||
| name: "approved", | ||
| status: &certificatesv1beta1.CertificateSigningRequestStatus{ | ||
| Conditions: []certificatesv1beta1.CertificateSigningRequestCondition{ | ||
| { | ||
| Type: certificatesv1beta1.CertificateApproved, | ||
| }, | ||
| }, | ||
| }, | ||
| expected: true, | ||
| }, | ||
| { | ||
| name: "denied", | ||
| status: &certificatesv1beta1.CertificateSigningRequestStatus{ | ||
| Conditions: []certificatesv1beta1.CertificateSigningRequestCondition{ | ||
| { | ||
| Type: certificatesv1beta1.CertificateDenied, | ||
| }, | ||
| }, | ||
| }, | ||
| expected: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, c := range cases { | ||
| t.Run(c.name, func(t *testing.T) { | ||
| actual := Isv1beta1CSRApproved(c.status) | ||
| if actual != c.expected { | ||
| t.Errorf("expected %t, but %t", c.expected, actual) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix the test function name to match its purpose.
The function name TestIsv1beta1CSRDenied suggests it's testing CSR denial logic, but it's actually testing the Isv1beta1CSRApproved function (line 334). This naming inconsistency could cause confusion.
Apply this diff to fix the function name:
-func TestIsv1beta1CSRDenied(t *testing.T) {
+func TestIsv1beta1CSRApproved(t *testing.T) {🤖 Prompt for AI Agents
In pkg/registration/helpers/helpers_test.go around lines 297 to 340, the test
function is named TestIsv1beta1CSRDenied but it tests the Isv1beta1CSRApproved
function. Rename the test function to TestIsv1beta1CSRApproved to accurately
reflect its purpose and avoid confusion.
| func (w *WorkService) RegisterHandler(handler server.EventHandler) { | ||
| if _, err := w.workInformer.Informer().AddEventHandler(w.EventHandlerFuncs(handler)); err != nil { | ||
| klog.Errorf("failed to register work informer event handler, %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return registration errors to caller.
Similar to other services, this method should return errors instead of just logging them.
Update the method signature and error handling:
-func (w *WorkService) RegisterHandler(handler server.EventHandler) {
+func (w *WorkService) RegisterHandler(handler server.EventHandler) error {
if _, err := w.workInformer.Informer().AddEventHandler(w.EventHandlerFuncs(handler)); err != nil {
- klog.Errorf("failed to register work informer event handler, %v", err)
+ return fmt.Errorf("failed to register work informer event handler: %v", err)
}
+ return nil
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/server/services/work/work.go around lines 148 to 152, the RegisterHandler
method currently logs errors internally instead of returning them. Modify the
method signature to return an error, and update the error handling to return the
error to the caller instead of just logging it. This aligns with the pattern
used in other services and allows callers to handle registration failures
appropriately.
| func (d *GRPCDriver) Fork(addonName string, secretOption register.SecretOption) register.RegisterDriver { | ||
| csrDriver := d.csrDriver.Fork(addonName, secretOption) | ||
| return &GRPCDriver{ | ||
| control: d.control, | ||
| opt: d.opt, | ||
| csrDriver: csrDriver.(*csr.CSRDriver), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type assertion safety check to prevent potential panic.
The type assertion on line 165 could panic if the returned driver is not of type *csr.CSRDriver.
Apply this diff to add a safety check:
func (d *GRPCDriver) Fork(addonName string, secretOption register.SecretOption) register.RegisterDriver {
csrDriver := d.csrDriver.Fork(addonName, secretOption)
+ csrDriverTyped, ok := csrDriver.(*csr.CSRDriver)
+ if !ok {
+ // This should never happen, but handle gracefully
+ klog.Errorf("Fork returned unexpected type: %T", csrDriver)
+ return nil
+ }
return &GRPCDriver{
control: d.control,
opt: d.opt,
- csrDriver: csrDriver.(*csr.CSRDriver),
+ csrDriver: csrDriverTyped,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (d *GRPCDriver) Fork(addonName string, secretOption register.SecretOption) register.RegisterDriver { | |
| csrDriver := d.csrDriver.Fork(addonName, secretOption) | |
| return &GRPCDriver{ | |
| control: d.control, | |
| opt: d.opt, | |
| csrDriver: csrDriver.(*csr.CSRDriver), | |
| } | |
| } | |
| func (d *GRPCDriver) Fork(addonName string, secretOption register.SecretOption) register.RegisterDriver { | |
| csrDriver := d.csrDriver.Fork(addonName, secretOption) | |
| csrDriverTyped, ok := csrDriver.(*csr.CSRDriver) | |
| if !ok { | |
| // This should never happen, but handle gracefully | |
| klog.Errorf("Fork returned unexpected type: %T", csrDriver) | |
| return nil | |
| } | |
| return &GRPCDriver{ | |
| control: d.control, | |
| opt: d.opt, | |
| csrDriver: csrDriverTyped, | |
| } | |
| } |
🤖 Prompt for AI Agents
In pkg/registration/register/grpc/spoke_driver.go around lines 160 to 167, the
type assertion on csrDriver to *csr.CSRDriver is done without checking, which
can cause a panic if the assertion fails. Modify the code to perform a safe type
assertion with the comma-ok idiom, check if the assertion succeeded, and handle
the failure case appropriately (e.g., return an error or panic with a clear
message) to prevent unexpected panics.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
pkg/server/services/lease/lease.go (2)
83-89: Add existence check before updating lease.The method directly updates the lease without verifying it exists. This could lead to confusing error messages if the lease has been deleted.
Consider adding an existence check:
switch eventType.Action { case types.UpdateRequestAction: + // Verify the lease exists before updating + if _, err := l.lister.Leases(lease.Namespace).Get(lease.Name); err != nil { + return fmt.Errorf("failed to get lease %s/%s: %v", lease.Namespace, lease.Name, err) + } _, err := l.client.CoordinationV1().Leases(lease.Namespace).Update(ctx, lease, metav1.UpdateOptions{}) return err
92-96: Propagate registration errors to caller.The method logs errors but doesn't return them, preventing proper error handling by callers.
Consider returning the error:
-func (l *LeaseService) RegisterHandler(handler server.EventHandler) { +func (l *LeaseService) RegisterHandler(handler server.EventHandler) error { if _, err := l.informer.Informer().AddEventHandler(l.EventHandlerFuncs(handler)); err != nil { - klog.Errorf("failed to register lease informer event handler, %v", err) + return fmt.Errorf("failed to register lease informer event handler: %v", err) } + return nil }pkg/server/services/work/work.go (1)
148-152: Return registration errors to caller.Similar to other services, this method should return errors instead of just logging them.
Update the method signature and error handling:
-func (w *WorkService) RegisterHandler(handler server.EventHandler) { +func (w *WorkService) RegisterHandler(handler server.EventHandler) error { if _, err := w.workInformer.Informer().AddEventHandler(w.EventHandlerFuncs(handler)); err != nil { - klog.Errorf("failed to register work informer event handler, %v", err) + return fmt.Errorf("failed to register work informer event handler: %v", err) } + return nil }pkg/registration/register/grpc/hub_driver.go (1)
145-148: Fix typo and undefinedsignervariable.The comment has a typo and the
signervariable is not defined.- // Do not sigh apiserver cert + // Do not sign apiserver cert if csr.Spec.SignerName != signer {Also, add the signer constant definition at the package level:
const signer = "open-cluster-management.io/grpc-signer"pkg/registration/register/grpc/spoke_driver.go (2)
52-62: Critical compilation error: undefined variablesigner.The variable
signeron line 53 is not defined anywhere in the function scope, which will cause a compilation error.The past review comment about this issue has been marked as addressed, but the undefined variable still appears in the code. Please verify if this fix was properly applied.
159-166: Type assertion safety issue may cause runtime panic.The type assertion on line 164 could panic if the returned driver is not of type
*csr.CSRDriver.This issue was flagged in a previous review and marked as addressed, but the unsafe type assertion still appears in the current code.
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/server.go (1)
135-156: Potential security issue: authentication bypass when no authenticators configured.The authentication interceptor may allow unauthenticated requests if no authenticators are configured, as it falls through to calling the handler when the loop completes without error.
This issue was previously identified and should have been addressed. The current code still shows the problematic pattern.
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go (1)
371-426: Code duplication in lifecycle event handlers.The
OnCreate,OnUpdate, andOnDeletemethods contain identical logic patterns with only the action string differing.This code duplication was identified in a previous review as a refactoring opportunity. The current implementation still shows the duplicated pattern.
🧹 Nitpick comments (2)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.go (1)
60-60: Address TODO: Add ManagedCluster validation.The TODO comment indicates missing validation for the ManagedCluster resource.
Would you like me to generate validation logic for the ManagedCluster or open an issue to track this task?
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go (1)
262-262: Fix typo in log message.There's a grammatical error in the log message.
- log.V(4).Info("there are is no objs from the list, do nothing") + log.V(4).Info("there are no objs from the list, do nothing")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (59)
cmd/registration/main.go(2 hunks)go.mod(3 hunks)pkg/common/helpers/constants.go(1 hunks)pkg/registration/hub/manager.go(4 hunks)pkg/registration/hub/manifests/rbac/managedcluster-registration-clusterrole.yaml(2 hunks)pkg/registration/hub/manifests/rbac/managedcluster-work-clusterrole.yaml(1 hunks)pkg/registration/register/csr/csr.go(6 hunks)pkg/registration/register/factory/options.go(3 hunks)pkg/registration/register/factory/options_test.go(2 hunks)pkg/registration/register/grpc/hub_driver.go(1 hunks)pkg/registration/register/grpc/hub_driver_test.go(1 hunks)pkg/registration/register/grpc/options.go(1 hunks)pkg/registration/register/grpc/options_test.go(1 hunks)pkg/registration/register/grpc/spoke_deriver_test.go(1 hunks)pkg/registration/register/grpc/spoke_driver.go(1 hunks)pkg/server/grpc/server.go(1 hunks)pkg/server/services/addon/addon.go(1 hunks)pkg/server/services/addon/addon_test.go(1 hunks)pkg/server/services/cluster/cluster.go(1 hunks)pkg/server/services/cluster/cluster_test.go(1 hunks)pkg/server/services/csr/csr.go(1 hunks)pkg/server/services/csr/csr_test.go(1 hunks)pkg/server/services/event/event.go(1 hunks)pkg/server/services/event/event_test.go(1 hunks)pkg/server/services/lease/lease.go(1 hunks)pkg/server/services/lease/lease_test.go(1 hunks)pkg/server/services/work/work.go(1 hunks)pkg/server/services/work/work_test.go(1 hunks)vendor/modules.txt(3 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/wrapper.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/clientholder.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/wrapper.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/common/common.go(0 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/clientholder.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/wrapper.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/client/manifestwork.go(2 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/source/client/manifestwork.go(3 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.go(2 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/interface.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/mtls.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/token.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/interface.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/kube/sar.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/options.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/server.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/interface.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/store.go(1 hunks)
💤 Files with no reviewable changes (1)
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/common/common.go
✅ Files skipped from review due to trivial changes (3)
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/clientholder.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/wrapper.go
🚧 Files skipped from review as they are similar to previous changes (47)
- pkg/registration/register/factory/options_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/source/client/manifestwork.go
- cmd/registration/main.go
- pkg/registration/register/grpc/spoke_deriver_test.go
- pkg/common/helpers/constants.go
- pkg/registration/hub/manifests/rbac/managedcluster-work-clusterrole.yaml
- go.mod
- pkg/registration/register/grpc/options_test.go
- pkg/registration/hub/manifests/rbac/managedcluster-registration-clusterrole.yaml
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/mtls.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/store.go
- pkg/registration/register/grpc/hub_driver_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/interface.go
- pkg/registration/register/grpc/options.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/interface.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/token.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/options.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/codec.go
- pkg/registration/register/factory/options.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/client/manifestwork.go
- pkg/server/services/addon/addon_test.go
- pkg/server/services/event/event.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/codec.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/clientholder.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/interface.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/wrapper.go
- pkg/server/grpc/server.go
- pkg/server/services/work/work_test.go
- pkg/server/services/addon/addon.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/client.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/codec.go
- pkg/server/services/event/event_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/codec.go
- pkg/server/services/lease/lease_test.go
- pkg/server/services/csr/csr_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/kube/sar.go
- pkg/server/services/cluster/cluster.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/wrapper.go
- vendor/modules.txt
- pkg/server/services/cluster/cluster_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/client.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/client.go
- pkg/registration/hub/manager.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/codec.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.go
- pkg/registration/register/csr/csr.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
pkg/registration/register/grpc/hub_driver.go (3)
vendor/github.com/openshift/library-go/pkg/operator/events/recorder.go (1)
Recorder(19-41)pkg/registration/register/interface.go (1)
HubDriver(92-109)vendor/open-cluster-management.io/api/cluster/v1/types.go (1)
ClusterNameLabelKey(237-237)
pkg/server/services/csr/csr.go (7)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/codec.go (3)
CSRCodec(23-23)NewCSRCodec(25-27)CSREventDataType(16-20)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/store.go (1)
Service(13-25)pkg/common/helpers/constants.go (1)
CloudEventsKubeSource(10-10)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go (5)
CloudEventsType(170-179)CloudEventsDataType(159-163)ListOptions(145-156)ParseCloudEventsType(203-224)CreateRequestAction(44-44)vendor/open-cluster-management.io/api/cluster/v1/types.go (1)
ClusterNameLabelKey(237-237)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/interface.go (1)
EventHandler(27-36)vendor/k8s.io/client-go/tools/cache/controller.go (1)
ResourceEventHandlerFuncs(237-241)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e-singleton
- GitHub Check: e2e
- GitHub Check: e2e-hosted
- GitHub Check: integration
- GitHub Check: unit
- GitHub Check: verify
🔇 Additional comments (16)
pkg/registration/register/grpc/spoke_driver.go (4)
64-157: LGTM! Well-structured CloudEvents client initialization.The
BuildClientsmethod properly initializes multiple CloudEvents clients with appropriate configurations and error handling. The client setup for clusters, leases, events, addons, and CSRs follows a consistent pattern with proper store initialization and informer integration.
168-173: LGTM! Clean template injection pattern.The
Processmethod appropriately injects the gRPC config template into the secret data before delegating to the CSR driver.
225-268: LGTM! Robust configuration loading with proper error handling.The
loadConfigmethod handles both bootstrap and main configuration files appropriately, with proper error messages and YAML marshaling of the gRPC configuration.
270-324: LGTM! Clean CSR control implementation.The
csrControlstruct properly implements the CSR control interface with appropriate error handling and context usage for API calls.vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/server.go (5)
39-61: LGTM! Clean builder pattern implementation.The server construction methods follow a clean builder pattern with fluent interfaces for configuring authenticators, authorizers, services, and hooks.
63-133: LGTM! Comprehensive gRPC server configuration.The
Runmethod properly configures all gRPC server options including message sizes, concurrency, timeouts, TLS with optional mTLS, and keepalive policies. The TLS configuration correctly enforces TLS 1.3 and handles client CA certificates appropriately.
158-190: LGTM! Robust authorization interceptor with proper request validation.The authorization interceptor correctly extracts CloudEvents type and cluster name from the publish request, with appropriate error handling for malformed requests.
192-237: LGTM! Proper stream context wrapping for authentication.The stream authentication interceptor correctly wraps the stream context to propagate authenticated identity through the request chain.
239-303: LGTM! Efficient subscription request caching for authorization.The authorization stream interceptor cleverly caches the subscription request after authorization to avoid re-reading from the stream, with proper thread safety using mutex.
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go (7)
53-67: LGTM! Clean broker initialization with proper service registration.The
NewGRPCBrokerconstructor properly initializes maps and registers the service with the gRPC server.
69-79: LGTM! Thread-safe subscriber enumeration.The
Subscribersmethod correctly uses mutex locking to safely enumerate cluster names from the subscriber map.
81-99: LGTM! Proper server lifecycle management with graceful shutdown.The
Startmethod correctly implements graceful shutdown by callingGracefulStop()when the context is cancelled, addressing the concern about abrupt connection terminations.
164-174: LGTM! Safe subscriber unregistration.The
unregistermethod correctly checks for subscriber existence before attempting to close the error channel and delete the entry, preventing potential panics.
452-462: LGTM! Safe type assertion in resource lookup.The
getObjfunction correctly performs safe type assertion when extracting the resource ID from event extensions, preventing potential panics.
101-142: LGTM! Robust event publishing with proper error handling.The
Publishmethod correctly converts protobuf to CloudEvents, validates event types, handles resync requests, and delegates to appropriate services with proper gRPC status code mapping.
176-227: LGTM! Comprehensive subscription handling with proper lifecycle management.The
Subscribemethod correctly validates subscription requests, registers subscribers with proper error handling, and manages subscriber lifecycle including graceful cleanup on context cancellation.
| func (c *CSRService) RegisterHandler(handler server.EventHandler) { | ||
| if _, err := c.csrInformer.Informer().AddEventHandler(c.EventHandlerFuncs(handler)); err != nil { | ||
| klog.Errorf("failed to register csr informer event handler, %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Return registration errors to caller.
Consistent with other services, this method should return errors instead of just logging them.
Update the method signature and error handling:
-func (c *CSRService) RegisterHandler(handler server.EventHandler) {
+func (c *CSRService) RegisterHandler(handler server.EventHandler) error {
if _, err := c.csrInformer.Informer().AddEventHandler(c.EventHandlerFuncs(handler)); err != nil {
- klog.Errorf("failed to register csr informer event handler, %v", err)
+ return fmt.Errorf("failed to register csr informer event handler: %v", err)
}
+ return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *CSRService) RegisterHandler(handler server.EventHandler) { | |
| if _, err := c.csrInformer.Informer().AddEventHandler(c.EventHandlerFuncs(handler)); err != nil { | |
| klog.Errorf("failed to register csr informer event handler, %v", err) | |
| } | |
| } | |
| func (c *CSRService) RegisterHandler(handler server.EventHandler) error { | |
| if _, err := c.csrInformer.Informer().AddEventHandler(c.EventHandlerFuncs(handler)); err != nil { | |
| return fmt.Errorf("failed to register csr informer event handler: %v", err) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In pkg/server/services/csr/csr.go around lines 94 to 98, the RegisterHandler
method currently logs errors instead of returning them, which is inconsistent
with other services. Modify the method signature to return an error, and update
the error handling to return the error to the caller instead of just logging it.
Ensure that the method returns nil if no error occurs.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/server.go (1)
142-155: Authentication interceptor allows unauthenticated requestsThe implementation might return a nil error if no authenticators are configured, allowing unauthenticated requests.
pkg/registration/register/grpc/spoke_driver.go (1)
160-167: Add type assertion safety check to prevent potential panic.The type assertion on line 165 could panic if the returned driver is not of type
*csr.CSRDriver.Apply this diff to add a safety check:
func (d *GRPCDriver) Fork(addonName string, secretOption register.SecretOption) register.RegisterDriver { csrDriver := d.csrDriver.Fork(addonName, secretOption) + csrDriverTyped, ok := csrDriver.(*csr.CSRDriver) + if !ok { + // This should never happen, but handle gracefully + klog.Errorf("Fork returned unexpected type: %T", csrDriver) + return nil + } return &GRPCDriver{ control: d.control, opt: d.opt, - csrDriver: csrDriver.(*csr.CSRDriver), + csrDriver: csrDriverTyped, } }vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go (1)
372-426: Consider consolidating the OnCreate, OnUpdate, and OnDelete methods to reduce code duplication.These three methods follow an identical pattern: find service, get resource, handle not-found case, and call
handleReswith different actions. This duplication makes maintenance harder and increases the risk of inconsistent behavior.Consider consolidating them into a single method:
-// OnCreate is called by the controller when a resource is created on the maestro server. -func (bkr *GRPCBroker) OnCreate(ctx context.Context, t types.CloudEventsDataType, id string) error { - service, ok := bkr.services[t] - if !ok { - return fmt.Errorf("failed to find service for event type %s", t) - } - - resource, err := service.Get(ctx, id) - // if the resource is not found, it indicates the resource has been processed. - if errors.IsNotFound(err) { - return nil - } - if err != nil { - return err - } - - return bkr.handleRes(ctx, resource, t, "create_request") -} - -// OnUpdate is called by the controller when a resource is updated on the maestro server. -func (bkr *GRPCBroker) OnUpdate(ctx context.Context, t types.CloudEventsDataType, id string) error { - service, ok := bkr.services[t] - if !ok { - return fmt.Errorf("failed to find service for event type %s", t) - } - - resource, err := service.Get(ctx, id) - // if the resource is not found, it indicates the resource has been processed. - if errors.IsNotFound(err) { - return nil - } - if err != nil { - return err - } - - return bkr.handleRes(ctx, resource, t, "update_request") -} - -// OnDelete is called by the controller when a resource is deleted from the maestro server. -func (bkr *GRPCBroker) OnDelete(ctx context.Context, t types.CloudEventsDataType, id string) error { - service, ok := bkr.services[t] - if !ok { - return fmt.Errorf("failed to find service for event type %s", t) - } - - resource, err := service.Get(ctx, id) - // if the resource is not found, it indicates the resource has been processed. - if errors.IsNotFound(err) { - return nil - } - if err != nil { - return err - } - - return bkr.handleRes(ctx, resource, t, "delete_request") -} +// OnCreate is called by the controller when a resource is created on the maestro server. +func (bkr *GRPCBroker) OnCreate(ctx context.Context, t types.CloudEventsDataType, id string) error { + return bkr.handleResourceEvent(ctx, t, id, "create_request") +} + +// OnUpdate is called by the controller when a resource is updated on the maestro server. +func (bkr *GRPCBroker) OnUpdate(ctx context.Context, t types.CloudEventsDataType, id string) error { + return bkr.handleResourceEvent(ctx, t, id, "update_request") +} + +// OnDelete is called by the controller when a resource is deleted from the maestro server. +func (bkr *GRPCBroker) OnDelete(ctx context.Context, t types.CloudEventsDataType, id string) error { + return bkr.handleResourceEvent(ctx, t, id, "delete_request") +} + +// handleResourceEvent is a common method for handling resource lifecycle events. +func (bkr *GRPCBroker) handleResourceEvent(ctx context.Context, t types.CloudEventsDataType, id string, action types.EventAction) error { + service, ok := bkr.services[t] + if !ok { + return fmt.Errorf("failed to find service for event type %s", t) + } + + resource, err := service.Get(ctx, id) + // if the resource is not found, it indicates the resource has been processed. + if errors.IsNotFound(err) { + return nil + } + if err != nil { + return err + } + + return bkr.handleRes(ctx, resource, t, action) +}
🧹 Nitpick comments (5)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.go (1)
60-60: Implement ManagedCluster validationThe TODO comment indicates that validation is missing. This could lead to invalid clusters being created.
Would you like me to generate validation logic for the ManagedCluster or open an issue to track this task?
pkg/registration/register/grpc/spoke_deriver_test.go (1)
43-55: Improve test clarity by setting cluster and agent names.The "cert is not issued for cluster1:agent1" test case correctly validates the mismatch, but the "valid" test case doesn't explicitly set
clusterNameandagentName. While this works because the driver skips issuer validation when these fields are empty, it would be clearer to explicitly test the positive case.Apply this diff to make the test more explicit:
{ name: "valid", + clusterName: "cluster1", + agentName: "agent1", tlsCert: cert.Cert, tlsKey: cert.Key, valid: true, },pkg/server/services/addon/addon.go (1)
99-122: Consider adding DeleteFunc handler.The EventHandlerFuncs only implements AddFunc and UpdateFunc, but not DeleteFunc. This means addon deletions won't trigger CloudEvents notifications.
If deletion events should be handled, add the DeleteFunc:
func (s *AddonService) EventHandlerFuncs(handler server.EventHandler) *cache.ResourceEventHandlerFuncs { return &cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { key, err := cache.MetaNamespaceKeyFunc(obj) if err != nil { klog.Errorf("failed to get key for addon %v", err) return } if err := handler.OnCreate(context.Background(), addonce.ManagedClusterAddOnEventDataType, key); err != nil { klog.Error(err) } }, UpdateFunc: func(oldObj, newObj interface{}) { key, err := cache.MetaNamespaceKeyFunc(newObj) if err != nil { klog.Errorf("failed to get key for addon %v", err) return } if err := handler.OnUpdate(context.Background(), addonce.ManagedClusterAddOnEventDataType, key); err != nil { klog.Error(err) } }, + DeleteFunc: func(obj interface{}) { + key, err := cache.DeletionHandlingMetaNamespaceKeyFunc(obj) + if err != nil { + klog.Errorf("failed to get key for addon %v", err) + return + } + if err := handler.OnDelete(context.Background(), addonce.ManagedClusterAddOnEventDataType, key); err != nil { + klog.Error(err) + } + }, } }vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go (1)
262-262: Fix typo in log message.There's a grammatical error in the log message.
Apply this diff to fix the typo:
- log.V(4).Info("there are is no objs from the list, do nothing") + log.V(4).Info("there are no objs from the list, do nothing")vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.go (1)
82-95: Fix variable naming for better readability.The variable name
clusteron line 90 is misleading since we're iterating over addon resources, not clusters.Apply this diff to improve clarity:
- for _, cluster := range addonList.Items { - items = append(items, *cluster) + for _, addon := range addonList.Items { + items = append(items, *addon)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (60)
cmd/registration/main.go(2 hunks)go.mod(3 hunks)pkg/common/helpers/constants.go(1 hunks)pkg/registration/hub/manager.go(4 hunks)pkg/registration/hub/manifests/rbac/managedcluster-registration-clusterrole.yaml(2 hunks)pkg/registration/hub/manifests/rbac/managedcluster-work-clusterrole.yaml(1 hunks)pkg/registration/register/csr/csr.go(6 hunks)pkg/registration/register/csr/csr_test.go(1 hunks)pkg/registration/register/factory/options.go(3 hunks)pkg/registration/register/factory/options_test.go(2 hunks)pkg/registration/register/grpc/hub_driver.go(1 hunks)pkg/registration/register/grpc/hub_driver_test.go(1 hunks)pkg/registration/register/grpc/options.go(1 hunks)pkg/registration/register/grpc/options_test.go(1 hunks)pkg/registration/register/grpc/spoke_deriver_test.go(1 hunks)pkg/registration/register/grpc/spoke_driver.go(1 hunks)pkg/server/grpc/server.go(1 hunks)pkg/server/services/addon/addon.go(1 hunks)pkg/server/services/addon/addon_test.go(1 hunks)pkg/server/services/cluster/cluster.go(1 hunks)pkg/server/services/cluster/cluster_test.go(1 hunks)pkg/server/services/csr/csr.go(1 hunks)pkg/server/services/csr/csr_test.go(1 hunks)pkg/server/services/event/event.go(1 hunks)pkg/server/services/event/event_test.go(1 hunks)pkg/server/services/lease/lease.go(1 hunks)pkg/server/services/lease/lease_test.go(1 hunks)pkg/server/services/work/work.go(1 hunks)pkg/server/services/work/work_test.go(1 hunks)vendor/modules.txt(3 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/wrapper.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/clientholder.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/wrapper.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/common/common.go(0 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/clientholder.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/wrapper.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/client/manifestwork.go(2 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/source/client/manifestwork.go(3 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.go(2 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/interface.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/mtls.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/token.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/interface.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/kube/sar.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/options.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/server.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/interface.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/store.go(1 hunks)
💤 Files with no reviewable changes (1)
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/common/common.go
✅ Files skipped from review due to trivial changes (1)
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/clientholder.go
🚧 Files skipped from review as they are similar to previous changes (47)
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/client/manifestwork.go
- pkg/registration/register/factory/options_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.go
- pkg/registration/hub/manifests/rbac/managedcluster-work-clusterrole.yaml
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/interface.go
- cmd/registration/main.go
- pkg/registration/hub/manifests/rbac/managedcluster-registration-clusterrole.yaml
- pkg/registration/register/grpc/options_test.go
- pkg/common/helpers/constants.go
- go.mod
- pkg/registration/register/grpc/options.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/source/client/manifestwork.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/interface.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/store.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/mtls.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/options.go
- pkg/server/grpc/server.go
- pkg/registration/register/grpc/hub_driver_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/codec.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/clientholder.go
- pkg/registration/register/factory/options.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/interface.go
- vendor/modules.txt
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/token.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/codec.go
- pkg/server/services/addon/addon_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/client.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/codec.go
- pkg/server/services/cluster/cluster.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/wrapper.go
- pkg/server/services/event/event.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go
- pkg/server/services/csr/csr.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/wrapper.go
- pkg/registration/register/csr/csr.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/kube/sar.go
- pkg/server/services/lease/lease.go
- pkg/server/services/lease/lease_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/codec.go
- pkg/server/services/csr/csr_test.go
- pkg/server/services/work/work.go
- pkg/registration/hub/manager.go
- pkg/registration/register/grpc/hub_driver.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/codec.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/client.go
- pkg/server/services/work/work_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/wrapper.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
pkg/registration/register/csr/csr_test.go (1)
pkg/registration/helpers/testing/testinghelpers.go (1)
NewKubeconfig(387-428)
pkg/registration/register/grpc/spoke_deriver_test.go (6)
pkg/registration/helpers/testing/testinghelpers.go (1)
NewTestCert(508-512)pkg/registration/register/interface.go (1)
SecretOption(27-50)pkg/registration/register/grpc/spoke_driver.go (2)
NewGRPCDriver(53-63)GRPCDriver(43-48)pkg/registration/register/csr/options.go (1)
NewCSROption(45-47)pkg/registration/register/grpc/options.go (1)
Option(10-13)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.go (1)
GRPCOptions(115-117)
pkg/server/services/addon/addon.go (7)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/codec.go (3)
ManagedClusterAddOnCodec(20-20)NewManagedClusterAddOnCodec(22-24)ManagedClusterAddOnEventDataType(13-17)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/store.go (1)
Service(13-25)pkg/common/helpers/constants.go (1)
CloudEventsKubeSource(10-10)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go (5)
CloudEventsType(170-179)CloudEventsDataType(159-163)ListOptions(145-156)ParseCloudEventsType(203-224)UpdateRequestAction(47-47)vendor/sigs.k8s.io/controller-runtime/pkg/client/options.go (1)
UpdateOptions(725-757)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/interface.go (1)
EventHandler(27-36)vendor/k8s.io/client-go/tools/cache/controller.go (1)
ResourceEventHandlerFuncs(237-241)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: integration
- GitHub Check: verify
- GitHub Check: unit
- GitHub Check: e2e-hosted
- GitHub Check: e2e-singleton
- GitHub Check: e2e
🔇 Additional comments (7)
pkg/server/services/cluster/cluster_test.go (1)
1-296: Well-structured test coverage!The test file provides comprehensive coverage for all ClusterService methods with proper test cases for success and error scenarios.
pkg/registration/register/csr/csr_test.go (1)
391-400: LGTM!The new test case correctly validates that a kubeconfig with a mismatched cluster name in the certificate is identified as invalid.
pkg/server/services/addon/addon.go (1)
93-97: Handle registration errors properly.The error from
AddEventHandleris only logged but not handled. While the method signature doesn't allow returning an error, consider whether registration failure should be treated more seriously.#!/bin/bash # Description: Check how other services handle AddEventHandler errors # Expected: Find consistent error handling patterns across services # Search for RegisterHandler implementations in other services ast-grep --pattern 'func ($_ $_) RegisterHandler(handler $_) { $$$ }' # Search for AddEventHandler usage patterns rg -A 5 "AddEventHandler.*err.*:="vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.go (4)
1-22: LGTM! Package structure and imports are well-organized.The imports are properly organized and all appear to be necessary for the client implementation.
24-46: Well-designed client structure with proper interface compliance.The client struct has appropriate fields and the constructor follows good patterns. The interface compliance verification at line 31 is a best practice.
48-67: Appropriate read-only design for CloudEvents-based client.The method not supported errors are consistent and appropriate for a CloudEvents-based client architecture where direct mutations are not supported.
107-143: Well-implemented status patch operation with proper CloudEvents integration.The Patch method correctly:
- Validates that only status subresource patches are allowed
- Applies patches using the utility function
- Publishes CloudEvents for the update
- Handles errors appropriately
The restriction to status-only patches aligns with the CloudEvents architecture where direct resource mutations go through event publishing.
| } | ||
|
|
||
| if err := e.cloudEventsClient.Publish(ctx, eventType, event); err != nil { | ||
| return nil, cloudeventserrors.ToStatusError(common.CSRGR, event.Name, err) |
There was a problem hiding this comment.
Fix incorrect resource group in error handling
The error handling uses common.CSRGR which appears to be for Certificate Signing Requests, not events. This should use the correct event resource group.
Apply this diff to fix the resource group:
- return nil, cloudeventserrors.ToStatusError(common.CSRGR, event.Name, err)
+ return nil, cloudeventserrors.ToStatusError(eventv1.Resource("events"), event.Name, err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return nil, cloudeventserrors.ToStatusError(common.CSRGR, event.Name, err) | |
| return nil, cloudeventserrors.ToStatusError(eventv1.Resource("events"), event.Name, err) |
🤖 Prompt for AI Agents
In
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/client.go
at line 61, the error handling incorrectly uses common.CSRGR as the resource
group, which is meant for Certificate Signing Requests. Replace common.CSRGR
with the correct event resource group constant to accurately reflect the
resource involved in the error. This will ensure proper error categorization and
handling.
| addon := &addonv1alpha1.ManagedClusterAddOn{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "test-addon", Namespace: "test-namespace"}, | ||
| } | ||
| evt.SetData(cloudevents.ApplicationJSON, addon) | ||
| return &evt |
There was a problem hiding this comment.
Fix incorrect test data type
The test is for event service but uses ManagedClusterAddOn instead of eventsv1.Event. This should use the correct event type to properly test the invalid action scenario.
Apply this diff to fix the test data:
- addon := &addonv1alpha1.ManagedClusterAddOn{
- ObjectMeta: metav1.ObjectMeta{Name: "test-addon", Namespace: "test-namespace"},
+ event := &eventsv1.Event{
+ ObjectMeta: metav1.ObjectMeta{Name: "test-event", Namespace: "test-namespace"},
}
- evt.SetData(cloudevents.ApplicationJSON, addon)
+ evt.SetData(cloudevents.ApplicationJSON, event)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| addon := &addonv1alpha1.ManagedClusterAddOn{ | |
| ObjectMeta: metav1.ObjectMeta{Name: "test-addon", Namespace: "test-namespace"}, | |
| } | |
| evt.SetData(cloudevents.ApplicationJSON, addon) | |
| return &evt | |
| event := &eventsv1.Event{ | |
| ObjectMeta: metav1.ObjectMeta{Name: "test-event", Namespace: "test-namespace"}, | |
| } | |
| evt.SetData(cloudevents.ApplicationJSON, event) | |
| return &evt |
🤖 Prompt for AI Agents
In pkg/server/services/event/event_test.go around lines 132 to 136, the test
incorrectly uses ManagedClusterAddOn as test data instead of the correct
eventsv1.Event type. Replace the ManagedClusterAddOn instance with an
eventsv1.Event object to properly simulate the event data and correctly test the
invalid action scenario.
| func NewManagedClusterClient( | ||
| cloudEventsClient *generic.CloudEventAgentClient[*clusterv1.ManagedCluster], | ||
| watcherStore store.ClientWatcherStore[*clusterv1.ManagedCluster], | ||
| clusterName string, | ||
| ) *ManagedClusterClient { | ||
| return &ManagedClusterClient{ | ||
| cloudEventsClient: cloudEventsClient, | ||
| watcherStore: watcherStore, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused clusterName parameter
The clusterName parameter in the constructor is not used. Consider removing it to avoid confusion.
Apply this diff to remove the unused parameter:
func NewManagedClusterClient(
cloudEventsClient *generic.CloudEventAgentClient[*clusterv1.ManagedCluster],
watcherStore store.ClientWatcherStore[*clusterv1.ManagedCluster],
- clusterName string,
) *ManagedClusterClient {
return &ManagedClusterClient{
cloudEventsClient: cloudEventsClient,
watcherStore: watcherStore,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func NewManagedClusterClient( | |
| cloudEventsClient *generic.CloudEventAgentClient[*clusterv1.ManagedCluster], | |
| watcherStore store.ClientWatcherStore[*clusterv1.ManagedCluster], | |
| clusterName string, | |
| ) *ManagedClusterClient { | |
| return &ManagedClusterClient{ | |
| cloudEventsClient: cloudEventsClient, | |
| watcherStore: watcherStore, | |
| } | |
| } | |
| func NewManagedClusterClient( | |
| cloudEventsClient *generic.CloudEventAgentClient[*clusterv1.ManagedCluster], | |
| watcherStore store.ClientWatcherStore[*clusterv1.ManagedCluster], | |
| ) *ManagedClusterClient { | |
| return &ManagedClusterClient{ | |
| cloudEventsClient: cloudEventsClient, | |
| watcherStore: watcherStore, | |
| } | |
| } |
🤖 Prompt for AI Agents
In
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.go
lines 33 to 42, the NewManagedClusterClient function has an unused parameter
clusterName. Remove the clusterName parameter from the function signature and
all calls to this constructor to avoid confusion and keep the code clean.
| var err error | ||
| ctx := ss.Context() | ||
| for _, authenticator := range authenticators { | ||
| ctx, err = authenticator.Authenticate(ctx) | ||
| if err == nil { | ||
| return handler(srv, newWrappedAuthStream(ctx, ss)) | ||
| } | ||
| } | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return handler(srv, newWrappedAuthStream(ctx, ss)) |
There was a problem hiding this comment.
Stream authentication interceptor has the same issue
Similar to the unary interceptor, the stream interceptor allows unauthenticated requests when no authenticators are configured.
Apply this diff to ensure authentication is required:
var err error
ctx := ss.Context()
for _, authenticator := range authenticators {
ctx, err = authenticator.Authenticate(ctx)
if err == nil {
return handler(srv, newWrappedAuthStream(ctx, ss))
}
}
- if err != nil {
- return err
- }
-
- return handler(srv, newWrappedAuthStream(ctx, ss))
+ if err == nil && len(authenticators) == 0 {
+ return fmt.Errorf("no authenticators configured")
+ }
+ return err📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var err error | |
| ctx := ss.Context() | |
| for _, authenticator := range authenticators { | |
| ctx, err = authenticator.Authenticate(ctx) | |
| if err == nil { | |
| return handler(srv, newWrappedAuthStream(ctx, ss)) | |
| } | |
| } | |
| if err != nil { | |
| return err | |
| } | |
| return handler(srv, newWrappedAuthStream(ctx, ss)) | |
| var err error | |
| ctx := ss.Context() | |
| for _, authenticator := range authenticators { | |
| ctx, err = authenticator.Authenticate(ctx) | |
| if err == nil { | |
| return handler(srv, newWrappedAuthStream(ctx, ss)) | |
| } | |
| } | |
| if err == nil && len(authenticators) == 0 { | |
| return fmt.Errorf("no authenticators configured") | |
| } | |
| return err |
🤖 Prompt for AI Agents
In
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/server.go
around lines 222 to 235, the stream authentication interceptor incorrectly
allows unauthenticated requests if no authenticators are configured. To fix
this, modify the logic to require authentication by returning an error when no
authenticators succeed, rather than allowing the request to proceed. Ensure that
if the authenticators slice is empty or none authenticate successfully, the
handler is not called and an authentication error is returned instead.
| expectedErr: false, | ||
| }, | ||
| { | ||
| name: "load bootstrap config", |
There was a problem hiding this comment.
Fix duplicate test case name.
Both test cases on lines 123 and 129 have the same name "load bootstrap config", which makes it harder to identify which test failed.
Apply this diff to fix the test case name:
- name: "load bootstrap config",
+ name: "load config",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: "load bootstrap config", | |
| name: "load config", |
🤖 Prompt for AI Agents
In pkg/registration/register/grpc/spoke_deriver_test.go at line 129, the test
case name "load bootstrap config" is duplicated from line 123, causing confusion
in test failure identification. Rename the test case at line 129 to a unique and
descriptive name that clearly differentiates it from the one on line 123.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.go (1)
33-42: Remove unused clusterName parameterpkg/registration/register/grpc/spoke_driver.go (1)
160-167: Add type assertion safety check to prevent potential panic.vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/server.go (2)
142-155: Improve error handling in authentication interceptor
222-235: Stream authentication interceptor has the same issuevendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go (2)
237-319: Refactor the respondResyncSpecRequest method to improve readability.This method remains overly complex with 82+ lines handling multiple responsibilities despite previous review feedback. The method decodes requests, lists resources, processes version comparisons, and manages deletions.
Consider extracting the resource processing logic into smaller helper methods as previously suggested to improve maintainability and readability.
371-426: Consolidate the OnCreate, OnUpdate, and OnDelete methods to reduce code duplication.These three methods follow identical patterns and remain duplicated despite previous review feedback. This violates the DRY principle and makes maintenance harder.
Consider implementing the previously suggested helper method to eliminate the code duplication:
+// handleResourceEvent is a common method for handling resource lifecycle events. +func (bkr *GRPCBroker) handleResourceEvent(ctx context.Context, t types.CloudEventsDataType, id string, action types.EventAction) error { + service, ok := bkr.services[t] + if !ok { + return fmt.Errorf("failed to find service for event type %s", t) + } + + resource, err := service.Get(ctx, id) + // if the resource is not found, it indicates the resource has been processed. + if errors.IsNotFound(err) { + return nil + } + if err != nil { + return err + } + + return bkr.handleRes(ctx, resource, t, action) +} -// OnCreate is called by the controller when a resource is created on the maestro server. -func (bkr *GRPCBroker) OnCreate(ctx context.Context, t types.CloudEventsDataType, id string) error { - service, ok := bkr.services[t] - if !ok { - return fmt.Errorf("failed to find service for event type %s", t) - } - - resource, err := service.Get(ctx, id) - // if the resource is not found, it indicates the resource has been processed. - if errors.IsNotFound(err) { - return nil - } - if err != nil { - return err - } - - return bkr.handleRes(ctx, resource, t, "create_request") -} +// OnCreate is called by the controller when a resource is created on the maestro server. +func (bkr *GRPCBroker) OnCreate(ctx context.Context, t types.CloudEventsDataType, id string) error { + return bkr.handleResourceEvent(ctx, t, id, "create_request") +} // Apply similar changes to OnUpdate and OnDelete
🧹 Nitpick comments (7)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.go (1)
60-60: Address the TODO comment for ManagedCluster validation.The TODO comment indicates that validation is missing for the ManagedCluster before publishing the create event. Consider implementing validation to ensure data integrity.
Do you want me to help generate validation logic for the ManagedCluster resource?
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.go (2)
83-83: Fix inconsistent log message naming.The log message uses "ManagedClusterAddon" while other places use "ManagedClusterAddOn" (with capital O). Consider using consistent naming throughout.
- klog.V(4).Info("list ManagedClusterAddon") + klog.V(4).Info("list ManagedClusterAddOn")
109-109: Fix inconsistent log message naming.Similar to the List method, use consistent naming for the resource type.
- klog.V(4).Infof("patching ManagedClusterAddon %s/%s", c.namespace, name) + klog.V(4).Infof("patching ManagedClusterAddOn %s/%s", c.namespace, name)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/kube/sar.go (1)
82-126: Consider adding validation for cluster name in SAR construction.The function doesn't validate that
clusterNameis non-empty, which could lead to invalid SAR requests with empty namespace.func toSubjectAccessReview(clusterName string, user string, groups []string, eventsType types.CloudEventsType) (*authv1.SubjectAccessReview, error) { + if clusterName == "" { + return nil, fmt.Errorf("cluster name cannot be empty") + } + verb, err := toVerb(eventsType.Action) if err != nil { return nil, err }pkg/registration/register/grpc/spoke_driver.go (1)
227-270: Consider extracting configuration loading into a separate method.The
loadConfigmethod is quite long and handles multiple concerns. Consider extracting the YAML marshaling logic into a separate helper method for better readability.+func (d *GRPCDriver) marshalGRPCConfig(grpcConfig *grpc.GRPCConfig, secretOption register.SecretOption) ([]byte, error) { + return yaml.Marshal(&grpc.GRPCConfig{ + CertConfig: cert.CertConfig{ + CAData: grpcConfig.CAData, + ClientKeyFile: path.Join(secretOption.HubKubeconfigDir, csr.TLSKeyFile), + ClientCertFile: path.Join(secretOption.HubKubeconfigDir, csr.TLSCertFile), + }, + URL: grpcConfig.URL, + KeepAliveConfig: grpcConfig.KeepAliveConfig, + }) +}vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/server.go (1)
247-261: Consider adding bounds checking in RecvMsg.The
RecvMsgmethod modifies the message fields but doesn't validate that the cached request is valid before copying values.func (c *wrappedAuthorizedStream) RecvMsg(m any) error { c.Lock() defer c.Unlock() + if c.authorizedReq == nil { + return fmt.Errorf("no authorized request cached") + } + msg, ok := m.(*pbv1.SubscriptionRequest) if !ok { return fmt.Errorf("unsupported request type %T", m) }vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go (1)
262-262: Fix grammatical error.There's a grammatical error in the log message.
- log.V(4).Info("there are is no objs from the list, do nothing") + log.V(4).Info("there are no objs from the list, do nothing")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (60)
cmd/registration/main.go(2 hunks)go.mod(3 hunks)pkg/common/helpers/constants.go(1 hunks)pkg/registration/hub/manager.go(4 hunks)pkg/registration/hub/manifests/rbac/managedcluster-registration-clusterrole.yaml(2 hunks)pkg/registration/hub/manifests/rbac/managedcluster-work-clusterrole.yaml(1 hunks)pkg/registration/register/csr/csr.go(6 hunks)pkg/registration/register/csr/csr_test.go(1 hunks)pkg/registration/register/factory/options.go(3 hunks)pkg/registration/register/factory/options_test.go(2 hunks)pkg/registration/register/grpc/hub_driver.go(1 hunks)pkg/registration/register/grpc/hub_driver_test.go(1 hunks)pkg/registration/register/grpc/options.go(1 hunks)pkg/registration/register/grpc/options_test.go(1 hunks)pkg/registration/register/grpc/spoke_deriver_test.go(1 hunks)pkg/registration/register/grpc/spoke_driver.go(1 hunks)pkg/server/grpc/server.go(1 hunks)pkg/server/services/addon/addon.go(1 hunks)pkg/server/services/addon/addon_test.go(1 hunks)pkg/server/services/cluster/cluster.go(1 hunks)pkg/server/services/cluster/cluster_test.go(1 hunks)pkg/server/services/csr/csr.go(1 hunks)pkg/server/services/csr/csr_test.go(1 hunks)pkg/server/services/event/event.go(1 hunks)pkg/server/services/event/event_test.go(1 hunks)pkg/server/services/lease/lease.go(1 hunks)pkg/server/services/lease/lease_test.go(1 hunks)pkg/server/services/work/work.go(1 hunks)pkg/server/services/work/work_test.go(1 hunks)vendor/modules.txt(3 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/wrapper.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/clientholder.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/wrapper.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/common/common.go(0 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/clientholder.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/wrapper.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/client/manifestwork.go(2 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/source/client/manifestwork.go(3 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.go(2 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/interface.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/mtls.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/token.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/interface.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/kube/sar.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/options.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/server.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/interface.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/store.go(1 hunks)
💤 Files with no reviewable changes (1)
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/common/common.go
✅ Files skipped from review due to trivial changes (1)
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/client.go
🚧 Files skipped from review as they are similar to previous changes (52)
- pkg/registration/register/factory/options_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.go
- cmd/registration/main.go
- pkg/registration/hub/manifests/rbac/managedcluster-registration-clusterrole.yaml
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/client/manifestwork.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/source/client/manifestwork.go
- pkg/registration/register/grpc/options_test.go
- go.mod
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/interface.go
- pkg/registration/hub/manifests/rbac/managedcluster-work-clusterrole.yaml
- pkg/common/helpers/constants.go
- pkg/registration/register/grpc/hub_driver_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/mtls.go
- pkg/registration/register/grpc/options.go
- pkg/registration/register/csr/csr_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/store.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/options.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/interface.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/clientholder.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/token.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/codec.go
- pkg/server/grpc/server.go
- vendor/modules.txt
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/wrapper.go
- pkg/registration/register/factory/options.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/interface.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/wrapper.go
- pkg/registration/register/grpc/spoke_deriver_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/clientholder.go
- pkg/registration/register/grpc/hub_driver.go
- pkg/server/services/event/event_test.go
- pkg/server/services/work/work_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/codec.go
- pkg/server/services/addon/addon.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/codec.go
- pkg/registration/hub/manager.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/codec.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go
- pkg/server/services/event/event.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/codec.go
- pkg/server/services/cluster/cluster.go
- pkg/registration/register/csr/csr.go
- pkg/server/services/addon/addon_test.go
- pkg/server/services/csr/csr_test.go
- pkg/server/services/csr/csr.go
- pkg/server/services/lease/lease_test.go
- pkg/server/services/cluster/cluster_test.go
- pkg/server/services/lease/lease.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/wrapper.go
- pkg/server/services/work/work.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/client.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/client.go
🧰 Additional context used
🧠 Learnings (1)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go (1)
Learnt from: skeeey
PR: open-cluster-management-io/ocm#1053
File: pkg/server/services/addon/addon_test.go:208-225
Timestamp: 2025-06-27T09:46:24.557Z
Learning: In the Open Cluster Management addon service, deletion actions are not expected to occur. The OnDelete handler method is implemented as a no-op, and test coverage for deletion is not needed since this operation is not part of the service's expected functionality.
🧬 Code Graph Analysis (3)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.go (8)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/store/interface.go (1)
ClientWatcherStore(32-63)vendor/open-cluster-management.io/api/client/cluster/clientset/versioned/typed/cluster/v1/managedcluster.go (1)
ManagedClusterInterface(23-35)vendor/k8s.io/klog/v2/klog.go (2)
V(1353-1355)Infof(1512-1514)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/common/common.go (1)
ManagedClusterGR(36-36)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go (7)
CloudEventsType(170-179)CloudEventsDataType(159-163)SubResourceSpec(27-27)CreateRequestAction(44-44)ListOptions(145-156)SubResourceStatus(30-30)UpdateRequestAction(47-47)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/codec.go (1)
ManagedClusterEventDataType(14-18)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/errors/errors.go (1)
ToStatusError(18-30)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/utils/utils.go (1)
IsStatusPatch(245-255)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go (7)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go (12)
CloudEventsDataType(159-163)ParseCloudEventsType(203-224)ResyncRequestAction(38-38)ParseCloudEventsDataType(187-198)ExtensionClusterName(71-71)ListOptions(145-156)ExtensionDeletionTimestamp(68-68)ExtensionResourceVersion(61-61)CloudEventsType(170-179)SubResourceSpec(27-27)EventAction(34-34)ExtensionResourceID(58-58)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/interface.go (1)
AgentEventServer(14-25)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/store.go (1)
Service(13-25)vendor/github.com/google/uuid/version4.go (1)
NewString(21-23)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/write_message.go (1)
WritePBMessage(20-32)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/payload/payload.go (2)
DecodeSpecResyncRequest(32-39)ResourceVersion(10-13)vendor/google.golang.org/grpc/status/status.go (1)
FromError(96-127)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.go (7)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/store/interface.go (1)
ClientWatcherStore(32-63)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/wrapper.go (1)
ManagedClusterAddOnInterface(59-68)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/common/common.go (1)
ManagedClusterAddOnGR(45-45)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go (5)
ListOptions(145-156)CloudEventsType(170-179)CloudEventsDataType(159-163)SubResourceStatus(30-30)UpdateRequestAction(47-47)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/codec.go (1)
ManagedClusterAddOnEventDataType(13-17)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/utils/utils.go (1)
IsStatusPatch(245-255)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/errors/errors.go (1)
ToStatusError(18-30)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: verify
- GitHub Check: integration
- GitHub Check: unit
- GitHub Check: e2e-singleton
- GitHub Check: e2e
- GitHub Check: e2e-hosted
🔇 Additional comments (13)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/kube/sar.go (1)
23-31: LGTM! Clean authorization interface implementation.The SAR authorizer properly implements the authorization interface with appropriate error handling and resource mapping.
pkg/registration/register/grpc/spoke_driver.go (1)
53-63: LGTM! Constructor properly initializes the GRPC driver.The constructor correctly sets up the CSR driver with the appropriate signer and handles errors properly.
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/server.go (1)
86-90: Excellent security configuration with TLS 1.3 enforcement.The TLS configuration properly enforces TLS 1.3 as both minimum and maximum version, providing strong security. The certificate loading and validation logic is also well implemented.
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go (10)
1-29: LGTM!The package declaration and imports are well-organized and include all necessary dependencies for the gRPC CloudEvents broker implementation.
31-51: LGTM!The type definitions are well-structured. The
subscriberstruct appropriately encapsulates subscription data, and theGRPCBrokerproperly implements the required interfaces.
53-67: LGTM!The constructor properly initializes all fields, and
RegisterServicecorrectly establishes the bidirectional relationship between the broker and services.
69-79: LGTM!The
Subscribersmethod properly uses mutex locking for thread safety and correctly collects unique cluster names from the subscribers map.
81-99: LGTM!The
Startmethod correctly implements graceful shutdown by callingGracefulStop()when the context is cancelled, addressing the previous review feedback.
101-142: LGTM!The
Publishmethod correctly handles protobuf to CloudEvent conversion, routes resync requests appropriately, and uses proper gRPC status codes for error handling.
144-174: LGTM!The
registerandunregistermethods are thread-safe and the previous issue with potential panic inunregisterhas been properly addressed by checking subscriber existence before closing the channel.
176-227: LGTM!The
Subscribemethod correctly handles gRPC streaming with proper validation, error handling, and subscriber cleanup. The CloudEvent to protobuf conversion is handled appropriately.
321-369: LGTM!The
handleResmethod properly handles resource publishing with appropriate thread safety, error handling for unrecoverable gRPC errors, and subscriber validation.
428-462: LGTM!The utility methods are well-implemented with proper thread safety in
IsConsumerSubscribedand the previous type assertion issue ingetObjhas been properly addressed with safe type checking.
Signed-off-by: Wei Liu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.go (1)
33-42: Remove unused clusterName parameterThe
clusterNameparameter in the constructor is not used. Consider removing it to avoid confusion.vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/server.go (2)
142-155: Authentication interceptor still allows unauthenticated requestsThe same issue identified in previous reviews persists: when no authenticators are configured, the interceptor allows unauthenticated requests to proceed.
222-235: Stream authentication interceptor has the same unresolved issueThis interceptor continues to allow unauthenticated requests when no authenticators are configured, as identified in previous reviews.
pkg/registration/register/grpc/spoke_driver.go (1)
160-167: Type assertion safety check still neededThe unsafe type assertion identified in previous reviews remains unaddressed and could cause a panic if the Fork method returns an unexpected type.
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go (2)
372-426: Code duplication in lifecycle methods persistsThe OnCreate, OnUpdate, and OnDelete methods continue to follow an identical pattern as identified in previous reviews, making maintenance harder and increasing the risk of inconsistent behavior.
237-319: Complex resync method still needs refactoringThe respondResyncSpecRequest method remains lengthy and handles multiple responsibilities as noted in previous reviews, impacting readability and maintainability.
🧹 Nitpick comments (4)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.go (2)
60-60: Missing validation for ManagedClusterThe TODO comment indicates that validation is missing. This should be implemented to ensure data integrity before publishing events.
Would you like me to generate validation logic for the ManagedCluster or open an issue to track this?
106-112: Optimize list method to avoid unnecessary copyingThe current implementation creates a new slice and copies each item, which is inefficient for large lists.
- items := []clusterv1.ManagedCluster{} - for _, cluster := range clusterList.Items { - items = append(items, *cluster) - } - - return &clusterv1.ManagedClusterList{ListMeta: clusterList.ListMeta, Items: items}, nil + return &clusterv1.ManagedClusterList{ListMeta: clusterList.ListMeta, Items: clusterList.Items}, nilpkg/server/grpc/server.go (1)
126-141: Consider making informer resync period configurableAll informers use a hardcoded 30-minute resync period. Different deployment scenarios might require different resync intervals.
Consider adding a configuration option for the resync period:
+const defaultResyncPeriod = 30 * time.Minute + type clients struct { kubeClient kubernetes.Interface clusterClient clusterv1client.Interface workClient workclientset.Interface addonClient addonv1alpha1client.Interface kubeInformers kubeinformers.SharedInformerFactory clusterInformers clusterv1informers.SharedInformerFactory workInformers workinformers.SharedInformerFactory addonInformers addoninformers.SharedInformerFactory + resyncPeriod time.Duration } -func newClients(controllerContext *controllercmd.ControllerContext) (*clients, error) { +func newClients(controllerContext *controllercmd.ControllerContext, resyncPeriod time.Duration) (*clients, error) { + if resyncPeriod == 0 { + resyncPeriod = defaultResyncPeriod + }Then use
resyncPeriodinstead of30*time.Minutein the informer factory constructors.vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/wrapper.go (1)
23-52: Consider returning errors instead of panicking for unsupported methodsWhile panicking clearly indicates these methods are unsupported, it could cause unexpected crashes. Consider returning errors for more graceful error handling.
Example refactor for one method:
func (a AddonClientSetWrapper) Discovery() discovery.DiscoveryInterface { - panic("Discovery is unsupported") + return &unsupportedDiscovery{} } +type unsupportedDiscovery struct{} + +func (u *unsupportedDiscovery) ServerGroups() (*metav1.APIGroupList, error) { + return nil, errors.NewMethodNotSupported(schema.GroupResource{}, "serverGroups") +} +// ... implement other discovery.DiscoveryInterface methods returning appropriate errorsAlternatively, if these interfaces are never expected to be called, document this clearly in the package documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (60)
cmd/registration/main.go(2 hunks)go.mod(3 hunks)pkg/common/helpers/constants.go(1 hunks)pkg/registration/hub/manager.go(4 hunks)pkg/registration/hub/manifests/rbac/managedcluster-registration-clusterrole.yaml(2 hunks)pkg/registration/hub/manifests/rbac/managedcluster-work-clusterrole.yaml(1 hunks)pkg/registration/register/csr/csr.go(6 hunks)pkg/registration/register/csr/csr_test.go(1 hunks)pkg/registration/register/factory/options.go(3 hunks)pkg/registration/register/factory/options_test.go(2 hunks)pkg/registration/register/grpc/hub_driver.go(1 hunks)pkg/registration/register/grpc/hub_driver_test.go(1 hunks)pkg/registration/register/grpc/options.go(1 hunks)pkg/registration/register/grpc/options_test.go(1 hunks)pkg/registration/register/grpc/spoke_deriver_test.go(1 hunks)pkg/registration/register/grpc/spoke_driver.go(1 hunks)pkg/server/grpc/server.go(1 hunks)pkg/server/services/addon/addon.go(1 hunks)pkg/server/services/addon/addon_test.go(1 hunks)pkg/server/services/cluster/cluster.go(1 hunks)pkg/server/services/cluster/cluster_test.go(1 hunks)pkg/server/services/csr/csr.go(1 hunks)pkg/server/services/csr/csr_test.go(1 hunks)pkg/server/services/event/event.go(1 hunks)pkg/server/services/event/event_test.go(1 hunks)pkg/server/services/lease/lease.go(1 hunks)pkg/server/services/lease/lease_test.go(1 hunks)pkg/server/services/work/work.go(1 hunks)pkg/server/services/work/work_test.go(1 hunks)vendor/modules.txt(3 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/wrapper.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/clientholder.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/wrapper.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/common/common.go(0 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/clientholder.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/wrapper.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/client.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/codec.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/client/manifestwork.go(2 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/source/client/manifestwork.go(3 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.go(2 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/interface.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/mtls.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/token.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/interface.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/kube/sar.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/options.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/server.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/interface.go(1 hunks)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/store.go(1 hunks)
💤 Files with no reviewable changes (1)
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/common/common.go
✅ Files skipped from review due to trivial changes (2)
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/clientholder.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go
🚧 Files skipped from review as they are similar to previous changes (48)
- pkg/registration/register/csr/csr_test.go
- pkg/registration/hub/manifests/rbac/managedcluster-work-clusterrole.yaml
- pkg/registration/register/factory/options_test.go
- pkg/registration/hub/manifests/rbac/managedcluster-registration-clusterrole.yaml
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/source/client/manifestwork.go
- pkg/common/helpers/constants.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/protocol.go
- cmd/registration/main.go
- go.mod
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/interface.go
- pkg/registration/register/grpc/hub_driver_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/client/manifestwork.go
- pkg/registration/register/grpc/options_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/store.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/interface.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/mtls.go
- pkg/registration/register/grpc/options.go
- vendor/modules.txt
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authn/token.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/clientholder.go
- pkg/registration/register/grpc/spoke_deriver_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options/options.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/codec.go
- pkg/registration/register/factory/options.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/wrapper.go
- pkg/server/services/event/event_test.go
- pkg/registration/hub/manager.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/wrapper.go
- pkg/registration/register/grpc/hub_driver.go
- pkg/server/services/addon/addon_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/codec.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/codec.go
- pkg/server/services/event/event.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/codec.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/codec.go
- pkg/server/services/cluster/cluster.go
- pkg/server/services/work/work_test.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/client.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/interface.go
- pkg/server/services/lease/lease_test.go
- pkg/server/services/csr/csr.go
- pkg/server/services/lease/lease.go
- pkg/server/services/addon/addon.go
- pkg/server/services/csr/csr_test.go
- pkg/server/services/work/work.go
- pkg/registration/register/csr/csr.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/client.go
- vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/client.go
🧰 Additional context used
🧠 Learnings (2)
pkg/server/services/cluster/cluster_test.go (1)
Learnt from: skeeey
PR: open-cluster-management-io/ocm#1053
File: pkg/server/services/addon/addon_test.go:208-225
Timestamp: 2025-06-27T09:46:24.557Z
Learning: In the Open Cluster Management addon service, deletion actions are not expected to occur. The OnDelete handler method is implemented as a no-op, and test coverage for deletion is not needed since this operation is not part of the service's expected functionality.
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go (1)
Learnt from: skeeey
PR: open-cluster-management-io/ocm#1053
File: pkg/server/services/addon/addon_test.go:208-225
Timestamp: 2025-06-27T09:46:24.557Z
Learning: In the Open Cluster Management addon service, deletion actions are not expected to occur. The OnDelete handler method is implemented as a no-op, and test coverage for deletion is not needed since this operation is not part of the service's expected functionality.
🧬 Code Graph Analysis (2)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/wrapper.go (7)
vendor/k8s.io/client-go/discovery/discovery_client.go (1)
DiscoveryInterface(79-90)vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1alpha1/addon_client.go (1)
AddonV1alpha1Interface(13-19)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.go (2)
ManagedClusterAddOnClient(25-29)NewManagedClusterAddOnClient(33-41)vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1alpha1/addondeploymentconfig.go (1)
AddOnDeploymentConfigInterface(23-33)vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1alpha1/addontemplate.go (1)
AddOnTemplateInterface(23-33)vendor/open-cluster-management.io/api/client/addon/clientset/versioned/typed/addon/v1alpha1/clustermanagementaddon.go (1)
ClusterManagementAddOnInterface(23-35)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/options/generic.go (1)
GenericClientOptions(15-23)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/server.go (7)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/types/types.go (11)
CloudEventsDataType(159-163)ParseCloudEventsType(203-224)ParseCloudEventsDataType(187-198)ExtensionClusterName(71-71)ListOptions(145-156)ExtensionDeletionTimestamp(68-68)ExtensionResourceVersion(61-61)CloudEventsType(170-179)SubResourceSpec(27-27)EventAction(34-34)ExtensionResourceID(58-58)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/interface.go (1)
AgentEventServer(14-25)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/store.go (1)
Service(13-25)vendor/github.com/google/uuid/version4.go (1)
NewString(21-23)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/protocol/write_message.go (1)
WritePBMessage(20-32)vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/payload/payload.go (2)
DecodeSpecResyncRequest(32-39)ResourceVersion(10-13)vendor/google.golang.org/grpc/status/status.go (1)
FromError(96-127)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e-singleton
- GitHub Check: e2e
- GitHub Check: e2e-hosted
- GitHub Check: unit
- GitHub Check: integration
- GitHub Check: verify
🔇 Additional comments (4)
pkg/server/services/cluster/cluster_test.go (1)
1-296: Well-structured and comprehensive test coverageThe test implementation follows best practices with table-driven tests, proper error handling verification, and good coverage of both success and failure scenarios. The no-op OnDelete handler aligns with the expected pattern where deletion operations are not part of the service's functionality.
pkg/server/grpc/server.go (1)
41-91: Well-structured gRPC server implementationThe server setup follows good practices with proper separation of concerns, comprehensive authentication/authorization, and clean service registration pattern.
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/authz/kube/sar.go (1)
1-144: Robust SAR authorization implementationThe implementation properly handles type assertions safely and includes comprehensive error handling. The mapping of cloud event types to Kubernetes verbs and resources is well-structured.
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.go (1)
24-143: Well-designed CloudEvents client implementationThe client implementation properly restricts operations to read-only access and status updates, which aligns with the CloudEvents architectural pattern. The error handling for unsupported operations is consistent and appropriate.
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores