Address comments

Author: NihaNallappagari

cns/fakes/imdsclientfake.go

@@ -58,14 +58,20 @@ func (m *MockIMDSClient) GetVMUniqueID(ctx context.Context) (string, error) {
 	return "55b8499d-9b42-4f85-843f-24ff69f4a643", nil
 }
 
-func (m *MockIMDSClient) GetNCVersionsFromIMDS(ctx context.Context) (map[string]string, error) {
+func (m *MockIMDSClient) GetNCVersions(ctx context.Context) ([]imds.NetworkInterface, error) {
 	if ctx.Value(SimulateError) != nil {
 		return nil, imds.ErrUnexpectedStatusCode
 	}
 
 	// Return some mock NC versions for testing
-	return map[string]string{
-		"nc1": "1",
-		"nc2": "2",
+	return []imds.NetworkInterface{
+		{
+			InterfaceCompartmentId:      "nc1",
+			InterfaceCompartmentVersion: "1",
+		},
+		{
+			InterfaceCompartmentId:      "nc2",
+			InterfaceCompartmentVersion: "2",
+		},
 	}, nil
 }

cns/imds/client.go

@@ -53,7 +53,6 @@ const (
 	metadataHeaderValue   = "true"
 	defaultRetryAttempts  = 3
 	defaultIMDSEndpoint   = "http://169.254.169.254"
-	ncVersion             = "ncVersion"
 )
 
 var (
@@ -104,7 +103,7 @@ func (c *Client) GetVMUniqueID(ctx context.Context) (string, error) {
 	return vmUniqueID, nil
 }
 
-func (c *Client) GetNCVersionsFromIMDS(ctx context.Context) (map[string]string, error) {
+func (c *Client) GetNCVersions(ctx context.Context) ([]NetworkInterface, error) {
 	var networkData NetworkMetadata
 	err := retry.Do(func() error {
 		networkMetadata, err := c.getInstanceMetadata(ctx, imdsNetworkPath)
@@ -128,20 +127,7 @@ func (c *Client) GetNCVersionsFromIMDS(ctx context.Context) (map[string]string,
 		return nil, err
 	}
 
-	ncVersions := make(map[string]string)
-	for _, iface := range networkData.Interface {
-		// IMDS only returns compartment fields (interfaceCompartmentId, interfaceCompartmentVersion)
-		// We map these to NC ID and NC version concepts
-		// Standard fields (ncId, ncVersion) are ignored even if present
-		ncId := iface.InterfaceCompartmentId
-		ncVersion := iface.InterfaceCompartmentVersion
-
-		if ncId != "" {
-			ncVersions[ncId] = ncVersion
-		}
-	}
-
-	return ncVersions, nil
+	return networkData.Interface, nil
 }
 
 func (c *Client) getInstanceMetadata(ctx context.Context, imdsComputePath string) (map[string]any, error) {
@@ -178,7 +164,6 @@ func (c *Client) getInstanceMetadata(ctx context.Context, imdsComputePath string
 
 // NetworkInterface represents a network interface from IMDS
 type NetworkInterface struct {
-	MacAddress string `json:"macAddress"`
 	// IMDS only returns compartment fields - these are mapped to NC ID and NC version concepts
 	InterfaceCompartmentId      string `json:"interfaceCompartmentId,omitempty"`
 	InterfaceCompartmentVersion string `json:"interfaceCompartmentVersion,omitempty"`

cns/imds/client_test.go

@@ -101,18 +101,16 @@ func TestInvalidVMUniqueID(t *testing.T) {
 	require.Equal(t, "", vmUniqueID)
 }
 
-func TestGetNCVersionsFromIMDS(t *testing.T) {
+func TestGetNCVersions(t *testing.T) {
 	networkMetadata := []byte(`{
 		"interface": [
 			{
-				"macAddress": "00:0D:3A:12:34:56",
 				"interfaceCompartmentVersion": "1",
 				"interfaceCompartmentId": "nc-12345-67890"
 			},
 			{
-				"macAddress": "00:0D:3A:CD:EF:12",
-				"interfaceCompartmentVersion": "",
-				"interfaceCompartmentId": "nc-abcdef-123456"
+				"interfaceCompartmentVersion": "1",
+				"interfaceCompartmentId": ""
 			}
 		]
 	}`)
@@ -138,23 +136,28 @@ func TestGetNCVersionsFromIMDS(t *testing.T) {
 	defer mockIMDSServer.Close()
 
 	imdsClient := imds.NewClient(imds.Endpoint(mockIMDSServer.URL))
-	ncVersions, err := imdsClient.GetNCVersionsFromIMDS(context.Background())
+	interfaces, err := imdsClient.GetNCVersions(context.Background())
 	require.NoError(t, err, "error querying testserver")
 
-	expectedNCVersions := map[string]string{
-		"nc-12345-67890":   "1",
-		"nc-abcdef-123456": "", // empty version
-	}
-	require.Equal(t, expectedNCVersions, ncVersions)
+	// Verify we got the expected interfaces
+	require.Len(t, interfaces, 2, "expected 2 interfaces")
+
+	// Check first interface
+	assert.Equal(t, "nc-12345-67890", interfaces[0].InterfaceCompartmentId)
+	assert.Equal(t, "1", interfaces[0].InterfaceCompartmentVersion)
+
+	// Check second interface
+	assert.Equal(t, "", interfaces[1].InterfaceCompartmentId)
+	assert.Equal(t, "1", interfaces[1].InterfaceCompartmentVersion)
 }
 
-func TestGetNCVersionsFromIMDSInvalidEndpoint(t *testing.T) {
+func TestGetNCVersionsInvalidEndpoint(t *testing.T) {
 	imdsClient := imds.NewClient(imds.Endpoint(string([]byte{0x7f})), imds.RetryAttempts(1))
-	_, err := imdsClient.GetNCVersionsFromIMDS(context.Background())
+	_, err := imdsClient.GetNCVersions(context.Background())
 	require.Error(t, err, "expected invalid path")
 }
 
-func TestGetNCVersionsFromIMDSInvalidJSON(t *testing.T) {
+func TestGetNCVersionsInvalidJSON(t *testing.T) {
 	mockIMDSServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(http.StatusOK)
 		_, err := w.Write([]byte("not json"))
@@ -163,15 +166,14 @@ func TestGetNCVersionsFromIMDSInvalidJSON(t *testing.T) {
 	defer mockIMDSServer.Close()
 
 	imdsClient := imds.NewClient(imds.Endpoint(mockIMDSServer.URL), imds.RetryAttempts(1))
-	_, err := imdsClient.GetNCVersionsFromIMDS(context.Background())
+	_, err := imdsClient.GetNCVersions(context.Background())
 	require.Error(t, err, "expected json decoding error")
 }
 
-func TestGetNCVersionsFromIMDSNoNCIDs(t *testing.T) {
+func TestGetNCVersionsNoNCIDs(t *testing.T) {
 	networkMetadataNoNC := []byte(`{
 		"interface": [
 			{
-				"macAddress": "00:0D:3A:12:34:56",
 				"ipv4": {
 					"ipAddress": [
 						{
@@ -180,17 +182,6 @@ func TestGetNCVersionsFromIMDSNoNCIDs(t *testing.T) {
 						}
 					]
 				}
-			},
-			{
-				"macAddress": "00:0D:3A:78:90:AB",
-				"ipv4": {
-					"ipAddress": [
-						{
-							"privateIpAddress": "10.0.1.4",
-							"publicIpAddress": ""
-						}
-					]
-				}
 			}
 		]
 	}`)
@@ -206,7 +197,10 @@ func TestGetNCVersionsFromIMDSNoNCIDs(t *testing.T) {
 	defer mockIMDSServer.Close()
 
 	imdsClient := imds.NewClient(imds.Endpoint(mockIMDSServer.URL))
-	ncVersions, err := imdsClient.GetNCVersionsFromIMDS(context.Background())
+	interfaces, err := imdsClient.GetNCVersions(context.Background())
 	require.NoError(t, err, "error querying testserver")
-	require.Empty(t, ncVersions, "expected empty NC versions map when no NC IDs present")
+
+	// Verify we got interfaces but they don't have compartment IDs
+	require.Len(t, interfaces, 1, "expected 1 interface")
+	assert.Equal(t, "", interfaces[0].InterfaceCompartmentId)
 }

cns/restserver/internalapi.go

@@ -172,7 +172,6 @@ func (service *HTTPRestService) SyncHostNCVersion(ctx context.Context, channelMo
 	service.Lock()
 	defer service.Unlock()
 	start := time.Now()
-
 	programmedNCCount, err := service.syncHostNCVersion(ctx, channelMode)
 	// even if we get an error, we want to write the CNI conflist if we have any NC programmed to any version
 	if programmedNCCount > 0 {
@@ -208,7 +207,7 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo
 			logger.Errorf("Received err when change nc version %s in containerstatus to int, err msg %v", service.state.ContainerStatus[idx].CreateNetworkContainerRequest.Version, err)
 			continue
 		}
-		logger.Printf("NC %s: local NC version %d, DNC NC version %d", service.state.ContainerStatus[idx].ID, localNCVersion, dncNCVersion)
+
 		// host NC version is the NC version from NMAgent, if it's smaller than NC version from DNC, then append it to indicate it needs update.
 		if localNCVersion < dncNCVersion {
 			outdatedNCs[service.state.ContainerStatus[idx].ID] = struct{}{}
@@ -223,7 +222,7 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo
 	if len(outdatedNCs) == 0 {
 		return len(programmedNCs), nil
 	}
-	logger.Printf("outdatedNCs: %v", outdatedNCs)
+
 	ncVersionListResp, err := service.nma.GetNCVersionList(ctx)
 	if err != nil {
 		return len(programmedNCs), errors.Wrap(err, "failed to get nc version list from nmagent")
@@ -232,7 +231,6 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo
 	// Get IMDS NC versions for delegated NIC scenarios
 	imdsNCVersions, err := service.GetIMDSNCVersions(ctx)
 	if err != nil {
-		logger.Printf("Failed to get NC versions from IMDS: %v", err)
 		// If any of the NMA API check calls, imds calls fails assume that nma build doesn't have the latest changes and create empty map
 		imdsNCVersions = make(map[string]string)
 	}
@@ -249,7 +247,7 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo
 	}
 	for ncID, version := range imdsNCVersions {
 		if _, exists := consolidatedNCs[ncID]; !exists {
-			consolidatedNCs[ncID] = version
+			consolidatedNCs[strings.ToLower(ncID)] = version
 		}
 	}
 	hasNC.Set(float64(len(consolidatedNCs)))
@@ -298,7 +296,7 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo
 	// if we didn't empty out the needs update set, NMA has not programmed all the NCs we are expecting, and we
 	// need to return an error indicating that
 	if len(outdatedNCs) > 0 {
-		return len(programmedNCs), errors.Errorf("unabled to update some NCs: %v, missing or bad response from NMA or ipam", outdatedNCs)
+		return len(programmedNCs), errors.Errorf("unable to update some NCs: %v, missing or bad response from NMA or IMDS", outdatedNCs)
 	}
 
 	return len(programmedNCs), nil
@@ -359,7 +357,6 @@ func (service *HTTPRestService) ReconcileIPAssignment(podInfoByIP map[string]cns
 
 		for _, ip := range ips {
 			if ncReq, ok := allSecIPsIdx[ip.String()]; ok {
-				logger.Printf("secondary ip %s is assigned to pod %+v, ncId: %s ncVersion: %s", ip, podIPs, ncReq.NetworkContainerid, ncReq.Version)
 				desiredIPs = append(desiredIPs, ip.String())
 				ncIDs = append(ncIDs, ncReq.NetworkContainerid)
 			} else {
@@ -388,7 +385,7 @@ func (service *HTTPRestService) ReconcileIPAssignment(podInfoByIP map[string]cns
 		}
 
 		if _, err := requestIPConfigsHelper(service, ipconfigsRequest); err != nil {
-			logger.Errorf("requestIPConfigsHelper failed for pod key %s, podInfo %+v, ncIds %v, error: %v", podKey, podIPs, ncIDs, err)
+			logger.Errorf("requestIPConfigsHelper failed for pod key %s, podInfo %+v, ncIDs %v, error: %v", podKey, podIPs, ncIDs, err)
 			return types.FailedToAllocateIPConfig
 		}
 	}
@@ -666,54 +663,51 @@ func (service *HTTPRestService) SetVFForAccelnetNICs() error {
 func (service *HTTPRestService) checkNMAgentAPISupport(ctx context.Context) (swiftV2Support bool, err error) {
 	// Use the existing NMAgent client instead of direct HTTP calls
 	if service.nma == nil {
-		return false, fmt.Errorf("NMAgent client is not available")
+		return false, errors.New("NMAgent client is not available")
 	}
 
 	apis, err := service.nma.SupportedAPIs(ctx)
 	if err != nil {
-		return false, fmt.Errorf("failed to get supported APIs from NMAgent client: %w", err)
+		logger.Errorf("failed to get supported APIs from NMAgent client: %v", err)
+		return false, errors.New("failed to get supported APIs from NMAgent client")
 	}
 
-	logger.Printf("[checkNMAgentAPISupport] Found %d APIs from NMAgent client", len(apis))
-	for i, api := range apis {
-		logger.Printf("[checkNMAgentAPISupport] API %d: %s", i+1, api)
-
-		if strings.Contains(api, nmAgentSwiftV2API) { // change
+	for _, api := range apis {
+		if strings.Contains(api, nmAgentSwiftV2API) {
 			swiftV2Support = true
 		}
 	}
 
-	logger.Printf("[checkNMAgentAPISupport] Support check - SwiftV2: %t", swiftV2Support)
 	return swiftV2Support, nil
 }
 
 // GetIMDSNCVersions gets NC versions from IMDS and returns them as a map
 func (service *HTTPRestService) GetIMDSNCVersions(ctx context.Context) (map[string]string, error) {
-	logger.Printf("[GetIMDSNCVersions] Getting NC versions from IMDS")
-
 	// Check NMAgent API support for SwiftV2, if it fails return empty map assuming support might not be available in that nma build
 	swiftV2Support, err := service.checkNMAgentAPISupport(ctx)
-	if err != nil {
-		logger.Printf("[GetIMDSNCVersions] Failed to check NMAgent API support, returning empty map: %v", err)
-		return make(map[string]string), nil
-	}
-
-	if !swiftV2Support {
-		logger.Printf("[GetIMDSNCVersions] SwiftV2 API not supported, returning empty NC versions map")
+	if err != nil || !swiftV2Support {
 		return make(map[string]string), nil
 	}
 
-	logger.Printf("[GetIMDSNCVersions] SwiftV2 support API exists (%t), proceeding to get NC versions from IMDS", swiftV2Support)
-
 	imdsClient := service.imdsClient
 
 	// Get all NC versions from IMDS
-	ncVersions, err := imdsClient.GetNCVersionsFromIMDS(ctx)
+	networkInterfaces, err := imdsClient.GetNCVersions(ctx)
 	if err != nil {
-		logger.Printf("[GetIMDSNCVersions] Failed to get NC versions from IMDS: %v", err)
 		return make(map[string]string), nil
 	}
 
-	logger.Printf("[GetIMDSNCVersions] Successfully got %d NC versions from IMDS", len(ncVersions))
+	// Build ncVersions map from the network interfaces
+	ncVersions := make(map[string]string)
+	for _, iface := range networkInterfaces {
+		// IMDS returns interfaceCompartmentId, interfaceCompartmentVersion fields, as nc id guid has different context on nma. We map these to NC ID and NC version
+		ncID := iface.InterfaceCompartmentId
+		ncVersion := iface.InterfaceCompartmentVersion
+
+		if ncID != "" {
+			ncVersions[ncID] = ncVersion
+		}
+	}
+
 	return ncVersions, nil
 }