From 58e29da56b110f17fabf423d623a148306ac194a Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Wed, 11 Jan 2023 17:20:19 -0500 Subject: [PATCH 1/4] 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. --- cns/restserver/api.go | 5 ++++- cns/restserver/api_test.go | 6 ++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/cns/restserver/api.go b/cns/restserver/api.go index af8124b882..658da7cf75 100644 --- a/cns/restserver/api.go +++ b/cns/restserver/api.go @@ -1162,12 +1162,15 @@ func (service *HTTPRestService) publishNetworkContainer(w http.ResponseWriter, r req cns.PublishNetworkContainerRequest returnCode types.ResponseCode returnMessage string - publishStatusCode int publishResponseBody []byte 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 diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index 63b7c96acd..b3c3a0265c 100644 --- a/cns/restserver/api_test.go +++ b/cns/restserver/api_test.go @@ -843,6 +843,12 @@ func publishNCViaCNS( return fmt.Errorf("decoding response: %w", err) } + expStatus := http.StatusOK + gotStatus := resp.PublishStatusCode + if gotStatus != expStatus { + return fmt.Errorf("unsuccessful request. exp: %d, got: %d", expStatus, gotStatus) + } + fmt.Printf("PublishNetworkContainer succeded with response %+v, raw:%+v\n", resp, w.Body) return nil } From 1361e9e5902ca4a30edb01b030b64139517d522a Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Thu, 12 Jan 2023 12:00:47 -0500 Subject: [PATCH 2/4] 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. --- cns/restserver/api.go | 20 +++++++++++++------- cns/restserver/api_test.go | 18 ++++++++++++++++++ 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/cns/restserver/api.go b/cns/restserver/api.go index 658da7cf75..e8fbe11fa8 100644 --- a/cns/restserver/api.go +++ b/cns/restserver/api.go @@ -1159,12 +1159,11 @@ func (service *HTTPRestService) publishNetworkContainer(w http.ResponseWriter, r ctx := r.Context() var ( - req cns.PublishNetworkContainerRequest - returnCode types.ResponseCode - returnMessage string - 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 @@ -1248,6 +1247,13 @@ func (service *HTTPRestService) publishNetworkContainer(w http.ResponseWriter, r returnCode = types.UnsupportedVerb } + // this is an ugly hack because DNC depends on checking this status code, + // even though it's no longer necessary for it to do so. It does not need to + // handle retries because retries will be handled by the nmagent client in + // CNS. However, there are versions of DNC out there that still rely on this + // body being present. + publishResponseBody := fmt.Sprintf(`{"httpStatusCode":"%d"}`, publishStatusCode) + response := cns.PublishNetworkContainerResponse{ Response: cns.Response{ ReturnCode: returnCode, @@ -1255,7 +1261,7 @@ func (service *HTTPRestService) publishNetworkContainer(w http.ResponseWriter, r }, PublishErrorStr: publishErrorStr, PublishStatusCode: publishStatusCode, - PublishResponseBody: publishResponseBody, + PublishResponseBody: []byte(publishResponseBody), } err = service.Listener.Encode(w, &response) diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index b3c3a0265c..12463bacd4 100644 --- a/cns/restserver/api_test.go +++ b/cns/restserver/api_test.go @@ -849,6 +849,24 @@ func publishNCViaCNS( 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 { + 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 } From 4fa2cfc5336a40b3ba9ac324250b19cef4ae6f0b Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Wed, 18 Jan 2023 11:46:32 -0500 Subject: [PATCH 3/4] 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. --- cns/restserver/api.go | 27 +++---- cns/restserver/api_test.go | 145 +++++++++++++++++++++++++------------ 2 files changed, 114 insertions(+), 58 deletions(-) diff --git a/cns/restserver/api.go b/cns/restserver/api.go index e8fbe11fa8..a6e6531897 100644 --- a/cns/restserver/api.go +++ b/cns/restserver/api.go @@ -1247,11 +1247,8 @@ func (service *HTTPRestService) publishNetworkContainer(w http.ResponseWriter, r returnCode = types.UnsupportedVerb } - // this is an ugly hack because DNC depends on checking this status code, - // even though it's no longer necessary for it to do so. It does not need to - // handle retries because retries will be handled by the nmagent client in - // CNS. However, there are versions of DNC out there that still rely on this - // body being present. + // 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{ @@ -1274,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 @@ -1364,6 +1361,10 @@ 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, @@ -1371,7 +1372,7 @@ func (service *HTTPRestService) unpublishNetworkContainer(w http.ResponseWriter, }, UnpublishErrorStr: unpublishErrorStr, UnpublishStatusCode: unpublishStatusCode, - UnpublishResponseBody: unpublishResponseBody, + UnpublishResponseBody: []byte(unpublishResponseBody), } err = service.Listener.Encode(w, &response) diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index 12463bacd4..558861c787 100644 --- a/cns/restserver/api_test.go +++ b/cns/restserver/api_test.go @@ -872,14 +872,32 @@ func publishNCViaCNS( } 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 }, } @@ -887,47 +905,75 @@ func TestUnpublishNCViaCNS(t *testing.T) { 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{ @@ -937,36 +983,47 @@ 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) } - fmt.Printf("UnpublishNetworkContainer succeded with response %+v, raw:%+v\n", resp, w.Body) + if resp.Response.ReturnCode != 0 { + return fmt.Errorf("UnpublishNetworkContainer failed with response %+v Err:%+v", resp, err) + } + + code := resp.UnpublishStatusCode + if code != http.StatusOK { + return fmt.Errorf("unsuccessful NMAgent response: status code %d", code) + } + + 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 { + return fmt.Errorf("mismatch between NMAgent status code (%d) and NMAgent body status code (%d)", code, bodyCode) + } return nil } @@ -1391,14 +1448,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 { + store, 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 = store nmagentClient := &fakes.NMAgentClientFake{} service, err = NewHTTPRestService(&config, &fakes.WireserverClientFake{}, nmagentClient, nil, nil, nil) @@ -1407,10 +1466,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) { From d7b68d19d9c54272a8ed55b9867a6c12ca7f6ef5 Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Wed, 18 Jan 2023 11:56:12 -0500 Subject: [PATCH 4/4] 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). --- cns/restserver/api_test.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index 558861c787..9ef9ea4342 100644 --- a/cns/restserver/api_test.go +++ b/cns/restserver/api_test.go @@ -846,6 +846,7 @@ func publishNCViaCNS( 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) } @@ -863,6 +864,7 @@ func publishNCViaCNS( } if bodyStatus != expStatus { + // nolint:goerr113 // this is okay in a test: return fmt.Errorf("unexpected status in body. exp: %d, got %d", expStatus, bodyStatus) } } @@ -1000,11 +1002,13 @@ func unpublishNCViaCNS(networkID, networkContainerID, deleteNetworkContainerURL } if resp.Response.ReturnCode != 0 { - return fmt.Errorf("UnpublishNetworkContainer failed with response %+v Err:%+v", resp, err) + // 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) } @@ -1022,6 +1026,7 @@ func unpublishNCViaCNS(networkID, networkContainerID, deleteNetworkContainerURL } 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) } @@ -1451,13 +1456,13 @@ func startService() error { // Create the service. config := common.ServiceConfig{} - // Create the key value store. - store, err := store.NewJsonFileStore(cnsJsonFileName, processlock.NewMockFileLock(false)) + // 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 = store + config.Store = fileStore nmagentClient := &fakes.NMAgentClientFake{} service, err = NewHTTPRestService(&config, &fakes.WireserverClientFake{}, nmagentClient, nil, nil, nil)