Merge pull request #100979 from mikedanese/tlscleanup

force implementors of dyanmiccertificates providers to think about notify

pkg/controlplane/instance.go

@@ -464,9 +464,7 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget)
 
 		// prime values and start listeners
 		if m.ClusterAuthenticationInfo.ClientCA != nil {
-			if notifier, ok := m.ClusterAuthenticationInfo.ClientCA.(dynamiccertificates.Notifier); ok {
+			m.ClusterAuthenticationInfo.ClientCA.AddListener(controller)
-				notifier.AddListener(controller)
-			}
 			if controller, ok := m.ClusterAuthenticationInfo.ClientCA.(dynamiccertificates.ControllerRunner); ok {
 				// runonce to be sure that we have a value.
 				if err := controller.RunOnce(); err != nil {
@@ -476,9 +474,7 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget)
 			}
 		}
 		if m.ClusterAuthenticationInfo.RequestHeaderCA != nil {
-			if notifier, ok := m.ClusterAuthenticationInfo.RequestHeaderCA.(dynamiccertificates.Notifier); ok {
+			m.ClusterAuthenticationInfo.RequestHeaderCA.AddListener(controller)
-				notifier.AddListener(controller)
-			}
 			if controller, ok := m.ClusterAuthenticationInfo.RequestHeaderCA.(dynamiccertificates.ControllerRunner); ok {
 				// runonce to be sure that we have a value.
 				if err := controller.RunOnce(); err != nil {

staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/delegating.go

@@ -32,6 +32,7 @@ import (
 	"k8s.io/apiserver/pkg/authentication/request/websocket"
 	"k8s.io/apiserver/pkg/authentication/request/x509"
 	"k8s.io/apiserver/pkg/authentication/token/cache"
+	"k8s.io/apiserver/pkg/server/dynamiccertificates"
 	webhooktoken "k8s.io/apiserver/plugin/pkg/authenticator/token/webhook"
 	authenticationclient "k8s.io/client-go/kubernetes/typed/authentication/v1"
 )
@@ -58,7 +59,7 @@ type DelegatingAuthenticatorConfig struct {
 	// CAContentProvider are the options for verifying incoming connections using mTLS and directly assigning to users.
 	// Generally this is the CA bundle file used to authenticate client certificates
 	// If this is nil, then mTLS will not be used.
-	ClientCertificateCAContentProvider CAContentProvider
+	ClientCertificateCAContentProvider dynamiccertificates.CAContentProvider
 
 	APIAudiences authenticator.Audiences
 

staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/requestheader.go

@@ -17,9 +17,8 @@ limitations under the License.
 package authenticatorfactory
 
 import (
-	"crypto/x509"
-
 	"k8s.io/apiserver/pkg/authentication/request/headerrequest"
+	"k8s.io/apiserver/pkg/server/dynamiccertificates"
 )
 
 type RequestHeaderConfig struct {
@@ -32,17 +31,7 @@ type RequestHeaderConfig struct {
 	ExtraHeaderPrefixes headerrequest.StringSliceProvider
 	// CAContentProvider the options for verifying incoming connections using mTLS.  Generally this points to CA bundle file which is used verify the identity of the front proxy.
 	//	It may produce different options at will.
-	CAContentProvider CAContentProvider
+	CAContentProvider dynamiccertificates.CAContentProvider
 	// AllowedClientNames is a list of common names that may be presented by the authenticating front proxy.  Empty means: accept any.
 	AllowedClientNames headerrequest.StringSliceProvider
 }
-
-// CAContentProvider provides ca bundle byte content
-type CAContentProvider interface {
-	// Name is just an identifier
-	Name() string
-	// CurrentCABundleContent provides ca bundle byte content
-	CurrentCABundleContent() []byte
-	// VerifyOptions provides VerifyOptions for authenticators
-	VerifyOptions() (x509.VerifyOptions, bool)
-}

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/cert_key.go

@@ -20,21 +20,6 @@ import (
 	"bytes"
 )
 
-// CertKeyContentProvider provides a certificate and matching private key
-type CertKeyContentProvider interface {
-	// Name is just an identifier
-	Name() string
-	// CurrentCertKeyContent provides cert and key byte content
-	CurrentCertKeyContent() ([]byte, []byte)
-}
-
-// SNICertKeyContentProvider provides a certificate and matching private key as well as optional explicit names
-type SNICertKeyContentProvider interface {
-	CertKeyContentProvider
-	// SNINames provides names used for SNI. May return nil.
-	SNINames() []string
-}
-
 // certKeyContent holds the content for the cert and key
 type certKeyContent struct {
 	cert []byte

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/client_ca.go

@@ -18,20 +18,8 @@ package dynamiccertificates
 
 import (
 	"bytes"
-	"crypto/x509"
 )
 
-// CAContentProvider provides ca bundle byte content
-type CAContentProvider interface {
-	// Name is just an identifier
-	Name() string
-	// CurrentCABundleContent provides ca bundle byte content.  Errors can be contained to the controllers initializing
-	// the value.  By the time you get here, you should always be returning a value that won't fail.
-	CurrentCABundleContent() []byte
-	// VerifyOptions provides VerifyOptions for authenticators
-	VerifyOptions() (x509.VerifyOptions, bool)
-}
-
 // dynamicCertificateContent holds the content that overrides the baseTLSConfig
 type dynamicCertificateContent struct {
 	// clientCA holds the content for the clientCA bundle

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/configmap_cafile_content.go

@@ -58,7 +58,6 @@ type ConfigMapCAController struct {
 	preRunCaches []cache.InformerSynced
 }
 
-var _ Notifier = &ConfigMapCAController{}
 var _ CAContentProvider = &ConfigMapCAController{}
 var _ ControllerRunner = &ConfigMapCAController{}
 

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_cafile_content.go

@@ -35,18 +35,6 @@ import (
 // FileRefreshDuration is exposed so that integration tests can crank up the reload speed.
 var FileRefreshDuration = 1 * time.Minute
 
-// Listener is an interface to use to notify interested parties of a change.
-type Listener interface {
-	// Enqueue should be called when an input may have changed
-	Enqueue()
-}
-
-// Notifier is a way to add listeners
-type Notifier interface {
-	// AddListener is adds a listener to be notified of potential input changes
-	AddListener(listener Listener)
-}
-
 // ControllerRunner is a generic interface for starting a controller
 type ControllerRunner interface {
 	// RunOnce runs the sync loop a single time.  This useful for synchronous priming

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_serving_content.go

@@ -47,7 +47,6 @@ type DynamicCertKeyPairContent struct {
 	queue workqueue.RateLimitingInterface
 }
 
-var _ Notifier = &DynamicCertKeyPairContent{}
 var _ CertKeyContentProvider = &DynamicCertKeyPairContent{}
 var _ ControllerRunner = &DynamicCertKeyPairContent{}
 

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_sni_content.go

@@ -22,7 +22,6 @@ type DynamicFileSNIContent struct {
 	sniNames []string
 }
 
-var _ Notifier = &DynamicFileSNIContent{}
 var _ SNICertKeyContentProvider = &DynamicFileSNIContent{}
 var _ ControllerRunner = &DynamicFileSNIContent{}
 

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/interfaces.go

@@ -0,0 +1,68 @@
+/*
+Copyright 2021 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package dynamiccertificates
+
+import (
+	"crypto/x509"
+)
+
+// Listener is an interface to use to notify interested parties of a change.
+type Listener interface {
+	// Enqueue should be called when an input may have changed
+	Enqueue()
+}
+
+// Notifier is a way to add listeners
+type Notifier interface {
+	// AddListener is adds a listener to be notified of potential input changes.
+	// This is a noop on static providers.
+	AddListener(listener Listener)
+}
+
+// CAContentProvider provides ca bundle byte content
+type CAContentProvider interface {
+	Notifier
+
+	// Name is just an identifier.
+	Name() string
+	// CurrentCABundleContent provides ca bundle byte content. Errors can be
+	// contained to the controllers initializing the value. By the time you get
+	// here, you should always be returning a value that won't fail.
+	CurrentCABundleContent() []byte
+	// VerifyOptions provides VerifyOptions for authenticators.
+	VerifyOptions() (x509.VerifyOptions, bool)
+}
+
+// CertKeyContentProvider provides a certificate and matching private key.
+type CertKeyContentProvider interface {
+	Notifier
+
+	// Name is just an identifier.
+	Name() string
+	// CurrentCertKeyContent provides cert and key byte content.
+	CurrentCertKeyContent() ([]byte, []byte)
+}
+
+// SNICertKeyContentProvider provides a certificate and matching private key as
+// well as optional explicit names.
+type SNICertKeyContentProvider interface {
+	Notifier
+
+	CertKeyContentProvider
+	// SNINames provides names used for SNI. May return nil.
+	SNINames() []string
+}

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/server_test.go

@@ -208,6 +208,8 @@ func (c *nullCAContent) Name() string {
 	return c.name
 }
 
+func (c *nullCAContent) AddListener(Listener) {}
+
 // CurrentCABundleContent provides ca bundle byte content
 func (c *nullCAContent) CurrentCABundleContent() (cabundle []byte) {
 	return nil

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/static_content.go

@@ -46,6 +46,8 @@ func (c *staticCAContent) Name() string {
 	return c.name
 }
 
+func (c *staticCAContent) AddListener(Listener) {}
+
 // CurrentCABundleContent provides ca bundle byte content
 func (c *staticCAContent) CurrentCABundleContent() (cabundle []byte) {
 	return c.caBundle.caBundle
@@ -61,11 +63,6 @@ type staticCertKeyContent struct {
 	key  []byte
 }
 
-type staticSNICertKeyContent struct {
-	staticCertKeyContent
-	sniNames []string
-}
-
 // NewStaticCertKeyContent returns a CertKeyContentProvider that always returns the same value
 func NewStaticCertKeyContent(name string, cert, key []byte) (CertKeyContentProvider, error) {
 	// Ensure that the key matches the cert and both are valid
@@ -81,6 +78,23 @@ func NewStaticCertKeyContent(name string, cert, key []byte) (CertKeyContentProvi
 	}, nil
 }
 
+// Name is just an identifier
+func (c *staticCertKeyContent) Name() string {
+	return c.name
+}
+
+func (c *staticCertKeyContent) AddListener(Listener) {}
+
+// CurrentCertKeyContent provides cert and key content
+func (c *staticCertKeyContent) CurrentCertKeyContent() ([]byte, []byte) {
+	return c.cert, c.key
+}
+
+type staticSNICertKeyContent struct {
+	staticCertKeyContent
+	sniNames []string
+}
+
 // NewStaticSNICertKeyContent returns a SNICertKeyContentProvider that always returns the same value
 func NewStaticSNICertKeyContent(name string, cert, key []byte, sniNames ...string) (SNICertKeyContentProvider, error) {
 	// Ensure that the key matches the cert and both are valid
@@ -99,16 +113,8 @@ func NewStaticSNICertKeyContent(name string, cert, key []byte, sniNames ...strin
 	}, nil
 }
 
-// Name is just an identifier
-func (c *staticCertKeyContent) Name() string {
-	return c.name
-}
-
-// CurrentCertKeyContent provides cert and key content
-func (c *staticCertKeyContent) CurrentCertKeyContent() ([]byte, []byte) {
-	return c.cert, c.key
-}
-
 func (c *staticSNICertKeyContent) SNINames() []string {
 	return c.sniNames
 }
+
+func (c *staticSNICertKeyContent) AddListener(Listener) {}

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/union_content.go

@@ -26,7 +26,6 @@ import (
 
 type unionCAContent []CAContentProvider
 
-var _ Notifier = &unionCAContent{}
 var _ CAContentProvider = &unionCAContent{}
 var _ ControllerRunner = &unionCAContent{}
 
@@ -77,9 +76,7 @@ func (c unionCAContent) VerifyOptions() (x509.VerifyOptions, bool) {
 // AddListener adds a listener to be notified when the CA content changes.
 func (c unionCAContent) AddListener(listener Listener) {
 	for _, curr := range c {
-		if notifier, ok := curr.(Notifier); ok {
+		curr.AddListener(listener)
-			notifier.AddListener(listener)
-		}
 	}
 }
 

staging/src/k8s.io/apiserver/pkg/server/options/authentication_dynamic_request_header.go

@@ -26,7 +26,6 @@ import (
 )
 
 var _ dynamiccertificates.ControllerRunner = &DynamicRequestHeaderController{}
-var _ dynamiccertificates.Notifier = &DynamicRequestHeaderController{}
 var _ dynamiccertificates.CAContentProvider = &DynamicRequestHeaderController{}
 
 var _ headerrequest.RequestHeaderAuthRequestProvider = &DynamicRequestHeaderController{}

staging/src/k8s.io/apiserver/pkg/server/secure_serving.go

@@ -86,13 +86,14 @@ func (s *SecureServingInfo) tlsConfig(stopCh <-chan struct{}) (*tls.Config, erro
 			s.SNICerts,
 			nil, // TODO see how to plumb an event recorder down in here. For now this results in simply klog messages.
 		)
-		// register if possible
+
-		if notifier, ok := s.ClientCA.(dynamiccertificates.Notifier); ok {
+		if s.ClientCA != nil {
-			notifier.AddListener(dynamicCertificateController)
+			s.ClientCA.AddListener(dynamicCertificateController)
 		}
-		if notifier, ok := s.Cert.(dynamiccertificates.Notifier); ok {
+		if s.Cert != nil {
-			notifier.AddListener(dynamicCertificateController)
+			s.Cert.AddListener(dynamicCertificateController)
 		}
+
 		// start controllers if possible
 		if controller, ok := s.ClientCA.(dynamiccertificates.ControllerRunner); ok {
 			// runonce to try to prime data.  If this fails, it's ok because we fail closed.
@@ -113,10 +114,7 @@ func (s *SecureServingInfo) tlsConfig(stopCh <-chan struct{}) (*tls.Config, erro
 			go controller.Run(1, stopCh)
 		}
 		for _, sniCert := range s.SNICerts {
-			if notifier, ok := sniCert.(dynamiccertificates.Notifier); ok {
+			sniCert.AddListener(dynamicCertificateController)
-				notifier.AddListener(dynamicCertificateController)
-			}
-
 			if controller, ok := sniCert.(dynamiccertificates.ControllerRunner); ok {
 				// runonce to try to prime data.  If this fails, it's ok because we fail closed.
 				// Files are required to be populated already, so this is for convenience.