Skip to content
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

✨ Client proxy support #322

Merged
merged 4 commits into from
May 10, 2021
Merged

✨ Client proxy support #322

merged 4 commits into from
May 10, 2021

Conversation

wwilsman
Copy link
Contributor

@wwilsman wwilsman commented May 7, 2021

What is this?

Node is one of the few languages that does not have proxy support built-in. For a while, the stance has been made that you could always provide your own http agent to requests which handle proxy environment variables accordingly. Looking at the history of existing proxy agent packages, there is a lot that is done to work around issues in older Node versions, support other third party libraries, workaround unexpected user provided request options, and other features that we simply do not need for our own use.

Rather than add a dependency (and in most cases several subdeps) to our very small @percy/client dependency tree (it only has a single dep which itself has a single dep), we can copy some existing work from several sources to fit our needs in a much more succinct way.

Our custom proxy agent only works with HTTPS requests since it only talks to our API which we know to be HTTPS. It works by overriding two methods that control how requests are made in node.

  • The first method, addRequest, is extended to ensure specific options are always available for the next method. One of those options will be proxy information if the appropriate environment variables are set and the request does not match any NO_PROXY pattern.

  • The second method, createConnection, opens a secure socket connection to the requested server. When proxying, the socket is connected to the proxy instead with appropriate connection headers in place.

The request util was moved into its own module where the extended HTTPS agent also lives. Another util was added, hostnameMatches, to determine when a URL matches hostnames in the NO_PROXY env var. This util is very similar to the downstream @percy/core util, domainMatch. It's so similar that they can be swapped out completely with very little additional logic inside the util. Now @percy/core will borrow the new @percy/client util in place of the old domainMatch function. Other internal import paths were updated in @percy/cli-exec to match the new request util location.

Tests

During initial testing it felt wrong to test that requests were being made to the proxy without knowing if a real proxy would be able to handle those requests. After trying several proxy packages, I landed again on creating a much smaller implementation that does not come with the weight of broader more general support since we're only testing specific things and not actually needing a full-fledged local proxy.

This small proxy server comes with a man-in-the-middle server. It avoids SSL issues by simply using the same test certs used to create the local receiving test server. This is enough to determine that requests are successfully flowing through it with the presence of proxy environment variables and has some helpful features to allow testing scenarios such as failed proxying or proxy authentication.

Related

Node's current open issue for built-in proxy support: nodejs/node#15620

wwilsman added 3 commits May 7, 2021 15:55
- Move request util into own module
- Create custom ProxyHttpsAgent which handles proxying HTTPs requests
- Does not support proxying HTTP requests since the client only communicates with an HTTPs API URL
- Test proxy support using a simple man-in-the-middle proxy (shares SSL certs with test server)
@wwilsman wwilsman added the ✨ enhancement New feature or request label May 7, 2021
@wwilsman wwilsman requested a review from Robdel12 May 7, 2021 22:26
@wwilsman wwilsman enabled auto-merge (squash) May 7, 2021 22:45
Copy link
Contributor

@Robdel12 Robdel12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏁 Very nice work on this! I know writing tests for this was not fun.

@wwilsman wwilsman merged commit 583a73d into master May 10, 2021
@wwilsman wwilsman deleted the ww/node-proxy branch May 10, 2021 15:45
@bopes
Copy link

bopes commented May 11, 2021

@wwilsman will there be usage instructions for this that we can share with customers?

@wwilsman
Copy link
Contributor Author

@bopes, this will work like any other proxy configuration where they will need to make sure the appropriate proxy environment variables are set. Based on other languages and other implementations common patterns, our implementation here supports HTTP_PROXY, HTTPS_PROXY, NO_PROXY, and their lowercase equivalents.

samarsault pushed a commit that referenced this pull request Mar 3, 2023
Bumps [cypress](https://github.com/cypress-io/cypress) from 7.0.1 to 7.1.0.
- [Release notes](https://github.com/cypress-io/cypress/releases)
- [Changelog](https://github.com/cypress-io/cypress/blob/develop/.releaserc.base.js)
- [Commits](cypress-io/cypress@v7.0.1...v7.1.0)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants