Skip to content

Commit 2d920ca

Browse files
NihaNallappagariNihaNallappagari
committed
address code review comments
1 parent 7f3bf07 commit 2d920ca

File tree

6 files changed

+68
-60
lines changed

6 files changed

+68
-60
lines changed

‎cns/fakes/imdsclientfake.go‎

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

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

66-
// Return some mock NC versions for testing
66+
// Return some mock network interfaces for testing
6767
return []imds.NetworkInterface{
6868
{
6969
InterfaceCompartmentID: "nc1",

‎cns/imds/client.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ func (c *Client) GetVMUniqueID(ctx context.Context) (string, error) {
103103
return vmUniqueID, nil
104104
}
105105

106-
func (c *Client) GetNCVersions(ctx context.Context) ([]NetworkInterface, error) {
106+
func (c *Client) GetNetworkInterfaces(ctx context.Context) ([]NetworkInterface, error) {
107107
var networkData NetworkMetadata
108108
err := retry.Do(func() error {
109109
networkMetadata, err := c.getInstanceMetadata(ctx, imdsNetworkPath)
@@ -130,14 +130,14 @@ func (c *Client) GetNCVersions(ctx context.Context) ([]NetworkInterface, error)
130130
return networkData.Interface, nil
131131
}
132132

133-
func (c *Client) getInstanceMetadata(ctx context.Context, imdsComputePath string) (map[string]any, error) {
134-
imdsComputeURL, err := url.JoinPath(c.config.endpoint, imdsComputePath)
133+
func (c *Client) getInstanceMetadata(ctx context.Context, imdsMetadataPath string) (map[string]any, error) {
134+
imdsRequestURL, err := url.JoinPath(c.config.endpoint, imdsMetadataPath)
135135
if err != nil {
136-
return nil, errors.Wrap(err, "unable to build path to IMDS metadata for path"+imdsComputePath)
136+
return nil, errors.Wrap(err, "unable to build path to IMDS metadata for path"+imdsMetadataPath)
137137
}
138-
imdsComputeURL = imdsComputeURL + "?" + imdsComputeAPIVersion + "&" + imdsFormatJSON
138+
imdsRequestURL = imdsRequestURL + "?" + imdsComputeAPIVersion + "&" + imdsFormatJSON
139139

140-
req, err := http.NewRequestWithContext(ctx, http.MethodGet, imdsComputeURL, http.NoBody)
140+
req, err := http.NewRequestWithContext(ctx, http.MethodGet, imdsRequestURL, http.NoBody)
141141
if err != nil {
142142
return nil, errors.Wrap(err, "error building IMDS http request")
143143
}
@@ -164,7 +164,7 @@ func (c *Client) getInstanceMetadata(ctx context.Context, imdsComputePath string
164164

165165
// NetworkInterface represents a network interface from IMDS
166166
type NetworkInterface struct {
167-
// IMDS only returns compartment fields - these are mapped to NC ID and NC version concepts
167+
// IMDS returns compartment fields - these are mapped to NC ID and NC version
168168
InterfaceCompartmentID string `json:"interfaceCompartmentID,omitempty"`
169169
InterfaceCompartmentVersion string `json:"interfaceCompartmentVersion,omitempty"`
170170
}

‎cns/imds/client_test.go‎

Lines changed: 8 additions & 8 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 TestGetNCVersions(t *testing.T) {
104+
func TestGetNetworkInterfaces(t *testing.T) {
105105
networkMetadata := []byte(`{
106106
"interface": [
107107
{
@@ -139,7 +139,7 @@ func TestGetNCVersions(t *testing.T) {
139139
defer mockIMDSServer.Close()
140140

141141
imdsClient := imds.NewClient(imds.Endpoint(mockIMDSServer.URL))
142-
interfaces, err := imdsClient.GetNCVersions(context.Background())
142+
interfaces, err := imdsClient.GetNetworkInterfaces(context.Background())
143143
require.NoError(t, err, "error querying testserver")
144144

145145
// Verify we got the expected interfaces
@@ -154,13 +154,13 @@ func TestGetNCVersions(t *testing.T) {
154154
assert.Equal(t, "1", interfaces[1].InterfaceCompartmentVersion)
155155
}
156156

157-
func TestGetNCVersionsInvalidEndpoint(t *testing.T) {
157+
func TestGetNetworkInterfacesInvalidEndpoint(t *testing.T) {
158158
imdsClient := imds.NewClient(imds.Endpoint(string([]byte{0x7f})), imds.RetryAttempts(1))
159-
_, err := imdsClient.GetNCVersions(context.Background())
159+
_, err := imdsClient.GetNetworkInterfaces(context.Background())
160160
require.Error(t, err, "expected invalid path")
161161
}
162162

163-
func TestGetNCVersionsInvalidJSON(t *testing.T) {
163+
func TestGetNetworkInterfacesInvalidJSON(t *testing.T) {
164164
mockIMDSServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
165165
w.WriteHeader(http.StatusOK)
166166
_, err := w.Write([]byte("not json"))
@@ -172,11 +172,11 @@ func TestGetNCVersionsInvalidJSON(t *testing.T) {
172172
defer mockIMDSServer.Close()
173173

174174
imdsClient := imds.NewClient(imds.Endpoint(mockIMDSServer.URL), imds.RetryAttempts(1))
175-
_, err := imdsClient.GetNCVersions(context.Background())
175+
_, err := imdsClient.GetNetworkInterfaces(context.Background())
176176
require.Error(t, err, "expected json decoding error")
177177
}
178178

179-
func TestGetNCVersionsNoNCIDs(t *testing.T) {
179+
func TestGetNetworkInterfacesNoNCIDs(t *testing.T) {
180180
networkMetadataNoNC := []byte(`{
181181
"interface": [
182182
{
@@ -206,7 +206,7 @@ func TestGetNCVersionsNoNCIDs(t *testing.T) {
206206
defer mockIMDSServer.Close()
207207

208208
imdsClient := imds.NewClient(imds.Endpoint(mockIMDSServer.URL))
209-
interfaces, err := imdsClient.GetNCVersions(context.Background())
209+
interfaces, err := imdsClient.GetNetworkInterfaces(context.Background())
210210
require.NoError(t, err, "error querying testserver")
211211

212212
// Verify we got interfaces but they don't have compartment IDs

‎cns/restserver/internalapi.go‎

Lines changed: 35 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -238,24 +238,24 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo
238238
nmaNCs[strings.ToLower(nc.NetworkContainerID)] = nc.Version
239239
}
240240

241-
// Consolidate both maps - NMA takes precedence, IMDS as fallback
242-
consolidatedNCs := make(map[string]string)
241+
// Consolidate both nc's from NMA and IMDS calls
242+
nmaProgrammedNCs := make(map[string]string)
243243
for ncID, version := range nmaNCs {
244-
consolidatedNCs[ncID] = version
244+
nmaProgrammedNCs[ncID] = version
245245
}
246246
for ncID, version := range imdsNCVersions {
247-
if _, exists := consolidatedNCs[ncID]; !exists {
248-
consolidatedNCs[strings.ToLower(ncID)] = version
247+
if _, exists := nmaProgrammedNCs[ncID]; !exists {
248+
nmaProgrammedNCs[strings.ToLower(ncID)] = version
249249
}
250250
}
251-
hasNC.Set(float64(len(consolidatedNCs)))
251+
hasNC.Set(float64(len(nmaProgrammedNCs)))
252252
for ncID := range outdatedNCs {
253-
consolidatedNCVersionStr, ok := consolidatedNCs[ncID]
253+
nmaProgrammedNCVersionStr, ok := nmaProgrammedNCs[ncID]
254254
if !ok {
255255
// Neither NMA nor IMDS has this NC that we need programmed yet, bail out
256256
continue
257257
}
258-
consolidatedNCVersion, err := strconv.Atoi(consolidatedNCVersionStr)
258+
nmaProgrammedNCVersion, err := strconv.Atoi(nmaProgrammedNCVersionStr)
259259
if err != nil {
260260
logger.Errorf("failed to parse container version of %s: %s", ncID, err)
261261
continue
@@ -268,7 +268,7 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo
268268
return len(programmedNCs), errors.Wrapf(errNonExistentContainerStatus, "can't find NC with ID %s in service state, stop updating this host NC version", ncID)
269269
}
270270
// if the NC still exists in state and is programmed to some version (doesn't have to be latest), add it to our set of NCs that have been programmed
271-
if consolidatedNCVersion > -1 {
271+
if nmaProgrammedNCVersion > -1 {
272272
programmedNCs[ncID] = struct{}{}
273273
}
274274

@@ -277,17 +277,17 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo
277277
logger.Errorf("failed to parse host nc version string %s: %s", ncInfo.HostVersion, err)
278278
continue
279279
}
280-
if localNCVersion > consolidatedNCVersion {
280+
if localNCVersion > nmaProgrammedNCVersion {
281281
//nolint:staticcheck // SA1019: suppress deprecated logger.Printf usage. Todo: legacy logger usage is consistent in cns repo. Migrates when all logger usage is migrated
282-
logger.Errorf("NC version from consolidated sources is decreasing: have %d, got %d", localNCVersion, consolidatedNCVersion)
282+
logger.Errorf("NC version from consolidated sources is decreasing: have %d, got %d", localNCVersion, nmaProgrammedNCVersion)
283283
continue
284284
}
285285
if channelMode == cns.CRD {
286-
service.MarkIpsAsAvailableUntransacted(ncInfo.ID, consolidatedNCVersion)
286+
service.MarkIpsAsAvailableUntransacted(ncInfo.ID, nmaProgrammedNCVersion)
287287
}
288288
//nolint:staticcheck // SA1019: suppress deprecated logger.Printf usage. Todo: legacy logger usage is consistent in cns repo. Migrates when all logger usage is migrated
289-
logger.Printf("Updating NC %s host version from %s to %s", ncID, ncInfo.HostVersion, consolidatedNCVersionStr)
290-
ncInfo.HostVersion = consolidatedNCVersionStr
289+
logger.Printf("Updating NC %s host version from %s to %s", ncID, ncInfo.HostVersion, nmaProgrammedNCVersionStr)
290+
ncInfo.HostVersion = nmaProgrammedNCVersionStr
291291
logger.Printf("Updated NC %s host version to %s", ncID, ncInfo.HostVersion)
292292
service.state.ContainerStatus[ncID] = ncInfo
293293
// if we successfully updated the NC, pop it from the needs update set.
@@ -660,44 +660,52 @@ func (service *HTTPRestService) SetVFForAccelnetNICs() error {
660660
return service.setVFForAccelnetNICs()
661661
}
662662

663-
// checkNMAgentAPISupport checks if specific APIs are supported by NMAgent using the existing client
664-
func (service *HTTPRestService) checkNMAgentAPISupport(ctx context.Context) (swiftV2Support bool, err error) {
665-
// Use the existing NMAgent client instead of direct HTTP calls
663+
func (service *HTTPRestService) getSupportedAPIsFromNMAgent(ctx context.Context) ([]string, error) {
666664
if service.nma == nil {
667-
return false, errors.New("NMAgent client is not available")
665+
return nil, errors.New("NMAgent client is not available")
668666
}
669667

670668
apis, err := service.nma.SupportedAPIs(ctx)
671669
if err != nil {
672-
return false, errors.New("failed to get supported APIs from NMAgent client")
670+
return nil, errors.Wrap(err, "failed to get supported APIs from NMAgent client")
671+
}
672+
673+
return apis, nil
674+
}
675+
676+
func (service *HTTPRestService) isSwiftV2NCSupported(ctx context.Context) bool {
677+
apis, err := service.getSupportedAPIsFromNMAgent(ctx)
678+
if err != nil {
679+
//nolint:staticcheck // SA1019: suppress deprecated logger.Printf usage. Todo: legacy logger usage is consistent in cns repo. Migrates when all logger usage is migrated
680+
logger.Errorf("Failed to get supported APIs from NMAgent: %v", err)
681+
return false
673682
}
674683

675684
for _, api := range apis {
676685
if strings.Contains(api, nmAgentSwiftV2API) {
677-
swiftV2Support = true
686+
return true
678687
}
679688
}
680689

681-
return swiftV2Support, nil
690+
return false
682691
}
683692

684693
// GetIMDSNCVersions gets NC versions from IMDS and returns them as a map
685694
func (service *HTTPRestService) GetIMDSNCVersions(ctx context.Context) (map[string]string, error) {
686695
// Check NMAgent API support for SwiftV2, if it fails return empty map assuming support might not be available in that nma build
687-
swiftV2Support, err := service.checkNMAgentAPISupport(ctx)
688-
if err != nil || !swiftV2Support {
696+
if !service.isSwiftV2NCSupported(ctx) {
689697
//nolint:staticcheck // SA1019: suppress deprecated logger.Printf usage. Todo: legacy logger usage is consistent in cns repo. Migrates when all logger usage is migrated
690-
logger.Errorf("NMAgent does not support SwiftV2 API or encountered an error: %v", err)
698+
logger.Errorf("NMAgent does not support SwiftV2 API")
691699
return make(map[string]string), nil
692700
}
693701

694702
imdsClient := service.imdsClient
695703

696-
// Get all NC versions from IMDS
697-
networkInterfaces, err := imdsClient.GetNCVersions(ctx)
704+
// Get all network interfaces from IMDS
705+
networkInterfaces, err := imdsClient.GetNetworkInterfaces(ctx)
698706
if err != nil {
699707
//nolint:staticcheck // SA1019: suppress deprecated logger.Printf usage. Todo: legacy logger usage is consistent in cns repo. Migrates when all logger usage is migrated
700-
logger.Errorf("Failed to get NC versions from IMDS: %v", err)
708+
logger.Errorf("Failed to get network interfaces from IMDS: %v", err)
701709
return make(map[string]string), nil
702710
}
703711

‎cns/restserver/internalapi_test.go‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -272,9 +272,9 @@ func TestSyncHostNCVersion(t *testing.T) {
272272

273273
// Create a custom IMDS mock that returns the second NC
274274
mockIMDS := &struct {
275-
ncVersions func(_ context.Context) ([]imds.NetworkInterface, error)
275+
networkInterfaces func(_ context.Context) ([]imds.NetworkInterface, error)
276276
}{
277-
ncVersions: func(_ context.Context) ([]imds.NetworkInterface, error) {
277+
networkInterfaces: func(_ context.Context) ([]imds.NetworkInterface, error) {
278278
return []imds.NetworkInterface{
279279
{
280280
InterfaceCompartmentID: imdsNCID,
@@ -372,9 +372,9 @@ func TestSyncHostNCVersionErrorMissingNC(t *testing.T) {
372372

373373
// Create IMDS mock that returns 1 NC but with different ID (not matching the outdated NC)
374374
mockIMDS := &struct {
375-
ncVersions func(_ context.Context) ([]imds.NetworkInterface, error)
375+
networkInterfaces func(_ context.Context) ([]imds.NetworkInterface, error)
376376
}{
377-
ncVersions: func(_ context.Context) ([]imds.NetworkInterface, error) {
377+
networkInterfaces: func(_ context.Context) ([]imds.NetworkInterface, error) {
378378
return []imds.NetworkInterface{
379379
{
380380
InterfaceCompartmentID: "different-nc-id",
@@ -434,9 +434,9 @@ func TestSyncHostNCVersionLocalVersionHigher(t *testing.T) {
434434

435435
// Create IMDS mock that returns lower version(2) than local host version(3)
436436
mockIMDS := &struct {
437-
ncVersions func(_ context.Context) ([]imds.NetworkInterface, error)
437+
networkInterfaces func(_ context.Context) ([]imds.NetworkInterface, error)
438438
}{
439-
ncVersions: func(_ context.Context) ([]imds.NetworkInterface, error) {
439+
networkInterfaces: func(_ context.Context) ([]imds.NetworkInterface, error) {
440440
return []imds.NetworkInterface{
441441
{
442442
InterfaceCompartmentID: req.NetworkContainerid,
@@ -492,9 +492,9 @@ func TestSyncHostNCVersionLocalHigherThanDNC(t *testing.T) {
492492
svc.Unlock()
493493

494494
mockIMDS := &struct {
495-
ncVersions func(_ context.Context) ([]imds.NetworkInterface, error)
495+
networkInterfaces func(_ context.Context) ([]imds.NetworkInterface, error)
496496
}{
497-
ncVersions: func(_ context.Context) ([]imds.NetworkInterface, error) {
497+
networkInterfaces: func(_ context.Context) ([]imds.NetworkInterface, error) {
498498
return []imds.NetworkInterface{}, nil
499499
},
500500
}
@@ -550,9 +550,9 @@ func TestSyncHostNCVersionNMAgentAPICallFailed(t *testing.T) {
550550

551551
// Create IMDS mock that returns empty, as nma api call failed
552552
mockIMDS := &struct {
553-
ncVersions func(_ context.Context) ([]imds.NetworkInterface, error)
553+
networkInterfaces func(_ context.Context) ([]imds.NetworkInterface, error)
554554
}{
555-
ncVersions: func(_ context.Context) ([]imds.NetworkInterface, error) {
555+
networkInterfaces: func(_ context.Context) ([]imds.NetworkInterface, error) {
556556
return []imds.NetworkInterface{}, nil
557557
},
558558
}
@@ -604,9 +604,9 @@ func TestSyncHostNCVersionSwiftV2APINotSupported(t *testing.T) {
604604

605605
// Create IMDS mock - this should not be called since SwiftV2 is not supported
606606
mockIMDS := &struct {
607-
ncVersions func(_ context.Context) ([]imds.NetworkInterface, error)
607+
networkInterfaces func(_ context.Context) ([]imds.NetworkInterface, error)
608608
}{
609-
ncVersions: func(_ context.Context) ([]imds.NetworkInterface, error) {
609+
networkInterfaces: func(_ context.Context) ([]imds.NetworkInterface, error) {
610610
t.Errorf("IMDS should not be called when SwiftV2 API is not supported")
611611
return []imds.NetworkInterface{}, nil
612612
},
@@ -1779,14 +1779,14 @@ func TestMustEnsureNoStaleNCs_PanicsWhenIPsFromStaleNCAreAssigned(t *testing.T)
17791779
// mockIMDSAdapter adapts the anonymous struct to implement the imdsClient interface
17801780
type mockIMDSAdapter struct {
17811781
mock *struct {
1782-
ncVersions func(_ context.Context) ([]imds.NetworkInterface, error)
1782+
networkInterfaces func(_ context.Context) ([]imds.NetworkInterface, error)
17831783
}
17841784
}
17851785

17861786
func (m *mockIMDSAdapter) GetVMUniqueID(_ context.Context) (string, error) {
17871787
panic("GetVMUniqueID should not be called in syncHostNCVersion tests, adding mockIMDSAdapter implements the full IMDS interface")
17881788
}
17891789

1790-
func (m *mockIMDSAdapter) GetNCVersions(ctx context.Context) ([]imds.NetworkInterface, error) {
1791-
return m.mock.ncVersions(ctx)
1790+
func (m *mockIMDSAdapter) GetNetworkInterfaces(ctx context.Context) ([]imds.NetworkInterface, error) {
1791+
return m.mock.networkInterfaces(ctx)
17921792
}

‎cns/restserver/restserver.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ type wireserverProxy interface {
5353

5454
type imdsClient interface {
5555
GetVMUniqueID(ctx context.Context) (string, error)
56-
GetNCVersions(ctx context.Context) ([]imds.NetworkInterface, error)
56+
GetNetworkInterfaces(ctx context.Context) ([]imds.NetworkInterface, error)
5757
}
5858

5959
type iptablesClient interface {

0 commit comments

Comments
 (0)