From 395416dca07a08c619cff20df027112a9382f11f Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Thu, 12 Oct 2023 13:45:42 -0700 Subject: [PATCH 01/14] Adds proxy configuration tests --- go.mod | 1 + go.sum | 2 + http_client_test.go | 16 +++ internal/config/config.go | 6 ++ internal/test/http_client.go | 174 +++++++++++++++++++++++++++++++ v2/awsv1shim/go.mod | 1 + v2/awsv1shim/go.sum | 2 + v2/awsv1shim/http_client_test.go | 20 ++++ 8 files changed, 222 insertions(+) diff --git a/go.mod b/go.mod index a743bf6b..770f7e7d 100644 --- a/go.mod +++ b/go.mod @@ -21,6 +21,7 @@ require ( go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws v0.45.0 go.opentelemetry.io/otel v1.19.0 golang.org/x/exp v0.0.0-20230905200255-921286631fa9 + golang.org/x/net v0.17.0 golang.org/x/text v0.13.0 ) diff --git a/go.sum b/go.sum index 86465750..0f8ba4d8 100644 --- a/go.sum +++ b/go.sum @@ -97,6 +97,8 @@ go.opentelemetry.io/otel/trace v1.19.0 h1:DFVQmlVbfVeOuBRrwdtaehRrWiL1JoVs9CPIQ1 go.opentelemetry.io/otel/trace v1.19.0/go.mod h1:mfaSyvGyEJEI0nyV2I4qhNQnbBOUUmYZpYojqMnX2vo= golang.org/x/exp v0.0.0-20230905200255-921286631fa9 h1:GoHiUyI/Tp2nVkLI2mCxVkOjsbSXD66ic0XW0js0R9g= golang.org/x/exp v0.0.0-20230905200255-921286631fa9/go.mod h1:S2oDrQGGwySpoQPVqRShND87VCbxmc6bL1Yd2oYrm6k= +golang.org/x/net v0.17.0 h1:pVaXccu2ozPjCXewfr1S7xza/zcXTity9cCdXQYSjIM= +golang.org/x/net v0.17.0/go.mod h1:NxSsAGuq816PNPmqtQdLE42eU2Fs7NoRIZrHJAlaCOE= golang.org/x/sys v0.0.0-20200116001909-b77594299b42/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200223170610-d5e6a3e2c0ae/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210630005230-0f9fa26af87c/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= diff --git a/http_client_test.go b/http_client_test.go index 36e45d64..b04ec420 100644 --- a/http_client_test.go +++ b/http_client_test.go @@ -4,6 +4,7 @@ package awsbase import ( + "net/http" "testing" "github.com/hashicorp/aws-sdk-go-base/v2/internal/config" @@ -33,3 +34,18 @@ func TestHTTPClientConfiguration_insecureHTTPS(t *testing.T) { test.HTTPClientConfigurationTest_insecureHTTPS(t, transport) } + +func TestHTTPClientConfiguration_proxy(t *testing.T) { + test.HTTPClientConfigurationTest_proxy(t, transport) +} + +func transport(t *testing.T, config *config.Config) *http.Transport { + t.Helper() + + client, err := defaultHttpClient(config) + if err != nil { + t.Fatalf("creating client: %s", err) + } + + return client.GetTransport() +} diff --git a/internal/config/config.go b/internal/config/config.go index 145aa2c4..dc88fce1 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -17,6 +17,7 @@ import ( "github.com/aws/aws-sdk-go-v2/feature/ec2/imds" "github.com/hashicorp/aws-sdk-go-base/v2/internal/expand" "github.com/hashicorp/aws-sdk-go-base/v2/logging" + "golang.org/x/net/http/httpproxy" ) type Config struct { @@ -112,6 +113,11 @@ func (c Config) HTTPTransportOptions() (func(*http.Transport), error) { if proxyUrl != nil { tr.Proxy = http.ProxyURL(proxyUrl) + } else { + proxyConfig := httpproxy.FromEnvironment() + tr.Proxy = func(req *http.Request) (*url.URL, error) { + return proxyConfig.ProxyFunc()(req.URL) + } } } diff --git a/internal/test/http_client.go b/internal/test/http_client.go index fd04a596..35c3a0b5 100644 --- a/internal/test/http_client.go +++ b/internal/test/http_client.go @@ -9,9 +9,15 @@ import ( "testing" awshttp "github.com/aws/aws-sdk-go-v2/aws/transport/http" + "github.com/hashicorp/aws-sdk-go-base/v2/internal/config" + "github.com/hashicorp/aws-sdk-go-base/v2/servicemocks" ) +type TransportGetter func(t *testing.T, config *config.Config) *http.Transport + func HTTPClientConfigurationTest_basic(t *testing.T, transport *http.Transport) { + t.Helper() + if a, e := transport.MaxIdleConns, awshttp.DefaultHTTPTransportMaxIdleConns; a != e { t.Errorf("expected MaxIdleConns to be %d, got %d", e, a) } @@ -44,8 +50,176 @@ func HTTPClientConfigurationTest_basic(t *testing.T, transport *http.Transport) } func HTTPClientConfigurationTest_insecureHTTPS(t *testing.T, transport *http.Transport) { + t.Helper() + tlsConfig := transport.TLSClientConfig if !tlsConfig.InsecureSkipVerify { t.Error("expected InsecureSkipVerify to be true, got false") } } + +type proxyCase struct { + url string + expectProxy bool +} + +func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { + t.Helper() + + testcases := map[string]struct { + config config.Config + environmentVariables map[string]string + urls []proxyCase + }{ + "no config": { + config: config.Config{}, + urls: []proxyCase{ + { + url: "http://example.com", + expectProxy: false, + }, + { + url: "https://example.com", + expectProxy: false, + }, + }, + }, + + "proxy config": { + config: config.Config{ + HTTPProxy: "http://the-proxy.test:1234", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectProxy: true, + }, + { + url: "https://example.com", + expectProxy: true, + }, + }, + }, + + "HTTP_PROXY envvar": { + config: config.Config{}, + environmentVariables: map[string]string{ + "HTTP_PROXY": "http://the-proxy.test:1234", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectProxy: true, + }, + { + url: "https://example.com", + expectProxy: false, + }, + }, + }, + + "HTTPS_PROXY envvar": { + config: config.Config{}, + environmentVariables: map[string]string{ + "HTTPS_PROXY": "http://the-proxy.test:1234", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectProxy: false, + }, + { + url: "https://example.com", + expectProxy: true, + }, + }, + }, + + "proxy config NO_PROXY envvar": { + config: config.Config{ + HTTPProxy: "http://the-proxy.test:1234", + }, + environmentVariables: map[string]string{ + "NO_PROXY": "http://dont-proxy.test", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectProxy: true, + }, + { + url: "http://dont-proxy.test", + expectProxy: true, + }, + { + url: "https://example.com", + expectProxy: true, + }, + { + url: "https://dont-proxy.test", + expectProxy: true, + }, + }, + }, + + "HTTP_PROXY envvar HTTPS_PROXY envvar NO_PROXY envvar": { + config: config.Config{}, + environmentVariables: map[string]string{ + "HTTP_PROXY": "http://the-proxy.test:1234", + "HTTPS_PROXY": "http://the-proxy.test:1234", + "NO_PROXY": "dont-proxy.test", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectProxy: true, + }, + { + url: "http://dont-proxy.test", + expectProxy: false, + }, + { + url: "https://example.com", + expectProxy: true, + }, + { + url: "https://dont-proxy.test", + expectProxy: false, + }, + }, + }, + } + + for name, testcase := range testcases { + testcase := testcase + + t.Run(name, func(t *testing.T) { + oldEnv := servicemocks.InitSessionTestEnv() + defer servicemocks.PopEnv(oldEnv) + + for k, v := range testcase.environmentVariables { + t.Setenv(k, v) + } + + transport := getter(t, &testcase.config) + proxy := transport.Proxy + + for _, url := range testcase.urls { + req, _ := http.NewRequest("GET", url.url, nil) + pUrl, err := proxy(req) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + if url.expectProxy { + if pUrl == nil { + t.Errorf("expected proxy for %q, got none", url.url) + } + } else { + if pUrl != nil { + t.Errorf("expected no proxy for %q, got %q", url.url, pUrl.String()) + } + } + } + }) + } +} diff --git a/v2/awsv1shim/go.mod b/v2/awsv1shim/go.mod index 97e3f979..b32f1d0c 100644 --- a/v2/awsv1shim/go.mod +++ b/v2/awsv1shim/go.mod @@ -49,6 +49,7 @@ require ( go.opentelemetry.io/otel/metric v1.19.0 // indirect go.opentelemetry.io/otel/trace v1.19.0 // indirect golang.org/x/exp v0.0.0-20230905200255-921286631fa9 // indirect + golang.org/x/net v0.17.0 // indirect golang.org/x/sys v0.13.0 // indirect golang.org/x/text v0.13.0 // indirect ) diff --git a/v2/awsv1shim/go.sum b/v2/awsv1shim/go.sum index 168eb256..52550908 100644 --- a/v2/awsv1shim/go.sum +++ b/v2/awsv1shim/go.sum @@ -110,6 +110,8 @@ golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLL golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c= golang.org/x/net v0.1.0/go.mod h1:Cx3nUiGt4eDBEyega/BKRp+/AlGL8hYe7U9odMt2Cco= +golang.org/x/net v0.17.0 h1:pVaXccu2ozPjCXewfr1S7xza/zcXTity9cCdXQYSjIM= +golang.org/x/net v0.17.0/go.mod h1:NxSsAGuq816PNPmqtQdLE42eU2Fs7NoRIZrHJAlaCOE= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= diff --git a/v2/awsv1shim/http_client_test.go b/v2/awsv1shim/http_client_test.go index 3d752acb..9da0d853 100644 --- a/v2/awsv1shim/http_client_test.go +++ b/v2/awsv1shim/http_client_test.go @@ -40,3 +40,23 @@ func TestHTTPClientConfiguration_insecureHTTPS(t *testing.T) { test.HTTPClientConfigurationTest_insecureHTTPS(t, transport) } + +func TestHTTPClientConfiguration_proxy(t *testing.T) { + test.HTTPClientConfigurationTest_proxy(t, transport) +} + +func transport(t *testing.T, config *config.Config) *http.Transport { + t.Helper() + + client, err := defaultHttpClient(config) + if err != nil { + t.Fatalf("creating client: %s", err) + } + + transport, ok := client.Transport.(*http.Transport) + if !ok { + t.Fatalf("Unexpected type for HTTP client transport: %T", client.Transport) + } + + return transport +} From b591e4e17ed7590fdd156c488d61ff10e4320e5a Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Thu, 12 Oct 2023 13:57:27 -0700 Subject: [PATCH 02/14] Checks for proxy URL --- internal/test/http_client.go | 84 ++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 41 deletions(-) diff --git a/internal/test/http_client.go b/internal/test/http_client.go index 35c3a0b5..6f837416 100644 --- a/internal/test/http_client.go +++ b/internal/test/http_client.go @@ -59,8 +59,8 @@ func HTTPClientConfigurationTest_insecureHTTPS(t *testing.T, transport *http.Tra } type proxyCase struct { - url string - expectProxy bool + url string + expectedProxy string } func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { @@ -75,28 +75,28 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { config: config.Config{}, urls: []proxyCase{ { - url: "http://example.com", - expectProxy: false, + url: "http://example.com", + expectedProxy: "", }, { - url: "https://example.com", - expectProxy: false, + url: "https://example.com", + expectedProxy: "", }, }, }, "proxy config": { config: config.Config{ - HTTPProxy: "http://the-proxy.test:1234", + HTTPProxy: "http://http-proxy.test:1234", }, urls: []proxyCase{ { - url: "http://example.com", - expectProxy: true, + url: "http://example.com", + expectedProxy: "http://http-proxy.test:1234", }, { - url: "https://example.com", - expectProxy: true, + url: "https://example.com", + expectedProxy: "http://http-proxy.test:1234", }, }, }, @@ -104,16 +104,16 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { "HTTP_PROXY envvar": { config: config.Config{}, environmentVariables: map[string]string{ - "HTTP_PROXY": "http://the-proxy.test:1234", + "HTTP_PROXY": "http://http-proxy.test:1234", }, urls: []proxyCase{ { - url: "http://example.com", - expectProxy: true, + url: "http://example.com", + expectedProxy: "http://http-proxy.test:1234", }, { - url: "https://example.com", - expectProxy: false, + url: "https://example.com", + expectedProxy: "", }, }, }, @@ -121,43 +121,43 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { "HTTPS_PROXY envvar": { config: config.Config{}, environmentVariables: map[string]string{ - "HTTPS_PROXY": "http://the-proxy.test:1234", + "HTTPS_PROXY": "http://https-proxy.test:1234", }, urls: []proxyCase{ { - url: "http://example.com", - expectProxy: false, + url: "http://example.com", + expectedProxy: "", }, { - url: "https://example.com", - expectProxy: true, + url: "https://example.com", + expectedProxy: "http://https-proxy.test:1234", }, }, }, "proxy config NO_PROXY envvar": { config: config.Config{ - HTTPProxy: "http://the-proxy.test:1234", + HTTPProxy: "http://http-proxy.test:1234", }, environmentVariables: map[string]string{ "NO_PROXY": "http://dont-proxy.test", }, urls: []proxyCase{ { - url: "http://example.com", - expectProxy: true, + url: "http://example.com", + expectedProxy: "http://http-proxy.test:1234", }, { - url: "http://dont-proxy.test", - expectProxy: true, + url: "http://dont-proxy.test", + expectedProxy: "http://http-proxy.test:1234", }, { - url: "https://example.com", - expectProxy: true, + url: "https://example.com", + expectedProxy: "http://http-proxy.test:1234", }, { - url: "https://dont-proxy.test", - expectProxy: true, + url: "https://dont-proxy.test", + expectedProxy: "http://http-proxy.test:1234", }, }, }, @@ -165,26 +165,26 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { "HTTP_PROXY envvar HTTPS_PROXY envvar NO_PROXY envvar": { config: config.Config{}, environmentVariables: map[string]string{ - "HTTP_PROXY": "http://the-proxy.test:1234", - "HTTPS_PROXY": "http://the-proxy.test:1234", + "HTTP_PROXY": "http://http-proxy.test:1234", + "HTTPS_PROXY": "http://https-proxy.test:1234", "NO_PROXY": "dont-proxy.test", }, urls: []proxyCase{ { - url: "http://example.com", - expectProxy: true, + url: "http://example.com", + expectedProxy: "http://http-proxy.test:1234", }, { - url: "http://dont-proxy.test", - expectProxy: false, + url: "http://dont-proxy.test", + expectedProxy: "", }, { - url: "https://example.com", - expectProxy: true, + url: "https://example.com", + expectedProxy: "http://https-proxy.test:1234", }, { - url: "https://dont-proxy.test", - expectProxy: false, + url: "https://dont-proxy.test", + expectedProxy: "", }, }, }, @@ -210,9 +210,11 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { if err != nil { t.Fatalf("unexpected error: %s", err) } - if url.expectProxy { + if url.expectedProxy != "" { if pUrl == nil { t.Errorf("expected proxy for %q, got none", url.url) + } else if pUrl.String() != url.expectedProxy { + t.Errorf("expected proxy %q for %q, got %q", url.expectedProxy, url.url, pUrl.String()) } } else { if pUrl != nil { From ab1034c11c50bd96ae213967848e235499622e85 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Thu, 12 Oct 2023 14:00:45 -0700 Subject: [PATCH 03/14] Adds test for config overriding envvar --- internal/test/http_client.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/internal/test/http_client.go b/internal/test/http_client.go index 6f837416..eccdb204 100644 --- a/internal/test/http_client.go +++ b/internal/test/http_client.go @@ -188,6 +188,25 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, }, + + "proxy config overrides HTTP_PROXY envvar": { + config: config.Config{ + HTTPProxy: "http://config-proxy.test:1234", + }, + environmentVariables: map[string]string{ + "HTTP_PROXY": "http://envvar-proxy.test:1234", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectedProxy: "http://config-proxy.test:1234", + }, + { + url: "https://example.com", + expectedProxy: "http://config-proxy.test:1234", + }, + }, + }, } for name, testcase := range testcases { From 462565d6ace0fbd8521756b591c1effb0175479a Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Thu, 12 Oct 2023 14:09:37 -0700 Subject: [PATCH 04/14] Supports `NO_PROXY` with `HTTPProxy` config parameter --- internal/config/config.go | 7 ++++++- internal/test/http_client.go | 6 +++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index dc88fce1..d1dd53a9 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -112,7 +112,12 @@ func (c Config) HTTPTransportOptions() (func(*http.Transport), error) { } if proxyUrl != nil { - tr.Proxy = http.ProxyURL(proxyUrl) + proxyConfig := httpproxy.FromEnvironment() + proxyConfig.HTTPProxy = proxyUrl.String() + proxyConfig.HTTPSProxy = proxyUrl.String() + tr.Proxy = func(req *http.Request) (*url.URL, error) { + return proxyConfig.ProxyFunc()(req.URL) + } } else { proxyConfig := httpproxy.FromEnvironment() tr.Proxy = func(req *http.Request) (*url.URL, error) { diff --git a/internal/test/http_client.go b/internal/test/http_client.go index eccdb204..1c1a1a22 100644 --- a/internal/test/http_client.go +++ b/internal/test/http_client.go @@ -140,7 +140,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { HTTPProxy: "http://http-proxy.test:1234", }, environmentVariables: map[string]string{ - "NO_PROXY": "http://dont-proxy.test", + "NO_PROXY": "dont-proxy.test", }, urls: []proxyCase{ { @@ -149,7 +149,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, { url: "http://dont-proxy.test", - expectedProxy: "http://http-proxy.test:1234", + expectedProxy: "", }, { url: "https://example.com", @@ -157,7 +157,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, { url: "https://dont-proxy.test", - expectedProxy: "http://http-proxy.test:1234", + expectedProxy: "", }, }, }, From d7d4f71ebf25fe2ff9927691f049f51110c7234b Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Thu, 12 Oct 2023 14:20:08 -0700 Subject: [PATCH 05/14] Support `HTTPS_PROXY` with `HTTPProxy` config parameter --- internal/config/config.go | 4 +++- internal/test/http_client.go | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/internal/config/config.go b/internal/config/config.go index d1dd53a9..b8f2b338 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -114,7 +114,9 @@ func (c Config) HTTPTransportOptions() (func(*http.Transport), error) { if proxyUrl != nil { proxyConfig := httpproxy.FromEnvironment() proxyConfig.HTTPProxy = proxyUrl.String() - proxyConfig.HTTPSProxy = proxyUrl.String() + if proxyConfig.HTTPSProxy == "" { + proxyConfig.HTTPSProxy = proxyUrl.String() + } tr.Proxy = func(req *http.Request) (*url.URL, error) { return proxyConfig.ProxyFunc()(req.URL) } diff --git a/internal/test/http_client.go b/internal/test/http_client.go index 1c1a1a22..665fdfb1 100644 --- a/internal/test/http_client.go +++ b/internal/test/http_client.go @@ -135,6 +135,25 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, + "proxy config HTTPS_PROXY envvar": { + config: config.Config{ + HTTPProxy: "http://http-proxy.test:1234", + }, + environmentVariables: map[string]string{ + "HTTPS_PROXY": "http://https-proxy.test:1234", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectedProxy: "http://http-proxy.test:1234", + }, + { + url: "https://example.com", + expectedProxy: "http://https-proxy.test:1234", + }, + }, + }, + "proxy config NO_PROXY envvar": { config: config.Config{ HTTPProxy: "http://http-proxy.test:1234", From a007d478cfc871ca6c73b87a4fc95b02a54087a5 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Thu, 12 Oct 2023 14:21:00 -0700 Subject: [PATCH 06/14] Adds tests for lower-case environment variables --- internal/test/http_client.go | 81 ++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/internal/test/http_client.go b/internal/test/http_client.go index 665fdfb1..7436ed52 100644 --- a/internal/test/http_client.go +++ b/internal/test/http_client.go @@ -66,6 +66,7 @@ type proxyCase struct { func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { t.Helper() + // Go supports both the upper- and lower-case versions of the proxy environment variables testcases := map[string]struct { config config.Config environmentVariables map[string]string @@ -118,6 +119,23 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, + "http_proxy envvar": { + config: config.Config{}, + environmentVariables: map[string]string{ + "http_proxy": "http://http-proxy.test:1234", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectedProxy: "http://http-proxy.test:1234", + }, + { + url: "https://example.com", + expectedProxy: "", + }, + }, + }, + "HTTPS_PROXY envvar": { config: config.Config{}, environmentVariables: map[string]string{ @@ -135,6 +153,23 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, + "https_proxy envvar": { + config: config.Config{}, + environmentVariables: map[string]string{ + "https_proxy": "http://https-proxy.test:1234", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectedProxy: "", + }, + { + url: "https://example.com", + expectedProxy: "http://https-proxy.test:1234", + }, + }, + }, + "proxy config HTTPS_PROXY envvar": { config: config.Config{ HTTPProxy: "http://http-proxy.test:1234", @@ -154,6 +189,25 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, + "proxy config https_proxy envvar": { + config: config.Config{ + HTTPProxy: "http://http-proxy.test:1234", + }, + environmentVariables: map[string]string{ + "https_proxy": "http://https-proxy.test:1234", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectedProxy: "http://http-proxy.test:1234", + }, + { + url: "https://example.com", + expectedProxy: "http://https-proxy.test:1234", + }, + }, + }, + "proxy config NO_PROXY envvar": { config: config.Config{ HTTPProxy: "http://http-proxy.test:1234", @@ -181,6 +235,33 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, + "proxy config no_proxy envvar": { + config: config.Config{ + HTTPProxy: "http://http-proxy.test:1234", + }, + environmentVariables: map[string]string{ + "no_proxy": "dont-proxy.test", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectedProxy: "http://http-proxy.test:1234", + }, + { + url: "http://dont-proxy.test", + expectedProxy: "", + }, + { + url: "https://example.com", + expectedProxy: "http://http-proxy.test:1234", + }, + { + url: "https://dont-proxy.test", + expectedProxy: "", + }, + }, + }, + "HTTP_PROXY envvar HTTPS_PROXY envvar NO_PROXY envvar": { config: config.Config{}, environmentVariables: map[string]string{ From ea4accd14b122cf4cdf21f2b1bf338c678c1d776 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Thu, 12 Oct 2023 14:35:19 -0700 Subject: [PATCH 07/14] Cleans up proxy configuration --- internal/config/config.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index b8f2b338..0c62a420 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -111,20 +111,15 @@ func (c Config) HTTPTransportOptions() (func(*http.Transport), error) { tr.TLSClientConfig.InsecureSkipVerify = true } + proxyConfig := httpproxy.FromEnvironment() if proxyUrl != nil { - proxyConfig := httpproxy.FromEnvironment() proxyConfig.HTTPProxy = proxyUrl.String() if proxyConfig.HTTPSProxy == "" { proxyConfig.HTTPSProxy = proxyUrl.String() } - tr.Proxy = func(req *http.Request) (*url.URL, error) { - return proxyConfig.ProxyFunc()(req.URL) - } - } else { - proxyConfig := httpproxy.FromEnvironment() - tr.Proxy = func(req *http.Request) (*url.URL, error) { - return proxyConfig.ProxyFunc()(req.URL) - } + } + tr.Proxy = func(req *http.Request) (*url.URL, error) { + return proxyConfig.ProxyFunc()(req.URL) } } From 84fdf5d80c92189daed4404ddd1c6946eef50141 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Thu, 12 Oct 2023 14:40:19 -0700 Subject: [PATCH 08/14] Adds `HTTPSProxy` config parameter --- internal/config/config.go | 21 +++++++++--- internal/test/http_client.go | 64 ++++++++++++++++++++++++++++++++---- 2 files changed, 74 insertions(+), 11 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 0c62a420..317be3d3 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -35,6 +35,7 @@ type Config struct { ForbiddenAccountIds []string HTTPClient *http.Client HTTPProxy string + HTTPSProxy string IamEndpoint string Insecure bool Logger logging.Logger @@ -88,13 +89,20 @@ func (c Config) CustomCABundleReader() (*bytes.Reader, error) { // The returned options function is called on both AWS SDKv1 and v2 default HTTP clients. func (c Config) HTTPTransportOptions() (func(*http.Transport), error) { var err error - var proxyUrl *url.URL + var httpProxyUrl *url.URL if c.HTTPProxy != "" { - proxyUrl, err = url.Parse(c.HTTPProxy) + httpProxyUrl, err = url.Parse(c.HTTPProxy) if err != nil { return nil, fmt.Errorf("error parsing HTTP proxy URL: %w", err) } } + var httpsProxyUrl *url.URL + if c.HTTPSProxy != "" { + httpsProxyUrl, err = url.Parse(c.HTTPSProxy) + if err != nil { + return nil, fmt.Errorf("error parsing HTTPS proxy URL: %w", err) + } + } opts := func(tr *http.Transport) { tr.MaxIdleConnsPerHost = awshttp.DefaultHTTPTransportMaxIdleConnsPerHost @@ -112,12 +120,15 @@ func (c Config) HTTPTransportOptions() (func(*http.Transport), error) { } proxyConfig := httpproxy.FromEnvironment() - if proxyUrl != nil { - proxyConfig.HTTPProxy = proxyUrl.String() + if httpProxyUrl != nil { + proxyConfig.HTTPProxy = httpProxyUrl.String() if proxyConfig.HTTPSProxy == "" { - proxyConfig.HTTPSProxy = proxyUrl.String() + proxyConfig.HTTPSProxy = httpProxyUrl.String() } } + if httpsProxyUrl != nil { + proxyConfig.HTTPSProxy = httpsProxyUrl.String() + } tr.Proxy = func(req *http.Request) (*url.URL, error) { return proxyConfig.ProxyFunc()(req.URL) } diff --git a/internal/test/http_client.go b/internal/test/http_client.go index 7436ed52..345d25e6 100644 --- a/internal/test/http_client.go +++ b/internal/test/http_client.go @@ -86,7 +86,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, - "proxy config": { + "HTTPProxy config": { config: config.Config{ HTTPProxy: "http://http-proxy.test:1234", }, @@ -102,6 +102,39 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, + "HTTPSProxy config": { + config: config.Config{ + HTTPSProxy: "http://https-proxy.test:1234", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectedProxy: "", + }, + { + url: "https://example.com", + expectedProxy: "http://https-proxy.test:1234", + }, + }, + }, + + "HTTPProxy config HTTPSProxy config": { + config: config.Config{ + HTTPProxy: "http://http-proxy.test:1234", + HTTPSProxy: "http://https-proxy.test:1234", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectedProxy: "http://http-proxy.test:1234", + }, + { + url: "https://example.com", + expectedProxy: "http://https-proxy.test:1234", + }, + }, + }, + "HTTP_PROXY envvar": { config: config.Config{}, environmentVariables: map[string]string{ @@ -170,7 +203,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, - "proxy config HTTPS_PROXY envvar": { + "HTTPProxy config HTTPS_PROXY envvar": { config: config.Config{ HTTPProxy: "http://http-proxy.test:1234", }, @@ -189,7 +222,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, - "proxy config https_proxy envvar": { + "HTTPProxy config https_proxy envvar": { config: config.Config{ HTTPProxy: "http://http-proxy.test:1234", }, @@ -208,7 +241,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, - "proxy config NO_PROXY envvar": { + "HTTPProxy config NO_PROXY envvar": { config: config.Config{ HTTPProxy: "http://http-proxy.test:1234", }, @@ -235,7 +268,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, - "proxy config no_proxy envvar": { + "HTTPProxy config no_proxy envvar": { config: config.Config{ HTTPProxy: "http://http-proxy.test:1234", }, @@ -289,7 +322,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, - "proxy config overrides HTTP_PROXY envvar": { + "HTTPProxy config overrides HTTP_PROXY envvar": { config: config.Config{ HTTPProxy: "http://config-proxy.test:1234", }, @@ -307,6 +340,25 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, }, + + "HTTPSProxy config overrides HTTPS_PROXY envvar": { + config: config.Config{ + HTTPSProxy: "http://config-proxy.test:1234", + }, + environmentVariables: map[string]string{ + "HTTPS_PROXY": "http://envvar-proxy.test:1234", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectedProxy: "", + }, + { + url: "https://example.com", + expectedProxy: "http://config-proxy.test:1234", + }, + }, + }, } for name, testcase := range testcases { From 3c7c71345c895fb56b6aaa1f457356004c48d013 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Thu, 12 Oct 2023 14:59:01 -0700 Subject: [PATCH 09/14] Adds `NoProxy` config parameter --- internal/config/config.go | 4 ++++ internal/test/http_client.go | 26 ++++++++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/internal/config/config.go b/internal/config/config.go index 317be3d3..1a3c70c9 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -40,6 +40,7 @@ type Config struct { Insecure bool Logger logging.Logger MaxRetries int + NoProxy string Profile string Region string RetryMode aws.RetryMode @@ -129,6 +130,9 @@ func (c Config) HTTPTransportOptions() (func(*http.Transport), error) { if httpsProxyUrl != nil { proxyConfig.HTTPSProxy = httpsProxyUrl.String() } + if c.NoProxy != "" { + proxyConfig.NoProxy = c.NoProxy + } tr.Proxy = func(req *http.Request) (*url.URL, error) { return proxyConfig.ProxyFunc()(req.URL) } diff --git a/internal/test/http_client.go b/internal/test/http_client.go index 345d25e6..2fa020a7 100644 --- a/internal/test/http_client.go +++ b/internal/test/http_client.go @@ -135,6 +135,32 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, + "HTTPProxy config HTTPSProxy config NoProxy config": { + config: config.Config{ + HTTPProxy: "http://http-proxy.test:1234", + HTTPSProxy: "http://https-proxy.test:1234", + NoProxy: "dont-proxy.test", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectedProxy: "http://http-proxy.test:1234", + }, + { + url: "http://dont-proxy.test", + expectedProxy: "", + }, + { + url: "https://example.com", + expectedProxy: "http://https-proxy.test:1234", + }, + { + url: "https://dont-proxy.test", + expectedProxy: "", + }, + }, + }, + "HTTP_PROXY envvar": { config: config.Config{}, environmentVariables: map[string]string{ From 78da5469508cc3975556e75e0519b78d74924e65 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Fri, 27 Oct 2023 13:51:29 -0700 Subject: [PATCH 10/14] Cleanup --- aws_config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws_config_test.go b/aws_config_test.go index 9b74804d..5df2a1d0 100644 --- a/aws_config_test.go +++ b/aws_config_test.go @@ -1239,7 +1239,7 @@ func testUserAgentProducts(t *testing.T, testCase test.UserAgentTestCase) { } } -var errCancelOperation = fmt.Errorf("Test: Cancelling request") +var errCancelOperation = errors.New("Test: Cancelling request") // cancelRequestMiddleware creates a Smithy middleware that intercepts the request before sending and cancels it func cancelRequestMiddleware(t *testing.T, id string, f func(t *testing.T, request *smithyhttp.Request)) middleware.FinalizeMiddleware { From 075685e1584de35a311764ebcf9d3738281e449c Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Fri, 27 Oct 2023 17:49:53 -0700 Subject: [PATCH 11/14] Adds `HTTPProxyMode` to specify `HTTPProxy` behavior when `HTTPSProxy` not set --- config.go | 7 +++ internal/config/config.go | 14 ++++- internal/test/http_client.go | 113 ++++++++++++++++++++++++++++++++--- 3 files changed, 123 insertions(+), 11 deletions(-) diff --git a/config.go b/config.go index f46d7915..0ec6f36e 100644 --- a/config.go +++ b/config.go @@ -33,3 +33,10 @@ func EC2MetadataEndpointMode_Values() []string { EC2MetadataEndpointModeIPv6, } } + +// type ProxyMode int + +const ( + HTTPProxyModeLegacy = config.HTTPProxyModeLegacy + HTTPProxyModeSeparate = config.HTTPProxyModeSeparate +) diff --git a/internal/config/config.go b/internal/config/config.go index 1a3c70c9..f4d3329a 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -20,6 +20,13 @@ import ( "golang.org/x/net/http/httpproxy" ) +type ProxyMode int + +const ( + HTTPProxyModeLegacy ProxyMode = iota + HTTPProxyModeSeparate +) + type Config struct { AccessKey string AllowedAccountIds []string @@ -42,6 +49,7 @@ type Config struct { MaxRetries int NoProxy string Profile string + HTTPProxyMode ProxyMode Region string RetryMode aws.RetryMode SecretKey string @@ -94,14 +102,14 @@ func (c Config) HTTPTransportOptions() (func(*http.Transport), error) { if c.HTTPProxy != "" { httpProxyUrl, err = url.Parse(c.HTTPProxy) if err != nil { - return nil, fmt.Errorf("error parsing HTTP proxy URL: %w", err) + return nil, fmt.Errorf("parsing HTTP proxy URL: %w", err) } } var httpsProxyUrl *url.URL if c.HTTPSProxy != "" { httpsProxyUrl, err = url.Parse(c.HTTPSProxy) if err != nil { - return nil, fmt.Errorf("error parsing HTTPS proxy URL: %w", err) + return nil, fmt.Errorf("parsing HTTPS proxy URL: %w", err) } } @@ -123,7 +131,7 @@ func (c Config) HTTPTransportOptions() (func(*http.Transport), error) { proxyConfig := httpproxy.FromEnvironment() if httpProxyUrl != nil { proxyConfig.HTTPProxy = httpProxyUrl.String() - if proxyConfig.HTTPSProxy == "" { + if c.HTTPProxyMode == HTTPProxyModeLegacy && proxyConfig.HTTPSProxy == "" { proxyConfig.HTTPSProxy = httpProxyUrl.String() } } diff --git a/internal/test/http_client.go b/internal/test/http_client.go index 2fa020a7..f915182d 100644 --- a/internal/test/http_client.go +++ b/internal/test/http_client.go @@ -86,9 +86,10 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, - "HTTPProxy config": { + "HTTPProxy config Legacy": { config: config.Config{ - HTTPProxy: "http://http-proxy.test:1234", + HTTPProxy: "http://http-proxy.test:1234", + HTTPProxyMode: config.HTTPProxyModeLegacy, }, urls: []proxyCase{ { @@ -102,6 +103,23 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, + "HTTPProxy config Separate": { + config: config.Config{ + HTTPProxy: "http://http-proxy.test:1234", + HTTPProxyMode: config.HTTPProxyModeSeparate, + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectedProxy: "http://http-proxy.test:1234", + }, + { + url: "https://example.com", + expectedProxy: "", + }, + }, + }, + "HTTPSProxy config": { config: config.Config{ HTTPSProxy: "http://https-proxy.test:1234", @@ -267,9 +285,10 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, - "HTTPProxy config NO_PROXY envvar": { + "HTTPProxy config NO_PROXY envvar Legacy": { config: config.Config{ - HTTPProxy: "http://http-proxy.test:1234", + HTTPProxy: "http://http-proxy.test:1234", + HTTPProxyMode: config.HTTPProxyModeLegacy, }, environmentVariables: map[string]string{ "NO_PROXY": "dont-proxy.test", @@ -294,9 +313,38 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, - "HTTPProxy config no_proxy envvar": { + "HTTPProxy config NO_PROXY envvar Separate": { config: config.Config{ - HTTPProxy: "http://http-proxy.test:1234", + HTTPProxy: "http://http-proxy.test:1234", + HTTPProxyMode: config.HTTPProxyModeSeparate, + }, + environmentVariables: map[string]string{ + "NO_PROXY": "dont-proxy.test", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectedProxy: "http://http-proxy.test:1234", + }, + { + url: "http://dont-proxy.test", + expectedProxy: "", + }, + { + url: "https://example.com", + expectedProxy: "", + }, + { + url: "https://dont-proxy.test", + expectedProxy: "", + }, + }, + }, + + "HTTPProxy config no_proxy envvar Legacy": { + config: config.Config{ + HTTPProxy: "http://http-proxy.test:1234", + HTTPProxyMode: config.HTTPProxyModeLegacy, }, environmentVariables: map[string]string{ "no_proxy": "dont-proxy.test", @@ -321,6 +369,34 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, + "HTTPProxy config no_proxy envvar Separate": { + config: config.Config{ + HTTPProxy: "http://http-proxy.test:1234", + HTTPProxyMode: config.HTTPProxyModeSeparate, + }, + environmentVariables: map[string]string{ + "no_proxy": "dont-proxy.test", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectedProxy: "http://http-proxy.test:1234", + }, + { + url: "http://dont-proxy.test", + expectedProxy: "", + }, + { + url: "https://example.com", + expectedProxy: "", + }, + { + url: "https://dont-proxy.test", + expectedProxy: "", + }, + }, + }, + "HTTP_PROXY envvar HTTPS_PROXY envvar NO_PROXY envvar": { config: config.Config{}, environmentVariables: map[string]string{ @@ -348,9 +424,10 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, - "HTTPProxy config overrides HTTP_PROXY envvar": { + "HTTPProxy config overrides HTTP_PROXY envvar Legacy": { config: config.Config{ - HTTPProxy: "http://config-proxy.test:1234", + HTTPProxy: "http://config-proxy.test:1234", + HTTPProxyMode: config.HTTPProxyModeLegacy, }, environmentVariables: map[string]string{ "HTTP_PROXY": "http://envvar-proxy.test:1234", @@ -367,6 +444,26 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, + "HTTPProxy config overrides HTTP_PROXY envvar Separate": { + config: config.Config{ + HTTPProxy: "http://config-proxy.test:1234", + HTTPProxyMode: config.HTTPProxyModeSeparate, + }, + environmentVariables: map[string]string{ + "HTTP_PROXY": "http://envvar-proxy.test:1234", + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectedProxy: "http://config-proxy.test:1234", + }, + { + url: "https://example.com", + expectedProxy: "", + }, + }, + }, + "HTTPSProxy config overrides HTTPS_PROXY envvar": { config: config.Config{ HTTPSProxy: "http://config-proxy.test:1234", From b3543e0e9b19dfbdeb33fa72504ce449abcf2775 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Tue, 31 Oct 2023 15:35:15 -0700 Subject: [PATCH 12/14] Allow `HTTPProxy` and `HTTPSProxy` to accept empty string --- aws_config.go | 5 ++ internal/config/config.go | 53 ++++++++++-- internal/config/config_test.go | 144 +++++++++++++++++++++++++++++++++ internal/test/http_client.go | 84 +++++++++++++++---- 4 files changed, 264 insertions(+), 22 deletions(-) diff --git a/aws_config.go b/aws_config.go index d3de208b..0673e806 100644 --- a/aws_config.go +++ b/aws_config.go @@ -75,6 +75,11 @@ func GetAwsConfig(ctx context.Context, c *Config) (context.Context, aws.Config, } } + c.ValidateProxySettings(&diags) + if diags.HasError() { + return ctx, aws.Config{}, diags + } + logger.Debug(baseCtx, "Resolving credentials provider") credentialsProvider, initialSource, d := getCredentialsProvider(baseCtx, c) if d.HasError() { diff --git a/internal/config/config.go b/internal/config/config.go index f4d3329a..ff3deba3 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -15,6 +15,7 @@ import ( "github.com/aws/aws-sdk-go-v2/aws" awshttp "github.com/aws/aws-sdk-go-v2/aws/transport/http" "github.com/aws/aws-sdk-go-v2/feature/ec2/imds" + "github.com/hashicorp/aws-sdk-go-base/v2/diag" "github.com/hashicorp/aws-sdk-go-base/v2/internal/expand" "github.com/hashicorp/aws-sdk-go-base/v2/logging" "golang.org/x/net/http/httpproxy" @@ -41,8 +42,8 @@ type Config struct { EC2MetadataServiceEndpointMode string ForbiddenAccountIds []string HTTPClient *http.Client - HTTPProxy string - HTTPSProxy string + HTTPProxy *string + HTTPSProxy *string IamEndpoint string Insecure bool Logger logging.Logger @@ -99,15 +100,15 @@ func (c Config) CustomCABundleReader() (*bytes.Reader, error) { func (c Config) HTTPTransportOptions() (func(*http.Transport), error) { var err error var httpProxyUrl *url.URL - if c.HTTPProxy != "" { - httpProxyUrl, err = url.Parse(c.HTTPProxy) + if c.HTTPProxy != nil { + httpProxyUrl, err = url.Parse(aws.ToString(c.HTTPProxy)) if err != nil { return nil, fmt.Errorf("parsing HTTP proxy URL: %w", err) } } var httpsProxyUrl *url.URL - if c.HTTPSProxy != "" { - httpsProxyUrl, err = url.Parse(c.HTTPSProxy) + if c.HTTPSProxy != nil { + httpsProxyUrl, err = url.Parse(aws.ToString(c.HTTPSProxy)) if err != nil { return nil, fmt.Errorf("parsing HTTPS proxy URL: %w", err) } @@ -149,6 +150,46 @@ func (c Config) HTTPTransportOptions() (func(*http.Transport), error) { return opts, nil } +func (c Config) ValidateProxySettings(diags *diag.Diagnostics) { + if c.HTTPProxy != nil { + if _, err := url.Parse(aws.ToString(c.HTTPProxy)); err != nil { + *diags = diags.AddError( + "Invalid HTTP Proxy", + fmt.Sprintf("Unable to parse URL: %s", err), + ) + } + } + + if c.HTTPSProxy != nil { + if _, err := url.Parse(aws.ToString(c.HTTPSProxy)); err != nil { + *diags = diags.AddError( + "Invalid HTTPS Proxy", + fmt.Sprintf("Unable to parse URL: %s", err), + ) + } + } + + if c.HTTPProxy != nil && c.HTTPSProxy == nil && os.Getenv("HTTPS_PROXY") == "" && os.Getenv("https_proxy") == "" { + if c.HTTPProxyMode == HTTPProxyModeLegacy { + *diags = diags.AddWarning( + "Missing HTTPS Proxy", + fmt.Sprintf( + "An HTTP proxy was set but no HTTPS proxy was. Using HTTP proxy %q for HTTPS requests. This behavior may change in future versions.\n\n"+ + "To specify no proxy for HTTPS, set the HTTPS to an empty string", + aws.ToString(c.HTTPProxy), + ), + ) + } else { + *diags = diags.AddWarning( + "Missing HTTPS Proxy", + "An HTTP proxy was set but no HTTPS proxy was.\n\n"+ + "To specify no proxy for HTTPS, set the HTTPS to an empty string", + ) + } + } + +} + func (c Config) ResolveSharedConfigFiles() ([]string, error) { v, err := expand.FilePaths(c.SharedConfigFiles) if err != nil { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index f18ceb23..e7e126f5 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -4,7 +4,14 @@ package config import ( + "fmt" + "net/url" "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/google/go-cmp/cmp" + "github.com/hashicorp/aws-sdk-go-base/v2/diag" + "github.com/hashicorp/aws-sdk-go-base/v2/servicemocks" ) func TestConfig_VerifyAccountIDAllowed(t *testing.T) { @@ -72,3 +79,140 @@ func TestConfig_VerifyAccountIDAllowed(t *testing.T) { }) } } + +func foo(_ *url.URL, err error) error { + return err +} + +func TestValidateProxyConfig(t *testing.T) { + testcases := map[string]struct { + config Config + environmentVariables map[string]string + expectedDiags diag.Diagnostics + }{ + "no config": {}, + + "invalid HTTP proxy": { + config: Config{ + HTTPProxy: aws.String(" http://invalid.test"), // explicit URL parse failure + HTTPSProxy: aws.String("http://valid.test"), + }, + expectedDiags: diag.Diagnostics{ + diag.NewErrorDiagnostic( + "Invalid HTTP Proxy", + fmt.Sprintf("Unable to parse URL: %s", foo(url.Parse(" http://invalid.test"))), //nolint:staticcheck + ), + }, + }, + + "invalid HTTPS proxy": { + config: Config{ + HTTPProxy: aws.String("http://valid.test"), + HTTPSProxy: aws.String(" http://invalid.test"), // explicit URL parse failure + }, + expectedDiags: diag.Diagnostics{ + diag.NewErrorDiagnostic( + "Invalid HTTPS Proxy", + fmt.Sprintf("Unable to parse URL: %s", foo(url.Parse(" http://invalid.test"))), //nolint:staticcheck + ), + }, + }, + + "invalid both proxies": { + config: Config{ + HTTPProxy: aws.String(" http://invalid.test"), // explicit URL parse failure + HTTPSProxy: aws.String(" http://invalid.test"), // explicit URL parse failure + }, + expectedDiags: diag.Diagnostics{ + diag.NewErrorDiagnostic( + "Invalid HTTP Proxy", + fmt.Sprintf("Unable to parse URL: %s", foo(url.Parse(" http://invalid.test"))), //nolint:staticcheck + ), + diag.NewErrorDiagnostic( + "Invalid HTTPS Proxy", + fmt.Sprintf("Unable to parse URL: %s", foo(url.Parse(" http://invalid.test"))), //nolint:staticcheck + ), + }, + }, + + "HTTP proxy without HTTPS proxy Legacy": { + config: Config{ + HTTPProxy: aws.String("http://valid.test"), + HTTPProxyMode: HTTPProxyModeLegacy, + }, + expectedDiags: diag.Diagnostics{ + diag.NewWarningDiagnostic( + "Missing HTTPS Proxy", + fmt.Sprintf( + "An HTTP proxy was set but no HTTPS proxy was. Using HTTP proxy %q for HTTPS requests. This behavior may change in future versions.\n\n"+ + "To specify no proxy for HTTPS, set the HTTPS to an empty string", + "http://valid.test"), + ), + }, + }, + + "HTTP proxy with empty string HTTPS proxy Legacy": { + config: Config{ + HTTPProxy: aws.String("http://valid.test"), + HTTPSProxy: aws.String(""), + HTTPProxyMode: HTTPProxyModeLegacy, + }, + expectedDiags: diag.Diagnostics{}, + }, + + "HTTP proxy config with HTTPS_PROXY envvar": { + config: Config{ + HTTPProxy: aws.String("http://valid.test"), + }, + environmentVariables: map[string]string{ + "HTTPS_PROXY": "http://envvar-proxy.test:1234", + }, + expectedDiags: diag.Diagnostics{}, + }, + + "HTTP proxy config with https_proxy envvar": { + config: Config{ + HTTPProxy: aws.String("http://valid.test"), + }, + environmentVariables: map[string]string{ + "https_proxy": "http://envvar-proxy.test:1234", + }, + expectedDiags: diag.Diagnostics{}, + }, + + "HTTP proxy without HTTPS proxy Separate": { + config: Config{ + HTTPProxy: aws.String("http://valid.test"), + HTTPProxyMode: HTTPProxyModeSeparate, + }, + expectedDiags: diag.Diagnostics{ + diag.NewWarningDiagnostic( + "Missing HTTPS Proxy", + "An HTTP proxy was set but no HTTPS proxy was.\n\n"+ + "To specify no proxy for HTTPS, set the HTTPS to an empty string", + ), + }, + }, + } + + for name, testcase := range testcases { + testcase := testcase + + t.Run(name, func(t *testing.T) { + oldEnv := servicemocks.InitSessionTestEnv() + defer servicemocks.PopEnv(oldEnv) + + for k, v := range testcase.environmentVariables { + t.Setenv(k, v) + } + + var diags diag.Diagnostics + + testcase.config.ValidateProxySettings(&diags) + + if diff := cmp.Diff(diags, testcase.expectedDiags); diff != "" { + t.Errorf("Unexpected response (+wanted, -got): %s", diff) + } + }) + } +} diff --git a/internal/test/http_client.go b/internal/test/http_client.go index f915182d..d0d7caab 100644 --- a/internal/test/http_client.go +++ b/internal/test/http_client.go @@ -9,6 +9,7 @@ import ( "testing" awshttp "github.com/aws/aws-sdk-go-v2/aws/transport/http" + "github.com/aws/aws-sdk-go/aws" "github.com/hashicorp/aws-sdk-go-base/v2/internal/config" "github.com/hashicorp/aws-sdk-go-base/v2/servicemocks" ) @@ -86,9 +87,25 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, + "HTTPProxy config empty string": { + config: config.Config{ + HTTPProxy: aws.String(""), + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectedProxy: "", + }, + { + url: "https://example.com", + expectedProxy: "", + }, + }, + }, + "HTTPProxy config Legacy": { config: config.Config{ - HTTPProxy: "http://http-proxy.test:1234", + HTTPProxy: aws.String("http://http-proxy.test:1234"), HTTPProxyMode: config.HTTPProxyModeLegacy, }, urls: []proxyCase{ @@ -105,7 +122,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { "HTTPProxy config Separate": { config: config.Config{ - HTTPProxy: "http://http-proxy.test:1234", + HTTPProxy: aws.String("http://http-proxy.test:1234"), HTTPProxyMode: config.HTTPProxyModeSeparate, }, urls: []proxyCase{ @@ -122,7 +139,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { "HTTPSProxy config": { config: config.Config{ - HTTPSProxy: "http://https-proxy.test:1234", + HTTPSProxy: aws.String("http://https-proxy.test:1234"), }, urls: []proxyCase{ { @@ -138,8 +155,8 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { "HTTPProxy config HTTPSProxy config": { config: config.Config{ - HTTPProxy: "http://http-proxy.test:1234", - HTTPSProxy: "http://https-proxy.test:1234", + HTTPProxy: aws.String("http://http-proxy.test:1234"), + HTTPSProxy: aws.String("http://https-proxy.test:1234"), }, urls: []proxyCase{ { @@ -153,10 +170,45 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { }, }, + "HTTPProxy config HTTPSProxy config empty string Legacy": { + config: config.Config{ + HTTPProxy: aws.String("http://http-proxy.test:1234"), + HTTPSProxy: aws.String(""), + HTTPProxyMode: config.HTTPProxyModeLegacy, + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectedProxy: "http://http-proxy.test:1234", + }, + { + url: "https://example.com", + expectedProxy: "", + }, + }, + }, + + "HTTPSProxy config HTTPProxy config empty string": { + config: config.Config{ + HTTPProxy: aws.String(""), + HTTPSProxy: aws.String("http://https-proxy.test:1234"), + }, + urls: []proxyCase{ + { + url: "http://example.com", + expectedProxy: "", + }, + { + url: "https://example.com", + expectedProxy: "http://https-proxy.test:1234", + }, + }, + }, + "HTTPProxy config HTTPSProxy config NoProxy config": { config: config.Config{ - HTTPProxy: "http://http-proxy.test:1234", - HTTPSProxy: "http://https-proxy.test:1234", + HTTPProxy: aws.String("http://http-proxy.test:1234"), + HTTPSProxy: aws.String("http://https-proxy.test:1234"), NoProxy: "dont-proxy.test", }, urls: []proxyCase{ @@ -249,7 +301,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { "HTTPProxy config HTTPS_PROXY envvar": { config: config.Config{ - HTTPProxy: "http://http-proxy.test:1234", + HTTPProxy: aws.String("http://http-proxy.test:1234"), }, environmentVariables: map[string]string{ "HTTPS_PROXY": "http://https-proxy.test:1234", @@ -268,7 +320,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { "HTTPProxy config https_proxy envvar": { config: config.Config{ - HTTPProxy: "http://http-proxy.test:1234", + HTTPProxy: aws.String("http://http-proxy.test:1234"), }, environmentVariables: map[string]string{ "https_proxy": "http://https-proxy.test:1234", @@ -287,7 +339,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { "HTTPProxy config NO_PROXY envvar Legacy": { config: config.Config{ - HTTPProxy: "http://http-proxy.test:1234", + HTTPProxy: aws.String("http://http-proxy.test:1234"), HTTPProxyMode: config.HTTPProxyModeLegacy, }, environmentVariables: map[string]string{ @@ -315,7 +367,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { "HTTPProxy config NO_PROXY envvar Separate": { config: config.Config{ - HTTPProxy: "http://http-proxy.test:1234", + HTTPProxy: aws.String("http://http-proxy.test:1234"), HTTPProxyMode: config.HTTPProxyModeSeparate, }, environmentVariables: map[string]string{ @@ -343,7 +395,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { "HTTPProxy config no_proxy envvar Legacy": { config: config.Config{ - HTTPProxy: "http://http-proxy.test:1234", + HTTPProxy: aws.String("http://http-proxy.test:1234"), HTTPProxyMode: config.HTTPProxyModeLegacy, }, environmentVariables: map[string]string{ @@ -371,7 +423,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { "HTTPProxy config no_proxy envvar Separate": { config: config.Config{ - HTTPProxy: "http://http-proxy.test:1234", + HTTPProxy: aws.String("http://http-proxy.test:1234"), HTTPProxyMode: config.HTTPProxyModeSeparate, }, environmentVariables: map[string]string{ @@ -426,7 +478,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { "HTTPProxy config overrides HTTP_PROXY envvar Legacy": { config: config.Config{ - HTTPProxy: "http://config-proxy.test:1234", + HTTPProxy: aws.String("http://config-proxy.test:1234"), HTTPProxyMode: config.HTTPProxyModeLegacy, }, environmentVariables: map[string]string{ @@ -446,7 +498,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { "HTTPProxy config overrides HTTP_PROXY envvar Separate": { config: config.Config{ - HTTPProxy: "http://config-proxy.test:1234", + HTTPProxy: aws.String("http://config-proxy.test:1234"), HTTPProxyMode: config.HTTPProxyModeSeparate, }, environmentVariables: map[string]string{ @@ -466,7 +518,7 @@ func HTTPClientConfigurationTest_proxy(t *testing.T, getter TransportGetter) { "HTTPSProxy config overrides HTTPS_PROXY envvar": { config: config.Config{ - HTTPSProxy: "http://config-proxy.test:1234", + HTTPSProxy: aws.String("http://config-proxy.test:1234"), }, environmentVariables: map[string]string{ "HTTPS_PROXY": "http://envvar-proxy.test:1234", From 53aabca0808688558f79f029057d2ae0d79d1da8 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Fri, 3 Nov 2023 10:36:41 -0700 Subject: [PATCH 13/14] Support empty string in `HTTPProxy` --- internal/config/config.go | 2 +- internal/config/config_test.go | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index ff3deba3..c6550e8c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -169,7 +169,7 @@ func (c Config) ValidateProxySettings(diags *diag.Diagnostics) { } } - if c.HTTPProxy != nil && c.HTTPSProxy == nil && os.Getenv("HTTPS_PROXY") == "" && os.Getenv("https_proxy") == "" { + if c.HTTPProxy != nil && *c.HTTPProxy != "" && c.HTTPSProxy == nil && os.Getenv("HTTPS_PROXY") == "" && os.Getenv("https_proxy") == "" { if c.HTTPProxyMode == HTTPProxyModeLegacy { *diags = diags.AddWarning( "Missing HTTPS Proxy", diff --git a/internal/config/config_test.go b/internal/config/config_test.go index e7e126f5..14a9fcd0 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -151,7 +151,14 @@ func TestValidateProxyConfig(t *testing.T) { }, }, - "HTTP proxy with empty string HTTPS proxy Legacy": { + "HTTP proxy empty string": { + config: Config{ + HTTPProxy: aws.String(""), + }, + expectedDiags: diag.Diagnostics{}, + }, + + "HTTP proxy with HTTPS proxy empty string Legacy": { config: Config{ HTTPProxy: aws.String("http://valid.test"), HTTPSProxy: aws.String(""), From f788913f1e51dfba514b0208a7e687165196f430 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Tue, 14 Nov 2023 10:36:09 -0800 Subject: [PATCH 14/14] Cleanup Co-authored-by: Jared Baker --- config.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/config.go b/config.go index 0ec6f36e..ffbf25f2 100644 --- a/config.go +++ b/config.go @@ -34,8 +34,6 @@ func EC2MetadataEndpointMode_Values() []string { } } -// type ProxyMode int - const ( HTTPProxyModeLegacy = config.HTTPProxyModeLegacy HTTPProxyModeSeparate = config.HTTPProxyModeSeparate