diff --git a/caddytls/filestorage.go b/caddytls/filestorage.go index 4e9d3f5c..f5487a05 100644 --- a/caddytls/filestorage.go +++ b/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 { - 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)) + siteData.Key, err = s.readFile(s.siteKeyFile(domain)) + if err != nil { + return nil, err } - return siteData, err + siteData.Meta, err = s.readFile(s.siteMetaFile(domain)) + if err != nil { + return nil, 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 { - err = ioutil.WriteFile(s.siteKeyFile(domain), data.Key, 0600) + if err != nil { + return fmt.Errorf("writing certificate file: %v", err) } - if err == nil { - err = ioutil.WriteFile(s.siteMetaFile(domain), data.Meta, 0600) + err = ioutil.WriteFile(s.siteKeyFile(domain), data.Key, 0600) + if err != nil { + return fmt.Errorf("writing key file: %v", err) } - return err + err = ioutil.WriteFile(s.siteMetaFile(domain), data.Meta, 0600) + if err != nil { + return fmt.Errorf("writing cert meta file: %v", err) + } + return nil } // DeleteSite implements Storage.DeleteSite by deleting just the cert from -// disk. If it is not present, the ErrStorageNotFound error instance is -// returned. +// disk. If it is not present, an instance of ErrNotExist is returned. func (s FileStorage) DeleteSite(domain string) error { err := os.Remove(s.siteCertFile(domain)) - if os.IsNotExist(err) { - return ErrStorageNotFound + if err != nil { + if os.IsNotExist(err) { + return ErrNotExist(err) + } + return 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 { - userData.Key, err = s.readFile(s.userKeyFile(email)) + if err != nil { + 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 { - err = ioutil.WriteFile(s.userKeyFile(email), data.Key, 0600) + if err != nil { + 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 diff --git a/caddytls/storage.go b/caddytls/storage.go index d2c1ff65..548fcb48 100644 --- a/caddytls/storage.go +++ b/caddytls/storage.go @@ -1,13 +1,6 @@ package caddytls -import ( - "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") +import "net/url" // 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 - // ErrStorageNotFound error instance is returned. For multi-server - // storage, care should be taken to make this load atomic to prevent - // race conditions that happen with multiple data loads. + // returns it. If data for the domain does not exist, an error value + // of type ErrNotExist is returned. For multi-server storage, care + // should be taken to make this load atomic to prevent race conditions + // 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 - // returned. + // the site does not exist, an error value of type ErrNotExist is 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 - // ErrStorageNotFound error instance is returned. Multi-server - // implementations should take care to make this operation atomic for - // all loaded data items. + // returns it. If data for the email does not exist, an error value + // of type ErrNotExist is returned. Multi-server implementations + // should take care to make this operation atomic for all loaded + // 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 } diff --git a/caddytls/storagetest/memorystorage.go b/caddytls/storagetest/memorystorage.go index 138342d5..f83e9dd5 100644 --- a/caddytls/storagetest/memorystorage.go +++ b/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 } diff --git a/caddytls/storagetest/storagetest.go b/caddytls/storagetest/storagetest.go index 9c11d77b..c759dee8 100644 --- a/caddytls/storagetest/storagetest.go +++ b/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 { - return fmt.Errorf("Expected ErrStorageNotFound from load, got: %v", err) + _, err := s.LoadSite("example.com") + 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 { - return fmt.Errorf("Expected ErrStorageNotFound from delete, got: %v", err) + err = s.DeleteSite("example.com") + 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 { - return fmt.Errorf("Expected ErrStorageNotFound after delete, got: %v", err) + _, err = s.LoadSite("example.com") + 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 { - return fmt.Errorf("Expected ErrStorageNotFound from load, got: %v", err) + _, err := s.LoadUser("foo@example.com") + 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 diff --git a/caddytls/user.go b/caddytls/user.go index 40f484d6..3b6f83ef 100644 --- a/caddytls/user.go +++ b/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) }