From d7afd06f55b15f6776b536af64cfce612f6f4fcb Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 7 Jul 2021 12:50:13 -0700 Subject: [PATCH] Add `https_proxy` and `no_proxy` settings for the Supervisor - Add new optional ytt params for the Supervisor deployment. - When the Supervisor is making calls to an upstream OIDC provider, use these variables if they were provided. - These settings are integration tested in the main CI pipeline by sometimes setting them on deployments in certain cases, and then letting the existing integration tests (e.g. TestE2EFullIntegration) provide the coverage, so there are no explicit changes to the integration tests themselves in this commit. --- deploy/supervisor/deployment.yaml | 11 ++++++++++- deploy/supervisor/values.yaml | 10 +++++++++- .../upstreamwatcher/upstreamwatcher.go | 8 ++++++-- .../upstreamwatcher/upstreamwatcher_test.go | 10 ++++++++++ 4 files changed, 35 insertions(+), 4 deletions(-) diff --git a/deploy/supervisor/deployment.yaml b/deploy/supervisor/deployment.yaml index 1e0c75c0a..effba2fb7 100644 --- a/deploy/supervisor/deployment.yaml +++ b/deploy/supervisor/deployment.yaml @@ -1,4 +1,4 @@ -#! Copyright 2020 the Pinniped contributors. All Rights Reserved. +#! Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. #! SPDX-License-Identifier: Apache-2.0 #@ load("@ytt:data", "data") @@ -101,6 +101,15 @@ spec: protocol: TCP - containerPort: 8443 protocol: TCP + env: + #@ if data.values.https_proxy: + - name: HTTPS_PROXY + value: #@ data.values.https_proxy + #@ end + #@ if data.values.no_proxy: + - name: NO_PROXY + value: #@ data.values.no_proxy + #@ end livenessProbe: httpGet: path: /healthz diff --git a/deploy/supervisor/values.yaml b/deploy/supervisor/values.yaml index a4575c3c6..62b632f7d 100644 --- a/deploy/supervisor/values.yaml +++ b/deploy/supervisor/values.yaml @@ -1,4 +1,4 @@ -#! Copyright 2020 the Pinniped contributors. All Rights Reserved. +#! Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. #! SPDX-License-Identifier: Apache-2.0 #@data/values @@ -56,3 +56,11 @@ service_loadbalancer_ip: #! e.g. 1.2.3.4 #! Specify the verbosity of logging: info ("nice to know" information), debug (developer #! information), trace (timing information), all (kitchen sink). log_level: #! By default, when this value is left unset, only warnings and errors are printed. There is no way to suppress warning and error logs. + +#! Set the standard golang HTTPS_PROXY and NO_PROXY environment variables on the Supervisor containers. +#! These will be used when the Supervisor makes backend-to-backend calls to upstream identity providers using HTTPS, +#! e.g. when the Supervisor fetches discovery documents, JWKS keys, and tokens from an upstream OIDC Provider. +#! The Supervisor never makes insecure HTTP calls, so there is no reason to set HTTP_PROXY. +#! Optional. +https_proxy: #! e.g. http://proxy.example.com +no_proxy: #! e.g. 127.0.0.1 diff --git a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go index b6b553283..edf9ba387 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go @@ -265,9 +265,13 @@ func (c *controller) validateIssuer(ctx context.Context, upstream *v1alpha1.OIDC Message: err.Error(), } } - httpClient = &http.Client{Transport: &http.Transport{TLSClientConfig: tlsConfig}} - discoveredProvider, err = oidc.NewProvider(oidc.ClientContext(ctx, httpClient), upstream.Spec.Issuer) + httpClient = &http.Client{Transport: &http.Transport{Proxy: http.ProxyFromEnvironment, TLSClientConfig: tlsConfig}} + + timeoutCtx, cancelFunc := context.WithTimeout(oidc.ClientContext(ctx, httpClient), time.Minute) + defer cancelFunc() + + discoveredProvider, err = oidc.NewProvider(timeoutCtx, upstream.Spec.Issuer) if err != nil { return &v1alpha1.Condition{ Type: typeOIDCDiscoverySucceeded, diff --git a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go index 02e96e8ee..4b97a07be 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go @@ -9,6 +9,7 @@ import ( "encoding/json" "net/http" "net/url" + "reflect" "strings" "testing" "time" @@ -650,6 +651,15 @@ func TestController(t *testing.T) { require.Equal(t, tt.wantResultingCache[i].GetUsernameClaim(), actualIDP.GetUsernameClaim()) require.Equal(t, tt.wantResultingCache[i].GetGroupsClaim(), actualIDP.GetGroupsClaim()) require.ElementsMatch(t, tt.wantResultingCache[i].GetScopes(), actualIDP.GetScopes()) + + // We always want to use the proxy from env on these clients, so although the following assertions + // are a little hacky, this is a cheap way to test that we are using it. + actualTransport, ok := actualIDP.Client.Transport.(*http.Transport) + require.True(t, ok, "expected cached provider to have client with Transport of type *http.Transport") + httpProxyFromEnvFunction := reflect.ValueOf(http.ProxyFromEnvironment).Pointer() + actualTransportProxyFunction := reflect.ValueOf(actualTransport.Proxy).Pointer() + require.Equal(t, httpProxyFromEnvFunction, actualTransportProxyFunction, + "Transport should have used http.ProxyFromEnvironment as its Proxy func") } actualUpstreams, err := fakePinnipedClient.IDPV1alpha1().OIDCIdentityProviders(testNamespace).List(ctx, metav1.ListOptions{})