From 5c0d67dc507da138cfd37e7f172cbe142dce5baf Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Tue, 19 Mar 2024 12:41:36 -0400 Subject: [PATCH] refactor WebhookAuthenticator newWebhookAuthenticator func --- .../webhookcachefiller/webhookcachefiller.go | 48 +++++--------- .../webhookcachefiller_test.go | 64 +++++-------------- 2 files changed, 31 insertions(+), 81 deletions(-) diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go index 592e04e9b..f5f8a3123 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go @@ -17,7 +17,7 @@ import ( "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" errorsutil "k8s.io/apimachinery/pkg/util/errors" - "k8s.io/apimachinery/pkg/util/net" + k8snetutil "k8s.io/apimachinery/pkg/util/net" "k8s.io/apiserver/pkg/authentication/authenticator" webhookutil "k8s.io/apiserver/pkg/util/webhook" "k8s.io/apiserver/plugin/pkg/authenticator/token/webhook" @@ -112,7 +112,7 @@ func (c *webhookCacheFillerController) Sync(ctx controllerlib.Context) error { specCopy := obj.Spec.DeepCopy() var errs []error - certPool, conditions, tlsBundleOk := c.validateTLSBundle(specCopy.TLS, conditions) + certPool, pemBytes, conditions, tlsBundleOk := c.validateTLSBundle(specCopy.TLS, conditions) endpointURL, conditions, endpointOk := c.validateEndpoint(specCopy.Endpoint, conditions) okSoFar := tlsBundleOk && endpointOk conditions, tlsNegotiateErr := c.validateTLSNegotiation(certPool, endpointURL, conditions, okSoFar) @@ -120,7 +120,8 @@ func (c *webhookCacheFillerController) Sync(ctx controllerlib.Context) error { okSoFar = okSoFar && tlsNegotiateErr == nil webhookAuthenticator, conditions, err := newWebhookAuthenticator( - &obj.Spec, + specCopy.Endpoint, + pemBytes, os.CreateTemp, clientcmd.WriteToFile, conditions, @@ -151,7 +152,8 @@ func (c *webhookCacheFillerController) Sync(ctx controllerlib.Context) error { // newWebhookAuthenticator creates a webhook from the provided API server url and caBundle // used to validate TLS connections. func newWebhookAuthenticator( - spec *auth1alpha1.WebhookAuthenticatorSpec, + endpoint string, + pemBytes []byte, tempfileFunc func(string, string) (*os.File, error), marshalFunc func(clientcmdapi.Config, string) error, conditions []*metav1.Condition, @@ -180,19 +182,8 @@ func newWebhookAuthenticator( } defer func() { _ = os.Remove(temp.Name()) }() - cluster := &clientcmdapi.Cluster{Server: spec.Endpoint} - _, cluster.CertificateAuthorityData, err = pinnipedauthenticator.CABundle(spec.TLS) - if err != nil { - errText := "invalid TLS configuration" - msg := fmt.Sprintf("%s: %s", errText, err.Error()) - conditions = append(conditions, &metav1.Condition{ - Type: typeAuthenticatorValid, - Status: metav1.ConditionFalse, - Reason: reasonInvalidTLSConfiguration, - Message: msg, - }) - return nil, conditions, fmt.Errorf("%s: %w", errText, err) - } + cluster := &clientcmdapi.Cluster{Server: endpoint} + cluster.CertificateAuthorityData = pemBytes kubeconfig := clientcmdapi.NewConfig() kubeconfig.Clusters["anonymous-cluster"] = cluster @@ -222,7 +213,7 @@ func newWebhookAuthenticator( // We set this to nil because we would only need this to support some of the // custom proxy stuff used by the API server. - var customDial net.DialFunc + var customDial k8snetutil.DialFunc // TODO refactor this code to directly construct the rest.Config // ideally we would keep rest config generation contained to the kubeclient package @@ -242,6 +233,7 @@ func newWebhookAuthenticator( // then use client.JSONConfig as clientConfig clientConfig, err := webhookutil.LoadKubeconfig(temp.Name(), customDial) if err != nil { + // no unit test for this failure. errText := "unable to load kubeconfig" msg := fmt.Sprintf("%s: %s", errText, err.Error()) conditions = append(conditions, &metav1.Condition{ @@ -258,6 +250,7 @@ func newWebhookAuthenticator( // NOTE: looks like the above was merged on Mar 18, 2022 webhookA, err := webhook.New(clientConfig, version, implicitAuds, *webhook.DefaultRetryBackoff()) if err != nil { + // no unit test for this failure. errText := "unable to instantiate webhook" msg := fmt.Sprintf("%s: %s", errText, err.Error()) conditions = append(conditions, &metav1.Condition{ @@ -330,8 +323,8 @@ func (c *webhookCacheFillerController) validateTLSNegotiation(certPool *x509.Cer return conditions, nil } -func (c *webhookCacheFillerController) validateTLSBundle(tlsSpec *auth1alpha1.TLSSpec, conditions []*metav1.Condition) (*x509.CertPool, []*metav1.Condition, bool) { - rootCAs, _, err := pinnipedauthenticator.CABundle(tlsSpec) +func (c *webhookCacheFillerController) validateTLSBundle(tlsSpec *auth1alpha1.TLSSpec, conditions []*metav1.Condition) (*x509.CertPool, []byte, []*metav1.Condition, bool) { + rootCAs, pemBytes, err := pinnipedauthenticator.CABundle(tlsSpec) if err != nil { msg := fmt.Sprintf("%s: %s", "invalid TLS configuration", err.Error()) conditions = append(conditions, &metav1.Condition{ @@ -340,7 +333,7 @@ func (c *webhookCacheFillerController) validateTLSBundle(tlsSpec *auth1alpha1.TL Reason: reasonInvalidTLSConfiguration, Message: msg, }) - return rootCAs, conditions, false + return rootCAs, pemBytes, conditions, false } msg := "successfully parsed specified CA bundle" if rootCAs == nil { @@ -352,7 +345,7 @@ func (c *webhookCacheFillerController) validateTLSBundle(tlsSpec *auth1alpha1.TL Reason: reasonSuccess, Message: msg, }) - return rootCAs, conditions, true + return rootCAs, pemBytes, conditions, true } func (c *webhookCacheFillerController) validateEndpoint(endpoint string, conditions []*metav1.Condition) (*url.URL, []*metav1.Condition, bool) { @@ -395,7 +388,7 @@ func (c *webhookCacheFillerController) updateStatus( ) error { updated := original.DeepCopy() - if hadErrorCondition(conditions) { + if conditionsutil.HadErrorCondition(conditions) { updated.Status.Phase = auth1alpha1.WebhookAuthenticatorPhaseError conditions = append(conditions, &metav1.Condition{ Type: typeReady, @@ -431,12 +424,3 @@ func (c *webhookCacheFillerController) updateStatus( } return err } - -func hadErrorCondition(conditions []*metav1.Condition) bool { - for _, c := range conditions { - if c.Status != metav1.ConditionTrue { - return true - } - } - return false -} diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go index 4cef02969..a786e2f39 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go @@ -395,9 +395,6 @@ func TestController(t *testing.T) { }, wantCacheEntries: 0, }, - // Existing code that was never tested. We would likely have to create a server with bad clients to - // simulate this. - // { name: "non-404 `failed to get webhook authenticator` for other API server reasons" } { name: "Sync: valid and unchanged WebhookAuthenticator: loop will preserve existing status conditions", syncKey: controllerlib.Key{Name: "test-name"}, @@ -443,10 +440,10 @@ func TestController(t *testing.T) { Spec: goodWebhookAuthenticatorSpecWithCA, Status: auth1alpha1.WebhookAuthenticatorStatus{ Conditions: conditionstestutil.Replace( - allHappyConditionsSuccess(goodEndpoint, frozenMetav1Now, 0), + allHappyConditionsSuccess(goodEndpoint, frozenMetav1Now, 666), []metav1.Condition{ - sadReadyCondition(frozenTimeInThePast, 0), - happyEndpointURLValid(frozenTimeInThePast, 0), + sadReadyCondition(frozenTimeInThePast, 777), + happyEndpointURLValid(frozenTimeInThePast, 888), }, ), Phase: "Ready", @@ -1119,7 +1116,7 @@ func TestController(t *testing.T) { 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.Error(t, errors.New("wantActions is required for test "+tt.name)) + require.Fail(t, "wantActions is required for test "+tt.name) } 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())) @@ -1132,7 +1129,7 @@ func TestNewWebhookAuthenticator(t *testing.T) { t.Run("prerequisites not ready, cannot create webhook authenticator", func(t *testing.T) { conditions := []*metav1.Condition{} - res, conditions, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{}, os.CreateTemp, clientcmd.WriteToFile, conditions, false) + res, conditions, err := newWebhookAuthenticator("", []byte("some pem bytes"), os.CreateTemp, clientcmd.WriteToFile, conditions, false) require.Equal(t, []*metav1.Condition{ { Type: "AuthenticatorValid", @@ -1142,13 +1139,13 @@ func TestNewWebhookAuthenticator(t *testing.T) { }, }, conditions) require.Nil(t, res) - require.Nil(t, err) + require.NoError(t, err) }) t.Run("temp file failure, cannot create webhook authenticator", func(t *testing.T) { brokenTempFile := func(_ string, _ string) (*os.File, error) { return nil, fmt.Errorf("some temp file error") } conditions := []*metav1.Condition{} - res, conditions, err := newWebhookAuthenticator(nil, brokenTempFile, clientcmd.WriteToFile, conditions, true) + res, conditions, err := newWebhookAuthenticator("", []byte("some pem bytes"), brokenTempFile, clientcmd.WriteToFile, conditions, true) require.Equal(t, []*metav1.Condition{ { Type: "AuthenticatorValid", @@ -1164,7 +1161,7 @@ func TestNewWebhookAuthenticator(t *testing.T) { t.Run("marshal failure, cannot create webhook authenticator", func(t *testing.T) { marshalError := func(_ clientcmdapi.Config, _ string) error { return fmt.Errorf("some marshal error") } conditions := []*metav1.Condition{} - res, conditions, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{}, os.CreateTemp, marshalError, conditions, true) + res, conditions, err := newWebhookAuthenticator("", []byte("some pem bytes"), os.CreateTemp, marshalError, conditions, true) require.Equal(t, []*metav1.Condition{ { Type: "AuthenticatorValid", @@ -1177,49 +1174,24 @@ func TestNewWebhookAuthenticator(t *testing.T) { require.EqualError(t, err, "unable to marshal kubeconfig: some marshal error") }) - // t.Run("load kubeconfig err, not currently tested, may not be necessary to test?") - - t.Run("invalid TLS config, base64 encoding err, cannot create webhook authenticator", func(t *testing.T) { + t.Run("invalid pem data, unable to parse bytes as PEM block", func(t *testing.T) { conditions := []*metav1.Condition{} - res, conditions, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{ - Endpoint: goodEndpoint, - TLS: &auth1alpha1.TLSSpec{CertificateAuthorityData: "invalid-base64"}, - }, os.CreateTemp, clientcmd.WriteToFile, conditions, true) + res, conditions, err := newWebhookAuthenticator(goodEndpoint, []byte("invalid-bas64"), os.CreateTemp, clientcmd.WriteToFile, conditions, true) require.Equal(t, []*metav1.Condition{ { Type: "AuthenticatorValid", Status: "False", - Reason: "InvalidTLSConfiguration", - Message: "invalid TLS configuration: illegal base64 data at input byte 7", + Reason: "UnableToInstantiateWebhook", + Message: "unable to instantiate webhook: unable to load root certificates: unable to parse bytes as PEM block", }, }, conditions) require.Nil(t, res) - require.EqualError(t, err, "invalid TLS configuration: illegal base64 data at input byte 7") - }) - - t.Run("invalid pem data, cannot create webhook authenticator", func(t *testing.T) { - conditions := []*metav1.Condition{} - res, conditions, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{ - Endpoint: goodEndpoint, - TLS: &auth1alpha1.TLSSpec{CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte("bad data"))}, - }, os.CreateTemp, clientcmd.WriteToFile, conditions, true) - require.Equal(t, []*metav1.Condition{ - { - Type: "AuthenticatorValid", - Status: "False", - Reason: "InvalidTLSConfiguration", - Message: "invalid TLS configuration: certificateAuthorityData is not valid PEM: data does not contain any valid RSA or ECDSA certificates", - }, - }, conditions) - require.Nil(t, res) - require.EqualError(t, err, "invalid TLS configuration: certificateAuthorityData is not valid PEM: data does not contain any valid RSA or ECDSA certificates") + require.EqualError(t, err, "unable to instantiate webhook: unable to load root certificates: unable to parse bytes as PEM block") }) t.Run("valid config with no TLS spec, webhook authenticator created", func(t *testing.T) { conditions := []*metav1.Condition{} - res, conditions, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{ - Endpoint: goodEndpoint, - }, os.CreateTemp, clientcmd.WriteToFile, conditions, true) + res, conditions, err := newWebhookAuthenticator(goodEndpoint, nil, os.CreateTemp, clientcmd.WriteToFile, conditions, true) require.Equal(t, []*metav1.Condition{ { Type: "AuthenticatorValid", @@ -1240,14 +1212,8 @@ func TestNewWebhookAuthenticator(t *testing.T) { _, err = w.Write([]byte(`{}`)) require.NoError(t, err) }) - spec := &auth1alpha1.WebhookAuthenticatorSpec{ - Endpoint: url, - TLS: &auth1alpha1.TLSSpec{ - CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(caBundle)), - }, - } conditions := []*metav1.Condition{} - res, conditions, err := newWebhookAuthenticator(spec, os.CreateTemp, clientcmd.WriteToFile, conditions, true) + res, conditions, err := newWebhookAuthenticator(url, []byte(caBundle), os.CreateTemp, clientcmd.WriteToFile, conditions, true) require.NoError(t, err) require.NotNil(t, res) require.Equal(t, []*metav1.Condition{