-
Notifications
You must be signed in to change notification settings - Fork 329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: use api boundary nodes as socks proxies #2712
base: master
Are you sure you want to change the base?
Conversation
Err(socks_err) => { | ||
self.metrics | ||
.socks_connections_attempts | ||
.with_label_values(&[&tries.to_string(), "failure"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I am not sure and maybe @rumenov has a better opinion:
How about we add the SOCKS proxy addr as a label? We expect to have maybe 50 API BNs (more realistic 20). So, that shouldn't increase the metrics too much. It would just help us to see the problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I was mostly concerned about overflooding the metrics with labels; but I'm pretty sure now it will be managable. There s about one socks request every ~3 seconds. So it should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from my side!
let mut tries = 0; | ||
|
||
for socks_proxy_addr in &socks_proxy_addrs { | ||
tries += 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would convert the socks_proxy_addr to URI here and use the URI from here on. You will have less errors to handle and propagate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm I would argue that working working with strings is easier.
First, we don't always need to parse to URI, only when there is a cache miss.
Second, the cache still works with string addresses, so we would still need to keep track of both the URI and the string address
} | ||
} | ||
|
||
fn maybe_add_socks_client_for_address(&self, cache: &mut Cache, address: &str) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I won't be passing the cache here.
Maybe have a free function that blindly creates the socks proxy when you pass in a URI.
This way you can upgrade the lock and insert into the cache in a single place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that will need some more error handling, but it works
#2191 creates socks proxies on API Boundary Nodes.
This PR fetches all of the API Boundary Nodes IPs through registry for each https outcalls request batch and sends them to the adapter. The adapter uses one of these IPs as SOCKS Proxy (if needed), at random, with a maximum of 3 retries.
This is a dark launch; still using the old boundary nodes as socks proxies, and comparing the results with results coming through the new api boundary nodes.
As bandwidth could go up drastically, we only perform dark launch comparisons on a small percentage of requests. That percentage is currently set to 0.