Fix incorrect HTTP status from publish NC (#1757)

* Fix incorrect HTTP status from publish NC

CNS was responding with an HTTP status code of "0" from NMAgent.
Successes are supposed to be 200. The C-style var block at the beginning
of publishNetworkContainer was the reason for this. During refactoring,
the location where this status code was set to a successful value of 200
was accidentally removed. Because the var block declared the variable
and silently initialized it to 0, the compiler did not flag this bug as
it otherwise would have. The status code has been removed from this
block and explicitly defined and initialized to a correct value of 200.
Subsequent error handling will change this as necessary.

Also, despite consumers depending on this status, there were no tests to
verify that the status was set correctly. Tests have been added to
reflect this dependency.

* Ensure that NMAgent body is always set

DNC depends on the NMAgent body being set for its vestigial functions of
retrying failed requests. Since failed requests will now be retried
internally (to CNS) by the NMAgent client, this isn't really necessary
anymore. There are versions of DNC out there that depend on this body
though, so it needs to be present in order for NC publishing to actually
work.

* Fix missing NMAgent status for Unpublish

It was discovered that the Unpublish endpoints also omitted the status
codes and bodies expected by clients. This adds those and fixes the
associated tests to guarantee the expected behavior.

* Silence the linter

There were two instances where the linter was flagging dynamic errors,
but this is just in a test. It's perfectly fine to bend the rules there,
since we don't expect to re-use the errors (they really should be
t.Fatal / t.Error anyway, but due to legacy we're returning errors here
instead).

Author: timraymond

cns/restserver/api.go

@@ -1159,15 +1159,17 @@ func (service *HTTPRestService) publishNetworkContainer(w http.ResponseWriter, r
 	ctx := r.Context()
 
 	var (
-		req                 cns.PublishNetworkContainerRequest
-		returnCode          types.ResponseCode
-		returnMessage       string
-		publishStatusCode   int
-		publishResponseBody []byte
-		publishErrorStr     string
-		isNetworkJoined     bool
+		req             cns.PublishNetworkContainerRequest
+		returnCode      types.ResponseCode
+		returnMessage   string
+		publishErrorStr string
+		isNetworkJoined bool
 	)
 
+	// publishing is assumed to succeed unless some other error handling sets it
+	// otherwise
+	publishStatusCode := http.StatusOK
+
 	err := service.Listener.Decode(w, r, &req)
 
 	creteNcURLCopy := req.CreateNetworkContainerURL
@@ -1245,14 +1247,18 @@ func (service *HTTPRestService) publishNetworkContainer(w http.ResponseWriter, r
 		returnCode = types.UnsupportedVerb
 	}
 
+	// create a synthetic response from NMAgent so that clients that previously
+	// relied on its presence can continue to do so.
+	publishResponseBody := fmt.Sprintf(`{"httpStatusCode":"%d"}`, publishStatusCode)
+
 	response := cns.PublishNetworkContainerResponse{
 		Response: cns.Response{
 			ReturnCode: returnCode,
 			Message:    returnMessage,
 		},
 		PublishErrorStr:     publishErrorStr,
 		PublishStatusCode:   publishStatusCode,
-		PublishResponseBody: publishResponseBody,
+		PublishResponseBody: []byte(publishResponseBody),
 	}
 
 	err = service.Listener.Encode(w, &response)
@@ -1265,15 +1271,15 @@ func (service *HTTPRestService) unpublishNetworkContainer(w http.ResponseWriter,
 	ctx := r.Context()
 
 	var (
-		req                   cns.UnpublishNetworkContainerRequest
-		returnCode            types.ResponseCode
-		returnMessage         string
-		unpublishStatusCode   int
-		unpublishResponseBody []byte
-		unpublishErrorStr     string
-		isNetworkJoined       bool
+		req               cns.UnpublishNetworkContainerRequest
+		returnCode        types.ResponseCode
+		returnMessage     string
+		unpublishErrorStr string
+		isNetworkJoined   bool
 	)
 
+	unpublishStatusCode := http.StatusOK
+
 	err := service.Listener.Decode(w, r, &req)
 
 	deleteNcURLCopy := req.DeleteNetworkContainerURL
@@ -1355,14 +1361,18 @@ func (service *HTTPRestService) unpublishNetworkContainer(w http.ResponseWriter,
 		returnCode = types.UnsupportedVerb
 	}
 
+	// create a synthetic response from NMAgent so that clients that previously
+	// relied on its presence can continue to do so.
+	unpublishResponseBody := fmt.Sprintf(`{"httpStatusCode":"%d"}`, unpublishStatusCode)
+
 	response := cns.UnpublishNetworkContainerResponse{
 		Response: cns.Response{
 			ReturnCode: returnCode,
 			Message:    returnMessage,
 		},
 		UnpublishErrorStr:     unpublishErrorStr,
 		UnpublishStatusCode:   unpublishStatusCode,
-		UnpublishResponseBody: unpublishResponseBody,
+		UnpublishResponseBody: []byte(unpublishResponseBody),
 	}
 
 	err = service.Listener.Encode(w, &response)

cns/restserver/api_test.go

@@ -843,67 +843,139 @@ func publishNCViaCNS(
 		return fmt.Errorf("decoding response: %w", err)
 	}
 
+	expStatus := http.StatusOK
+	gotStatus := resp.PublishStatusCode
+	if gotStatus != expStatus {
+		// nolint:goerr113 // this is okay in a test:
+		return fmt.Errorf("unsuccessful request. exp: %d, got: %d", expStatus, gotStatus)
+	}
+
+	// ensure that there is an NMA body for legacy purposes
+	nmaResp := make(map[string]any)
+	err = json.Unmarshal(resp.PublishResponseBody, &nmaResp)
+	if err != nil {
+		return fmt.Errorf("decoding response body from nmagent: %w", err)
+	}
+
+	if statusStr, ok := nmaResp["httpStatusCode"]; ok {
+		bodyStatus, err := strconv.Atoi(statusStr.(string))
+		if err != nil {
+			return fmt.Errorf("parsing http status string from nmagent: %w", err)
+		}
+
+		if bodyStatus != expStatus {
+			// nolint:goerr113 // this is okay in a test:
+			return fmt.Errorf("unexpected status in body. exp: %d, got %d", expStatus, bodyStatus)
+		}
+	}
+
 	fmt.Printf("PublishNetworkContainer succeded with response %+v, raw:%+v\n", resp, w.Body)
 	return nil
 }
 
 func TestUnpublishNCViaCNS(t *testing.T) {
+	// Publishing and Unpublishing via CNS effectively uses CNS as a proxy to
+	// NMAgent. This test asserts that the correct methods are invoked on the
+	// NMAgent client in the correct order based on a sequence of creating and
+	// destroying an example NC.
+
+	// create a mock NMAgent that allows assertions on the methods called. There
+	// should be a certain sequence of invocations based on the high-level
+	// actions that are taken here.
+	const (
+		joinNetwork            = "JoinNetwork"
+		deleteNetworkContainer = "DeleteNetworkContainer"
+		putNetworkContainer    = "PutNetworkContainer"
+	)
+
+	got := []string{}
 	mnma := &fakes.NMAgentClientFake{
 		JoinNetworkF: func(_ context.Context, _ nmagent.JoinNetworkRequest) error {
+			got = append(got, joinNetwork)
 			return nil
 		},
 		DeleteNetworkContainerF: func(_ context.Context, _ nmagent.DeleteContainerRequest) error {
+			got = append(got, deleteNetworkContainer)
 			return nil
 		},
 		PutNetworkContainerF: func(_ context.Context, _ *nmagent.PutNetworkContainerRequest) error {
+			got = append(got, putNetworkContainer)
 			return nil
 		},
 	}
 
 	cleanup := setMockNMAgent(svc, mnma)
 	defer cleanup()
 
-	deleteNetworkContainerURL := "http://" + nmagentEndpoint +
-		"/machine/plugins/?comp=nmagent&type=NetworkManagement/interfaces/dummyIntf/networkContainers/dummyNCURL/authenticationToken/dummyT/api-version/1/method/DELETE"
-	err := publishNCViaCNS("vnet1", "ethWebApp", deleteNetworkContainerURL)
+	// create a network container as the subject of this test.
+	createNetworkContainerURL := "http://" + nmagentEndpoint +
+		"/machine/plugins/?comp=nmagent&type=NetworkManagement/interfaces/dummyIntf/networkContainers/dummyNCURL/authenticationToken/dummyT/api-version/1"
+	err := publishNCViaCNS("vnet1", "ethWebApp", createNetworkContainerURL)
 	if err != nil {
-		t.Fatal(err)
+		t.Fatal(fmt.Errorf("publish container failed %w ", err))
 	}
 
-	deleteNetworkContainerURL = "http://" + nmagentEndpoint +
+	// prior to the actual deletion, attempt to delete using an invalid URL (by
+	// omitting a letter from "authenticationToken"). This should fail:
+	deleteNetworkContainerURL := "http://" + nmagentEndpoint +
 		"/machine/plugins/?comp=nmagent&type=NetworkManagement/interfaces/dummyIntf/networkContainers/dummyNCURL/authenticationToke/" +
 		"8636c99d-7861-401f-b0d3-7e5b7dc8183c" +
 		"/api-version/1/method/DELETE"
-
-	err = publishNCViaCNS("vnet1", "ethWebApp", deleteNetworkContainerURL)
+	err = unpublishNCViaCNS("vnet1", "ethWebApp", deleteNetworkContainerURL)
 	if err == nil {
 		t.Fatal("Expected a bad request error due to delete network url being incorrect")
 	}
 
+	// also ensure that deleting a network container with an invalid
+	// authentication token (one that is too long) also fails:
 	deleteNetworkContainerURL = "http://" + nmagentEndpoint +
 		"/machine/plugins/?comp=nmagent&NetworkManagement/interfaces/dummyIntf/networkContainers/dummyNCURL/authenticationToken/" +
 		"8636c99d-7861-401f-b0d3-7e5b7dc8183c8636c99d-7861-401f-b0d3-7e5b7dc8183c" +
 		"/api-version/1/method/DELETE"
-
-	err = testUnpublishNCViaCNS(t, "vnet1", "ethWebApp", deleteNetworkContainerURL, true)
+	err = unpublishNCViaCNS("vnet1", "ethWebApp", deleteNetworkContainerURL)
 	if err == nil {
 		t.Fatal("Expected a bad request error due to create network url having more characters than permitted in auth token")
 	}
-}
 
-func testUnpublishNCViaCNS(t *testing.T,
-	networkID,
-	networkContainerID,
-	deleteNetworkContainerURL string,
-	expectError bool,
-) error {
-	var (
-		body bytes.Buffer
-		resp cns.UnpublishNetworkContainerResponse
-	)
+	// now actually perform the deletion:
+	deleteNetworkContainerURL = "http://" + nmagentEndpoint +
+		"/machine/plugins/?comp=nmagent&type=NetworkManagement/interfaces/dummyIntf/networkContainers/dummyNCURL/authenticationToken/dummyT/api-version/1/method/DELETE"
+	err = unpublishNCViaCNS("vnet1", "ethWebApp", deleteNetworkContainerURL)
+	if err != nil {
+		t.Fatal(err)
+	}
 
-	fmt.Println("Test: unpublishNetworkContainer")
+	// Assert the correct sequence of invocations on the NMAgent client. Creating
+	// a network container involves first joining the network and then creating a
+	// network container. Deleting the network container only involved the Delete
+	// call. Even though there were two other invalid Delete calls, we do not
+	// expect them to generate invocations of methods on the NMAgent
+	// client--these should be captured by CNS.
+	exp := []string{
+		// These two methods in the sequence are from the NC creation:
+		joinNetwork,
+		putNetworkContainer,
 
+		// This one is from the delete. There is technically one code path where a
+		// "JoinNetwork" can appear here, but we don't expect it because
+		// "JoinNetwork" appeared as part of the NC creation.
+		deleteNetworkContainer,
+	}
+
+	// with the expectation set, match up expectations with the method calls
+	// received:
+	if len(exp) != len(got) {
+		t.Fatal("unexpected sequence of methods invoked on NMAgent client: exp:", exp, "got:", got)
+	}
+
+	for idx := range exp {
+		if got[idx] != exp[idx] {
+			t.Error("unexpected sequence of methods invoked on NMAgent client: exp:", exp, "got:", got)
+		}
+	}
+}
+
+func unpublishNCViaCNS(networkID, networkContainerID, deleteNetworkContainerURL string) error {
 	joinNetworkURL := "http://" + nmagentEndpoint + "/dummyVnetURL"
 
 	unpublishNCRequest := &cns.UnpublishNetworkContainerRequest{
@@ -913,36 +985,50 @@ func testUnpublishNCViaCNS(t *testing.T,
 		DeleteNetworkContainerURL: deleteNetworkContainerURL,
 	}
 
+	var body bytes.Buffer
 	json.NewEncoder(&body).Encode(unpublishNCRequest)
 	req, err := http.NewRequest(http.MethodPost, cns.UnpublishNetworkContainer, &body)
 	if err != nil {
 		return fmt.Errorf("Failed to create unpublish request %w", err)
 	}
 
-	mnma := &fakes.NMAgentClientFake{
-		DeleteNetworkContainerF: func(_ context.Context, _ nmagent.DeleteContainerRequest) error {
-			return nil
-		},
-		JoinNetworkF: func(_ context.Context, _ nmagent.JoinNetworkRequest) error {
-			return nil
-		},
-	}
-
-	cleanup := setMockNMAgent(svc, mnma)
-	defer cleanup()
-
 	w := httptest.NewRecorder()
 	mux.ServeHTTP(w, req)
 
+	var resp cns.UnpublishNetworkContainerResponse
 	err = decodeResponse(w, &resp)
-	if err != nil || resp.Response.ReturnCode != 0 {
-		if !expectError {
-			t.Errorf("UnpublishNetworkContainer failed with response %+v Err:%+v", resp, err)
-		}
-		return err
+	if err != nil {
+		return fmt.Errorf("error decoding json: err: %w", err)
+	}
+
+	if resp.Response.ReturnCode != 0 {
+		// nolint:goerr113 // this is okay in a test:
+		return fmt.Errorf("UnpublishNetworkContainer failed with response %+v: err: %w", resp, err)
+	}
+
+	code := resp.UnpublishStatusCode
+	if code != http.StatusOK {
+		// nolint:goerr113 // this is okay in a test:
+		return fmt.Errorf("unsuccessful NMAgent response: status code %d", code)
 	}
 
-	fmt.Printf("UnpublishNetworkContainer succeded with response %+v, raw:%+v\n", resp, w.Body)
+	nmaBody := struct {
+		StatusCode string `json:"httpStatusCode"`
+	}{}
+	err = json.Unmarshal(resp.UnpublishResponseBody, &nmaBody)
+	if err != nil {
+		return fmt.Errorf("unmarshaling NMAgent response body: %w", err)
+	}
+
+	bodyCode, err := strconv.Atoi(nmaBody.StatusCode)
+	if err != nil {
+		return fmt.Errorf("parsing NMAgent body status code as an integer: %w", err)
+	}
+
+	if bodyCode != code {
+		// nolint:goerr113 // this is okay in a test:
+		return fmt.Errorf("mismatch between NMAgent status code (%d) and NMAgent body status code (%d)", code, bodyCode)
+	}
 
 	return nil
 }
@@ -1367,14 +1453,16 @@ func setEnv(t *testing.T) *httptest.ResponseRecorder {
 }
 
 func startService() error {
-	var err error
 	// Create the service.
 	config := common.ServiceConfig{}
-	// Create the key value store.
-	if config.Store, err = store.NewJsonFileStore(cnsJsonFileName, processlock.NewMockFileLock(false)); err != nil {
+
+	// Create the key value fileStore.
+	fileStore, err := store.NewJsonFileStore(cnsJsonFileName, processlock.NewMockFileLock(false))
+	if err != nil {
 		logger.Errorf("Failed to create store file: %s, due to error %v\n", cnsJsonFileName, err)
 		return err
 	}
+	config.Store = fileStore
 
 	nmagentClient := &fakes.NMAgentClientFake{}
 	service, err = NewHTTPRestService(&config, &fakes.WireserverClientFake{}, nmagentClient, nil, nil, nil)
@@ -1383,10 +1471,6 @@ func startService() error {
 	}
 	svc = service.(*HTTPRestService)
 	svc.Name = "cns-test-server"
-	if err != nil {
-		logger.Errorf("Failed to create CNS object, err:%v.\n", err)
-		return err
-	}
 
 	svc.IPAMPoolMonitor = &fakes.MonitorFake{}
 	nmagentClient.GetNCVersionListF = func(context.Context) (nmagent.NCVersionList, error) {