refactor WebhookAuthenticator newWebhookAuthenticator func

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 := &clientcmdapi.Cluster{Server: endpoint}
-	_, cluster.CertificateAuthorityData, err = pinnipedauthenticator.CABundle(spec.TLS)
+	cluster.CertificateAuthorityData = pemBytes
-	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)
-	}
 
 	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) {
+func (c *webhookCacheFillerController) validateTLSBundle(tlsSpec *auth1alpha1.TLSSpec, conditions []*metav1.Condition) (*x509.CertPool, []byte, []*metav1.Condition, bool) {
-	rootCAs, _, err := pinnipedauthenticator.CABundle(tlsSpec)
+	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
-}

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),
+								sadReadyCondition(frozenTimeInThePast, 777),
-								happyEndpointURLValid(frozenTimeInThePast, 0),
+								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 pem data, unable to parse bytes as PEM block", func(t *testing.T) {
-
-	t.Run("invalid TLS config, base64 encoding err, cannot create webhook authenticator", func(t *testing.T) {
 		conditions := []*metav1.Condition{}
-		res, conditions, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{
+		res, conditions, err := newWebhookAuthenticator(goodEndpoint, []byte("invalid-bas64"), os.CreateTemp, clientcmd.WriteToFile, conditions, true)
-			Endpoint: goodEndpoint,
-			TLS:      &auth1alpha1.TLSSpec{CertificateAuthorityData: "invalid-base64"},
-		}, os.CreateTemp, clientcmd.WriteToFile, conditions, true)
 		require.Equal(t, []*metav1.Condition{
 			{
 				Type:    "AuthenticatorValid",
 				Status:  "False",
-				Reason:  "InvalidTLSConfiguration",
+				Reason:  "UnableToInstantiateWebhook",
-				Message: "invalid TLS configuration: illegal base64 data at input byte 7",
+				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")
+		require.EqualError(t, err, "unable to instantiate webhook: unable to load root certificates: unable to parse bytes as PEM block")
-	})
-
-	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")
 	})
 
 	t.Run("valid config with no TLS spec, webhook authenticator created", func(t *testing.T) {
 		conditions := []*metav1.Condition{}
-		res, conditions, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{
+		res, conditions, err := newWebhookAuthenticator(goodEndpoint, nil, os.CreateTemp, clientcmd.WriteToFile, conditions, true)
-			Endpoint: goodEndpoint,
-		}, 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{