fix(server/v2/grpc): fix reflection #23333
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the gRPC server reflection implementation in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (12)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/v2/api/grpc/gogoreflection/serverreflection.go (2)
461-491: Add documentation and improve error handling.While the implementation is correct, consider these improvements:
- Add documentation explaining the message-based service lookup strategy.
- Enhance error messages with more context about why the descriptor or service lookup failed.
+// getServices returns a unique list of services and their file descriptors based on the provided messages. +// It looks up each message in the registry and finds associated services by scanning method input/output types. func (s *serverReflectionServer) getServices(messages []string) (svcs []string, fds []*dpb.FileDescriptorProto) { registry, err := gogoproto.MergedRegistry() if err != nil { - s.log.Error("unable to load merged registry", "err", err) + s.log.Error("failed to load protobuf registry for reflection", "err", err) return nil, nil } seenSvc := map[protoreflect.FullName]struct{}{} for _, messageName := range messages { md, err := registry.FindDescriptorByName(protoreflect.FullName(messageName)) if err != nil { - s.log.Error("unable to load message descriptor", "message", messageName, "err", err) + s.log.Error("failed to find message descriptor in registry", + "message", messageName, + "err", err, + "hint", "ensure the message is properly registered") continue }
493-517: Add documentation and improve return values.The implementation is efficient, but consider these improvements:
- Add documentation explaining the function's purpose and return values.
- Consider returning (protoreflect.ServiceDescriptor, error) instead of a boolean for more idiomatic Go.
+// findServiceForMessage searches the registry for a service that uses the given message +// as either an input or output type in any of its methods. It returns the first matching +// service descriptor and true if found, or a nil descriptor and false if not found. func findServiceForMessage(registry *protoregistry.Files, messageDesc protoreflect.MessageDescriptor) (protoreflect.ServiceDescriptor, bool) { var ( service protoreflect.ServiceDescriptor found bool )Alternative implementation with error return:
-func findServiceForMessage(registry *protoregistry.Files, messageDesc protoreflect.MessageDescriptor) (protoreflect.ServiceDescriptor, bool) { +func findServiceForMessage(registry *protoregistry.Files, messageDesc protoreflect.MessageDescriptor) (protoreflect.ServiceDescriptor, error) { var service protoreflect.ServiceDescriptor registry.RangeFiles(func(fileDescriptor protoreflect.FileDescriptor) bool { // ... existing implementation ... }) - return service, found + if service == nil { + return nil, fmt.Errorf("no service found for message %s", messageDesc.FullName()) + } + return service, nil }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/v2/api/grpc/gogoreflection/serverreflection.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/api/grpc/gogoreflection/serverreflection.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (3)
server/v2/api/grpc/gogoreflection/serverreflection.go (3)
57-57: LGTM!The addition of the protoregistry import is necessary for the new message-based reflection implementation.
66-66: LGTM!The field rename from
methodstomessagesaccurately reflects the shift to message-based reflection.
75-80: LGTM!The Register function changes are consistent with the shift to message-based reflection, and the logger is properly initialized.
|
I will migrate this test: as a system test, as it was testing reflection but only for v0 apps. |
Adding the backport label only backporting the test changes |
julienrbrt
added
the
backport/v0.52.x
github-actions
bot
added
C:server/v2
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
tests/systemtests/grpc_test.go (2)
22-24: Consider making the gRPC endpoint configurable.The gRPC endpoint (
localhost:9090) is hardcoded. Consider making it configurable through environment variables or test configuration.+const ( + defaultGRPCHost = "localhost" + defaultGRPCPort = "9090" +) + ctx := context.Background() -grpcClient, err := grpc.NewClient("localhost:9090", grpc.WithTransportCredentials(insecure.NewCredentials())) +host := getEnvOrDefault("GRPC_HOST", defaultGRPCHost) +port := getEnvOrDefault("GRPC_PORT", defaultGRPCPort) +grpcClient, err := grpc.NewClient(fmt.Sprintf("%s:%s", host, port), grpc.WithTransportCredentials(insecure.NewCredentials()))
26-30: Consider adding more comprehensive service checks.The test only verifies one service. Consider adding checks for other critical services that should be available through reflection.
server/v2/api/grpc/gogoreflection/serverreflection.go (3)
461-489: Consider adding metrics for reflection service health.The error handling is good, but consider adding metrics to track:
- Number of failed message descriptor lookups
- Number of services not found for messages
- Total number of successful reflections
493-517: Consider optimizing service lookup performance.The
findServiceForMessagefunction iterates through all files and services. For large service registries, this could be inefficient. Consider:
- Caching service-to-message mappings
- Using a more efficient lookup structure
+type serviceCache struct { + sync.RWMutex + messageToService map[protoreflect.FullName]protoreflect.ServiceDescriptor +} + +func (s *serverReflectionServer) buildServiceCache(registry *protoregistry.Files) { + s.cache.Lock() + defer s.cache.Unlock() + + registry.RangeFiles(func(fd protoreflect.FileDescriptor) bool { + // Build cache + return true + }) +}
506-510: Consider adding debug logging for service matching.Adding debug logs here would help troubleshoot when messages don't match with any service.
if methodDesc.Input() == messageDesc || methodDesc.Output() == messageDesc { + s.log.Debug("found service for message", + "message", messageDesc.FullName(), + "service", serviceDesc.FullName(), + "method", methodDesc.FullName()) service = serviceDesc found = true return false }CHANGELOG.md (3)
Line range hint
1-1: Add title to the CHANGELOG fileThe CHANGELOG file should start with a clear title like "# Changelog" to follow standard changelog conventions.
+ # Changelog
Line range hint
44-48: Consider adding migration guide linksFor major breaking changes sections, consider adding links to migration guides to help users upgrade smoothly.
Line range hint
2960-2965: Add link to current changelogThe reference to previous changelog versions should include a link to the current changelog location for better navigation.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/systemtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
CHANGELOG.md(2 hunks)server/v2/api/grpc/gogoreflection/serverreflection.go(5 hunks)tests/integration/runtime/query_test.go(0 hunks)tests/systemtests/go.mod(6 hunks)tests/systemtests/grpc_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/runtime/query_test.go
🧰 Additional context used
📓 Path-based instructions (4)
tests/systemtests/grpc_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/systemtests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
server/v2/api/grpc/gogoreflection/serverreflection.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (4)
tests/systemtests/grpc_test.go (1)
23-23: Review security implications of using insecure credentials.The test uses
insecure.NewCredentials(). While this might be acceptable for testing, ensure this aligns with your security requirements.tests/systemtests/go.mod (1)
9-9: LGTM! Dependencies look appropriate.The added dependencies align well with the gRPC reflection testing requirements.
Also applies to: 30-30, 37-37, 67-68, 106-106, 126-126, 171-171
server/v2/api/grpc/gogoreflection/serverreflection.go (1)
66-66: LGTM! Field rename reflects the new message-based approach.The rename from
methodstomessagesbetter represents the new message-based reflection approach.CHANGELOG.md (1)
Line range hint
1-3000: LGTM on overall changelog structure and contentThe changelog follows good practices by:
- Organizing changes by type (Features, Bug Fixes, etc.)
- Including version numbers and dates
- Documenting breaking changes clearly
- Providing detailed descriptions of changes
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tests/systemtests/grpc_test.go (6)
11-11: Add explanation for the nolint directive.The
//nolint:staticcheckdirective should include a comment explaining why the staticcheck linter is being disabled for this import.- "github.com/fullstorydev/grpcurl" //nolint:staticcheck: input in grpcurl + "github.com/fullstorydev/grpcurl" //nolint:staticcheck // required for gRPC reflection testing despite staticcheck warning
24-24: Use timeout context for gRPC operations.Using a background context without timeout could lead to hanging tests if the gRPC server is unresponsive.
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel()
29-32: Enhance service verification.The test only verifies one service. Consider checking for other essential services to ensure complete reflection functionality.
services, err := grpcurl.ListServices(descSource) require.NoError(t, err) require.Greater(t, len(services), 0) require.Contains(t, services, "cosmos.staking.v1beta1.Query") + // Verify other essential services + essentialServices := []string{ + "cosmos.auth.v1beta1.Query", + "cosmos.bank.v1beta1.Query", + "grpc.reflection.v1alpha.ServerReflection", + } + for _, service := range essentialServices { + require.Contains(t, services, service, "Missing essential service: %s", service) + }
35-58: Improve query test robustness.The test could be enhanced in several ways:
- Add error case testing
- Make the response verification more specific
- Add cleanup for resources
Consider refactoring like this:
func TestGRPCQuery(t *testing.T) { + tests := []struct { + name string + method string + expectedError string + contains string + }{ + { + name: "valid params query", + method: "cosmos.staking.v1beta1.Query/Params", + contains: "max_validators", + }, + { + name: "invalid method", + method: "invalid.method", + expectedError: "not found", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + systest.Sut.ResetChain(t) + systest.Sut.StartChain(t) + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + grpcClient, err := grpc.NewClient("localhost:9090", grpc.WithTransportCredentials(insecure.NewCredentials())) + require.NoError(t, err) + defer grpcClient.Close() + + // ... rest of the setup code ... + + err = grpcurl.InvokeRPC(ctx, descSource, grpcClient, tc.method, nil, h, rf.Next) + if tc.expectedError != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tc.expectedError) + return + } + require.NoError(t, err) + require.Contains(t, buf.String(), tc.contains) + }) + } }
61-61: Enhance TODO comment with more context.The TODO comment should provide more context about why the autocli query needs to be re-added and what specific functionality is pending.
- t.Skip() // TODO(@julienrbrt): re-add autocli query in v2 in follow-up + t.Skip() // TODO(@julienrbrt): re-add autocli query functionality in v2 after implementing the new query interface - tracked in issue #XXXX
60-84: Reduce code duplication in test setup.The setup code is duplicated across all three test functions. Consider extracting common setup into a helper function.
+func setupGRPCTest(t *testing.T) (context.Context, *grpc.ClientConn, grpcurl.DescriptorSource, func()) { + systest.Sut.ResetChain(t) + systest.Sut.StartChain(t) + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + + grpcClient, err := grpc.NewClient("localhost:9090", grpc.WithTransportCredentials(insecure.NewCredentials())) + require.NoError(t, err) + + descSource := grpcurl.DescriptorSourceFromServer(ctx, grpcreflect.NewClientAuto(ctx, grpcClient)) + + cleanup := func() { + grpcClient.Close() + cancel() + } + + return ctx, grpcClient, descSource, cleanup +} func TestGRPCQueryAutoCLIOptions(t *testing.T) { t.Skip() // TODO: enhance comment as suggested above - systest.Sut.ResetChain(t) - systest.Sut.StartChain(t) - - ctx := context.Background() - grpcClient, err := grpc.NewClient("localhost:9090", grpc.WithTransportCredentials(insecure.NewCredentials())) - require.NoError(t, err) - - descSource := grpcurl.DescriptorSourceFromServer(ctx, grpcreflect.NewClientAuto(ctx, grpcClient)) + ctx, grpcClient, descSource, cleanup := setupGRPCTest(t) + defer cleanup()
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/systemtests/grpc_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/systemtests/grpc_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: build (amd64)
- GitHub Check: Summary
| grpcClient, err := grpc.NewClient("localhost:9090", grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for gRPC client.
The gRPC client connection should be properly closed after the test.
grpcClient, err := grpc.NewClient("localhost:9090", grpc.WithTransportCredentials(insecure.NewCredentials()))
require.NoError(t, err)
+ defer grpcClient.Close()📝 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.
| grpcClient, err := grpc.NewClient("localhost:9090", grpc.WithTransportCredentials(insecure.NewCredentials())) | |
| require.NoError(t, err) | |
| grpcClient, err := grpc.NewClient("localhost:9090", grpc.WithTransportCredentials(insecure.NewCredentials())) | |
| require.NoError(t, err) | |
| defer grpcClient.Close() |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/systemtests/grpc_test.go (1)
20-50: Consider adding error case tests.The test only covers the happy path. Consider adding test cases for:
- Invalid service names
- Invalid method names
- Server unavailable scenarios
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/v2/api/grpc/gogoreflection/serverreflection.go(5 hunks)tests/systemtests/grpc_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
tests/systemtests/grpc_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
server/v2/api/grpc/gogoreflection/serverreflection.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
server/v2/api/grpc/gogoreflection/serverreflection.go
491-491: naked return in func getServices with 31 lines of code
(nakedret)
🪛 GitHub Actions: Lint
server/v2/api/grpc/gogoreflection/serverreflection.go
[error] 491-491: naked return in func getServices with 31 lines of code (nakedret)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (3)
tests/systemtests/grpc_test.go (1)
52-76: 🛠️ Refactor suggestionAdd cleanup and reduce code duplication.
- Add
defer grpcClient.Close()after line 60 to properly clean up resources- Consider extracting the common setup code into a helper function to reduce duplication with TestGRPC
grpcClient, err := grpc.NewClient("localhost:9090", grpc.WithTransportCredentials(insecure.NewCredentials())) require.NoError(t, err) + defer grpcClient.Close()Likely invalid or redundant comment.
server/v2/api/grpc/gogoreflection/serverreflection.go (2)
494-518: LGTM!The implementation is clean and efficient, with appropriate early returns when a service is found.
75-81: LGTM!The function is simple and correctly registers the reflection server with the provided messages.
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit