Add rpc alt proxy for request filtering and transformation #22181
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
emmazzz
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
emmazzz
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
There was a problem hiding this comment.
Thanks for the fast turnaround on this @emmazzz, my questions are mostly nits, but here are the high level themes:
- Some things that the edge proxy does is probably not appropriate for our use case. It seems like the edge proxy fails open (if it can't figure out some detail about the request, it sends it through anyway), while we should fail closed.
- There is some duplication of logic between edge proxy and cursor update logic, related to parsing method bodies which we should clean up and de-duplicated.
- There is also an opportunity to de-duplicate this between the benchmarking tool and this service, but we can address that later.
- Look out for opportunities to use early return style with
if let ...andlet .. else. - Standardise logging to use
tracing, and to use structured logging instead of interpolation to make it easier to pull data out from telemetry. - Use
format!("{foo}")instead offormat!("{}", foo)where possible for readability.
| async fn main() { | ||
| let args = Args::parse(); | ||
|
|
||
| println!("args: {:?}", args); |
There was a problem hiding this comment.
| println!("args: {:?}", args); | |
| info!(?args, "Starting service"); |
nit: it would be preferable to use tracing (e.g. info!) for this -- you will need to move the call to after initialising the telemetry subscriber.
| .with_prom_registry(&prometheus_registry) | ||
| .init(); | ||
|
|
||
| info!("Metrics server started at {}", config.metrics_address); |
There was a problem hiding this comment.
In this case, I don't think it makes much difference, but as a habit, it's good to avoid interpolating data into log strings, in favour of using the structured logging format, which makes it easier to extract this info from the logs.
| info!("Metrics server started at {}", config.metrics_address); | |
| info!(address = config.metrics_address, "Metrics server started"); |
| .fallback(any(proxy_handler)) | ||
| .with_state(app_state); | ||
|
|
||
| info!("Starting server on {}", config.listen_address); |
There was a problem hiding this comment.
| info!("Starting server on {}", config.listen_address); | |
| info!(address = config.listen_address, "Starting server"); |
| pub async fn load<P: AsRef<std::path::Path>>(path: P) -> Result<(ProxyConfig, Client)> { | ||
| let path = path.as_ref(); | ||
| let config: ProxyConfig = serde_yaml::from_reader( | ||
| std::fs::File::open(path).context(format!("cannot open {:?}", path))?, |
There was a problem hiding this comment.
| std::fs::File::open(path).context(format!("cannot open {:?}", path))?, | |
| std::fs::File::open(path) | |
| .with_context(|| format!("Failed to open ProxyConfig at {}", path.display()))?, |
| response.status() | ||
| ); | ||
| } | ||
| return Ok(()); |
There was a problem hiding this comment.
If the health check fails (or the fullnode is not using HTTP/2) is the fullnode considered validated?
| if res.is_err() { | ||
| warn!( | ||
| "Failed to update pagination cursor state: {}", | ||
| res.err().unwrap() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Use an if let to avoid the unwrap.
| } | ||
| let (response, response_bytes) = | ||
| proxy_request(&state, parts, body_bytes).await?; | ||
| if response.status().is_success() { |
There was a problem hiding this comment.
If the response fails, should we clear the cursor from the cache?
| Ok(response) | ||
| } | ||
| _ => { | ||
| // We can't find out what the method is so we directly proxy the request. |
| "Proxying request: method={:?}, uri={:?}, headers={:?}, body_len={}", | ||
| parts.method, | ||
| parts.uri, | ||
| parts.headers, | ||
| body_bytes.len(), |
There was a problem hiding this comment.
| "Proxying request: method={:?}, uri={:?}, headers={:?}, body_len={}", | |
| parts.method, | |
| parts.uri, | |
| parts.headers, | |
| body_bytes.len(), | |
| method=?parts.method, | |
| uri=?parts.uri, | |
| headers=?parts.headers, | |
| body_len=body_bytes.len(), | |
| "Proxying request", |
| let mut target_url = state.fullnode_address.clone(); | ||
| target_url.set_path(parts.uri.path()); | ||
| if let Some(query) = parts.uri.query() { | ||
| target_url.set_query(Some(query)); | ||
| } |
There was a problem hiding this comment.
Is this potentially too general? For a generic edge proxy, it makes sense to forward the path and query params, but for us, we should (a) filter the requests based on the ones that are going to the expected path, and we should choose which path we route the requests to.
There was a problem hiding this comment.
the path between fullnodes and indexer-alt-jsonrpc are different ('/' vs '/json-rpc'). We could hard code /json-rpc path here I suppose but its just as easy to allow the path to be defined in the config how we're doing it now.
| .observe(body_bytes.len() as f64); | ||
|
|
||
| let mut target_url = state.fullnode_address.clone(); | ||
| target_url.set_path(parts.uri.path()); |
There was a problem hiding this comment.
need to remove this to utilize the full path defined in the config.yaml.
emmazzz
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
emmazzz
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
Description
Adds a proxy sending rpc traffic to jsonrpc-alt server, adapted from
sui-edge-proxy.The proxy:
unsupported-methodsas configuredTest plan
How did you test the new or updated feature?
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.