diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go index 80915ae9f..2ad717a9f 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go @@ -64,18 +64,16 @@ func New( webhooks authinformers.WebhookAuthenticatorInformer, clock clock.Clock, log plog.Logger, - tlsDialerFunc func(network string, addr string, config *tls.Config) (*tls.Conn, error), ) controllerlib.Controller { return controllerlib.New( controllerlib.Config{ Name: controllerName, Syncer: &webhookCacheFillerController{ - cache: cache, - client: client, - webhooks: webhooks, - clock: clock, - log: log.WithName(controllerName), - tlsDialerFunc: tlsDialerFunc, + cache: cache, + client: client, + webhooks: webhooks, + clock: clock, + log: log.WithName(controllerName), }, }, controllerlib.WithInformer( @@ -87,12 +85,11 @@ func New( } type webhookCacheFillerController struct { - cache *authncache.Cache - webhooks authinformers.WebhookAuthenticatorInformer - client conciergeclientset.Interface - clock clock.Clock - log plog.Logger - tlsDialerFunc func(network string, addr string, config *tls.Config) (*tls.Conn, error) // for mocking, use tls.Dial in prod + cache *authncache.Cache + webhooks authinformers.WebhookAuthenticatorInformer + client conciergeclientset.Interface + clock clock.Clock + log plog.Logger } // Sync implements controllerlib.Syncer. @@ -237,7 +234,7 @@ func (c *webhookCacheFillerController) validateConnection(certPool *x509.CertPoo return conditions, nil } - conn, err := c.tlsDialerFunc("tcp", endpointHostPort.Endpoint(), ptls.Default(certPool)) + conn, err := tls.Dial("tcp", endpointHostPort.Endpoint(), ptls.Default(certPool)) if err != nil { errText := "cannot dial server" diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go index 67038f2df..77a95df0d 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go @@ -19,8 +19,6 @@ import ( "testing" "time" - "github.com/google/go-cmp/cmp" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" authenticationv1beta1 "k8s.io/api/authentication/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -28,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" coretesting "k8s.io/client-go/testing" clocktesting "k8s.io/utils/clock/testing" + "k8s.io/utils/ptr" auth1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/authentication/v1alpha1" pinnipedfake "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned/fake" @@ -107,6 +106,8 @@ func TestController(t *testing.T) { s.TLS.Certificates = []tls.Certificate{*localButExampleDotComServerCert} }) + hostLocalIPv6Server := tlsserver.TLSTestIPv6Server(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}), tlsserver.RecordTLSHello) + mux := http.NewServeMux() mux.Handle("/nothing/here", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // note that we are only dialing, so we shouldn't actually get here @@ -358,7 +359,6 @@ func TestController(t *testing.T) { webhooks []runtime.Object // for modifying the clients to hack in arbitrary api responses configClient func(*pinnipedfake.Clientset) - tlsDialerFunc func(network string, addr string, config *tls.Config) (*tls.Conn, error) wantSyncLoopErr testutil.RequireErrorStringFunc wantLogs []map[string]any wantActions func() []coretesting.Action @@ -521,6 +521,63 @@ func TestController(t *testing.T) { }, wantCacheEntries: 1, }, + { + name: "Sync: valid WebhookAuthenticator with IPV6 and CA: will complete sync loop successfully with success conditions and ready phase", + syncKey: controllerlib.Key{Name: "test-name"}, + webhooks: []runtime.Object{ + &auth1alpha1.WebhookAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-name", + }, + Spec: func() auth1alpha1.WebhookAuthenticatorSpec { + ipv6 := goodWebhookAuthenticatorSpecWithCA.DeepCopy() + ipv6.Endpoint = hostLocalIPv6Server.URL + ipv6.TLS = ptr.To(auth1alpha1.TLSSpec{ + CertificateAuthorityData: base64.StdEncoding.EncodeToString(tlsserver.TLSTestServerCA(hostLocalIPv6Server)), + }) + return *ipv6 + }(), + }, + }, + wantLogs: []map[string]any{ + { + "level": "info", + "timestamp": "2099-08-08T13:57:36.123456Z", + "logger": "webhookcachefiller-controller", + "message": "added new webhook authenticator", + "endpoint": hostLocalIPv6Server.URL, + "webhook": map[string]interface{}{ + "name": "test-name", + }, + }, + }, + wantActions: func() []coretesting.Action { + updateStatusAction := coretesting.NewUpdateAction(webhookAuthenticatorGVR, "", &auth1alpha1.WebhookAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-name", + }, + Spec: func() auth1alpha1.WebhookAuthenticatorSpec { + ipv6 := goodWebhookAuthenticatorSpecWithCA.DeepCopy() + ipv6.Endpoint = hostLocalIPv6Server.URL + ipv6.TLS = ptr.To(auth1alpha1.TLSSpec{ + CertificateAuthorityData: base64.StdEncoding.EncodeToString(tlsserver.TLSTestServerCA(hostLocalIPv6Server)), + }) + return *ipv6 + }(), + Status: auth1alpha1.WebhookAuthenticatorStatus{ + Conditions: allHappyConditionsSuccess(hostLocalIPv6Server.URL, frozenMetav1Now, 0), + Phase: "Ready", + }, + }) + updateStatusAction.Subresource = "status" + return []coretesting.Action{ + coretesting.NewListAction(webhookAuthenticatorGVR, webhookAuthenticatorGVK, "", metav1.ListOptions{}), + coretesting.NewWatchAction(webhookAuthenticatorGVR, "", metav1.ListOptions{}), + updateStatusAction, + } + }, + wantCacheEntries: 1, + }, { name: "Sync: valid WebhookAuthenticator without CA: loop will fail to cache the authenticator, will write failed and unknown status conditions, and will enqueue resync", syncKey: controllerlib.Key{Name: "test-name"}, @@ -691,9 +748,6 @@ func TestController(t *testing.T) { { name: "validateEndpoint: should error if endpoint cannot be parsed", syncKey: controllerlib.Key{Name: "test-name"}, - tlsDialerFunc: func(network string, addr string, config *tls.Config) (*tls.Conn, error) { - return nil, errors.New("IPv6 test fake error") - }, webhooks: []runtime.Object{ &auth1alpha1.WebhookAuthenticator{ ObjectMeta: metav1.ObjectMeta{ @@ -869,16 +923,6 @@ func TestController(t *testing.T) { { name: "validateConnection: IPv6 address with port: should call dialer func with correct arguments", syncKey: controllerlib.Key{Name: "test-name"}, - tlsDialerFunc: func(network string, addr string, config *tls.Config) (*tls.Conn, error) { - assert.Equal(t, "tcp", network) - assert.Equal(t, "[0:0:0:0:0:0:0:1]:4242", addr) - defaultTLSConfig := ptls.Default(nil) - assert.True(t, caForLocalhostAs127001.Pool().Equal(config.RootCAs)) - assert.Equal(t, defaultTLSConfig.MinVersion, config.MinVersion) - assert.Equal(t, defaultTLSConfig.CipherSuites, config.CipherSuites) - assert.Equal(t, defaultTLSConfig.NextProtos, config.NextProtos) - return nil, errors.New("IPv6 fake dial error") - }, webhooks: []runtime.Object{ &auth1alpha1.WebhookAuthenticator{ ObjectMeta: metav1.ObjectMeta{ @@ -907,7 +951,7 @@ func TestController(t *testing.T) { Conditions: conditionstestutil.Replace( allHappyConditionsSuccess("https://[0:0:0:0:0:0:0:1]:4242/some/fake/path", frozenMetav1Now, 0), []metav1.Condition{ - sadWebhookConnectionValidWithMessage(frozenMetav1Now, 0, "cannot dial server: IPv6 fake dial error"), + sadWebhookConnectionValidWithMessage(frozenMetav1Now, 0, "cannot dial server: dial tcp [::1]:4242: connect: connection refused"), sadReadyCondition(frozenMetav1Now, 0), unknownAuthenticatorValid(frozenMetav1Now, 0), }, @@ -922,22 +966,12 @@ func TestController(t *testing.T) { updateStatusAction, } }, - wantSyncLoopErr: testutil.WantExactErrorString(`cannot dial server: IPv6 fake dial error`), + wantSyncLoopErr: testutil.WantExactErrorString(`cannot dial server: dial tcp [::1]:4242: connect: connection refused`), wantCacheEntries: 0, }, { name: "validateConnection: IPv6 address without port: should call dialer func with correct arguments", syncKey: controllerlib.Key{Name: "test-name"}, - tlsDialerFunc: func(network string, addr string, config *tls.Config) (*tls.Conn, error) { - assert.Equal(t, "tcp", network) - assert.Equal(t, "[0:0:0:0:0:0:0:1]:443", addr, "should add default port when port not provided") - defaultTLSConfig := ptls.Default(nil) - assert.True(t, caForLocalhostAs127001.Pool().Equal(config.RootCAs)) - assert.Equal(t, defaultTLSConfig.MinVersion, config.MinVersion) - assert.Equal(t, defaultTLSConfig.CipherSuites, config.CipherSuites) - assert.Equal(t, defaultTLSConfig.NextProtos, config.NextProtos) - return nil, errors.New("IPv6 fake dial error") - }, webhooks: []runtime.Object{ &auth1alpha1.WebhookAuthenticator{ ObjectMeta: metav1.ObjectMeta{ @@ -966,7 +1000,7 @@ func TestController(t *testing.T) { Conditions: conditionstestutil.Replace( allHappyConditionsSuccess("https://[0:0:0:0:0:0:0:1]/some/fake/path", frozenMetav1Now, 0), []metav1.Condition{ - sadWebhookConnectionValidWithMessage(frozenMetav1Now, 0, "cannot dial server: IPv6 fake dial error"), + sadWebhookConnectionValidWithMessage(frozenMetav1Now, 0, "cannot dial server: dial tcp [::1]:443: connect: connection refused"), sadReadyCondition(frozenMetav1Now, 0), unknownAuthenticatorValid(frozenMetav1Now, 0), }, @@ -981,7 +1015,7 @@ func TestController(t *testing.T) { updateStatusAction, } }, - wantSyncLoopErr: testutil.WantExactErrorString(`cannot dial server: IPv6 fake dial error`), + wantSyncLoopErr: testutil.WantExactErrorString(`cannot dial server: dial tcp [::1]:443: connect: connection refused`), wantCacheEntries: 0, }, { @@ -1070,16 +1104,6 @@ func TestController(t *testing.T) { { name: "validateConnection: IPv6 address without port or brackets: should succeed since IPv6 brackets are optional without port", syncKey: controllerlib.Key{Name: "test-name"}, - tlsDialerFunc: func(network string, addr string, config *tls.Config) (*tls.Conn, error) { - assert.Equal(t, "tcp", network) - assert.Equal(t, "[0:0:0:0:0:0:0:1]:443", addr) - defaultTLSConfig := ptls.Default(nil) - assert.True(t, caForLocalhostAs127001.Pool().Equal(config.RootCAs)) - assert.Equal(t, defaultTLSConfig.MinVersion, config.MinVersion) - assert.Equal(t, defaultTLSConfig.CipherSuites, config.CipherSuites) - assert.Equal(t, defaultTLSConfig.NextProtos, config.NextProtos) - return nil, errors.New("IPv6 fake dial error") - }, webhooks: []runtime.Object{ &auth1alpha1.WebhookAuthenticator{ ObjectMeta: metav1.ObjectMeta{ @@ -1108,7 +1132,7 @@ func TestController(t *testing.T) { Conditions: conditionstestutil.Replace( allHappyConditionsSuccess("https://0:0:0:0:0:0:0:1/some/fake/path", frozenMetav1Now, 0), []metav1.Condition{ - sadWebhookConnectionValidWithMessage(frozenMetav1Now, 0, "cannot dial server: IPv6 fake dial error"), + sadWebhookConnectionValidWithMessage(frozenMetav1Now, 0, "cannot dial server: dial tcp [::1]:443: connect: connection refused"), sadReadyCondition(frozenMetav1Now, 0), unknownAuthenticatorValid(frozenMetav1Now, 0), }, @@ -1123,7 +1147,7 @@ func TestController(t *testing.T) { updateStatusAction, } }, - wantSyncLoopErr: testutil.WantExactErrorString(`cannot dial server: IPv6 fake dial error`), + wantSyncLoopErr: testutil.WantExactErrorString(`cannot dial server: dial tcp [::1]:443: connect: connection refused`), wantCacheEntries: 0, }, { @@ -1291,16 +1315,12 @@ func TestController(t *testing.T) { var log bytes.Buffer logger := plog.TestLogger(t, &log) - if tt.tlsDialerFunc == nil { - tt.tlsDialerFunc = tls.Dial - } controller := New( cache, pinnipedAPIClient, informers.Authentication().V1alpha1().WebhookAuthenticators(), frozenClock, - logger, - tt.tlsDialerFunc) + logger) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -1337,17 +1357,8 @@ func TestController(t *testing.T) { } } - if tt.wantActions != nil { - if !assert.ElementsMatch(t, tt.wantActions(), pinnipedAPIClient.Actions()) { - // cmp.Diff is superior to require.ElementsMatch in terms of readability here. - // require.ElementsMatch will handle pointers better than require.Equal, but - // the timestamps are still incredibly verbose. - require.Fail(t, cmp.Diff(tt.wantActions(), pinnipedAPIClient.Actions()), "actions should be exactly the expected number of actions and also contain the correct resources") - } - } else { - require.Fail(t, "wantActions is required for test "+tt.name) - } - + require.NotEmpty(t, tt.wantActions, "wantActions is required for test %s", tt.name) + require.Equal(t, tt.wantActions(), pinnipedAPIClient.Actions()) require.Equal(t, tt.wantCacheEntries, len(cache.Keys()), fmt.Sprintf("expected cache entries is incorrect. wanted:%d, got: %d, keys: %v", tt.wantCacheEntries, len(cache.Keys()), cache.Keys())) }) } diff --git a/internal/controllermanager/prepare_controllers.go b/internal/controllermanager/prepare_controllers.go index 3a65c7caf..379db68a4 100644 --- a/internal/controllermanager/prepare_controllers.go +++ b/internal/controllermanager/prepare_controllers.go @@ -6,7 +6,6 @@ package controllermanager import ( - "crypto/tls" "fmt" "time" @@ -241,7 +240,6 @@ func PrepareControllers(c *Config) (controllerinit.RunnerBuilder, error) { //nol informers.pinniped.Authentication().V1alpha1().WebhookAuthenticators(), clock.RealClock{}, plog.New(), - tls.Dial, ), singletonWorker, ). diff --git a/internal/testutil/tlsserver/tlsserver.go b/internal/testutil/tlsserver/tlsserver.go index 9669764d4..0c3e2851d 100644 --- a/internal/testutil/tlsserver/tlsserver.go +++ b/internal/testutil/tlsserver/tlsserver.go @@ -30,10 +30,34 @@ const ( helloKey ) +// TLSTestIPv6Server returns a TLS-required server that listens at an IPv6 loopback +// TODO: Rename since the package name already includes TLS. +func TLSTestIPv6Server(t *testing.T, handler http.Handler, f func(*httptest.Server)) *httptest.Server { + t.Helper() + + listener, err := net.Listen("tcp6", "[::1]:0") + require.NoError(t, err, "TLSTestIPv6Server: failed to listen on a port") + + server := &httptest.Server{ + Listener: listener, + Config: &http.Server{Handler: handler}, //nolint:gosec //ReadHeaderTimeout is not needed for a localhost listener + } + + return testServer(t, server, f) +} + +// TLSTestServer returns a TLS-required server that listens at an IPv4 loopback. +// TODO: Rename since the package name already includes TLS. func TLSTestServer(t *testing.T, handler http.Handler, f func(*httptest.Server)) *httptest.Server { t.Helper() server := httptest.NewUnstartedServer(handler) + return testServer(t, server, f) +} + +func testServer(t *testing.T, server *httptest.Server, f func(*httptest.Server)) *httptest.Server { + t.Helper() + server.TLS = ptls.Default(nil) // mimic API server config if f != nil { f(server)