From 6c555f94e34592763107c1c11186be8d3bb32b98 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 23 Sep 2020 08:26:59 -0400 Subject: [PATCH] internal/provider -> internal/dynamiccert 3 main reasons: - The cert and key that we store in this object are not always used for TLS. - The package name "provider" was a little too generic. - dynamiccert.Provider reads more go-ish than provider.DynamicCertProvider. Signed-off-by: Andrew Keesler --- cmd/local-user-authenticator/main.go | 12 ++--- cmd/local-user-authenticator/main_test.go | 8 ++-- .../controller/apicerts/certs_observer.go | 6 +-- .../apicerts/certs_observer_test.go | 6 +-- internal/controller/kubecertagent/execer.go | 6 +-- .../controller/kubecertagent/execer_test.go | 6 +-- .../controllermanager/prepare_controllers.go | 4 +- internal/dynamiccert/doc.go | 6 +++ internal/dynamiccert/provider.go | 45 +++++++++++++++++++ .../dynamic_tls_serving_cert_provider.go | 43 ------------------ internal/server/server.go | 10 ++--- 11 files changed, 80 insertions(+), 72 deletions(-) create mode 100644 internal/dynamiccert/doc.go create mode 100644 internal/dynamiccert/provider.go delete mode 100644 internal/provider/dynamic_tls_serving_cert_provider.go diff --git a/cmd/local-user-authenticator/main.go b/cmd/local-user-authenticator/main.go index 9513136a..4616c65d 100644 --- a/cmd/local-user-authenticator/main.go +++ b/cmd/local-user-authenticator/main.go @@ -37,7 +37,7 @@ import ( "go.pinniped.dev/internal/constable" "go.pinniped.dev/internal/controller/apicerts" "go.pinniped.dev/internal/controllerlib" - "go.pinniped.dev/internal/provider" + "go.pinniped.dev/internal/dynamiccert" ) const ( @@ -53,12 +53,12 @@ const ( ) type webhook struct { - certProvider provider.DynamicTLSServingCertProvider + certProvider dynamiccert.Provider secretInformer corev1informers.SecretInformer } func newWebhook( - certProvider provider.DynamicTLSServingCertProvider, + certProvider dynamiccert.Provider, secretInformer corev1informers.SecretInformer, ) *webhook { return &webhook{ @@ -295,7 +295,7 @@ func newK8sClient() (kubernetes.Interface, error) { func startControllers( ctx context.Context, - dynamicCertProvider provider.DynamicTLSServingCertProvider, + dynamicCertProvider dynamiccert.Provider, kubeClient kubernetes.Interface, kubeInformers kubeinformers.SharedInformerFactory, ) { @@ -339,7 +339,7 @@ func startControllers( func startWebhook( ctx context.Context, l net.Listener, - dynamicCertProvider provider.DynamicTLSServingCertProvider, + dynamicCertProvider dynamiccert.Provider, secretInformer corev1informers.SecretInformer, ) error { return newWebhook(dynamicCertProvider, secretInformer).start(ctx, l) @@ -366,7 +366,7 @@ func run() error { kubeinformers.WithNamespace(namespace), ) - dynamicCertProvider := provider.NewDynamicTLSServingCertProvider() + dynamicCertProvider := dynamiccert.New() startControllers(ctx, dynamicCertProvider, kubeClient, kubeInformers) klog.InfoS("controllers are ready") diff --git a/cmd/local-user-authenticator/main_test.go b/cmd/local-user-authenticator/main_test.go index 989292f1..3db5a542 100644 --- a/cmd/local-user-authenticator/main_test.go +++ b/cmd/local-user-authenticator/main_test.go @@ -33,7 +33,7 @@ import ( kubernetesfake "k8s.io/client-go/kubernetes/fake" "go.pinniped.dev/internal/certauthority" - "go.pinniped.dev/internal/provider" + "go.pinniped.dev/internal/dynamiccert" ) func TestWebhook(t *testing.T) { @@ -459,10 +459,10 @@ func createSecretInformer(t *testing.T, kubeClient kubernetes.Interface) corev1i return secretInformer } -// newClientProvider returns a provider.DynamicTLSServingCertProvider configured +// newClientProvider returns a dynamiccert.Provider configured // with valid serving cert, the CA bundle that can be used to verify the serving // cert, and the server name that can be used to verify the TLS peer. -func newCertProvider(t *testing.T) (provider.DynamicTLSServingCertProvider, []byte, string) { +func newCertProvider(t *testing.T) (dynamiccert.Provider, []byte, string) { t.Helper() serverName := "local-user-authenticator" @@ -476,7 +476,7 @@ func newCertProvider(t *testing.T) (provider.DynamicTLSServingCertProvider, []by certPEM, keyPEM, err := certauthority.ToPEM(cert) require.NoError(t, err) - certProvider := provider.NewDynamicTLSServingCertProvider() + certProvider := dynamiccert.New() certProvider.Set(certPEM, keyPEM) return certProvider, ca.Bundle(), serverName diff --git a/internal/controller/apicerts/certs_observer.go b/internal/controller/apicerts/certs_observer.go index 7a85fcb4..7f05d243 100644 --- a/internal/controller/apicerts/certs_observer.go +++ b/internal/controller/apicerts/certs_observer.go @@ -12,20 +12,20 @@ import ( pinnipedcontroller "go.pinniped.dev/internal/controller" "go.pinniped.dev/internal/controllerlib" - "go.pinniped.dev/internal/provider" + "go.pinniped.dev/internal/dynamiccert" ) type certsObserverController struct { namespace string certsSecretResourceName string - dynamicCertProvider provider.DynamicTLSServingCertProvider + dynamicCertProvider dynamiccert.Provider secretInformer corev1informers.SecretInformer } func NewCertsObserverController( namespace string, certsSecretResourceName string, - dynamicCertProvider provider.DynamicTLSServingCertProvider, + dynamicCertProvider dynamiccert.Provider, secretInformer corev1informers.SecretInformer, withInformer pinnipedcontroller.WithInformerOptionFunc, ) controllerlib.Controller { diff --git a/internal/controller/apicerts/certs_observer_test.go b/internal/controller/apicerts/certs_observer_test.go index adf965fe..452c3544 100644 --- a/internal/controller/apicerts/certs_observer_test.go +++ b/internal/controller/apicerts/certs_observer_test.go @@ -17,7 +17,7 @@ import ( kubernetesfake "k8s.io/client-go/kubernetes/fake" "go.pinniped.dev/internal/controllerlib" - "go.pinniped.dev/internal/provider" + "go.pinniped.dev/internal/dynamiccert" "go.pinniped.dev/internal/testutil" ) @@ -107,7 +107,7 @@ func TestObserverControllerSync(t *testing.T) { var timeoutContext context.Context var timeoutContextCancel context.CancelFunc var syncContext *controllerlib.Context - var dynamicCertProvider provider.DynamicTLSServingCertProvider + var dynamicCertProvider dynamiccert.Provider // Defer starting the informers until the last possible moment so that the // nested Before's can keep adding things to the informer caches. @@ -143,7 +143,7 @@ func TestObserverControllerSync(t *testing.T) { kubeInformerClient = kubernetesfake.NewSimpleClientset() kubeInformers = kubeinformers.NewSharedInformerFactory(kubeInformerClient, 0) - dynamicCertProvider = provider.NewDynamicTLSServingCertProvider() + dynamicCertProvider = dynamiccert.New() }) it.After(func() { diff --git a/internal/controller/kubecertagent/execer.go b/internal/controller/kubecertagent/execer.go index 7993fbd6..ccb2783a 100644 --- a/internal/controller/kubecertagent/execer.go +++ b/internal/controller/kubecertagent/execer.go @@ -19,14 +19,14 @@ import ( pinnipedcontroller "go.pinniped.dev/internal/controller" "go.pinniped.dev/internal/controller/issuerconfig" "go.pinniped.dev/internal/controllerlib" - "go.pinniped.dev/internal/provider" + "go.pinniped.dev/internal/dynamiccert" ) type execerController struct { agentInfo *Info credentialIssuerConfigNamespaceName string credentialIssuerConfigResourceName string - dynamicCertProvider provider.DynamicTLSServingCertProvider + dynamicCertProvider dynamiccert.Provider podCommandExecutor kubecertauthority.PodCommandExecutor clock clock.Clock pinnipedAPIClient pinnipedclientset.Interface @@ -37,7 +37,7 @@ func NewExecerController( agentInfo *Info, credentialIssuerConfigNamespaceName string, credentialIssuerConfigResourceName string, - dynamicCertProvider provider.DynamicTLSServingCertProvider, + dynamicCertProvider dynamiccert.Provider, podCommandExecutor kubecertauthority.PodCommandExecutor, pinnipedAPIClient pinnipedclientset.Interface, clock clock.Clock, diff --git a/internal/controller/kubecertagent/execer_test.go b/internal/controller/kubecertagent/execer_test.go index 3c36c3c8..1c15359a 100644 --- a/internal/controller/kubecertagent/execer_test.go +++ b/internal/controller/kubecertagent/execer_test.go @@ -24,7 +24,7 @@ import ( configv1alpha1 "go.pinniped.dev/generated/1.19/apis/config/v1alpha1" pinnipedfake "go.pinniped.dev/generated/1.19/client/clientset/versioned/fake" "go.pinniped.dev/internal/controllerlib" - "go.pinniped.dev/internal/provider" + "go.pinniped.dev/internal/dynamiccert" "go.pinniped.dev/internal/testutil" ) @@ -161,7 +161,7 @@ func TestManagerControllerSync(t *testing.T) { var agentPodInformerClient *kubernetesfake.Clientset var fakeExecutor *fakePodExecutor var agentPodTemplate *corev1.Pod - var dynamicCertProvider provider.DynamicTLSServingCertProvider + var dynamicCertProvider dynamiccert.Provider var fakeCertPEM, fakeKeyPEM string var credentialIssuerConfigGVR schema.GroupVersionResource var frozenNow time.Time @@ -241,7 +241,7 @@ func TestManagerControllerSync(t *testing.T) { agentPodInformer = kubeinformers.NewSharedInformerFactory(agentPodInformerClient, 0) fakeExecutor = &fakePodExecutor{r: r} frozenNow = time.Date(2020, time.September, 23, 7, 42, 0, 0, time.Local) - dynamicCertProvider = provider.NewDynamicTLSServingCertProvider() + dynamicCertProvider = dynamiccert.New() dynamicCertProvider.Set([]byte(defaultDynamicCertProviderCert), []byte(defaultDynamicCertProviderKey)) loadFile := func(filename string) string { diff --git a/internal/controllermanager/prepare_controllers.go b/internal/controllermanager/prepare_controllers.go index 4444c904..9d3776af 100644 --- a/internal/controllermanager/prepare_controllers.go +++ b/internal/controllermanager/prepare_controllers.go @@ -28,7 +28,7 @@ import ( "go.pinniped.dev/internal/controller/issuerconfig" "go.pinniped.dev/internal/controller/kubecertagent" "go.pinniped.dev/internal/controllerlib" - "go.pinniped.dev/internal/provider" + "go.pinniped.dev/internal/dynamiccert" "go.pinniped.dev/pkg/config/api" ) @@ -53,7 +53,7 @@ type Config struct { DiscoveryURLOverride *string // DynamicCertProvider provides a setter and a getter to the Pinniped API's serving cert. - DynamicCertProvider provider.DynamicTLSServingCertProvider + DynamicCertProvider dynamiccert.Provider // ServingCertDuration is the validity period, in seconds, of the API serving certificate. ServingCertDuration time.Duration diff --git a/internal/dynamiccert/doc.go b/internal/dynamiccert/doc.go new file mode 100644 index 00000000..d4da3cdc --- /dev/null +++ b/internal/dynamiccert/doc.go @@ -0,0 +1,6 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package dynamiccert provides a simple way of communicating a dynamically updating PEM-encoded +// certificate and key. +package dynamiccert diff --git a/internal/dynamiccert/provider.go b/internal/dynamiccert/provider.go new file mode 100644 index 00000000..a4ca6ad5 --- /dev/null +++ b/internal/dynamiccert/provider.go @@ -0,0 +1,45 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package dynamiccert + +import ( + "sync" + + "k8s.io/apiserver/pkg/server/dynamiccertificates" +) + +// Provider provides a getter, CurrentCertKeyContent(), and a setter, Set(), for a PEM-formatted +// certificate and matching key. +type Provider interface { + dynamiccertificates.CertKeyContentProvider + Set(certPEM, keyPEM []byte) +} + +type provider struct { + certPEM []byte + keyPEM []byte + mutex sync.RWMutex +} + +// New returns an empty Provider. The returned Provider is thread-safe. +func New() Provider { + return &provider{} +} + +func (p *provider) Set(certPEM, keyPEM []byte) { + p.mutex.Lock() // acquire a write lock + defer p.mutex.Unlock() + p.certPEM = certPEM + p.keyPEM = keyPEM +} + +func (p *provider) Name() string { + return "DynamicCertProvider" +} + +func (p *provider) CurrentCertKeyContent() (cert []byte, key []byte) { + p.mutex.RLock() // acquire a read lock + defer p.mutex.RUnlock() + return p.certPEM, p.keyPEM +} diff --git a/internal/provider/dynamic_tls_serving_cert_provider.go b/internal/provider/dynamic_tls_serving_cert_provider.go deleted file mode 100644 index 05a1d346..00000000 --- a/internal/provider/dynamic_tls_serving_cert_provider.go +++ /dev/null @@ -1,43 +0,0 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -package provider - -import ( - "sync" - - "k8s.io/apiserver/pkg/server/dynamiccertificates" -) - -type DynamicTLSServingCertProvider interface { - dynamiccertificates.CertKeyContentProvider - Set(certPEM, keyPEM []byte) -} - -type dynamicTLSServingCertProvider struct { - certPEM []byte - keyPEM []byte - mutex sync.RWMutex -} - -// TODO rename this type to DynamicCertProvider, since we are now going to use it for other types of certs too. -func NewDynamicTLSServingCertProvider() DynamicTLSServingCertProvider { - return &dynamicTLSServingCertProvider{} -} - -func (p *dynamicTLSServingCertProvider) Set(certPEM, keyPEM []byte) { - p.mutex.Lock() // acquire a write lock - defer p.mutex.Unlock() - p.certPEM = certPEM - p.keyPEM = keyPEM -} - -func (p *dynamicTLSServingCertProvider) Name() string { - return "DynamicTLSServingCertProvider" -} - -func (p *dynamicTLSServingCertProvider) CurrentCertKeyContent() (cert []byte, key []byte) { - p.mutex.RLock() // acquire a read lock - defer p.mutex.RUnlock() - return p.certPEM, p.keyPEM -} diff --git a/internal/server/server.go b/internal/server/server.go index 2c84cbaf..2eb484e8 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -29,8 +29,8 @@ import ( "go.pinniped.dev/internal/controller/issuerconfig" "go.pinniped.dev/internal/controllermanager" "go.pinniped.dev/internal/downward" + "go.pinniped.dev/internal/dynamiccert" "go.pinniped.dev/internal/here" - "go.pinniped.dev/internal/provider" "go.pinniped.dev/internal/registry/credentialrequest" "go.pinniped.dev/pkg/config" configapi "go.pinniped.dev/pkg/config/api" @@ -154,7 +154,7 @@ func (a *App) runServer(ctx context.Context) error { // is stored in a k8s Secret. Therefore it also effectively acting as // an in-memory cache of what is stored in the k8s Secret, helping to // keep incoming requests fast. - dynamicCertProvider := provider.NewDynamicTLSServingCertProvider() + dynamicCertProvider := dynamiccert.New() // Prepare to start the controllers, but defer actually starting them until the // post start hook of the aggregated API server. @@ -164,7 +164,7 @@ func (a *App) runServer(ctx context.Context) error { NamesConfig: &cfg.NamesConfig, DiscoveryURLOverride: cfg.DiscoveryInfo.URL, DynamicCertProvider: dynamicCertProvider, - //KubeAPISigningCertProvider: nil, // TODO pass this as a NewDynamicTLSServingCertProvider(), so it can be passed into the new controller + //KubeAPISigningCertProvider: nil, // TODO pass this as a dynamiccert.New(), so it can be passed into the new controller ServingCertDuration: time.Duration(*cfg.APIConfig.ServingCertificateConfig.DurationSeconds) * time.Second, ServingCertRenewBefore: time.Duration(*cfg.APIConfig.ServingCertificateConfig.RenewBeforeSeconds) * time.Second, IDPCache: idpCache, @@ -181,7 +181,7 @@ func (a *App) runServer(ctx context.Context) error { aggregatedAPIServerConfig, err := getAggregatedAPIServerConfig( dynamicCertProvider, idpCache, - k8sClusterCA, // TODO pass the same instance of DynamicTLSServingCertProvider as above, but wrapped into a new type that implements credentialrequest.CertIssuer, which should return ErrIncapableOfIssuingCertificates until the certs are available + k8sClusterCA, // TODO pass the same instance of dynamiccert.Provider as above, but wrapped into a new type that implements credentialrequest.CertIssuer, which should return ErrIncapableOfIssuingCertificates until the certs are available startControllersFunc, ) if err != nil { @@ -286,7 +286,7 @@ func getClusterCASigner( // Create a configuration for the aggregated API server. func getAggregatedAPIServerConfig( - dynamicCertProvider provider.DynamicTLSServingCertProvider, + dynamicCertProvider dynamiccert.Provider, authenticator credentialrequest.TokenCredentialRequestAuthenticator, issuer credentialrequest.CertIssuer, startControllersPostStartHook func(context.Context),