Better support for retrying failed requests with the API

Also implements more logic error returns from the Get/Post functions in the client, rather than making the developer call r.Error() on responses.

remote/errors.go

@@ -3,6 +3,8 @@ package remote
 import (
 	"fmt"
 	"net/http"
+
+	"emperror.dev/errors"
 )
 
 type RequestErrors struct {
@@ -16,13 +18,31 @@ type RequestError struct {
 	Detail   string `json:"detail"`
 }
 
+// IsRequestError checks if the given error is of the RequestError type.
 func IsRequestError(err error) bool {
-	_, ok := err.(*RequestError)
+	var rerr *RequestError
-
+	if err == nil {
-	return ok
+		return false
+	}
+	return errors.As(err, &rerr)
 }
 
-// Returns the error response in a string form that can be more easily consumed.
+// AsRequestError transforms the error into a RequestError if it is currently
+// one, checking the wrap status from the other error handlers. If the error
+// is not a RequestError nil is returned.
+func AsRequestError(err error) *RequestError {
+	if err == nil {
+		return nil
+	}
+	var rerr *RequestError
+	if errors.As(err, &rerr) {
+		return rerr
+	}
+	return nil
+}
+
+// Error returns the error response in a string form that can be more easily
+// consumed.
 func (re *RequestError) Error() string {
 	c := 0
 	if re.response != nil {
@@ -32,6 +52,11 @@ func (re *RequestError) Error() string {
 	return fmt.Sprintf("Error response from Panel: %s: %s (HTTP/%d)", re.Code, re.Detail, c)
 }
 
+// StatusCode returns the status code of the response.
+func (re *RequestError) StatusCode() int {
+	return re.response.StatusCode
+}
+
 type SftpInvalidCredentialsError struct {
 }
 

remote/http.go

@@ -97,7 +97,7 @@ func (c *client) Post(ctx context.Context, path string, data interface{}) (*Resp
 // over this method when possible. It appends the path to the endpoint of the
 // client and adds the authentication token to the request.
 func (c *client) requestOnce(ctx context.Context, method, path string, body io.Reader, opts ...func(r *http.Request)) (*Response, error) {
-	req, err := http.NewRequest(method, c.baseUrl+path, body)
+	req, err := http.NewRequestWithContext(ctx, method, c.baseUrl+path, body)
 	if err != nil {
 		return nil, err
 	}
@@ -114,7 +114,7 @@ func (c *client) requestOnce(ctx context.Context, method, path string, body io.R
 
 	debugLogRequest(req)
 
-	res, err := c.httpClient.Do(req.WithContext(ctx))
+	res, err := c.httpClient.Do(req)
 	return &Response{res}, err
 }
 
@@ -140,10 +140,14 @@ func (c *client) request(ctx context.Context, method, path string, body io.Reade
 		}
 		res = r
 		if r.HasError() {
+			// Close the request body after returning the error to free up resources.
+			defer r.Body.Close()
 			// Don't keep spamming the endpoint if we've already made too many requests or
 			// if we're not even authenticated correctly. Retrying generally won't fix either
 			// of these issues.
-			if r.StatusCode == http.StatusTooManyRequests || r.StatusCode == http.StatusUnauthorized {
+			if r.StatusCode == http.StatusForbidden ||
+				r.StatusCode == http.StatusTooManyRequests ||
+				r.StatusCode == http.StatusUnauthorized {
 				return backoff.Permanent(r.Error())
 			}
 			return r.Error()
@@ -151,10 +155,6 @@ func (c *client) request(ctx context.Context, method, path string, body io.Reade
 		return nil
 	}, c.backoff(ctx))
 	if err != nil {
-		var rerr *RequestError
-		if errors.As(err, &rerr) {
-			return res, nil
-		}
 		if v, ok := err.(*backoff.PermanentError); ok {
 			return nil, v.Unwrap()
 		}
@@ -220,15 +220,12 @@ func (r *Response) HasError() bool {
 func (r *Response) Read() ([]byte, error) {
 	var b []byte
 	if r.Response == nil {
-		return nil, errors.New("http: attempting to read missing response")
+		return nil, errors.New("remote: attempting to read missing response")
 	}
-
 	if r.Response.Body != nil {
 		b, _ = ioutil.ReadAll(r.Response.Body)
 	}
-
 	r.Response.Body = ioutil.NopCloser(bytes.NewBuffer(b))
-
 	return b, nil
 }
 
@@ -240,9 +237,8 @@ func (r *Response) BindJSON(v interface{}) error {
 	if err != nil {
 		return err
 	}
-
 	if err := json.Unmarshal(b, &v); err != nil {
-		return errors.Wrap(err, "http: could not unmarshal response")
+		return errors.Wrap(err, "remote: could not unmarshal response")
 	}
 	return nil
 }
@@ -272,7 +268,7 @@ func (r *Response) Error() error {
 
 	e.response = r.Response
 
-	return e
+	return errors.WithStackDepth(e, 1)
 }
 
 // Logs the request into the debug log with all of the important request bits.

remote/servers.go

@@ -60,9 +60,9 @@ func (c *client) GetServers(ctx context.Context, limit int) ([]RawServerData, er
 func (c *client) ResetServersState(ctx context.Context) error {
 	res, err := c.Post(ctx, "/servers/reset", nil)
 	if err != nil {
-		return errors.WrapIf(err, "remote/servers: failed to reset server state on Panel")
+		return errors.WrapIf(err, "remote: failed to reset server state on Panel")
 	}
-	res.Body.Close()
+	_ = res.Body.Close()
 	return nil
 }
 
@@ -74,10 +74,6 @@ func (c *client) GetServerConfiguration(ctx context.Context, uuid string) (Serve
 	}
 	defer res.Body.Close()
 
-	if res.HasError() {
-		return config, res.Error()
-	}
-
 	err = res.BindJSON(&config)
 	return config, err
 }
@@ -89,10 +85,6 @@ func (c *client) GetInstallationScript(ctx context.Context, uuid string) (Instal
 	}
 	defer res.Body.Close()
 
-	if res.HasError() {
-		return InstallationScript{}, res.Error()
-	}
-
 	var config InstallationScript
 	err = res.BindJSON(&config)
 	return config, err
@@ -103,8 +95,8 @@ func (c *client) SetInstallationStatus(ctx context.Context, uuid string, success
 	if err != nil {
 		return err
 	}
-	defer resp.Body.Close()
+	_ = resp.Body.Close()
-	return resp.Error()
+	return nil
 }
 
 func (c *client) SetArchiveStatus(ctx context.Context, uuid string, successful bool) error {
@@ -112,8 +104,8 @@ func (c *client) SetArchiveStatus(ctx context.Context, uuid string, successful b
 	if err != nil {
 		return err
 	}
-	defer resp.Body.Close()
+	_ = resp.Body.Close()
-	return resp.Error()
+	return nil
 }
 
 func (c *client) SetTransferStatus(ctx context.Context, uuid string, successful bool) error {
@@ -125,8 +117,8 @@ func (c *client) SetTransferStatus(ctx context.Context, uuid string, successful
 	if err != nil {
 		return err
 	}
-	defer resp.Body.Close()
+	_ = resp.Body.Close()
-	return resp.Error()
+	return nil
 }
 
 // ValidateSftpCredentials makes a request to determine if the username and
@@ -138,27 +130,18 @@ func (c *client) ValidateSftpCredentials(ctx context.Context, request SftpAuthRe
 	var auth SftpAuthResponse
 	res, err := c.Post(ctx, "/sftp/auth", request)
 	if err != nil {
+		if err := AsRequestError(err); err != nil && (err.StatusCode() >= 400 && err.StatusCode() < 500) {
+			log.WithFields(log.Fields{"subsystem": "sftp", "username": request.User, "ip": request.IP}).Warn(err.Error())
+			return auth, &SftpInvalidCredentialsError{}
+		}
 		return auth, err
 	}
 	defer res.Body.Close()
 
-	e := res.Error()
+	if err := res.BindJSON(&auth); err != nil {
-	if e != nil {
-		if res.StatusCode >= 400 && res.StatusCode < 500 {
-			log.WithFields(log.Fields{
-				"subsystem": "sftp",
-				"username":  request.User,
-				"ip":        request.IP,
-			}).Warn(e.Error())
-
-			return auth, &SftpInvalidCredentialsError{}
-		}
-
-		return auth, errors.New(e.Error())
-	}
-
-	err = res.BindJSON(&auth)
 		return auth, err
+	}
+	return auth, nil
 }
 
 func (c *client) GetBackupRemoteUploadURLs(ctx context.Context, backup string, size int64) (BackupRemoteUploadResponse, error) {
@@ -168,13 +151,10 @@ func (c *client) GetBackupRemoteUploadURLs(ctx context.Context, backup string, s
 		return data, err
 	}
 	defer res.Body.Close()
-
+	if err := res.BindJSON(&data); err != nil {
-	if res.HasError() {
-		return data, res.Error()
-	}
-
-	err = res.BindJSON(&data)
 		return data, err
+	}
+	return data, nil
 }
 
 func (c *client) SetBackupStatus(ctx context.Context, backup string, data BackupRequest) error {
@@ -182,8 +162,8 @@ func (c *client) SetBackupStatus(ctx context.Context, backup string, data Backup
 	if err != nil {
 		return err
 	}
-	defer resp.Body.Close()
+	_ = resp.Body.Close()
-	return resp.Error()
+	return nil
 }
 
 // SendRestorationStatus triggers a request to the Panel to notify it that a
@@ -194,8 +174,8 @@ func (c *client) SendRestorationStatus(ctx context.Context, backup string, succe
 	if err != nil {
 		return err
 	}
-	defer resp.Body.Close()
+	_ = resp.Body.Close()
-	return resp.Error()
+	return nil
 }
 
 // getServersPaged returns a subset of servers from the Panel API using the
@@ -214,10 +194,6 @@ func (c *client) getServersPaged(ctx context.Context, page, limit int) ([]RawSer
 		return nil, r.Meta, err
 	}
 	defer res.Body.Close()
-
-	if res.HasError() {
-		return nil, r.Meta, res.Error()
-	}
 	if err := res.BindJSON(&r); err != nil {
 		return nil, r.Meta, err
 	}

server/install.go

@@ -90,13 +90,8 @@ func (s *Server) Reinstall() error {
 func (s *Server) internalInstall() error {
 	script, err := s.client.GetInstallationScript(s.Context(), s.Id())
 	if err != nil {
-		if !remote.IsRequestError(err) {
 		return err
 	}
-
-		return errors.New(err.Error())
-	}
-
 	p, err := NewInstallationProcess(s, &script)
 	if err != nil {
 		return err
@@ -535,19 +530,10 @@ func (ip *InstallationProcess) StreamOutput(ctx context.Context, id string) erro
 	return nil
 }
 
-// Makes a HTTP request to the Panel instance notifying it that the server has
+// SyncInstallState makes a HTTP request to the Panel instance notifying it that
-// completed the installation process, and what the state of the server is. A boolean
+// the server has completed the installation process, and what the state of the
-// value of "true" means everything was successful, "false" means something went
+// server is. A boolean value of "true" means everything was successful, "false"
-// wrong and the server must be deleted and re-created.
+// means something went wrong and the server must be deleted and re-created.
 func (s *Server) SyncInstallState(successful bool) error {
-	err := s.client.SetInstallationStatus(s.Context(), s.Id(), successful)
+	return s.client.SetInstallationStatus(s.Context(), s.Id(), successful)
-	if err != nil {
-		if !remote.IsRequestError(err) {
-			return err
-		}
-
-		return errors.New(err.Error())
-	}
-
-	return nil
 }

server/server.go

@@ -3,6 +3,7 @@ package server
 import (
 	"context"
 	"fmt"
+	"net/http"
 	"os"
 	"strings"
 	"sync"
@@ -152,17 +153,11 @@ func (s *Server) Log() *log.Entry {
 func (s *Server) Sync() error {
 	cfg, err := s.client.GetServerConfiguration(s.Context(), s.Id())
 	if err != nil {
-		if !remote.IsRequestError(err) {
+		if err := remote.AsRequestError(err); err != nil && err.StatusCode() == http.StatusNotFound {
-			return err
-		}
-
-		if err.(*remote.RequestError).Status == "404" {
 			return &serverDoesNotExist{}
 		}
-
+		return errors.WithStackIf(err)
-		return errors.New(err.Error())
 	}
-
 	return s.SyncWithConfiguration(cfg)
 }