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(cluster): support HTTP(S) proxy for cluster readiness & OIDC config #365

Merged
merged 1 commit into from
Apr 3, 2020

Conversation

metral
Copy link
Contributor

@metral metral commented Apr 1, 2020

Proposed changes

Users in a proxied environment are unable to create and configure clusters,
as HTTP requests made by Node during install and setup are not being properly
routed to the proxy first.

Using a proxy is now supported by either setting the conventional proxy environment
variables (HTTP(S)_PROXY and/or http(s)_proxy), or alternatively by
specifying the proxy: string property in the cluster definition with the
settings to use.

Format: <protocol>://<host>:<port>
Auth Format: <protocol>://<username>:<password>@<host>:<port>

Ex:

  • "http://proxy.example.com:3128"
  • "https://proxy.example.com"
  • "http://username:password@proxy.example.com:3128"

Support for both cluster and OIDC examples was tested using a local Squid v3.5
proxy server running in Docker.

Related issues (optional)

Closes #350

@metral metral added the feature label Apr 1, 2020
@metral metral added this to the 0.33 milestone Apr 1, 2020
@metral metral self-assigned this Apr 1, 2020
nodejs/eks/cert-thumprint.ts Show resolved Hide resolved
nodejs/eks/cert-thumprint.ts Outdated Show resolved Hide resolved
nodejs/eks/cluster.ts Show resolved Hide resolved
nodejs/eks/cluster.ts Show resolved Hide resolved
nodejs/eks/cluster.ts Show resolved Hide resolved
return new https.Agent({
// Cached sessions can result in the certificate not being
// available since its already been "accepted." Disable caching.
maxCachedSessions: 0,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we not pass this to HttpsProxyAgent as well?

Copy link
Contributor Author

@metral metral Apr 2, 2020

Choose a reason for hiding this comment

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

HttpsProxyAgent uses the HTTP CONNECT method which is not cacheable.

maxCachedSessions only exists in https.Agent, it's not an HttpsProxyAgent option or an option on either net.connect() & tls.connect() per: https://github.com/TooTallNate/node-https-proxy-agent#api

nodejs/eks/cluster.ts Show resolved Hide resolved
nodejs/eks/package.json Show resolved Hide resolved
@@ -18,7 +18,8 @@
"netmask": "^1.0.6",
"@types/js-yaml": "^3.12.0",
"js-yaml": "^3.13.0",
"which": "^1.3.1"
"which": "^1.3.1",
"https-proxy-agent": "^5.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Does this have any types? (Or is out use of it weakly typed in the code?)

Copy link
Contributor Author

@metral metral Apr 2, 2020

Choose a reason for hiding this comment

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

It has HttpsAgentOptions but we do not make use of it. We only use the exported HttpsProxyAgent which maps to the func createHttpsProxyAgent() that picks between a string or HttpsAgentOptions. We use the common string approach

https://github.com/TooTallNate/node-https-proxy-agent/blob/master/src/index.ts#L8-L12

Users in a proxied environment are unable to create and configure clusters,
as HTTP requests made by Node during install and setup are not being properly
routed to the proxy first.

Using a proxy is now supported by either setting the standard proxy environment
variables (`HTTP(S)_PROXY` and/or `http(s)_proxy`), or alternatively by
specifying the `proxy: string` property in the cluster definition with the
settings to use.

Format:      `<protocol>://<host>:<port>`
Auth Format: `<protocol>://<username>:<password>@<host>:<port>`

Ex:
  - `"http://proxy.example.com:3128"`
  - `"https://proxy.example.com"`
  - `"http://username:password@proxy.example.com:3128"`

Support for both cluster and OIDC examples was tested using a local Squid v3.5
proxy server [running in Docker.](https://hub.docker.com/r/metral/squid)
@metral metral merged commit 643a2ec into master Apr 3, 2020
@pulumi-bot pulumi-bot deleted the metral/http-proxy branch April 3, 2020 17:37
@infin8x infin8x added the kind/enhancement Improvements or new features label Jul 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements or new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http_proxy and https_proxy are ignored during waiting for eks endpoint
4 participants