Conversation
drahnr
force-pushed
the
bernhard-feat-add-rate-limiter
branch
from
3ea5ce1 to
558e109
Compare
| ) -> runtime::Runtime { | ||
| let genesis_state = GenesisState::new(vec![], test_fee_params(), 1, 1, random_secret_key()); | ||
| fs::create_dir_all(data_directory.join(ACCOUNT_TREE_STORAGE_DIR)) | ||
| .expect("account tree directory should be created"); |
There was a problem hiding this comment.
Is this diff related and intentional?
There was a problem hiding this comment.
I don't think it's required, let me remove it
| // TODO double check how to address proxy rate limiting based on i.e. `X-Real-IP`. | ||
| request | ||
| .metadata_mut() | ||
| .insert("forwarded", format!("for={addr}").try_into().unwrap()); |
There was a problem hiding this comment.
we shouldn't panic here right? this is a callback for every request?
There was a problem hiding this comment.
It is a callback per request, unless someone manages to inject a broken/malicious IP in the Linux Kernel and manage to pop it up in our end, it's virtually impossible to break this invariant.
Since we're using SmartIPKeyExtractor* we might get away without this. It's here for ensuring we have a fallback
| } | ||
| }); | ||
| Ok(tower_governor::GovernorLayer::new(config)) | ||
| } |
There was a problem hiding this comment.
Just checking this only actually gets called once (rate_limit_per_ip())
There was a problem hiding this comment.
I get 7 hits in the diff? It creates a layer so this is fine, unless I am missing smth?
There was a problem hiding this comment.
I don't think we should be applying these to all servers? Only the RPC server or no?
The rest are internal; and we should be bounding the calls we make. e.g. we don't want to disconnect our own gRPC connections after some time..
| let addr = request | ||
| .remote_addr() | ||
| .ok_or_else(|| Status::failed_precondition("Expected TCP connection"))?; | ||
| // TODO double check how to address proxy rate limiting based on i.e. `X-Real-IP`. |
There was a problem hiding this comment.
Was this addressed?
There was a problem hiding this comment.
This is addressed with the SmartKeyExtractor* which tests for 3 distinct metadatadata entries before using the raw incoming IP
| > { | ||
| let config = GovernorConfigBuilder::default() | ||
| .key_extractor(SmartIpKeyExtractor) | ||
| .per_second(grpc_options.replenish_per_sec) |
There was a problem hiding this comment.
I think we're using this invertedly. .per_second specifies the number of seconds to wait until a single permit is replenished. And not the number of permits to replenish each second.
| let config = GovernorConfigBuilder::default() | ||
| .key_extractor(SmartIpKeyExtractor) | ||
| .per_second(grpc_options.replenish_per_sec) | ||
| .burst_size(grpc_options.burst_size as u32) |
There was a problem hiding this comment.
Should we make burst_size a u32 then?
| .burst_size(grpc_options.burst_size as u32) | ||
| .use_headers() | ||
| .finish() | ||
| .context("config parameters are inconsistent, i.e. burst < per second")?; |
There was a problem hiding this comment.
I don't think burst < per second is an error condition. Only that they're non-zero. Which we can enforce with types.
3 participants
Adds a rate-limiter with quotas using
tower_governorandtower::limit::GlobalConcurrencyLimitLayer.Done in scope of our recent fd consumption, generally a useful thing to do.
Makes all parameters configurable and group them into
GrpcOptionsand moves them tomiden-node-utilsto avoid duplication, similarly for limiting layer setup helpers.