v3.2.0 No Longer Supports Proxy Settings #198
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ import ( | |
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/elazarl/goproxy" | ||
| "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" | ||
| ) | ||
|
|
||
|
|
@@ -471,6 +472,36 @@ func TestDataSource_UnsupportedInsecureCaCert(t *testing.T) { | |
| }) | ||
| } | ||
|
|
||
| func TestDataSource_HTTPViaProxyWithEnv(t *testing.T) { | ||
| svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.WriteHeader(http.StatusOK) | ||
| })) | ||
| defer svr.Close() | ||
|
|
||
| p := goproxy.NewProxyHttpServer() | ||
|
|
||
| proxy := httptest.NewServer(p) | ||
| defer proxy.Close() | ||
|
There was a problem hiding this comment. Over in the terraform-provider-tls acceptance testing, all the testing HTTP servers, including the proxies, have connection counting: Then there is a I think we'll need something similar here to verify that the proxy is correctly being used, unless there is a better testing strategy. There was a problem hiding this comment. I'm looking into the current test failures (e.g., https://github.com/hashicorp/terraform-provider-http/actions/runs/3376234811/jobs/5603729486). I can't reproduce this failure locally, I see a different failure. There was a problem hiding this comment. I see the same failures in CI (e.g., https://github.com/hashicorp/terraform-provider-http/actions/runs/3376368850/jobs/5604018894) when using But all tests pass when running locally with this configuration. |
||
|
|
||
| t.Setenv("HTTP_PROXY", proxy.URL) | ||
| t.Setenv("HTTPS_PROXY", proxy.URL) | ||
|
|
||
| resource.UnitTest(t, resource.TestCase{ | ||
| ProtoV5ProviderFactories: protoV5ProviderFactories(), | ||
|
|
||
| Steps: []resource.TestStep{ | ||
| { | ||
| Config: fmt.Sprintf(` | ||
| data "http" "http_test" { | ||
| url = "%s" | ||
| } | ||
| `, svr.URL), | ||
| Check: resource.TestCheckResourceAttr("data.http.http_test", "status_code", "200"), | ||
|
There was a problem hiding this comment. This test looks correct to me, but for some reason is unhappy on Terraform version 0.14: I'll try to do some investigative work with my end of day. There was a problem hiding this comment. 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: 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 svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "text/plain")
w.WriteHeader(http.StatusOK)
}))There was a problem hiding this comment. Thanks for the investigation. Have added the fix you suggested. |
||
| }, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| type TestHttpMock struct { | ||
| server *httptest.Server | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, andForceAttemptHTTP2which will also default to different values than those from older provider versions?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also recommend using
http.DefaultTransportto 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 mutatedhttp.DefaultTransport.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theTransportso in order to avoid changes in behaviour this has not been used at this time.