Add NotFound error handling to NMAgent client (#2163)

This PR makes some minor changes to the NMAgent client package as a part of a larger work item. This commit adds a NotFound method to the error returned by the NMAgent client. It also adds special handling to treat a 400 BadRequest as a NotFound when returned by the DeleteNetwork API call, since this case should be interpreted as a NotFound by the caller.

Author: debecerra

nmagent/client.go

@@ -69,7 +69,7 @@ func (c *Client) JoinNetwork(ctx context.Context, jnr JoinNetworkRequest) error
 		defer resp.Body.Close()
 
 		if resp.StatusCode != http.StatusOK {
-			return die(resp.StatusCode, resp.Header, resp.Body)
+			return die(resp.StatusCode, resp.Header, resp.Body, req.URL.Path)
 		}
 		return nil
 	})
@@ -91,7 +91,7 @@ func (c *Client) DeleteNetwork(ctx context.Context, dnr DeleteNetworkRequest) er
 	defer resp.Body.Close()
 
 	if resp.StatusCode != http.StatusOK {
-		return die(resp.StatusCode, resp.Header, resp.Body)
+		return die(resp.StatusCode, resp.Header, resp.Body, req.URL.Path)
 	}
 	return nil
 }
@@ -114,7 +114,7 @@ func (c *Client) GetNetworkConfiguration(ctx context.Context, gncr GetNetworkCon
 		defer resp.Body.Close()
 
 		if resp.StatusCode != http.StatusOK {
-			return die(resp.StatusCode, resp.Header, resp.Body)
+			return die(resp.StatusCode, resp.Header, resp.Body, req.URL.Path)
 		}
 
 		ct := resp.Header.Get(internal.HeaderContentType)
@@ -152,7 +152,7 @@ func (c *Client) GetNCVersion(ctx context.Context, ncvr NCVersionRequest) (NCVer
 	defer resp.Body.Close()
 
 	if resp.StatusCode != http.StatusOK {
-		return NCVersion{}, die(resp.StatusCode, resp.Header, resp.Body)
+		return NCVersion{}, die(resp.StatusCode, resp.Header, resp.Body, req.URL.Path)
 	}
 
 	var out NCVersion
@@ -179,7 +179,7 @@ func (c *Client) PutNetworkContainer(ctx context.Context, pncr *PutNetworkContai
 	defer resp.Body.Close()
 
 	if resp.StatusCode != http.StatusOK {
-		return die(resp.StatusCode, resp.Header, resp.Body)
+		return die(resp.StatusCode, resp.Header, resp.Body, req.URL.Path)
 	}
 	return nil
 }
@@ -223,7 +223,7 @@ func (c *Client) DeleteNetworkContainer(ctx context.Context, dcr DeleteContainer
 	defer resp.Body.Close()
 
 	if resp.StatusCode != http.StatusOK {
-		return die(resp.StatusCode, resp.Header, resp.Body)
+		return die(resp.StatusCode, resp.Header, resp.Body, req.URL.Path)
 	}
 
 	return nil
@@ -242,7 +242,7 @@ func (c *Client) GetNCVersionList(ctx context.Context) (NCVersionList, error) {
 	defer resp.Body.Close()
 
 	if resp.StatusCode != http.StatusOK {
-		return NCVersionList{}, die(resp.StatusCode, resp.Header, resp.Body)
+		return NCVersionList{}, die(resp.StatusCode, resp.Header, resp.Body, req.URL.Path)
 	}
 
 	var out NCVersionList
@@ -270,7 +270,7 @@ func (c *Client) GetHomeAz(ctx context.Context) (AzResponse, error) {
 	defer resp.Body.Close()
 
 	if resp.StatusCode != http.StatusOK {
-		return homeAzResponse, die(resp.StatusCode, resp.Header, resp.Body)
+		return homeAzResponse, die(resp.StatusCode, resp.Header, resp.Body, req.URL.Path)
 	}
 
 	err = json.NewDecoder(resp.Body).Decode(&homeAzResponse)
@@ -281,7 +281,7 @@ func (c *Client) GetHomeAz(ctx context.Context) (AzResponse, error) {
 	return homeAzResponse, nil
 }
 
-func die(code int, headers http.Header, body io.ReadCloser) error {
+func die(code int, headers http.Header, body io.ReadCloser, path string) error {
 	// nolint:errcheck // make a best effort to return whatever information we can
 	// returning an error here without the code and source would
 	// be less helpful
@@ -292,6 +292,7 @@ func die(code int, headers http.Header, body io.ReadCloser) error {
 		// consumers to depend on an internal type (which they can't anyway)
 		Source: internal.GetErrorSource(headers).String(),
 		Body:   bodyContent,
+		Path:   path,
 	}
 }
 

nmagent/client_test.go

@@ -124,32 +124,44 @@ func TestNMAgentClientJoinNetworkRetry(t *testing.T) {
 
 func TestNMAgentClientDeleteNetwork(t *testing.T) {
 	deleteNetTests := []struct {
-		name       string
-		id         string
-		exp        string
-		respStatus int
-		shouldErr  bool
+		name           string
+		id             string
+		exp            string
+		respStatus     int
+		shouldErr      bool
+		shouldNotFound bool
 	}{
 		{
 			"happy path",
 			"00000000-0000-0000-0000-000000000000",
 			"/machine/plugins?comp=nmagent&type=NetworkManagement%2FjoinedVirtualNetworks%2F00000000-0000-0000-0000-000000000000%2Fapi-version%2F1%2Fmethod%2FDELETE",
 			http.StatusOK,
 			false,
+			false,
 		},
 		{
 			"empty network ID",
 			"",
 			"",
 			http.StatusOK, // this shouldn't be checked
 			true,
+			false,
 		},
 		{
 			"internal error",
 			"00000000-0000-0000-0000-000000000000",
 			"/machine/plugins?comp=nmagent&type=NetworkManagement%2FjoinedVirtualNetworks%2F00000000-0000-0000-0000-000000000000%2Fapi-version%2F1%2Fmethod%2FDELETE",
 			http.StatusInternalServerError,
 			true,
+			false,
+		},
+		{
+			"network does not exist",
+			"00000000-0000-0000-0000-000000000000",
+			"/machine/plugins?comp=nmagent&type=NetworkManagement%2FjoinedVirtualNetworks%2F00000000-0000-0000-0000-000000000000%2Fapi-version%2F1%2Fmethod%2FDELETE",
+			http.StatusBadRequest,
+			true,
+			true,
 		},
 	}
 
@@ -177,6 +189,12 @@ func TestNMAgentClientDeleteNetwork(t *testing.T) {
 			err := client.DeleteNetwork(ctx, nmagent.DeleteNetworkRequest{test.id})
 			checkErr(t, err, test.shouldErr)
 
+			var nmaError nmagent.Error
+			errors.As(err, &nmaError)
+			if nmaError.NotFound() != test.shouldNotFound {
+				t.Error("unexpected NotFound value: got:", nmaError.NotFound(), "exp:", test.shouldNotFound)
+			}
+
 			if got != test.exp {
 				t.Error("received URL differs from expectation: got", got, "exp:", test.exp)
 			}

nmagent/error.go

@@ -5,11 +5,14 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"regexp"
 
 	"github.com/Azure/azure-container-networking/nmagent/internal"
 	pkgerrors "github.com/pkg/errors"
 )
 
+var deleteNetworkPattern = regexp.MustCompile(`/NetworkManagement/joinedVirtualNetworks/[^/]+/api-version/\d+/method/DELETE`)
+
 // ContentError is encountered when an unexpected content type is obtained from
 // NMAgent.
 type ContentError struct {
@@ -52,6 +55,7 @@ type Error struct {
 	Code   int    // the HTTP status code received
 	Source string // the component responsible for producing the error
 	Body   []byte // the body of the error returned
+	Path   string // the path of the request that produced the error
 }
 
 // Error constructs a string representation of this error in accordance with
@@ -103,3 +107,12 @@ func (e Error) StatusCode() int {
 func (e Error) Unauthorized() bool {
 	return e.Code == http.StatusUnauthorized
 }
+
+// NotFound reports whether the error was produced as a result of the resource
+// not existing.
+func (e Error) NotFound() bool {
+	if deleteNetworkPattern.MatchString(e.Path) {
+		return e.Code == http.StatusBadRequest || e.Code == http.StatusNotFound
+	}
+	return e.Code == http.StatusNotFound
+}