From 41735bf4786ab13459f1e04d52fa57052c19d55f Mon Sep 17 00:00:00 2001 From: Samuel Davidson Date: Tue, 3 Dec 2019 16:01:56 -0800 Subject: [PATCH] Changed Kubelet client and serving cert TTL/Expiry certs to use gaugefunc for calculating time remaining. Kubernetes-commit: aba0b315269dab469694af7fca879438a7f87e41 --- plugin/pkg/client/auth/exec/exec.go | 1 - plugin/pkg/client/auth/exec/metrics.go | 32 ++++++-------- plugin/pkg/client/auth/exec/metrics_test.go | 46 ++++++++------------ tools/metrics/metrics.go | 20 ++++----- util/certificate/certificate_manager.go | 11 ----- util/certificate/certificate_manager_test.go | 14 ++---- 6 files changed, 45 insertions(+), 79 deletions(-) diff --git a/plugin/pkg/client/auth/exec/exec.go b/plugin/pkg/client/auth/exec/exec.go index 9f029ee0..71ed045a 100644 --- a/plugin/pkg/client/auth/exec/exec.go +++ b/plugin/pkg/client/auth/exec/exec.go @@ -262,7 +262,6 @@ func (a *Authenticator) cert() (*tls.Certificate, error) { func (a *Authenticator) getCreds() (*credentials, error) { a.mu.Lock() defer a.mu.Unlock() - defer expirationMetrics.report(time.Now) if a.cachedCreds != nil && !a.credsExpired() { return a.cachedCreds, nil diff --git a/plugin/pkg/client/auth/exec/metrics.go b/plugin/pkg/client/auth/exec/metrics.go index 6ec6556c..caf0cca3 100644 --- a/plugin/pkg/client/auth/exec/metrics.go +++ b/plugin/pkg/client/auth/exec/metrics.go @@ -24,20 +24,25 @@ import ( ) type certificateExpirationTracker struct { - mu sync.RWMutex - m map[*Authenticator]time.Time - earliest time.Time + mu sync.RWMutex + m map[*Authenticator]time.Time + metricSet func(*time.Time) } -var expirationMetrics = &certificateExpirationTracker{m: map[*Authenticator]time.Time{}} +var expirationMetrics = &certificateExpirationTracker{ + m: map[*Authenticator]time.Time{}, + metricSet: func(e *time.Time) { + metrics.ClientCertExpiry.Set(e) + }, +} -// set stores the given expiration time and updates the updates earliest. +// set stores the given expiration time and updates the updates the certificate +// expiry metric to the earliest expiration time. func (c *certificateExpirationTracker) set(a *Authenticator, t time.Time) { c.mu.Lock() defer c.mu.Unlock() c.m[a] = t - // update earliest earliest := time.Time{} for _, t := range c.m { if t.IsZero() { @@ -47,18 +52,9 @@ func (c *certificateExpirationTracker) set(a *Authenticator, t time.Time) { earliest = t } } - c.earliest = earliest -} - -// report reports the ttl to the earliest reported expiration time. -// If no Authenticators have reported a certificate expiration, this reports nil. -func (c *certificateExpirationTracker) report(now func() time.Time) { - c.mu.RLock() - defer c.mu.RUnlock() - if c.earliest.IsZero() { - metrics.ClientCertTTL.Set(nil) + if earliest.IsZero() { + c.metricSet(nil) } else { - ttl := c.earliest.Sub(now()) - metrics.ClientCertTTL.Set(&ttl) + c.metricSet(&earliest) } } diff --git a/plugin/pkg/client/auth/exec/metrics_test.go b/plugin/pkg/client/auth/exec/metrics_test.go index 974f433c..dae90f5d 100644 --- a/plugin/pkg/client/auth/exec/metrics_test.go +++ b/plugin/pkg/client/auth/exec/metrics_test.go @@ -19,36 +19,27 @@ package exec import ( "testing" "time" - - "k8s.io/client-go/tools/metrics" ) -type mockTTLGauge struct { - v *time.Duration +type mockExpiryGauge struct { + v *time.Time } -func (m *mockTTLGauge) Set(d *time.Duration) { - m.v = d +func (m *mockExpiryGauge) Set(t *time.Time) { + m.v = t } -func ptr(d time.Duration) *time.Duration { - return &d +func ptr(t time.Time) *time.Time { + return &t } func TestCertificateExpirationTracker(t *testing.T) { now := time.Now() - nowFn := func() time.Time { return now } - mockMetric := &mockTTLGauge{} - realMetric := metrics.ClientCertTTL - metrics.ClientCertTTL = mockMetric - defer func() { - metrics.ClientCertTTL = realMetric - }() + mockMetric := &mockExpiryGauge{} - tracker := &certificateExpirationTracker{m: map[*Authenticator]time.Time{}} - tracker.report(nowFn) - if mockMetric.v != nil { - t.Error("empty tracker should record nil value") + tracker := &certificateExpirationTracker{ + m: map[*Authenticator]time.Time{}, + metricSet: mockMetric.Set, } firstAuthenticator := &Authenticator{} @@ -57,31 +48,31 @@ func TestCertificateExpirationTracker(t *testing.T) { desc string auth *Authenticator time time.Time - want *time.Duration + want *time.Time }{ { desc: "ttl for one authenticator", auth: firstAuthenticator, time: now.Add(time.Minute * 10), - want: ptr(time.Minute * 10), + want: ptr(now.Add(time.Minute * 10)), }, { desc: "second authenticator shorter ttl", auth: secondAuthenticator, time: now.Add(time.Minute * 5), - want: ptr(time.Minute * 5), + want: ptr(now.Add(time.Minute * 5)), }, { desc: "update shorter to be longer", auth: secondAuthenticator, time: now.Add(time.Minute * 15), - want: ptr(time.Minute * 10), + want: ptr(now.Add(time.Minute * 10)), }, { desc: "update shorter to be zero time", auth: firstAuthenticator, time: time.Time{}, - want: ptr(time.Minute * 15), + want: ptr(now.Add(time.Minute * 15)), }, { desc: "update last to be zero time records nil", @@ -93,13 +84,12 @@ func TestCertificateExpirationTracker(t *testing.T) { // Must run in series as the tests build off each other. t.Run(tc.desc, func(t *testing.T) { tracker.set(tc.auth, tc.time) - tracker.report(nowFn) if mockMetric.v != nil && tc.want != nil { - if mockMetric.v.Seconds() != tc.want.Seconds() { - t.Errorf("got: %v; want: %v", mockMetric.v, tc.want) + if !mockMetric.v.Equal(*tc.want) { + t.Errorf("got: %s; want: %s", mockMetric.v, tc.want) } } else if mockMetric.v != tc.want { - t.Errorf("got: %v; want: %v", mockMetric.v, tc.want) + t.Errorf("got: %s; want: %s", mockMetric.v, tc.want) } }) } diff --git a/tools/metrics/metrics.go b/tools/metrics/metrics.go index ee6800f3..6a8f25a9 100644 --- a/tools/metrics/metrics.go +++ b/tools/metrics/metrics.go @@ -31,9 +31,9 @@ type DurationMetric interface { Observe(duration time.Duration) } -// TTLMetric sets the time to live of something. -type TTLMetric interface { - Set(ttl *time.Duration) +// ExpiryMetric sets some time of expiry. If nil, assume not relevant. +type ExpiryMetric interface { + Set(expiry *time.Time) } // LatencyMetric observes client latency partitioned by verb and url. @@ -47,8 +47,8 @@ type ResultMetric interface { } var ( - // ClientCertTTL is the time to live of a client certificate - ClientCertTTL TTLMetric = noopTTL{} + // ClientCertExpiry is the expiry time of a client certificate + ClientCertExpiry ExpiryMetric = noopExpiry{} // ClientCertRotationAge is the age of a certificate that has just been rotated. ClientCertRotationAge DurationMetric = noopDuration{} // RequestLatency is the latency metric that rest clients will update. @@ -59,7 +59,7 @@ var ( // RegisterOpts contains all the metrics to register. Metrics may be nil. type RegisterOpts struct { - ClientCertTTL TTLMetric + ClientCertExpiry ExpiryMetric ClientCertRotationAge DurationMetric RequestLatency LatencyMetric RequestResult ResultMetric @@ -69,8 +69,8 @@ type RegisterOpts struct { // only be called once. func Register(opts RegisterOpts) { registerMetrics.Do(func() { - if opts.ClientCertTTL != nil { - ClientCertTTL = opts.ClientCertTTL + if opts.ClientCertExpiry != nil { + ClientCertExpiry = opts.ClientCertExpiry } if opts.ClientCertRotationAge != nil { ClientCertRotationAge = opts.ClientCertRotationAge @@ -88,9 +88,9 @@ type noopDuration struct{} func (noopDuration) Observe(time.Duration) {} -type noopTTL struct{} +type noopExpiry struct{} -func (noopTTL) Set(*time.Duration) {} +func (noopExpiry) Set(*time.Time) {} type noopLatency struct{} diff --git a/util/certificate/certificate_manager.go b/util/certificate/certificate_manager.go index e432aed7..1af46aba 100644 --- a/util/certificate/certificate_manager.go +++ b/util/certificate/certificate_manager.go @@ -112,12 +112,6 @@ type Config struct { // initialized using a generic, multi-use cert/key pair which will be // quickly replaced with a unique cert/key pair. BootstrapKeyPEM []byte - // CertificateExpiration will record a metric that shows the remaining - // lifetime of the certificate. This metric is a gauge because only the - // current cert expiry time is really useful. Reading this metric at any - // time simply gives the next expiration date, no need to keep some - // history (histogram) of all previous expiry dates. - CertificateExpiration Gauge // CertificateRotation will record a metric showing the time in seconds // that certificates lived before being rotated. This metric is a histogram // because there is value in keeping a history of rotation cadences. It @@ -185,7 +179,6 @@ type manager struct { certStore Store - certificateExpiration Gauge certificateRotation Histogram certificateRenewFailure Counter @@ -230,7 +223,6 @@ func NewManager(config *Config) (Manager, error) { certStore: config.CertificateStore, cert: cert, forceRotation: forceRotation, - certificateExpiration: config.CertificateExpiration, certificateRotation: config.CertificateRotation, certificateRenewFailure: config.CertificateRenewFailure, now: time.Now, @@ -554,9 +546,6 @@ func (m *manager) nextRotationDeadline() time.Time { deadline := m.cert.Leaf.NotBefore.Add(jitteryDuration(totalDuration)) klog.V(2).Infof("Certificate expiration is %v, rotation deadline is %v", notAfter, deadline) - if m.certificateExpiration != nil { - m.certificateExpiration.Set(float64(notAfter.Unix())) - } return deadline } diff --git a/util/certificate/certificate_manager_test.go b/util/certificate/certificate_manager_test.go index 85969857..96650adc 100644 --- a/util/certificate/certificate_manager_test.go +++ b/util/certificate/certificate_manager_test.go @@ -200,7 +200,6 @@ func TestSetRotationDeadline(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - g := metricMock{} m := manager{ cert: &tls.Certificate{ Leaf: &x509.Certificate{ @@ -208,10 +207,9 @@ func TestSetRotationDeadline(t *testing.T) { NotAfter: tc.notAfter, }, }, - getTemplate: func() *x509.CertificateRequest { return &x509.CertificateRequest{} }, - usages: []certificates.KeyUsage{}, - certificateExpiration: &g, - now: func() time.Time { return now }, + getTemplate: func() *x509.CertificateRequest { return &x509.CertificateRequest{} }, + usages: []certificates.KeyUsage{}, + now: func() time.Time { return now }, } jitteryDuration = func(float64) time.Duration { return time.Duration(float64(tc.notAfter.Sub(tc.notBefore)) * 0.7) } lowerBound := tc.notBefore.Add(time.Duration(float64(tc.notAfter.Sub(tc.notBefore)) * 0.7)) @@ -225,12 +223,6 @@ func TestSetRotationDeadline(t *testing.T) { deadline, lowerBound) } - if g.calls != 1 { - t.Errorf("%d metrics were recorded, wanted %d", g.calls, 1) - } - if g.lastValue != float64(tc.notAfter.Unix()) { - t.Errorf("%f value for metric was recorded, wanted %d", g.lastValue, tc.notAfter.Unix()) - } }) } }