✨ Allow proxying HTTP requests with client request util#508
Conversation
Robdel12
left a comment
There was a problem hiding this comment.
For example, the upcoming sitemap feature for the snapshot command might need to fetch insecure sitemaps through potential proxies.
🏁 Nice, good foresight.
| async run() { | ||
| let { port } = this.flags; | ||
| let stop = `http://localhost:${port}/percy/stop`; | ||
| let ping = `http://localhost:${port}/percy/healthcheck`; |
There was a problem hiding this comment.
Should these be set with PERCY_SERVER_ADDRESS? 🤔 is that configurable?
There was a problem hiding this comment.
That variable is meant for the SDKs and is set by the exec command (not exec:start or others). When set by exec, it actually does end up as http://localhost:${port} (same as here) via the port flag being passed to core which doesn't allow changing the hostname and just uses localhost.
That variable should only be manually set if the SDK is behind a different IP address than the CLI. You could argue that it'd be nice to ping and stop from other IPs, but then I'd argue that it's probably an additional feature to come in another PR. If we did want to implement that feature, we'd have to reconcile on how to handle both the env var and the flag (e.g. should we warn when both exist or silently ignore --port?).
451cfb2 to
3e90466
Compare
What is this?
When initially implementing HTTPS proxy support, it was decided to only implement HTTPS proxy support (and not HTTP) since client only communicates with the secure Percy API. However, this
requestutil is frequently borrowed by other packages to utilize its retry-ability, and sometimes those other packages may need to make requests to insecure URLs while still proxying. For example, the upcoming sitemap feature for thesnapshotcommand might need to fetch insecure sitemaps through potential proxies.This PR adds proxy support to HTTP requests via a new
ProxyHttpAgent. The similarities with the existingProxyHttpsAgentwere refactored out into a dedicatedgetProxyfunction. Both proxy agent classes use this function to determine when and how to proxy requests. Therequestutil was updated to automatically create the necessary proxy agent for a URL when one isn't provided and cache said agent for each unique host. Thanks to this, client no longer has to keep its own agent instance around as it can rely on one being cached for it.The existing proxy tests revolved around client specifically, and so they were adapted to become the new unit tests. Tests that involved differing proxy protocols were deduplicated and now the majority of proxy tests are iterated over with a matrix that tests the various protocol combinations. Some protocols and combinations have additional tests that the others do not, such as HTTPs socket errors or delayed HTTP connections.
Internal usage of this util to communicate with the CLI API was also updated to provide a new
noProxyoption which will bypass the auto-proxy agent used with a specific request. This was done since internal communication shouldn't need to be proxied, but can of course be changed if we find that they should be proxied after all.