Fix incorrect HTTP status from publish NC

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) {