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

Disable automatic proxy detection from environment variables #672

Closed
3 tasks done
aoberoi opened this issue Dec 17, 2018 · 16 comments
Closed
3 tasks done

Disable automatic proxy detection from environment variables #672

aoberoi opened this issue Dec 17, 2018 · 16 comments
Labels
bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented

Comments

@aoberoi
Copy link
Contributor

aoberoi commented Dec 17, 2018

Description

There is currently an undocumented behavior in this package, inherited from a dependency on axios, such that setting the environment variables can change the proxy configuration. Here is a description of this feature:

  // 'proxy' defines the hostname and port of the proxy server.
  // You can also define your proxy using the conventional `http_proxy` and
  // `https_proxy` environment variables. If you are using environment variables
  // for your proxy configuration, you can also define a `no_proxy` environment
  // variable as a comma-separated list of domains that should not be proxied.
  // Use `false` to disable proxies, ignoring environment variables.
  // `auth` indicates that HTTP Basic auth should be used to connect to the proxy, and
  // supplies credentials.
  // This will set an `Proxy-Authorization` header, overwriting any existing
  // `Proxy-Authorization` custom headers you have set using `headers`.

Since this is undocumented, a user who doesn't expect these variables to interfere with the operation of this package could end up in a broken situation with no idea of how this occurred. An example of this is #642.

It has been suggested that we use the axios option (proxy: false) to disable this behavior when no proxy configuration is provided. This has an advantage that users are less likely to end up in a situation where an unexpected problem occurs. But it also takes away some utility, since the http_proxy and https_proxy environment variables are seen as a common way to configure all HTTP clients in an entire process (config all dependencies that make HTTP requests at the same time). In fact, some might already be depending on the behavior, so taking it away might have to be delayed into a semver:major enhancement.

I'd like to hear others' opinions on what could and should be done.

Requirements (place an x in each of the [ ])

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've searched for any related issues and avoided creating a duplicate issue.
@aoberoi aoberoi added the discussion M-T: An issue where more input is needed to reach a decision label Dec 17, 2018
@Shegox
Copy link

Shegox commented Jan 24, 2019

Hey,
we just run into the same issue of not wanting to use the proxy for slack calls. Unfortantly axios@0.18.0 only implements the http(s)_proxy env variables and not the no_proxy variables. Resulting in breaking our setup when updating to a newer slack/client.

It would be really great if this can be released rather quick or by adding the new axios@0.19.0-beta.1 which adds no_proxy support.

Do you by chance know a quick workaround for this issue?

@aoberoi
Copy link
Contributor Author

aoberoi commented Jan 29, 2019

@Shegox unfortunately, i don't have a quick workaround right now. even if we depend on the beta release of axios and utilize the no_proxy option, this would result in a breaking change for existing users who depended on the environment variables to set their slack proxy.

is it possible to modify the parts of your application that use the environment variables to configure the proxy to instead use explicit configuration?

i feel that we should address this issue with the next major version of the package, but i'm not sure when the axios 0.19.0 release will stable. we are hoping to ship our next major version before the end of February.

@Shegox
Copy link

Shegox commented Jan 29, 2019

Hey @aoberoi,
thanks for the response. We now opted to upgrade the dependency locally to the axios beta release for our deployment. That worked without problems so far.

From my perspective it would be good to ignore the env variables from axios (if no stable release is released) and let the slack client set the proxy, if desired.

@clavin
Copy link
Contributor

clavin commented Jan 30, 2019

My two cents: keep this not only enabled, but ensure it keeps working (i.e. through a test).

More than anything, I see the problem not as an undocumented feature, but rather as an undocumented feature. How to combat it? Document it!

### Custom agent for proxy support
...

+ > 🕵️‍♀️ If no agent is provided, a proxy may be established using the
+ > `http_proxy` and `https_proxy` environment variables. Supply `agent: false`
+ > to disable this behavior.
+ > .....

Add in a reference to GNU's specification (maybe a bit too terse to use as reference in docs) or axios' specification (i.e. the comments in this issue's initial post), and it's documented. I can see the visibility of a note like this being up for debate, but I still feel like documentation is the key issue here.

My justification lies in two observations:

First ☝️, these variables are opt-in. Setting these environment variables is akin to enabling a linter rule: you get what you ask for (at least, that's the hope when setting it).

... a user who doesn't expect these variables to interfere with the operation of this package ...

This feels like a direct consequence of the lack of documentation (not the feature itself!). I can see why someone might be curious as to whether http(s)_proxy does work with this package, but setting it and assuming it won't affect your WebClient calls is where I'm lost. Why set it if you don't want it?

Second ✌️, it'd be odd to have to opt in to "automatic" detection (e.g. via proxy: true as a WebClient option) of these environment variables, which are mentally already opt-in when you set them--or, worse, have to migrate from a working app to having to parse process.env.https_proxy using url.parse, and supplying it to https-proxy-agent (after adding that package as a dependency on top of all this).


Special case: RTMClient

I feel like the real case that's being discussed here has to do with RTMClient, yet it's not mentioned in this issue so far. 👀

It more logically follows that a developer might not expect the RTMClient to be affected by these environment variables. The rationale is that the RTMClient works primarily off a WebSocket connection (wss://...), and it's not immediately obvious that an https://... request is made.

I feel like the current behavior is correct: the internal call to the Web API should continue to detect the proxy from the environment, as that is what the variable is meant to control (re: opt-in). As it is now, passing agent: false to either WebClient or RTMClient should disable this automatic detection in case you specifically want to opt out (specifically) @slack/client instances after opting in to this behavior.

Again, one more time: I feel like the real issue resolves to awareness on this one. I'm curious as to thoughts on how visibility in the docs could affect this issue.


Observation: this issue's title (and, inherently, topic) is vulnerable to self-selection bias 📉.

[Example:] Take [for instance] a poll which measures level of confidence in parenting among university graduates. Those who are proud of their parenting are more likely to want to talk about it, and therefore more likely to fill in the survey.

-- https://www.statisticshowto.datasciencecentral.com/self-selection-bias/

I would expect this issue to only be seen/responded to mostly by people who have had issues with automatic proxy detection, and thus favor "disabl[ing] automatic proxy detection in axios". Those who are dependent the detection on are unlikely to go out of their way to survey this repo's issues, thus unlikely to find issues like this one. Sounds like an echo chamber 😅.

The body of this issue does a very good job at being impartial and objective, but I feel like the nature of this discussion alone is bound to end up biased in its responses.

@Shegox
Copy link

Shegox commented Jan 31, 2019

@clavin general I'm 100% on your side. It would be inline with everything if the proxy is detected correctly.

However with the current implementation it is lacking two major things. I think support for the NO_PROXY environment variable would be critical.

Also in the current implementation (4.8.0) agent:false doesn't seem to work for the WebClient.
I tried overriding it and it can't reach the the slack api:
this.appWebClient = new WebClient(slackAppToken, {agent: false }).

Please correct me, if I'm wrong, but with the current package it is not possible to disable the proxy when you set the https_proxy environment variable?

If there is an way to disable it and it is documented, I would find it okay behavior.

@clavin
Copy link
Contributor

clavin commented Jan 31, 2019

Ah, you'd be correct! It's the axios option proxy: false (rather than what I incorrectly believed, agent) that disables automatic proxy detection, which WebClient doesn't forward from its opts.

I feel like it'd be justified for WebClient to set proxy: false when the agent option supplied to WebClient is false, yes? Likewise, RTMClient should accept & handle false as a valid value for agent only for passing to WebClient (RTMClient#agentConfig would just be undefined if its agent option is false). I'd be happy to open a PR for a change as simple as this 😊.

As for current workarounds, there's nothing I know of that you can do with @slack/client to avoid this bug 🙁. Your process of manually upgrading the axios package used by @slack/client to the latest beta so it can identify your no_proxy env var is probably your best option currently.

@aoberoi aoberoi changed the title Discuss: Disable automatic proxy detection in axios Discuss: Automatic proxy detection in axios Feb 7, 2019
@aoberoi
Copy link
Contributor Author

aoberoi commented Feb 7, 2019

Thanks for your detailed thoughts @clavin and @Shegox!

My perspective is that axios is an implementation detail, and we should declare (and document) our intended interface for those without knowledge of our implementation.

Today, our declared interface states that we do not handle proxies (out of scope) and that in order to configure one, you should add a dependency to your own app and then leverage the agent option. So in terms of our declared interface, we have a bug 😦 (not an undocumented feature).

The question remains, what will tomorrow's interface become? Here's what I see as the options:

  1. Continue to call proxy config out-of-scope and fix the bug: this means setting proxy: false so that automatic detection doesn't kick in and break the abstraction.
  2. Adopt proxy config as a first class feature: This could be @clavin's suggestion (pass proxy: false to axios when WebClient option agent is set to false), but I actually think a better experience would be to take the proxy URLs directly as a whole new option on WebClient and RTMClient, and to promote that as the way to use the functionality. We would then keep the automatic detection from environment variables. At that point, it might be worth considering also deprecating the agent option because I'm not sure which use cases require that aside from proxying.

I don't see others that look favorable, but I'm willing to hear more ideas.

Now we've got to weigh the pros and cons of these options.

Pro Option 1: The smaller we keep the scope, the less vulnerable this package is to risks of breaking API contracts. For example, in v4.5.0 we were able to swap out got for axios with relatively little pain because we limited our scope and were very careful about leaking the abstraction - and we still introduced this bug!

Pro Option 2: Proxying is (by anecdotal accounts) a popular need, especially for developers who deploy inside corporate controlled environments - lots of Slack developers. Taking away the extra dependency and step they need to make it work is nice.

Con Option 2: Providing proxy support may add complexity to eventually targeting the browser or Electron platform (possible future goal). Even if we decided that it was only supported in a subset of environments, communicating this and documenting it is not free - and some users will still always have questions.

Pro Option 2: When the SDK is embedded inside a framework (instead of being directly depended on by the app developer), then the app developer could retain the ability to set the proxy using environment variables, whether the framework (intermediary) was aware of the options or not.

I'm leaning more towards Option 2 now, but would like to hear more thoughts.

@aoberoi
Copy link
Contributor Author

aoberoi commented Feb 7, 2019

By the way, we're preparing for a new major version release soon. So if we wanted to drop the agent option in favor of a proxy option, now would be a good time.

@aoberoi aoberoi changed the title Discuss: Automatic proxy detection in axios Deprecate agent option and add proxy option for WebClient and RTMClient Feb 8, 2019
@aoberoi aoberoi added enhancement M-T: A feature request for new functionality and removed discussion M-T: An issue where more input is needed to reach a decision labels Feb 8, 2019
@aoberoi aoberoi added this to the v5 - Major version update milestone Feb 8, 2019
@aoberoi aoberoi changed the title Deprecate agent option and add proxy option for WebClient and RTMClient Deprecate agent option for WebClient and RTMClient Feb 8, 2019
@aoberoi aoberoi removed this from the v5 - Major version update milestone Feb 8, 2019
@aoberoi
Copy link
Contributor Author

aoberoi commented Feb 26, 2019

As I started working on this change, I learned about a few more pros and cons.

Pro Option 1: The agent option can be configure to set a larger variety of proxy protocols than the proxy option in axios offers: HTTP, HTTPS, SOCKS. They would all be separate dependencies the developer needs to add in their app ([https-proxy-agent](https://www.npmjs.com/package/https-proxy-agent), [http-proxy-agent](https://www.npmjs.com/package/http-proxy-agent), [socks-proxy-agent](https://www.npmjs.com/package/socks-proxy-agent)).

Con Option 2: In order to support the proxy option directly for the RTMClient, we would need to require at least one of the previously mentioned packages as a direct dependency of this package, and use it to configure the agent. This is the best (only?) way to support proxies for websockets.

There's also one more option:

  1. Keep agent supported in both WebClient and RTMClient, add proxy option to only WebClient. This option feels strange to me because its not clear wha the interaction between proxy and agent options would be. For example, if I configure an HTTPS proxy with the proxy option, but also configure a SOCKS proxy using the agent option, what is the expected behavior? Ultimately, it feels like axios's proxy support is designed for use inside the browser environment, where the full power of agent is not available. But since this package targets node and not the browser, it feels like we should just (and only) directly support the more powerful construct.

With these new data points, I'm starting to lean towards leaving the functionality as is (not deprecating agent), and fixing the bug (by passing proxy: false to axios initialization).

Any thoughts?

@RodneyU215
Copy link

@aoberoi Thanks for breaking down the issue and the available options so well. From everything that's mentioned and what I've recently experienced troubleshooting proxies in the Python Slack SDK I'd recommend first fixing the bug by implementing option 1 above.

In another PR I'd explore and test how well it would work if you made proxy configs a first class feature like you mentioned. One concern I'd have with making proxy configs work is the number of existing axios issues that are currently open related to this.

@aoberoi aoberoi changed the title Deprecate agent option for WebClient and RTMClient Disable automatic proxy detection from environment variables Mar 5, 2019
@aoberoi aoberoi added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented and removed enhancement M-T: A feature request for new functionality semver:minor labels Mar 5, 2019
@aoberoi
Copy link
Contributor Author

aoberoi commented Mar 5, 2019

Thanks for the input. I'm reclassifying this issue as a bug (explained in the previous two comments).

There's nothing stopping us from introducing proxy as a first-class option in a feature release in the future. We welcome that conversation! If that's something you want to discuss, please open another issue.

@SpencerKaiser
Copy link

@aoberoi any idea when y’all might ship? We have an issue against our project to implement a workaround, but I’d rather fix it with an update if possible! Thanks!

@aoberoi
Copy link
Contributor Author

aoberoi commented Mar 7, 2019

@SpencerKaiser we should have a fix released before the end of the week, and possibly as soon as tomorrow.

@SpencerKaiser
Copy link

Fantastic, thanks @aoberoi!

aoberoi added a commit to aoberoi/node-slack-sdk that referenced this issue Mar 7, 2019
@SpencerKaiser
Copy link

@aoberoi which version will this fix be released in?

@aoberoi
Copy link
Contributor Author

aoberoi commented Mar 7, 2019

@SpencerKaiser the next one 😉. Haha, if all goes well, it should be shipping before the end of the day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented
Projects
None yet
Development

No branches or pull requests

5 participants