feat: support secure grpc connections #576
Conversation
WalkthroughThe changes introduced in this diff primarily revolve around the addition of a new flag Changes
Poem
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (19)
- cmd/blobstream/base/config.go (2 hunks)
- cmd/blobstream/common/helpers.go (2 hunks)
- cmd/blobstream/deploy/cmd.go (1 hunks)
- cmd/blobstream/deploy/config.go (4 hunks)
- cmd/blobstream/orchestrator/cmd.go (1 hunks)
- cmd/blobstream/orchestrator/config.go (4 hunks)
- cmd/blobstream/query/cmd.go (2 hunks)
- cmd/blobstream/query/config.go (4 hunks)
- cmd/blobstream/relayer/cmd.go (1 hunks)
- cmd/blobstream/relayer/config.go (4 hunks)
- e2e/qgb_network.go (8 hunks)
- e2e/relayer_test.go (2 hunks)
- e2e/scripts/deploy_blobstream_contract.sh (1 hunks)
- e2e/scripts/start_orchestrator_after_validator_created.sh (1 hunks)
- e2e/scripts/start_relayer.sh (1 hunks)
- orchestrator/orchestrator_test.go (1 hunks)
- rpc/app_querier.go (2 hunks)
- rpc/app_querier_test.go (9 hunks)
- testing/qgb.go (2 hunks)
Files skipped from review due to trivial changes (5)
- cmd/blobstream/base/config.go
- e2e/scripts/deploy_blobstream_contract.sh
- e2e/scripts/start_orchestrator_after_validator_created.sh
- e2e/scripts/start_relayer.sh
- testing/qgb.go
Additional comments: 37
cmd/blobstream/orchestrator/cmd.go (1)
- 57-63: The
NewTmAndAppQuerierfunction now accepts an additional argumentconfig.grpcInsecure. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, make sure that thegrpcInsecureflag is properly validated before being passed to the function.cmd/blobstream/relayer/cmd.go (1)
- 97-103: The
NewTmAndAppQuerierfunction now accepts an additional argumentconfig.grpcInsecure. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that thegrpcInsecureflag is being set correctly based on user input or configuration.rpc/app_querier.go (2)
2-11: The imports are correctly organized and necessary for the code in this file.
28-45: The
Startfunction has been updated to accept a new parametergrpcInsecure. This parameter is used to determine whether to establish a secure or insecure gRPC connection. The error handling for thegrpc.Dialfunction is correctly implemented. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.rpc/app_querier_test.go (1)
- 126-130: The
Startfunction now accepts a boolean argument to indicate whether the gRPC connection should be insecure or not. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, consider the implications of using insecure connections in a production environment. It's generally recommended to use secure connections, especially when dealing with sensitive data.cmd/blobstream/deploy/cmd.go (1)
- 44-50: The
Startfunction now accepts a boolean parameter to indicate whether the gRPC connection should be insecure or not. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, consider the implications of allowing insecure gRPC connections. While this may be useful for testing or development environments, it could pose a security risk in a production environment. It's recommended to ensure that this option is clearly documented and that users are warned of the potential risks.e2e/relayer_test.go (2)
183-187: The
Startmethod ofappQuerieris now called withtrueas an argument, which indicates that the gRPC connection should be insecure. Ensure that this is the intended behavior and that it doesn't introduce any security vulnerabilities.270-274: The
Startmethod ofappQuerieris now called withtrueas an argument, which indicates that the gRPC connection should be insecure. Ensure that this is the intended behavior and that it doesn't introduce any security vulnerabilities.orchestrator/orchestrator_test.go (1)
- 182-185: The
Startmethod ofappQuerieris now called withtrueas an argument, which means that the gRPC connection will be insecure. Ensure that this is the intended behavior and that it doesn't introduce any security vulnerabilities.- require.NoError(s.T(), appQuerier.Start(true)) + require.NoError(s.T(), appQuerier.Start(false))cmd/blobstream/relayer/config.go (4)
49-49: The
addRelayerStartFlagsfunction has been updated to include thegrpcInsecureflag. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.63-63: The
StartConfigstruct has been updated to include thegrpcInsecurefield. Ensure that all instances of this struct throughout the codebase have been updated to include this new field.140-143: The
parseRelayerStartFlagsfunction has been updated to parse thegrpcInsecureflag. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.160-160: The
StartConfigstruct is being returned with thegrpcInsecurefield set. Ensure that all instances where this struct is being used have been updated to handle this new field.cmd/blobstream/orchestrator/config.go (4)
37-41: The
addOrchestratorFlagsfunction has been updated to include theAddGRPCInsecureFlagfunction call. This adds the--grpc-insecureflag to the command.47-51: The
StartConfigstruct now includes agrpcInsecurefield of typebool. This field will hold the value of the--grpc-insecureflag.101-107: The
parseOrchestratorFlagsfunction has been updated to parse the--grpc-insecureflag and store its value in thegrpcInsecurevariable.117-123: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [109-121]
The
StartConfigstruct is now initialized with thegrpcInsecurefield set to the value of the--grpc-insecureflag.cmd/blobstream/deploy/config.go (4)
42-46: The
addDeployFlagsfunction has been updated to include theAddGRPCInsecureFlagfunction call. This adds the--grpc-insecureflag to the command. Ensure that theAddGRPCInsecureFlagfunction correctly adds the flag and that it defaults tofalse.53-57: The
deployConfigstruct now includes agrpcInsecurefield. This field will hold the value of the--grpc-insecureflag.103-109: The
parseDeployFlagsfunction has been updated to parse the--grpc-insecureflag and assign its value to thegrpcInsecurefield in thedeployConfigstruct. Ensure that the flag is correctly parsed and that any errors are properly handled.119-123: The
grpcInsecurefield is now included in thedeployConfigstruct returned by theparseDeployFlagsfunction. Ensure that the value of this field is correctly used in the rest of the codebase.cmd/blobstream/common/helpers.go (2)
27-30: The function signature has been updated to include a new parameter
grpcInsecure bool. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.48-48: The function call
appQuerier.Start()has been updated toappQuerier.Start(grpcInsecure). This change allows the caller to specify whether the gRPC connection should be insecure or not. Ensure that theStartfunction in theAppQuerierstruct has been updated to accept this new parameter.cmd/blobstream/query/cmd.go (2)
75-81: The changes made here are correct and align with the pull request summary. The
common.NewTmAndAppQuerier()function now accepts an additional argumentconfig.grpcInsecurewhich is used to determine whether the gRPC connection should be insecure or not. Thestopsvariable is also correctly appended with the returnedstopsvalue fromcommon.NewTmAndAppQuerier(). Ensure that all calls tocommon.NewTmAndAppQuerier()throughout the codebase have been updated to match the new signature.358-364: The changes made here are correct and align with the pull request summary. The
common.NewTmAndAppQuerier()function now accepts an additional argumentconfig.grpcInsecurewhich is used to determine whether the gRPC connection should be insecure or not. Thestopsvariable is also correctly appended with the returnedstopsvalue fromcommon.NewTmAndAppQuerier(). Ensure that all calls tocommon.NewTmAndAppQuerier()throughout the codebase have been updated to match the new signature.cmd/blobstream/query/config.go (4)
3-10: The new import statement is correctly added. Ensure that the package is available in the project dependencies.
21-24: The new flag is correctly added to the command. Ensure that the flag is documented in the user guide or help command.
30-34: The new field is correctly added to the Config struct. Ensure that the field is used in the rest of the codebase.
58-72: The new flag is correctly parsed and added to the Config struct. Ensure that error handling is consistent with the rest of the codebase.
e2e/qgb_network.go (8)
423-426: Ensure that the
Startmethod ofappQueriercan handle thetrueargument correctly and that it doesn't introduce any security vulnerabilities due to the insecure gRPC connection.494-497: Same as above.
542-545: Same as above.
605-608: Same as above.
654-657: Same as above.
675-678: Same as above.
697-700: Same as above.
995-998: Same as above.
Codecov Report
@@ Coverage Diff @@
## main #576 +/- ##
==========================================
- Coverage 34.98% 34.84% -0.15%
==========================================
Files 27 27
Lines 2221 2230 +9
==========================================
Hits 777 777
- Misses 1351 1360 +9
Partials 93 93
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- e2e/scripts/start_orchestrator_after_validator_created.sh (2 hunks)
Additional comments: 2
e2e/scripts/start_orchestrator_after_validator_created.sh (2)
53-59: The
--grpc.insecureflag is added to the command-line arguments. This flag enables insecure gRPC communication. Ensure that this flag is used responsibly and only in environments where security is not a concern, such as local testing or development. In production environments, secure communication should always be used.67-73: The
--grpc.insecureflag is added to the command-line arguments. This flag enables insecure gRPC communication. Ensure that this flag is used responsibly and only in environments where security is not a concern, such as local testing or development. In production environments, secure communication should always be used.
| @@ -46,6 +46,7 @@ const ( | |||
| FlagBootstrappers = "p2p.bootstrappers" | |||
| FlagP2PListenAddress = "p2p.listen-addr" | |||
| FlagP2PNickname = "p2p.nickname" | |||
| FlagGRPCInsecure = "grpc.insecure" | |||
There was a problem hiding this comment.
does comet/sdk even support secure?
There was a problem hiding this comment.
Secure grpc is supported not rpc
Overview
Closes #352
Checklist
Summary by CodeRabbit
New Features
--grpc-insecureflag to enable gRPC communication over insecure channels.Bug Fixes
grpcInsecureparameter, ensuring correct behavior based on the user's preference for secure or insecure gRPC connections.Tests
grpcInsecureparameter in function calls.