Skip to content

Commit eaad57b

Browse files
NihaNallappagariNihaNallappagari
committed
Address comments
1 parent e4e92ca commit eaad57b

File tree

6 files changed

+93
-75
lines changed

6 files changed

+93
-75
lines changed

‎cns/fakes/imdsclientfake.go‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,14 +58,20 @@ func (m *MockIMDSClient) GetVMUniqueID(ctx context.Context) (string, error) {
5858
return "55b8499d-9b42-4f85-843f-24ff69f4a643", nil
5959
}
6060

61-
func (m *MockIMDSClient) GetNCVersionsFromIMDS(ctx context.Context) (map[string]string, error) {
61+
func (m *MockIMDSClient) GetNCVersions(ctx context.Context) ([]imds.NetworkInterface, error) {
6262
if ctx.Value(SimulateError) != nil {
6363
return nil, imds.ErrUnexpectedStatusCode
6464
}
6565

6666
// Return some mock NC versions for testing
67-
return map[string]string{
68-
"nc1": "1",
69-
"nc2": "2",
67+
return []imds.NetworkInterface{
68+
{
69+
InterfaceCompartmentId: "nc1",
70+
InterfaceCompartmentVersion: "1",
71+
},
72+
{
73+
InterfaceCompartmentId: "nc2",
74+
InterfaceCompartmentVersion: "2",
75+
},
7076
}, nil
7177
}

‎cns/imds/client.go‎

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@ const (
5353
metadataHeaderValue = "true"
5454
defaultRetryAttempts = 3
5555
defaultIMDSEndpoint = "http://169.254.169.254"
56-
ncVersion = "ncVersion"
5756
)
5857

5958
var (
@@ -104,7 +103,7 @@ func (c *Client) GetVMUniqueID(ctx context.Context) (string, error) {
104103
return vmUniqueID, nil
105104
}
106105

107-
func (c *Client) GetNCVersionsFromIMDS(ctx context.Context) (map[string]string, error) {
106+
func (c *Client) GetNCVersions(ctx context.Context) ([]NetworkInterface, error) {
108107
var networkData NetworkMetadata
109108
err := retry.Do(func() error {
110109
networkMetadata, err := c.getInstanceMetadata(ctx, imdsNetworkPath)
@@ -128,20 +127,7 @@ func (c *Client) GetNCVersionsFromIMDS(ctx context.Context) (map[string]string,
128127
return nil, err
129128
}
130129

131-
ncVersions := make(map[string]string)
132-
for _, iface := range networkData.Interface {
133-
// IMDS only returns compartment fields (interfaceCompartmentId, interfaceCompartmentVersion)
134-
// We map these to NC ID and NC version concepts
135-
// Standard fields (ncId, ncVersion) are ignored even if present
136-
ncId := iface.InterfaceCompartmentId
137-
ncVersion := iface.InterfaceCompartmentVersion
138-
139-
if ncId != "" {
140-
ncVersions[ncId] = ncVersion
141-
}
142-
}
143-
144-
return ncVersions, nil
130+
return networkData.Interface, nil
145131
}
146132

147133
func (c *Client) getInstanceMetadata(ctx context.Context, imdsComputePath string) (map[string]any, error) {

‎cns/imds/client_test.go‎

Lines changed: 29 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ func TestInvalidVMUniqueID(t *testing.T) {
101101
require.Equal(t, "", vmUniqueID)
102102
}
103103

104-
func TestGetNCVersionsFromIMDS(t *testing.T) {
104+
func TestGetNCVersions(t *testing.T) {
105105
networkMetadata := []byte(`{
106106
"interface": [
107107
{
@@ -111,8 +111,8 @@ func TestGetNCVersionsFromIMDS(t *testing.T) {
111111
},
112112
{
113113
"macAddress": "00:0D:3A:CD:EF:12",
114-
"interfaceCompartmentVersion": "",
115-
"interfaceCompartmentId": "nc-abcdef-123456"
114+
"interfaceCompartmentVersion": "1",
115+
"interfaceCompartmentId": ""
116116
}
117117
]
118118
}`)
@@ -138,23 +138,30 @@ func TestGetNCVersionsFromIMDS(t *testing.T) {
138138
defer mockIMDSServer.Close()
139139

140140
imdsClient := imds.NewClient(imds.Endpoint(mockIMDSServer.URL))
141-
ncVersions, err := imdsClient.GetNCVersionsFromIMDS(context.Background())
141+
interfaces, err := imdsClient.GetNCVersions(context.Background())
142142
require.NoError(t, err, "error querying testserver")
143143

144-
expectedNCVersions := map[string]string{
145-
"nc-12345-67890": "1",
146-
"nc-abcdef-123456": "", // empty version
147-
}
148-
require.Equal(t, expectedNCVersions, ncVersions)
144+
// Verify we got the expected interfaces
145+
require.Len(t, interfaces, 2, "expected 2 interfaces")
146+
147+
// Check first interface
148+
assert.Equal(t, "00:0D:3A:12:34:56", interfaces[0].MacAddress)
149+
assert.Equal(t, "nc-12345-67890", interfaces[0].InterfaceCompartmentId)
150+
assert.Equal(t, "1", interfaces[0].InterfaceCompartmentVersion)
151+
152+
// Check second interface
153+
assert.Equal(t, "00:0D:3A:CD:EF:12", interfaces[1].MacAddress)
154+
assert.Equal(t, "", interfaces[1].InterfaceCompartmentId)
155+
assert.Equal(t, "1", interfaces[1].InterfaceCompartmentVersion)
149156
}
150157

151-
func TestGetNCVersionsFromIMDSInvalidEndpoint(t *testing.T) {
158+
func TestGetNCVersionsInvalidEndpoint(t *testing.T) {
152159
imdsClient := imds.NewClient(imds.Endpoint(string([]byte{0x7f})), imds.RetryAttempts(1))
153-
_, err := imdsClient.GetNCVersionsFromIMDS(context.Background())
160+
_, err := imdsClient.GetNCVersions(context.Background())
154161
require.Error(t, err, "expected invalid path")
155162
}
156163

157-
func TestGetNCVersionsFromIMDSInvalidJSON(t *testing.T) {
164+
func TestGetNCVersionsInvalidJSON(t *testing.T) {
158165
mockIMDSServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
159166
w.WriteHeader(http.StatusOK)
160167
_, err := w.Write([]byte("not json"))
@@ -163,11 +170,11 @@ func TestGetNCVersionsFromIMDSInvalidJSON(t *testing.T) {
163170
defer mockIMDSServer.Close()
164171

165172
imdsClient := imds.NewClient(imds.Endpoint(mockIMDSServer.URL), imds.RetryAttempts(1))
166-
_, err := imdsClient.GetNCVersionsFromIMDS(context.Background())
173+
_, err := imdsClient.GetNCVersions(context.Background())
167174
require.Error(t, err, "expected json decoding error")
168175
}
169176

170-
func TestGetNCVersionsFromIMDSNoNCIDs(t *testing.T) {
177+
func TestGetNCVersionsNoNCIDs(t *testing.T) {
171178
networkMetadataNoNC := []byte(`{
172179
"interface": [
173180
{
@@ -180,17 +187,6 @@ func TestGetNCVersionsFromIMDSNoNCIDs(t *testing.T) {
180187
}
181188
]
182189
}
183-
},
184-
{
185-
"macAddress": "00:0D:3A:78:90:AB",
186-
"ipv4": {
187-
"ipAddress": [
188-
{
189-
"privateIpAddress": "10.0.1.4",
190-
"publicIpAddress": ""
191-
}
192-
]
193-
}
194190
}
195191
]
196192
}`)
@@ -206,7 +202,13 @@ func TestGetNCVersionsFromIMDSNoNCIDs(t *testing.T) {
206202
defer mockIMDSServer.Close()
207203

208204
imdsClient := imds.NewClient(imds.Endpoint(mockIMDSServer.URL))
209-
ncVersions, err := imdsClient.GetNCVersionsFromIMDS(context.Background())
205+
interfaces, err := imdsClient.GetNCVersions(context.Background())
210206
require.NoError(t, err, "error querying testserver")
211-
require.Empty(t, ncVersions, "expected empty NC versions map when no NC IDs present")
207+
208+
// Verify we got interfaces but they don't have compartment IDs
209+
require.Len(t, interfaces, 1, "expected 1 interface")
210+
211+
// Check that interfaces have MAC addresses but no compartment IDs
212+
assert.Equal(t, "00:0D:3A:12:34:56", interfaces[0].MacAddress)
213+
assert.Equal(t, "", interfaces[0].InterfaceCompartmentId)
212214
}

‎cns/restserver/internalapi.go‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,7 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo
249249
}
250250
for ncID, version := range imdsNCVersions {
251251
if _, exists := consolidatedNCs[ncID]; !exists {
252-
consolidatedNCs[ncID] = version
252+
consolidatedNCs[strings.ToLower(ncID)] = version
253253
}
254254
}
255255
hasNC.Set(float64(len(consolidatedNCs)))
@@ -298,7 +298,7 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo
298298
// if we didn't empty out the needs update set, NMA has not programmed all the NCs we are expecting, and we
299299
// need to return an error indicating that
300300
if len(outdatedNCs) > 0 {
301-
return len(programmedNCs), errors.Errorf("unabled to update some NCs: %v, missing or bad response from NMA or ipam", outdatedNCs)
301+
return len(programmedNCs), errors.Errorf("unable to update some NCs: %v, missing or bad response from NMA or IMDS", outdatedNCs)
302302
}
303303

304304
return len(programmedNCs), nil
@@ -708,12 +708,25 @@ func (service *HTTPRestService) GetIMDSNCVersions(ctx context.Context) (map[stri
708708
imdsClient := service.imdsClient
709709

710710
// Get all NC versions from IMDS
711-
ncVersions, err := imdsClient.GetNCVersionsFromIMDS(ctx)
711+
networkInterfaces, err := imdsClient.GetNCVersions(ctx)
712712
if err != nil {
713713
logger.Printf("[GetIMDSNCVersions] Failed to get NC versions from IMDS: %v", err)
714714
return make(map[string]string), nil
715715
}
716716

717+
// Build ncVersions map from the network interfaces
718+
ncVersions := make(map[string]string)
719+
for _, iface := range networkInterfaces {
720+
// IMDS only returns compartment fields (interfaceCompartmentId, interfaceCompartmentVersion)
721+
// We map these to NC ID and NC version concepts
722+
ncId := iface.InterfaceCompartmentId
723+
ncVersion := iface.InterfaceCompartmentVersion
724+
725+
if ncId != "" {
726+
ncVersions[ncId] = ncVersion
727+
}
728+
}
729+
717730
logger.Printf("[GetIMDSNCVersions] Successfully got %d NC versions from IMDS", len(ncVersions))
718731
return ncVersions, nil
719732
}

‎cns/restserver/internalapi_test.go‎

Lines changed: 34 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"github.com/Azure/azure-container-networking/cns/common"
2121
"github.com/Azure/azure-container-networking/cns/configuration"
2222
"github.com/Azure/azure-container-networking/cns/fakes"
23+
"github.com/Azure/azure-container-networking/cns/imds"
2324
"github.com/Azure/azure-container-networking/cns/types"
2425
"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
2526
nma "github.com/Azure/azure-container-networking/nmagent"
@@ -271,11 +272,14 @@ func TestSyncHostNCVersion(t *testing.T) {
271272

272273
// Create a custom IMDS mock that returns the second NC
273274
mockIMDS := &struct {
274-
ncVersions func(ctx context.Context) (map[string]string, error)
275+
ncVersions func(ctx context.Context) ([]imds.NetworkInterface, error)
275276
}{
276-
ncVersions: func(ctx context.Context) (map[string]string, error) {
277-
return map[string]string{
278-
imdsNCID: "2",
277+
ncVersions: func(ctx context.Context) ([]imds.NetworkInterface, error) {
278+
return []imds.NetworkInterface{
279+
{
280+
InterfaceCompartmentId: imdsNCID,
281+
InterfaceCompartmentVersion: "2",
282+
},
279283
}, nil
280284
},
281285
}
@@ -368,11 +372,14 @@ func TestSyncHostNCVersionErrorMissingNC(t *testing.T) {
368372

369373
// Create IMDS mock that returns 1 NC but with different ID (not matching the outdated NC)
370374
mockIMDS := &struct {
371-
ncVersions func(ctx context.Context) (map[string]string, error)
375+
ncVersions func(ctx context.Context) ([]imds.NetworkInterface, error)
372376
}{
373-
ncVersions: func(ctx context.Context) (map[string]string, error) {
374-
return map[string]string{
375-
"different-nc-id": "1", // IMDS has a different NC, not the outdated one
377+
ncVersions: func(ctx context.Context) ([]imds.NetworkInterface, error) {
378+
return []imds.NetworkInterface{
379+
{
380+
InterfaceCompartmentId: "different-nc-id",
381+
InterfaceCompartmentVersion: "1", // IMDS has a different NC, not the outdated one
382+
},
376383
}, nil
377384
},
378385
}
@@ -401,7 +408,7 @@ func TestSyncHostNCVersionErrorMissingNC(t *testing.T) {
401408
}
402409

403410
// Check that the error message contains the expected text
404-
expectedErrorText := "unabled to update some NCs"
411+
expectedErrorText := "unable to update some NCs"
405412
if !strings.Contains(err.Error(), expectedErrorText) {
406413
t.Errorf("Expected error to contain '%s', but got: %v", expectedErrorText, err)
407414
}
@@ -427,11 +434,14 @@ func TestSyncHostNCVersionLocalVersionHigher(t *testing.T) {
427434

428435
// Create IMDS mock that returns lower version(2) than local host version(3)
429436
mockIMDS := &struct {
430-
ncVersions func(ctx context.Context) (map[string]string, error)
437+
ncVersions func(ctx context.Context) ([]imds.NetworkInterface, error)
431438
}{
432-
ncVersions: func(ctx context.Context) (map[string]string, error) {
433-
return map[string]string{
434-
req.NetworkContainerid: "1", // IMDS has version 1, which is lower than local version 3
439+
ncVersions: func(ctx context.Context) ([]imds.NetworkInterface, error) {
440+
return []imds.NetworkInterface{
441+
{
442+
InterfaceCompartmentId: req.NetworkContainerid,
443+
InterfaceCompartmentVersion: "1", // IMDS has version 1, which is lower than local version 3
444+
},
435445
}, nil
436446
},
437447
}
@@ -482,10 +492,10 @@ func TestSyncHostNCVersionLocalHigherThanDNC(t *testing.T) {
482492
svc.Unlock()
483493

484494
mockIMDS := &struct {
485-
ncVersions func(ctx context.Context) (map[string]string, error)
495+
ncVersions func(ctx context.Context) ([]imds.NetworkInterface, error)
486496
}{
487-
ncVersions: func(ctx context.Context) (map[string]string, error) {
488-
return map[string]string{}, nil
497+
ncVersions: func(ctx context.Context) ([]imds.NetworkInterface, error) {
498+
return []imds.NetworkInterface{}, nil
489499
},
490500
}
491501

@@ -540,10 +550,10 @@ func TestSyncHostNCVersionNMAgentAPICallFailed(t *testing.T) {
540550

541551
// Create IMDS mock that returns empty, as nma api call failed
542552
mockIMDS := &struct {
543-
ncVersions func(ctx context.Context) (map[string]string, error)
553+
ncVersions func(ctx context.Context) ([]imds.NetworkInterface, error)
544554
}{
545-
ncVersions: func(ctx context.Context) (map[string]string, error) {
546-
return map[string]string{}, nil
555+
ncVersions: func(ctx context.Context) ([]imds.NetworkInterface, error) {
556+
return []imds.NetworkInterface{}, nil
547557
},
548558
}
549559

@@ -595,11 +605,11 @@ func TestSyncHostNCVersionSwiftV2APINotSupported(t *testing.T) {
595605

596606
// Create IMDS mock - this should not be called since SwiftV2 is not supported
597607
mockIMDS := &struct {
598-
ncVersions func(ctx context.Context) (map[string]string, error)
608+
ncVersions func(ctx context.Context) ([]imds.NetworkInterface, error)
599609
}{
600-
ncVersions: func(ctx context.Context) (map[string]string, error) {
610+
ncVersions: func(ctx context.Context) ([]imds.NetworkInterface, error) {
601611
t.Errorf("IMDS should not be called when SwiftV2 API is not supported")
602-
return map[string]string{}, nil
612+
return []imds.NetworkInterface{}, nil
603613
},
604614
}
605615

@@ -1770,14 +1780,14 @@ func TestMustEnsureNoStaleNCs_PanicsWhenIPsFromStaleNCAreAssigned(t *testing.T)
17701780
// mockIMDSAdapter adapts the anonymous struct to implement the imdsClient interface
17711781
type mockIMDSAdapter struct {
17721782
mock *struct {
1773-
ncVersions func(ctx context.Context) (map[string]string, error)
1783+
ncVersions func(ctx context.Context) ([]imds.NetworkInterface, error)
17741784
}
17751785
}
17761786

17771787
func (m *mockIMDSAdapter) GetVMUniqueID(ctx context.Context) (string, error) {
17781788
panic("GetVMUniqueID should not be called in syncHostNCVersion tests, adding mockIMDSAdapter implements the full IMDS interface")
17791789
}
17801790

1781-
func (m *mockIMDSAdapter) GetNCVersionsFromIMDS(ctx context.Context) (map[string]string, error) {
1791+
func (m *mockIMDSAdapter) GetNCVersions(ctx context.Context) ([]imds.NetworkInterface, error) {
17821792
return m.mock.ncVersions(ctx)
17831793
}

‎cns/restserver/restserver.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"github.com/Azure/azure-container-networking/cns"
1212
"github.com/Azure/azure-container-networking/cns/common"
1313
"github.com/Azure/azure-container-networking/cns/dockerclient"
14+
"github.com/Azure/azure-container-networking/cns/imds"
1415
"github.com/Azure/azure-container-networking/cns/logger"
1516
"github.com/Azure/azure-container-networking/cns/networkcontainers"
1617
"github.com/Azure/azure-container-networking/cns/nodesubnet"
@@ -52,7 +53,7 @@ type wireserverProxy interface {
5253

5354
type imdsClient interface {
5455
GetVMUniqueID(ctx context.Context) (string, error)
55-
GetNCVersionsFromIMDS(ctx context.Context) (map[string]string, error)
56+
GetNCVersions(ctx context.Context) ([]imds.NetworkInterface, error)
5657
}
5758

5859
type iptablesClient interface {

0 commit comments

Comments
 (0)