From 86a5993007e3c781749a5099b540307f65a4f377 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Tue, 3 Mar 2020 14:58:43 -0500 Subject: [PATCH 1/3] dynamic certs: do not copy mutex via shallow copy of tls.Config go vet error: call of dynamiccertificates.NewDynamicServingCertificateController copies lock value: crypto/tls.Config contains sync.Once contains sync.Mutex Signed-off-by: Monis Khan --- .../apiserver/pkg/server/dynamiccertificates/server_test.go | 2 +- .../apiserver/pkg/server/dynamiccertificates/tlsconfig.go | 4 ++-- staging/src/k8s.io/apiserver/pkg/server/secure_serving.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/server_test.go b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/server_test.go index 0503c2c10b1..4a1a9761040 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/server_test.go +++ b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/server_test.go @@ -79,7 +79,7 @@ func TestServingCert(t *testing.T) { } dynamicCertificateController := NewDynamicServingCertificateController( - *tlsConfig, + tlsConfig, &nullCAContent{name: "client-ca"}, defaultCertProvider, sniCerts, diff --git a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/tlsconfig.go b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/tlsconfig.go index 23a9b2e429e..a4bbd1e8fa7 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/tlsconfig.go +++ b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/tlsconfig.go @@ -41,7 +41,7 @@ const workItemKey = "key" type DynamicServingCertificateController struct { // baseTLSConfig is the static portion of the tlsConfig for serving to clients. It is copied and the copy is mutated // based on the dynamic cert state. - baseTLSConfig tls.Config + baseTLSConfig *tls.Config // clientCA provides the very latest content of the ca bundle clientCA CAContentProvider @@ -65,7 +65,7 @@ var _ Listener = &DynamicServingCertificateController{} // NewDynamicServingCertificateController returns a controller that can be used to keep a TLSConfig up to date. func NewDynamicServingCertificateController( - baseTLSConfig tls.Config, + baseTLSConfig *tls.Config, clientCA CAContentProvider, servingCert CertKeyContentProvider, sniCerts []SNICertKeyContentProvider, diff --git a/staging/src/k8s.io/apiserver/pkg/server/secure_serving.go b/staging/src/k8s.io/apiserver/pkg/server/secure_serving.go index 6d165abdf55..92149f124cc 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/secure_serving.go +++ b/staging/src/k8s.io/apiserver/pkg/server/secure_serving.go @@ -66,7 +66,7 @@ func (s *SecureServingInfo) tlsConfig(stopCh <-chan struct{}) (*tls.Config, erro if s.ClientCA != nil || s.Cert != nil || len(s.SNICerts) > 0 { dynamicCertificateController := dynamiccertificates.NewDynamicServingCertificateController( - *tlsConfig, + tlsConfig, s.ClientCA, s.Cert, s.SNICerts, From 3bc918e48427720938c731a6b26e9474b4819716 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Tue, 3 Mar 2020 18:40:34 -0500 Subject: [PATCH 2/3] dynamic certs: use correct name with event recorder Signed-off-by: Monis Khan --- .../apiserver/pkg/server/dynamiccertificates/tlsconfig.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/tlsconfig.go b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/tlsconfig.go index a4bbd1e8fa7..fde9e9a194e 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/tlsconfig.go +++ b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/tlsconfig.go @@ -200,7 +200,7 @@ func (c *DynamicServingCertificateController) syncCerts() error { klog.V(2).Infof("loaded serving cert [%q]: %s", c.servingCert.Name(), GetHumanCertDetail(x509Cert)) if c.eventRecorder != nil { - c.eventRecorder.Eventf(nil, nil, v1.EventTypeWarning, "TLSConfigChanged", "ServingCertificateReload", "loaded serving cert [%q]: %s", c.clientCA.Name(), GetHumanCertDetail(x509Cert)) + c.eventRecorder.Eventf(nil, nil, v1.EventTypeWarning, "TLSConfigChanged", "ServingCertificateReload", "loaded serving cert [%q]: %s", c.servingCert.Name(), GetHumanCertDetail(x509Cert)) } newTLSConfigCopy.Certificates = []tls.Certificate{cert} From 2cd6abece45bc62121097ce7cbe7f0d14b9be5e0 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Wed, 4 Mar 2020 09:54:27 -0500 Subject: [PATCH 3/3] dynamic certs: pass valid object to event recorder Signed-off-by: Monis Khan --- .../pkg/server/dynamiccertificates/named_certificates.go | 4 ++-- .../apiserver/pkg/server/dynamiccertificates/tlsconfig.go | 7 +++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/named_certificates.go b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/named_certificates.go index f4a7ed116e1..90a67f7b211 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/named_certificates.go +++ b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/named_certificates.go @@ -23,7 +23,7 @@ import ( "net" "strings" - v1 "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/validation" "k8s.io/klog" ) @@ -52,7 +52,7 @@ func (c *DynamicServingCertificateController) BuildNamedCertificates(sniCerts [] klog.V(2).Infof("loaded SNI cert [%d/%q]: %s", i, c.sniCerts[i].Name(), GetHumanCertDetail(x509Cert)) if c.eventRecorder != nil { - c.eventRecorder.Eventf(nil, nil, v1.EventTypeWarning, "TLSConfigChanged", "SNICertificateReload", "loaded SNI cert [%d/%q]: %s with explicit names %v", i, c.sniCerts[i].Name(), GetHumanCertDetail(x509Cert), names) + c.eventRecorder.Eventf(&corev1.ObjectReference{Name: c.sniCerts[i].Name()}, nil, corev1.EventTypeWarning, "TLSConfigChanged", "SNICertificateReload", "loaded SNI cert [%d/%q]: %s with explicit names %v", i, c.sniCerts[i].Name(), GetHumanCertDetail(x509Cert), names) } if len(names) == 0 { diff --git a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/tlsconfig.go b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/tlsconfig.go index fde9e9a194e..aef0710b5b9 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/tlsconfig.go +++ b/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/tlsconfig.go @@ -25,8 +25,7 @@ import ( "sync/atomic" "time" - v1 "k8s.io/api/core/v1" - + corev1 "k8s.io/api/core/v1" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/tools/events" @@ -178,7 +177,7 @@ func (c *DynamicServingCertificateController) syncCerts() error { for i, cert := range newClientCAs { klog.V(2).Infof("loaded client CA [%d/%q]: %s", i, c.clientCA.Name(), GetHumanCertDetail(cert)) if c.eventRecorder != nil { - c.eventRecorder.Eventf(nil, nil, v1.EventTypeWarning, "TLSConfigChanged", "CACertificateReload", "loaded client CA [%d/%q]: %s", i, c.clientCA.Name(), GetHumanCertDetail(cert)) + c.eventRecorder.Eventf(&corev1.ObjectReference{Name: c.clientCA.Name()}, nil, corev1.EventTypeWarning, "TLSConfigChanged", "CACertificateReload", "loaded client CA [%d/%q]: %s", i, c.clientCA.Name(), GetHumanCertDetail(cert)) } newClientCAPool.AddCert(cert) @@ -200,7 +199,7 @@ func (c *DynamicServingCertificateController) syncCerts() error { klog.V(2).Infof("loaded serving cert [%q]: %s", c.servingCert.Name(), GetHumanCertDetail(x509Cert)) if c.eventRecorder != nil { - c.eventRecorder.Eventf(nil, nil, v1.EventTypeWarning, "TLSConfigChanged", "ServingCertificateReload", "loaded serving cert [%q]: %s", c.servingCert.Name(), GetHumanCertDetail(x509Cert)) + c.eventRecorder.Eventf(&corev1.ObjectReference{Name: c.servingCert.Name()}, nil, corev1.EventTypeWarning, "TLSConfigChanged", "ServingCertificateReload", "loaded serving cert [%q]: %s", c.servingCert.Name(), GetHumanCertDetail(x509Cert)) } newTLSConfigCopy.Certificates = []tls.Certificate{cert}