fix!: change grpc deny to allow #6177
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SWvheerden
reviewed
Mar 1, 2024
| @@ -13,39 +14,26 @@ grpc_enabled = true | |||
| #grpc_tls_enabled = false | |||
|
|
|||
| # Uncomment all gRPC server methods that should be denied default (only active when `grpc_enabled = true`) | |||
There was a problem hiding this comment.
Suggested change
| # Uncomment all gRPC server methods that should be denied default (only active when `grpc_enabled = true`) | |
| # All gRPC server methods that should be allowed (only active when `grpc_enabled = true`) |
ghpbot-tari-project
added
P-acks_required
Comment on lines
+17
to
+38
| grpc_server_allow_methods = [ | ||
| "get_version", | ||
| "check_for_updates", | ||
| "get_sync_info", | ||
| "get_sync_progress", | ||
| #"get_tip_info", | ||
| "identify", | ||
| "get_network_status", | ||
| #"list_headers", | ||
| "get_header_by_hash", | ||
| "get_blocks", | ||
| "get_block_timing", | ||
| "get_constants", | ||
| "get_block_size", | ||
| "get_block_fees", | ||
| #"get_tokens_in_circulation", | ||
| #"get_network_difficulty", | ||
| #"get_new_block_template", | ||
| #"get_new_block", | ||
| #"get_new_block_blob", | ||
| #"submit_block", | ||
| #"submit_block_blob", | ||
| #"submit_transaction", | ||
| #"search_kernels", | ||
| #"search_utxos", | ||
| #"fetch_matching_utxos", | ||
| "get_peers", | ||
| "get_mempool_transactions", | ||
| #"transaction_state", | ||
| #"list_connected_peers", | ||
| #"get_mempool_stats", | ||
| #"get_active_validator_nodes", | ||
| #"get_shard_key", | ||
| #"get_template_registrations", | ||
| #"get_side_chain_utxos", | ||
| "get_tip_info", | ||
| "list_headers", | ||
| "get_tokens_in_circulation", | ||
| "get_network_difficulty", | ||
| "get_new_block_template", | ||
| "get_new_block", | ||
| "get_new_block_blob", | ||
| "submit_block", | ||
| "submit_block_blob", | ||
| "submit_transaction", | ||
| "search_kernels", | ||
| "search_utxos", | ||
| "fetch_matching_utxos", | ||
| "transaction_state", | ||
| "list_connected_peers", | ||
| "get_mempool_stats", | ||
| "get_active_validator_nodes", | ||
| "get_shard_key", | ||
| "get_template_registrations", | ||
| "get_side_chain_utxos", |
There was a problem hiding this comment.
Please add the missing methods back in but commented out
Comment on lines
+17
to
18
| grpc_server_allow_methods = [ | ||
| "get_version", |
There was a problem hiding this comment.
ditto other methods commented out
Test Results (Integration tests)12 tests +12 12 ✅ +12 59s ⏱️ +59s For more details on these parsing errors, see this check. Results for commit 65dd095. ± Comparison against base commit 464f2c3. ♻️ This comment has been updated with latest results. |
Test Results (CI) 3 files 117 suites 39m 12s ⏱️ Results for commit 65dd095. ♻️ This comment has been updated with latest results. |
Comment on lines
16
to
+17
| # Uncomment all gRPC server methods that should be denied default (only active when `grpc_enabled = true`) | ||
| grpc_server_deny_methods = [ | ||
| grpc_server_allow_methods = [ |
There was a problem hiding this comment.
The comment is still outdated
Comment on lines
16
to
+17
| # Uncomment all gRPC server methods that should be denied default (only active when `grpc_enabled = true`) | ||
| grpc_server_deny_methods = [ | ||
| grpc_server_allow_methods = [ |
There was a problem hiding this comment.
ditto comment outdated
Comment on lines
188
to
189
| base_node_config.base_node.grpc_server_allow_methods = vec![]; | ||
|
|
There was a problem hiding this comment.
I suspect this list must be fully populated now instead of being left empty.
Cucumber errors:
Captured output: called `Result::unwrap()` on an `Err` value: Status { code: PermissionDenied, message: "`ListConnectedPeers` method not made available"
Step panicked. Captured output: called `Result::unwrap()` on an `Err` value: ExitError { exit_code: GrpcError, details: Some("Base node not responding to gRPC requests: 'get_new_block_template'") }
Step panicked. Captured output: called `Result::unwrap()` on an `Err` value: Status { code: PermissionDenied, message: "`GetTipInfo` method not made available",
|
Replacing this with: #6218 |
SWvheerden
added a commit
that referenced
this pull request
Mar 20, 2024
Description --- Removes the antipattern of a deny list and puts in an allow list Motivation and Context --- A deny list is an antipattern because new methods are not automatically added to the list Replaces: #6177 --------- Co-authored-by: stringhandler <[email protected]> Co-authored-by: Stan Bondi <[email protected]> Co-authored-by: Hansie Odendaal <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Removes the antipattern of a deny list and puts in an allow list
Motivation and Context
A deny list is an antipattern because new methods are not automatically added to the list
How Has This Been Tested?
Tested with dan testing
What process can a PR reviewer use to test or verify this change?
Breaking Changes
BREAKING CHANGE: Removed
grpc_server_deny_methodsand addedgrpc_server_allow_methods