Conversation
GlenDC
left a comment
GlenDC
left a comment
There was a problem hiding this comment.
Left some feedback, let me know if something is not clear, as you're dealing with non-trivial concepts here.
| ) -> Result<Self::Response, Self::Error> { | ||
| let pac_file = fetch_pac(&self.web_client, &self.pac_uri) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
given it's wip this is ok, but obviously in the final code errors like this should be handled or bubbled up. no unwraps please unless you have a very good reason
| *attempt.uri_mut() = proxy_uri; | ||
| } | ||
| ProxyDirective::Socks5(hp) => { | ||
| let proxy_uri: Uri = format!("socks5://{hp}") |
There was a problem hiding this comment.
The proxy info will have to be added in the extensions (context for now, but in some days part of request), so that the inner connectors will use it as proxy
| *attempt.uri_mut() = proxy_uri; | ||
| } | ||
| } | ||
| match self.service.serve(ctx.clone(), attempt).await { |
There was a problem hiding this comment.
Inner service is a connector service so might as well call it connector. E.g. is going to be something like an AutoConnector wrapped around tcp etc.
| @@ -0,0 +1,3 @@ | |||
| pub mod connector; | |||
| pub mod pac_fetcher; | |||
There was a problem hiding this comment.
no need to make all modules public if you only use 1 or 2 types it's better to keep the public module structure flat.
| .body(Body::empty()) | ||
| .map_err(Into::<BoxError>::into)?; | ||
|
|
||
| let response = web_client.execute(Context::default(), request).await?; |
There was a problem hiding this comment.
given you already use HttpClientExt you can make this simpler by doing web_client.get(pac_ure).send(Context::default()) or something like it
|
|
||
| const PAC_STDLIB: &str = r#" | ||
| // spec-ish helpers | ||
| function dnsDomainIs(host, domain) { |
There was a problem hiding this comment.
Instead of using these js interpreted functions, use rust host functions please, so we can use std rama code and it will be easier to keep it straight
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub enum ProxyDirective { | ||
| Direct, | ||
| Proxy(String), |
There was a problem hiding this comment.
We can probably do better, type-wise, no? As in a SocketAddress, or something like that?
| use rama_core::{Context, error::BoxError}; | ||
| use rama_http::{Body, Request, Uri, service::client::HttpClientExt}; | ||
|
|
||
| pub async fn fetch_pac<W>(web_client: &W, pac_uri: &Uri) -> Result<String, BoxError> |
There was a problem hiding this comment.
why is this public? This is just a detail of your connector I would think
| ctx: Context, | ||
| req: Request<Body>, | ||
| ) -> Result<Self::Response, Self::Error> { | ||
| let pac_file = fetch_pac(&self.web_client, &self.pac_uri) |
There was a problem hiding this comment.
this should be cached in your connector I think with an ArcSwap together with a duration and only refetch it after some cache duration, default pretty high but can be overwritten.
| where | ||
| S: Service<Request<Body>, Error: Into<BoxError> + Send + Sync + 'static>, | ||
| W: HttpClientExt + Send + Sync + 'static, | ||
| Body: Clone + Send + Sync + 'static, |
There was a problem hiding this comment.
Body is never clone. Neither should it. Your connector is not dealing with the actual Http request anyway, what you need to implement is a service as a connection service. For any request which has a target uri, but that doesn't have to be a http request perse. This way can also work for any other protocol that has uris and again it's; not to already serve the input but to just establish connection. The inner service is the actual connector stack establishing the connection.
2 participants
Details
Issue number
#566
Summary
rama-paccrate that will handle logic of creating a PAC Connector, getting and reading the PAC file for the outgoing request URL proxy options and route the request through first successful proxy.