diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go index 15a1bed38..9d4ff80fa 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go @@ -510,7 +510,7 @@ func (c *webhookCacheFillerController) validateEndpoint(endpoint string, conditi return nil, conditions, false, false } - usingProxyForHost, err := c.proxyDetector.UsingProxyForHost(endpointHostPort.Host) + usingProxyForHost, err := c.proxyDetector.UsingProxyForHost(endpointHostPort.Endpoint()) if err != nil { msg := fmt.Sprintf("%s: %s", "spec.endpoint URL error", err.Error()) conditions = append(conditions, &metav1.Condition{ diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go index 4b0b5fa0c..e3735c19d 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go @@ -1068,7 +1068,10 @@ func TestController(t *testing.T) { fakeProxyDetect := fakeproxydetect.New(true, nil) t.Cleanup(func() { require.Equal(t, 1, fakeProxyDetect.NumberOfInvocations()) - require.Equal(t, "127.0.0.1", fakeProxyDetect.ReceivedHostDuringMostRecentInvocation()) + require.Equal(t, + strings.TrimPrefix(goodWebhookAuthenticatorSpecWithCA.Endpoint, "https://"), + fakeProxyDetect.ReceivedHostDuringMostRecentInvocation(), + ) }) return fakeProxyDetect }, @@ -1117,7 +1120,10 @@ func TestController(t *testing.T) { fakeProxyDetect := fakeproxydetect.New(false, nil) t.Cleanup(func() { require.Equal(t, 1, fakeProxyDetect.NumberOfInvocations()) - require.Equal(t, "127.0.0.1", fakeProxyDetect.ReceivedHostDuringMostRecentInvocation()) + require.Equal(t, + strings.TrimPrefix(goodWebhookAuthenticatorSpecWithCA.Endpoint, "https://"), + fakeProxyDetect.ReceivedHostDuringMostRecentInvocation(), + ) }) return fakeProxyDetect }, @@ -1160,7 +1166,10 @@ func TestController(t *testing.T) { fakeProxyDetect := fakeproxydetect.New(true, nil) t.Cleanup(func() { require.Equal(t, 1, fakeProxyDetect.NumberOfInvocations()) - require.Equal(t, "127.0.0.1", fakeProxyDetect.ReceivedHostDuringMostRecentInvocation()) + require.Equal(t, + strings.TrimPrefix(goodWebhookAuthenticatorSpecWithCA.Endpoint, "https://"), + fakeProxyDetect.ReceivedHostDuringMostRecentInvocation(), + ) }) return fakeProxyDetect }, @@ -1208,7 +1217,10 @@ func TestController(t *testing.T) { fakeProxyDetect := fakeproxydetect.New(false, errors.New("fake proxy detector error")) t.Cleanup(func() { require.Equal(t, 1, fakeProxyDetect.NumberOfInvocations()) - require.Equal(t, "127.0.0.1", fakeProxyDetect.ReceivedHostDuringMostRecentInvocation()) + require.Equal(t, + strings.TrimPrefix(goodWebhookAuthenticatorSpecWithCA.Endpoint, "https://"), + fakeProxyDetect.ReceivedHostDuringMostRecentInvocation(), + ) }) return fakeProxyDetect }, diff --git a/internal/proxydetect/proxydetect.go b/internal/proxydetect/proxydetect.go index eaa2e99fe..b7b8c892c 100644 --- a/internal/proxydetect/proxydetect.go +++ b/internal/proxydetect/proxydetect.go @@ -8,6 +8,8 @@ import ( "fmt" "net/http" "net/url" + + "k8s.io/apimachinery/pkg/util/net" ) type ProxyDetect interface { @@ -26,7 +28,13 @@ type detector struct { var _ ProxyDetect = (*detector)(nil) func New() ProxyDetect { - return &detector{proxyFromEnvironmentFunc: http.ProxyFromEnvironment} + return &detector{ + // Because this is intended for use with the WebhookAuthenticator, let's + // use the same proxy function that we use for the WebhookAuthenticator. + // Refer to how webhookcachefiller.go constructs the webhook authenticator, + // and you'll find how it sets the proxy function in kubeclient.go to be this... + proxyFromEnvironmentFunc: net.NewProxierWithNoProxyCIDR(http.ProxyFromEnvironment), + } } func (d *detector) UsingProxyForHost(host string) (bool, error) { diff --git a/internal/proxydetect/proxydetect_test.go b/internal/proxydetect/proxydetect_test.go index 160b10da5..950b5c6f1 100644 --- a/internal/proxydetect/proxydetect_test.go +++ b/internal/proxydetect/proxydetect_test.go @@ -12,9 +12,100 @@ import ( "github.com/stretchr/testify/require" ) -func TestProxyDetect(t *testing.T) { - t.Parallel() +func TestProxyDetectWithoutMock(t *testing.T) { + // Setting these real env vars means that we cannot run this test in parallel. + t.Setenv("HTTPS_PROXY", "http://proxy.pinniped.dev") + t.Setenv("NO_PROXY", "1.2.3.4,1.2.3.5:3333,4.4.4.0/28,example1.pinniped.dev:8443,example2.pinniped.dev") + subject := New() + + tests := []struct { + name string + host string + wantProxied bool + wantErr string + }{ + // This does not test all permutations of how HTTPS_PROXY and NO_PROXY work. + // Some basic tests to understand how these settings work are included below. + // See https://pkg.go.dev/golang.org/x/net/http/httpproxy for docs. + { + name: "any host not included in NO_PROXY should use the proxy", + host: "www.pinniped.dev", + wantProxied: true, + }, + { + name: "a port different from the one specified in NO_PROXY should use the proxy, for the default HTTPS port", + host: "example1.pinniped.dev", + wantProxied: true, + }, + { + name: "a port different from the one specified in NO_PROXY should use the proxy, for an explicit port", + host: "example1.pinniped.dev:994", + wantProxied: true, + }, + { + name: "same port as the one specified in NO_PROXY should skip the proxy", + host: "example1.pinniped.dev:8443", + wantProxied: false, + }, + { + name: "any host included in NO_PROXY should skip the proxy, with default ports", + host: "example2.pinniped.dev", + wantProxied: false, + }, + { + name: "an IP specified in NO_PROXY should skip the proxy", + host: "1.2.3.4", + wantProxied: false, + }, + { + name: "an IP specified in NO_PROXY should skip the proxy, with matching explicit ports", + host: "1.2.3.5:3333", + wantProxied: false, + }, + { + name: "an IP specified in NO_PROXY should use the proxy when the ports don't match", + host: "1.2.3.5:1234", + wantProxied: true, + }, + { + name: "an IP included in a NO_PROXY CIDR should skip the proxy", + host: "4.4.4.4", + wantProxied: false, + }, + { + name: "an IP outside a NO_PROXY CIDR should use the proxy", + host: "4.4.4.16", + wantProxied: true, + }, + { + name: "as a special case in the Go documentation, localhost never uses the proxy, regardless of NO_PROXY settings", + host: "localhost", + wantProxied: false, + }, + { + name: "a bad hostname returns an error", + host: "bad hostname", + wantProxied: false, + wantErr: `could not determine if requests will be proxied for host "bad hostname": parse "https://bad hostname": invalid character " " in host name`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + usingProxyForHost, err := subject.UsingProxyForHost(tt.host) + if tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + } else { + require.NoError(t, err) + } + require.Equal(t, tt.wantProxied, usingProxyForHost) + }) + } +} + +func TestProxyDetectWithMock(t *testing.T) { proxyURL, err := url.Parse("http://myproxy.com") require.NoError(t, err) @@ -66,12 +157,12 @@ func TestProxyDetect(t *testing.T) { proxied, err := subject.UsingProxyForHost(tt.host) - require.Equal(t, tt.wantProxied, proxied) if tt.wantErr != "" { - require.Equal(t, tt.wantErr, err.Error()) + require.EqualError(t, err, tt.wantErr) } else { require.NoError(t, err) } + require.Equal(t, tt.wantProxied, proxied) }) } }