diff --git a/cmd/amppkg/main.go b/cmd/amppkg/main.go index 7aa6941cf..d5e8a5bb9 100644 --- a/cmd/amppkg/main.go +++ b/cmd/amppkg/main.go @@ -98,7 +98,7 @@ func main() { die(errors.Wrap(err, "building cert cache")) } - if err = certCache.Init(nil); err != nil { + if err = certCache.Init(); err != nil { if *flagDevelopment { fmt.Println("WARNING:", err) } else { diff --git a/packager/certcache/certcache.go b/packager/certcache/certcache.go index 5ae6e2cb6..80bfd6a23 100644 --- a/packager/certcache/certcache.go +++ b/packager/certcache/certcache.go @@ -88,6 +88,7 @@ type CertCache struct { renewedCerts []*x509.Certificate ocspUpdateAfterMu sync.RWMutex ocspUpdateAfter time.Time + stop chan struct{} // TODO(twifkak): Implement a registry of Updateable instances which can be configured in the toml. ocspFile Updateable ocspFilePath string @@ -141,6 +142,7 @@ func New(certs []*x509.Certificate, certFetcher *certfetcher.CertFetcher, domain // you'd have one request, in the backend, and updating them all. ocspFile: &Chained{first: &InMemory{}, second: &LocalFile{path: ocspCache}}, ocspFilePath: ocspCache, + stop: make(chan struct{}), generateOCSPResponse: generateOCSPResponse, client: http.Client{Timeout: 60 * time.Second}, extractOCSPServer: func(cert *x509.Certificate) (string, error) { @@ -165,7 +167,7 @@ func New(certs []*x509.Certificate, certFetcher *certfetcher.CertFetcher, domain } } -func (this *CertCache) Init(stop chan struct{}) error { +func (this *CertCache) Init() error { this.updateCertIfNecessary() // Prime the OCSP disk and memory cache, so we can start serving immediately. @@ -181,11 +183,11 @@ func (this *CertCache) Init(stop chan struct{}) error { // like the OCSP responder giving you junk, but also sufficient time // to raise an alert if something has gone really wrong. // 7. The ability to serve old responses while fetching new responses. - go this.maintainOCSP(stop) + go this.maintainOCSP() if this.certFetcher != nil { // Update Certs in the background. - go this.maintainCerts(stop) + go this.maintainCerts() } this.isInitialized = true @@ -193,6 +195,19 @@ func (this *CertCache) Init(stop chan struct{}) error { return nil } +// Stop stops the goroutines spawned in Init, which are automatically updating the certificate and the OCSP response. +// It returns true if the call actually stops them, false if they have already been stopped. +func (this *CertCache) Stop() bool { + select { + // this.stop will never be used for sending a value. Thus this case matches only when it has already been closed. + case <-this.stop: + return false + default: + close(this.stop) + return true + } +} + // Gets the latest cert. // Returns the current cert if the cache has not been initialized or if the certFetcher is not set (good for testing) // If cert is invalid, it will attempt to renew. @@ -422,7 +437,7 @@ func waitForSpecifiedTime(waitTimeInMinutes int, numRetries int) int { // Checks for OCSP updates every hour. Terminates only when stop receives // a message. -func (this *CertCache) maintainOCSP(stop chan struct{}) { +func (this *CertCache) maintainOCSP() { // Only make one request per ocspCheckInterval, to minimize the impact // on OCSP servers that are buckling under load, per sleevi requirement: // 5. As with any system doing background requests on a remote server, @@ -440,7 +455,7 @@ func (this *CertCache) maintainOCSP(stop chan struct{}) { if err != nil { log.Println("Warning: OCSP update failed. Cached response may expire:", err) } - case <-stop: + case <-this.stop: ticker.Stop() return } @@ -635,7 +650,7 @@ func (this *CertCache) fetchOCSP(orig []byte, certs []*x509.Certificate, ocspUpd // Checks for cert updates every certCheckInterval hours. Terminates only when stop // receives a message. -func (this *CertCache) maintainCerts(stop chan struct{}) { +func (this *CertCache) maintainCerts() { // Only make one request per certCheckInterval, to minimize the impact // on servers that are buckling under load. ticker := time.NewTicker(certCheckInterval) @@ -644,7 +659,7 @@ func (this *CertCache) maintainCerts(stop chan struct{}) { select { case <-ticker.C: this.updateCertIfNecessary() - case <-stop: + case <-this.stop: ticker.Stop() return } diff --git a/packager/certcache/certcache_test.go b/packager/certcache/certcache_test.go index c8d079de8..b8ca00ab0 100644 --- a/packager/certcache/certcache_test.go +++ b/packager/certcache/certcache_test.go @@ -68,7 +68,6 @@ type CertCacheSuite struct { ocspServerWasCalled bool ocspHandler func(w http.ResponseWriter, req *http.Request) tempDir string - stop chan struct{} handler *CertCache } @@ -93,7 +92,7 @@ func (this *CertCacheSuite) New() (*CertCache, error) { return defaultHttpExpiry(req, resp) } } - err := certCache.Init(this.stop) + err := certCache.Init() return certCache, err } @@ -121,8 +120,6 @@ func (this *CertCacheSuite) SetupTest() { this.tempDir, err = ioutil.TempDir(os.TempDir(), "certcache_test") this.Require().NoError(err, "setting up test harness") - this.stop = make(chan struct{}) - this.handler, err = this.New() this.Require().NoError(err, "instantiating CertCache") } @@ -132,7 +129,7 @@ func (this *CertCacheSuite) TearDownTest() { this.fakeOCSPExpiry = nil // Reverse SetupTest. - this.stop <- struct{}{} + this.handler.Stop() err := os.RemoveAll(this.tempDir) if err != nil {