From 2f53eab8ac75afe4939f6848b07fe4cf2cc8e2ae Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Fri, 10 Feb 2023 11:46:00 -0500 Subject: [PATCH 1/5] Fix incorrect 200 for a 401 from NMAgent In scenarios where the subnet token does not match (leading to a 401 from NMAgent), CNS returns a 200 for the PublishStatusCode. This is incorrect, and a 401 should be returned instead. This leads clients of CNS to take incorrect action on, what they believe to be, a successful response. --- cns/restserver/api.go | 14 +++++--- cns/restserver/api_test.go | 69 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 5 deletions(-) diff --git a/cns/restserver/api.go b/cns/restserver/api.go index c6a56e9cd9..47df3f164d 100644 --- a/cns/restserver/api.go +++ b/cns/restserver/api.go @@ -1124,7 +1124,7 @@ func getAuthTokenAndInterfaceIDFromNcURL(networkContainerURL string) (*cns.Netwo } //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) { +func (h *HTTPRestService) doPublish(ctx context.Context, req cns.PublishNetworkContainerRequest, ncParameters *cns.NetworkContainerParameters) (string, types.ResponseCode, int) { innerReqBytes := req.CreateNetworkContainerRequestBody var innerReq nmagent.PutNetworkContainerRequest @@ -1133,7 +1133,7 @@ func (h *HTTPRestService) doPublish(ctx context.Context, req cns.PublishNetworkC returnMessage := fmt.Sprintf("Failed to unmarshal embedded NC publish request for NC %s, with err: %v", req.NetworkContainerID, err) returnCode := types.NetworkContainerPublishFailed logger.Errorf("[Azure-CNS] %s", returnMessage) - return returnMessage, returnCode + return returnMessage, returnCode, http.StatusInternalServerError } innerReq.AuthenticationToken = ncParameters.AuthToken @@ -1146,10 +1146,14 @@ func (h *HTTPRestService) doPublish(ctx context.Context, req cns.PublishNetworkC returnMessage := fmt.Sprintf("Failed to publish Network Container %s in put Network Container call, with err: %v", req.NetworkContainerID, err) returnCode := types.NetworkContainerPublishFailed logger.Errorf("[Azure-CNS] %s", returnMessage) - return returnMessage, returnCode + var nmaErr nmagent.Error + if errors.As(err, &nmaErr) { + return returnMessage, returnCode, nmaErr.StatusCode() + } + return returnMessage, returnCode, http.StatusInternalServerError } - return "", types.Success + return "", types.Success, http.StatusOK } // Publish Network Container by calling nmagent @@ -1231,7 +1235,7 @@ func (service *HTTPRestService) publishNetworkContainer(w http.ResponseWriter, r if isNetworkJoined { // Publish Network Container - returnMessage, returnCode = service.doPublish(ctx, req, ncParameters) + returnMessage, returnCode, publishStatusCode = service.doPublish(ctx, req, ncParameters) } default: diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index c3a8a5bd40..ad94f59028 100644 --- a/cns/restserver/api_test.go +++ b/cns/restserver/api_test.go @@ -847,6 +847,75 @@ func TestPublishNCBadBody(t *testing.T) { } } +func TestPublishNC401(t *testing.T) { + mnma := &fakes.NMAgentClientFake{ + PutNetworkContainerF: func(_ context.Context, _ *nmagent.PutNetworkContainerRequest) error { + return nmagent.Error{ + Code: http.StatusUnauthorized, + Source: "nmagent", + } + }, + 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("{\"version\":\"0\"}"), + } + + 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) + + 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) + } + + expBodyStatus := http.StatusUnauthorized + gotBodyStatus := resp.PublishStatusCode + if expBodyStatus != gotBodyStatus { + t.Error("unexpected publish body status: exp:", expBodyStatus, "got:", gotBodyStatus) + } +} + func publishNCViaCNS( networkID, networkContainerID, From 1d507d95cdf6febd3560ed9b87cc227de0eed9a9 Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Fri, 10 Feb 2023 12:57:01 -0500 Subject: [PATCH 2/5] Doc the returns from doPublish --- cns/restserver/api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cns/restserver/api.go b/cns/restserver/api.go index 47df3f164d..f947d7c96a 100644 --- a/cns/restserver/api.go +++ b/cns/restserver/api.go @@ -1124,7 +1124,7 @@ func getAuthTokenAndInterfaceIDFromNcURL(networkContainerURL string) (*cns.Netwo } //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, int) { +func (h *HTTPRestService) doPublish(ctx context.Context, req cns.PublishNetworkContainerRequest, ncParameters *cns.NetworkContainerParameters) (msg string, code types.ResponseCode, status int) { innerReqBytes := req.CreateNetworkContainerRequestBody var innerReq nmagent.PutNetworkContainerRequest From 9ed89ae48a264ab508d28f3fe6095f7a77c125f6 Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Tue, 14 Feb 2023 06:40:08 -0500 Subject: [PATCH 3/5] Add assertions for PublishResponseBody It's important that we ensure that PublishResponseBody is, indeed, JSON. Also, that JSON needs to have an httpStatusCode property with its value set to the response code returned from NMAgent (in this test case, a 401). --- cns/restserver/api_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index ad94f59028..4196e2f0fd 100644 --- a/cns/restserver/api_test.go +++ b/cns/restserver/api_test.go @@ -914,6 +914,24 @@ func TestPublishNC401(t *testing.T) { if expBodyStatus != gotBodyStatus { t.Error("unexpected publish body status: exp:", expBodyStatus, "got:", gotBodyStatus) } + + // ensure that the PublishResponseBody is JSON + pubResp := make(map[string]any) + err = json.Unmarshal(resp.PublishResponseBody, &pubResp) + if err != nil { + t.Fatal("unexpected error unmarshaling PublishResponseBody: err:", err) + } + + // ensure that the PublishResponseBody also contains the embedded status from + // NMAgent + expStatusStr := strconv.Itoa(expBodyStatus) + if gotStatusStr, ok := pubResp["httpStatusCode"]; ok { + if gotStatusStr != expStatusStr { + t.Fatalf("expected PublishResponseBody's httpStatusCode to be %q, but was %q\n", expStatusStr, gotStatusStr) + } + } else { + t.Fatal("PublishResponseBody did not contain httpStatusCode") + } } func publishNCViaCNS( From 620dd55a161595ff01da33f08964a273339043de Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Tue, 14 Feb 2023 07:52:12 -0500 Subject: [PATCH 4/5] Ensure that Unpublish sets status from NMAgent The UnpublishStatusCode was not being set by CNS, which could lead to misbehavior from clients that expect it to be set appropriately. This uses the StatusCode provided by the nmagent.Error to propagate the httpStatusCode. --- cns/restserver/api.go | 4 ++ cns/restserver/api_test.go | 78 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+) diff --git a/cns/restserver/api.go b/cns/restserver/api.go index f947d7c96a..0378584c17 100644 --- a/cns/restserver/api.go +++ b/cns/restserver/api.go @@ -1346,6 +1346,10 @@ func (service *HTTPRestService) unpublishNetworkContainer(w http.ResponseWriter, if err != nil { returnMessage = fmt.Sprintf("Failed to unpublish Network Container: %s", req.NetworkContainerID) returnCode = types.NetworkContainerUnpublishFailed + var nmaErr nmagent.Error + if errors.As(err, &nmaErr) { + unpublishStatusCode = nmaErr.StatusCode() + } logger.Errorf("[Azure-CNS] %s", returnMessage) } } diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index 4196e2f0fd..565fafbb21 100644 --- a/cns/restserver/api_test.go +++ b/cns/restserver/api_test.go @@ -1100,6 +1100,84 @@ func TestUnpublishNCViaCNS(t *testing.T) { } } +func TestUnpublishNCViaCNS401(t *testing.T) { + mnma := &fakes.NMAgentClientFake{ + DeleteNetworkContainerF: func(_ context.Context, _ nmagent.DeleteContainerRequest) error { + // simulate a 401 from just Delete + return nmagent.Error{ + Code: http.StatusUnauthorized, + Source: "nmagent", + } + }, + JoinNetworkF: func(_ context.Context, _ nmagent.JoinNetworkRequest) error { + return nil + }, + } + + cleanup := setMockNMAgent(svc, mnma) + t.Cleanup(cleanup) + + deleteNetworkContainerURL := "http://" + nmagentEndpoint + + "/machine/plugins/?comp=nmagent&type=NetworkManagement/interfaces/dummyIntf/networkContainers/dummyNCURL/authenticationToken/dummyT/api-version/1/method/DELETE" + + networkContainerID := "ethWebApp" + networkID := "vnet1" + + joinNetworkURL := "http://" + nmagentEndpoint + "/dummyVnetURL" + + unpublishNCRequest := &cns.UnpublishNetworkContainerRequest{ + NetworkID: networkID, + NetworkContainerID: networkContainerID, + JoinNetworkURL: joinNetworkURL, + DeleteNetworkContainerURL: deleteNetworkContainerURL, + } + + var body bytes.Buffer + json.NewEncoder(&body).Encode(unpublishNCRequest) + req, err := http.NewRequest(http.MethodPost, cns.UnpublishNetworkContainer, &body) + if err != nil { + t.Fatal("error submitting unpublish request: err:", err) + } + + w := httptest.NewRecorder() + mux.ServeHTTP(w, req) + + var resp cns.UnpublishNetworkContainerResponse + err = decodeResponse(w, &resp) + if err != nil { + t.Fatal("error decoding json: err:", err) + } + + expCode := types.NetworkContainerUnpublishFailed + if gotCode := resp.Response.ReturnCode; gotCode != expCode { + t.Error("unexpected return code: got:", gotCode, "exp:", expCode) + } + + gotStatus := resp.UnpublishStatusCode + expStatus := http.StatusUnauthorized + if gotStatus != expStatus { + t.Error("unexpected http status during unpublish: got:", gotStatus, "exp:", expStatus) + } + + nmaBody := struct { + StatusCode string `json:"httpStatusCode"` + }{} + err = json.Unmarshal(resp.UnpublishResponseBody, &nmaBody) + if err != nil { + t.Fatal("error decoding UnpublishResponseBody as JSON: err:", err) + } + + gotBodyStatus, err := strconv.Atoi(nmaBody.StatusCode) + if err != nil { + t.Fatal("error parsing NMAgent body status code as an integer: err:", err) + } + + expBodyStatus := http.StatusUnauthorized + if gotBodyStatus != expBodyStatus { + t.Errorf("mismatch between expected NMAgent status code (%d) and NMAgent body status code (%d)\n", expBodyStatus, gotBodyStatus) + } +} + func unpublishNCViaCNS(networkID, networkContainerID, deleteNetworkContainerURL string) error { joinNetworkURL := "http://" + nmagentEndpoint + "/dummyVnetURL" From 432b34fee2751b2693de3b91584835bcadf61ab7 Mon Sep 17 00:00:00 2001 From: Tim Raymond Date: Tue, 14 Feb 2023 07:57:45 -0500 Subject: [PATCH 5/5] Address lints One of these lints is inappropriate in a test, so it's been silenced (adding context to the HTTP Request). The other one is marginal, but easy to fix, so there's now a check for an error from JSON encoding. --- cns/restserver/api_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index 565fafbb21..7823489316 100644 --- a/cns/restserver/api_test.go +++ b/cns/restserver/api_test.go @@ -1133,7 +1133,12 @@ func TestUnpublishNCViaCNS401(t *testing.T) { } var body bytes.Buffer - json.NewEncoder(&body).Encode(unpublishNCRequest) + err := json.NewEncoder(&body).Encode(unpublishNCRequest) + if err != nil { + t.Fatal("error encoding unpublish request: err:", err) + } + + //nolint:noctx // not important in a test req, err := http.NewRequest(http.MethodPost, cns.UnpublishNetworkContainer, &body) if err != nil { t.Fatal("error submitting unpublish request: err:", err)