Implement all optional methods in dynamic certs provider

Signed-off-by: Monis Khan <mok@vmware.com>

internal/controller/apicerts/certs_observer.go

@@ -57,12 +57,15 @@ func (c *certsObserverController) Sync(_ controllerlib.Context) error {
 	if notFound {
 		klog.Info("certsObserverController Sync found that the secret does not exist yet or was deleted")
 		// The secret does not exist yet or was deleted.
-		c.dynamicCertProvider.Set(nil, nil)
+		c.dynamicCertProvider.UnsetCertKeyContent()
 		return nil
 	}
 
 	// Mutate the in-memory cert provider to update with the latest cert values.
-	c.dynamicCertProvider.Set(certSecret.Data[TLSCertificateChainSecretKey], certSecret.Data[tlsPrivateKeySecretKey])
+	if err := c.dynamicCertProvider.SetCertKeyContent(certSecret.Data[TLSCertificateChainSecretKey], certSecret.Data[tlsPrivateKeySecretKey]); err != nil {
+		return fmt.Errorf("failed to set serving cert/key content from secret %s/%s: %w", c.namespace, c.certsSecretResourceName, err)
+	}
+
 	klog.Info("certsObserverController Sync updated certs in the dynamic cert provider")
 	return nil
 }

internal/controller/apicerts/certs_observer_test.go

@@ -5,7 +5,9 @@ package apicerts
 
 import (
 	"context"
+	"strings"
 	"testing"
+	"time"
 
 	"github.com/sclevine/spec"
 	"github.com/sclevine/spec/report"
@@ -94,6 +96,7 @@ func TestObserverControllerInformerFilters(t *testing.T) {
 }
 
 func TestObserverControllerSync(t *testing.T) {
+	name := t.Name()
 	spec.Run(t, "Sync", func(t *testing.T, when spec.G, it spec.S) {
 		const installedInNamespace = "some-namespace"
 		const certsSecretResourceName = "some-resource-name"
@@ -142,7 +145,7 @@ func TestObserverControllerSync(t *testing.T) {
 
 			kubeInformerClient = kubernetesfake.NewSimpleClientset()
 			kubeInformers = kubeinformers.NewSharedInformerFactory(kubeInformerClient, 0)
-			dynamicCertProvider = dynamiccert.New()
+			dynamicCertProvider = dynamiccert.New(name)
 		})
 
 		it.After(func() {
@@ -160,7 +163,14 @@ func TestObserverControllerSync(t *testing.T) {
 				err := kubeInformerClient.Tracker().Add(unrelatedSecret)
 				r.NoError(err)
 
-				dynamicCertProvider.Set([]byte("some cert"), []byte("some private key"))
+				crt, key, err := testutil.CreateCertificate(
+					time.Now().Add(-time.Hour),
+					time.Now().Add(time.Hour),
+				)
+				require.NoError(t, err)
+
+				err = dynamicCertProvider.SetCertKeyContent(crt, key)
+				r.NoError(err)
 			})
 
 			it("sets the dynamicCertProvider's cert and key to nil", func() {
@@ -176,6 +186,12 @@ func TestObserverControllerSync(t *testing.T) {
 
 		when("there is a serving cert Secret with the expected keys already in the installation namespace", func() {
 			it.Before(func() {
+				crt, key, err := testutil.CreateCertificate(
+					time.Now().Add(-time.Hour),
+					time.Now().Add(time.Hour),
+				)
+				require.NoError(t, err)
+
 				apiServingCertSecret := &corev1.Secret{
 					ObjectMeta: metav1.ObjectMeta{
 						Name:      certsSecretResourceName,
@@ -183,24 +199,29 @@ func TestObserverControllerSync(t *testing.T) {
 					},
 					Data: map[string][]byte{
 						"caCertificate":       []byte("fake cert"),
-						"tlsPrivateKey":       []byte("fake private key"),
-						"tlsCertificateChain": []byte("fake cert chain"),
+						"tlsPrivateKey":       key,
+						"tlsCertificateChain": crt,
 					},
 				}
-				err := kubeInformerClient.Tracker().Add(apiServingCertSecret)
+				err = kubeInformerClient.Tracker().Add(apiServingCertSecret)
 				r.NoError(err)
 
-				dynamicCertProvider.Set(nil, nil)
+				dynamicCertProvider.UnsetCertKeyContent()
 			})
 
 			it("updates the dynamicCertProvider's cert and key", func() {
 				startInformersAndController()
+
+				actualCertChain, actualKey := dynamicCertProvider.CurrentCertKeyContent()
+				r.Nil(actualCertChain)
+				r.Nil(actualKey)
+
 				err := controllerlib.TestSync(t, subject, *syncContext)
 				r.NoError(err)
 
-				actualCertChain, actualKey := dynamicCertProvider.CurrentCertKeyContent()
-				r.Equal("fake cert chain", string(actualCertChain))
-				r.Equal("fake private key", string(actualKey))
+				actualCertChain, actualKey = dynamicCertProvider.CurrentCertKeyContent()
+				r.True(strings.HasPrefix(string(actualCertChain), `-----BEGIN CERTIFICATE-----`), "not a cert:\n%s", string(actualCertChain))
+				r.True(strings.HasPrefix(string(actualKey), `-----BEGIN PRIVATE KEY-----`), "not a key:\n%s", string(actualKey))
 			})
 		})
 
@@ -216,17 +237,22 @@ func TestObserverControllerSync(t *testing.T) {
 				err := kubeInformerClient.Tracker().Add(apiServingCertSecret)
 				r.NoError(err)
 
-				dynamicCertProvider.Set(nil, nil)
+				dynamicCertProvider.UnsetCertKeyContent()
 			})
 
-			it("set the missing values in the dynamicCertProvider as nil", func() {
+			it("returns an error and does not change the dynamicCertProvider", func() {
 				startInformersAndController()
-				err := controllerlib.TestSync(t, subject, *syncContext)
-				r.NoError(err)
 
 				actualCertChain, actualKey := dynamicCertProvider.CurrentCertKeyContent()
 				r.Nil(actualCertChain)
 				r.Nil(actualKey)
+
+				err := controllerlib.TestSync(t, subject, *syncContext)
+				r.EqualError(err, "failed to set serving cert/key content from secret some-namespace/some-resource-name: TestObserverControllerSync: attempt to set invalid key pair: tls: failed to find any PEM data in certificate input")
+
+				actualCertChain, actualKey = dynamicCertProvider.CurrentCertKeyContent()
+				r.Nil(actualCertChain)
+				r.Nil(actualKey)
 			})
 		})
 	}, spec.Parallel(), spec.Report(report.Terminal{}))

internal/controller/impersonatorconfig/impersonator_config.go

@@ -117,7 +117,7 @@ func NewImpersonatorConfigController(
 				clock:                             clock,
 				impersonationSigningCertProvider:  impersonationSigningCertProvider,
 				impersonatorFunc:                  impersonatorFunc,
-				tlsServingCertDynamicCertProvider: dynamiccert.New(),
+				tlsServingCertDynamicCertProvider: dynamiccert.New("impersonation-proxy-serving-cert"),
 			},
 		},
 		withInformer(
@@ -238,7 +238,7 @@ func (c *impersonatorConfigController) doSync(syncCtx controllerlib.Context) (*v
 	nameInfo, err := c.findDesiredTLSCertificateName(config)
 	if err != nil {
 		// Unexpected error while determining the name that should go into the certs, so clear any existing certs.
-		c.tlsServingCertDynamicCertProvider.Set(nil, nil)
+		c.tlsServingCertDynamicCertProvider.UnsetCertKeyContent()
 		return nil, err
 	}
 
@@ -371,7 +371,7 @@ func (c *impersonatorConfigController) ensureImpersonatorIsStarted(syncCtx contr
 	startImpersonatorFunc, err := c.impersonatorFunc(
 		impersonationProxyPort,
 		c.tlsServingCertDynamicCertProvider,
-		dynamiccert.NewCAProvider(c.impersonationSigningCertProvider),
+		c.impersonationSigningCertProvider,
 	)
 	if err != nil {
 		return err
@@ -750,16 +750,17 @@ func (c *impersonatorConfigController) createNewTLSSecret(ctx context.Context, c
 func (c *impersonatorConfigController) loadTLSCertFromSecret(tlsSecret *v1.Secret) error {
 	certPEM := tlsSecret.Data[v1.TLSCertKey]
 	keyPEM := tlsSecret.Data[v1.TLSPrivateKeyKey]
-	_, err := tls.X509KeyPair(certPEM, keyPEM)
-	if err != nil {
-		c.tlsServingCertDynamicCertProvider.Set(nil, nil)
+
+	if err := c.tlsServingCertDynamicCertProvider.SetCertKeyContent(certPEM, keyPEM); err != nil {
+		c.tlsServingCertDynamicCertProvider.UnsetCertKeyContent()
 		return fmt.Errorf("could not parse TLS cert PEM data from Secret: %w", err)
 	}
+
 	plog.Info("Loading TLS certificates for impersonation proxy",
 		"certPEM", string(certPEM),
 		"secret", c.tlsSecretName,
 		"namespace", c.namespace)
-	c.tlsServingCertDynamicCertProvider.Set(certPEM, keyPEM)
+
 	return nil
 }
 
@@ -779,7 +780,7 @@ func (c *impersonatorConfigController) ensureTLSSecretIsRemoved(ctx context.Cont
 		return err
 	}
 
-	c.tlsServingCertDynamicCertProvider.Set(nil, nil)
+	c.tlsServingCertDynamicCertProvider.UnsetCertKeyContent()
 
 	return nil
 }
@@ -798,8 +799,8 @@ func (c *impersonatorConfigController) loadSignerCA(status v1alpha1.StrategyStat
 
 	certPEM := signingCertSecret.Data[apicerts.CACertificateSecretKey]
 	keyPEM := signingCertSecret.Data[apicerts.CACertificatePrivateKeySecretKey]
-	_, err = tls.X509KeyPair(certPEM, keyPEM)
-	if err != nil {
+
+	if err := c.impersonationSigningCertProvider.SetCertKeyContent(certPEM, keyPEM); err != nil {
 		return fmt.Errorf("could not load the impersonator's credential signing secret: %w", err)
 	}
 
@@ -807,13 +808,13 @@ func (c *impersonatorConfigController) loadSignerCA(status v1alpha1.StrategyStat
 		"certPEM", string(certPEM),
 		"fromSecret", c.impersonationSignerSecretName,
 		"namespace", c.namespace)
-	c.impersonationSigningCertProvider.Set(certPEM, keyPEM)
+
 	return nil
 }
 
 func (c *impersonatorConfigController) clearSignerCA() {
 	plog.Info("Clearing credential signing certificate for impersonation proxy")
-	c.impersonationSigningCertProvider.Set(nil, nil)
+	c.impersonationSigningCertProvider.UnsetCertKeyContent()
 }
 
 func (c *impersonatorConfigController) doSyncResult(nameInfo *certNameInfo, config *impersonator.Config, ca *certauthority.CA) *v1alpha1.CredentialIssuerStrategy {

internal/controller/impersonatorconfig/impersonator_config_test.go

@@ -30,7 +30,6 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/clock"
-	"k8s.io/apiserver/pkg/server/dynamiccertificates"
 	kubeinformers "k8s.io/client-go/informers"
 	kubernetesfake "k8s.io/client-go/kubernetes/fake"
 	coretesting "k8s.io/client-go/testing"
@@ -266,6 +265,7 @@ func TestImpersonatorConfigControllerOptions(t *testing.T) {
 }
 
 func TestImpersonatorConfigControllerSync(t *testing.T) {
+	name := t.Name()
 	spec.Run(t, "Sync", func(t *testing.T, when spec.G, it spec.S) {
 		const installedInNamespace = "some-namespace"
 		const configMapResourceName = "some-configmap-resource-name"
@@ -306,8 +306,8 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 
 		var impersonatorFunc = func(
 			port int,
-			dynamicCertProvider dynamiccertificates.CertKeyContentProvider,
-			impersonationProxySignerCAProvider dynamiccertificates.CAContentProvider,
+			dynamicCertProvider dynamiccert.Private,
+			impersonationProxySignerCAProvider dynamiccert.Public,
 		) (func(stopCh <-chan struct{}) error, error) {
 			impersonatorFuncWasCalled++
 			r.Equal(8444, port)
@@ -972,7 +972,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			kubeAPIClient = kubernetesfake.NewSimpleClientset()
 			pinnipedAPIClient = pinnipedfake.NewSimpleClientset()
 			frozenNow = time.Date(2021, time.March, 2, 7, 42, 0, 0, time.Local)
-			signingCertProvider = dynamiccert.New()
+			signingCertProvider = dynamiccert.New(name)
 
 			ca := newCA()
 			signingCACertPEM = ca.Bundle()
@@ -2408,7 +2408,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 
 				it("returns the error", func() {
 					startInformersAndController()
-					errString := `could not load the impersonator's credential signing secret: tls: failed to find any PEM data in certificate input`
+					errString := `could not load the impersonator's credential signing secret: TestImpersonatorConfigControllerSync: attempt to set invalid key pair: tls: failed to find any PEM data in certificate input`
 					r.EqualError(runControllerSync(), errString)
 					requireCredentialIssuer(newErrorStrategy(errString))
 					requireSigningCertProviderIsEmpty()
@@ -2423,7 +2423,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 
 				it("returns the error", func() {
 					startInformersAndController()
-					errString := `could not load the impersonator's credential signing secret: tls: failed to find any PEM data in certificate input`
+					errString := `could not load the impersonator's credential signing secret: TestImpersonatorConfigControllerSync: attempt to set invalid key pair: tls: failed to find any PEM data in certificate input`
 					r.EqualError(runControllerSync(), errString)
 					requireCredentialIssuer(newErrorStrategy(errString))
 					requireSigningCertProviderIsEmpty()
@@ -2459,7 +2459,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					addSecretToTrackers(updatedSigner, kubeInformerClient)
 					waitForObjectToAppearInInformer(updatedSigner, kubeInformers.Core().V1().Secrets())
 
-					errString := `could not load the impersonator's credential signing secret: tls: failed to find any PEM data in certificate input`
+					errString := `could not load the impersonator's credential signing secret: TestImpersonatorConfigControllerSync: attempt to set invalid key pair: tls: failed to find any PEM data in certificate input`
 					r.EqualError(runControllerSync(), errString)
 					requireCredentialIssuer(newErrorStrategy(errString))
 					requireSigningCertProviderIsEmpty()

internal/controller/kubecertagent/execer.go

@@ -11,9 +11,9 @@ import (
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/clock"
+	"k8s.io/apimachinery/pkg/util/errors"
 	corev1informers "k8s.io/client-go/informers/core/v1"
 	"k8s.io/client-go/tools/clientcmd"
-	"k8s.io/klog/v2"
 
 	configv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/config/v1alpha1"
 	pinnipedclientset "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned"
@@ -119,8 +119,7 @@ func (c *execerController) Sync(ctx controllerlib.Context) error {
 			c.pinnipedAPIClient,
 			strategyError(c.clock, err),
 		)
-		klog.ErrorS(strategyResultUpdateErr, "could not create or update CredentialIssuer with strategy success")
-		return err
+		return newAggregate(err, strategyResultUpdateErr)
 	}
 
 	keyPEM, err := c.podCommandExecutor.Exec(agentPod.Namespace, agentPod.Name, "cat", keyPath)
@@ -132,11 +131,20 @@ func (c *execerController) Sync(ctx controllerlib.Context) error {
 			c.pinnipedAPIClient,
 			strategyError(c.clock, err),
 		)
-		klog.ErrorS(strategyResultUpdateErr, "could not create or update CredentialIssuer with strategy success")
-		return err
+		return newAggregate(err, strategyResultUpdateErr)
 	}
 
-	c.dynamicCertProvider.Set([]byte(certPEM), []byte(keyPEM))
+	if err := c.dynamicCertProvider.SetCertKeyContent([]byte(certPEM), []byte(keyPEM)); err != nil {
+		err = fmt.Errorf("failed to set signing cert/key content from agent pod %s/%s: %w", agentPod.Namespace, agentPod.Name, err)
+		strategyResultUpdateErr := issuerconfig.UpdateStrategy(
+			ctx.Context,
+			c.credentialIssuerLocationConfig.Name,
+			c.credentialIssuerLabels,
+			c.pinnipedAPIClient,
+			strategyError(c.clock, err),
+		)
+		return newAggregate(err, strategyResultUpdateErr)
+	}
 
 	apiInfo, err := c.getTokenCredentialRequestAPIInfo()
 	if err != nil {
@@ -153,8 +161,7 @@ func (c *execerController) Sync(ctx controllerlib.Context) error {
 				LastUpdateTime: metav1.NewTime(c.clock.Now()),
 			},
 		)
-		klog.ErrorS(strategyResultUpdateErr, "could not create or update CredentialIssuer with strategy success")
-		return err
+		return newAggregate(err, strategyResultUpdateErr)
 	}
 
 	return issuerconfig.UpdateStrategy(
@@ -219,3 +226,7 @@ func (c *execerController) getKeypairFilePaths(pod *v1.Pod) (string, string) {
 
 	return certPath, keyPath
 }
+
+func newAggregate(errs ...error) error {
+	return errors.NewAggregate(errs)
+}

internal/controller/kubecertagent/execer_test.go

@@ -132,6 +132,7 @@ func (s *fakePodExecutor) Exec(podNamespace string, podName string, commandAndAr
 }
 
 func TestManagerControllerSync(t *testing.T) {
+	name := t.Name()
 	spec.Run(t, "Sync", func(t *testing.T, when spec.G, it spec.S) {
 		const agentPodNamespace = "some-namespace"
 		const agentPodName = "some-agent-pod-name-123"
@@ -139,8 +140,6 @@ func TestManagerControllerSync(t *testing.T) {
 		const keyPathAnnotationName = "kube-cert-agent.pinniped.dev/key-path"
 		const fakeCertPath = "/some/cert/path"
 		const fakeKeyPath = "/some/key/path"
-		const defaultDynamicCertProviderCert = "initial-cert"
-		const defaultDynamicCertProviderKey = "initial-key"
 		const credentialIssuerResourceName = "ci-resource-name"
 
 		var r *require.Assertions
@@ -159,6 +158,8 @@ func TestManagerControllerSync(t *testing.T) {
 		var fakeCertPEM, fakeKeyPEM string
 		var credentialIssuerGVR schema.GroupVersionResource
 		var frozenNow time.Time
+		var defaultDynamicCertProviderCert string
+		var defaultDynamicCertProviderKey string
 
 		// Defer starting the informers until the last possible moment so that the
 		// nested Before's can keep adding things to the informer caches.
@@ -228,14 +229,23 @@ func TestManagerControllerSync(t *testing.T) {
 		it.Before(func() {
 			r = require.New(t)
 
+			crt, key, err := testutil.CreateCertificate(
+				time.Now().Add(-time.Hour),
+				time.Now().Add(time.Hour),
+			)
+			require.NoError(t, err)
+			defaultDynamicCertProviderCert = string(crt)
+			defaultDynamicCertProviderKey = string(key)
+
 			cancelContext, cancelContextCancelFunc = context.WithCancel(context.Background())
 			pinnipedAPIClient = pinnipedfake.NewSimpleClientset()
 			kubeClientset = kubernetesfake.NewSimpleClientset()
 			kubeInformerFactory = kubeinformers.NewSharedInformerFactory(kubeClientset, 0)
 			fakeExecutor = &fakePodExecutor{r: r}
 			frozenNow = time.Date(2020, time.September, 23, 7, 42, 0, 0, time.Local)
-			dynamicCertProvider = dynamiccert.New()
-			dynamicCertProvider.Set([]byte(defaultDynamicCertProviderCert), []byte(defaultDynamicCertProviderKey))
+			dynamicCertProvider = dynamiccert.New(name)
+			err = dynamicCertProvider.SetCertKeyContent([]byte(defaultDynamicCertProviderCert), []byte(defaultDynamicCertProviderKey))
+			r.NoError(err)
 
 			loadFile := func(filename string) string {
 				bytes, err := ioutil.ReadFile(filename)
@@ -669,6 +679,55 @@ func TestManagerControllerSync(t *testing.T) {
 					r.Equal([]coretesting.Action{expectedGetAction, expectedCreateAction, expectedUpdateAction}, pinnipedAPIClient.Actions())
 				})
 			})
+
+			when("the third resulting pod exec has invalid key data", func() {
+				var keyParseErrorMessage string
+
+				it.Before(func() {
+					keyParseErrorMessage = "failed to set signing cert/key content from agent pod some-namespace/some-agent-pod-name-123: TestManagerControllerSync: attempt to set invalid key pair: tls: failed to find any PEM data in key input"
+					fakeExecutor.errorsToReturn = []error{nil, nil}
+					fakeExecutor.resultsToReturn = []string{fakeCertPEM, ""}
+					startInformersAndController()
+				})
+
+				it("does not update the dynamic certificates provider", func() {
+					r.EqualError(controllerlib.TestSync(t, subject, *syncContext), keyParseErrorMessage)
+					requireDynamicCertProviderHasDefaultValues()
+				})
+
+				it("creates or updates the the CredentialIssuer status field with an error", func() {
+					r.EqualError(controllerlib.TestSync(t, subject, *syncContext), keyParseErrorMessage)
+
+					expectedCreateCredentialIssuer := &configv1alpha1.CredentialIssuer{
+						TypeMeta: metav1.TypeMeta{},
+						ObjectMeta: metav1.ObjectMeta{
+							Name: credentialIssuerResourceName,
+						},
+					}
+
+					expectedCredentialIssuer := &configv1alpha1.CredentialIssuer{
+						TypeMeta: metav1.TypeMeta{},
+						ObjectMeta: metav1.ObjectMeta{
+							Name: credentialIssuerResourceName,
+						},
+						Status: configv1alpha1.CredentialIssuerStatus{
+							Strategies: []configv1alpha1.CredentialIssuerStrategy{
+								{
+									Type:           configv1alpha1.KubeClusterSigningCertificateStrategyType,
+									Status:         configv1alpha1.ErrorStrategyStatus,
+									Reason:         configv1alpha1.CouldNotFetchKeyStrategyReason,
+									Message:        keyParseErrorMessage,
+									LastUpdateTime: metav1.NewTime(frozenNow),
+								},
+							},
+						},
+					}
+					expectedGetAction := coretesting.NewRootGetAction(credentialIssuerGVR, credentialIssuerResourceName)
+					expectedCreateAction := coretesting.NewRootCreateAction(credentialIssuerGVR, expectedCreateCredentialIssuer)
+					expectedUpdateAction := coretesting.NewRootUpdateSubresourceAction(credentialIssuerGVR, "status", expectedCredentialIssuer)
+					r.Equal([]coretesting.Action{expectedGetAction, expectedCreateAction, expectedUpdateAction}, pinnipedAPIClient.Actions())
+				})
+			})
 		})
 	}, spec.Parallel(), spec.Report(report.Terminal{}))
 }