Skip to content

Commit a166a58

Browse files
NihaNallappagariNihaNallappagari
committed
code review comment
1 parent 14aa47b commit a166a58

File tree

7 files changed

+26
-43
lines changed

7 files changed

+26
-43
lines changed

‎cns/kubecontroller/nodenetworkconfig/conversion.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"strings"
88

99
"github.com/Azure/azure-container-networking/cns"
10-
"github.com/Azure/azure-container-networking/cns/configuration"
1110
"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
1211
"github.com/pkg/errors"
1312
)
@@ -74,7 +73,7 @@ func CreateNCRequestFromDynamicNC(nc v1alpha.NetworkContainer) (*cns.CreateNetwo
7473
// CreateNCRequestFromStaticNC generates a CreateNetworkContainerRequest from a static NetworkContainer.
7574
//
7675
//nolint:gocritic //ignore hugeparam
77-
func CreateNCRequestFromStaticNC(nc v1alpha.NetworkContainer, config *configuration.CNSConfig) (*cns.CreateNetworkContainerRequest, error) {
76+
func CreateNCRequestFromStaticNC(nc v1alpha.NetworkContainer, isSwiftV2 bool) (*cns.CreateNetworkContainerRequest, error) {
7877
if nc.Type == v1alpha.Overlay {
7978
nc.Version = 0 // fix for NMA always giving us version 0 for Overlay NCs
8079
}
@@ -98,7 +97,7 @@ func CreateNCRequestFromStaticNC(nc v1alpha.NetworkContainer, config *configurat
9897
subnet.IPAddress = primaryPrefix.Addr().String()
9998
}
10099

101-
req, err := createNCRequestFromStaticNCHelper(nc, primaryPrefix, subnet, config)
100+
req, err := createNCRequestFromStaticNCHelper(nc, primaryPrefix, subnet, isSwiftV2)
102101
if err != nil {
103102
return nil, errors.Wrapf(err, "error while creating NC request from static NC")
104103
}

‎cns/kubecontroller/nodenetworkconfig/conversion_linux.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"strconv"
66

77
"github.com/Azure/azure-container-networking/cns"
8-
"github.com/Azure/azure-container-networking/cns/configuration"
98
"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
109
"github.com/pkg/errors"
1110
)
@@ -14,13 +13,13 @@ import (
1413
// by adding all IPs in the the block to the secondary IP configs list. It does not skip any IPs.
1514
//
1615
//nolint:gocritic //ignore hugeparam
17-
func createNCRequestFromStaticNCHelper(nc v1alpha.NetworkContainer, primaryIPPrefix netip.Prefix, subnet cns.IPSubnet, config *configuration.CNSConfig) (*cns.CreateNetworkContainerRequest, error) {
16+
func createNCRequestFromStaticNCHelper(nc v1alpha.NetworkContainer, primaryIPPrefix netip.Prefix, subnet cns.IPSubnet, isSwiftV2 bool) (*cns.CreateNetworkContainerRequest, error) {
1817
secondaryIPConfigs := map[string]cns.SecondaryIPConfig{}
1918

2019
// iterate through all IP addresses in the subnet described by primaryPrefix and
2120
// add them to the request as secondary IPConfigs.
2221
// Process primary prefix IPs in all scenarios except when nc.Type is v1alpha.VNETBlock AND SwiftV2 is enabled
23-
if !(config.EnableSwiftV2 && nc.Type == v1alpha.VNETBlock) {
22+
if !(isSwiftV2 && nc.Type == v1alpha.VNETBlock) {
2423
for addr := primaryIPPrefix.Masked().Addr(); primaryIPPrefix.Contains(addr); addr = addr.Next() {
2524
secondaryIPConfigs[addr.String()] = cns.SecondaryIPConfig{
2625
IPAddress: addr.String(),

‎cns/kubecontroller/nodenetworkconfig/conversion_test.go‎

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"testing"
66

77
"github.com/Azure/azure-container-networking/cns"
8-
"github.com/Azure/azure-container-networking/cns/configuration"
98
"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
109
"github.com/stretchr/testify/assert"
1110
"github.com/stretchr/testify/require"
@@ -341,8 +340,7 @@ func TestCreateNCRequestFromStaticNC(t *testing.T) {
341340
for _, tt := range tests {
342341
tt := tt
343342
t.Run(tt.name, func(t *testing.T) {
344-
config := &configuration.CNSConfig{}
345-
got, err := CreateNCRequestFromStaticNC(tt.input, config)
343+
got, err := CreateNCRequestFromStaticNC(tt.input, false)
346344
if tt.wantErr {
347345
assert.Error(t, err)
348346
return
@@ -355,11 +353,11 @@ func TestCreateNCRequestFromStaticNC(t *testing.T) {
355353

356354
func TestCreateNCRequestFromStaticNCWithConfig(t *testing.T) {
357355
tests := []struct {
358-
name string
359-
input v1alpha.NetworkContainer
360-
config *configuration.CNSConfig
361-
want *cns.CreateNetworkContainerRequest
362-
wantErr bool
356+
name string
357+
input v1alpha.NetworkContainer
358+
isSwiftV2 bool
359+
want *cns.CreateNetworkContainerRequest
360+
wantErr bool
363361
}{
364362
{
365363
name: "SwiftV2 enabled with VNETBlock - should NOT process all IPs in prefix",
@@ -373,9 +371,7 @@ func TestCreateNCRequestFromStaticNCWithConfig(t *testing.T) {
373371
Version: 1,
374372
Status: "Available",
375373
},
376-
config: &configuration.CNSConfig{
377-
EnableSwiftV2: true,
378-
},
374+
isSwiftV2: true,
379375
want: &cns.CreateNetworkContainerRequest{
380376
NetworkContainerid: ncID,
381377
NetworkContainerType: cns.Docker,
@@ -413,7 +409,7 @@ func TestCreateNCRequestFromStaticNCWithConfig(t *testing.T) {
413409
},
414410
},
415411
},
416-
config: &configuration.CNSConfig{},
412+
isSwiftV2: false,
417413
want: &cns.CreateNetworkContainerRequest{
418414
NetworkContainerid: ncID,
419415
NetworkContainerType: cns.Docker,
@@ -453,7 +449,7 @@ func TestCreateNCRequestFromStaticNCWithConfig(t *testing.T) {
453449
},
454450
},
455451
},
456-
config: &configuration.CNSConfig{},
452+
isSwiftV2: false,
457453
want: &cns.CreateNetworkContainerRequest{
458454
NetworkContainerid: ncID,
459455
NetworkContainerType: cns.Docker,
@@ -477,7 +473,7 @@ func TestCreateNCRequestFromStaticNCWithConfig(t *testing.T) {
477473

478474
for _, tt := range tests {
479475
t.Run(tt.name, func(t *testing.T) {
480-
got, err := CreateNCRequestFromStaticNC(tt.input, tt.config)
476+
got, err := CreateNCRequestFromStaticNC(tt.input, tt.isSwiftV2)
481477
if tt.wantErr {
482478
assert.Error(t, err)
483479
return

‎cns/kubecontroller/nodenetworkconfig/conversion_windows.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"strconv"
66

77
"github.com/Azure/azure-container-networking/cns"
8-
"github.com/Azure/azure-container-networking/cns/configuration"
98
"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
109
"github.com/pkg/errors"
1110
)
@@ -15,7 +14,7 @@ import (
1514
// secondary IPs. If the gateway is not empty, it will not reserve the 2nd IP and add it as a secondary IP.
1615
//
1716
//nolint:gocritic //ignore hugeparam
18-
func createNCRequestFromStaticNCHelper(nc v1alpha.NetworkContainer, primaryIPPrefix netip.Prefix, subnet cns.IPSubnet, config *configuration.CNSConfig) (*cns.CreateNetworkContainerRequest, error) {
17+
func createNCRequestFromStaticNCHelper(nc v1alpha.NetworkContainer, primaryIPPrefix netip.Prefix, subnet cns.IPSubnet, isSwiftV2 bool) (*cns.CreateNetworkContainerRequest, error) {
1918
secondaryIPConfigs := map[string]cns.SecondaryIPConfig{}
2019
// the masked address is the 0th IP in the subnet and startingAddr is the 2nd IP (*.1)
2120
startingAddr := primaryIPPrefix.Masked().Addr().Next()
@@ -29,7 +28,7 @@ func createNCRequestFromStaticNCHelper(nc v1alpha.NetworkContainer, primaryIPPre
2928
// iterate through all IP addresses in the subnet described by primaryPrefix and
3029
// add them to the request as secondary IPConfigs.
3130
// Process primary prefix IPs in all scenarios except when nc.Type is v1alpha.VNETBlock AND SwiftV2 is enabled
32-
if !(config.EnableSwiftV2 && nc.Type == v1alpha.VNETBlock) {
31+
if !(isSwiftV2 && nc.Type == v1alpha.VNETBlock) {
3332
for addr := startingAddr; primaryIPPrefix.Contains(addr); addr = addr.Next() {
3433
secondaryIPConfigs[addr.String()] = cns.SecondaryIPConfig{
3534
IPAddress: addr.String(),

‎cns/kubecontroller/nodenetworkconfig/reconciler.go‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"sync"
66

77
"github.com/Azure/azure-container-networking/cns"
8-
"github.com/Azure/azure-container-networking/cns/configuration"
98
"github.com/Azure/azure-container-networking/cns/logger"
109
"github.com/Azure/azure-container-networking/cns/restserver"
1110
cnstypes "github.com/Azure/azure-container-networking/cns/types"
@@ -44,20 +43,20 @@ type Reconciler struct {
4443
once sync.Once
4544
started chan interface{}
4645
nodeIP string
47-
config *configuration.CNSConfig
46+
isSwiftV2 bool
4847
}
4948

5049
// NewReconciler creates a NodeNetworkConfig Reconciler which will get updates from the Kubernetes
5150
// apiserver for NNC events.
5251
// Provided nncListeners are passed the NNC after the Reconcile preprocesses it. Note: order matters! The
5352
// passed Listeners are notified in the order provided.
54-
func NewReconciler(cnscli cnsClient, ipampoolmonitorcli nodeNetworkConfigListener, nodeIP string, config *configuration.CNSConfig) *Reconciler {
53+
func NewReconciler(cnscli cnsClient, ipampoolmonitorcli nodeNetworkConfigListener, nodeIP string, isSwiftV2 bool) *Reconciler {
5554
return &Reconciler{
5655
cnscli: cnscli,
5756
ipampoolmonitorcli: ipampoolmonitorcli,
5857
started: make(chan interface{}),
5958
nodeIP: nodeIP,
60-
config: config,
59+
isSwiftV2: isSwiftV2,
6160
}
6261
}
6362

@@ -107,7 +106,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
107106
switch nnc.Status.NetworkContainers[i].AssignmentMode { //nolint:exhaustive // skipping dynamic case
108107
// For Overlay and Vnet Scale Scenarios
109108
case v1alpha.Static:
110-
req, err = CreateNCRequestFromStaticNC(nnc.Status.NetworkContainers[i], r.config)
109+
req, err = CreateNCRequestFromStaticNC(nnc.Status.NetworkContainers[i], r.isSwiftV2)
111110
// For Pod Subnet scenario
112111
default: // For backward compatibility, default will be treated as Dynamic too.
113112
req, err = CreateNCRequestFromDynamicNC(nnc.Status.NetworkContainers[i])

‎cns/kubecontroller/nodenetworkconfig/reconciler_test.go‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"testing"
66

77
"github.com/Azure/azure-container-networking/cns"
8-
"github.com/Azure/azure-container-networking/cns/configuration"
98
"github.com/Azure/azure-container-networking/cns/logger"
109
cnstypes "github.com/Azure/azure-container-networking/cns/types"
1110
"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
@@ -193,8 +192,7 @@ func TestReconcile(t *testing.T) {
193192
}
194193

195194
t.Run(tt.name, func(t *testing.T) {
196-
config := &configuration.CNSConfig{} // Use default config for tests
197-
r := NewReconciler(&tt.cnsClient, &tt.cnsClient, tt.nodeIP, config)
195+
r := NewReconciler(&tt.cnsClient, &tt.cnsClient, tt.nodeIP, false)
198196
r.nnccli = &tt.ncGetter
199197
got, err := r.Reconcile(context.Background(), tt.in)
200198
if tt.wantErr {
@@ -251,8 +249,7 @@ func TestReconcileStaleNCs(t *testing.T) {
251249
return &nncLog[len(nncLog)-1], nil
252250
}
253251

254-
config := &configuration.CNSConfig{} // Use default config for tests
255-
r := NewReconciler(&cnsClient, &cnsClient, nodeIP, config)
252+
r := NewReconciler(&cnsClient, &cnsClient, nodeIP, false)
256253
r.nnccli = &mockNCGetter{get: nncIterator}
257254

258255
_, err := r.Reconcile(context.Background(), reconcile.Request{})

‎cns/service/main.go‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1311,13 +1311,7 @@ type ipamStateReconciler interface {
13111311

13121312
// TODO(rbtr) where should this live??
13131313
// reconcileInitialCNSState initializes cns by passing pods and a CreateNetworkContainerRequest
1314-
func reconcileInitialCNSState(
1315-
ctx context.Context,
1316-
cli nodeNetworkConfigGetter,
1317-
ipamReconciler ipamStateReconciler,
1318-
podInfoByIPProvider cns.PodInfoByIPProvider,
1319-
config *configuration.CNSConfig,
1320-
) error {
1314+
func reconcileInitialCNSState(ctx context.Context, cli nodeNetworkConfigGetter, ipamReconciler ipamStateReconciler, podInfoByIPProvider cns.PodInfoByIPProvider, isSwiftV2 bool) error {
13211315
// Get nnc using direct client
13221316
nnc, err := cli.Get(ctx)
13231317
if err != nil {
@@ -1356,7 +1350,7 @@ func reconcileInitialCNSState(
13561350
)
13571351
switch nnc.Status.NetworkContainers[i].AssignmentMode { //nolint:exhaustive // skipping dynamic case
13581352
case v1alpha.Static:
1359-
ncRequest, err = nncctrl.CreateNCRequestFromStaticNC(nnc.Status.NetworkContainers[i], config)
1353+
ncRequest, err = nncctrl.CreateNCRequestFromStaticNC(nnc.Status.NetworkContainers[i], isSwiftV2)
13601354
default: // For backward compatibility, default will be treated as Dynamic too.
13611355
ncRequest, err = nncctrl.CreateNCRequestFromDynamicNC(nnc.Status.NetworkContainers[i])
13621356
}
@@ -1464,7 +1458,7 @@ func InitializeCRDState(ctx context.Context, z *zap.Logger, httpRestService cns.
14641458
_ = retry.Do(func() error {
14651459
attempt++
14661460
logger.Printf("reconciling initial CNS state attempt: %d", attempt)
1467-
err = reconcileInitialCNSState(ctx, directscopedcli, httpRestServiceImplementation, podInfoByIPProvider, cnsconfig)
1461+
err = reconcileInitialCNSState(ctx, directscopedcli, httpRestServiceImplementation, podInfoByIPProvider, cnsconfig.EnableSwiftV2)
14681462
if err != nil {
14691463
logger.Errorf("failed to reconcile initial CNS state, attempt: %d err: %v", attempt, err)
14701464
nncInitFailure.Inc()
@@ -1561,7 +1555,7 @@ func InitializeCRDState(ctx context.Context, z *zap.Logger, httpRestService cns.
15611555

15621556
// get CNS Node IP to compare NC Node IP with this Node IP to ensure NCs were created for this node
15631557
nodeIP := configuration.NodeIP()
1564-
nncReconciler := nncctrl.NewReconciler(httpRestServiceImplementation, poolMonitor, nodeIP, cnsconfig)
1558+
nncReconciler := nncctrl.NewReconciler(httpRestServiceImplementation, poolMonitor, nodeIP, cnsconfig.EnableSwiftV2)
15651559
// pass Node to the Reconciler for Controller xref
15661560
// IPAMv1 - reconcile only status changes (where generation doesn't change).
15671561
// IPAMv2 - reconcile all updates.

0 commit comments

Comments
 (0)