Skip to content

Conversation

@jmedelmann
Copy link
Contributor

PR for issue #702 (Node feature does not pick up proxy settings).
Docker automatically sets up proxy environmentals in the container when configured (see https://docs.docker.com/network/proxy/) so usually everything works without additional configuration. An exception is npm, which does not pick up these environmentals and needs to be configured manually. This PR adds the necessary configuration steps.

@jmedelmann jmedelmann requested a review from a team as a code owner October 3, 2023 16:28
@jmedelmann
Copy link
Contributor Author

@microsoft-github-policy-service agree

Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

Thanks for contributing this change.

Currently, this PR is automatically setting proxy if the env variables exist. Instead, can we wrap this logic inside a condition on a new Feature option (say useProxyConfig:boolean with default false). This way, the users are willingly enabling it as per their needs, and we won't have to worry about folks raising security concerns.

As per https://docs.docker.com/network/proxy/#configure-the-docker-client, adding proxies to ~/.docker/config.json seems like a manual approach where users are opting into it. So wrapping it inside a Feature option makes sense to me!

@jmedelmann
Copy link
Contributor Author

jmedelmann commented Oct 3, 2023

Currently, this PR is automatically setting proxy if the env variables exist. Instead, can we wrap this logic inside a condition on a new Feature option (say useProxyConfig:boolean with default false). This way, the users are willingly enabling it as per their needs, and we won't have to worry about folks raising security concerns.

As per https://docs.docker.com/network/proxy/#configure-the-docker-client, adding proxies to ~/.docker/config.json seems like a manual approach where users are opting into it. So wrapping it inside a Feature option makes sense to me!

These env variables are a kind of standard and almost all programs will use them automatically. If we introduce an additional option for just npm we will create an inconsistent state with no benefit. Note that other programs already use these environmentals. For example, apt-get automatically uses the proxy environmentals in install.sh. If we want to make them optional it would require to make them optional for the other programs as well. This would be out of scope of this issue which is about the special treatment required for npm.

Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

Had a quick chat with the team, turns out the devcontainers/cli supports the automatic support as well. Approving! ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants