From 60cfa470b5ddca480fa1b30d6e336d99fcfd9809 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 9 Oct 2024 17:07:48 -0700 Subject: [PATCH 01/10] hack/prepare-for-integration-tests.sh has new option to firewall IDPs --- hack/prepare-for-integration-tests.sh | 90 ++++++++++++++++++++++++++- 1 file changed, 89 insertions(+), 1 deletion(-) diff --git a/hack/prepare-for-integration-tests.sh b/hack/prepare-for-integration-tests.sh index 338f212b1..0f1f3511c 100755 --- a/hack/prepare-for-integration-tests.sh +++ b/hack/prepare-for-integration-tests.sh @@ -34,7 +34,7 @@ clean_kind=no api_group_suffix="pinniped.dev" # same default as in the values.yaml ytt file dockerfile_path="" get_active_directory_vars="" # specify a filename for a script to get AD related env variables -get_github_vars="" # specify a filename for a script to get GitHub related env variables +get_github_vars="" # specify a filename for a script to get GitHub related env variables alternate_deploy="undefined" pre_install="undefined" @@ -319,6 +319,15 @@ service_https_nodeport_nodeport: $service_https_nodeport_nodeport service_https_clusterip_port: $service_https_clusterip_port EOF +if [[ "${FIREWALL_IDPS:-no}" == "yes" ]]; then + # Configure the web proxy on the Supervisor pods. Note that .svc and .cluster.local are not included, + # so requests for things like dex.tools.svc.cluster.local will go through the web proxy. + cat <>"$data_values_file" +https_proxy: "http://proxy.tools.svc.cluster.local:3128" +no_proxy: "\$(KUBERNETES_SERVICE_HOST),169.254.169.254,127.0.0.1,localhost" +EOF +fi + if [ "$alternate_deploy" != "undefined" ]; then log_note "The Pinniped Supervisor will be deployed with $alternate_deploy pinniped-supervisor $tag $registry_with_port $repo $data_values_file ..." $alternate_deploy pinniped-supervisor "$tag" $registry_with_port $repo $data_values_file @@ -354,6 +363,16 @@ image_tag: $tag discovery_url: $discovery_url EOF +if [[ "${FIREWALL_IDPS:-no}" == "yes" ]]; then + # Configure the web proxy on the Concierge pods. Note that .svc and .cluster.local are not included, + # so requests for things like pinniped-supervisor-clusterip.supervisor.svc.cluster.local and + # local-user-authenticator.local-user-authenticator.svc will go through the web proxy. + cat <>"$data_values_file" +https_proxy: "http://proxy.tools.svc.cluster.local:3128" +no_proxy: "\$(KUBERNETES_SERVICE_HOST),169.254.169.254,127.0.0.1,localhost" +EOF +fi + if [ "$alternate_deploy" != "undefined" ]; then log_note "The Pinniped Concierge will be deployed with $alternate_deploy pinniped-concierge $tag $registry_with_port $repo $data_values_file ..." $alternate_deploy pinniped-concierge "$tag" $registry_with_port $repo $data_values_file @@ -366,6 +385,75 @@ else popd >/dev/null fi +# +# Now that the everything is deployed, optionally firewall the Dex server, the local user authenticator server, +# and the GitHub API so that the Supervisor and Concierge cannot reach them directly. However, the Squid +# proxy server can reach them all, so the Supervisor and Concierge can reach them through the proxy. +# +if [[ "${FIREWALL_IDPS:-no}" == "yes" ]]; then + log_note "Setting up firewalls for the Supervisor and Concierge's outgoing TCP/UDP/SCTP network traffic..." + cat < Date: Wed, 9 Oct 2024 17:11:00 -0700 Subject: [PATCH 02/10] pay attention to web proxy settings during connection probes - WebhookAuthenticator will now detect the proxy setting and skip dialing the connection probe if it should go through a proxy - GitHubIdentityProvider will avoid using tls.Dial altogether by instead making a real request to the GitHub API as its connection probe, because this will respect the proxy settings --- .../webhookcachefiller/webhookcachefiller.go | 67 ++++-- .../webhookcachefiller_test.go | 227 +++++++++++++++++- .../conditionsutil/conditions_util.go | 9 +- .../github_upstream_watcher.go | 89 +++++-- .../github_upstream_watcher_test.go | 225 +++++++---------- .../controllermanager/prepare_controllers.go | 2 + internal/proxydetect/proxydetect.go | 56 +++++ internal/proxydetect/proxydetect_test.go | 77 ++++++ internal/supervisor/server/server.go | 2 +- .../fakeproxydetect/fakeproxydetect.go | 36 +++ ...cierge_webhookauthenticator_status_test.go | 36 ++- .../integration/supervisor_github_idp_test.go | 6 +- test/testlib/client.go | 20 ++ 13 files changed, 643 insertions(+), 209 deletions(-) create mode 100644 internal/proxydetect/proxydetect.go create mode 100644 internal/proxydetect/proxydetect_test.go create mode 100644 internal/testutil/fakeproxydetect/fakeproxydetect.go diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go index edd11b423..15a1bed38 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go @@ -39,6 +39,7 @@ import ( "go.pinniped.dev/internal/endpointaddr" "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/plog" + "go.pinniped.dev/internal/proxydetect" ) const ( @@ -49,10 +50,12 @@ const ( typeEndpointURLValid = "EndpointURLValid" typeAuthenticatorValid = "AuthenticatorValid" - reasonUnableToCreateClient = "UnableToCreateClient" - reasonUnableToInstantiateWebhook = "UnableToInstantiateWebhook" - reasonInvalidEndpointURL = "InvalidEndpointURL" - reasonInvalidEndpointURLScheme = "InvalidEndpointURLScheme" + reasonUnableToCreateClient = "UnableToCreateClient" + reasonUnableToInstantiateWebhook = "UnableToInstantiateWebhook" + reasonInvalidEndpointURL = "InvalidEndpointURL" + reasonInvalidEndpointURLScheme = "InvalidEndpointURLScheme" + reasonInvalidEndpointCannotDetermineProxy = "InvalidEndpointCannotDetermineProxy" + reasonUnableToDialServer = "UnableToDialServer" ) type cachedWebhookAuthenticator struct { @@ -77,6 +80,7 @@ func New( clock clock.Clock, log plog.Logger, dialer ptls.Dialer, + proxyDetector proxydetect.ProxyDetect, ) controllerlib.Controller { return controllerlib.New( controllerlib.Config{ @@ -91,6 +95,7 @@ func New( clock: clock, log: log.WithName(controllerName), dialer: dialer, + proxyDetector: proxyDetector, }, }, withInformer( @@ -127,6 +132,7 @@ type webhookCacheFillerController struct { clock clock.Clock log plog.Logger dialer ptls.Dialer + proxyDetector proxydetect.ProxyDetect } // Sync implements controllerlib.Syncer. @@ -173,7 +179,7 @@ func (c *webhookCacheFillerController) syncIndividualWebhookAuthenticator(ctx co caBundle, conditions, tlsBundleOk := c.validateTLSBundle(webhookAuthenticator.Spec.TLS, conditions) - endpointHostPort, conditions, endpointOk := c.validateEndpoint(webhookAuthenticator.Spec.Endpoint, conditions) + endpointHostPort, conditions, usingProxyForHost, endpointOk := c.validateEndpoint(webhookAuthenticator.Spec.Endpoint, conditions) okSoFar := tlsBundleOk && endpointOk // Only revalidate and update the cache if the cached authenticator is different from the desired authenticator. @@ -194,12 +200,14 @@ func (c *webhookCacheFillerController) syncIndividualWebhookAuthenticator(ctx co // if they need to be updated. logger.Info("cached webhook authenticator and desired webhook authenticator are the same: already cached, so skipping validations") conditions = append(conditions, - successfulWebhookConnectionValidCondition(), + successfulWebhookConnectionValidCondition(usingProxyForHost), successfulAuthenticatorValidCondition(), ) } else { // Run all remaining validations. - a, moreConditions, moreErrs := c.doExpensiveValidations(ctx, webhookAuthenticator, endpointHostPort, caBundle, okSoFar, logger) + a, moreConditions, moreErrs := c.doExpensiveValidations( + ctx, webhookAuthenticator, endpointHostPort, caBundle, okSoFar, usingProxyForHost, logger, + ) newWebhookAuthenticatorForCache = a conditions = append(conditions, moreConditions...) errs = append(errs, moreErrs...) @@ -243,13 +251,14 @@ func (c *webhookCacheFillerController) doExpensiveValidations( endpointHostPort *endpointaddr.HostPort, caBundle *tlsconfigutil.CABundle, okSoFar bool, + usingProxyForHost bool, logger plog.Logger, ) (*cachedWebhookAuthenticator, []*metav1.Condition, []error) { var newWebhookAuthenticatorForCache *cachedWebhookAuthenticator var conditions []*metav1.Condition var errs []error - conditions, tlsNegotiateErr := c.validateConnection(ctx, caBundle.CertPool(), endpointHostPort, conditions, okSoFar, logger) + conditions, tlsNegotiateErr := c.validateConnection(ctx, caBundle.CertPool(), endpointHostPort, conditions, okSoFar, usingProxyForHost, logger) errs = append(errs, tlsNegotiateErr) okSoFar = okSoFar && tlsNegotiateErr == nil @@ -405,12 +414,16 @@ func newWebhookAuthenticator( return webhookAuthenticator, conditions, nil } -func successfulWebhookConnectionValidCondition() *metav1.Condition { +func successfulWebhookConnectionValidCondition(usingProxyForHost bool) *metav1.Condition { + msg := "successfully dialed webhook server" + if usingProxyForHost { + msg = "skipped dialing connection probe because HTTPS_PROXY is configured for use with the specified host" + } return &metav1.Condition{ Type: typeWebhookConnectionValid, Status: metav1.ConditionTrue, Reason: conditionsutil.ReasonSuccess, - Message: "successfully dialed webhook server", + Message: msg, } } @@ -420,6 +433,7 @@ func (c *webhookCacheFillerController) validateConnection( endpointHostPort *endpointaddr.HostPort, conditions []*metav1.Condition, prereqOk bool, + usingProxyForHost bool, logger plog.Logger, ) ([]*metav1.Condition, error) { if !prereqOk { @@ -432,6 +446,13 @@ func (c *webhookCacheFillerController) validateConnection( return conditions, nil } + if usingProxyForHost { + // We cannot assume that we can directly dial the host in the case where we should + // be using a web proxy to reach the host, so skip the dial probe in that case. + conditions = append(conditions, successfulWebhookConnectionValidCondition(usingProxyForHost)) + return conditions, nil + } + dialCtx, dialCancel := context.WithTimeout(ctx, 30*time.Second) defer dialCancel() err := c.dialer.IsReachableAndTLSValidationSucceeds(dialCtx, endpointHostPort.Endpoint(), certPool, logger) @@ -442,17 +463,17 @@ func (c *webhookCacheFillerController) validateConnection( conditions = append(conditions, &metav1.Condition{ Type: typeWebhookConnectionValid, Status: metav1.ConditionFalse, - Reason: conditionsutil.ReasonUnableToDialServer, + Reason: reasonUnableToDialServer, Message: msg, }) return conditions, fmt.Errorf("%s: %w", errText, err) } - conditions = append(conditions, successfulWebhookConnectionValidCondition()) + conditions = append(conditions, successfulWebhookConnectionValidCondition(usingProxyForHost)) return conditions, nil } -func (c *webhookCacheFillerController) validateEndpoint(endpoint string, conditions []*metav1.Condition) (*endpointaddr.HostPort, []*metav1.Condition, bool) { +func (c *webhookCacheFillerController) validateEndpoint(endpoint string, conditions []*metav1.Condition) (*endpointaddr.HostPort, []*metav1.Condition, bool, bool) { endpointURL, err := url.Parse(endpoint) if err != nil { msg := fmt.Sprintf("%s: %s", "spec.endpoint URL cannot be parsed", err.Error()) @@ -462,7 +483,7 @@ func (c *webhookCacheFillerController) validateEndpoint(endpoint string, conditi Reason: reasonInvalidEndpointURL, Message: msg, }) - return nil, conditions, false + return nil, conditions, false, false } // handles empty string and other issues as well. @@ -474,7 +495,7 @@ func (c *webhookCacheFillerController) validateEndpoint(endpoint string, conditi Reason: reasonInvalidEndpointURLScheme, Message: msg, }) - return nil, conditions, false + return nil, conditions, false, false } endpointHostPort, err := endpointaddr.ParseFromURL(endpointURL, 443) @@ -486,7 +507,19 @@ func (c *webhookCacheFillerController) validateEndpoint(endpoint string, conditi Reason: reasonInvalidEndpointURL, Message: msg, }) - return nil, conditions, false + return nil, conditions, false, false + } + + usingProxyForHost, err := c.proxyDetector.UsingProxyForHost(endpointHostPort.Host) + if err != nil { + msg := fmt.Sprintf("%s: %s", "spec.endpoint URL error", err.Error()) + conditions = append(conditions, &metav1.Condition{ + Type: typeEndpointURLValid, + Status: metav1.ConditionFalse, + Reason: reasonInvalidEndpointCannotDetermineProxy, + Message: msg, + }) + return nil, conditions, false, false } conditions = append(conditions, &metav1.Condition{ @@ -495,7 +528,7 @@ func (c *webhookCacheFillerController) validateEndpoint(endpoint string, conditi Reason: conditionsutil.ReasonSuccess, Message: "spec.endpoint is a valid URL", }) - return &endpointHostPort, conditions, true + return &endpointHostPort, conditions, usingProxyForHost, true } func (c *webhookCacheFillerController) updateStatus( diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go index e75e51607..4b0b5fa0c 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go @@ -44,8 +44,10 @@ import ( "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/mocks/mockcachevalue" "go.pinniped.dev/internal/plog" + "go.pinniped.dev/internal/proxydetect" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/internal/testutil/conditionstestutil" + "go.pinniped.dev/internal/testutil/fakeproxydetect" "go.pinniped.dev/internal/testutil/tlsserver" ) @@ -302,6 +304,11 @@ func TestController(t *testing.T) { Message: "successfully dialed webhook server", } } + happyWebhookConnectionValidWithoutDialingDueToProxy := func(time metav1.Time, observedGeneration int64) metav1.Condition { + c := happyWebhookConnectionValid(time, observedGeneration) + c.Message = "skipped dialing connection probe because HTTPS_PROXY is configured for use with the specified host" + return c + } unknownWebhookConnectionValid := func(time metav1.Time, observedGeneration int64) metav1.Condition { return metav1.Condition{ Type: "WebhookConnectionValid", @@ -373,7 +380,16 @@ func TestController(t *testing.T) { Message: fmt.Sprintf(`spec.endpoint URL %s has invalid scheme, require 'https'`, endpoint), } } - + sadEndpointURLValidProxyDetectErr := func(msg string, time metav1.Time, observedGeneration int64) metav1.Condition { + return metav1.Condition{ + Type: "EndpointURLValid", + Status: "False", + ObservedGeneration: observedGeneration, + LastTransitionTime: time, + Reason: "InvalidEndpointCannotDetermineProxy", + Message: msg, + } + } sadEndpointURLValidWithMessage := func(time metav1.Time, observedGeneration int64, msg string) metav1.Condition { return metav1.Condition{ Type: "EndpointURLValid", @@ -412,10 +428,11 @@ func TestController(t *testing.T) { webhookAuthenticators []runtime.Object secretsAndConfigMaps []runtime.Object // for modifying the clients to hack in arbitrary api responses - configClient func(*conciergefake.Clientset) - wantSyncErr testutil.RequireErrorStringFunc - wantLogLines []string - wantActions func() []coretesting.Action + configClient func(*conciergefake.Clientset) + proxyDetector func(t *testing.T) proxydetect.ProxyDetect + wantSyncErr testutil.RequireErrorStringFunc + wantLogLines []string + wantActions func() []coretesting.Action // random comment so lines above don't have huge indents wantNamesOfWebhookAuthenticatorsInCache []string }{ @@ -1011,6 +1028,80 @@ func TestController(t *testing.T) { }, wantNamesOfWebhookAuthenticatorsInCache: []string{"test-name"}, // keeps the old entry in the cache }, + { + name: "Sync: previously cached valid authenticator with unchanged endpoint URL and CA bundle hash has invalid status conditions in informer cache, as can happen on subsequent sync soon after multiple quick status updates (when the informer cache finally catches up), and the webhook host would be reached through a proxy: should update status in current sync", + cache: func(t *testing.T, cache *authncache.Cache) { + oldCA, err := base64.StdEncoding.DecodeString(goodWebhookAuthenticatorSpecWithCA.TLS.CertificateAuthorityData) + require.NoError(t, err) + cache.Store( + authncache.Key{ + Name: "test-name", + Kind: "WebhookAuthenticator", + APIGroup: authenticationv1alpha1.SchemeGroupVersion.Group, + }, + newCacheValue(t, goodWebhookAuthenticatorSpecWithCA, string(oldCA)), + ) + }, + webhookAuthenticators: []runtime.Object{ + &authenticationv1alpha1.WebhookAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-name", + Generation: 1234, + }, + Spec: goodWebhookAuthenticatorSpecWithCA, + Status: authenticationv1alpha1.WebhookAuthenticatorStatus{ + Conditions: conditionstestutil.Replace( + allHappyConditionsSuccess(goodWebhookDefaultServingCertEndpoint, frozenMetav1Now, 0), + []metav1.Condition{ + sadTLSConfigurationValid(frozenMetav1Now, 0), + unknownWebhookConnectionValid(frozenMetav1Now, 0), + unknownAuthenticatorValid(frozenMetav1Now, 0), + sadReadyCondition(frozenMetav1Now, 0), + }, + ), + Phase: "Error", + }, + }, + }, + proxyDetector: func(t *testing.T) proxydetect.ProxyDetect { + // Detect that a proxy is required for the webhook host. + fakeProxyDetect := fakeproxydetect.New(true, nil) + t.Cleanup(func() { + require.Equal(t, 1, fakeProxyDetect.NumberOfInvocations()) + require.Equal(t, "127.0.0.1", fakeProxyDetect.ReceivedHostDuringMostRecentInvocation()) + }) + return fakeProxyDetect + }, + wantLogLines: []string{ + fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"webhookcachefiller-controller","caller":"webhookcachefiller/webhookcachefiller.go:$webhookcachefiller.(*webhookCacheFillerController).syncIndividualWebhookAuthenticator","message":"cached webhook authenticator and desired webhook authenticator are the same: already cached, so skipping validations","webhookAuthenticator":"test-name","endpoint":"%s"}`, goodWebhookAuthenticatorSpecWithCA.Endpoint), + fmt.Sprintf(`{"level":"debug","timestamp":"2099-08-08T13:57:36.123456Z","logger":"webhookcachefiller-controller","caller":"webhookcachefiller/webhookcachefiller.go:$webhookcachefiller.(*webhookCacheFillerController).updateStatus","message":"webhookauthenticator status successfully updated","webhookAuthenticator":"test-name","endpoint":"%s","phase":"Ready"}`, goodWebhookAuthenticatorSpecWithCA.Endpoint), + }, + wantActions: func() []coretesting.Action { + updateStatusAction := coretesting.NewUpdateAction(webhookAuthenticatorGVR, "", &authenticationv1alpha1.WebhookAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-name", + Generation: 1234, + }, + Spec: goodWebhookAuthenticatorSpecWithCA, + Status: authenticationv1alpha1.WebhookAuthenticatorStatus{ // updates the status to ready + Conditions: conditionstestutil.Replace( + allHappyConditionsSuccess(goodWebhookDefaultServingCertEndpoint, frozenMetav1Now, 1234), + []metav1.Condition{ + happyWebhookConnectionValidWithoutDialingDueToProxy(frozenMetav1Now, 1234), + }, + ), + Phase: "Ready", + }, + }) + updateStatusAction.Subresource = "status" + return []coretesting.Action{ + coretesting.NewListAction(webhookAuthenticatorGVR, webhookAuthenticatorGVK, "", metav1.ListOptions{}), + coretesting.NewWatchAction(webhookAuthenticatorGVR, "", metav1.ListOptions{}), + updateStatusAction, + } + }, + wantNamesOfWebhookAuthenticatorsInCache: []string{"test-name"}, // keeps the old entry in the cache + }, { name: "Sync: valid WebhookAuthenticator with CA: will complete sync loop successfully with success conditions and ready phase", webhookAuthenticators: []runtime.Object{ @@ -1021,6 +1112,15 @@ func TestController(t *testing.T) { Spec: goodWebhookAuthenticatorSpecWithCA, }, }, + proxyDetector: func(t *testing.T) proxydetect.ProxyDetect { + // Detect that a proxy is not required for the webhook host. + fakeProxyDetect := fakeproxydetect.New(false, nil) + t.Cleanup(func() { + require.Equal(t, 1, fakeProxyDetect.NumberOfInvocations()) + require.Equal(t, "127.0.0.1", fakeProxyDetect.ReceivedHostDuringMostRecentInvocation()) + }) + return fakeProxyDetect + }, wantLogLines: []string{ fmt.Sprintf(`{"level":"debug","timestamp":"2099-08-08T13:57:36.123456Z","logger":"webhookcachefiller-controller","caller":"webhookcachefiller/webhookcachefiller.go:$webhookcachefiller.(*webhookCacheFillerController).updateStatus","message":"webhookauthenticator status successfully updated","webhookAuthenticator":"test-name","endpoint":"%s","phase":"Ready"}`, goodWebhookDefaultServingCertEndpoint), fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"webhookcachefiller-controller","caller":"webhookcachefiller/webhookcachefiller.go:$webhookcachefiller.(*webhookCacheFillerController).syncIndividualWebhookAuthenticator","message":"added or updated webhook authenticator in cache","webhookAuthenticator":"test-name","endpoint":"%s","isOverwrite":false}`, goodWebhookDefaultServingCertEndpoint), @@ -1045,6 +1145,104 @@ func TestController(t *testing.T) { }, wantNamesOfWebhookAuthenticatorsInCache: []string{"test-name"}, }, + { + name: "Sync: valid WebhookAuthenticator when webhook host will use web proxy: will complete sync loop successfully with success conditions and ready phase", + webhookAuthenticators: []runtime.Object{ + &authenticationv1alpha1.WebhookAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-name", + }, + Spec: goodWebhookAuthenticatorSpecWithCA, + }, + }, + proxyDetector: func(t *testing.T) proxydetect.ProxyDetect { + // Detect that a proxy is required for the webhook host. + fakeProxyDetect := fakeproxydetect.New(true, nil) + t.Cleanup(func() { + require.Equal(t, 1, fakeProxyDetect.NumberOfInvocations()) + require.Equal(t, "127.0.0.1", fakeProxyDetect.ReceivedHostDuringMostRecentInvocation()) + }) + return fakeProxyDetect + }, + wantLogLines: []string{ + fmt.Sprintf(`{"level":"debug","timestamp":"2099-08-08T13:57:36.123456Z","logger":"webhookcachefiller-controller","caller":"webhookcachefiller/webhookcachefiller.go:$webhookcachefiller.(*webhookCacheFillerController).updateStatus","message":"webhookauthenticator status successfully updated","webhookAuthenticator":"test-name","endpoint":"%s","phase":"Ready"}`, goodWebhookDefaultServingCertEndpoint), + fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"webhookcachefiller-controller","caller":"webhookcachefiller/webhookcachefiller.go:$webhookcachefiller.(*webhookCacheFillerController).syncIndividualWebhookAuthenticator","message":"added or updated webhook authenticator in cache","webhookAuthenticator":"test-name","endpoint":"%s","isOverwrite":false}`, goodWebhookDefaultServingCertEndpoint), + }, + wantActions: func() []coretesting.Action { + updateStatusAction := coretesting.NewUpdateAction(webhookAuthenticatorGVR, "", &authenticationv1alpha1.WebhookAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-name", + }, + Spec: goodWebhookAuthenticatorSpecWithCA, + Status: authenticationv1alpha1.WebhookAuthenticatorStatus{ + Conditions: conditionstestutil.Replace( + allHappyConditionsSuccess(goodWebhookDefaultServingCertEndpoint, frozenMetav1Now, 0), + []metav1.Condition{ + happyWebhookConnectionValidWithoutDialingDueToProxy(frozenMetav1Now, 0), + }, + ), + Phase: "Ready", + }, + }) + updateStatusAction.Subresource = "status" + return []coretesting.Action{ + coretesting.NewListAction(webhookAuthenticatorGVR, webhookAuthenticatorGVK, "", metav1.ListOptions{}), + coretesting.NewWatchAction(webhookAuthenticatorGVR, "", metav1.ListOptions{}), + updateStatusAction, + } + }, + wantNamesOfWebhookAuthenticatorsInCache: []string{"test-name"}, + }, + { + name: "Sync: valid WebhookAuthenticator when error while checking if webhook host will use web proxy: will complete sync loop successfully with error conditions and error phase", + webhookAuthenticators: []runtime.Object{ + &authenticationv1alpha1.WebhookAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-name", + }, + Spec: goodWebhookAuthenticatorSpecWithCA, + }, + }, + proxyDetector: func(t *testing.T) proxydetect.ProxyDetect { + // Return a fake error when trying to determine if a proxy is needed for the webhook host. + 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()) + }) + return fakeProxyDetect + }, + wantLogLines: []string{ + fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"webhookcachefiller-controller","caller":"webhookcachefiller/webhookcachefiller.go:$webhookcachefiller.(*webhookCacheFillerController).syncIndividualWebhookAuthenticator","message":"invalid webhook authenticator","webhookAuthenticator":"test-name","endpoint":"%s","removedFromCache":false}`, goodWebhookDefaultServingCertEndpoint), + fmt.Sprintf(`{"level":"debug","timestamp":"2099-08-08T13:57:36.123456Z","logger":"webhookcachefiller-controller","caller":"webhookcachefiller/webhookcachefiller.go:$webhookcachefiller.(*webhookCacheFillerController).updateStatus","message":"webhookauthenticator status successfully updated","webhookAuthenticator":"test-name","endpoint":"%s","phase":"Error"}`, goodWebhookDefaultServingCertEndpoint), + }, + wantActions: func() []coretesting.Action { + updateStatusAction := coretesting.NewUpdateAction(webhookAuthenticatorGVR, "", &authenticationv1alpha1.WebhookAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-name", + }, + Spec: goodWebhookAuthenticatorSpecWithCA, + Status: authenticationv1alpha1.WebhookAuthenticatorStatus{ + Conditions: conditionstestutil.Replace( + allHappyConditionsSuccess(goodWebhookDefaultServingCertEndpoint, frozenMetav1Now, 0), + []metav1.Condition{ + sadEndpointURLValidProxyDetectErr("spec.endpoint URL error: fake proxy detector error", frozenMetav1Now, 0), + unknownWebhookConnectionValid(frozenMetav1Now, 0), + unknownAuthenticatorValid(frozenMetav1Now, 0), + sadReadyCondition(frozenMetav1Now, 0), + }, + ), + Phase: "Error", + }, + }) + updateStatusAction.Subresource = "status" + return []coretesting.Action{ + coretesting.NewListAction(webhookAuthenticatorGVR, webhookAuthenticatorGVK, "", metav1.ListOptions{}), + coretesting.NewWatchAction(webhookAuthenticatorGVR, "", metav1.ListOptions{}), + updateStatusAction, + } + }, + }, { name: "Sync: valid WebhookAuthenticator with IPV6 and CA: will complete sync loop successfully with success conditions and ready phase", webhookAuthenticators: []runtime.Object{ @@ -1925,6 +2123,13 @@ func TestController(t *testing.T) { tt.cache(t, cache) } + if tt.proxyDetector == nil { + // By default, detect that a proxy is not required for any webhook hosts. + tt.proxyDetector = func(t *testing.T) proxydetect.ProxyDetect { + return fakeproxydetect.New(false, nil) + } + } + controller := New( "concierge", // namespace for controller cache, @@ -1935,7 +2140,9 @@ func TestController(t *testing.T) { controllerlib.WithInformer, frozenClock, logger, - ptls.NewDialer()) + ptls.NewDialer(), + tt.proxyDetector(t), + ) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -2179,7 +2386,9 @@ func TestControllerFilterSecret(t *testing.T) { observableInformers.WithInformer, frozenClock, logger, - ptls.NewDialer()) + ptls.NewDialer(), + fakeproxydetect.New(false, nil), + ) unrelated := &corev1.Secret{} filter := observableInformers.GetFilterForInformer(secretInformer) @@ -2241,7 +2450,9 @@ func TestControllerFilterConfigMap(t *testing.T) { observableInformers.WithInformer, frozenClock, logger, - ptls.NewDialer()) + ptls.NewDialer(), + fakeproxydetect.New(false, nil), + ) unrelated := &corev1.ConfigMap{} filter := observableInformers.GetFilterForInformer(configMapInformer) diff --git a/internal/controller/conditionsutil/conditions_util.go b/internal/controller/conditionsutil/conditions_util.go index ee2f2434f..6416b8a88 100644 --- a/internal/controller/conditionsutil/conditions_util.go +++ b/internal/controller/conditionsutil/conditions_util.go @@ -15,11 +15,10 @@ import ( // Some common reasons and messages shared by conditions of various resources. const ( - ReasonSuccess = "Success" - ReasonNotReady = "NotReady" - ReasonUnableToValidate = "UnableToValidate" - ReasonUnableToDialServer = "UnableToDialServer" - ReasonInvalidIssuerURL = "InvalidIssuerURL" + ReasonSuccess = "Success" + ReasonNotReady = "NotReady" + ReasonUnableToValidate = "UnableToValidate" + ReasonInvalidIssuerURL = "InvalidIssuerURL" MessageUnableToValidate = "unable to validate; see other conditions for details" ) diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go index 71cd980f3..93b0cd1f0 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go @@ -33,7 +33,6 @@ import ( "go.pinniped.dev/internal/controller/supervisorconfig/upstreamwatchers" "go.pinniped.dev/internal/controller/tlsconfigutil" "go.pinniped.dev/internal/controllerlib" - "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/endpointaddr" "go.pinniped.dev/internal/federationdomain/upstreamprovider" "go.pinniped.dev/internal/net/phttp" @@ -58,8 +57,9 @@ const ( GitHubConnectionValid string = "GitHubConnectionValid" ClaimsValid string = "ClaimsValid" - reasonInvalid = "Invalid" - reasonInvalidHost = "InvalidHost" + reasonInvalid = "Invalid" + reasonInvalidHost = "InvalidHost" + reasonUnableToMakeRequest = "UnableToMakeRequest" apiDotGithubDotCom = "api.github.com" githubDotCom = "github.com" @@ -107,6 +107,8 @@ func NewGitHubValidatedAPICache(cache *cache.Expiring) GitHubValidatedAPICacheI return &GitHubValidatedAPICache{cache: cache} } +type ProbeURLFunc func(ctx context.Context, client *http.Client, url string) error + type gitHubWatcherController struct { namespace string cache UpstreamGitHubIdentityProviderICache @@ -116,7 +118,7 @@ type gitHubWatcherController struct { secretInformer corev1informers.SecretInformer configMapInformer corev1informers.ConfigMapInformer clock clock.Clock - dialer ptls.Dialer + probeURLFunc ProbeURLFunc validatedCache GitHubValidatedAPICacheI } @@ -131,7 +133,7 @@ func New( log plog.Logger, withInformer pinnipedcontroller.WithInformerOptionFunc, clock clock.Clock, - dialer ptls.Dialer, + probeURLFunc ProbeURLFunc, validatedCache *cache.Expiring, ) controllerlib.Controller { c := gitHubWatcherController{ @@ -143,7 +145,7 @@ func New( secretInformer: secretInformer, configMapInformer: configMapInformer, clock: clock, - dialer: dialer, + probeURLFunc: probeURLFunc, validatedCache: NewGitHubValidatedAPICache(validatedCache), } @@ -299,7 +301,7 @@ func validateOrganizationsPolicy(organizationsSpec *idpv1alpha1.GitHubOrganizati func (c *gitHubWatcherController) validateUpstreamAndUpdateConditions(ctx context.Context, upstream *idpv1alpha1.GitHubIdentityProvider) ( *upstreamgithub.Provider, // If validated, returns the config - error, // This error will only refer to programmatic errors such as inability to perform a Dial or dereference a pointer, not configuration errors + error, // This error will only refer to programmatic errors such as inability to perform a connection probe or dereference a pointer, not configuration errors ) { conditions := make([]*metav1.Condition, 0) applicationErrors := make([]error, 0) @@ -365,7 +367,7 @@ func (c *gitHubWatcherController) validateUpstreamAndUpdateConditions(ctx contex upstreamgithub.ProviderConfig{ Name: upstream.Name, ResourceUID: upstream.UID, - APIBaseURL: apiBaseUrl(apiHostPort), + APIBaseURL: apiBaseURL(apiHostPort), GroupNameAttribute: groupNameAttribute, UsernameAttribute: usernameAttribute, OAuth2Config: &oauth2.Config{ @@ -388,7 +390,7 @@ func (c *gitHubWatcherController) validateUpstreamAndUpdateConditions(ctx contex return provider, utilerrors.NewAggregate(applicationErrors) } -func apiBaseUrl(apiHostPort *endpointaddr.HostPort) string { +func apiBaseURL(apiHostPort *endpointaddr.HostPort) string { endpoint := hostPortForHTTPS(apiHostPort) if strings.ToLower(apiHostPort.Host) == apiDotGithubDotCom { @@ -471,19 +473,33 @@ func (c *gitHubWatcherController) validateGitHubConnection( apiAddress := apiHostPort.Endpoint() - if !c.validatedCache.IsValid(apiAddress, caBundle.Hash()) { - dialCtx, dialCancel := context.WithTimeout(ctx, 30*time.Second) - defer dialCancel() + // Note that this client already has some reasonable timeouts configured on it. + httpClient := phttp.Default(caBundle.CertPool()) - tlsDialErr := c.dialer.IsReachableAndTLSValidationSucceeds(dialCtx, apiAddress, caBundle.CertPool(), c.log) - if tlsDialErr != nil { + if !c.validatedCache.IsValid(apiAddress, caBundle.Hash()) { + // In order to provide feedback on the configured host and CA bundle, probe the configured server by using + // a real HTTPS request, so we can also respect the pod's proxy settings during the request, if they are present. + // To make a real request, we need to choose a real endpoint. + // According to the GitHub API docs for your getting your rate limit status, "Accessing this endpoint does not + // count against your REST API rate limit" and you do not need to be authenticated to use this endpoint. + // See https://docs.github.com/en/rest/rate-limit/rate-limit?apiVersion=2022-11-28#get-rate-limit-status-for-the-authenticated-user. + // Note that like other APIs, the URL path is different for GitHub Enterprise Server (https://HOSTNAME/api/v3/rate_limit) + // versus Enterprise Cloud or Public (https://api.github.com/rate_limit). + // The docs also say that "Unauthenticated requests are associated with the originating IP address" and + // "The primary rate limit for unauthenticated requests is 60 requests per hour." + // So we'll use this endpoint in hopes that we can avoid impacting the rate limit for our originating IP, + // which could potentially be shared with other apps. + rateLimitAPIURL := fmt.Sprintf("%s/rate_limit", apiBaseURL(apiHostPort)) + + err := c.probeURLFunc(ctx, httpClient, rateLimitAPIURL) + if err != nil { return &metav1.Condition{ Type: GitHubConnectionValid, Status: metav1.ConditionFalse, - Reason: conditionsutil.ReasonUnableToDialServer, - Message: fmt.Sprintf("cannot dial %q for spec.githubAPI.host (%q): %s", - apiAddress, *specifiedHost, buildDialErrorMessage(tlsDialErr)), - }, nil, tlsDialErr + Reason: reasonUnableToMakeRequest, + Message: fmt.Sprintf("cannot make connection probe request for spec.githubAPI.host (%q): %s", + *specifiedHost, buildProbeErrorMessage(err)), + }, nil, err } } @@ -493,18 +509,43 @@ func (c *gitHubWatcherController) validateGitHubConnection( Type: GitHubConnectionValid, Status: metav1.ConditionTrue, Reason: conditionsutil.ReasonSuccess, - Message: fmt.Sprintf("dialed %q for spec.githubAPI.host (%q): host is reachable and TLS verification succeeds", + Message: fmt.Sprintf("probed connection to %q for spec.githubAPI.host (%q): host is reachable and TLS verification succeeds", apiAddress, *specifiedHost), - }, phttp.Default(caBundle.CertPool()), nil + }, httpClient, nil } -// buildDialErrorMessage standardizes DNS error messages that appear differently on different platforms, so that tests and log grepping is uniform. -func buildDialErrorMessage(tlsDialErr error) string { - reason := tlsDialErr.Error() +// ProbeURL is the production code for how to probe a GitHub URL to test our connection to the GitHub API. +// It can be replaced via constructor injection for testing. +func ProbeURL(ctx context.Context, client *http.Client, url string) error { + probeRequestCtx, probeRequestCancel := context.WithTimeout(ctx, 30*time.Second) + defer probeRequestCancel() + + probeRequest, err := http.NewRequestWithContext(probeRequestCtx, http.MethodGet, url, nil) + if err != nil { + // Shouldn't really get here as long as the URL is valid. + return err + } + + probeResponse, err := client.Do(probeRequest) + if err != nil { + return err + } + + // Don't care what the response was, even if it was an error status, as long as we were able to connect + // successfully to the specified host using the specified CA bundle and the pod's proxy env var settings. + _ = probeResponse.Body.Close() + + return nil +} + +// buildProbeErrorMessage standardizes DNS error messages that appear differently on different platforms, +// so that tests and log grepping is uniform. +func buildProbeErrorMessage(err error) string { + reason := err.Error() var opError *net.OpError var dnsError *net.DNSError - if errors.As(tlsDialErr, &opError) && errors.As(tlsDialErr, &dnsError) { + if errors.As(err, &opError) && errors.As(err, &dnsError) { dnsError.Server = "" opError.Err = dnsError return opError.Error() diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go index 74e5e6151..c56f30c91 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go @@ -40,7 +40,6 @@ import ( "go.pinniped.dev/internal/controller/supervisorconfig/upstreamwatchers" "go.pinniped.dev/internal/controller/tlsconfigutil" "go.pinniped.dev/internal/controllerlib" - "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/federationdomain/dynamicupstreamprovider" "go.pinniped.dev/internal/federationdomain/upstreamprovider" "go.pinniped.dev/internal/net/phttp" @@ -61,32 +60,27 @@ var ( githubIDPKind = idpv1alpha1.SchemeGroupVersion.WithKind("GitHubIdentityProvider") ) -type fakeGithubDialer struct { - t *testing.T - realAddress string - realCertPool *x509.CertPool +func fakeGithubProbeFuncProbesOtherAddress(t *testing.T, realAddress string) ProbeURLFunc { + return func(ctx context.Context, client *http.Client, url string) error { + t.Helper() + require.Equal(t, "https://api.github.com/rate_limit", url) + r, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "https://"+realAddress+"/rate_limit", nil) + require.NoError(t, err) + resp, err := client.Do(r) + _ = resp.Body.Close() + return err + } } -func (f fakeGithubDialer) IsReachableAndTLSValidationSucceeds(ctx context.Context, address string, _ *x509.CertPool, logger plog.Logger) error { - require.Equal(f.t, "api.github.com:443", address) - - return ptls.NewDialer().IsReachableAndTLSValidationSucceeds(ctx, f.realAddress, f.realCertPool, logger) +func fakeGithubProbeFuncDisallowsAllProbes(t *testing.T) ProbeURLFunc { + return func(ctx context.Context, client *http.Client, url string) error { + t.Helper() + t.Errorf("this test should not perform any connection probe") + t.FailNow() + return nil + } } -var _ ptls.Dialer = (*fakeGithubDialer)(nil) - -type allowNoDials struct { - t *testing.T -} - -func (f allowNoDials) IsReachableAndTLSValidationSucceeds(_ context.Context, _ string, _ *x509.CertPool, _ plog.Logger) error { - f.t.Errorf("this test should not perform dial") - f.t.FailNow() - return nil -} - -var _ ptls.Dialer = (*allowNoDials)(nil) - func TestController(t *testing.T) { require.Equal(t, 6, countExpectedConditions) @@ -215,8 +209,6 @@ func TestController(t *testing.T) { } buildHostValidTrue := func(t *testing.T, host string) metav1.Condition { - t.Helper() - return metav1.Condition{ Type: HostValid, Status: metav1.ConditionTrue, @@ -228,8 +220,6 @@ func TestController(t *testing.T) { } buildHostValidFalse := func(t *testing.T, host, message string) metav1.Condition { - t.Helper() - return metav1.Condition{ Type: HostValid, Status: metav1.ConditionFalse, @@ -240,8 +230,6 @@ func TestController(t *testing.T) { } } buildTLSConfigurationValidTrueWithMsg := func(t *testing.T, msg string) metav1.Condition { - t.Helper() - return metav1.Condition{ Type: TLSConfigurationValid, Status: metav1.ConditionTrue, @@ -253,13 +241,10 @@ func TestController(t *testing.T) { } buildTLSConfigurationValidTrue := func(t *testing.T) metav1.Condition { - t.Helper() return buildTLSConfigurationValidTrueWithMsg(t, "using configured CA bundle") } buildTLSConfigurationValidFalse := func(t *testing.T, message string) metav1.Condition { - t.Helper() - return metav1.Condition{ Type: TLSConfigurationValid, Status: metav1.ConditionFalse, @@ -271,8 +256,6 @@ func TestController(t *testing.T) { } buildOrganizationsPolicyValidTrue := func(t *testing.T, policy idpv1alpha1.GitHubAllowedAuthOrganizationsPolicy) metav1.Condition { - t.Helper() - return metav1.Condition{ Type: OrganizationsPolicyValid, Status: metav1.ConditionTrue, @@ -284,8 +267,6 @@ func TestController(t *testing.T) { } buildOrganizationsPolicyValidFalse := func(t *testing.T, message string) metav1.Condition { - t.Helper() - return metav1.Condition{ Type: OrganizationsPolicyValid, Status: metav1.ConditionFalse, @@ -297,8 +278,6 @@ func TestController(t *testing.T) { } buildClientCredentialsSecretValidTrue := func(t *testing.T, secretName string) metav1.Condition { - t.Helper() - return metav1.Condition{ Type: ClientCredentialsSecretValid, Status: metav1.ConditionTrue, @@ -310,8 +289,6 @@ func TestController(t *testing.T) { } buildClientCredentialsSecretValidFalse := func(t *testing.T, prefix, secretName, namespace, reason string) metav1.Condition { - t.Helper() - return metav1.Condition{ Type: ClientCredentialsSecretValid, Status: metav1.ConditionFalse, @@ -328,8 +305,6 @@ func TestController(t *testing.T) { } buildClaimsValidatedTrue := func(t *testing.T) metav1.Condition { - t.Helper() - return metav1.Condition{ Type: ClaimsValid, Status: metav1.ConditionTrue, @@ -341,8 +316,6 @@ func TestController(t *testing.T) { } buildClaimsValidatedFalse := func(t *testing.T, message string) metav1.Condition { - t.Helper() - return metav1.Condition{ Type: ClaimsValid, Status: metav1.ConditionFalse, @@ -353,35 +326,29 @@ func TestController(t *testing.T) { } } - buildGitHubConnectionValidTrue := func(t *testing.T, hostDialed string, hostSpecified string) metav1.Condition { - t.Helper() - + buildGitHubConnectionValidTrue := func(t *testing.T, hostProbed string, hostSpecified string) metav1.Condition { return metav1.Condition{ Type: GitHubConnectionValid, Status: metav1.ConditionTrue, ObservedGeneration: wantObservedGeneration, LastTransitionTime: wantLastTransitionTime, Reason: conditionsutil.ReasonSuccess, - Message: fmt.Sprintf("dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds", hostDialed, hostSpecified), + Message: fmt.Sprintf("probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds", hostProbed, hostSpecified), } } buildGitHubConnectionValidFalse := func(t *testing.T, message string) metav1.Condition { - t.Helper() - return metav1.Condition{ Type: GitHubConnectionValid, Status: metav1.ConditionFalse, ObservedGeneration: wantObservedGeneration, LastTransitionTime: wantLastTransitionTime, - Reason: "UnableToDialServer", + Reason: "UnableToMakeRequest", Message: message, } } buildGitHubConnectionValidUnknown := func(t *testing.T) metav1.Condition { - t.Helper() - return metav1.Condition{ Type: GitHubConnectionValid, Status: metav1.ConditionUnknown, @@ -416,8 +383,12 @@ func TestController(t *testing.T) { return fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"github-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"some-namespace","name":"%s","type":"TLSConfigurationValid","status":"%s","reason":"%s","message":"%s"}`, name, status, reason, message) } - buildLogForUpdatingGitHubConnectionValid := func(name, status, reason, messageFmt, dialHost, specHost string) string { - return fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"github-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"some-namespace","name":"%s","type":"GitHubConnectionValid","status":"%s","reason":"%s","message":"%s"}`, name, status, reason, fmt.Sprintf(messageFmt, dialHost, specHost)) + buildLogForUpdatingGitHubConnectionValid := func(name, status, reason, messageFmt, probedHost, specHost string) string { + return fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"github-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"some-namespace","name":"%s","type":"GitHubConnectionValid","status":"%s","reason":"%s","message":"%s"}`, name, status, reason, fmt.Sprintf(messageFmt, probedHost, specHost)) + } + + buildLogForUpdatingGitHubConnectionInvalid := func(name, msg string) string { + return fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"github-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"some-namespace","name":"%s","type":"GitHubConnectionValid","status":"%s","reason":"%s","message":"%s"}`, name, "False", "UnableToMakeRequest", msg) } buildLogForUpdatingGitHubConnectionValidUnknown := func(name string) string { @@ -432,7 +403,7 @@ func TestController(t *testing.T) { name string githubIdentityProviders []runtime.Object secretsAndConfigMaps []runtime.Object - mockDialer func(*testing.T) ptls.Dialer + mockProbeURLFunc func(t *testing.T) ProbeURLFunc preexistingValidatedCache []GitHubValidatedAPICacheKey wantErr string wantLogs []string @@ -504,7 +475,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("some-idp-name", "Ready"), }, }, @@ -566,7 +537,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validMinimalIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validMinimalIDP.Spec.GitHubAPI.Host, *validMinimalIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validMinimalIDP.Spec.GitHubAPI.Host, *validMinimalIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("minimal-idp-name", "Ready"), }, }, @@ -581,14 +552,8 @@ func TestController(t *testing.T) { return githubIDP }(), }, - mockDialer: func(t *testing.T) ptls.Dialer { - t.Helper() - - return &fakeGithubDialer{ - t: t, - realAddress: goodServerDomain, - realCertPool: goodServerCertPool, - } + mockProbeURLFunc: func(t *testing.T) ProbeURLFunc { + return fakeGithubProbeFuncProbesOtherAddress(t, goodServerDomain) }, wantResultingCache: []*upstreamgithub.ProviderConfig{ { @@ -647,7 +612,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, "github.com"), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, "api.github.com:443", "github.com"), + buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, "api.github.com:443", "github.com"), buildLogForUpdatingPhase("minimal-idp-name", "Ready"), }, }, @@ -662,14 +627,8 @@ func TestController(t *testing.T) { return githubIDP }(), }, - mockDialer: func(t *testing.T) ptls.Dialer { - t.Helper() - - return &fakeGithubDialer{ - t: t, - realAddress: goodServerDomain, - realCertPool: goodServerCertPool, - } + mockProbeURLFunc: func(t *testing.T) ProbeURLFunc { + return fakeGithubProbeFuncProbesOtherAddress(t, goodServerDomain) }, wantResultingCache: []*upstreamgithub.ProviderConfig{ { @@ -728,7 +687,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, "GitHub.com"), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, "api.github.com:443", "GitHub.com"), + buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, "api.github.com:443", "GitHub.com"), buildLogForUpdatingPhase("minimal-idp-name", "Ready"), }, }, @@ -743,14 +702,8 @@ func TestController(t *testing.T) { return githubIDP }(), }, - mockDialer: func(t *testing.T) ptls.Dialer { - t.Helper() - - return &fakeGithubDialer{ - t: t, - realAddress: goodServerDomain, - realCertPool: goodServerCertPool, - } + mockProbeURLFunc: func(t *testing.T) ProbeURLFunc { + return fakeGithubProbeFuncProbesOtherAddress(t, goodServerDomain) }, wantResultingCache: []*upstreamgithub.ProviderConfig{ { @@ -809,7 +762,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, "api.github.com"), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, "api.github.com:443", "api.github.com"), + buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, "api.github.com:443", "api.github.com"), buildLogForUpdatingPhase("minimal-idp-name", "Ready"), }, }, @@ -824,14 +777,8 @@ func TestController(t *testing.T) { return githubIDP }(), }, - mockDialer: func(t *testing.T) ptls.Dialer { - t.Helper() - - return &fakeGithubDialer{ - t: t, - realAddress: goodServerDomain, - realCertPool: goodServerCertPool, - } + mockProbeURLFunc: func(t *testing.T) ProbeURLFunc { + return fakeGithubProbeFuncProbesOtherAddress(t, goodServerDomain) }, wantResultingCache: []*upstreamgithub.ProviderConfig{ { @@ -890,7 +837,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, "github.com:443"), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, "api.github.com:443", "github.com:443"), + buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, "api.github.com:443", "github.com:443"), buildLogForUpdatingPhase("minimal-idp-name", "Ready"), }, }, @@ -905,14 +852,8 @@ func TestController(t *testing.T) { return githubIDP }(), }, - mockDialer: func(t *testing.T) ptls.Dialer { - t.Helper() - - return &fakeGithubDialer{ - t: t, - realAddress: goodServerDomain, - realCertPool: goodServerCertPool, - } + mockProbeURLFunc: func(t *testing.T) ProbeURLFunc { + return fakeGithubProbeFuncProbesOtherAddress(t, goodServerDomain) }, wantResultingCache: []*upstreamgithub.ProviderConfig{ { @@ -971,7 +912,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, "api.GitHub.com:443"), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, "api.github.com:443", "api.GitHub.com:443"), + buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, "api.github.com:443", "api.GitHub.com:443"), buildLogForUpdatingPhase("minimal-idp-name", "Ready"), }, }, @@ -1048,7 +989,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, goodServerIPv6Domain), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, goodServerIPv6Domain, goodServerIPv6Domain), + buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, goodServerIPv6Domain, goodServerIPv6Domain), buildLogForUpdatingPhase("minimal-idp-name", "Ready"), }, }, @@ -1208,7 +1149,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("invalid-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("invalid-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("invalid-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("invalid-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("invalid-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("invalid-idp-name", "Error"), buildLogForUpdatingClientCredentialsSecretValid("other-idp-name", "True", "Success", `clientID and clientSecret have been read from spec.client.SecretName (\"other-secret-name\")`), @@ -1216,7 +1157,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("other-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("other-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("other-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("other-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("other-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("other-idp-name", "Ready"), buildLogForUpdatingClientCredentialsSecretValid("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), @@ -1224,7 +1165,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("some-idp-name", "Ready"), }, }, @@ -1379,7 +1320,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("idp-with-tls-in-config-map", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("idp-with-tls-in-config-map", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("idp-with-tls-in-config-map", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("idp-with-tls-in-config-map", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("idp-with-tls-in-config-map", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("idp-with-tls-in-config-map", "Ready"), buildLogForUpdatingClientCredentialsSecretValid("idp-with-tls-in-secret", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), @@ -1387,7 +1328,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("idp-with-tls-in-secret", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("idp-with-tls-in-secret", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("idp-with-tls-in-secret", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("idp-with-tls-in-secret", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("idp-with-tls-in-secret", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("idp-with-tls-in-secret", "Ready"), }, }, @@ -1395,11 +1336,7 @@ func TestController(t *testing.T) { name: "happy path with previously validated address/CA Bundle does not validate again", secretsAndConfigMaps: []runtime.Object{goodClientCredentialsSecret}, githubIdentityProviders: []runtime.Object{validFilledOutIDP}, - mockDialer: func(t *testing.T) ptls.Dialer { - t.Helper() - - return &allowNoDials{t: t} - }, + mockProbeURLFunc: fakeGithubProbeFuncDisallowsAllProbes, preexistingValidatedCache: []GitHubValidatedAPICacheKey{ { address: goodServerDomain, @@ -1458,7 +1395,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("some-idp-name", "Ready"), }, }, @@ -1777,7 +1714,7 @@ func TestController(t *testing.T) { Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), buildClientCredentialsSecretValidTrue(t, validMinimalIDP.Spec.Client.SecretName), - buildGitHubConnectionValidFalse(t, fmt.Sprintf(`cannot dial "%s" for spec.githubAPI.host (%q): dial tcp: lookup nowhere.bad-tld: no such host`, "nowhere.bad-tld:443", "nowhere.bad-tld")), + buildGitHubConnectionValidFalse(t, fmt.Sprintf(`cannot make connection probe request for spec.githubAPI.host (%q): dial tcp: lookup nowhere.bad-tld: no such host`, "nowhere.bad-tld")), buildHostValidTrue(t, "nowhere.bad-tld"), buildOrganizationsPolicyValidTrue(t, *validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy), buildTLSConfigurationValidTrue(t), @@ -1791,7 +1728,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, "nowhere.bad-tld"), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "False", "UnableToDialServer", `cannot dial \"%s\" for spec.githubAPI.host (\"%s\"): dial tcp: lookup nowhere.bad-tld: no such host`, "nowhere.bad-tld:443", "nowhere.bad-tld"), + buildLogForUpdatingGitHubConnectionInvalid("minimal-idp-name", fmt.Sprintf(`cannot make connection probe request for spec.githubAPI.host (\"%s\"): dial tcp: lookup nowhere.bad-tld: no such host`, "nowhere.bad-tld")), buildLogForUpdatingPhase("minimal-idp-name", "Error"), }, }, @@ -1860,7 +1797,7 @@ func TestController(t *testing.T) { Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), - buildGitHubConnectionValidFalse(t, fmt.Sprintf(`cannot dial "%s" for spec.githubAPI.host (%q): tls: failed to verify certificate: x509: certificate signed by unknown authority`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host)), + buildGitHubConnectionValidFalse(t, fmt.Sprintf(`cannot make connection probe request for spec.githubAPI.host (%q): Get %q: tls: failed to verify certificate: x509: certificate signed by unknown authority`, *validFilledOutIDP.Spec.GitHubAPI.Host, fmt.Sprintf("https://%s/api/v3/rate_limit", *validFilledOutIDP.Spec.GitHubAPI.Host))), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildOrganizationsPolicyValidTrue(t, *validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy), buildTLSConfigurationValidTrueWithMsg(t, "no TLS configuration provided: using default root CA bundle from container image"), @@ -1874,7 +1811,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls is valid: no TLS configuration provided: using default root CA bundle from container image"), - buildLogForUpdatingGitHubConnectionValid("some-idp-name", "False", "UnableToDialServer", `cannot dial \"%s\" for spec.githubAPI.host (\"%s\"): tls: failed to verify certificate: x509: certificate signed by unknown authority`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionInvalid("some-idp-name", fmt.Sprintf(`cannot make connection probe request for spec.githubAPI.host (\"%s\"): Get \"%s\": tls: failed to verify certificate: x509: certificate signed by unknown authority`, *validFilledOutIDP.Spec.GitHubAPI.Host, fmt.Sprintf("https://%s/api/v3/rate_limit", *validFilledOutIDP.Spec.GitHubAPI.Host))), buildLogForUpdatingPhase("some-idp-name", "Error"), }, }, @@ -1906,7 +1843,7 @@ func TestController(t *testing.T) { Conditions: []metav1.Condition{ buildClaimsValidatedTrue(t), buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), - buildGitHubConnectionValidFalse(t, fmt.Sprintf(`cannot dial "%s" for spec.githubAPI.host (%q): tls: failed to verify certificate: x509: certificate signed by unknown authority`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host)), + buildGitHubConnectionValidFalse(t, fmt.Sprintf(`cannot make connection probe request for spec.githubAPI.host (%q): Get %q: tls: failed to verify certificate: x509: certificate signed by unknown authority`, *validFilledOutIDP.Spec.GitHubAPI.Host, fmt.Sprintf("https://%s/api/v3/rate_limit", *validFilledOutIDP.Spec.GitHubAPI.Host))), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildOrganizationsPolicyValidTrue(t, *validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy), buildTLSConfigurationValidTrue(t), @@ -1920,7 +1857,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("some-idp-name", "False", "UnableToDialServer", `cannot dial \"%s\" for spec.githubAPI.host (\"%s\"): tls: failed to verify certificate: x509: certificate signed by unknown authority`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionInvalid("some-idp-name", fmt.Sprintf(`cannot make connection probe request for spec.githubAPI.host (\"%s\"): Get \"%s\": tls: failed to verify certificate: x509: certificate signed by unknown authority`, *validFilledOutIDP.Spec.GitHubAPI.Host, fmt.Sprintf("https://%s/api/v3/rate_limit", *validFilledOutIDP.Spec.GitHubAPI.Host))), buildLogForUpdatingPhase("some-idp-name", "Error"), }, }, @@ -1961,7 +1898,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "False", "Invalid", "spec.allowAuthentication.organizations.policy must be 'OnlyUsersFromAllowedOrganizations' when spec.allowAuthentication.organizations.allowed has organizations listed"), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("some-idp-name", "Error"), }, }, @@ -2002,7 +1939,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "False", "Invalid", "spec.allowAuthentication.organizations.policy must be 'OnlyUsersFromAllowedOrganizations' when spec.allowAuthentication.organizations.allowed has organizations listed"), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("some-idp-name", "Error"), }, }, @@ -2043,7 +1980,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "False", "Invalid", "spec.allowAuthentication.organizations.policy must be 'OnlyUsersFromAllowedOrganizations' when spec.allowAuthentication.organizations.allowed has organizations listed"), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("some-idp-name", "Error"), }, }, @@ -2084,7 +2021,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "False", "Invalid", "spec.allowAuthentication.organizations.policy must be 'AllGitHubUsers' when spec.allowAuthentication.organizations.allowed is empty"), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("some-idp-name", "Error"), }, }, @@ -2125,7 +2062,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("some-idp-name", "Error"), }, }, @@ -2166,7 +2103,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("some-idp-name", "Error"), }, }, @@ -2207,7 +2144,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("some-idp-name", "Error"), }, }, @@ -2248,7 +2185,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("some-idp-name", "Error"), }, }, @@ -2291,7 +2228,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("minimal-idp-name", "Error"), }, }, @@ -2334,7 +2271,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validMinimalIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validMinimalIDP.Spec.GitHubAPI.Host, *validMinimalIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validMinimalIDP.Spec.GitHubAPI.Host, *validMinimalIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("minimal-idp-name", "Error"), }, }, @@ -2377,7 +2314,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("minimal-idp-name", "Error"), }, }, @@ -2420,7 +2357,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validMinimalIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validMinimalIDP.Spec.GitHubAPI.Host, *validMinimalIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validMinimalIDP.Spec.GitHubAPI.Host, *validMinimalIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("minimal-idp-name", "Error"), }, }, @@ -2463,7 +2400,7 @@ func TestController(t *testing.T) { buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls is valid: using configured CA bundle"), - buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `dialed \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `probed connection to \"%s\" for spec.githubAPI.host (\"%s\"): host is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingPhase("minimal-idp-name", "Error"), }, }, @@ -2491,9 +2428,9 @@ func TestController(t *testing.T) { gitHubIdentityProviderInformer := supervisorInformers.IDP().V1alpha1().GitHubIdentityProviders() - var dialer ptls.Dialer = ptls.NewDialer() - if tt.mockDialer != nil { - dialer = tt.mockDialer(t) + var probeURLFunc ProbeURLFunc = ProbeURL // by default use the production code for probing a URL + if tt.mockProbeURLFunc != nil { + probeURLFunc = tt.mockProbeURLFunc(t) } validatedCache := cache.NewExpiring() @@ -2511,7 +2448,7 @@ func TestController(t *testing.T) { logger, controllerlib.WithInformer, frozenClockForLastTransitionTime, - dialer, + probeURLFunc, validatedCache, ) @@ -2722,7 +2659,7 @@ func TestController_OnlyWantActions(t *testing.T) { ObservedGeneration: 333, LastTransitionTime: oneHourAgo, Reason: conditionsutil.ReasonSuccess, - Message: fmt.Sprintf("dialed %q for spec.githubAPI.host (%q): host is reachable and TLS verification succeeds", goodServerDomain, goodServerDomain), + Message: fmt.Sprintf("probed connection to %q for spec.githubAPI.host (%q): host is reachable and TLS verification succeeds", goodServerDomain, goodServerDomain), }, { Type: HostValid, @@ -2834,7 +2771,7 @@ func TestController_OnlyWantActions(t *testing.T) { ObservedGeneration: 1234, LastTransitionTime: wantLastTransitionTime, Reason: conditionsutil.ReasonSuccess, - Message: fmt.Sprintf("dialed %q for spec.githubAPI.host (%q): host is reachable and TLS verification succeeds", goodServerDomain, goodServerDomain), + Message: fmt.Sprintf("probed connection to %q for spec.githubAPI.host (%q): host is reachable and TLS verification succeeds", goodServerDomain, goodServerDomain), }, { Type: HostValid, @@ -2894,7 +2831,7 @@ func TestController_OnlyWantActions(t *testing.T) { logger, controllerlib.WithInformer, frozenClockForLastTransitionTime, - ptls.NewDialer(), + ProbeURL, cache.NewExpiring(), ) @@ -3018,7 +2955,7 @@ func TestGitHubUpstreamWatcherControllerFilterSecret(t *testing.T) { logger, observableInformers.WithInformer, clock.RealClock{}, - ptls.NewDialer(), + nil, cache.NewExpiring(), ) @@ -3075,7 +3012,7 @@ func TestGitHubUpstreamWatcherControllerFilterConfigMaps(t *testing.T) { logger, observableInformers.WithInformer, clock.RealClock{}, - ptls.NewDialer(), + nil, cache.NewExpiring(), ) @@ -3132,7 +3069,7 @@ func TestGitHubUpstreamWatcherControllerFilterGitHubIDP(t *testing.T) { logger, observableInformers.WithInformer, clock.RealClock{}, - ptls.NewDialer(), + nil, cache.NewExpiring(), ) diff --git a/internal/controllermanager/prepare_controllers.go b/internal/controllermanager/prepare_controllers.go index 0dfc2396a..b3aa5468c 100644 --- a/internal/controllermanager/prepare_controllers.go +++ b/internal/controllermanager/prepare_controllers.go @@ -36,6 +36,7 @@ import ( "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/leaderelection" "go.pinniped.dev/internal/plog" + "go.pinniped.dev/internal/proxydetect" "go.pinniped.dev/internal/tokenclient" ) @@ -246,6 +247,7 @@ func PrepareControllers(c *Config) (controllerinit.RunnerBuilder, error) { //nol clock.RealClock{}, plog.New(), ptls.NewDialer(), + proxydetect.New(), ), singletonWorker, ). diff --git a/internal/proxydetect/proxydetect.go b/internal/proxydetect/proxydetect.go new file mode 100644 index 000000000..eaa2e99fe --- /dev/null +++ b/internal/proxydetect/proxydetect.go @@ -0,0 +1,56 @@ +// Copyright 2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package proxydetect + +import ( + "context" + "fmt" + "net/http" + "net/url" +) + +type ProxyDetect interface { + // UsingProxyForHost returns true if HTTPS requests made to the specified host would be sent through a web proxy. + // It returns false if requests would not be sent through a proxy. It returns an error if it cannot be determined. + UsingProxyForHost(host string) (bool, error) +} + +type detector struct { + // The real http.ProxyFromEnvironment func only reads the env vars once, and then never reads them again + // for the rest of the process's lifetime. This makes it hard to write unit tests that use the real func, + // because you cannot vary the env variables' values between tests, so we'll use a fake in unit tests. + proxyFromEnvironmentFunc func(req *http.Request) (*url.URL, error) +} + +var _ ProxyDetect = (*detector)(nil) + +func New() ProxyDetect { + return &detector{proxyFromEnvironmentFunc: http.ProxyFromEnvironment} +} + +func (d *detector) UsingProxyForHost(host string) (bool, error) { + const msgFmt = "could not determine if requests will be proxied for host %q: %v" + + // Make a request object that represents any HTTPS request to the specified server. + // The other parameter values don't matter, as long as they are valid, because we won't actually make this request. + r, err := http.NewRequestWithContext( + context.Background(), + http.MethodGet, + fmt.Sprintf("https://%s", host), + nil, + ) + if err != nil { + // This will return an error if the host string has an invalid format. + return false, fmt.Errorf(msgFmt, host, err) + } + + // Ask if the request would use a proxy or not. This does not actually make the request. + proxyURL, err := d.proxyFromEnvironmentFunc(r) + if err != nil { + // This could return an error if the HTTPS_PROXY env variable's value had an invalid format, for example. + return false, fmt.Errorf(msgFmt, host, err) + } + + return proxyURL != nil, nil +} diff --git a/internal/proxydetect/proxydetect_test.go b/internal/proxydetect/proxydetect_test.go new file mode 100644 index 000000000..160b10da5 --- /dev/null +++ b/internal/proxydetect/proxydetect_test.go @@ -0,0 +1,77 @@ +// Copyright 2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package proxydetect + +import ( + "errors" + "net/http" + "net/url" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestProxyDetect(t *testing.T) { + t.Parallel() + + proxyURL, err := url.Parse("http://myproxy.com") + require.NoError(t, err) + + tests := []struct { + name string + + proxyFromEnvironmentReturnsURL *url.URL + proxyFromEnvironmentReturnsErr error + + host string + + wantProxied bool + wantErr string + }{ + { + name: "when using proxy for host", + proxyFromEnvironmentReturnsURL: proxyURL, + host: "example.com", + wantProxied: true, + }, + { + name: "when not using proxy for host", + proxyFromEnvironmentReturnsURL: nil, + host: "example.com", + wantProxied: false, + }, + { + name: "when ProxyFromEnvironment returns an error", + proxyFromEnvironmentReturnsErr: errors.New("some error"), + host: "example.com", + wantProxied: false, + wantErr: `could not determine if requests will be proxied for host "example.com": some error`, + }, + { + name: "invalid host", + host: "invalid hostname", + wantProxied: false, + wantErr: `could not determine if requests will be proxied for host "invalid hostname": parse "https://invalid hostname": invalid character " " in host name`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + subject := detector{proxyFromEnvironmentFunc: func(req *http.Request) (*url.URL, error) { + return tt.proxyFromEnvironmentReturnsURL, tt.proxyFromEnvironmentReturnsErr + }} + + proxied, err := subject.UsingProxyForHost(tt.host) + + require.Equal(t, tt.wantProxied, proxied) + if tt.wantErr != "" { + require.Equal(t, tt.wantErr, err.Error()) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index ae2b65167..83c28664a 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -342,7 +342,7 @@ func prepareControllers( plog.New(), controllerlib.WithInformer, clock.RealClock{}, - ptls.NewDialer(), + githubupstreamwatcher.ProbeURL, cache.NewExpiring(), ), singletonWorker). diff --git a/internal/testutil/fakeproxydetect/fakeproxydetect.go b/internal/testutil/fakeproxydetect/fakeproxydetect.go new file mode 100644 index 000000000..5d45e47db --- /dev/null +++ b/internal/testutil/fakeproxydetect/fakeproxydetect.go @@ -0,0 +1,36 @@ +// Copyright 2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package fakeproxydetect + +import "go.pinniped.dev/internal/proxydetect" + +type FakeProxyDetect struct { + returnBool bool + returnErr error + numCalls int + receivedHost string +} + +var _ proxydetect.ProxyDetect = (*FakeProxyDetect)(nil) + +func (f *FakeProxyDetect) UsingProxyForHost(host string) (bool, error) { + f.numCalls++ + f.receivedHost = host + return f.returnBool, f.returnErr +} + +func (f *FakeProxyDetect) ReceivedHostDuringMostRecentInvocation() string { + return f.receivedHost +} + +func (f *FakeProxyDetect) NumberOfInvocations() int { + return f.numCalls +} + +func New(returnBool bool, returnErr error) *FakeProxyDetect { + return &FakeProxyDetect{ + returnBool: returnBool, + returnErr: returnErr, + } +} diff --git a/test/integration/concierge_webhookauthenticator_status_test.go b/test/integration/concierge_webhookauthenticator_status_test.go index c5a9dec3c..985c3e19e 100644 --- a/test/integration/concierge_webhookauthenticator_status_test.go +++ b/test/integration/concierge_webhookauthenticator_status_test.go @@ -153,14 +153,16 @@ func TestConciergeWebhookAuthenticatorStatus_Parallel(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) t.Cleanup(cancel) + conciergeUsingHTTPSProxy := testlib.DeploymentsContainerHasHTTPSProxyEnvVar(t, env.ConciergeNamespace, env.ConciergeAppName) + caBundleSomePivotalCA := "LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSURVVENDQWptZ0F3SUJBZ0lWQUpzNStTbVRtaTJXeUI0bGJJRXBXaUs5a1RkUE1BMEdDU3FHU0liM0RRRUIKQ3dVQU1COHhDekFKQmdOVkJBWVRBbFZUTVJBd0RnWURWUVFLREFkUWFYWnZkR0ZzTUI0WERUSXdNRFV3TkRFMgpNamMxT0ZvWERUSTBNRFV3TlRFMk1qYzFPRm93SHpFTE1Ba0dBMVVFQmhNQ1ZWTXhFREFPQmdOVkJBb01CMUJwCmRtOTBZV3d3Z2dFaU1BMEdDU3FHU0liM0RRRUJBUVVBQTRJQkR3QXdnZ0VLQW9JQkFRRERZWmZvWGR4Z2NXTEMKZEJtbHB5a0tBaG9JMlBuUWtsVFNXMno1cGcwaXJjOGFRL1E3MXZzMTRZYStmdWtFTGlvOTRZYWw4R01DdVFrbApMZ3AvUEE5N1VYelhQNDBpK25iNXcwRGpwWWd2dU9KQXJXMno2MFRnWE5NSFh3VHk4ME1SZEhpUFVWZ0VZd0JpCmtkNThzdEFVS1Y1MnBQTU1reTJjNy9BcFhJNmRXR2xjalUvaFBsNmtpRzZ5dEw2REtGYjJQRWV3MmdJM3pHZ2IKOFVVbnA1V05DZDd2WjNVY0ZHNXlsZEd3aGc3cnZ4U1ZLWi9WOEhCMGJmbjlxamlrSVcxWFM4dzdpUUNlQmdQMApYZWhKZmVITlZJaTJtZlczNlVQbWpMdnVKaGpqNDIrdFBQWndvdDkzdWtlcEgvbWpHcFJEVm9wamJyWGlpTUYrCkYxdnlPNGMxQWdNQkFBR2pnWU13Z1lBd0hRWURWUjBPQkJZRUZNTWJpSXFhdVkwajRVWWphWDl0bDJzby9LQ1IKTUI4R0ExVWRJd1FZTUJhQUZNTWJpSXFhdVkwajRVWWphWDl0bDJzby9LQ1JNQjBHQTFVZEpRUVdNQlFHQ0NzRwpBUVVGQndNQ0JnZ3JCZ0VGQlFjREFUQVBCZ05WSFJNQkFmOEVCVEFEQVFIL01BNEdBMVVkRHdFQi93UUVBd0lCCkJqQU5CZ2txaGtpRzl3MEJBUXNGQUFPQ0FRRUFYbEh4M2tIMDZwY2NDTDlEVE5qTnBCYnlVSytGd2R6T2IwWFYKcmpNaGtxdHVmdEpUUnR5T3hKZ0ZKNXhUR3pCdEtKamcrVU1pczBOV0t0VDBNWThVMU45U2c5SDl0RFpHRHBjVQpxMlVRU0Y4dXRQMVR3dnJIUzIrdzB2MUoxdHgrTEFiU0lmWmJCV0xXQ21EODUzRlVoWlFZekkvYXpFM28vd0p1CmlPUklMdUpNUk5vNlBXY3VLZmRFVkhaS1RTWnk3a25FcHNidGtsN3EwRE91eUFWdG9HVnlkb3VUR0FOdFhXK2YKczNUSTJjKzErZXg3L2RZOEJGQTFzNWFUOG5vZnU3T1RTTzdiS1kzSkRBUHZOeFQzKzVZUXJwNGR1Nmh0YUFMbAppOHNaRkhidmxpd2EzdlhxL3p1Y2JEaHEzQzBhZnAzV2ZwRGxwSlpvLy9QUUFKaTZLQT09Ci0tLS0tRU5EIENFUlRJRklDQVRFLS0tLS0K" tests := []struct { name string + maybeSkip func(t *testing.T) spec func() *authenticationv1alpha1.WebhookAuthenticatorSpec initialPhase authenticationv1alpha1.WebhookAuthenticatorPhase finalConditions []metav1.Condition - run func(t *testing.T) }{ { name: "basic test to see if the WebhookAuthenticator wakes up or not", @@ -168,10 +170,15 @@ func TestConciergeWebhookAuthenticatorStatus_Parallel(t *testing.T) { return &env.TestWebhook }, initialPhase: authenticationv1alpha1.WebhookAuthenticatorPhaseReady, - finalConditions: allSuccessfulWebhookAuthenticatorConditions(), + finalConditions: allSuccessfulWebhookAuthenticatorConditions(conciergeUsingHTTPSProxy), }, { name: "valid spec with invalid CA in TLS config will result in a WebhookAuthenticator that is not ready", + maybeSkip: func(t *testing.T) { + if conciergeUsingHTTPSProxy { + t.Skip("Skipping test that requires HTTPS_PROXY to be unset") + } + }, spec: func() *authenticationv1alpha1.WebhookAuthenticatorSpec { caBundleString := "invalid base64-encoded data" webhookSpec := env.TestWebhook.DeepCopy() @@ -182,7 +189,7 @@ func TestConciergeWebhookAuthenticatorStatus_Parallel(t *testing.T) { }, initialPhase: authenticationv1alpha1.WebhookAuthenticatorPhaseError, finalConditions: replaceSomeConditions( - allSuccessfulWebhookAuthenticatorConditions(), + allSuccessfulWebhookAuthenticatorConditions(false), []metav1.Condition{ { Type: "Ready", @@ -210,6 +217,11 @@ func TestConciergeWebhookAuthenticatorStatus_Parallel(t *testing.T) { }, { name: "valid spec with valid CA in TLS config but does not match issuer server will result in a WebhookAuthenticator that is not ready", + maybeSkip: func(t *testing.T) { + if conciergeUsingHTTPSProxy { + t.Skip("Skipping test that requires HTTPS_PROXY to be unset") + } + }, spec: func() *authenticationv1alpha1.WebhookAuthenticatorSpec { webhookSpec := env.TestWebhook.DeepCopy() webhookSpec.TLS = &authenticationv1alpha1.TLSSpec{ @@ -219,7 +231,7 @@ func TestConciergeWebhookAuthenticatorStatus_Parallel(t *testing.T) { }, initialPhase: authenticationv1alpha1.WebhookAuthenticatorPhaseError, finalConditions: replaceSomeConditions( - allSuccessfulWebhookAuthenticatorConditions(), + allSuccessfulWebhookAuthenticatorConditions(false), []metav1.Condition{ { Type: "Ready", @@ -242,6 +254,11 @@ func TestConciergeWebhookAuthenticatorStatus_Parallel(t *testing.T) { }, { name: "invalid with unresponsive endpoint will result in a WebhookAuthenticator that is not ready", + maybeSkip: func(t *testing.T) { + if conciergeUsingHTTPSProxy { + t.Skip("Skipping test that requires HTTPS_PROXY to be unset") + } + }, spec: func() *authenticationv1alpha1.WebhookAuthenticatorSpec { webhookSpec := env.TestWebhook.DeepCopy() webhookSpec.TLS = &authenticationv1alpha1.TLSSpec{ @@ -252,7 +269,7 @@ func TestConciergeWebhookAuthenticatorStatus_Parallel(t *testing.T) { }, initialPhase: authenticationv1alpha1.WebhookAuthenticatorPhaseError, finalConditions: replaceSomeConditions( - allSuccessfulWebhookAuthenticatorConditions(), + allSuccessfulWebhookAuthenticatorConditions(false), []metav1.Condition{ { Type: "Ready", @@ -397,7 +414,12 @@ func TestConciergeWebhookAuthenticatorCRDValidations_Parallel(t *testing.T) { } } -func allSuccessfulWebhookAuthenticatorConditions() []metav1.Condition { +func allSuccessfulWebhookAuthenticatorConditions(conciergeUsingHTTPSProxy bool) []metav1.Condition { + dialMessage := "successfully dialed webhook server" + if conciergeUsingHTTPSProxy { + dialMessage = "skipped dialing connection probe because HTTPS_PROXY is configured for use with the specified host" + } + return []metav1.Condition{ { Type: "AuthenticatorValid", @@ -427,7 +449,7 @@ func allSuccessfulWebhookAuthenticatorConditions() []metav1.Condition { Type: "WebhookConnectionValid", Status: "True", Reason: "Success", - Message: "successfully dialed webhook server", + Message: dialMessage, }, } } diff --git a/test/integration/supervisor_github_idp_test.go b/test/integration/supervisor_github_idp_test.go index bd713254a..ae71060d4 100644 --- a/test/integration/supervisor_github_idp_test.go +++ b/test/integration/supervisor_github_idp_test.go @@ -392,7 +392,7 @@ func TestGitHubIDPPhaseAndConditions_Parallel(t *testing.T) { Type: "GitHubConnectionValid", Status: metav1.ConditionTrue, Reason: "Success", - Message: `dialed "api.github.com:443" for spec.githubAPI.host ("github.com"): host is reachable and TLS verification succeeds`, + Message: `probed connection to "api.github.com:443" for spec.githubAPI.host ("github.com"): host is reachable and TLS verification succeeds`, }, { Type: "HostValid", @@ -461,7 +461,7 @@ func TestGitHubIDPPhaseAndConditions_Parallel(t *testing.T) { Type: "GitHubConnectionValid", Status: metav1.ConditionTrue, Reason: "Success", - Message: `dialed "api.github.com:443" for spec.githubAPI.host ("api.github.com"): host is reachable and TLS verification succeeds`, + Message: `probed connection to "api.github.com:443" for spec.githubAPI.host ("api.github.com"): host is reachable and TLS verification succeeds`, }, { Type: "HostValid", @@ -668,7 +668,7 @@ func TestGitHubIDPSecretInOtherNamespace_Parallel(t *testing.T) { Type: "GitHubConnectionValid", Status: metav1.ConditionTrue, Reason: "Success", - Message: `dialed "api.github.com:443" for spec.githubAPI.host ("github.com"): host is reachable and TLS verification succeeds`, + Message: `probed connection to "api.github.com:443" for spec.githubAPI.host ("github.com"): host is reachable and TLS verification succeeds`, }, { Type: "HostValid", diff --git a/test/testlib/client.go b/test/testlib/client.go index 731e26d21..01fa87b4f 100644 --- a/test/testlib/client.go +++ b/test/testlib/client.go @@ -1109,3 +1109,23 @@ func ObjectMetaWithRandomName(t *testing.T, baseName string) metav1.ObjectMeta { Annotations: map[string]string{"pinniped.dev/testName": t.Name()}, } } + +func DeploymentsContainerHasHTTPSProxyEnvVar(t *testing.T, namespaceName string, deploymentName string) bool { + client := NewKubernetesClientset(t) + + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + + d, err := client.AppsV1().Deployments(namespaceName).Get(ctx, deploymentName, metav1.GetOptions{}) + require.NoError(t, err) + + for _, c := range d.Spec.Template.Spec.Containers { + for _, e := range c.Env { + if e.Name == "HTTPS_PROXY" { + return true + } + } + } + + return false +} From 2aa30edb88cb8b288145103d3deb4d92f86ff954 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 10 Oct 2024 11:45:16 -0700 Subject: [PATCH 03/10] explicitly define "podSelector" in NetworkPolicies because Kube API docs say it is not optional --- hack/prepare-for-integration-tests.sh | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/hack/prepare-for-integration-tests.sh b/hack/prepare-for-integration-tests.sh index 0f1f3511c..88e95c0d8 100755 --- a/hack/prepare-for-integration-tests.sh +++ b/hack/prepare-for-integration-tests.sh @@ -391,7 +391,7 @@ fi # proxy server can reach them all, so the Supervisor and Concierge can reach them through the proxy. # if [[ "${FIREWALL_IDPS:-no}" == "yes" ]]; then - log_note "Setting up firewalls for the Supervisor and Concierge's outgoing TCP/UDP/SCTP network traffic..." + log_note "Setting up firewalls for the Supervisor and Concierge's outgoing TCP/UDP network traffic..." cat < Date: Thu, 10 Oct 2024 14:41:49 -0700 Subject: [PATCH 04/10] fix integration test for WebhookAuthenticator status conditions --- .../concierge_jwtauthenticator_status_test.go | 6 ++-- ...cierge_webhookauthenticator_status_test.go | 33 +++++++++--------- ...supervisor_federationdomain_status_test.go | 34 ++++++++++++------- 3 files changed, 41 insertions(+), 32 deletions(-) diff --git a/test/integration/concierge_jwtauthenticator_status_test.go b/test/integration/concierge_jwtauthenticator_status_test.go index bc2d502b9..2a34d5816 100644 --- a/test/integration/concierge_jwtauthenticator_status_test.go +++ b/test/integration/concierge_jwtauthenticator_status_test.go @@ -192,7 +192,7 @@ func TestConciergeJWTAuthenticatorStatus_Parallel(t *testing.T) { }, }, wantPhase: authenticationv1alpha1.JWTAuthenticatorPhaseError, - wantConditions: replaceSomeConditions( + wantConditions: replaceSomeConditions(t, allSuccessfulJWTAuthenticatorConditions(true), []metav1.Condition{ { @@ -242,7 +242,7 @@ func TestConciergeJWTAuthenticatorStatus_Parallel(t *testing.T) { }, }, wantPhase: authenticationv1alpha1.JWTAuthenticatorPhaseError, - wantConditions: replaceSomeConditions( + wantConditions: replaceSomeConditions(t, allSuccessfulJWTAuthenticatorConditions(true), []metav1.Condition{ { @@ -286,7 +286,7 @@ func TestConciergeJWTAuthenticatorStatus_Parallel(t *testing.T) { }, }, wantPhase: authenticationv1alpha1.JWTAuthenticatorPhaseError, - wantConditions: replaceSomeConditions( + wantConditions: replaceSomeConditions(t, allSuccessfulJWTAuthenticatorConditions(len(env.SupervisorUpstreamOIDC.CABundle) != 0), []metav1.Condition{ { diff --git a/test/integration/concierge_webhookauthenticator_status_test.go b/test/integration/concierge_webhookauthenticator_status_test.go index 985c3e19e..1444ac204 100644 --- a/test/integration/concierge_webhookauthenticator_status_test.go +++ b/test/integration/concierge_webhookauthenticator_status_test.go @@ -165,7 +165,7 @@ func TestConciergeWebhookAuthenticatorStatus_Parallel(t *testing.T) { finalConditions []metav1.Condition }{ { - name: "basic test to see if the WebhookAuthenticator wakes up or not", + name: "happy path", spec: func() *authenticationv1alpha1.WebhookAuthenticatorSpec { return &env.TestWebhook }, @@ -174,11 +174,6 @@ func TestConciergeWebhookAuthenticatorStatus_Parallel(t *testing.T) { }, { name: "valid spec with invalid CA in TLS config will result in a WebhookAuthenticator that is not ready", - maybeSkip: func(t *testing.T) { - if conciergeUsingHTTPSProxy { - t.Skip("Skipping test that requires HTTPS_PROXY to be unset") - } - }, spec: func() *authenticationv1alpha1.WebhookAuthenticatorSpec { caBundleString := "invalid base64-encoded data" webhookSpec := env.TestWebhook.DeepCopy() @@ -188,7 +183,7 @@ func TestConciergeWebhookAuthenticatorStatus_Parallel(t *testing.T) { return webhookSpec }, initialPhase: authenticationv1alpha1.WebhookAuthenticatorPhaseError, - finalConditions: replaceSomeConditions( + finalConditions: replaceSomeConditions(t, allSuccessfulWebhookAuthenticatorConditions(false), []metav1.Condition{ { @@ -219,6 +214,9 @@ func TestConciergeWebhookAuthenticatorStatus_Parallel(t *testing.T) { name: "valid spec with valid CA in TLS config but does not match issuer server will result in a WebhookAuthenticator that is not ready", maybeSkip: func(t *testing.T) { if conciergeUsingHTTPSProxy { + // Skip this test when HTTPS_PROXY is in use, because WebhookConnectionValid will have status Success + // with a message saying that the dialing was skipped due to the proxy setting, so the expectations + // below are wrong for that case. t.Skip("Skipping test that requires HTTPS_PROXY to be unset") } }, @@ -230,7 +228,7 @@ func TestConciergeWebhookAuthenticatorStatus_Parallel(t *testing.T) { return webhookSpec }, initialPhase: authenticationv1alpha1.WebhookAuthenticatorPhaseError, - finalConditions: replaceSomeConditions( + finalConditions: replaceSomeConditions(t, allSuccessfulWebhookAuthenticatorConditions(false), []metav1.Condition{ { @@ -254,21 +252,20 @@ func TestConciergeWebhookAuthenticatorStatus_Parallel(t *testing.T) { }, { name: "invalid with unresponsive endpoint will result in a WebhookAuthenticator that is not ready", - maybeSkip: func(t *testing.T) { - if conciergeUsingHTTPSProxy { - t.Skip("Skipping test that requires HTTPS_PROXY to be unset") - } - }, spec: func() *authenticationv1alpha1.WebhookAuthenticatorSpec { webhookSpec := env.TestWebhook.DeepCopy() webhookSpec.TLS = &authenticationv1alpha1.TLSSpec{ CertificateAuthorityData: caBundleSomePivotalCA, } - webhookSpec.Endpoint = "https://127.0.0.1:443/some-fake-endpoint" + // Note that requests to 127.0.0.1 will not be proxied even when HTTPS_PROXY is set on the Concierge, + // so it's okay to run this test in that case too. The Concierge will attempt the dial because it sees + // that the request would not be proxied anyway. 127.0.0.1:8781 will be seen by the Concierge as a port + // local to its own pod, which does not exist so the connection will fail. + webhookSpec.Endpoint = "https://127.0.0.1:8781/some-fake-endpoint" return webhookSpec }, initialPhase: authenticationv1alpha1.WebhookAuthenticatorPhaseError, - finalConditions: replaceSomeConditions( + finalConditions: replaceSomeConditions(t, allSuccessfulWebhookAuthenticatorConditions(false), []metav1.Condition{ { @@ -285,7 +282,7 @@ func TestConciergeWebhookAuthenticatorStatus_Parallel(t *testing.T) { Type: "WebhookConnectionValid", Status: "False", Reason: "UnableToDialServer", - Message: "cannot dial server: dial tcp 127.0.0.1:443: connect: connection refused", + Message: "cannot dial server: dial tcp 127.0.0.1:8781: connect: connection refused", }, }, ), @@ -296,6 +293,10 @@ func TestConciergeWebhookAuthenticatorStatus_Parallel(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() + if tt.maybeSkip != nil { + tt.maybeSkip(t) + } + webhookAuthenticator := testlib.CreateTestWebhookAuthenticator( ctx, t, diff --git a/test/integration/supervisor_federationdomain_status_test.go b/test/integration/supervisor_federationdomain_status_test.go index 27877b616..712bb6577 100644 --- a/test/integration/supervisor_federationdomain_status_test.go +++ b/test/integration/supervisor_federationdomain_status_test.go @@ -46,8 +46,8 @@ func TestSupervisorFederationDomainStatus_Disruptive(t *testing.T) { fd := testlib.CreateTestFederationDomain(ctx, t, supervisorconfigv1alpha1.FederationDomainSpec{ Issuer: "https://example.com/fake", }, supervisorconfigv1alpha1.FederationDomainPhaseError) - testlib.WaitForFederationDomainStatusConditions(ctx, t, fd.Name, replaceSomeConditions( - allSuccessfulLegacyFederationDomainConditions("", fd.Spec), + testlib.WaitForFederationDomainStatusConditions(ctx, t, fd.Name, replaceSomeConditions(t, + allSuccessfulLegacyFederationDomainConditions(t, "", fd.Spec), []metav1.Condition{ { Type: "IdentityProvidersFound", Status: "False", Reason: "LegacyConfigurationIdentityProviderNotFound", @@ -67,7 +67,7 @@ func TestSupervisorFederationDomainStatus_Disruptive(t *testing.T) { }, idpv1alpha1.PhaseError) testlib.WaitForFederationDomainStatusPhase(ctx, t, fd.Name, supervisorconfigv1alpha1.FederationDomainPhaseReady) testlib.WaitForFederationDomainStatusConditions(ctx, t, fd.Name, - allSuccessfulLegacyFederationDomainConditions(oidcIdentityProvider1.Name, fd.Spec)) + allSuccessfulLegacyFederationDomainConditions(t, oidcIdentityProvider1.Name, fd.Spec)) // Creating a second IDP should put the FederationDomain back into an error status again. oidcIdentityProvider2 := testlib.CreateTestOIDCIdentityProvider(t, idpv1alpha1.OIDCIdentityProviderSpec{ @@ -75,8 +75,8 @@ func TestSupervisorFederationDomainStatus_Disruptive(t *testing.T) { Client: idpv1alpha1.OIDCClient{SecretName: "this-will-not-exist-but-does-not-matter"}, }, idpv1alpha1.PhaseError) testlib.WaitForFederationDomainStatusPhase(ctx, t, fd.Name, supervisorconfigv1alpha1.FederationDomainPhaseError) - testlib.WaitForFederationDomainStatusConditions(ctx, t, fd.Name, replaceSomeConditions( - allSuccessfulLegacyFederationDomainConditions(oidcIdentityProvider2.Name, fd.Spec), + testlib.WaitForFederationDomainStatusConditions(ctx, t, fd.Name, replaceSomeConditions(t, + allSuccessfulLegacyFederationDomainConditions(t, oidcIdentityProvider2.Name, fd.Spec), []metav1.Condition{ { Type: "IdentityProvidersFound", Status: "False", Reason: "IdentityProviderNotSpecified", @@ -121,7 +121,7 @@ func TestSupervisorFederationDomainStatus_Disruptive(t *testing.T) { }, }, }, supervisorconfigv1alpha1.FederationDomainPhaseError) - testlib.WaitForFederationDomainStatusConditions(ctx, t, fd.Name, replaceSomeConditions( + testlib.WaitForFederationDomainStatusConditions(ctx, t, fd.Name, replaceSomeConditions(t, allSuccessfulFederationDomainConditions(fd.Spec), []metav1.Condition{ { @@ -145,7 +145,7 @@ func TestSupervisorFederationDomainStatus_Disruptive(t *testing.T) { Client: idpv1alpha1.OIDCClient{SecretName: "this-will-not-exist-but-does-not-matter"}, }, oidcIDP1Meta, idpv1alpha1.PhaseError) testlib.WaitForFederationDomainStatusPhase(ctx, t, fd.Name, supervisorconfigv1alpha1.FederationDomainPhaseError) - testlib.WaitForFederationDomainStatusConditions(ctx, t, fd.Name, replaceSomeConditions( + testlib.WaitForFederationDomainStatusConditions(ctx, t, fd.Name, replaceSomeConditions(t, allSuccessfulFederationDomainConditions(fd.Spec), []metav1.Condition{ { @@ -173,7 +173,7 @@ func TestSupervisorFederationDomainStatus_Disruptive(t *testing.T) { err := oidcIDPClient.Delete(ctx, oidcIdentityProvider1.Name, metav1.DeleteOptions{}) require.NoError(t, err) testlib.WaitForFederationDomainStatusPhase(ctx, t, fd.Name, supervisorconfigv1alpha1.FederationDomainPhaseError) - testlib.WaitForFederationDomainStatusConditions(ctx, t, fd.Name, replaceSomeConditions( + testlib.WaitForFederationDomainStatusConditions(ctx, t, fd.Name, replaceSomeConditions(t, allSuccessfulFederationDomainConditions(fd.Spec), []metav1.Condition{ { @@ -340,7 +340,7 @@ func TestSupervisorFederationDomainStatus_Disruptive(t *testing.T) { }, }, supervisorconfigv1alpha1.FederationDomainPhaseError) - testlib.WaitForFederationDomainStatusConditions(ctx, t, fd.Name, replaceSomeConditions( + testlib.WaitForFederationDomainStatusConditions(ctx, t, fd.Name, replaceSomeConditions(t, allSuccessfulFederationDomainConditions(fd.Spec), []metav1.Condition{ { @@ -483,7 +483,7 @@ func TestSupervisorFederationDomainStatus_Disruptive(t *testing.T) { }) require.NoError(t, err) - testlib.WaitForFederationDomainStatusConditions(ctx, t, fd.Name, replaceSomeConditions( + testlib.WaitForFederationDomainStatusConditions(ctx, t, fd.Name, replaceSomeConditions(t, allSuccessfulFederationDomainConditions(fd.Spec), []metav1.Condition{ { @@ -950,22 +950,30 @@ func TestSupervisorFederationDomainCRDValidations_Parallel(t *testing.T) { } } -func replaceSomeConditions(conditions []metav1.Condition, replaceWithTheseConditions []metav1.Condition) []metav1.Condition { +func replaceSomeConditions(t *testing.T, conditions []metav1.Condition, replaceWithTheseConditions []metav1.Condition) []metav1.Condition { cp := make([]metav1.Condition, len(conditions)) copy(cp, conditions) for _, replacementCond := range replaceWithTheseConditions { + found := false for i, cond := range cp { if replacementCond.Type == cond.Type { cp[i] = replacementCond + found = true break } } + if !found { + require.Failf(t, + "test setup problem", + "replaceSomeConditions() helper was called to replace condition of type %q but no such condition was found in conditions slice", + replacementCond.Type) + } } return cp } -func allSuccessfulLegacyFederationDomainConditions(idpName string, federationDomainSpec supervisorconfigv1alpha1.FederationDomainSpec) []metav1.Condition { - return replaceSomeConditions( +func allSuccessfulLegacyFederationDomainConditions(t *testing.T, idpName string, federationDomainSpec supervisorconfigv1alpha1.FederationDomainSpec) []metav1.Condition { + return replaceSomeConditions(t, allSuccessfulFederationDomainConditions(federationDomainSpec), []metav1.Condition{ { From 4d2bbac674cdfce65e825b5bdc9e127b6189e5e6 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 10 Oct 2024 14:44:14 -0700 Subject: [PATCH 05/10] use .cluster.local address for LUA (squid cannot resolve .svc addresses) --- hack/prepare-for-integration-tests.sh | 4 ++-- hack/prepare-impersonator-on-kind.sh | 4 ++-- internal/controller/apicerts/certs_manager.go | 3 ++- internal/controller/apicerts/certs_manager_test.go | 3 ++- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/hack/prepare-for-integration-tests.sh b/hack/prepare-for-integration-tests.sh index 88e95c0d8..0f8f7c045 100755 --- a/hack/prepare-for-integration-tests.sh +++ b/hack/prepare-for-integration-tests.sh @@ -347,7 +347,7 @@ manifest=/tmp/pinniped-concierge.yaml data_values_file=/tmp/concierge-values.yml concierge_app_name="pinniped-concierge" concierge_namespace="concierge" -webhook_url="https://local-user-authenticator.local-user-authenticator.svc/authenticate" +webhook_url="https://local-user-authenticator.local-user-authenticator.svc.cluster.local/authenticate" discovery_url="$(TERM=dumb kubectl cluster-info | awk '/master|control plane/ {print $NF}')" concierge_custom_labels="{myConciergeCustomLabelName: myConciergeCustomLabelValue}" log_level="debug" @@ -366,7 +366,7 @@ EOF if [[ "${FIREWALL_IDPS:-no}" == "yes" ]]; then # Configure the web proxy on the Concierge pods. Note that .svc and .cluster.local are not included, # so requests for things like pinniped-supervisor-clusterip.supervisor.svc.cluster.local and - # local-user-authenticator.local-user-authenticator.svc will go through the web proxy. + # local-user-authenticator.local-user-authenticator.svc.cluster.local will go through the web proxy. cat <>"$data_values_file" https_proxy: "http://proxy.tools.svc.cluster.local:3128" no_proxy: "\$(KUBERNETES_SERVICE_HOST),169.254.169.254,127.0.0.1,localhost" diff --git a/hack/prepare-impersonator-on-kind.sh b/hack/prepare-impersonator-on-kind.sh index df0355747..1dc1d13ff 100755 --- a/hack/prepare-impersonator-on-kind.sh +++ b/hack/prepare-impersonator-on-kind.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash -# Copyright 2021-2023 the Pinniped contributors. All Rights Reserved. +# Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 # @@ -50,7 +50,7 @@ kind: WebhookAuthenticator metadata: name: local-user-authenticator spec: - endpoint: https://local-user-authenticator.local-user-authenticator.svc/authenticate + endpoint: https://local-user-authenticator.local-user-authenticator.svc.cluster.local/authenticate tls: certificateAuthorityData: $LOCAL_USER_AUTHENTICATOR_CA EOF diff --git a/internal/controller/apicerts/certs_manager.go b/internal/controller/apicerts/certs_manager.go index c99d5162d..e462794e4 100644 --- a/internal/controller/apicerts/certs_manager.go +++ b/internal/controller/apicerts/certs_manager.go @@ -119,7 +119,8 @@ func (c *certsManagerController) Sync(ctx controllerlib.Context) error { // Using the CA from above, create a TLS server cert if we have service name. if len(c.serviceNameForGeneratedCertCommonName) != 0 { serviceEndpoint := c.serviceNameForGeneratedCertCommonName + "." + c.namespace + ".svc" - tlsCert, err := ca.IssueServerCert([]string{serviceEndpoint}, nil, c.certDuration) + // Allow clients to use either service-name.namespace.svc or service-name.namespace.svc.cluster.local to verify TLS. + tlsCert, err := ca.IssueServerCert([]string{serviceEndpoint, serviceEndpoint + ".cluster.local"}, nil, c.certDuration) if err != nil { return fmt.Errorf("could not issue serving certificate: %w", err) } diff --git a/internal/controller/apicerts/certs_manager_test.go b/internal/controller/apicerts/certs_manager_test.go index 9520f85f1..e51c618f3 100644 --- a/internal/controller/apicerts/certs_manager_test.go +++ b/internal/controller/apicerts/certs_manager_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package apicerts @@ -225,6 +225,7 @@ func TestManagerControllerSync(t *testing.T) { // Validate the created cert using the CA, and also validate the cert's hostname validCert := testutil.ValidateServerCertificate(t, actualCACert, actualCertChain) validCert.RequireDNSName("pinniped-api." + installedInNamespace + ".svc") + validCert.RequireDNSName("pinniped-api." + installedInNamespace + ".svc.cluster.local") validCert.RequireLifetime(time.Now(), time.Now().Add(certDuration), 6*time.Minute) validCert.RequireMatchesPrivateKey(actualPrivateKey) }) From dc195536d09dbb56401a64e6f6c73c0867d25cac Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 11 Oct 2024 14:49:46 -0700 Subject: [PATCH 06/10] also use port number when checking https proxy for WebhookAuthenticator --- .../webhookcachefiller/webhookcachefiller.go | 2 +- .../webhookcachefiller_test.go | 20 +++- internal/proxydetect/proxydetect.go | 10 +- internal/proxydetect/proxydetect_test.go | 99 ++++++++++++++++++- 4 files changed, 121 insertions(+), 10 deletions(-) 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) }) } } From 5c252fd08325d220326acc6f3ec7227a5a4d0687 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 14 Oct 2024 09:32:06 -0700 Subject: [PATCH 07/10] increase allowed delta in test to allow for slower CI workers --- test/integration/e2e_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go index dc6897df7..2d54abcd3 100644 --- a/test/integration/e2e_test.go +++ b/test/integration/e2e_test.go @@ -2196,7 +2196,8 @@ func requireUserCanUseKubectlWithoutAuthenticatingAgain( func requireGCAnnotationsOnSessionStorage(ctx context.Context, t *testing.T, supervisorNamespace string, startTime time.Time, token *oidctypes.Token) { // check that the access token is new (since it's just been refreshed) and has close to two minutes left. - testutil.RequireTimeInDelta(t, startTime.Add(2*time.Minute), token.AccessToken.Expiry.Time, 15*time.Second) + // Give some significant fudge factor to allow for slow CI workers. + testutil.RequireTimeInDelta(t, startTime.Add(2*time.Minute), token.AccessToken.Expiry.Time, 35*time.Second) kubeClient := testlib.NewKubernetesClientset(t).CoreV1() From c3afa55738184bf0c343bd0c2a8dd347c5aad50f Mon Sep 17 00:00:00 2001 From: Pinny Date: Mon, 14 Oct 2024 13:01:21 +0000 Subject: [PATCH 08/10] Bump dependencies --- go.mod | 4 ++-- go.sum | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index 688e8fe1d..2acbf9cdd 100644 --- a/go.mod +++ b/go.mod @@ -64,7 +64,7 @@ require ( github.com/spf13/cobra v1.8.1 github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.9.0 - github.com/tdewolff/minify/v2 v2.20.37 + github.com/tdewolff/minify/v2 v2.21.0 go.uber.org/mock v0.4.0 go.uber.org/zap v1.27.0 golang.org/x/crypto v0.28.0 @@ -162,7 +162,7 @@ require ( github.com/spf13/viper v1.16.0 // indirect github.com/stoewer/go-strcase v1.2.0 // indirect github.com/subosito/gotenv v1.4.2 // indirect - github.com/tdewolff/parse/v2 v2.7.15 // indirect + github.com/tdewolff/parse/v2 v2.7.17 // indirect github.com/x448/float16 v0.8.4 // indirect go.etcd.io/etcd/api/v3 v3.5.14 // indirect go.etcd.io/etcd/client/pkg/v3 v3.5.14 // indirect diff --git a/go.sum b/go.sum index deee538e2..75715cb37 100644 --- a/go.sum +++ b/go.sum @@ -560,10 +560,10 @@ github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsT github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= github.com/subosito/gotenv v1.4.2 h1:X1TuBLAMDFbaTAChgCBLu3DU3UPyELpnF2jjJ2cz/S8= github.com/subosito/gotenv v1.4.2/go.mod h1:ayKnFf/c6rvx/2iiLrJUk1e6plDbT3edrFNGqEflhK0= -github.com/tdewolff/minify/v2 v2.20.37 h1:Q97cx4STXCh1dlWDlNHZniE8BJ2EBL0+2b0n92BJQhw= -github.com/tdewolff/minify/v2 v2.20.37/go.mod h1:L1VYef/jwKw6Wwyk5A+T0mBjjn3mMPgmjjA688RNsxU= -github.com/tdewolff/parse/v2 v2.7.15 h1:hysDXtdGZIRF5UZXwpfn3ZWRbm+ru4l53/ajBRGpCTw= -github.com/tdewolff/parse/v2 v2.7.15/go.mod h1:3FbJWZp3XT9OWVN3Hmfp0p/a08v4h8J9W1aghka0soA= +github.com/tdewolff/minify/v2 v2.21.0 h1:nAPP1UVx0aK1xsQh/JiG3xyEnnqWw+agPstn+V6Pkto= +github.com/tdewolff/minify/v2 v2.21.0/go.mod h1:hGcthJ6Vj51NG+9QRIfN/DpWj5loHnY3bfhThzWWq08= +github.com/tdewolff/parse/v2 v2.7.17 h1:uC10p6DaQQORDy72eaIyD+AvAkaIUOouQ0nWp4uD0D0= +github.com/tdewolff/parse/v2 v2.7.17/go.mod h1:3FbJWZp3XT9OWVN3Hmfp0p/a08v4h8J9W1aghka0soA= github.com/tdewolff/test v1.0.11-0.20231101010635-f1265d231d52/go.mod h1:6DAvZliBAAnD7rhVgwaM7DE5/d9NMOAJ09SqYqeK4QE= github.com/tdewolff/test v1.0.11-0.20240106005702-7de5f7df4739 h1:IkjBCtQOOjIn03u/dMQK9g+Iw9ewps4mCl1nB8Sscbo= github.com/tdewolff/test v1.0.11-0.20240106005702-7de5f7df4739/go.mod h1:XPuWBzvdUzhCuxWO1ojpXsyzsA5bFoS3tO/Q3kFuTG8= From f36298c5427b8e03733080cee0567cda03a742e1 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 14 Oct 2024 11:12:34 -0700 Subject: [PATCH 09/10] use required headers for GitHub API connection probe request --- .../github_upstream_watcher.go | 15 +++++++++- .../github_upstream_watcher_test.go | 28 +++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go index 93b0cd1f0..56fd12146 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go @@ -63,6 +63,13 @@ const ( apiDotGithubDotCom = "api.github.com" githubDotCom = "github.com" + + headerNameAPIVersion = "X-GitHub-Api-Version" + headerValueAPIVersion2022 = "2022-11-28" + headerNameUserAgent = "User-Agent" + headerValueUserAgentConcierge = "pinniped-concierge/v1" + headerNameAccept = "Accept" + headerValueAcceptGitHubJSON = "application/vnd.github+json" ) // UpstreamGitHubIdentityProviderICache is a thread safe cache that holds a list of validated upstream GitHub IDP configurations. @@ -515,7 +522,7 @@ func (c *gitHubWatcherController) validateGitHubConnection( } // ProbeURL is the production code for how to probe a GitHub URL to test our connection to the GitHub API. -// It can be replaced via constructor injection for testing. +// It can be replaced via constructor injection for testing. Implements ProbeURLFunc. func ProbeURL(ctx context.Context, client *http.Client, url string) error { probeRequestCtx, probeRequestCancel := context.WithTimeout(ctx, 30*time.Second) defer probeRequestCancel() @@ -526,6 +533,12 @@ func ProbeURL(ctx context.Context, client *http.Client, url string) error { return err } + // GitHub API docs say that these headers are required for all requests. + // See https://docs.github.com/en/rest/using-the-rest-api/getting-started-with-the-rest-api?apiVersion=2022-11-28#headers. + probeRequest.Header.Set(headerNameAPIVersion, headerValueAPIVersion2022) + probeRequest.Header.Set(headerNameUserAgent, headerValueUserAgentConcierge) + probeRequest.Header.Set(headerNameAccept, headerValueAcceptGitHubJSON) + probeResponse, err := client.Do(probeRequest) if err != nil { return err diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go index c56f30c91..51b273dd0 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go @@ -81,6 +81,34 @@ func fakeGithubProbeFuncDisallowsAllProbes(t *testing.T) ProbeURLFunc { } } +// TestProbeURL tests the production version of the ProbeURLFunc type. +func TestProbeURL(t *testing.T) { + serverEndpointReached := false + testServer, testServerCA := tlsserver.TestServerIPv4(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, []string{"2022-11-28"}, r.Header.Values("X-GitHub-Api-Version")) + require.Equal(t, []string{"pinniped-concierge/v1"}, r.Header.Values("User-Agent")) + require.Equal(t, []string{"application/vnd.github+json"}, r.Header.Values("Accept")) + serverEndpointReached = true + }), nil) + + testServerCertPool := x509.NewCertPool() + testServerCertPool.AppendCertsFromPEM(testServerCA) + httpClient := phttp.Default(testServerCertPool) + + // Happy path. + err := ProbeURL(context.Background(), httpClient, testServer.URL) + require.NoError(t, err) + require.True(t, serverEndpointReached) + + // Invalid URL. + err = ProbeURL(context.Background(), httpClient, "https://invalid hostname") + require.EqualError(t, err, `parse "https://invalid hostname": invalid character " " in host name`) + + // Did not trust server's CA, as an example of a failed connection. + err = ProbeURL(context.Background(), phttp.Default(nil), testServer.URL) + require.EqualError(t, err, fmt.Sprintf("Get %q: tls: failed to verify certificate: x509: certificate signed by unknown authority", testServer.URL)) +} + func TestController(t *testing.T) { require.Equal(t, 6, countExpectedConditions) From c235239ee79f907a5300720daff42e8df451a469 Mon Sep 17 00:00:00 2001 From: Pinny Date: Tue, 15 Oct 2024 13:01:04 +0000 Subject: [PATCH 10/10] Bump dependencies --- go.mod | 4 ++-- go.sum | 9 ++++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/go.mod b/go.mod index 2acbf9cdd..8b3743df3 100644 --- a/go.mod +++ b/go.mod @@ -33,8 +33,8 @@ replace github.com/hashicorp/go-retryablehttp => github.com/hashicorp/go-retryab require ( github.com/MakeNowJust/heredoc/v2 v2.0.1 - github.com/chromedp/cdproto v0.0.0-20241003230502-a4a8f7c660df - github.com/chromedp/chromedp v0.10.0 + github.com/chromedp/cdproto v0.0.0-20241014181340-cb3a7a1d51d7 + github.com/chromedp/chromedp v0.11.0 github.com/coreos/go-oidc/v3 v3.11.0 github.com/coreos/go-semver v0.3.1 github.com/creack/pty v1.1.23 diff --git a/go.sum b/go.sum index 75715cb37..a3edc8ffa 100644 --- a/go.sum +++ b/go.sum @@ -64,11 +64,11 @@ github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA github.com/cespare/xxhash/v2 v2.1.1/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= -github.com/chromedp/cdproto v0.0.0-20240801214329-3f85d328b335/go.mod h1:GKljq0VrfU4D5yc+2qA6OVr8pmO/MBbPEWqWQ/oqGEs= -github.com/chromedp/cdproto v0.0.0-20241003230502-a4a8f7c660df h1:cbtSn19AtqQha1cxmP2Qvgd3fFMz51AeAEKLJMyEUhc= github.com/chromedp/cdproto v0.0.0-20241003230502-a4a8f7c660df/go.mod h1:GKljq0VrfU4D5yc+2qA6OVr8pmO/MBbPEWqWQ/oqGEs= -github.com/chromedp/chromedp v0.10.0 h1:bRclRYVpMm/UVD76+1HcRW9eV3l58rFfy7AdBvKab1E= -github.com/chromedp/chromedp v0.10.0/go.mod h1:ei/1ncZIqXX1YnAYDkxhD4gzBgavMEUu7JCKvztdomE= +github.com/chromedp/cdproto v0.0.0-20241014181340-cb3a7a1d51d7 h1:VDBgUGgdCBw9lTKwp0KPExhnqmGfGVJQTER2MehoICk= +github.com/chromedp/cdproto v0.0.0-20241014181340-cb3a7a1d51d7/go.mod h1:GKljq0VrfU4D5yc+2qA6OVr8pmO/MBbPEWqWQ/oqGEs= +github.com/chromedp/chromedp v0.11.0 h1:1PT6O4g39sBAFjlljIHTpxmCSk8meeYL6+R+oXH4bWA= +github.com/chromedp/chromedp v0.11.0/go.mod h1:jsD7OHrX0Qmskqb5Y4fn4jHnqquqW22rkMFgKbECsqg= github.com/chromedp/sysutil v1.0.0 h1:+ZxhTpfpZlmchB58ih/LBHX52ky7w2VhQVKQMucy3Ic= github.com/chromedp/sysutil v1.0.0/go.mod h1:kgWmDdq8fTzXYcKIBqIYvRRTnYb9aNS9moAV0xufSww= github.com/chzyer/logex v1.1.10/go.mod h1:+Ywpsq7O8HXn0nuIou7OrIPyXbp3wmkHB+jjWRnGsAI= @@ -851,7 +851,6 @@ golang.org/x/sys v0.7.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/sys v0.18.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -golang.org/x/sys v0.22.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/sys v0.26.0 h1:KHjCJyddX0LoSTb3J+vWpupP9p0oznkqVk/IfjymZbo= golang.org/x/sys v0.26.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/term v0.0.0-20201117132131-f5c789dd3221/go.mod h1:Nr5EML6q2oocZ2LXRh80K7BxOlk5/8JxuGnuhpl+muw=