From f1e63c55d4859b5f7cc53ac8161cb5aede284094 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 | 9 +++++++++ deploy/supervisor/values.yaml | 8 ++++++++ .../oidcupstreamwatcher/oidc_upstream_watcher.go | 8 ++++++-- .../oidcupstreamwatcher/oidc_upstream_watcher_test.go | 10 ++++++++++ 4 files changed, 33 insertions(+), 2 deletions(-) diff --git a/deploy/supervisor/deployment.yaml b/deploy/supervisor/deployment.yaml index 48607e3f..783f10aa 100644 --- a/deploy/supervisor/deployment.yaml +++ b/deploy/supervisor/deployment.yaml @@ -102,6 +102,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 1cbed34c..041d8b70 100644 --- a/deploy/supervisor/values.yaml +++ b/deploy/supervisor/values.yaml @@ -65,3 +65,11 @@ run_as_group: 1001 #! run_as_group specifies the group ID that will own the proc #! authentication.concierge.pinniped.dev, etc. As an example, if this is set to tuna.io, then #! Pinniped API groups will look like foo.tuna.io. authentication.concierge.tuna.io, etc. api_group_suffix: pinniped.dev + +#! 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/oidcupstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go index b610a2c6..d5a8d302 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go @@ -263,9 +263,13 @@ func (c *oidcWatcherController) validateIssuer(ctx context.Context, upstream *v1 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 { const klogLevelTrace = 6 c.log.V(klogLevelTrace).WithValues( diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go index 9370aad3..24cb9b14 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go @@ -9,6 +9,7 @@ import ( "encoding/json" "net/http" "net/url" + "reflect" "strings" "testing" "time" @@ -797,6 +798,15 @@ oidc: issuer did not match the issuer returned by provider, expected "` + testIs 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{})