Create NNC scoped client and isolate ipampoolmonitor and NNC reconciler (#1006)

* create nodenetworkconfig client

Signed-off-by: Evan Baker <[email protected]>

* add envnodename to config

Signed-off-by: Evan Baker <[email protected]>

* rewrite and fully test nnc to nc conversion

Signed-off-by: Evan Baker <[email protected]>

* use nnc clients to separate reconciler and ipampoolmonitor

Signed-off-by: Evan Baker <[email protected]>

* inline reconciler event filters

Signed-off-by: Evan Baker <[email protected]>

* delint

Signed-off-by: Evan Baker <[email protected]>

* test env config

Signed-off-by: Evan Baker <[email protected]>

* wrap errs

Signed-off-by: Evan Baker <[email protected]>

* test reconciler

Signed-off-by: Evan Baker <[email protected]>

* address review comments

Signed-off-by: Evan Baker <[email protected]>

* rebase cleanup

Signed-off-by: Evan Baker <[email protected]>

* fix test

Signed-off-by: Evan Baker <[email protected]>

* address review feedback

Signed-off-by: Evan Baker <[email protected]>

Author: rbtr

cns/configuration/env.go

@@ -0,0 +1,24 @@
+package configuration
+
+import (
+	"os"
+
+	"github.com/pkg/errors"
+)
+
+const (
+	// EnvNodeName is the NODENAME env var string key.
+	EnvNodeName = "NODENAME"
+)
+
+// ErrNodeNameUnset indicates the the $EnvNodeName variable is unset in the environment.
+var ErrNodeNameUnset = errors.Errorf("must declare %s environment variable", EnvNodeName)
+
+// NodeName checks the environment variables for the NODENAME and returns it or an error if unset.
+func NodeName() (string, error) {
+	nodeName := os.Getenv(EnvNodeName)
+	if nodeName == "" {
+		return "", ErrNodeNameUnset
+	}
+	return nodeName, nil
+}

cns/configuration/env_test.go

@@ -0,0 +1,19 @@
+package configuration
+
+import (
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestNodeName(t *testing.T) {
+	_, err := NodeName()
+	require.Error(t, err)
+	require.ErrorIs(t, err, ErrNodeNameUnset)
+	os.Setenv(EnvNodeName, "test")
+	name, err := NodeName()
+	assert.NoError(t, err)
+	assert.Equal(t, "test", name)
+}

cns/fakes/requestcontrollerfake.go

@@ -8,23 +8,20 @@ import (
 	"net"
 
 	"github.com/Azure/azure-container-networking/cns"
-	"github.com/Azure/azure-container-networking/cns/singletenantcontroller"
 	"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
 	"github.com/google/uuid"
 )
 
-var _ singletenantcontroller.RequestController = (*RequestControllerFake)(nil)
-
 type RequestControllerFake struct {
-	fakecns   *HTTPServiceFake
-	cachedCRD v1alpha.NodeNetworkConfig
-	ip        net.IP
+	cnscli *HTTPServiceFake
+	NNC    *v1alpha.NodeNetworkConfig
+	ip     net.IP
 }
 
 func NewRequestControllerFake(cnsService *HTTPServiceFake, scalar v1alpha.Scaler, subnetAddressSpace string, numberOfIPConfigs int) *RequestControllerFake {
 	rc := &RequestControllerFake{
-		fakecns: cnsService,
-		cachedCRD: v1alpha.NodeNetworkConfig{
+		cnscli: cnsService,
+		NNC: &v1alpha.NodeNetworkConfig{
 			Spec: v1alpha.NodeNetworkConfigSpec{},
 			Status: v1alpha.NodeNetworkConfigStatus{
 				Scaler: scalar,
@@ -40,7 +37,7 @@ func NewRequestControllerFake(cnsService *HTTPServiceFake, scalar v1alpha.Scaler
 	rc.ip, _, _ = net.ParseCIDR(subnetAddressSpace)
 
 	rc.CarveIPConfigsAndAddToStatusAndCNS(numberOfIPConfigs)
-	rc.cachedCRD.Spec.RequestedIPCount = int64(numberOfIPConfigs)
+	rc.NNC.Spec.RequestedIPCount = int64(numberOfIPConfigs)
 
 	return rc
 }
@@ -53,7 +50,7 @@ func (rc *RequestControllerFake) CarveIPConfigsAndAddToStatusAndCNS(numberOfIPCo
 			Name: uuid.New().String(),
 			IP:   rc.ip.String(),
 		}
-		rc.cachedCRD.Status.NetworkContainers[0].IPAssignments = append(rc.cachedCRD.Status.NetworkContainers[0].IPAssignments, ipconfigCRD)
+		rc.NNC.Status.NetworkContainers[0].IPAssignments = append(rc.NNC.Status.NetworkContainers[0].IPAssignments, ipconfigCRD)
 
 		ipconfigCNS := cns.IPConfigurationStatus{
 			ID:        ipconfigCRD.Name,
@@ -65,7 +62,7 @@ func (rc *RequestControllerFake) CarveIPConfigsAndAddToStatusAndCNS(numberOfIPCo
 		incrementIP(rc.ip)
 	}
 
-	rc.fakecns.IPStateManager.AddIPConfigs(cnsIPConfigs)
+	rc.cnscli.IPStateManager.AddIPConfigs(cnsIPConfigs)
 
 	return cnsIPConfigs
 }
@@ -82,17 +79,12 @@ func (rc *RequestControllerFake) IsStarted() bool {
 	return true
 }
 
-func (rc *RequestControllerFake) UpdateCRDSpec(_ context.Context, desiredSpec v1alpha.NodeNetworkConfigSpec) error {
-	rc.cachedCRD.Spec = desiredSpec
-	return nil
-}
-
 func remove(slice []v1alpha.IPAssignment, s int) []v1alpha.IPAssignment {
 	return append(slice[:s], slice[s+1:]...)
 }
 
 func (rc *RequestControllerFake) Reconcile(removePendingReleaseIPs bool) error {
-	diff := int(rc.cachedCRD.Spec.RequestedIPCount) - len(rc.fakecns.GetPodIPConfigState())
+	diff := int(rc.NNC.Spec.RequestedIPCount) - len(rc.cnscli.GetPodIPConfigState())
 
 	if diff > 0 {
 		// carve the difference of test IPs and add them to CNS, assume dnc has populated the CRD status
@@ -101,28 +93,28 @@ func (rc *RequestControllerFake) Reconcile(removePendingReleaseIPs bool) error {
 		// Assume DNC has removed the IPConfigs from the status
 
 		// mimic DNC removing IPConfigs from the CRD
-		for _, notInUseIPConfigName := range rc.cachedCRD.Spec.IPsNotInUse {
+		for _, notInUseIPConfigName := range rc.NNC.Spec.IPsNotInUse {
 
 			// remove ipconfig from status
 			index := 0
-			for _, ipconfig := range rc.cachedCRD.Status.NetworkContainers[0].IPAssignments {
+			for _, ipconfig := range rc.NNC.Status.NetworkContainers[0].IPAssignments {
 				if notInUseIPConfigName == ipconfig.Name {
 					break
 				}
 				index++
 			}
-			rc.cachedCRD.Status.NetworkContainers[0].IPAssignments = remove(rc.cachedCRD.Status.NetworkContainers[0].IPAssignments, index)
+			rc.NNC.Status.NetworkContainers[0].IPAssignments = remove(rc.NNC.Status.NetworkContainers[0].IPAssignments, index)
 
 		}
 	}
 
 	// remove ipconfig from CNS
 	if removePendingReleaseIPs {
-		rc.fakecns.IPStateManager.RemovePendingReleaseIPConfigs(rc.cachedCRD.Spec.IPsNotInUse)
+		rc.cnscli.IPStateManager.RemovePendingReleaseIPConfigs(rc.NNC.Spec.IPsNotInUse)
 	}
 
 	// update
-	rc.fakecns.PoolMonitor.Update(rc.cachedCRD.Status.Scaler, rc.cachedCRD.Spec)
+	rc.cnscli.PoolMonitor.Update(rc.NNC.Status.Scaler, rc.NNC.Spec)
 
 	return nil
 }

cns/ipampoolmonitor/ipampoolmonitor.go

@@ -9,28 +9,31 @@ import (
 	"github.com/Azure/azure-container-networking/cns"
 	"github.com/Azure/azure-container-networking/cns/logger"
 	"github.com/Azure/azure-container-networking/cns/metric"
-	"github.com/Azure/azure-container-networking/cns/singletenantcontroller"
 	"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
 )
 
 const defaultMaxIPCount = int64(250)
 
+type nodeNetworkConfigSpecUpdater interface {
+	UpdateSpec(context.Context, *v1alpha.NodeNetworkConfigSpec) (*v1alpha.NodeNetworkConfig, error)
+}
+
 type CNSIPAMPoolMonitor struct {
 	MaximumFreeIps           int64
 	MinimumFreeIps           int64
 	cachedNNC                v1alpha.NodeNetworkConfig
 	httpService              cns.HTTPService
 	mu                       sync.RWMutex
-	rc                       singletenantcontroller.RequestController
 	scalarUnits              v1alpha.Scaler
 	updatingIpsNotInUseCount int
+	nnccli                   nodeNetworkConfigSpecUpdater
 }
 
-func NewCNSIPAMPoolMonitor(httpService cns.HTTPService, rc singletenantcontroller.RequestController) *CNSIPAMPoolMonitor {
+func NewCNSIPAMPoolMonitor(httpService cns.HTTPService, nnccli nodeNetworkConfigSpecUpdater) *CNSIPAMPoolMonitor {
 	logger.Printf("NewCNSIPAMPoolMonitor: Create IPAM Pool Monitor")
 	return &CNSIPAMPoolMonitor{
 		httpService: httpService,
-		rc:          rc,
+		nnccli:      nnccli,
 	}
 }
 
@@ -135,7 +138,7 @@ func (pm *CNSIPAMPoolMonitor) increasePoolSize(ctx context.Context) error {
 
 	logger.Printf("[ipam-pool-monitor] Increasing pool size, Current Pool Size: %v, Updated Requested IP Count: %v, Pods with IP's:%v, ToBeDeleted Count: %v", len(pm.httpService.GetPodIPConfigState()), tempNNCSpec.RequestedIPCount, len(pm.httpService.GetAllocatedIPConfigs()), len(tempNNCSpec.IPsNotInUse))
 
-	if err := pm.rc.UpdateCRDSpec(ctx, tempNNCSpec); err != nil {
+	if _, err := pm.nnccli.UpdateSpec(ctx, &tempNNCSpec); err != nil {
 		// caller will retry to update the CRD again
 		return err
 	}
@@ -204,7 +207,7 @@ func (pm *CNSIPAMPoolMonitor) decreasePoolSize(ctx context.Context, existingPend
 	tempNNCSpec.RequestedIPCount -= int64(len(pendingIPAddresses))
 	logger.Printf("[ipam-pool-monitor] Decreasing pool size, Current Pool Size: %v, Requested IP Count: %v, Pods with IP's: %v, ToBeDeleted Count: %v", len(pm.httpService.GetPodIPConfigState()), tempNNCSpec.RequestedIPCount, len(pm.httpService.GetAllocatedIPConfigs()), len(tempNNCSpec.IPsNotInUse))
 
-	err := pm.rc.UpdateCRDSpec(ctx, tempNNCSpec)
+	_, err := pm.nnccli.UpdateSpec(ctx, &tempNNCSpec)
 	if err != nil {
 		// caller will retry to update the CRD again
 		return err
@@ -232,7 +235,7 @@ func (pm *CNSIPAMPoolMonitor) cleanPendingRelease(ctx context.Context) error {
 
 	tempNNCSpec := pm.createNNCSpecForCRD()
 
-	err := pm.rc.UpdateCRDSpec(ctx, tempNNCSpec)
+	_, err := pm.nnccli.UpdateSpec(ctx, &tempNNCSpec)
 	if err != nil {
 		// caller will retry to update the CRD again
 		return err

cns/ipampoolmonitor/ipampoolmonitor_test.go

@@ -10,6 +10,15 @@ import (
 	"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
 )
 
+type fakeNodeNetworkConfigUpdater struct {
+	nnc *v1alpha.NodeNetworkConfig
+}
+
+func (f *fakeNodeNetworkConfigUpdater) UpdateSpec(ctx context.Context, spec *v1alpha.NodeNetworkConfigSpec) (*v1alpha.NodeNetworkConfig, error) {
+	f.nnc.Spec = *spec
+	return f.nnc, nil
+}
+
 func initFakes(t *testing.T,
 	batchSize,
 	initialIPConfigCount,
@@ -29,7 +38,7 @@ func initFakes(t *testing.T,
 	fakecns := fakes.NewHTTPServiceFake()
 	fakerc := fakes.NewRequestControllerFake(fakecns, scalarUnits, subnetaddresspace, initialIPConfigCount)
 
-	poolmonitor := NewCNSIPAMPoolMonitor(fakecns, fakerc)
+	poolmonitor := NewCNSIPAMPoolMonitor(fakecns, &fakeNodeNetworkConfigUpdater{fakerc.NNC})
 
 	fakecns.PoolMonitor = poolmonitor
 

cns/restserver/internalapi.go

@@ -302,9 +302,7 @@ func (service *HTTPRestService) DeleteNetworkContainerInternal(
 }
 
 // This API will be called by CNS RequestController on CRD update.
-func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(
-	req *cns.CreateNetworkContainerRequest,
-) types.ResponseCode {
+func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req *cns.CreateNetworkContainerRequest) types.ResponseCode {
 	if req.NetworkContainerid == "" {
 		logger.Errorf("[Azure CNS] Error. NetworkContainerid is empty")
 		return types.NetworkContainerNotSpecified