diff --git a/cns/fakes/nmagentclientfake.go b/cns/fakes/nmagentclientfake.go index 94babee35e..36f7bb14e7 100644 --- a/cns/fakes/nmagentclientfake.go +++ b/cns/fakes/nmagentclientfake.go @@ -18,7 +18,6 @@ type NMAgentClientFake struct { DeleteNetworkContainerF func(context.Context, nmagent.DeleteContainerRequest) error JoinNetworkF func(context.Context, nmagent.JoinNetworkRequest) error SupportedAPIsF func(context.Context) ([]string, error) - GetNCVersionF func(context.Context, nmagent.NCVersionRequest) (nmagent.NCVersion, error) GetNCVersionListF func(context.Context) (nmagent.NCVersionList, error) GetHomeAzF func(context.Context) (nmagent.AzResponse, error) } @@ -39,10 +38,6 @@ func (n *NMAgentClientFake) SupportedAPIs(ctx context.Context) ([]string, error) return n.SupportedAPIsF(ctx) } -func (n *NMAgentClientFake) GetNCVersion(ctx context.Context, req nmagent.NCVersionRequest) (nmagent.NCVersion, error) { - return n.GetNCVersionF(ctx, req) -} - func (n *NMAgentClientFake) GetNCVersionList(ctx context.Context) (nmagent.NCVersionList, error) { return n.GetNCVersionListF(ctx) } diff --git a/cns/nmagent/client.go b/cns/nmagent/client.go index d58777a6af..ba28b4d8da 100644 --- a/cns/nmagent/client.go +++ b/cns/nmagent/client.go @@ -1,9 +1,5 @@ package nmagent -import ( - "fmt" -) - const ( // GetNmAgentSupportedApiURLFmt Api endpoint to get supported Apis of NMAgent GetNmAgentSupportedApiURLFmt = "http://%s/machine/plugins/?comp=nmagent&type=GetSupportedApis" @@ -38,18 +34,3 @@ type NetworkContainerListResponse struct { ResponseCode string `json:"httpStatusCode"` Containers []ContainerInfo `json:"networkContainers"` } - -// Client is client to handle queries to nmagent -type Client struct { - connectionURL string -} - -// NewClient create a new nmagent client. -func NewClient(url string) (*Client, error) { - if url == "" { - url = fmt.Sprintf(GetNcVersionListWithOutTokenURLFmt, WireserverIP, getNcVersionListWithOutTokenURLVersion) - } - return &Client{ - connectionURL: url, - }, nil -} diff --git a/cns/restserver/api.go b/cns/restserver/api.go index af8124b882..a6f71df3ed 100644 --- a/cns/restserver/api.go +++ b/cns/restserver/api.go @@ -1232,14 +1232,6 @@ func (service *HTTPRestService) publishNetworkContainer(w http.ResponseWriter, r returnMessage, returnCode = service.doPublish(ctx, req, ncParameters) } - req := nmagent.NCVersionRequest{ - AuthToken: ncParameters.AuthToken, - NetworkContainerID: req.NetworkContainerID, - PrimaryAddress: ncParameters.AssociatedInterfaceID, - } - - ncVersionURLs.Store(cns.SwiftPrefix+req.NetworkContainerID, req) - default: returnMessage = "PublishNetworkContainer API expects a POST" returnCode = types.UnsupportedVerb @@ -1347,9 +1339,6 @@ func (service *HTTPRestService) unpublishNetworkContainer(w http.ResponseWriter, logger.Errorf("[Azure-CNS] %s", returnMessage) } } - - // Remove the NC version URL entry added during publish - ncVersionURLs.Delete(cns.SwiftPrefix + req.NetworkContainerID) default: returnMessage = "UnpublishNetworkContainer API expects a POST" returnCode = types.UnsupportedVerb diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index 63b7c96acd..8e60dbc2d5 100644 --- a/cns/restserver/api_test.go +++ b/cns/restserver/api_test.go @@ -550,10 +550,14 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) { t.Fatal("error creating NC: err:", err) } - mnma.GetNCVersionF = func(_ context.Context, _ nmagent.NCVersionRequest) (nmagent.NCVersion, error) { - return nmagent.NCVersion{ - NetworkContainerID: params.ncID, - Version: params.ncVersion, + mnma.GetNCVersionListF = func(_ context.Context) (nmagent.NCVersionList, error) { + return nmagent.NCVersionList{ + Containers: []nmagent.NCVersion{ + { + NetworkContainerID: cns.SwiftPrefix + params.ncID, + Version: params.ncVersion, + }, + }, }, nil } @@ -588,10 +592,14 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) { podNamespace: "testpodnamespace", } - mnma.GetNCVersionF = func(_ context.Context, _ nmagent.NCVersionRequest) (nmagent.NCVersion, error) { - return nmagent.NCVersion{ - NetworkContainerID: params.ncID, - Version: "0", // explicitly 1 less than the version above + mnma.GetNCVersionListF = func(_ context.Context) (nmagent.NCVersionList, error) { + return nmagent.NCVersionList{ + Containers: []nmagent.NCVersion{ + { + NetworkContainerID: cns.SwiftPrefix + params.ncID, + Version: "0", + }, + }, }, nil } @@ -612,7 +620,6 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) { } // Testing the path where NMAgent response status code is not 200. - // 2. NMAgent response status code is 200 but embedded response is 401 params = createOrUpdateNetworkContainerParams{ ncID: "nc-nma-fail-500", ncIP: "11.0.0.5", @@ -623,9 +630,13 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) { podNamespace: "testpodnamespace", } - mnma.GetNCVersionF = func(_ context.Context, _ nmagent.NCVersionRequest) (nmagent.NCVersion, error) { - return nmagent.NCVersion{}, errors.New("boom") //nolint:goerr113 // it's just a test + mnma.GetNCVersionListF = func(_ context.Context) (nmagent.NCVersionList, error) { + rsp := nmagent.NCVersionList{ + Containers: []nmagent.NCVersion{}, + } + return rsp, errors.New("boom") //nolint:goerr113 // it's just a test } + mnma.JoinNetworkF = func(_ context.Context, _ nmagent.JoinNetworkRequest) error { return errors.New("boom") //nolint:goerr113 // it's just a test } @@ -660,12 +671,13 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) { podNamespace: "testpodnamespace", } - // set the mock NMAgent to be "successful" again - mnma.GetNCVersionF = func(_ context.Context, _ nmagent.NCVersionRequest) (nmagent.NCVersion, error) { - return nmagent.NCVersion{}, nmagent.Error{ - Code: http.StatusUnauthorized, + mnma.GetNCVersionListF = func(_ context.Context) (nmagent.NCVersionList, error) { + rsp := nmagent.NCVersionList{ + Containers: []nmagent.NCVersion{}, } + return rsp, nil } + mnma.JoinNetworkF = func(_ context.Context, _ nmagent.JoinNetworkRequest) error { return nil } diff --git a/cns/restserver/const.go b/cns/restserver/const.go index d51814a1d7..0782755596 100644 --- a/cns/restserver/const.go +++ b/cns/restserver/const.go @@ -1,5 +1,7 @@ package restserver +import "time" + const ( // Key against which CNS state is persisted. storeKey = "ContainerNetworkService" @@ -9,4 +11,5 @@ const ( // Rest service state identifier for named lock stateJoinedNetworks = "JoinedNetworks" dncApiVersion = "?api-version=2018-03-01" + nmaAPICallTimeout = 2 * time.Second ) diff --git a/cns/restserver/internalapi.go b/cns/restserver/internalapi.go index b6aada9a35..154af65683 100644 --- a/cns/restserver/internalapi.go +++ b/cns/restserver/internalapi.go @@ -19,7 +19,6 @@ import ( "github.com/Azure/azure-container-networking/cns/types" "github.com/Azure/azure-container-networking/common" "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" - "github.com/Azure/azure-container-networking/nmagent" "github.com/pkg/errors" ) @@ -43,7 +42,6 @@ func (service *HTTPRestService) SetNodeOrchestrator(r *cns.SetOrchestratorTypeRe service.setOrchestratorType(httptest.NewRecorder(), req) } -// SyncNodeStatus :- Retrieve the latest node state from DNC & returns the first occurence of returnCode and error with respect to contextFromCNI func (service *HTTPRestService) SyncNodeStatus(dncEP, infraVnet, nodeID string, contextFromCNI json.RawMessage) (returnCode types.ResponseCode, errStr string) { logger.Printf("[Azure CNS] SyncNodeStatus") var ( @@ -94,33 +92,49 @@ func (service *HTTPRestService) SyncNodeStatus(dncEP, infraVnet, nodeID string, } service.RUnlock() - // check if the version is valid and save it to service state - for ncid, nc := range ncsToBeAdded { - nmaReq := nmagent.NCVersionRequest{ - AuthToken: nc.AuthorizationToken, - NetworkContainerID: nc.NetworkContainerid, - PrimaryAddress: nc.PrimaryInterfaceIdentifier, - } - - ncVersionURLs.Store(nc.NetworkContainerid, nmaReq) - waitingForUpdate, _, _ := service.isNCWaitingForUpdate(nc.Version, nc.NetworkContainerid) + skipNCVersionCheck := false + ctx, cancel := context.WithTimeout(context.Background(), nmaAPICallTimeout) + defer cancel() + ncVersionListResp, err := service.nma.GetNCVersionList(ctx) + if err != nil { + skipNCVersionCheck = true + logger.Errorf("failed to get nc version list from nmagent") + } - body, _ = json.Marshal(nc) - req, _ = http.NewRequest(http.MethodPost, "", bytes.NewBuffer(body)) - req.Header.Set(common.ContentType, common.JsonContent) + if !skipNCVersionCheck { + nmaNCs := map[string]string{} + for _, nc := range ncVersionListResp.Containers { + nmaNCs[cns.SwiftPrefix+nc.NetworkContainerID] = nc.Version + } - w := httptest.NewRecorder() - service.createOrUpdateNetworkContainer(w, req) + // check if the version is valid and save it to service state + for ncid := range ncsToBeAdded { + waitingForUpdate, _, _ := service.isNCWaitingForUpdate(ncsToBeAdded[ncid].Version, ncsToBeAdded[ncid].NetworkContainerid, nmaNCs) - if w.Result().StatusCode == http.StatusOK { - var resp cns.CreateNetworkContainerResponse - if err = json.Unmarshal(w.Body.Bytes(), &resp); err == nil && resp.Response.ReturnCode == types.Success { - service.Lock() - ncstatus := service.state.ContainerStatus[ncid] - ncstatus.VfpUpdateComplete = !waitingForUpdate - service.state.ContainerStatus[ncid] = ncstatus - service.Unlock() + body, err = json.Marshal(ncsToBeAdded[ncid]) + if err != nil { + logger.Errorf("[Azure-CNS] Failed to marshal nc with nc id %s and content %v", ncid, ncsToBeAdded[ncid]) + } + req, err = http.NewRequestWithContext(ctx, http.MethodPost, "", bytes.NewBuffer(body)) + if err != nil { + logger.Errorf("[Azure CNS] Error received while creating http POST request for nc %v", ncsToBeAdded[ncid]) } + req.Header.Set(common.ContentType, common.JsonContent) + + w := httptest.NewRecorder() + service.createOrUpdateNetworkContainer(w, req) + result := w.Result() + if result.StatusCode == http.StatusOK { + var resp cns.CreateNetworkContainerResponse + if err = json.Unmarshal(w.Body.Bytes(), &resp); err == nil && resp.Response.ReturnCode == types.Success { + service.Lock() + ncstatus := service.state.ContainerStatus[ncid] + ncstatus.VfpUpdateComplete = !waitingForUpdate + service.state.ContainerStatus[ncid] = ncstatus + service.Unlock() + } + } + result.Body.Close() } } @@ -140,10 +154,7 @@ func (service *HTTPRestService) SyncNodeStatus(dncEP, infraVnet, nodeID string, } else { logger.Errorf("[Azure-CNS] Failed to delete NC request to sync state: %s", err.Error()) } - - ncVersionURLs.Delete(nc) } - return } diff --git a/cns/restserver/restserver.go b/cns/restserver/restserver.go index 0fdcbcbaf6..965c3eda5f 100644 --- a/cns/restserver/restserver.go +++ b/cns/restserver/restserver.go @@ -31,8 +31,6 @@ import ( var ( // Named Lock for accessing different states in httpRestServiceState namedLock = acn.InitNamedLock() - // map of NC to their respective NMA getVersion URLs - ncVersionURLs sync.Map ) type interfaceGetter interface { @@ -44,7 +42,6 @@ type nmagentClient interface { DeleteNetworkContainer(context.Context, nma.DeleteContainerRequest) error JoinNetwork(context.Context, nma.JoinNetworkRequest) error SupportedAPIs(context.Context) ([]string, error) - GetNCVersion(context.Context, nma.NCVersionRequest) (nma.NCVersion, error) GetNCVersionList(context.Context) (nma.NCVersionList, error) GetHomeAz(context.Context) (nma.AzResponse, error) } diff --git a/cns/restserver/util.go b/cns/restserver/util.go index e1e7f34f23..446be88b91 100644 --- a/cns/restserver/util.go +++ b/cns/restserver/util.go @@ -384,9 +384,23 @@ func (service *HTTPRestService) getNetworkContainerResponse( containerID, exists = service.state.ContainerIDByOrchestratorContext[podInfo.Name()+podInfo.Namespace()] - if exists { + skipNCVersionCheck := false + ctx, cancel := context.WithTimeout(context.Background(), nmaAPICallTimeout) + defer cancel() + ncVersionListResp, err := service.nma.GetNCVersionList(ctx) + if err != nil { + skipNCVersionCheck = true + logger.Errorf("failed to get nc version list from nmagent") + } + nmaNCs := map[string]string{} + for _, nc := range ncVersionListResp.Containers { + nmaNCs[nc.NetworkContainerID] = nc.Version + } + + if exists && !skipNCVersionCheck { // If the goal state is available with CNS, check if the NC is pending VFP programming - waitingForUpdate, getNetworkContainerResponse.Response.ReturnCode, getNetworkContainerResponse.Response.Message = service.isNCWaitingForUpdate(service.state.ContainerStatus[containerID].CreateNetworkContainerRequest.Version, containerID) //nolint:lll // bad code + waitingForUpdate, getNetworkContainerResponse.Response.ReturnCode, getNetworkContainerResponse.Response.Message = service.isNCWaitingForUpdate( + service.state.ContainerStatus[containerID].CreateNetworkContainerRequest.Version, containerID, nmaNCs) // If the return code is not success, return the error to the caller if getNetworkContainerResponse.Response.ReturnCode == types.NetworkContainerVfpProgramPending { logger.Errorf("[Azure-CNS] isNCWaitingForUpdate failed for NC: %s with error: %s", @@ -534,7 +548,21 @@ func (service *HTTPRestService) attachOrDetachHelper(req cns.ConfigureContainerN if service.ChannelMode == cns.Managed && operation == attach { if ok { if !existing.VfpUpdateComplete { - _, returnCode, message := service.isNCWaitingForUpdate(existing.CreateNetworkContainerRequest.Version, req.NetworkContainerid) + ctx, cancel := context.WithTimeout(context.Background(), nmaAPICallTimeout) + defer cancel() + ncVersionListResp, err := service.nma.GetNCVersionList(ctx) + if err != nil { + logger.Errorf("failed to get nc version list from nmagent") + return cns.Response{ + ReturnCode: types.NmAgentInternalServerError, + Message: err.Error(), + } + } + nmaNCs := map[string]string{} + for _, nc := range ncVersionListResp.Containers { + nmaNCs[nc.NetworkContainerID] = nc.Version + } + _, returnCode, message := service.isNCWaitingForUpdate(existing.CreateNetworkContainerRequest.Version, req.NetworkContainerid, nmaNCs) if returnCode == types.NetworkContainerVfpProgramPending { return cns.Response{ ReturnCode: returnCode, @@ -777,8 +805,9 @@ func (service *HTTPRestService) populateIPConfigInfoUntransacted(ipConfigStatus // Return error and waitingForUpdate as true only CNS gets response from NMAgent indicating // the VFP programming is pending // This returns success / waitingForUpdate as false in all other cases. +// V2 is using the nmagent get nc version list api v2 which doesn't need authentication token func (service *HTTPRestService) isNCWaitingForUpdate( - ncVersion, ncid string, + ncVersion, ncid string, ncVersionList map[string]string, ) (waitingForUpdate bool, returnCode types.ResponseCode, message string) { ncStatus, ok := service.state.ContainerStatus[ncid] if ok { @@ -789,27 +818,21 @@ func (service *HTTPRestService) isNCWaitingForUpdate( } } - getNCVersionURL, ok := ncVersionURLs.Load(ncid) - if !ok { - logger.Printf("[Azure CNS] getNCVersionURL for Network container %s not found. Skipping GetNCVersionStatus check from NMAgent", - ncid) - return true, types.NetworkContainerVfpProgramCheckSkipped, "" - } - - resp, err := service.nma.GetNCVersion(context.TODO(), getNCVersionURL.(nmagent.NCVersionRequest)) - var nmaErr nmagent.Error - if errors.As(err, &nmaErr) && nmaErr.Unauthorized() { + ncTargetVersion, err := strconv.Atoi(ncVersion) + if err != nil { + // NMA doesn't have this NC version in string type, bail out + logger.Printf("[Azure CNS] NC %s version %v from NMAgent NC version list is not string "+ + "Skipping GetNCVersionStatus check from NMAgent", ncVersion, ncid) return true, types.NetworkContainerVfpProgramPending, "" } - - if err != nil { - logger.Printf("[Azure CNS] Failed to get NC version status from NMAgent with error: %+v. "+ - "Skipping GetNCVersionStatus check from NMAgent", err) - return true, types.NetworkContainerVfpProgramCheckSkipped, "" + nmaProgrammedNCVersionStr, ok := ncVersionList[ncid] + if !ok { + // NMA doesn't have this NC that we need programmed yet, bail out + logger.Printf("[Azure CNS] Failed to get NC %s doesn't exist in NMAgent NC version list "+ + "Skipping GetNCVersionStatus check from NMAgent", ncid) + return true, types.NetworkContainerVfpProgramPending, "" } - - ncTargetVersion, _ := strconv.Atoi(ncVersion) - nmaProgrammedNCVersion, err := strconv.Atoi(resp.Version) + nmaProgrammedNCVersion, err := strconv.Atoi(nmaProgrammedNCVersionStr) if err != nil { // it's unclear whether or not this can actually happen. In the NMAgent // documentation, Version is described as a string, but in practice the diff --git a/nmagent/client_test.go b/nmagent/client_test.go index 696a5171ec..acd73573a9 100644 --- a/nmagent/client_test.go +++ b/nmagent/client_test.go @@ -590,7 +590,7 @@ func TestGetNCVersionList(t *testing.T) { }, }, }, - "/machine/plugins?comp=nmagent&type=NetworkManagement%2Finterfaces%2Fapi-version%2F1", + "/machine/plugins?comp=nmagent&type=NetworkManagement%2Finterfaces%2Fapi-version%2F2", nmagent.NCVersionList{ Containers: []nmagent.NCVersion{ { @@ -606,7 +606,7 @@ func TestGetNCVersionList(t *testing.T) { map[string]interface{}{ "httpStatusCode": "500", }, - "/machine/plugins?comp=nmagent&type=NetworkManagement%2Finterfaces%2Fapi-version%2F1", + "/machine/plugins?comp=nmagent&type=NetworkManagement%2Finterfaces%2Fapi-version%2F2", nmagent.NCVersionList{}, true, }, diff --git a/nmagent/requests.go b/nmagent/requests.go index 336edeef26..1103dcd269 100644 --- a/nmagent/requests.go +++ b/nmagent/requests.go @@ -448,7 +448,7 @@ func (NCVersionListRequest) Method() string { // Path returns the path required to issue the request. func (NCVersionListRequest) Path() string { - return "/NetworkManagement/interfaces/api-version/1" + return "/NetworkManagement/interfaces/api-version/2" } // Validate performs any necessary validations for the request.