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).
This commit is contained in:
Timothy J. Raymond
GitHub
Родитель
bcbb60561a
Коммит
45108ae514
|
|
@ -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,6 +1247,10 @@ 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,
|
||||
|
|
@ -1252,7 +1258,7 @@ func (service *HTTPRestService) publishNetworkContainer(w http.ResponseWriter, r
|
|||
},
|
||||
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,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,
|
||||
|
|
@ -1362,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)
|
||||
|
|
|
|||
|
|
@ -843,19 +843,63 @@ 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
|
||||
},
|
||||
}
|
||||
|
|
@ -863,47 +907,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")
|
||||
}
|
||||
|
||||
// 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)
|
||||
}
|
||||
|
||||
// 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 testUnpublishNCViaCNS(t *testing.T,
|
||||
networkID,
|
||||
networkContainerID,
|
||||
deleteNetworkContainerURL string,
|
||||
expectError bool,
|
||||
) error {
|
||||
var (
|
||||
body bytes.Buffer
|
||||
resp cns.UnpublishNetworkContainerResponse
|
||||
)
|
||||
|
||||
fmt.Println("Test: unpublishNetworkContainer")
|
||||
|
||||
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)
|
||||
}
|
||||
|
||||
fmt.Printf("UnpublishNetworkContainer succeded with response %+v, raw:%+v\n", resp, w.Body)
|
||||
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)
|
||||
}
|
||||
|
||||
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) {
|
||||
|
|
|
|||
Загрузка…
Ссылка в новой задаче