Fix inability to unmarshal nmagent request (#1695)

* Fix inability to unmarshal nmagent request

During the previous switchover to using the client from the
`nmagent` client (as opposed to the `cns/nmagent` client), an assumption
was made that "proxied" requests to NMAgent were provided by clients as
nested JSON. This assumption was wrong--they are Base64-encoded strings
of JSON. Even though they're ultimately similar, they're very different
from the perspective of the JSON unmarshaler. Consequently, this
restores the nested request body back to a []byte and performs the
second-stage decoding manually (similarly to how it was previously
done).

Fixes #1694

* Fix swallowed error when body is not JSON

In one instance, an error was accidentally swallowed because the
existing code does not return errors (it sets variables instead). This
makes controlling the flow of execution difficult. To fix this, the
offending code has been moved to a separate function where returns can
be used effectively.

Author: timraymond

cns/NetworkContainerContract.go

@@ -8,7 +8,6 @@ import (
 	"strings"
 
 	"github.com/Azure/azure-container-networking/cns/types"
-	"github.com/Azure/azure-container-networking/nmagent"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 )
@@ -497,7 +496,7 @@ type PublishNetworkContainerRequest struct {
 	NetworkContainerID                string
 	JoinNetworkURL                    string
 	CreateNetworkContainerURL         string
-	CreateNetworkContainerRequestBody nmagent.PutNetworkContainerRequest
+	CreateNetworkContainerRequestBody []byte
 }
 
 // NetworkContainerParameters parameters available in network container operations

cns/client/client_test.go

@@ -24,7 +24,6 @@ import (
 	"github.com/Azure/azure-container-networking/cns/types"
 	"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
 	"github.com/Azure/azure-container-networking/log"
-	"github.com/Azure/azure-container-networking/nmagent"
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/uuid"
 	"github.com/stretchr/testify/assert"
@@ -1033,14 +1032,14 @@ func TestPublishNC(t *testing.T) {
 				NetworkContainerID:                "frob",
 				JoinNetworkURL:                    "http://example.com",
 				CreateNetworkContainerURL:         "http://example.com",
-				CreateNetworkContainerRequestBody: nmagent.PutNetworkContainerRequest{},
+				CreateNetworkContainerRequestBody: []byte("{}"),
 			},
 			&cns.PublishNetworkContainerRequest{
 				NetworkID:                         "foo",
 				NetworkContainerID:                "frob",
 				JoinNetworkURL:                    "http://example.com",
 				CreateNetworkContainerURL:         "http://example.com",
-				CreateNetworkContainerRequestBody: nmagent.PutNetworkContainerRequest{},
+				CreateNetworkContainerRequestBody: []byte("{}"),
 			},
 			false,
 		},
@@ -1062,14 +1061,14 @@ func TestPublishNC(t *testing.T) {
 				NetworkContainerID:                "frob",
 				JoinNetworkURL:                    "http://example.com",
 				CreateNetworkContainerURL:         "http://example.com",
-				CreateNetworkContainerRequestBody: nmagent.PutNetworkContainerRequest{},
+				CreateNetworkContainerRequestBody: []byte("{}"),
 			},
 			&cns.PublishNetworkContainerRequest{
 				NetworkID:                         "foo",
 				NetworkContainerID:                "frob",
 				JoinNetworkURL:                    "http://example.com",
 				CreateNetworkContainerURL:         "http://example.com",
-				CreateNetworkContainerRequestBody: nmagent.PutNetworkContainerRequest{},
+				CreateNetworkContainerRequestBody: []byte("{}"),
 			},
 			true,
 		},

cns/restserver/api.go

@@ -5,6 +5,7 @@ package restserver
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"net"
 	"net/http"
@@ -18,7 +19,7 @@ import (
 	"github.com/Azure/azure-container-networking/cns/logger"
 	"github.com/Azure/azure-container-networking/cns/types"
 	"github.com/Azure/azure-container-networking/cns/wireserver"
-	nma "github.com/Azure/azure-container-networking/nmagent"
+	"github.com/Azure/azure-container-networking/nmagent"
 	"github.com/Azure/azure-container-networking/platform"
 	"github.com/pkg/errors"
 )
@@ -1110,6 +1111,34 @@ func getAuthTokenAndInterfaceIDFromNcURL(networkContainerURL string) (*cns.Netwo
 	return &cns.NetworkContainerParameters{AssociatedInterfaceID: matches[1], AuthToken: matches[3]}, nil
 }
 
+//nolint:revive // the previous receiver naming "service" is bad, this is correct:
+func (h *HTTPRestService) doPublish(ctx context.Context, req cns.PublishNetworkContainerRequest, ncParameters *cns.NetworkContainerParameters) (string, types.ResponseCode) {
+	innerReqBytes := req.CreateNetworkContainerRequestBody
+
+	var innerReq nmagent.PutNetworkContainerRequest
+	err := json.Unmarshal(innerReqBytes, &innerReq)
+	if err != nil {
+		returnMessage := fmt.Sprintf("Failed to publish Network Container: %s", req.NetworkContainerID)
+		returnCode := types.NetworkContainerPublishFailed
+		logger.Errorf("[Azure-CNS] %s", returnMessage)
+		return returnMessage, returnCode
+	}
+
+	innerReq.AuthenticationToken = ncParameters.AuthToken
+	innerReq.PrimaryAddress = ncParameters.AssociatedInterfaceID
+
+	err = h.nma.PutNetworkContainer(ctx, &innerReq)
+	// nolint:bodyclose // existing code needs refactoring
+	if err != nil {
+		returnMessage := fmt.Sprintf("Failed to publish Network Container: %s", req.NetworkContainerID)
+		returnCode := types.NetworkContainerPublishFailed
+		logger.Errorf("[Azure-CNS] %s", returnMessage)
+		return returnMessage, returnCode
+	}
+
+	return "", types.Success
+}
+
 // Publish Network Container by calling nmagent
 func (service *HTTPRestService) publishNetworkContainer(w http.ResponseWriter, r *http.Request) {
 	logger.Printf("[Azure-CNS] PublishNetworkContainer")
@@ -1177,7 +1206,7 @@ func (service *HTTPRestService) publishNetworkContainer(w http.ResponseWriter, r
 			returnCode = types.NetworkJoinFailed
 			publishErrorStr = err.Error()
 
-			var nmaErr nma.Error
+			var nmaErr nmagent.Error
 			if errors.As(err, &nmaErr) {
 				publishStatusCode = nmaErr.StatusCode()
 			}
@@ -1187,21 +1216,10 @@ func (service *HTTPRestService) publishNetworkContainer(w http.ResponseWriter, r
 
 		if isNetworkJoined {
 			// Publish Network Container
-
-			pncr := req.CreateNetworkContainerRequestBody
-			pncr.AuthenticationToken = ncParameters.AuthToken
-			pncr.PrimaryAddress = ncParameters.AssociatedInterfaceID
-
-			err = service.nma.PutNetworkContainer(ctx, &pncr)
-			// nolint:bodyclose // existing code needs refactoring
-			if err != nil {
-				returnMessage = fmt.Sprintf("Failed to publish Network Container: %s", req.NetworkContainerID)
-				returnCode = types.NetworkContainerPublishFailed
-				logger.Errorf("[Azure-CNS] %s", returnMessage)
-			}
+			returnMessage, returnCode = service.doPublish(ctx, req, ncParameters)
 		}
 
-		req := nma.NCVersionRequest{
+		req := nmagent.NCVersionRequest{
 			AuthToken:          ncParameters.AuthToken,
 			NetworkContainerID: req.NetworkContainerID,
 			PrimaryAddress:     ncParameters.AssociatedInterfaceID,
@@ -1292,7 +1310,7 @@ func (service *HTTPRestService) unpublishNetworkContainer(w http.ResponseWriter,
 				returnCode = types.NetworkJoinFailed
 				unpublishErrorStr = err.Error()
 
-				var nmaErr nma.Error
+				var nmaErr nmagent.Error
 				if errors.As(err, &nmaErr) {
 					unpublishStatusCode = nmaErr.StatusCode()
 				}
@@ -1303,7 +1321,7 @@ func (service *HTTPRestService) unpublishNetworkContainer(w http.ResponseWriter,
 		}
 
 		if isNetworkJoined {
-			dcr := nma.DeleteContainerRequest{
+			dcr := nmagent.DeleteContainerRequest{
 				NCID:                req.NetworkContainerID,
 				PrimaryAddress:      ncParameters.AssociatedInterfaceID,
 				AuthenticationToken: ncParameters.AuthToken,

cns/restserver/api_test.go

@@ -747,6 +747,68 @@ func TestPublishNCViaCNS(t *testing.T) {
 	}
 }
 
+func TestPublishNCBadBody(t *testing.T) {
+	mnma := &fakes.NMAgentClientFake{
+		PutNetworkContainerF: func(_ context.Context, _ *nmagent.PutNetworkContainerRequest) error {
+			return nil
+		},
+		JoinNetworkF: func(_ context.Context, _ nmagent.JoinNetworkRequest) error {
+			return nil
+		},
+	}
+
+	cleanup := setMockNMAgent(svc, mnma)
+	t.Cleanup(cleanup)
+
+	joinNetworkURL := "http://" + nmagentEndpoint + "/dummyVnetURL"
+
+	createNetworkContainerURL := "http://" + nmagentEndpoint +
+		"/machine/plugins/?comp=nmagent&type=NetworkManagement/interfaces/dummyIntf/networkContainers/dummyNCURL/authenticationToken/dummyT/api-version/1"
+	publishNCRequest := &cns.PublishNetworkContainerRequest{
+		NetworkID:                         "foo",
+		NetworkContainerID:                "bar",
+		JoinNetworkURL:                    joinNetworkURL,
+		CreateNetworkContainerURL:         createNetworkContainerURL,
+		CreateNetworkContainerRequestBody: []byte("this is not even remotely JSON"),
+	}
+
+	var body bytes.Buffer
+	err := json.NewEncoder(&body).Encode(publishNCRequest)
+	if err != nil {
+		t.Fatal("error encoding json: err:", err)
+	}
+
+	//nolint:noctx // also just a test
+	req, err := http.NewRequest(http.MethodPost, cns.PublishNetworkContainer, &body)
+	if err != nil {
+		t.Fatal("error creating new HTTP request: err:", err)
+	}
+
+	w := httptest.NewRecorder()
+	mux.ServeHTTP(w, req)
+
+	// the request should fail because the inner request body was incorrectly
+	// formatted
+	expStatus := http.StatusOK
+	gotStatus := w.Code
+	if expStatus != gotStatus {
+		t.Error("unexpected http status code: exp:", expStatus, "got:", gotStatus)
+	}
+
+	var resp cns.PublishNetworkContainerResponse
+	//nolint:bodyclose // unnnecessary in a test
+	err = json.NewDecoder(w.Result().Body).Decode(&resp)
+	if err != nil {
+		t.Fatal("unexpected error decoding JSON: err:", err)
+	}
+
+	expCode := types.NetworkContainerPublishFailed
+	gotCode := resp.Response.ReturnCode
+	if expCode != gotCode {
+		t.Error("unexpected return code: exp:", expCode, "got:", gotCode)
+	}
+}
+
 func publishNCViaCNS(
 	networkID,
 	networkContainerID,
@@ -764,7 +826,7 @@ func publishNCViaCNS(
 		NetworkContainerID:                networkContainerID,
 		JoinNetworkURL:                    joinNetworkURL,
 		CreateNetworkContainerURL:         createNetworkContainerURL,
-		CreateNetworkContainerRequestBody: nmagent.PutNetworkContainerRequest{},
+		CreateNetworkContainerRequestBody: []byte("{}"),
 	}
 
 	json.NewEncoder(&body).Encode(publishNCRequest)