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

feat: enable proxy handling for native fetch and connectrpc calls #1124

Merged

Conversation

AndreasZeissner
Copy link
Contributor

@AndreasZeissner AndreasZeissner commented Aug 26, 2024

Motivation and Context

Setting the proxy relies on undici for now undici does not respect the HTTP(S)_PROXY vars, it's planned to build it in from v23 on by default:

nodejs/node#43187 (comment)

My current plan is to add support for the HTTP_PROXY env variable
behind a flag in v22 (and possibly v20), then unflag in v23.

Using global dispatcher is the current way to go for native fetch,
therefore undici needs to be added.

For connectrpc the agent will be passed down the chain:

      /**
       * Options passed to the request() call of the Node.js built-in
       * http or https module.
       */
      nodeOptions?:
        | Omit<http.RequestOptions, "signal">
        | Omit<https.RequestOptions, "signal">;

proxy

Depending on the proxy, setting: export NODE_TLS_REJECT_UNAUTHORIZED=0 might be needed (e.g. calling the wgc npm "update" command)

TODO

Setting the proxy relies on undici for now undici does not respect the
HTTP_PROXY vars, it's planned to build it in from v23 on by default:

nodejs/node#43187 (comment)

> My current plan is to add support for the HTTP_PROXY env variable
behind a flag in v22 (and possibly v20), then unflag in v23.

Using global dispatcher is the current way to go for native fetch,
      therefore undici needs to be added.

For connectrpc the agent will be passed down the chain:
/**
 * Options passed to the request() call of the Node.js built-in
 * http or https module.
 *

 example proxy trace:

 22:05:23 HTTPS POST
 localhost/auth/realms/cosmo/protocol/openid-connect/token              200    application/json  2.0k 202ms
 22:05:35 HTTP  POST
 localhost/wg.cosmo.platform.v1.PlatformService/DeleteFederatedGraph    200   application/proto   44b  64ms
@github-actions github-actions bot added the cli label Aug 26, 2024
@AndreasZeissner AndreasZeissner marked this pull request as ready for review August 27, 2024 12:10
cli/test/proxy.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@Aenimus Aenimus left a comment

Choose a reason for hiding this comment

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

Some minor comments but LGTM

@AndreasZeissner AndreasZeissner merged commit e4edd4e into main Aug 28, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants