nmagent get nv version list api V2 refactor

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

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
-}

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

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
 	}

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
 )

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
 }
 

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

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