Skip to content

Commit

Permalink
Addressed twifkak comments: refactored IsHealthy, also added healthz …
Browse files Browse the repository at this point in the history
…http handler.
  • Loading branch information
banaag committed Oct 23, 2019
1 parent f02330c commit 11d9918
Show file tree
Hide file tree
Showing 10 changed files with 170 additions and 108 deletions.
8 changes: 7 additions & 1 deletion cmd/amppkg/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"github.com/pkg/errors"

"github.com/ampproject/amppackager/packager/certloader"
"github.com/ampproject/amppackager/packager/healthz"
"github.com/ampproject/amppackager/packager/mux"
"github.com/ampproject/amppackager/packager/rtv"
"github.com/ampproject/amppackager/packager/signer"
Expand Down Expand Up @@ -95,6 +96,11 @@ func main() {
}
}

healthz, err := healthz.New(certCache)
if err != nil {
die(errors.Wrap(err, "building healthz"))
}

rtvCache, err := rtv.New()
if err != nil {
die(errors.Wrap(err, "initializing rtv cache"))
Expand Down Expand Up @@ -127,7 +133,7 @@ func main() {
Addr: addr,
// Don't use DefaultServeMux, per
// https://blog.cloudflare.com/exposing-go-on-the-internet/.
Handler: logIntercept{mux.New(certCache, signer, validityMap)},
Handler: logIntercept{mux.New(certCache, signer, validityMap, healthz)},
ReadTimeout: 10 * time.Second,
ReadHeaderTimeout: 5 * time.Second,
// If needing to stream the response, disable WriteTimeout and
Expand Down
58 changes: 19 additions & 39 deletions packager/certcache/certcache.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"context"
"crypto/x509"
"encoding/base64"
"fmt"
"io"
"io/ioutil"
"log"
Expand Down Expand Up @@ -53,7 +52,8 @@ const maxOCSPResponseBytes = 1024 * 1024
const ocspCheckInterval = 1 * time.Hour

type CertHandler interface {
GetLatestCert() (*x509.Certificate)
GetLatestCert() *x509.Certificate
IsHealthy() error
}

type CertCache struct {
Expand Down Expand Up @@ -129,7 +129,7 @@ func (this *CertCache) Init(stop chan struct{}) error {
// For now this just returns the first entry in the certs field in the cache.
// For follow-on changes, we will transform this to a lambda so that anything
// that needs a cert can do the cert refresh logic (if needed) on demand.
func (this *CertCache) GetLatestCert() (*x509.Certificate) {
func (this *CertCache) GetLatestCert() *x509.Certificate {
// TODO(banaag): check if cert is still valid, refresh if not.
return this.certs[0]
}
Expand Down Expand Up @@ -158,26 +158,6 @@ func (this *CertCache) ocspMidpoint(bytes []byte, issuer *x509.Certificate) (tim
}

func (this *CertCache) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
// Follow https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/
// for /healthz responses.
if req.URL.EscapedPath() == "/healthz" {
ocsp, _, errorOcsp := this.readOCSP()
_, errorHealth := this.isHealthy(ocsp)
// errorHealth = errors.New("Error parsing OCSP response.")
if errorOcsp != nil || errorHealth != nil {
resp.WriteHeader(500)
if errorOcsp != nil {
resp.Write([]byte(fmt.Sprintf("not healthy: %v", errorOcsp)))
} else if errorHealth != nil {
resp.Write([]byte(fmt.Sprintf("not healthy: %v", errorHealth)))
}
} else {
resp.WriteHeader(200)
resp.Write([]byte("ok"))
}
return
}

params := mux.Params(req)
if params["certName"] == this.certName {
// https://tools.ietf.org/html/draft-yasskin-httpbis-origin-signed-exchanges-impl-00#section-3.3
Expand Down Expand Up @@ -222,34 +202,34 @@ func (this *CertCache) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
// 8. Some idea of what to do when "things go bad".
// What happens when it's been 7 days, no new OCSP response can be obtained,
// and the current response is about to expire?
func (this *CertCache) IsHealthy() bool {
func (this *CertCache) IsHealthy() error {
ocsp, _, errorOcsp := this.readOCSP()
isHealthy, errHealth := this.isHealthy(ocsp)
if errorOcsp != nil {
return false
return errorOcsp
}
if errHealth != nil {
return false
errorHealth := this.isHealthy(ocsp)
if errorHealth != nil {
return errorHealth
}
return isHealthy
return nil
}

func (this *CertCache) isHealthy(ocspResp []byte) (bool, error) {
func (this *CertCache) isHealthy(ocspResp []byte) error {
if ocspResp == nil {
return false, errors.New("OCSP response not yet fetched.")
return errors.New("OCSP response not yet fetched.")
}
issuer := this.findIssuer()
if issuer == nil {
return false, errors.New("Cannot find issuer certificate in CertFile.")
return errors.New("Cannot find issuer certificate in CertFile.")
}
resp, err := ocsp.ParseResponseForCert(ocspResp, this.certs[0], issuer)
if err != nil {
return false, errors.Wrap(err, "Error parsing OCSP response.")
return errors.Wrap(err, "Error parsing OCSP response.")
}
if resp.NextUpdate.Before(time.Now()) {
return false, errors.Errorf("Cached OCSP is stale, NextUpdate: %v", resp.NextUpdate)
return errors.Errorf("Cached OCSP is stale, NextUpdate: %v", resp.NextUpdate)
}
return true, nil
return nil
}

// Returns the OCSP response and expiry, refreshing if necessary.
Expand All @@ -264,9 +244,9 @@ func (this *CertCache) readOCSP() ([]byte, time.Time, error) {
if len(ocsp) == 0 {
return nil, time.Time{}, errors.New("Missing OCSP response.")
}
health, err := this.isHealthy(ocsp)
if err != nil || !health {
return nil, time.Time{}, errors.New("OCSP failed health check.")
err = this.isHealthy(ocsp)
if err != nil {
return nil, time.Time{}, errors.Wrap(err, "OCSP failed health check.")
}
this.ocspUpdateAfterMu.Lock()
defer this.ocspUpdateAfterMu.Unlock()
Expand Down Expand Up @@ -459,7 +439,7 @@ func (this *CertCache) fetchOCSP(orig []byte, ocspUpdateAfter *time.Time) []byte
// OCSP duration must be <=7 days, per
// https://wicg.github.io/webpackage/draft-yasskin-httpbis-origin-signed-exchanges-impl.html#cross-origin-trust.
// Serving these responses may cause UAs to reject the SXG.
if resp.NextUpdate.Sub(resp.ThisUpdate) > time.Hour * 24 * 7 {
if resp.NextUpdate.Sub(resp.ThisUpdate) > time.Hour*24*7 {
log.Printf("OCSP nextUpdate %+v too far ahead of thisUpdate %+v\n", resp.NextUpdate, resp.ThisUpdate)
return orig
}
Expand Down
71 changes: 18 additions & 53 deletions packager/certcache/certcache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ func (this *CertCacheSuite) TearDownTest() {
}

func (this *CertCacheSuite) mux() http.Handler {
return mux.New(this.handler, nil, nil)
return mux.New(this.handler, nil, nil, nil)
}

func (this *CertCacheSuite) ocspServerCalled(f func()) bool {
Expand Down Expand Up @@ -181,68 +181,33 @@ func (this *CertCacheSuite) TestServesCertificate() {
this.Assert().NotContains(cbor, "sct")
}

func (this *CertCacheSuite) TestHealthzOk() {
resp := pkgt.Get(this.T(), this.mux(), "/healthz")
this.Assert().Equal(http.StatusOK, resp.StatusCode, "ok", resp)
}

func (this *CertCacheSuite) TestHealthzFail() {
// Prime memory cache with a past-midpoint OCSP:
err := os.Remove(filepath.Join(this.tempDir, "ocsp"))
this.Require().NoError(err, "deleting OCSP tempfile")
this.fakeOCSP, err = FakeOCSPResponse(time.Now().Add(-4 * 24 * time.Hour))
this.Require().NoError(err, "creating stale OCSP response")
this.Require().True(this.ocspServerCalled(func() {
this.handler, err = this.New()
this.Require().NoError(err, "reinstantiating CertCache")
}))

// Prime disk cache with a bad OCSP:
freshOCSP := []byte("0xdeadbeef")
this.fakeOCSP = freshOCSP
// freshOCSP, err := FakeOCSPResponse(time.Now())
// this.Require().NoError(err, "creating fresh OCSP response")
err = ioutil.WriteFile(filepath.Join(this.tempDir, "ocsp"), freshOCSP, 0644)
this.Require().NoError(err, "writing fresh OCSP response to disk")

// On update, verify network is not called (fresh OCSP from disk is used):
this.Assert().True(this.ocspServerCalled(func() {
this.handler.readOCSP()
}))

resp := pkgt.Get(this.T(), this.mux(), "/healthz")
this.Assert().Equal(http.StatusInternalServerError, resp.StatusCode, "error", resp)
}

func (this *CertCacheSuite) TestCertCacheIsHealthy() {
this.Assert().True(this.handler.IsHealthy())
this.Assert().Nil(this.handler.IsHealthy())
}

func (this *CertCacheSuite) TestCertCacheIsNotHealthy() {
// Prime memory cache with a past-midpoint OCSP:
err := os.Remove(filepath.Join(this.tempDir, "ocsp"))
this.Require().NoError(err, "deleting OCSP tempfile")
this.fakeOCSP, err = FakeOCSPResponse(time.Now().Add(-4 * 24 * time.Hour))
this.Require().NoError(err, "creating stale OCSP response")
this.Require().True(this.ocspServerCalled(func() {
this.handler, err = this.New()
this.Require().NoError(err, "reinstantiating CertCache")
}))
// Prime memory cache with a past-midpoint OCSP:
err := os.Remove(filepath.Join(this.tempDir, "ocsp"))
this.Require().NoError(err, "deleting OCSP tempfile")
this.fakeOCSP, err = FakeOCSPResponse(time.Now().Add(-4 * 24 * time.Hour))
this.Require().NoError(err, "creating stale OCSP response")
this.Require().True(this.ocspServerCalled(func() {
this.handler, err = this.New()
this.Require().NoError(err, "reinstantiating CertCache")
}))

// Prime disk cache with a bad OCSP:
freshOCSP := []byte("0xdeadbeef")
this.fakeOCSP = freshOCSP
// freshOCSP, err := FakeOCSPResponse(time.Now())
// this.Require().NoError(err, "creating fresh OCSP response")
err = ioutil.WriteFile(filepath.Join(this.tempDir, "ocsp"), freshOCSP, 0644)
this.Require().NoError(err, "writing fresh OCSP response to disk")
err = ioutil.WriteFile(filepath.Join(this.tempDir, "ocsp"), freshOCSP, 0644)
this.Require().NoError(err, "writing fresh OCSP response to disk")

// On update, verify network is not called (fresh OCSP from disk is used):
this.Assert().True(this.ocspServerCalled(func() {
this.handler.readOCSP()
}))
// On update, verify network is not called (fresh OCSP from disk is used):
this.Assert().True(this.ocspServerCalled(func() {
this.handler.readOCSP()
}))

this.Assert().False(this.handler.IsHealthy())
this.Assert().NotNil(this.handler.IsHealthy())
}

func (this *CertCacheSuite) TestServes404OnMissingCertificate() {
Expand Down
2 changes: 1 addition & 1 deletion packager/certcache/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import (
"runtime"
"sync"

"github.com/pkg/errors"
"github.com/gofrs/flock"
"github.com/pkg/errors"
)

// This is an abstraction over a single file on a remote storage mechanism. It
Expand Down
41 changes: 41 additions & 0 deletions packager/healthz/healthz.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Copyright 2019 Google LLC
//
// 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
//
// https://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 healthz

import (
"fmt"
"github.com/ampproject/amppackager/packager/certcache"
"net/http"
)

type Healthz struct {
certHandler certcache.CertHandler
}

func New(certHandler certcache.CertHandler) (*Healthz, error) {
return &Healthz{certHandler}, nil
}

func (this *Healthz) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
// Follow https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/
err := this.certHandler.IsHealthy()
if err != nil {
resp.WriteHeader(500)
resp.Write([]byte(fmt.Sprintf("not healthy: %v", err)))
} else {
resp.WriteHeader(200)
resp.Write([]byte("ok"))
}
}
64 changes: 64 additions & 0 deletions packager/healthz/healthz_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// Copyright 2019 Google LLC
//
// 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
//
// https://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 healthz

import (
"crypto/x509"
"net/http"
"testing"

"github.com/ampproject/amppackager/packager/mux"
"github.com/pkg/errors"

pkgt "github.com/ampproject/amppackager/packager/testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

type fakeHealthyCertHandler struct {
}

func (this fakeHealthyCertHandler) GetLatestCert() *x509.Certificate {
return pkgt.Certs[0]
}

func (this fakeHealthyCertHandler) IsHealthy() error {
return nil
}

type fakeNotHealthyCertHandler struct {
}

func (this fakeNotHealthyCertHandler) GetLatestCert() *x509.Certificate {
return pkgt.Certs[0]
}

func (this fakeNotHealthyCertHandler) IsHealthy() error {
return errors.New("random error")
}

func TestHealthzOk(t *testing.T) {
handler, err := New(fakeHealthyCertHandler{})
require.NoError(t, err)
resp := pkgt.Get(t, mux.New(nil, nil, nil, handler), "/healthz")
assert.Equal(t, http.StatusOK, resp.StatusCode, "ok", resp)
}

func TestHealthzFail(t *testing.T) {
handler, err := New(fakeNotHealthyCertHandler{})
require.NoError(t, err)
resp := pkgt.Get(t, mux.New(nil, nil, nil, handler), "/healthz")
assert.Equal(t, http.StatusInternalServerError, resp.StatusCode, "error", resp)
}
7 changes: 4 additions & 3 deletions packager/mux/mux.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,12 @@ type mux struct {
certCache http.Handler
signer http.Handler
validityMap http.Handler
healthz http.Handler
}

// The main entry point. Use the return value for http.Server.Handler.
func New(certCache http.Handler, signer http.Handler, validityMap http.Handler) http.Handler {
return &mux{certCache, signer, validityMap}
func New(certCache http.Handler, signer http.Handler, validityMap http.Handler, healthz http.Handler) http.Handler {
return &mux{certCache, signer, validityMap, healthz}
}

func tryTrimPrefix(s, prefix string) (string, bool) {
Expand Down Expand Up @@ -98,7 +99,7 @@ func (this *mux) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
this.certCache.ServeHTTP(resp, req)
}
} else if path == util.HealthzPath {
this.certCache.ServeHTTP(resp, req)
this.healthz.ServeHTTP(resp, req)
} else if path == util.ValidityMapPath {
this.validityMap.ServeHTTP(resp, req)
} else {
Expand Down
Loading

0 comments on commit 11d9918

Please sign in to comment.