Secure socks proxy: Allow the env variables to be overriden

[stephaniehingtgen]

What this PR does / why we need it:
This PR adds functionality for environment variables to be overriden by a config. This allows us to use the plugin sdk within Grafana and reduce code redundancy.

A draft Grafana PR is up now on how this will be used

Which issue(s) this PR fixes:

Related to https://github.com/grafana/hosted-grafana/issues/3754

Special notes for your reviewer:

• golang.org/x/net needed to be updated to allow for the password to be empty in socks, which was introduced right after 0.9.0 was released

[andresmgot]

I don't think this approach is deterministic enough. We should not keep global state. For example, calling SecureSocksProxyEnabled would return different values depening on when it's called.

The desired approach would be to make all the exposed methods as part of an interface and then instantiate a secureSocksProxy based either on env vars or a config.

I understand that is a breaking change (since now something needs to be initialized before accessing the exposed functions) and if that's something we cannot break, I would suggest to keep exposing those functions but falling back to use environment variables (since that's the current behavior). Something like:

func SecureSocksProxyEnabled(opts *Options) bool {
  return NewSecureSocksProxyFromEnv().SecureSocksProxyEnabled(opts *Options)
}

If we are fine with the breaking change, we can skip that.

Is what I propose possible?

[wbrowne]

Also in the interests of multitenancy, having any plugin state that could potentially be shared amongst tenants would be best avoided. I realize this isn't how things are working now, but ideally we move in a direction where we can build more confidence 🙏

[stephaniehingtgen]

I made some updates, but this is definitely something to be revisited when a solution for the environment variables is determined.

To make it consistent, I used similar logic to the Logger, where the Default Logger is available for plugins, but then Grafana overrides the default logger internally. For the proxy.Cli, I also have a default of the environment variables, but override that default in the grafana core server based on the config.

I would prefer to wait to add a breaking change until we have the environment variables figured out, otherwise we'll have to update 15+ plugins twice (once to initialize the environment variables to initialize the client, and then go back through when the env variables are removed). But let me know if this is a blocking concern

I tested these changes out with various core datasources and MongoDB and things are looking good.

[stephaniehingtgen]

I also updated the Grafana PR to include the changes for this

[andresmgot]

I am sorry, I completely missed this message and the related line of code. Now, reviewing your other PR I noticed the change. Unfortunately this is not something we want to move forward with. The Default Logger you point out is an (ugly) workaround that we had to implement so core plugins could behave as external plugins. We are currently working on removing differences between core and external plugins and having different behavior calling the same SDK functions is not something we want to promote.

I would prefer to wait to add a breaking change

This was a breaking change anyway since all the plugins would need to change the call from, for example, proxy.SecureSocksProxyEnabled to proxy.Cli.SecureSocksProxyEnabled. Which I guess is more convenient than having to pass down a client but still a breaking change.

and then go back through when the env variables are removed

We don't know how that would look like so better not to base current code on unknowns.

But let me know if this is a blocking concern

Sorry again, this should have been blocked, my bad.

[andresmgot]

After a conversation with @stephaniehingtgen offline discussing the issue. This is my summary:

• The end goal of this is to automatically set the secure proxy when creating a new http client.
• All data sources have access to data source settings when creating the client but not to Grafana config. The required information to set up the client is in the Grafana config, not in the data source config.
• External plugins using the SDK obtain that configuration through environment variables.

The goal is to use the same SDK functions both for core plugins and external plugins but those two access to Grafana config differently. We have the following options:

• The current one. Grafana core changes the SDK implementation to create a default client from the config that it has, not environment variables.
• Manually modify the clients generated in Grafana for its core plugins. This has been discarded because it adds boilerplate code and adds to the differences between core plugins code and external ones.
• Set the required config in Grafana as environment variables. That way the SDK would have access to that info as environment variables in any case. This option has been discarded as well by @marefr.

Since there is no better option, we concluded to continue with the code as it is. Thanks for the context @stephaniehingtgen

[andresmgot]

To clarify, this invalidates the possibility to use a multi tenant instance as @wbrowne highlightes but it may be fine since people using secure proxied connections will likely run on a standalone instance.

[andresmgot]

Code wise LGTM. Make sure to add a notice about the breaking change when releasing.