Fix validation service address #293
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
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
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (1)
pkg/mlsvalidate/service.go (1)
30-30: Maintain consistency in error message capitalizationError messages should follow consistent capitalization. Consider this change:
- return nil, fmt.Errorf("Failed to convert HTTP address to gRPC target: %v", err) + return nil, fmt.Errorf("failed to convert HTTP address to gRPC target: %v", err) - return nil, fmt.Errorf("Failed to get credentials: %v", err) + return nil, fmt.Errorf("failed to get credentials: %v", err)Also applies to: 35-35
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
pkg/mlsvalidate/service.go(2 hunks)pkg/server/server.go(1 hunks)pkg/server/server_test.go(2 hunks)pkg/utils/grpc.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- pkg/server/server_test.go
- pkg/utils/grpc.go
🔇 Additional comments (5)
pkg/server/server.go (2)
99-102: LGTM! Verify indexer integration with the validation service.
The addition of logging to the MLS validation service initialization looks good. The error handling is properly implemented, and the service is correctly integrated with the indexer when enabled.
Let's verify the integration points:
✅ Verification successful
Integration with validation service is properly implemented
The validation service is correctly integrated throughout the codebase:
- Properly initialized in the server with logging and configuration
- Correctly passed to the indexer's StartIndexer function
- Used in the IdentityUpdateStorer for validating association states
- Well-tested with proper mocking in test files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the validation service integration with indexer
# and configuration structure
# Check validation service usage in indexer
ast-grep --pattern 'StartIndexer($_) {
$$$
validationService
$$$
}'
# Verify MlsValidation options structure
ast-grep --pattern 'type MlsValidationOptions struct {
$$$
}'
Length of output: 387
Script:
#!/bin/bash
# Let's check how validationService is used in the server implementation
rg "validationService" -A 3 -B 3
# Also check for any indexer-related code that might use the validation service
ast-grep --pattern 'func ($_) StartIndexer($_) {
$$$
}'
Length of output: 5280
99-99: Verify GRPC address format in configuration.
Since this PR addresses GRPC target format changes, ensure that options.MlsValidation contains the correctly formatted GRPC address.
Let's check the configuration handling:
✅ Verification successful
Let me gather more information about the address conversion utility that's being used.
Let me check the complete implementation of the address conversion utility and its tests.
GRPC address format is properly handled
The implementation in pkg/utils/grpc.go correctly handles GRPC address conversion by:
- Parsing URLs with both HTTP and HTTPS schemes
- Properly extracting hostname and port information
- Setting TLS flag based on the scheme
- Handling empty schemes and ports appropriately
- Returning well-formatted GRPC target addresses
The configuration is properly utilized in pkg/mlsvalidate/service.go where it:
- Converts the HTTP address to GRPC target
- Sets up appropriate credentials based on TLS
- Logs the connection details for monitoring
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the GRPC address format handling
# Check for GRPC address format handling
rg -A 3 "MlsValidation.*Address|GrpcAddress"
# Look for any address conversion utilities
ast-grep --pattern 'func $_($_, address string) string {
$$$
}'
Length of output: 1272
Script:
#!/bin/bash
# Check the implementation of HttpAddressToGrpcTarget
rg -A 10 "HttpAddressToGrpcTarget"
# Look for any related tests
rg -A 5 "TestHttpAddressToGrpcTarget"
Length of output: 1874
Script:
#!/bin/bash
# Get the complete implementation of HttpAddressToGrpcTarget
ast-grep --pattern 'func HttpAddressToGrpcTarget(httpAddress string) (string, bool, error) {
$$$
}'
# Look for tests in a different way
rg -l "HttpAddressToGrpcTarget.*test"
Length of output: 1046
pkg/mlsvalidate/service.go (3)
14-15: LGTM: Import additions are appropriate
The new imports for utils and zap logger are necessary for the enhanced functionality.
24-41: LGTM: Improved GRPC connection handling with proper security
The changes correctly implement:
- GRPC target format conversion
- Proper TLS credential handling
- Comprehensive error handling
- Informative logging
28-31: Verify GRPC address format compatibility
Let's ensure the new GRPC target format is compatible with all existing configurations.
✅ Verification successful
Let me gather more information about the GRPC address format conversion utility.
Let me check the implementation details and test coverage of the GRPC address conversion utility.
GRPC address format is well-defined and safely implemented
The implementation of HttpAddressToGrpcTarget follows standard URL parsing and GRPC target format specifications. The function:
- Handles both HTTP and HTTPS schemes
- Properly manages empty schemes defaulting to non-TLS
- Correctly extracts hostname and port information
- Is used consistently across the codebase (mlsvalidate and registry services)
- Has clear error handling for invalid URLs and unknown schemes
The only test configuration found uses a valid format (http://localhost:60051), which is compatible with the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GRPC address configurations and usage
echo "Searching for GRPC address configurations..."
rg -A 2 "GrpcAddress"
echo "Searching for NewMlsValidationService usage..."
ast-grep --pattern 'NewMlsValidationService($$$)'
Length of output: 1240
Script:
#!/bin/bash
# Search for HttpAddressToGrpcTarget implementation and usage
echo "Searching for HttpAddressToGrpcTarget implementation..."
ast-grep --pattern 'HttpAddressToGrpcTarget($$$)'
echo "Searching for related test cases..."
rg -A 5 "HttpAddressToGrpcTarget"
Length of output: 1421
Script:
#!/bin/bash
# Search for the complete implementation and tests
echo "Searching for complete implementation in utils/grpc.go..."
cat pkg/utils/grpc.go
echo "Searching for related test files..."
fd "grpc_test.go" --exec cat {}
Length of output: 1567
|
do we need to fix the |
GRPC uses a different target format
Summary by CodeRabbit
New Features
Bug Fixes
Chores