reloader: Split data/error callbacks (#1704)

The reloader package currently discriminates between file reading errors (in the reloader package) and data parsing errors (in the client code). Because of this, errors in a hot-replaced policy file may go unnoticed.

The former design, a (data, error) callback is replaced with a data callback and an error callback. This allows for uniform reporting of errors sourced from the library and errors sourced from the user-provided parser.

https://github.com/letsencrypt/boulder/pull/1704

policy/policy-authority.go

@@ -71,20 +71,20 @@ type blacklistJSON struct {
 // SetHostnamePolicyFile will load the given policy file, returning error if it
 // fails. It will also start a reloader in case the file changes.
 func (pa *AuthorityImpl) SetHostnamePolicyFile(f string) error {
-	_, err := reloader.New(f, pa.loadHostnamePolicy)
+	_, err := reloader.New(f, pa.loadHostnamePolicy, pa.hostnamePolicyLoadError)
 	return err
 }
 
-func (pa *AuthorityImpl) loadHostnamePolicy(b []byte, err error) error {
-	if err != nil {
-		pa.log.Err(fmt.Sprintf("loading hostname policy: %s", err))
-		return err
-	}
+func (pa *AuthorityImpl) hostnamePolicyLoadError(err error) {
+	pa.log.Err(fmt.Sprintf("error loading hostname policy: %s", err))
+}
+
+func (pa *AuthorityImpl) loadHostnamePolicy(b []byte) error {
 	hash := sha256.Sum256(b)
 	pa.log.Info(fmt.Sprintf("loading hostname policy, sha256: %s",
 		hex.EncodeToString(hash[:])))
 	var bl blacklistJSON
-	err = json.Unmarshal(b, &bl)
+	err := json.Unmarshal(b, &bl)
 	if err != nil {
 		return err
 	}

reloader/reloader.go

@@ -29,7 +29,10 @@ func (r *Reloader) Stop() {
 // synchronously, so it is easy for the caller to check for errors and fail
 // fast. New will return an error if it occurs on the first load. Otherwise all
 // errors are sent to the callback.
-func New(filename string, callback func([]byte, error) error) (*Reloader, error) {
+func New(filename string, dataCallback func([]byte) error, errorCallback func(error)) (*Reloader, error) {
+	if errorCallback == nil {
+		errorCallback = func(e error) {}
+	}
 	fileInfo, err := os.Stat(filename)
 	if err != nil {
 		return nil, err
@@ -49,23 +52,28 @@ func New(filename string, callback func([]byte, error) error) (*Reloader, error)
 			case <-tickChan:
 				currentFileInfo, err := os.Stat(filename)
 				if err != nil {
-					callback(nil, err)
+					errorCallback(err)
 					continue
 				}
-				if currentFileInfo.ModTime().After(fileInfo.ModTime()) {
-					b, err := ioutil.ReadFile(filename)
-					if err != nil {
-						callback(nil, err)
-						continue
-					}
-					fileInfo = currentFileInfo
-					callback(b, nil)
+				if !currentFileInfo.ModTime().After(fileInfo.ModTime()) {
+					continue
+				}
+				b, err := ioutil.ReadFile(filename)
+				if err != nil {
+					errorCallback(err)
+					continue
+				}
+				fileInfo = currentFileInfo
+				err = dataCallback(b)
+				if err != nil {
+					errorCallback(err)
 				}
 			}
 		}
 	}
-	err = callback(b, nil)
+	err = dataCallback(b)
 	if err != nil {
+		tickerStop()
 		return nil, err
 	}
 	go loop()

reloader/reloader_test.go

@@ -9,13 +9,25 @@ import (
 	"time"
 )
 
-func noop([]byte, error) error {
+func noop([]byte) error {
 	return nil
 }
 
+func testErrCb(t *testing.T) func(error) {
+	return func(e error) {
+		t.Error(e)
+	}
+}
+
+func testFatalCb(t *testing.T) func(error) {
+	return func(e error) {
+		t.Fatal(e)
+	}
+}
+
 func TestNoStat(t *testing.T) {
 	filename := os.TempDir() + "/doesntexist.123456789"
-	_, err := New(filename, noop)
+	_, err := New(filename, noop, testErrCb(t))
 	if err == nil {
 		t.Fatalf("Expected New to return error when the file doesn't exist.")
 	}
@@ -25,7 +37,7 @@ func TestNoRead(t *testing.T) {
 	f, _ := ioutil.TempFile("", "test-no-read.txt")
 	defer os.Remove(f.Name())
 	f.Chmod(0) // no read permissions
-	_, err := New(f.Name(), noop)
+	_, err := New(f.Name(), noop, testErrCb(t))
 	if err == nil {
 		t.Fatalf("Expected New to return error when permission denied.")
 	}
@@ -34,9 +46,9 @@ func TestNoRead(t *testing.T) {
 func TestFirstError(t *testing.T) {
 	f, _ := ioutil.TempFile("", "test-first-error.txt")
 	defer os.Remove(f.Name())
-	_, err := New(f.Name(), func([]byte, error) error {
+	_, err := New(f.Name(), func([]byte) error {
 		return fmt.Errorf("i die")
-	})
+	}, testErrCb(t))
 	if err == nil {
 		t.Fatalf("Expected New to return error when the callback returned error the first time.")
 	}
@@ -45,9 +57,9 @@ func TestFirstError(t *testing.T) {
 func TestFirstSuccess(t *testing.T) {
 	f, _ := ioutil.TempFile("", "test-first-success.txt")
 	defer os.Remove(f.Name())
-	r, err := New(f.Name(), func([]byte, error) error {
+	r, err := New(f.Name(), func([]byte) error {
 		return nil
-	})
+	}, testErrCb(t))
 	if err != nil {
 		t.Errorf("Expected New to succeed, got %s", err)
 	}
@@ -82,14 +94,11 @@ func TestReload(t *testing.T) {
 
 	var bodies []string
 	reloads := make(chan []byte, 1)
-	r, err := New(filename, func(b []byte, err error) error {
-		if err != nil {
-			t.Fatalf("Got error in callback: %s", err)
-		}
+	r, err := New(filename, func(b []byte) error {
 		bodies = append(bodies, string(b))
 		reloads <- b
 		return nil
-	})
+	}, testFatalCb(t))
 	if err != nil {
 		t.Fatalf("Expected New to succeed, got %s", err)
 	}
@@ -144,9 +153,11 @@ func TestReloadFailure(t *testing.T) {
 	}
 
 	reloads := make(chan res, 1)
-	_, err := New(filename, func(b []byte, err error) error {
-		reloads <- res{b, err}
+	_, err := New(filename, func(b []byte) error {
+		reloads <- res{b, nil}
 		return nil
+	}, func(e error) {
+		reloads <- res{nil, e}
 	})
 	if err != nil {
 		t.Fatalf("Expected New to succeed.")