From 03c8f52f43ecd385a8fba3fb25fe78c68d7c8e79 Mon Sep 17 00:00:00 2001 From: Delyan Raychev Date: Wed, 18 Nov 2020 09:24:56 -0800 Subject: [PATCH] certificate/vault: Change cache from a map to sync.Map (#2074) --- .../providers/vault/certificate_manager.go | 28 ++++++------------- .../vault/certificate_manager_test.go | 6 ++-- pkg/certificate/providers/vault/debugger.go | 9 +++--- .../providers/vault/debugger_test.go | 10 ++----- pkg/certificate/providers/vault/types.go | 4 +-- 5 files changed, 20 insertions(+), 37 deletions(-) diff --git a/pkg/certificate/providers/vault/certificate_manager.go b/pkg/certificate/providers/vault/certificate_manager.go index caa0df75d1..e52d34d770 100644 --- a/pkg/certificate/providers/vault/certificate_manager.go +++ b/pkg/certificate/providers/vault/certificate_manager.go @@ -33,10 +33,8 @@ const ( // NewCertManager implements certificate.Manager and wraps a Hashi Vault with methods to allow easy certificate issuance. func NewCertManager(vaultAddr, token string, role string, cfg configurator.Configurator) (*CertManager, error) { - cache := make(map[certificate.CommonName]certificate.Certificater) c := &CertManager{ announcements: make(chan announcements.Announcement), - cache: &cache, role: vaultRole(role), cfg: cfg, } @@ -82,15 +80,12 @@ func (cm *CertManager) issue(cn certificate.CommonName, validityPeriod time.Dura } func (cm *CertManager) deleteFromCache(cn certificate.CommonName) { - cm.cacheLock.Lock() - delete(*cm.cache, cn) - cm.cacheLock.Unlock() + cm.cache.Delete(cn) } func (cm *CertManager) getFromCache(cn certificate.CommonName) certificate.Certificater { - cm.cacheLock.Lock() - defer cm.cacheLock.Unlock() - if cert, exists := (*cm.cache)[cn]; exists { + if certificateInterface, exists := cm.cache.Load(cn); exists { + cert := certificateInterface.(certificate.Certificater) log.Trace().Msgf("Certificate found in cache CN=%s", cn) if rotor.ShouldRotate(cert) { log.Trace().Msgf("Certificate found in cache but has expired CN=%s", cn) @@ -116,9 +111,7 @@ func (cm *CertManager) IssueCertificate(cn certificate.CommonName, validityPerio return cert, err } - cm.cacheLock.Lock() - (*cm.cache)[cn] = cert - cm.cacheLock.Unlock() + cm.cache.Store(cn, cert) log.Info().Msgf("Issuing new certificate for CN=%s took %+v", cn, time.Since(start)) @@ -133,11 +126,10 @@ func (cm *CertManager) ReleaseCertificate(cn certificate.CommonName) { // ListCertificates lists all certificates issued func (cm *CertManager) ListCertificates() ([]certificate.Certificater, error) { var certs []certificate.Certificater - cm.cacheLock.Lock() - for _, cert := range *cm.cache { - certs = append(certs, cert) - } - cm.cacheLock.Unlock() + cm.cache.Range(func(cnInterface interface{}, certInterface interface{}) bool { + certs = append(certs, certInterface.(certificate.Certificater)) + return true + }) return certs, nil } @@ -170,9 +162,7 @@ func (cm *CertManager) RotateCertificate(cn certificate.CommonName) (certificate return cert, err } - cm.cacheLock.Lock() - (*cm.cache)[cn] = cert - cm.cacheLock.Unlock() + cm.cache.Store(cn, cert) cm.announcements <- announcements.Announcement{} log.Info().Msgf("Rotating certificate CN=%s took %+v", cn, time.Since(start)) diff --git a/pkg/certificate/providers/vault/certificate_manager_test.go b/pkg/certificate/providers/vault/certificate_manager_test.go index 9389f978f6..59833d37ca 100644 --- a/pkg/certificate/providers/vault/certificate_manager_test.go +++ b/pkg/certificate/providers/vault/certificate_manager_test.go @@ -101,12 +101,10 @@ var _ = Describe("Test client helpers", func() { Context("Test Hashi Vault functions", func() { cm := CertManager{ - cache: &map[certificate.CommonName]certificate.Certificater{ - expiredCertCN: expiredCert, - validCertCN: validCert, - }, ca: rootCert, } + cm.cache.Store(expiredCertCN, expiredCert) + cm.cache.Store(validCertCN, validCert) It("gets certs from cache", func() { // This cert does not exist - returns nil diff --git a/pkg/certificate/providers/vault/debugger.go b/pkg/certificate/providers/vault/debugger.go index e679d0df83..1e44c4399f 100644 --- a/pkg/certificate/providers/vault/debugger.go +++ b/pkg/certificate/providers/vault/debugger.go @@ -7,10 +7,9 @@ import ( // ListIssuedCertificates implements CertificateDebugger interface and returns the list of issued certificates. func (cm *CertManager) ListIssuedCertificates() []certificate.Certificater { var certs []certificate.Certificater - cm.cacheLock.Lock() - defer cm.cacheLock.Unlock() - for _, cert := range *cm.cache { - certs = append(certs, cert) - } + cm.cache.Range(func(cnInterface interface{}, certInterface interface{}) bool { + certs = append(certs, certInterface.(certificate.Certificater)) + return true + }) return certs } diff --git a/pkg/certificate/providers/vault/debugger_test.go b/pkg/certificate/providers/vault/debugger_test.go index 540952d7d1..c375f6851a 100644 --- a/pkg/certificate/providers/vault/debugger_test.go +++ b/pkg/certificate/providers/vault/debugger_test.go @@ -19,13 +19,9 @@ var _ = Describe("Test Vault Debugger", func() { expiration: time.Now(), commonName: "foo.bar.co.uk", } - cache := map[certificate.CommonName]certificate.Certificater{ - "foo": cert, - } - cm := CertManager{ - cache: &cache, - } - It("lists all issued certificets", func() { + cm := CertManager{} + cm.cache.Store("foo", cert) + It("lists all issued certificates", func() { actual := cm.ListIssuedCertificates() expected := []certificate.Certificater{cert} Expect(actual).To(Equal(expected)) diff --git a/pkg/certificate/providers/vault/types.go b/pkg/certificate/providers/vault/types.go index b31b0f2114..fb0d6bd5ce 100644 --- a/pkg/certificate/providers/vault/types.go +++ b/pkg/certificate/providers/vault/types.go @@ -19,8 +19,8 @@ type CertManager struct { announcements chan announcements.Announcement // Cache for all the certificates issued - cache *map[certificate.CommonName]certificate.Certificater - cacheLock sync.Mutex + // Types: map[certificate.CommonName]certificate.Certificater + cache sync.Map // Hashicorp Vault client client *api.Client