v3.2.0 No Longer Supports Proxy Settings #198

bendbennett · 2022-11-01T18:09:50Z

Closes: #197

dcaswell-square · 2022-11-01T18:26:07Z

internal/provider/data_source_http.go

@@ -163,6 +163,7 @@ func (d *httpDataSource) Read(ctx context.Context, req datasource.ReadRequest, r
 	caCertificate := model.CaCertificate

 	tr := &http.Transport{
+		Proxy:           http.ProxyFromEnvironment,


I'm not sure this is the most complete way to handle behavior differences that were introduced in v3.2.0, as the Proxy is only one of several properties that are set on the DefaultTransport (https://github.com/golang/go/blob/f3c39a83a3076eb560c7f687cbb35eef9b506e7d/src/net/http/transport.go#L38-L54) that are no longer going to be consistent with the previous version behavior. Ignoring the proxy values was the most immediately noticeable to users, but what about all of the timeouts, MaxIdleConns, and ForceAttemptHTTP2 which will also default to different values than those from older provider versions?

I would also recommend using http.DefaultTransport to prevent other potential behavior changes. There could also be consideration for using https://pkg.go.dev/github.com/hashicorp/go-cleanhttp similar to other HashiCorp maintained Go modules to prevent potential issues with multiple host connections across data sources or other regressions involving a mutated http.DefaultTransport.

Have switched to using http.DefaultTransport. I took a look at https://pkg.go.dev/github.com/hashicorp/go-cleanhttp but it seems that there are differences in the configuration of the Transport so in order to avoid changes in behaviour this has not been used at this time.

bflad · 2022-11-01T20:11:56Z

internal/provider/data_source_http_test.go

+	p := goproxy.NewProxyHttpServer()
+
+	proxy := httptest.NewServer(p)
+	defer proxy.Close()


Over in the terraform-provider-tls acceptance testing, all the testing HTTP servers, including the proxies, have connection counting:

https://github.com/hashicorp/terraform-provider-tls/blob/b9cbb62bc2a8b90f6af91e86047082a5357e8c2f/internal/provider/testutils/local_server.go#L38-L46

Then there is a TestCheckFunc which ensures that the "real" HTTP server and proxy HTTP server have matching connection counts:

https://github.com/hashicorp/terraform-provider-tls/blob/b9cbb62bc2a8b90f6af91e86047082a5357e8c2f/internal/provider/testutils/local_server.go#L117-L127

I think we'll need something similar here to verify that the proxy is correctly being used, unless there is a better testing strategy.

I'm looking into the current test failures (e.g., https://github.com/hashicorp/terraform-provider-http/actions/runs/3376234811/jobs/5603729486).

Check 2/2 error: expected server and proxy active connection count to match: server was 1, while proxy was 0

I can't reproduce this failure locally, I see a different failure.

Check 2/2 error: expected server and proxy active connection count to match: server was 3, while proxy was 1

I see the same failures in CI (e.g., https://github.com/hashicorp/terraform-provider-http/actions/runs/3376368850/jobs/5604018894) when using

tr := &http.Transport{ Proxy: http.ProxyFromEnvironment, }

Check 2/2 error: expected server and proxy active connection count to match: server was 1, while proxy was 0

But all tests pass when running locally with this configuration.

bflad · 2022-11-01T20:20:45Z

internal/provider/data_source_http.go

@@ -163,6 +163,7 @@ func (d *httpDataSource) Read(ctx context.Context, req datasource.ReadRequest, r
 	caCertificate := model.CaCertificate

 	tr := &http.Transport{
+		Proxy:           http.ProxyFromEnvironment,


I would also recommend using http.DefaultTransport to prevent other potential behavior changes. There could also be consideration for using https://pkg.go.dev/github.com/hashicorp/go-cleanhttp similar to other HashiCorp maintained Go modules to prevent potential issues with multiple host connections across data sources or other regressions involving a mutated http.DefaultTransport.

bflad · 2022-11-01T20:23:45Z

internal/provider/data_source_http_test.go

+						url = "%s"
+					}
+				`, svr.URL),
+				Check: resource.TestCheckResourceAttr("data.http.http_test", "status_code", "200"),


This test looks correct to me, but for some reason is unhappy on Terraform version 0.14:

=== RUN TestDataSource_HTTPViaProxyWithEnv data_source_http_test.go:489: Step 1/1 error: Check failed: Not found: data.http.http_test in [root] --- FAIL: TestDataSource_HTTPViaProxyWithEnv (0.40s)

I'll try to do some investigative work with my end of day.

The quirk with Terraform version 0.14.x is that it will not set the Terraform state for a data source which returns a warning diagnostic. The warning being generated here is:

2022-11-01T16:50:56.310-0400 [TRACE] sdk.proto: Received downstream response: diagnostic_error_count=0 diagnostic_warning_count=1 tf_req_duration_ms=0 tf_data_source_type=http tf_req_id=5887d459-0e1a-5aa1-0241-13d21f8698ca tf_provider_addr=registry.terraform.io/hashicorp/http tf_proto_version=5.3 tf_rpc=ReadDataSource 2022-11-01T16:50:56.310-0400 [WARN] sdk.proto: Response contains warning diagnostic: tf_provider_addr=registry.terraform.io/hashicorp/http tf_proto_version=5.3 tf_rpc=ReadDataSource tf_data_source_type=http diagnostic_severity=WARNING diagnostic_summary="Content-Type is not recognized as a text type, got \"\"" diagnostic_detail="If the content is binary data, Terraform may not properly handle the contents of the response." tf_req_id=5887d459-0e1a-5aa1-0241-13d21f8698ca 2022-11-01T16:50:56.310-0400 [TRACE] sdk.proto: Served request: tf_provider_addr=registry.terraform.io/hashicorp/http tf_proto_version=5.3 tf_rpc=ReadDataSource tf_data_source_type=http tf_req_id=5887d459-0e1a-5aa1-0241-13d21f8698ca

The data source correctly does not exit early in this case. The testing fix for this is to ensure the HTTP response also includes a valid Content-Type header, e.g.

svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "text/plain") w.WriteHeader(http.StatusOK) }))

Thanks for the investigation. Have added the fix you suggested.

…#197)

…st failures are attributable to using http DefaultTransport (#197)

…g during testing (#197)

…ng during testing (#197)

bflad

One more little consideration, otherwise looks good to me 🚀

internal/provider/data_source_http.go

bflad · 2022-11-03T18:28:55Z

internal/provider/data_source_http.go

+		return httpproxy.FromEnvironment().ProxyFunc()(req.URL)
+	}
+
+	if clonedTr.TLSClientConfig == nil {


Nit: To further prevent any differences due version to 3.2.0 changes, it is mentioned in the TLSClientConfig documentation that:

// If nil, the default configuration is used.
// If non-nil, HTTP/2 support may not be enabled by default.

I think we may want to consider moving this nil check inside each of the !IsNull() branches below to prevent any sort of further HTTP/2 oddities when there is no custom TLS configuration.

internal/provider/data_source_http.go

bflad · 2022-11-03T18:48:05Z

My editor is also prompting to run go mod tidy, which moves golang.org/x/net v0.0.0-20220708220712-1185a9018129 to the direct dependencies. While we're doing that, we may want to consider updating it to its first tagged release (v0.1.0; go get golang.org/x/net@v0.1.0), so dependabot can appropriately prompt us for updates. I'm not seeing anything in golang/net@1185a90...v0.1.0 that gives me pause.

RaphaelDucay · 2022-11-03T23:40:00Z

Any date in mind for the release of the fix ? Thx in advance

Co-authored-by: Brian Flad <bflad417@gmail.com>

bendbennett · 2022-11-04T08:21:15Z

Any date in mind for the release of the fix ? Thx in advance

Hi @RaphaelDucay 👋
We are currently finalising the changes and hope to have the fix released in the next few days.

github-actions · 2024-05-26T15:37:39Z

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.

Adding ProxyFromEnvironment to http transport (#197)

c93e1b2

bendbennett requested a review from a team as a code owner November 1, 2022 18:09

github-actions bot added dependencies size/S labels Nov 1, 2022

dcaswell-square reviewed Nov 1, 2022

View reviewed changes

bflad reviewed Nov 1, 2022

View reviewed changes

bflad suggested changes Nov 1, 2022

View reviewed changes

Switching to using DefaultTransport (#197)

7f7b4c1

github-actions bot added size/L and removed size/S labels Nov 2, 2022

bendbennett added 4 commits November 2, 2022 09:11

Reinstating equivalency check for server and proxy active connections (…

dafa614

…#197)

Using http Transport with ProxyFromEnvironment set to determine if te…

0d80943

…st failures are attributable to using http DefaultTransport (#197)

Switching to using httputil single host reverse proxy for testing (#197)

015f875

Removing unneeded files and moving test (#197)

31abbf1

github-actions bot added size/M and removed dependencies size/L labels Nov 3, 2022

Adding some clarifying comments to test (#197)

18bcde8

bendbennett requested a review from bflad November 3, 2022 12:47

bendbennett added 3 commits November 3, 2022 13:14

Updating CHANGELOG.md (#197)

0914057

Use func to obtain proxy from environment to avoid issues with cachin…

837d6b2

…g during testing (#197)

Cloning default transport before modifying to avoid issues with cachi…

80082d5

…ng during testing (#197)

bflad approved these changes Nov 3, 2022

View reviewed changes

bflad added the bug label Nov 3, 2022

bendbennett and others added 2 commits November 4, 2022 08:09

Apply suggestions from code review

38b9b1d

Co-authored-by: Brian Flad <bflad417@gmail.com>

Bumping golang.org/x/net to v0.1.0 (#197)

82d62de

github-actions bot added the dependencies label Nov 4, 2022

bendbennett added this to the v3.2.1 milestone Nov 7, 2022

bendbennett merged commit bdf31bd into main Nov 7, 2022

bendbennett deleted the bendbennett/issues-197 branch November 7, 2022 15:17

github-actions bot locked as resolved and limited conversation to collaborators May 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v3.2.0 No Longer Supports Proxy Settings #198

v3.2.0 No Longer Supports Proxy Settings #198

bendbennett commented Nov 1, 2022

dcaswell-square Nov 1, 2022 •

edited

Loading

bflad Nov 1, 2022

bendbennett Nov 2, 2022

bflad Nov 1, 2022

bendbennett Nov 2, 2022 •

edited

Loading

bendbennett Nov 2, 2022 •

edited

Loading

bflad Nov 1, 2022

bflad Nov 1, 2022

bflad Nov 1, 2022

bendbennett Nov 2, 2022

bflad left a comment

bflad Nov 3, 2022

bflad commented Nov 3, 2022

RaphaelDucay commented Nov 3, 2022

bendbennett commented Nov 4, 2022

github-actions bot commented May 26, 2024

v3.2.0 No Longer Supports Proxy Settings #198

v3.2.0 No Longer Supports Proxy Settings #198

Conversation

bendbennett commented Nov 1, 2022

dcaswell-square Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendbennett Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

bendbennett Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Nov 3, 2022

RaphaelDucay commented Nov 3, 2022

bendbennett commented Nov 4, 2022

github-actions bot commented May 26, 2024

dcaswell-square Nov 1, 2022 •

edited

Loading

bendbennett Nov 2, 2022 •

edited

Loading

bendbennett Nov 2, 2022 •

edited

Loading