Improve TLS storage provider errors

We renamed caddytls.ErrStorageNotFound to caddytls.ErrNotExist to more
closely mirror the os package. We changed it to an interface wrapper
so that the custom error message can be preserved. Returning only "data
not found" was useless in debugging because we couldn't know the
concrete value of the error (like what it was trying to load).

Users can do a type assertion to determine if the error value is a "not
found" error instead of doing an equality check.

caddytls/filestorage.go

@@ -1,12 +1,14 @@
 package caddytls
 
 import (
-	"github.com/mholt/caddy"
+	"fmt"
 	"io/ioutil"
 	"net/url"
 	"os"
 	"path/filepath"
 	"strings"
+
+	"github.com/mholt/caddy"
 )
 
 func init() {
@@ -114,13 +116,14 @@ func (s FileStorage) userKeyFile(email string) string {
 }
 
 // readFile abstracts a simple ioutil.ReadFile, making sure to return an
-// ErrStorageNotFound instance when the file is not found.
+// ErrNotExist instance when the file is not found.
 func (s FileStorage) readFile(file string) ([]byte, error) {
-	byts, err := ioutil.ReadFile(file)
+	b, err := ioutil.ReadFile(file)
 	if os.IsNotExist(err) {
-		return nil, ErrStorageNotFound
+		return nil, ErrNotExist(err)
 	}
-	return byts, err
+	return b, err
+
 }
 
 // SiteExists implements Storage.SiteExists by checking for the presence of
@@ -141,18 +144,23 @@ func (s FileStorage) SiteExists(domain string) (bool, error) {
 }
 
 // LoadSite implements Storage.LoadSite by loading it from disk. If it is not
-// present, the ErrStorageNotFound error instance is returned.
+// present, an instance of ErrNotExist is returned.
 func (s FileStorage) LoadSite(domain string) (*SiteData, error) {
 	var err error
 	siteData := new(SiteData)
 	siteData.Cert, err = s.readFile(s.siteCertFile(domain))
-	if err == nil {
+	if err != nil {
+		return nil, err
+	}
 	siteData.Key, err = s.readFile(s.siteKeyFile(domain))
+	if err != nil {
+		return nil, err
 	}
-	if err == nil {
 	siteData.Meta, err = s.readFile(s.siteMetaFile(domain))
+	if err != nil {
+		return nil, err
 	}
-	return siteData, err
+	return siteData, nil
 }
 
 // StoreSite implements Storage.StoreSite by writing it to disk. The base
@@ -160,27 +168,34 @@ func (s FileStorage) LoadSite(domain string) (*SiteData, error) {
 func (s FileStorage) StoreSite(domain string, data *SiteData) error {
 	err := os.MkdirAll(s.site(domain), 0700)
 	if err != nil {
-		return err
+		return fmt.Errorf("making site directory: %v", err)
 	}
 	err = ioutil.WriteFile(s.siteCertFile(domain), data.Cert, 0600)
-	if err == nil {
+	if err != nil {
+		return fmt.Errorf("writing certificate file: %v", err)
+	}
 	err = ioutil.WriteFile(s.siteKeyFile(domain), data.Key, 0600)
+	if err != nil {
+		return fmt.Errorf("writing key file: %v", err)
 	}
-	if err == nil {
 	err = ioutil.WriteFile(s.siteMetaFile(domain), data.Meta, 0600)
+	if err != nil {
+		return fmt.Errorf("writing cert meta file: %v", err)
 	}
-	return err
+	return nil
 }
 
 // DeleteSite implements Storage.DeleteSite by deleting just the cert from
-// disk. If it is not present, the ErrStorageNotFound error instance is
+// disk. If it is not present, an instance of ErrNotExist is returned.
-// returned.
 func (s FileStorage) DeleteSite(domain string) error {
 	err := os.Remove(s.siteCertFile(domain))
+	if err != nil {
 		if os.IsNotExist(err) {
-		return ErrStorageNotFound
+			return ErrNotExist(err)
 		}
 		return err
+	}
+	return nil
 }
 
 // LockRegister implements Storage.LockRegister by just returning true because
@@ -196,15 +211,19 @@ func (s FileStorage) UnlockRegister(domain string) error {
 }
 
 // LoadUser implements Storage.LoadUser by loading it from disk. If it is not
-// present, the ErrStorageNotFound error instance is returned.
+// present, an instance of ErrNotExist is returned.
 func (s FileStorage) LoadUser(email string) (*UserData, error) {
 	var err error
 	userData := new(UserData)
 	userData.Reg, err = s.readFile(s.userRegFile(email))
-	if err == nil {
+	if err != nil {
-		userData.Key, err = s.readFile(s.userKeyFile(email))
+		return nil, err
 	}
-	return userData, err
+	userData.Key, err = s.readFile(s.userKeyFile(email))
+	if err != nil {
+		return nil, err
+	}
+	return userData, nil
 }
 
 // StoreUser implements Storage.StoreUser by writing it to disk. The base
@@ -212,13 +231,17 @@ func (s FileStorage) LoadUser(email string) (*UserData, error) {
 func (s FileStorage) StoreUser(email string, data *UserData) error {
 	err := os.MkdirAll(s.user(email), 0700)
 	if err != nil {
-		return err
+		return fmt.Errorf("making user directory: %v", err)
 	}
 	err = ioutil.WriteFile(s.userRegFile(email), data.Reg, 0600)
-	if err == nil {
+	if err != nil {
-		err = ioutil.WriteFile(s.userKeyFile(email), data.Key, 0600)
+		return fmt.Errorf("writing user registration file: %v", err)
 	}
-	return err
+	err = ioutil.WriteFile(s.userKeyFile(email), data.Key, 0600)
+	if err != nil {
+		return fmt.Errorf("writing user key file: %v", err)
+	}
+	return nil
 }
 
 // MostRecentUserEmail implements Storage.MostRecentUserEmail by finding the

caddytls/storage.go

@@ -1,13 +1,6 @@
 package caddytls
 
-import (
+import "net/url"
-	"errors"
-	"net/url"
-)
-
-// ErrStorageNotFound is returned by Storage implementations when data is
-// expected to be present but is not.
-var ErrStorageNotFound = errors.New("data not found")
 
 // StorageCreator is a function type that is used in the Config to instantiate
 // a new Storage instance. This function can return a nil Storage even without
@@ -42,10 +35,10 @@ type Storage interface {
 	SiteExists(domain string) (bool, error)
 
 	// LoadSite obtains the site data from storage for the given domain and
-	// returns it. If data for the domain does not exist, the
+	// returns it. If data for the domain does not exist, an error value
-	// ErrStorageNotFound error instance is returned. For multi-server
+	// of type ErrNotExist is returned. For multi-server storage, care
-	// storage, care should be taken to make this load atomic to prevent
+	// should be taken to make this load atomic to prevent race conditions
-	// race conditions that happen with multiple data loads.
+	// that happen with multiple data loads.
 	LoadSite(domain string) (*SiteData, error)
 
 	// StoreSite persists the given site data for the given domain in
@@ -58,8 +51,7 @@ type Storage interface {
 
 	// DeleteSite deletes the site for the given domain from storage.
 	// Multi-server implementations should attempt to make this atomic. If
-	// the site does not exist, the ErrStorageNotFound error instance is
+	// the site does not exist, an error value of type ErrNotExist is returned.
-	// returned.
 	DeleteSite(domain string) error
 
 	// LockRegister is called before Caddy attempts to obtain or renew a
@@ -86,10 +78,10 @@ type Storage interface {
 	UnlockRegister(domain string) error
 
 	// LoadUser obtains user data from storage for the given email and
-	// returns it. If data for the email does not exist, the
+	// returns it. If data for the email does not exist, an error value
-	// ErrStorageNotFound error instance is returned. Multi-server
+	// of type ErrNotExist is returned. Multi-server implementations
-	// implementations should take care to make this operation atomic for
+	// should take care to make this operation atomic for all loaded
-	// all loaded data items.
+	// data items.
 	LoadUser(email string) (*UserData, error)
 
 	// StoreUser persists the given user data for the given email in
@@ -101,4 +93,10 @@ type Storage interface {
 	// in StoreUser. The result is an empty string if there are no
 	// persisted users in storage.
 	MostRecentUserEmail() string
+
+// ErrNotExist is returned by Storage implementations when
+// a resource is not found. It is similar to os.ErrNotExist
+// except this is a type, not a variable.
+type ErrNotExist interface {
+	error
 }

caddytls/storagetest/memorystorage.go

@@ -1,9 +1,11 @@
 package storagetest
 
 import (
-	"github.com/mholt/caddy/caddytls"
+	"errors"
 	"net/url"
 	"sync"
+
+	"github.com/mholt/caddy/caddytls"
 )
 
 // memoryMutex is a mutex used to control access to memoryStoragesByCAURL.
@@ -65,7 +67,7 @@ func (s *InMemoryStorage) Clear() {
 func (s *InMemoryStorage) LoadSite(domain string) (*caddytls.SiteData, error) {
 	siteData, ok := s.Sites[domain]
 	if !ok {
-		return nil, caddytls.ErrStorageNotFound
+		return nil, caddytls.ErrNotExist(errors.New("not found"))
 	}
 	return siteData, nil
 }
@@ -89,7 +91,7 @@ func (s *InMemoryStorage) StoreSite(domain string, data *caddytls.SiteData) erro
 // DeleteSite implements caddytls.Storage.DeleteSite in memory.
 func (s *InMemoryStorage) DeleteSite(domain string) error {
 	if _, ok := s.Sites[domain]; !ok {
-		return caddytls.ErrStorageNotFound
+		return caddytls.ErrNotExist(errors.New("not found"))
 	}
 	delete(s.Sites, domain)
 	return nil
@@ -111,7 +113,7 @@ func (s *InMemoryStorage) UnlockRegister(domain string) error {
 func (s *InMemoryStorage) LoadUser(email string) (*caddytls.UserData, error) {
 	userData, ok := s.Users[email]
 	if !ok {
-		return nil, caddytls.ErrStorageNotFound
+		return nil, caddytls.ErrNotExist(errors.New("not found"))
 	}
 	return userData, nil
 }

caddytls/storagetest/storagetest.go

@@ -6,8 +6,9 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
-	"github.com/mholt/caddy/caddytls"
 	"testing"
+
+	"github.com/mholt/caddy/caddytls"
 )
 
 // StorageTest is a test harness that contains tests to execute all exposed
@@ -160,13 +161,15 @@ func (s *StorageTest) TestSite() error {
 	defer s.runPostTest()
 
 	// Should be a not-found error at first
-	if _, err := s.LoadSite("example.com"); err != caddytls.ErrStorageNotFound {
+	_, err := s.LoadSite("example.com")
-		return fmt.Errorf("Expected ErrStorageNotFound from load, got: %v", err)
+	if _, ok := err.(caddytls.ErrNotExist); !ok {
+		return fmt.Errorf("Expected caddytls.ErrNotExist from load, got %T: %v", err, err)
 	}
 
 	// Delete should also be a not-found error at first
-	if err := s.DeleteSite("example.com"); err != caddytls.ErrStorageNotFound {
+	err = s.DeleteSite("example.com")
-		return fmt.Errorf("Expected ErrStorageNotFound from delete, got: %v", err)
+	if _, ok := err.(caddytls.ErrNotExist); !ok {
+		return fmt.Errorf("Expected ErrNotExist from delete, got: %v", err)
 	}
 
 	// Should store successfully and then load just fine
@@ -197,8 +200,9 @@ func (s *StorageTest) TestSite() error {
 	if err := s.DeleteSite("example.com"); err != nil {
 		return err
 	}
-	if _, err := s.LoadSite("example.com"); err != caddytls.ErrStorageNotFound {
+	_, err = s.LoadSite("example.com")
-		return fmt.Errorf("Expected ErrStorageNotFound after delete, got: %v", err)
+	if _, ok := err.(caddytls.ErrNotExist); !ok {
+		return fmt.Errorf("Expected caddytls.ErrNotExist after delete, got %T: %v", err, err)
 	}
 
 	return nil
@@ -221,8 +225,9 @@ func (s *StorageTest) TestUser() error {
 	defer s.runPostTest()
 
 	// Should be a not-found error at first
-	if _, err := s.LoadUser("foo@example.com"); err != caddytls.ErrStorageNotFound {
+	_, err := s.LoadUser("foo@example.com")
-		return fmt.Errorf("Expected ErrStorageNotFound from load, got: %v", err)
+	if _, ok := err.(caddytls.ErrNotExist); !ok {
+		return fmt.Errorf("Expected caddytls.ErrNotExist from load, got %T: %v", err, err)
 	}
 
 	// Should store successfully and then load just fine

caddytls/user.go

@@ -103,7 +103,7 @@ func getUser(storage Storage, email string) (User, error) {
 	// open user reg
 	userData, err := storage.LoadUser(email)
 	if err != nil {
-		if err == ErrStorageNotFound {
+		if _, ok := err.(ErrNotExist); ok {
 			// create a new user
 			return newUser(email)
 		}