Support for Proxy on the Provider level

[detro]

Close #96
Supersedes #113

By introducing support for Proxy on the Provider level, we enable the tls_certificate data source (as well as future resources/data sources), to reach out endpoints via the optionally configured proxy.

The Proxy can be configured via a url (with optional username and password), or from_env`: this means that we leverage the excellent https://pkg.go.dev/net/http#ProxyFromEnvironment API to look for the environment variables and configure the proxy that way.

[bflad]

Overall looking pretty good, just some things to consider around the new net/http and from_env handling.

[bflad]

Previously the data source would only open a TLS connection and not attempt to finish an actual HTTP call (in this new code the GET verb) or read the response body from the URL. A few questions:

• Can we avoid downloading the response body? e.g. should this execute a HEAD request or might a custom request handler function help here?
• Do we need to do anything to avoid new errors from switching to net/http?

[detro]

About using HEAD vs GET comes out of my experience that out there there are still HTTP servers that don't implement HEAD by default. In the past, where I had to do this kind of stuff, I'd even have logic to try HEAD-then-GET. I'm frankly unsure what's the best option here.

About the full HTTP call, I'm also unsure. It's out of this uncertainty that I introduced the support for tls:// vs https:/ schemes: my thinking was, until now we enforced https:/ but actually never executed HTTP in the first place - just opened a socket.

So, I thought that adding tls:// was going to be an easy way to suggest a "if you want to keep the previous behaviour...".

I'm frankly unsure what's the best strategy here. I'm tempted to suggest to make this PR a breaking change, because the only alternative I see is a .mode attribute that allows to chose the way the resource works. And I don't like it much.

[bflad]

Just to capture some of our out-of-band conversation (please correct me if I misspeak anywhere!):

1. We are going to try and implement this so the HTTP client is avoided, continuing to use a direct TLS connection instead, unless a HTTP proxy has been configured (currently via the provider proxy configuration block)
2. Since IANA does not have a URI scheme for strictly a TCP network socket connection, we can use a synthetic tls:// scheme (or potentially use no scheme).
3. When using a HTTP proxied client, it will first attempt a HEAD call, then fallback to a GET call in case of a proxy that does not support HEAD and blocks the subsequent HTTP connection.

Not discussed, but thinking out loud: I'm presuming we can capture 405 Method Not Allowed or 501 Not Implemented HTTP status codes for the HTTP proxied client fallback. Another thing I'm not sure about is whether we will need special 407 Proxy Authentication Required handling anywhere.

[detro]

Actually, while I want to handle HEAD then GET, I think we can't give lots of weight to the actual HTTP response: the TLS negotiation happens much earlier then any HTTP Protocol byte has moved.

So, I think it makes sense to handle error response from an http.Get() call, but if the reps.TLS.PeerCertificates are there, this provider shouldn't do much more than process those.

I'll show you what I mean in the next commits I push.

[bflad]

A few things here:

• It'd be great to introduce an acceptance test for this that utilizes t.Setenv("HTTP_PROXY", /* ... */) with and without from_env set.
• The description also notes (default: true) but the schema is marked Default: false. Looking at the code below it appears the description should be updated.
• Should we note that this will be enabled by default in the next major version and add a code comment so that gets done?

[bflad]

For the first item, the HTTP server might need a way to reject connections except those from the proxy.

[detro]

1. will add tests to use t.Setenv 👍
2. Will fix the description
3. I'm not sure, my intention wasn't to make the from_env default: are you suggesting it should be made the default in a major release?
4. I can look into ways to orchestrate a "reject if not via proxy": I can try to play with headers injection more, but I have noticed that, for what our provider does, we never get to exchange http body - so requests never actually get to exchange headers. Will give it another pass.

[bflad]

my intention wasn't to make the from_env default: are you suggesting it should be made the default in a major release?

I think we should certainly consider this for the next major version. 😄 Many pieces of software handling HTTP calls will conventionally support the NO_PROXY/HTTP_PROXY/HTTPS_PROXY environment variables without additional configuration. github.com/hashicorp/go-cleanhttp default clients for example, implement net/http.ProxyFromEnvironment, which I believe is used by the majority of HTTP handling within Terraform CLI itself.

[detro]

So, the last point you make is great, and it plays well with what we decided: to not use an HTTP client unless a Proxy is configured (YET!).

Enabling ProxyFromEnvironment by default would also change the current behaviour, so we can't do it yet.

So, I think now I follow better what you head in mind earlier when you suggested:

Should we note that this will be enabled by default in the next major version and add a code comment so that gets done?

In the light of this, I think I'll do the following:

• by default, if no proxy config is provided, we keep using tls.Client
• if a proxy is configured, then we switch the logic to use http.Client
• from_env is defaulted to false

Then, I'll create a follow up issue for breaking changes that we want to launch in 4.x (similar to this one). In this issue I'll include:

• from_env becomes defaulted to true
• the URL scheme defines the client we will use: this means https:// will always use the http.Client, while tls:// will always use tls.Client

Does this strategy sound good? Am I missing something?

[detro]

Here is the issue #183

[detro]

@bflad I pushed the changes as discussed above, but I need to call out a few things:

• no point in checking status code when what our code is trying to do is grab the certificates exchanged as part of the TLS handshake: we can rely in our logic exclusively on the errs returned
• if you look at the set of tests, they should prove all the error scenarios you mentioned (authentication required + unreachable proxy)
• I have created Make provider proxy.from_env = true default + enforce "scheme determines client" in tls_certificate data source  #183 as the follow up (things to change in the major release)

I have started accumulating the "major release stuff" in a 4.0.0 milestone.

[bflad]

Looks good to me 🚀

[bflad]

I still think the next major version should still perform this type of check and use the TLS client whenever possible for simplicity -- the check could see if http.ProxyFromEnvironment would actually use a proxy for the request by checking for nil.

[detro]

The reason I insist on using an actual HTTP client, is that we were and are requiring a scheme for the url argument. The fact that practitioners have to write https:// is, in my opinion, an implicit implication we are making.

The original implementation simply didn't respect that. It would have made more sense if there was no scheme, and then it only (and exclusively) used tls.Client. Though that would have prevented even talking about supporting HTTP Proxies.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.