chore: tidy up nmagent client for context timeouts

Makefile

@@ -372,7 +372,7 @@ test-all:
 # run all tests
 .PHONY: test-integration
 test-integration:
-	go test -coverpkg=./... -v -race -covermode atomic -coverprofile=coverage.out -tags=integration ./test/integration...
+	go test -timeout 1h -coverpkg=./... -v -race -covermode atomic -coverprofile=coverage.out -tags=integration ./test/integration...
 
 .PHONY: test-cyclonus
 test-cyclonus:

cns/client/client_test.go

@@ -168,7 +168,7 @@ func TestMain(m *testing.M) {
 	logger.InitLogger(logName, 0, 0, tmpLogDir+"/")
 	config := common.ServiceConfig{}
 
-	httpRestService, err := restserver.NewHTTPRestService(&config, &fakes.WireserverClientFake{}, fakes.NewFakeNMAgentClient())
+	httpRestService, err := restserver.NewHTTPRestService(&config, &fakes.WireserverClientFake{}, &fakes.NMAgentClientFake{})
 	svc = httpRestService.(*restserver.HTTPRestService)
 	svc.Name = "cns-test-server"
 	fakeNNC := v1alpha.NodeNetworkConfig{

cns/configuration/configuration.go

@@ -151,9 +151,9 @@ func SetCNSConfigDefaults(config *CNSConfig) {
 		config.MetricsBindAddress = ":9090"
 	}
 	if config.SyncHostNCVersionIntervalMs == 0 {
-		config.SyncHostNCVersionIntervalMs = 1000
+		config.SyncHostNCVersionIntervalMs = 1000 * time.Millisecond //nolint:gomnd // default times
 	}
 	if config.SyncHostNCTimeoutMs == 0 {
-		config.SyncHostNCTimeoutMs = 500
+		config.SyncHostNCTimeoutMs = 500 * time.Millisecond //nolint:gomnd // default times
 	}
 }

cns/configuration/configuration_test.go

@@ -4,6 +4,7 @@ import (
 	"os"
 	"path/filepath"
 	"testing"
+	"time"
 
 	"github.com/Azure/azure-container-networking/common"
 	"github.com/stretchr/testify/assert"
@@ -187,8 +188,8 @@ func TestSetCNSConfigDefaults(t *testing.T) {
 					NodeSyncIntervalInSeconds: 30,
 				},
 				MetricsBindAddress:          ":9090",
-				SyncHostNCTimeoutMs:         500,
-				SyncHostNCVersionIntervalMs: 1000,
+				SyncHostNCTimeoutMs:         500 * time.Millisecond,
+				SyncHostNCVersionIntervalMs: 1000 * time.Millisecond,
 				TelemetrySettings: TelemetrySettings{
 					TelemetryBatchSizeBytes:      32768,
 					TelemetryBatchIntervalInSecs: 30,

cns/fakes/imdsclientfake.go

@@ -12,7 +12,7 @@ import (
 	"github.com/Azure/azure-container-networking/cns/wireserver"
 )
 
-var (
+const (
 	// HostPrimaryIP 10.0.0.4
 	HostPrimaryIP = "10.0.0.4"
 	// HostSubnet 10.0.0.0/24

cns/fakes/nmagentclientfake.go

@@ -6,19 +6,18 @@
 
 package fakes
 
-// NMAgentClientTest can be used to query to VM Host info.
-type NMAgentClientTest struct{}
+import (
+	"context"
 
-// NewFakeNMAgentClient return a mock implemetation of NMAgentClient
-func NewFakeNMAgentClient() *NMAgentClientTest {
-	return &NMAgentClientTest{}
+	"github.com/Azure/azure-container-networking/cns/nmagent"
+)
+
+// NMAgentClientFake can be used to query to VM Host info.
+type NMAgentClientFake struct {
+	GetNCVersionListFunc func(context.Context) (*nmagent.NetworkContainerListResponse, error)
 }
 
 // GetNcVersionListWithOutToken is mock implementation to return nc version list.
-func (nmagentclient *NMAgentClientTest) GetNcVersionListWithOutToken(ncNeedUpdateList []string) map[string]int {
-	ncVersionList := make(map[string]int)
-	for _, ncID := range ncNeedUpdateList {
-		ncVersionList[ncID] = 0
-	}
-	return ncVersionList
+func (c *NMAgentClientFake) GetNCVersionList(ctx context.Context) (*nmagent.NetworkContainerListResponse, error) {
+	return c.GetNCVersionListFunc(ctx)
 }

cns/nmagentclient/nmagentclient.go → cns/nmagent/client.go

@@ -1,17 +1,18 @@
-package nmagentclient
+package nmagent
 
 import (
 	"bytes"
+	"context"
 	"encoding/json"
 	"encoding/xml"
 	"fmt"
-	"io/ioutil"
+	"io"
 	"net/http"
-	"strconv"
 	"time"
 
 	"github.com/Azure/azure-container-networking/cns/logger"
 	"github.com/Azure/azure-container-networking/common"
+	"github.com/pkg/errors"
 )
 
 const (
@@ -27,14 +28,14 @@ var (
 	getNcVersionListWithOutTokenURLVersion = "2"
 )
 
-// NMANetworkContainerResponse - NMAgent response.
-type NMANetworkContainerResponse struct {
+// NetworkContainerResponse - NMAgent response.
+type NetworkContainerResponse struct {
 	ResponseCode       string `json:"httpStatusCode"`
 	NetworkContainerID string `json:"networkContainerId"`
 	Version            string `json:"version"`
 }
 
-type NMAgentSupportedApisResponseXML struct {
+type SupportedAPIsResponseXML struct {
 	SupportedApis []string `xml:"type"`
 }
 
@@ -43,35 +44,28 @@ type ContainerInfo struct {
 	Version            string `json:"version"`
 }
 
-type NMANetworkContainerListResponse struct {
+type NetworkContainerListResponse struct {
 	ResponseCode string          `json:"httpStatusCode"`
 	Containers   []ContainerInfo `json:"networkContainers"`
 }
 
-// NMAgentClient is client to handle queries to nmagent
-type NMAgentClient struct {
+// Client is client to handle queries to nmagent
+type Client struct {
 	connectionURL string
 }
 
-// NMAgentClientInterface has interface that nmagent client will handle
-type NMAgentClientInterface interface {
-	GetNcVersionListWithOutToken(ncNeedUpdateList []string) map[string]int
-}
-
-// NewNMAgentClient create a new nmagent client.
-func NewNMAgentClient(url string) (*NMAgentClient, error) {
+// NewClient create a new nmagent client.
+func NewClient(url string) (*Client, error) {
 	if url == "" {
 		url = fmt.Sprintf(GetNcVersionListWithOutTokenURLFmt, WireserverIP, getNcVersionListWithOutTokenURLVersion)
 	}
-	return &NMAgentClient{
+	return &Client{
 		connectionURL: url,
 	}, nil
 }
 
 // JoinNetwork joins the given network
-func JoinNetwork(
-	networkID string,
-	joinNetworkURL string) (*http.Response, error) {
+func JoinNetwork(networkID, joinNetworkURL string) (*http.Response, error) {
 	logger.Printf("[NMAgentClient] JoinNetwork: %s", networkID)
 
 	// Empty body is required as wireserver cannot handle a post without the body.
@@ -90,10 +84,7 @@ func JoinNetwork(
 }
 
 // PublishNetworkContainer publishes given network container
-func PublishNetworkContainer(
-	networkContainerID string,
-	createNetworkContainerURL string,
-	requestBodyData []byte) (*http.Response, error) {
+func PublishNetworkContainer(networkContainerID, createNetworkContainerURL string, requestBodyData []byte) (*http.Response, error) {
 	logger.Printf("[NMAgentClient] PublishNetworkContainer NC: %s", networkContainerID)
 
 	requestBody := bytes.NewBuffer(requestBodyData)
@@ -106,9 +97,7 @@ func PublishNetworkContainer(
 }
 
 // UnpublishNetworkContainer unpublishes given network container
-func UnpublishNetworkContainer(
-	networkContainerID string,
-	deleteNetworkContainerURL string) (*http.Response, error) {
+func UnpublishNetworkContainer(networkContainerID, deleteNetworkContainerURL string) (*http.Response, error) {
 	logger.Printf("[NMAgentClient] UnpublishNetworkContainer NC: %s", networkContainerID)
 
 	// Empty body is required as wireserver cannot handle a post without the body.
@@ -123,9 +112,7 @@ func UnpublishNetworkContainer(
 }
 
 // GetNetworkContainerVersion :- Retrieves NC version from NMAgent
-func GetNetworkContainerVersion(
-	networkContainerID,
-	getNetworkContainerVersionURL string) (*http.Response, error) {
+func GetNetworkContainerVersion(networkContainerID, getNetworkContainerVersionURL string) (*http.Response, error) {
 	logger.Printf("[NMAgentClient] GetNetworkContainerVersion NC: %s", networkContainerID)
 
 	response, err := common.GetHttpClient().Get(getNetworkContainerVersionURL)
@@ -136,43 +123,48 @@ func GetNetworkContainerVersion(
 }
 
 // GetNmAgentSupportedApis :- Retrieves Supported Apis from NMAgent
-func GetNmAgentSupportedApis(
-	httpc *http.Client,
-	getNmAgentSupportedApisURL string) ([]string, error) {
+func GetNmAgentSupportedApis(httpc *http.Client, getNmAgentSupportedApisURL string) ([]string, error) {
 	var returnErr error
 
 	if getNmAgentSupportedApisURL == "" {
 		getNmAgentSupportedApisURL = fmt.Sprintf(
 			GetNmAgentSupportedApiURLFmt, WireserverIP)
 	}
 
-	response, err := httpc.Get(getNmAgentSupportedApisURL)
+	req, err := http.NewRequestWithContext(context.TODO(), http.MethodGet, getNmAgentSupportedApisURL, nil)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to build request")
+	}
+
+	resp, err := httpc.Do(req)
 	if err != nil {
 		returnErr = fmt.Errorf(
 			"Failed to retrieve Supported Apis from NMAgent with error %v",
 			err.Error())
 		logger.Errorf("[Azure-CNS] %s", returnErr)
 		return nil, returnErr
 	}
-	if response == nil {
+	if resp == nil {
 		returnErr = fmt.Errorf(
 			"Response from getNmAgentSupportedApis call is <nil>")
 		logger.Errorf("[Azure-CNS] %s", returnErr)
 		return nil, returnErr
 	}
-	defer response.Body.Close()
-
-	if response.StatusCode != http.StatusOK {
+	defer resp.Body.Close()
+	b, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to read response body")
+	}
+	if resp.StatusCode != http.StatusOK {
 		returnErr = fmt.Errorf(
 			"Failed to retrieve Supported Apis from NMAgent with StatusCode: %d",
-			response.StatusCode)
+			resp.StatusCode)
 		logger.Errorf("[Azure-CNS] %s", returnErr)
 		return nil, returnErr
 	}
 
-	var xmlDoc NMAgentSupportedApisResponseXML
-	decoder := xml.NewDecoder(response.Body)
-	err = decoder.Decode(&xmlDoc)
+	var xmlDoc SupportedAPIsResponseXML
+	err = xml.NewDecoder(bytes.NewReader(b)).Decode(&xmlDoc)
 	if err != nil {
 		returnErr = fmt.Errorf(
 			"Failed to decode XML response of Supported Apis from NMAgent with error %v",
@@ -181,44 +173,35 @@ func GetNmAgentSupportedApis(
 		return nil, returnErr
 	}
 
-	logger.Printf("[NMAgentClient][Response] GetNmAgentSupportedApis. Response: %+v.", response)
+	logger.Printf("[NMAgentClient][Response] GetNmAgentSupportedApis. Response: %+v.", resp)
 	return xmlDoc.SupportedApis, nil
 }
 
-// GetNcVersionListWithOutToken query nmagent for programmed container version.
-func (nmagentclient *NMAgentClient) GetNcVersionListWithOutToken(ncNeedUpdateList []string) map[string]int {
-	ncVersionList := make(map[string]int)
+// GetNCVersionList query nmagent for programmed container versions.
+func (c *Client) GetNCVersionList(ctx context.Context) (*NetworkContainerListResponse, error) {
 	now := time.Now()
-	response, err := http.Get(nmagentclient.connectionURL)
-	latency := time.Since(now)
-	logger.Printf("[NMAgentClient][Response] GetNcVersionListWithOutToken response: %+v, latency is %d", response, latency.Milliseconds())
-
-	if response.StatusCode != http.StatusOK {
-		logger.Printf("[NMAgentClient][Response] GetNcVersionListWithOutToken failed with %d, err is %v", response.StatusCode, err)
-		return nil
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.connectionURL, nil)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to build nmagent request")
 	}
-
-	var nmaNcListResponse NMANetworkContainerListResponse
-	rBytes, _ := ioutil.ReadAll(response.Body)
-	if err := json.Unmarshal(rBytes, &nmaNcListResponse); err != nil {
-		logger.Printf("[NMAgentClient][Response] GetNcVersionListWithOutToken unmarshal failed with %s", err)
-		return nil
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to make nmagent request")
 	}
-	logger.Printf("NMAgent NC List Response is %s", nmaNcListResponse)
+	defer resp.Body.Close()
+	b, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to read response body")
+	}
+	logger.Printf("[NMAgentClient][Response] GetNcVersionListWithOutToken response: %s, latency is %d", string(b), time.Since(now).Milliseconds())
 
-	receivedNcVersionListInMap := make(map[string]string)
-	for _, containers := range nmaNcListResponse.Containers {
-		receivedNcVersionListInMap[containers.NetworkContainerID] = containers.Version
+	if resp.StatusCode != http.StatusOK {
+		return nil, errors.Wrap(err, "failed to GetNCVersionList")
 	}
-	for _, ncID := range ncNeedUpdateList {
-		if version, ok := receivedNcVersionListInMap[ncID]; ok {
-			if versionInInt, err := strconv.Atoi(version); err != nil {
-				logger.Printf("[NMAgentClient][Response] GetNcVersionListWithOutToken translate version %s to int failed with %s", version, err)
-			} else {
-				ncVersionList[ncID] = versionInInt
-				logger.Printf("Containers id is %s, programmed NC version is %d", ncID, versionInInt)
-			}
-		}
+
+	var response NetworkContainerListResponse
+	if err := json.Unmarshal(b, &response); err != nil {
+		return nil, errors.Wrap(err, "failed to unmarshal response")
 	}
-	return ncVersionList
+	return &response, nil
 }