From c6e6bb8c0ffbb04a9cba7993773bf75e1fcf42ef Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Fri, 8 Apr 2022 23:20:45 +0000 Subject: [PATCH 1/4] add overlay static nnc to nc listener Signed-off-by: Evan Baker --- cns/service/main.go | 2 +- cns/singletenantcontroller/conversion.go | 102 +++++++++++-- cns/singletenantcontroller/conversion_test.go | 140 ++++++++++-------- 3 files changed, 169 insertions(+), 75 deletions(-) diff --git a/cns/service/main.go b/cns/service/main.go index cecbd7fbad..6178ccced9 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -865,7 +865,7 @@ func reconcileInitialCNSState(ctx context.Context, cli nodeNetworkConfigGetter, } // Convert to CreateNetworkContainerRequest - ncRequest, err := kubecontroller.CRDStatusToNCRequest(&nnc.Status) + ncRequest, err := kubecontroller.CreateNCRequestFromDynamicNNC(nnc) if err != nil { return errors.Wrap(err, "failed to convert NNC status to network container request") } diff --git a/cns/singletenantcontroller/conversion.go b/cns/singletenantcontroller/conversion.go index 8cf7dd2de0..8b3e897d25 100644 --- a/cns/singletenantcontroller/conversion.go +++ b/cns/singletenantcontroller/conversion.go @@ -37,12 +37,12 @@ func (f NodeNetworkConfigListenerFunc) Update(nnc *v1alpha.NodeNetworkConfig) er // SwiftNodeNetworkConfigListener return a function which satisfies the NodeNetworkConfigListener // interface. It accepts a CreateOrUpdateNetworkContainerInternal implementation, and when Update -// is called, transforms the NNC in to an NC Request and calls the CNS Service implementation with +// is called, transforms the dynamic NNC in to an NC Request and calls the CNS Service implementation with // that request. func SwiftNodeNetworkConfigListener(cnscli cnsClient) NodeNetworkConfigListenerFunc { return func(nnc *v1alpha.NodeNetworkConfig) error { // Create NC request and hand it off to CNS - ncRequest, err := CRDStatusToNCRequest(&nnc.Status) + ncRequest, err := CreateNCRequestFromDynamicNNC(nnc) if err != nil { return errors.Wrap(err, "failed to convert NNC status to network container request") } @@ -59,19 +59,19 @@ func SwiftNodeNetworkConfigListener(cnscli cnsClient) NodeNetworkConfigListenerF } } -// CRDStatusToNCRequest translates a crd status to createnetworkcontainer request -func CRDStatusToNCRequest(status *v1alpha.NodeNetworkConfigStatus) (cns.CreateNetworkContainerRequest, error) { +// CreateNCRequestFromDynamicNNC translates a crd status to createnetworkcontainer request for dynamic NNC (swift) +func CreateNCRequestFromDynamicNNC(nnc *v1alpha.NodeNetworkConfig) (cns.CreateNetworkContainerRequest, error) { // if NNC has no NC, return an empty request - if len(status.NetworkContainers) == 0 { + if len(nnc.Status.NetworkContainers) == 0 { return cns.CreateNetworkContainerRequest{}, nil } // only support a single NC per node, error on more - if len(status.NetworkContainers) > 1 { - return cns.CreateNetworkContainerRequest{}, errors.Wrapf(ErrUnsupportedNCQuantity, "count: %d", len(status.NetworkContainers)) + if len(nnc.Status.NetworkContainers) > 1 { + return cns.CreateNetworkContainerRequest{}, errors.Wrapf(ErrUnsupportedNCQuantity, "count: %d", len(nnc.Status.NetworkContainers)) } - nc := status.NetworkContainers[0] + nc := nnc.Status.NetworkContainers[0] primaryIP := nc.PrimaryIP // if the PrimaryIP is not a CIDR, append a /32 @@ -84,14 +84,14 @@ func CRDStatusToNCRequest(status *v1alpha.NodeNetworkConfigStatus) (cns.CreateNe return cns.CreateNetworkContainerRequest{}, errors.Wrapf(err, "IP: %s", primaryIP) } - secondaryPrefix, err := netip.ParsePrefix(nc.SubnetAddressSpace) + subnetPrefix, err := netip.ParsePrefix(nc.SubnetAddressSpace) if err != nil { return cns.CreateNetworkContainerRequest{}, errors.Wrapf(err, "invalid SubnetAddressSpace %s", nc.SubnetAddressSpace) } subnet := cns.IPSubnet{ IPAddress: primaryPrefix.Addr().String(), - PrefixLength: uint8(secondaryPrefix.Bits()), + PrefixLength: uint8(subnetPrefix.Bits()), } secondaryIPConfigs := map[string]cns.SecondaryIPConfig{} @@ -109,7 +109,87 @@ func CRDStatusToNCRequest(status *v1alpha.NodeNetworkConfigStatus) (cns.CreateNe SecondaryIPConfigs: secondaryIPConfigs, NetworkContainerid: nc.ID, NetworkContainerType: cns.Docker, - Version: strconv.FormatInt(nc.Version, 10), + Version: strconv.FormatInt(nc.Version, 10), //nolint:gomnd // it's decimal + IPConfiguration: cns.IPConfiguration{ + IPSubnet: subnet, + GatewayIPAddress: nc.DefaultGateway, + }, + }, nil +} + +// OverlayNodeNetworkConfigListener returns a function which satisfies the NodeNetworkConfigListener +// interface. It accepts a CreateOrUpdateNetworkContainerInternal implementation, and when Update +// is called, transforms the static NNC in to an NC Request and calls the CNS Service implementation with +// that request. +func OverlayNodeNetworkConfigListener(cnscli cnsClient) NodeNetworkConfigListenerFunc { + return func(nnc *v1alpha.NodeNetworkConfig) error { + // Create NC request and hand it off to CNS + ncRequest, err := CreateNCRequestFromDynamicNNC(nnc) + if err != nil { + return errors.Wrap(err, "failed to convert NNC status to network container request") + } + responseCode := cnscli.CreateOrUpdateNetworkContainerInternal(&ncRequest) + err = restserver.ResponseCodeToError(responseCode) + if err != nil { + logger.Errorf("[cns-rc] Error creating or updating NC in reconcile: %v", err) + return errors.Wrap(err, "failed to create or update network container") + } + + // record assigned IPs metric + allocatedIPs.Set(float64(len(nnc.Status.NetworkContainers[0].IPAssignments))) + return nil + } +} + +// CreateNCRequestFromStaticNNC translates a crd status to createnetworkcontainer request for static NNC (overlay) +func CreateNCRequestFromStaticNNC(nnc *v1alpha.NodeNetworkConfig) (cns.CreateNetworkContainerRequest, error) { + // if NNC has no NC, return an empty request + if len(nnc.Status.NetworkContainers) == 0 { + return cns.CreateNetworkContainerRequest{}, nil + } + + // only support a single NC per node, error on more + if len(nnc.Status.NetworkContainers) > 1 { + return cns.CreateNetworkContainerRequest{}, errors.Wrapf(ErrUnsupportedNCQuantity, "count: %d", len(nnc.Status.NetworkContainers)) + } + + nc := nnc.Status.NetworkContainers[0] + + primaryPrefix, err := netip.ParsePrefix(nc.PrimaryIP) + if err != nil { + return cns.CreateNetworkContainerRequest{}, errors.Wrapf(err, "IP: %s", nc.PrimaryIP) + } + + subnetPrefix, err := netip.ParsePrefix(nc.SubnetAddressSpace) + if err != nil { + return cns.CreateNetworkContainerRequest{}, errors.Wrapf(err, "invalid SubnetAddressSpace %s", nc.SubnetAddressSpace) + } + + subnet := cns.IPSubnet{ + IPAddress: primaryPrefix.Addr().String(), + PrefixLength: uint8(subnetPrefix.Bits()), + } + + secondaryIPConfigs := map[string]cns.SecondaryIPConfig{} + + // iterate through all IP addresses in the subnet described by primaryPrefix and + // add them to the request as secondary IPConfigs. Skip the specific IP of the + // primaryPrefix; that is the gateway. + zeroAddr := primaryPrefix.Masked().Addr() // the masked address is the 0th IP in the subnet + for addr := zeroAddr.Next(); primaryPrefix.Contains(addr); addr = addr.Next() { + if addr == primaryPrefix.Addr() { + continue + } + secondaryIPConfigs[addr.String()] = cns.SecondaryIPConfig{ + IPAddress: addr.String(), + NCVersion: int(nc.Version), + } + } + return cns.CreateNetworkContainerRequest{ + SecondaryIPConfigs: secondaryIPConfigs, + NetworkContainerid: nc.ID, + NetworkContainerType: cns.Docker, + Version: strconv.FormatInt(nc.Version, 10), //nolint:gomnd // it's decimal IPConfiguration: cns.IPConfiguration{ IPSubnet: subnet, GatewayIPAddress: nc.DefaultGateway, diff --git a/cns/singletenantcontroller/conversion_test.go b/cns/singletenantcontroller/conversion_test.go index 4dd4ea4651..ab8b0b9089 100644 --- a/cns/singletenantcontroller/conversion_test.go +++ b/cns/singletenantcontroller/conversion_test.go @@ -74,41 +74,47 @@ var validRequest = cns.CreateNetworkContainerRequest{ func TestConvertNNCStatusToNCRequest(t *testing.T) { tests := []struct { name string - input v1alpha.NodeNetworkConfigStatus + input v1alpha.NodeNetworkConfig want cns.CreateNetworkContainerRequest wantErr bool }{ { - name: "valid", - input: validStatus, + name: "valid", + input: v1alpha.NodeNetworkConfig{ + Status: validStatus, + }, wantErr: false, want: validRequest, }, { name: "no nc", - input: v1alpha.NodeNetworkConfigStatus{}, + input: v1alpha.NodeNetworkConfig{}, wantErr: false, want: cns.CreateNetworkContainerRequest{}, }, { - name: ">1 nc", - input: invalidStatusMultiNC, + name: ">1 nc", + input: v1alpha.NodeNetworkConfig{ + Status: invalidStatusMultiNC, + }, wantErr: true, }, { name: "malformed primary IP", - input: v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: ipMalformed, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: uuid, - IP: testSecIP, + input: v1alpha.NodeNetworkConfig{ + Status: v1alpha.NodeNetworkConfigStatus{ + NetworkContainers: []v1alpha.NetworkContainer{ + { + PrimaryIP: ipMalformed, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: testSecIP, + }, }, + SubnetAddressSpace: subnetAddressSpace, }, - SubnetAddressSpace: subnetAddressSpace, }, }, }, @@ -116,18 +122,20 @@ func TestConvertNNCStatusToNCRequest(t *testing.T) { }, { name: "malformed IP assignment", - input: v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: primaryIP, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: uuid, - IP: ipMalformed, + input: v1alpha.NodeNetworkConfig{ + Status: v1alpha.NodeNetworkConfigStatus{ + NetworkContainers: []v1alpha.NetworkContainer{ + { + PrimaryIP: primaryIP, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: ipMalformed, + }, }, + SubnetAddressSpace: subnetAddressSpace, }, - SubnetAddressSpace: subnetAddressSpace, }, }, }, @@ -135,25 +143,27 @@ func TestConvertNNCStatusToNCRequest(t *testing.T) { }, { name: "IP is CIDR", - input: v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: ipIsCIDR, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: uuid, - IP: testSecIP, + input: v1alpha.NodeNetworkConfig{ + Status: v1alpha.NodeNetworkConfigStatus{ + NetworkContainers: []v1alpha.NetworkContainer{ + { + PrimaryIP: ipIsCIDR, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: testSecIP, + }, }, + SubnetName: subnetName, + DefaultGateway: defaultGateway, + SubnetAddressSpace: subnetAddressSpace, + Version: version, }, - SubnetName: subnetName, - DefaultGateway: defaultGateway, - SubnetAddressSpace: subnetAddressSpace, - Version: version, }, - }, - Scaler: v1alpha.Scaler{ - BatchSize: 1, + Scaler: v1alpha.Scaler{ + BatchSize: 1, + }, }, }, wantErr: false, @@ -161,18 +171,20 @@ func TestConvertNNCStatusToNCRequest(t *testing.T) { }, { name: "IP assignment is CIDR", - input: v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: primaryIP, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: uuid, - IP: ipIsCIDR, + input: v1alpha.NodeNetworkConfig{ + Status: v1alpha.NodeNetworkConfigStatus{ + NetworkContainers: []v1alpha.NetworkContainer{ + { + PrimaryIP: primaryIP, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: ipIsCIDR, + }, }, + SubnetAddressSpace: subnetAddressSpace, }, - SubnetAddressSpace: subnetAddressSpace, }, }, }, @@ -180,18 +192,20 @@ func TestConvertNNCStatusToNCRequest(t *testing.T) { }, { name: "address space is not CIDR", - input: v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: primaryIP, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: uuid, - IP: testSecIP, + input: v1alpha.NodeNetworkConfig{ + Status: v1alpha.NodeNetworkConfigStatus{ + NetworkContainers: []v1alpha.NetworkContainer{ + { + PrimaryIP: primaryIP, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: testSecIP, + }, }, + SubnetAddressSpace: "10.0.0.0", // not a cidr range }, - SubnetAddressSpace: "10.0.0.0", // not a cidr range }, }, }, @@ -201,7 +215,7 @@ func TestConvertNNCStatusToNCRequest(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - got, err := CRDStatusToNCRequest(&tt.input) + got, err := CreateNCRequestFromDynamicNNC(&tt.input) if tt.wantErr { assert.Error(t, err) return From 627911df310a7d43d011c52db51de1349f1e2c89 Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Tue, 12 Apr 2022 00:58:33 +0000 Subject: [PATCH 2/4] inline err check Signed-off-by: Evan Baker --- cns/singletenantcontroller/conversion.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/cns/singletenantcontroller/conversion.go b/cns/singletenantcontroller/conversion.go index 8b3e897d25..1c4ed70d46 100644 --- a/cns/singletenantcontroller/conversion.go +++ b/cns/singletenantcontroller/conversion.go @@ -47,8 +47,7 @@ func SwiftNodeNetworkConfigListener(cnscli cnsClient) NodeNetworkConfigListenerF return errors.Wrap(err, "failed to convert NNC status to network container request") } responseCode := cnscli.CreateOrUpdateNetworkContainerInternal(&ncRequest) - err = restserver.ResponseCodeToError(responseCode) - if err != nil { + if err := restserver.ResponseCodeToError(responseCode); err != nil { logger.Errorf("[cns-rc] Error creating or updating NC in reconcile: %v", err) return errors.Wrap(err, "failed to create or update network container") } @@ -129,8 +128,7 @@ func OverlayNodeNetworkConfigListener(cnscli cnsClient) NodeNetworkConfigListene return errors.Wrap(err, "failed to convert NNC status to network container request") } responseCode := cnscli.CreateOrUpdateNetworkContainerInternal(&ncRequest) - err = restserver.ResponseCodeToError(responseCode) - if err != nil { + if err := restserver.ResponseCodeToError(responseCode); err != nil { logger.Errorf("[cns-rc] Error creating or updating NC in reconcile: %v", err) return errors.Wrap(err, "failed to create or update network container") } From a9de88cbe5936ff6f5f5647ce95968e5defcbc4f Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Wed, 13 Apr 2022 17:33:08 +0000 Subject: [PATCH 3/4] pass NodeNetworkConfigs to Update method by value Signed-off-by: Evan Baker --- cns/api.go | 2 +- cns/client/client_test.go | 2 +- cns/fakes/monitor.go | 5 ++-- cns/fakes/requestcontrollerfake.go | 2 +- cns/ipampool/monitor.go | 5 ++-- cns/ipampool/monitor_test.go | 3 ++- cns/restserver/internalapi_test.go | 4 ++-- cns/restserver/ipam_test.go | 2 +- cns/service/main.go | 2 +- cns/singletenantcontroller/conversion.go | 23 +++++++++---------- cns/singletenantcontroller/conversion_test.go | 2 +- cns/singletenantcontroller/reconciler.go | 4 ++-- cns/singletenantcontroller/reconciler_test.go | 11 +++++---- 13 files changed, 35 insertions(+), 32 deletions(-) diff --git a/cns/api.go b/cns/api.go index 1e2b8349db..5ac3c9d21b 100644 --- a/cns/api.go +++ b/cns/api.go @@ -271,7 +271,7 @@ type NodeConfiguration struct { type IPAMPoolMonitor interface { Start(ctx context.Context) error - Update(nnc *v1alpha.NodeNetworkConfig) error + Update(nnc v1alpha.NodeNetworkConfig) error GetStateSnapshot() IpamPoolMonitorStateSnapshot } diff --git a/cns/client/client_test.go b/cns/client/client_test.go index fb241c9e21..14fd67ac7b 100644 --- a/cns/client/client_test.go +++ b/cns/client/client_test.go @@ -97,7 +97,7 @@ func addTestStateToRestServer(t *testing.T, secondaryIps []string) { t.Fatalf("Failed to createNetworkContainerRequest, req: %+v, err: %d", req, returnCode) } - svc.IPAMPoolMonitor.Update(&v1alpha.NodeNetworkConfig{ + _ = svc.IPAMPoolMonitor.Update(v1alpha.NodeNetworkConfig{ Spec: v1alpha.NodeNetworkConfigSpec{ RequestedIPCount: 16, IPsNotInUse: []string{"abc"}, diff --git a/cns/fakes/monitor.go b/cns/fakes/monitor.go index e3dd49b2a8..592ac7a340 100644 --- a/cns/fakes/monitor.go +++ b/cns/fakes/monitor.go @@ -19,8 +19,9 @@ func (*MonitorFake) Start(ctx context.Context) error { return nil } -func (f *MonitorFake) Update(nnc *v1alpha.NodeNetworkConfig) error { - f.NodeNetworkConfig = nnc +//nolint:gocritic //ignore hugeparam +func (f *MonitorFake) Update(nnc v1alpha.NodeNetworkConfig) error { + f.NodeNetworkConfig = &nnc return nil } diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index f7b968e78c..93cead4125 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/cns/fakes/requestcontrollerfake.go @@ -115,7 +115,7 @@ func (rc *RequestControllerFake) Reconcile(removePendingReleaseIPs bool) error { } // update - rc.cnscli.PoolMonitor.Update(rc.NNC) + _ = rc.cnscli.PoolMonitor.Update(*rc.NNC) return nil } diff --git a/cns/ipampool/monitor.go b/cns/ipampool/monitor.go index dceb3be3e0..2cf8347725 100644 --- a/cns/ipampool/monitor.go +++ b/cns/ipampool/monitor.go @@ -370,7 +370,8 @@ func GenerateARMID(nc *v1alpha.NetworkContainer) string { // the pool reconcile loop. // If the Monitor has not been Started, this will block until Start() is called, which will // immediately read this passed NNC and start the pool reconcile loop. -func (pm *Monitor) Update(nnc *v1alpha.NodeNetworkConfig) error { +//nolint:gocritic //ignore hugeparam +func (pm *Monitor) Update(nnc v1alpha.NodeNetworkConfig) error { pm.clampScaler(&nnc.Status.Scaler) // if the nnc has converged, observe the pool scaling latency (if any). @@ -380,7 +381,7 @@ func (pm *Monitor) Update(nnc *v1alpha.NodeNetworkConfig) error { metric.ObserverPoolScaleLatency() } logger.Printf("[ipam-pool-monitor] pushing NodeNetworkConfig update, allocatedIPs = %d", allocatedIPs) - pm.nncSource <- *nnc + pm.nncSource <- nnc return nil } diff --git a/cns/ipampool/monitor_test.go b/cns/ipampool/monitor_test.go index fcdbbb00c0..8a8e2e8727 100644 --- a/cns/ipampool/monitor_test.go +++ b/cns/ipampool/monitor_test.go @@ -26,7 +26,8 @@ type directUpdatePoolMonitor struct { cns.IPAMPoolMonitor } -func (d *directUpdatePoolMonitor) Update(nnc *v1alpha.NodeNetworkConfig) error { +//nolint:gocritic //ignore hugeparam +func (d *directUpdatePoolMonitor) Update(nnc v1alpha.NodeNetworkConfig) error { scaler := nnc.Status.Scaler d.m.spec = nnc.Spec d.m.metastate.minFreeCount, d.m.metastate.maxFreeCount = CalculateMinFreeIPs(scaler), CalculateMaxFreeIPs(scaler) diff --git a/cns/restserver/internalapi_test.go b/cns/restserver/internalapi_test.go index 64fd1b3cf7..be4342b76b 100644 --- a/cns/restserver/internalapi_test.go +++ b/cns/restserver/internalapi_test.go @@ -425,7 +425,7 @@ func createAndValidateNCRequest(t *testing.T, secondaryIPConfigs map[string]cns. if returnCode != 0 { t.Fatalf("Failed to createNetworkContainerRequest, req: %+v, err: %d", req, returnCode) } - svc.IPAMPoolMonitor.Update(&v1alpha.NodeNetworkConfig{ + _ = svc.IPAMPoolMonitor.Update(v1alpha.NodeNetworkConfig{ Status: v1alpha.NodeNetworkConfigStatus{ Scaler: v1alpha.Scaler{ BatchSize: batchSize, @@ -598,7 +598,7 @@ func createNCReqInternal(t *testing.T, secondaryIPConfigs map[string]cns.Seconda if returnCode != 0 { t.Fatalf("Failed to createNetworkContainerRequest, req: %+v, err: %d", req, returnCode) } - svc.IPAMPoolMonitor.Update(&v1alpha.NodeNetworkConfig{ + _ = svc.IPAMPoolMonitor.Update(v1alpha.NodeNetworkConfig{ Status: v1alpha.NodeNetworkConfigStatus{ Scaler: v1alpha.Scaler{ BatchSize: batchSize, diff --git a/cns/restserver/ipam_test.go b/cns/restserver/ipam_test.go index dc0a5f8e9c..8239998e8f 100644 --- a/cns/restserver/ipam_test.go +++ b/cns/restserver/ipam_test.go @@ -598,7 +598,7 @@ func TestIPAMMarkIPAsPendingWithPendingProgrammingIPs(t *testing.T) { t.Fatalf("Failed to createNetworkContainerRequest, req: %+v, err: %d", req, returnCode) } svc.IPAMPoolMonitor.Update( - &v1alpha.NodeNetworkConfig{ + v1alpha.NodeNetworkConfig{ Status: v1alpha.NodeNetworkConfigStatus{ Scaler: v1alpha.Scaler{ BatchSize: batchSize, diff --git a/cns/service/main.go b/cns/service/main.go index 6178ccced9..2a8123a5c2 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -865,7 +865,7 @@ func reconcileInitialCNSState(ctx context.Context, cli nodeNetworkConfigGetter, } // Convert to CreateNetworkContainerRequest - ncRequest, err := kubecontroller.CreateNCRequestFromDynamicNNC(nnc) + ncRequest, err := kubecontroller.CreateNCRequestFromDynamicNNC(*nnc) if err != nil { return errors.Wrap(err, "failed to convert NNC status to network container request") } diff --git a/cns/singletenantcontroller/conversion.go b/cns/singletenantcontroller/conversion.go index 1c4ed70d46..358fbfd4f3 100644 --- a/cns/singletenantcontroller/conversion.go +++ b/cns/singletenantcontroller/conversion.go @@ -29,9 +29,10 @@ type cnsClient interface { var _ nodeNetworkConfigListener = (NodeNetworkConfigListenerFunc)(nil) //nolint:gocritic // clarity -type NodeNetworkConfigListenerFunc func(*v1alpha.NodeNetworkConfig) error +type NodeNetworkConfigListenerFunc func(v1alpha.NodeNetworkConfig) error -func (f NodeNetworkConfigListenerFunc) Update(nnc *v1alpha.NodeNetworkConfig) error { +//nolint:gocritic //ignore hugeparam +func (f NodeNetworkConfigListenerFunc) Update(nnc v1alpha.NodeNetworkConfig) error { return f(nnc) } @@ -40,7 +41,7 @@ func (f NodeNetworkConfigListenerFunc) Update(nnc *v1alpha.NodeNetworkConfig) er // is called, transforms the dynamic NNC in to an NC Request and calls the CNS Service implementation with // that request. func SwiftNodeNetworkConfigListener(cnscli cnsClient) NodeNetworkConfigListenerFunc { - return func(nnc *v1alpha.NodeNetworkConfig) error { + return func(nnc v1alpha.NodeNetworkConfig) error { // Create NC request and hand it off to CNS ncRequest, err := CreateNCRequestFromDynamicNNC(nnc) if err != nil { @@ -59,7 +60,8 @@ func SwiftNodeNetworkConfigListener(cnscli cnsClient) NodeNetworkConfigListenerF } // CreateNCRequestFromDynamicNNC translates a crd status to createnetworkcontainer request for dynamic NNC (swift) -func CreateNCRequestFromDynamicNNC(nnc *v1alpha.NodeNetworkConfig) (cns.CreateNetworkContainerRequest, error) { +//nolint:gocritic //ignore hugeparam +func CreateNCRequestFromDynamicNNC(nnc v1alpha.NodeNetworkConfig) (cns.CreateNetworkContainerRequest, error) { // if NNC has no NC, return an empty request if len(nnc.Status.NetworkContainers) == 0 { return cns.CreateNetworkContainerRequest{}, nil @@ -121,9 +123,9 @@ func CreateNCRequestFromDynamicNNC(nnc *v1alpha.NodeNetworkConfig) (cns.CreateNe // is called, transforms the static NNC in to an NC Request and calls the CNS Service implementation with // that request. func OverlayNodeNetworkConfigListener(cnscli cnsClient) NodeNetworkConfigListenerFunc { - return func(nnc *v1alpha.NodeNetworkConfig) error { + return func(nnc v1alpha.NodeNetworkConfig) error { // Create NC request and hand it off to CNS - ncRequest, err := CreateNCRequestFromDynamicNNC(nnc) + ncRequest, err := CreateNCRequestFromStaticNNC(nnc) if err != nil { return errors.Wrap(err, "failed to convert NNC status to network container request") } @@ -140,7 +142,8 @@ func OverlayNodeNetworkConfigListener(cnscli cnsClient) NodeNetworkConfigListene } // CreateNCRequestFromStaticNNC translates a crd status to createnetworkcontainer request for static NNC (overlay) -func CreateNCRequestFromStaticNNC(nnc *v1alpha.NodeNetworkConfig) (cns.CreateNetworkContainerRequest, error) { +//nolint:gocritic //ignore hugeparam +func CreateNCRequestFromStaticNNC(nnc v1alpha.NodeNetworkConfig) (cns.CreateNetworkContainerRequest, error) { // if NNC has no NC, return an empty request if len(nnc.Status.NetworkContainers) == 0 { return cns.CreateNetworkContainerRequest{}, nil @@ -171,13 +174,9 @@ func CreateNCRequestFromStaticNNC(nnc *v1alpha.NodeNetworkConfig) (cns.CreateNet secondaryIPConfigs := map[string]cns.SecondaryIPConfig{} // iterate through all IP addresses in the subnet described by primaryPrefix and - // add them to the request as secondary IPConfigs. Skip the specific IP of the - // primaryPrefix; that is the gateway. + // add them to the request as secondary IPConfigs. zeroAddr := primaryPrefix.Masked().Addr() // the masked address is the 0th IP in the subnet for addr := zeroAddr.Next(); primaryPrefix.Contains(addr); addr = addr.Next() { - if addr == primaryPrefix.Addr() { - continue - } secondaryIPConfigs[addr.String()] = cns.SecondaryIPConfig{ IPAddress: addr.String(), NCVersion: int(nc.Version), diff --git a/cns/singletenantcontroller/conversion_test.go b/cns/singletenantcontroller/conversion_test.go index ab8b0b9089..48acffbc9b 100644 --- a/cns/singletenantcontroller/conversion_test.go +++ b/cns/singletenantcontroller/conversion_test.go @@ -215,7 +215,7 @@ func TestConvertNNCStatusToNCRequest(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - got, err := CreateNCRequestFromDynamicNNC(&tt.input) + got, err := CreateNCRequestFromDynamicNNC(tt.input) if tt.wantErr { assert.Error(t, err) return diff --git a/cns/singletenantcontroller/reconciler.go b/cns/singletenantcontroller/reconciler.go index e0b436911b..ec68eff710 100644 --- a/cns/singletenantcontroller/reconciler.go +++ b/cns/singletenantcontroller/reconciler.go @@ -19,7 +19,7 @@ import ( ) type nodeNetworkConfigListener interface { - Update(*v1alpha.NodeNetworkConfig) error + Update(v1alpha.NodeNetworkConfig) error } type nncGetter interface { @@ -68,7 +68,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco // push the NNC to the registered NNC Sinks for i := range r.nncListeners { - if err := r.nncListeners[i].Update(nnc); err != nil { + if err := r.nncListeners[i].Update(*nnc); err != nil { return reconcile.Result{}, errors.Wrap(err, "nnc listener return error during update") } } diff --git a/cns/singletenantcontroller/reconciler_test.go b/cns/singletenantcontroller/reconciler_test.go index 39c9cd2e4e..368a990173 100644 --- a/cns/singletenantcontroller/reconciler_test.go +++ b/cns/singletenantcontroller/reconciler_test.go @@ -19,13 +19,13 @@ import ( type cnsClientState struct { req *cns.CreateNetworkContainerRequest - nnc *v1alpha.NodeNetworkConfig + nnc v1alpha.NodeNetworkConfig } type mockCNSClient struct { state cnsClientState createOrUpdateNC func(*cns.CreateNetworkContainerRequest) cnstypes.ResponseCode - update func(*v1alpha.NodeNetworkConfig) error + update func(v1alpha.NodeNetworkConfig) error } //nolint:gocritic // ignore hugeParam pls @@ -34,7 +34,8 @@ func (m *mockCNSClient) CreateOrUpdateNetworkContainerInternal(req *cns.CreateNe return m.createOrUpdateNC(req) } -func (m *mockCNSClient) Update(nnc *v1alpha.NodeNetworkConfig) error { +//nolint:gocritic //ignore hugeparam +func (m *mockCNSClient) Update(nnc v1alpha.NodeNetworkConfig) error { m.state.nnc = nnc return m.update(nnc) } @@ -131,14 +132,14 @@ func TestReconcile(t *testing.T) { createOrUpdateNC: func(*cns.CreateNetworkContainerRequest) cnstypes.ResponseCode { return cnstypes.Success }, - update: func(*v1alpha.NodeNetworkConfig) error { + update: func(v1alpha.NodeNetworkConfig) error { return nil }, }, wantErr: false, wantCNSClientState: cnsClientState{ req: &validRequest, - nnc: &v1alpha.NodeNetworkConfig{ + nnc: v1alpha.NodeNetworkConfig{ Status: validStatus, Spec: v1alpha.NodeNetworkConfigSpec{ RequestedIPCount: 1, From 7881f76f06a19d42ab6ea33ff96f6ae09e266bc8 Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Fri, 15 Apr 2022 00:02:42 +0000 Subject: [PATCH 4/4] rework nnc reconcile flow for NetworkContainer modes Signed-off-by: Evan Baker --- Makefile | 4 +- cns/api.go | 2 +- cns/client/client_test.go | 2 +- cns/fakes/monitor.go | 5 +- cns/fakes/requestcontrollerfake.go | 2 +- cns/ipampool/monitor.go | 5 +- cns/ipampool/monitor_test.go | 3 +- cns/restserver/internalapi_test.go | 4 +- cns/restserver/ipam_test.go | 2 +- cns/service/main.go | 30 +- cns/singletenantcontroller/conversion.go | 109 +------ cns/singletenantcontroller/conversion_test.go | 306 +++++++++++------- cns/singletenantcontroller/reconciler.go | 65 +++- cns/singletenantcontroller/reconciler_test.go | 28 +- 14 files changed, 299 insertions(+), 268 deletions(-) diff --git a/Makefile b/Makefile index bca5c20a79..f13cc98563 100644 --- a/Makefile +++ b/Makefile @@ -500,11 +500,11 @@ COVER_PKG ?= . test-all: ## run all unit tests. @$(eval COVER_FILTER=`go list --tags ignore_uncovered,ignore_autogenerated $(COVER_PKG)/... | tr '\n' ','`) @echo Test coverpkg: $(COVER_FILTER) - go test -buildvcs=false -tags "unit" -coverpkg=$(COVER_FILTER) -v -race -covermode atomic -failfast -coverprofile=coverage.out $(COVER_PKG)/... + go test -buildvcs=false -tags "unit" -coverpkg=$(COVER_FILTER) -race -covermode atomic -failfast -coverprofile=coverage.out $(COVER_PKG)/... test-integration: ## run all integration tests. - go test -buildvcs=false -timeout 1h -coverpkg=./... -v -race -covermode atomic -coverprofile=coverage.out -tags=integration ./test/integration... + go test -buildvcs=false -timeout 1h -coverpkg=./... -race -covermode atomic -coverprofile=coverage.out -tags=integration ./test/integration... test-cyclonus: ## run the cyclonus test for npm. cd test/cyclonus && bash ./test-cyclonus.sh diff --git a/cns/api.go b/cns/api.go index 5ac3c9d21b..1e2b8349db 100644 --- a/cns/api.go +++ b/cns/api.go @@ -271,7 +271,7 @@ type NodeConfiguration struct { type IPAMPoolMonitor interface { Start(ctx context.Context) error - Update(nnc v1alpha.NodeNetworkConfig) error + Update(nnc *v1alpha.NodeNetworkConfig) error GetStateSnapshot() IpamPoolMonitorStateSnapshot } diff --git a/cns/client/client_test.go b/cns/client/client_test.go index 14fd67ac7b..ab4cace1ce 100644 --- a/cns/client/client_test.go +++ b/cns/client/client_test.go @@ -97,7 +97,7 @@ func addTestStateToRestServer(t *testing.T, secondaryIps []string) { t.Fatalf("Failed to createNetworkContainerRequest, req: %+v, err: %d", req, returnCode) } - _ = svc.IPAMPoolMonitor.Update(v1alpha.NodeNetworkConfig{ + _ = svc.IPAMPoolMonitor.Update(&v1alpha.NodeNetworkConfig{ Spec: v1alpha.NodeNetworkConfigSpec{ RequestedIPCount: 16, IPsNotInUse: []string{"abc"}, diff --git a/cns/fakes/monitor.go b/cns/fakes/monitor.go index 592ac7a340..e3dd49b2a8 100644 --- a/cns/fakes/monitor.go +++ b/cns/fakes/monitor.go @@ -19,9 +19,8 @@ func (*MonitorFake) Start(ctx context.Context) error { return nil } -//nolint:gocritic //ignore hugeparam -func (f *MonitorFake) Update(nnc v1alpha.NodeNetworkConfig) error { - f.NodeNetworkConfig = &nnc +func (f *MonitorFake) Update(nnc *v1alpha.NodeNetworkConfig) error { + f.NodeNetworkConfig = nnc return nil } diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index 93cead4125..776e971643 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/cns/fakes/requestcontrollerfake.go @@ -115,7 +115,7 @@ func (rc *RequestControllerFake) Reconcile(removePendingReleaseIPs bool) error { } // update - _ = rc.cnscli.PoolMonitor.Update(*rc.NNC) + _ = rc.cnscli.PoolMonitor.Update(rc.NNC) return nil } diff --git a/cns/ipampool/monitor.go b/cns/ipampool/monitor.go index 2cf8347725..dceb3be3e0 100644 --- a/cns/ipampool/monitor.go +++ b/cns/ipampool/monitor.go @@ -370,8 +370,7 @@ func GenerateARMID(nc *v1alpha.NetworkContainer) string { // the pool reconcile loop. // If the Monitor has not been Started, this will block until Start() is called, which will // immediately read this passed NNC and start the pool reconcile loop. -//nolint:gocritic //ignore hugeparam -func (pm *Monitor) Update(nnc v1alpha.NodeNetworkConfig) error { +func (pm *Monitor) Update(nnc *v1alpha.NodeNetworkConfig) error { pm.clampScaler(&nnc.Status.Scaler) // if the nnc has converged, observe the pool scaling latency (if any). @@ -381,7 +380,7 @@ func (pm *Monitor) Update(nnc v1alpha.NodeNetworkConfig) error { metric.ObserverPoolScaleLatency() } logger.Printf("[ipam-pool-monitor] pushing NodeNetworkConfig update, allocatedIPs = %d", allocatedIPs) - pm.nncSource <- nnc + pm.nncSource <- *nnc return nil } diff --git a/cns/ipampool/monitor_test.go b/cns/ipampool/monitor_test.go index 8a8e2e8727..fcdbbb00c0 100644 --- a/cns/ipampool/monitor_test.go +++ b/cns/ipampool/monitor_test.go @@ -26,8 +26,7 @@ type directUpdatePoolMonitor struct { cns.IPAMPoolMonitor } -//nolint:gocritic //ignore hugeparam -func (d *directUpdatePoolMonitor) Update(nnc v1alpha.NodeNetworkConfig) error { +func (d *directUpdatePoolMonitor) Update(nnc *v1alpha.NodeNetworkConfig) error { scaler := nnc.Status.Scaler d.m.spec = nnc.Spec d.m.metastate.minFreeCount, d.m.metastate.maxFreeCount = CalculateMinFreeIPs(scaler), CalculateMaxFreeIPs(scaler) diff --git a/cns/restserver/internalapi_test.go b/cns/restserver/internalapi_test.go index be4342b76b..f5b657ae10 100644 --- a/cns/restserver/internalapi_test.go +++ b/cns/restserver/internalapi_test.go @@ -425,7 +425,7 @@ func createAndValidateNCRequest(t *testing.T, secondaryIPConfigs map[string]cns. if returnCode != 0 { t.Fatalf("Failed to createNetworkContainerRequest, req: %+v, err: %d", req, returnCode) } - _ = svc.IPAMPoolMonitor.Update(v1alpha.NodeNetworkConfig{ + _ = svc.IPAMPoolMonitor.Update(&v1alpha.NodeNetworkConfig{ Status: v1alpha.NodeNetworkConfigStatus{ Scaler: v1alpha.Scaler{ BatchSize: batchSize, @@ -598,7 +598,7 @@ func createNCReqInternal(t *testing.T, secondaryIPConfigs map[string]cns.Seconda if returnCode != 0 { t.Fatalf("Failed to createNetworkContainerRequest, req: %+v, err: %d", req, returnCode) } - _ = svc.IPAMPoolMonitor.Update(v1alpha.NodeNetworkConfig{ + _ = svc.IPAMPoolMonitor.Update(&v1alpha.NodeNetworkConfig{ Status: v1alpha.NodeNetworkConfigStatus{ Scaler: v1alpha.Scaler{ BatchSize: batchSize, diff --git a/cns/restserver/ipam_test.go b/cns/restserver/ipam_test.go index 8239998e8f..dc0a5f8e9c 100644 --- a/cns/restserver/ipam_test.go +++ b/cns/restserver/ipam_test.go @@ -598,7 +598,7 @@ func TestIPAMMarkIPAsPendingWithPendingProgrammingIPs(t *testing.T) { t.Fatalf("Failed to createNetworkContainerRequest, req: %+v, err: %d", req, returnCode) } svc.IPAMPoolMonitor.Update( - v1alpha.NodeNetworkConfig{ + &v1alpha.NodeNetworkConfig{ Status: v1alpha.NodeNetworkConfigStatus{ Scaler: v1alpha.Scaler{ BatchSize: batchSize, diff --git a/cns/service/main.go b/cns/service/main.go index 2a8123a5c2..23b6bc5444 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -865,19 +865,23 @@ func reconcileInitialCNSState(ctx context.Context, cli nodeNetworkConfigGetter, } // Convert to CreateNetworkContainerRequest - ncRequest, err := kubecontroller.CreateNCRequestFromDynamicNNC(*nnc) - if err != nil { - return errors.Wrap(err, "failed to convert NNC status to network container request") - } - // rebuild CNS state - podInfoByIP, err := podInfoByIPProvider.PodInfoByIP() - if err != nil { - return errors.Wrap(err, "provider failed to provide PodInfoByIP") - } + for i := range nnc.Status.NetworkContainers { + ncRequest, err := kubecontroller.CreateNCRequestFromDynamicNC(nnc.Status.NetworkContainers[i]) + if err != nil { + return errors.Wrap(err, "failed to convert NNC status to network container request") + } + // rebuild CNS state + podInfoByIP, err := podInfoByIPProvider.PodInfoByIP() + if err != nil { + return errors.Wrap(err, "provider failed to provide PodInfoByIP") + } - // Call cnsclient init cns passing those two things. - err = restserver.ResponseCodeToError(ncReconciler.ReconcileNCState(&ncRequest, podInfoByIP, nnc)) - return errors.Wrap(err, "failed to reconcile NC state") + // Call cnsclient init cns passing those two things. + if err := restserver.ResponseCodeToError(ncReconciler.ReconcileNCState(ncRequest, podInfoByIP, nnc)); err != nil { + return errors.Wrap(err, "failed to reconcile NC state") + } + } + return nil } // InitializeCRDState builds and starts the CRD controllers. @@ -1010,7 +1014,7 @@ func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cn return errors.Wrapf(err, "failed to get node %s", nodeName) } - reconciler := kubecontroller.NewReconciler(nnccli, kubecontroller.SwiftNodeNetworkConfigListener(httpRestServiceImplementation), poolMonitor) + reconciler := kubecontroller.NewReconciler(httpRestServiceImplementation, nnccli, poolMonitor) // pass Node to the Reconciler for Controller xref if err := reconciler.SetupWithManager(manager, node); err != nil { return errors.Wrapf(err, "failed to setup reconciler with manager") diff --git a/cns/singletenantcontroller/conversion.go b/cns/singletenantcontroller/conversion.go index 358fbfd4f3..afab6c034d 100644 --- a/cns/singletenantcontroller/conversion.go +++ b/cns/singletenantcontroller/conversion.go @@ -7,9 +7,6 @@ import ( "strings" "github.com/Azure/azure-container-networking/cns" - "github.com/Azure/azure-container-networking/cns/logger" - "github.com/Azure/azure-container-networking/cns/restserver" - cnstypes "github.com/Azure/azure-container-networking/cns/types" "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" "github.com/pkg/errors" ) @@ -23,57 +20,9 @@ var ( ErrUnsupportedNCQuantity = errors.New("unsupported number of network containers") ) -type cnsClient interface { - CreateOrUpdateNetworkContainerInternal(*cns.CreateNetworkContainerRequest) cnstypes.ResponseCode -} - -var _ nodeNetworkConfigListener = (NodeNetworkConfigListenerFunc)(nil) //nolint:gocritic // clarity - -type NodeNetworkConfigListenerFunc func(v1alpha.NodeNetworkConfig) error - +// CreateNCRequestFromDynamicNC generates a CreateNetworkContainerRequest from a dynamic NetworkContainer. //nolint:gocritic //ignore hugeparam -func (f NodeNetworkConfigListenerFunc) Update(nnc v1alpha.NodeNetworkConfig) error { - return f(nnc) -} - -// SwiftNodeNetworkConfigListener return a function which satisfies the NodeNetworkConfigListener -// interface. It accepts a CreateOrUpdateNetworkContainerInternal implementation, and when Update -// is called, transforms the dynamic NNC in to an NC Request and calls the CNS Service implementation with -// that request. -func SwiftNodeNetworkConfigListener(cnscli cnsClient) NodeNetworkConfigListenerFunc { - return func(nnc v1alpha.NodeNetworkConfig) error { - // Create NC request and hand it off to CNS - ncRequest, err := CreateNCRequestFromDynamicNNC(nnc) - if err != nil { - return errors.Wrap(err, "failed to convert NNC status to network container request") - } - responseCode := cnscli.CreateOrUpdateNetworkContainerInternal(&ncRequest) - if err := restserver.ResponseCodeToError(responseCode); err != nil { - logger.Errorf("[cns-rc] Error creating or updating NC in reconcile: %v", err) - return errors.Wrap(err, "failed to create or update network container") - } - - // record assigned IPs metric - allocatedIPs.Set(float64(len(nnc.Status.NetworkContainers[0].IPAssignments))) - return nil - } -} - -// CreateNCRequestFromDynamicNNC translates a crd status to createnetworkcontainer request for dynamic NNC (swift) -//nolint:gocritic //ignore hugeparam -func CreateNCRequestFromDynamicNNC(nnc v1alpha.NodeNetworkConfig) (cns.CreateNetworkContainerRequest, error) { - // if NNC has no NC, return an empty request - if len(nnc.Status.NetworkContainers) == 0 { - return cns.CreateNetworkContainerRequest{}, nil - } - - // only support a single NC per node, error on more - if len(nnc.Status.NetworkContainers) > 1 { - return cns.CreateNetworkContainerRequest{}, errors.Wrapf(ErrUnsupportedNCQuantity, "count: %d", len(nnc.Status.NetworkContainers)) - } - - nc := nnc.Status.NetworkContainers[0] - +func CreateNCRequestFromDynamicNC(nc v1alpha.NetworkContainer) (*cns.CreateNetworkContainerRequest, error) { primaryIP := nc.PrimaryIP // if the PrimaryIP is not a CIDR, append a /32 if !strings.Contains(primaryIP, "/") { @@ -82,12 +31,12 @@ func CreateNCRequestFromDynamicNNC(nnc v1alpha.NodeNetworkConfig) (cns.CreateNet primaryPrefix, err := netip.ParsePrefix(primaryIP) if err != nil { - return cns.CreateNetworkContainerRequest{}, errors.Wrapf(err, "IP: %s", primaryIP) + return nil, errors.Wrapf(err, "IP: %s", primaryIP) } subnetPrefix, err := netip.ParsePrefix(nc.SubnetAddressSpace) if err != nil { - return cns.CreateNetworkContainerRequest{}, errors.Wrapf(err, "invalid SubnetAddressSpace %s", nc.SubnetAddressSpace) + return nil, errors.Wrapf(err, "invalid SubnetAddressSpace %s", nc.SubnetAddressSpace) } subnet := cns.IPSubnet{ @@ -99,14 +48,14 @@ func CreateNCRequestFromDynamicNNC(nnc v1alpha.NodeNetworkConfig) (cns.CreateNet for _, ipAssignment := range nc.IPAssignments { secondaryIP := net.ParseIP(ipAssignment.IP) if secondaryIP == nil { - return cns.CreateNetworkContainerRequest{}, errors.Wrapf(ErrInvalidSecondaryIP, "IP: %s", ipAssignment.IP) + return nil, errors.Wrapf(ErrInvalidSecondaryIP, "IP: %s", ipAssignment.IP) } secondaryIPConfigs[ipAssignment.Name] = cns.SecondaryIPConfig{ IPAddress: secondaryIP.String(), NCVersion: int(nc.Version), } } - return cns.CreateNetworkContainerRequest{ + return &cns.CreateNetworkContainerRequest{ SecondaryIPConfigs: secondaryIPConfigs, NetworkContainerid: nc.ID, NetworkContainerType: cns.Docker, @@ -118,54 +67,18 @@ func CreateNCRequestFromDynamicNNC(nnc v1alpha.NodeNetworkConfig) (cns.CreateNet }, nil } -// OverlayNodeNetworkConfigListener returns a function which satisfies the NodeNetworkConfigListener -// interface. It accepts a CreateOrUpdateNetworkContainerInternal implementation, and when Update -// is called, transforms the static NNC in to an NC Request and calls the CNS Service implementation with -// that request. -func OverlayNodeNetworkConfigListener(cnscli cnsClient) NodeNetworkConfigListenerFunc { - return func(nnc v1alpha.NodeNetworkConfig) error { - // Create NC request and hand it off to CNS - ncRequest, err := CreateNCRequestFromStaticNNC(nnc) - if err != nil { - return errors.Wrap(err, "failed to convert NNC status to network container request") - } - responseCode := cnscli.CreateOrUpdateNetworkContainerInternal(&ncRequest) - if err := restserver.ResponseCodeToError(responseCode); err != nil { - logger.Errorf("[cns-rc] Error creating or updating NC in reconcile: %v", err) - return errors.Wrap(err, "failed to create or update network container") - } - - // record assigned IPs metric - allocatedIPs.Set(float64(len(nnc.Status.NetworkContainers[0].IPAssignments))) - return nil - } -} - -// CreateNCRequestFromStaticNNC translates a crd status to createnetworkcontainer request for static NNC (overlay) +// CreateNCRequestFromStaticNC generates a CreateNetworkContainerRequest from a static NetworkContainer. //nolint:gocritic //ignore hugeparam -func CreateNCRequestFromStaticNNC(nnc v1alpha.NodeNetworkConfig) (cns.CreateNetworkContainerRequest, error) { - // if NNC has no NC, return an empty request - if len(nnc.Status.NetworkContainers) == 0 { - return cns.CreateNetworkContainerRequest{}, nil - } - - // only support a single NC per node, error on more - if len(nnc.Status.NetworkContainers) > 1 { - return cns.CreateNetworkContainerRequest{}, errors.Wrapf(ErrUnsupportedNCQuantity, "count: %d", len(nnc.Status.NetworkContainers)) - } - - nc := nnc.Status.NetworkContainers[0] - +func CreateNCRequestFromStaticNC(nc v1alpha.NetworkContainer) (*cns.CreateNetworkContainerRequest, error) { primaryPrefix, err := netip.ParsePrefix(nc.PrimaryIP) if err != nil { - return cns.CreateNetworkContainerRequest{}, errors.Wrapf(err, "IP: %s", nc.PrimaryIP) + return nil, errors.Wrapf(err, "IP: %s", nc.PrimaryIP) } subnetPrefix, err := netip.ParsePrefix(nc.SubnetAddressSpace) if err != nil { - return cns.CreateNetworkContainerRequest{}, errors.Wrapf(err, "invalid SubnetAddressSpace %s", nc.SubnetAddressSpace) + return nil, errors.Wrapf(err, "invalid SubnetAddressSpace %s", nc.SubnetAddressSpace) } - subnet := cns.IPSubnet{ IPAddress: primaryPrefix.Addr().String(), PrefixLength: uint8(subnetPrefix.Bits()), @@ -182,7 +95,7 @@ func CreateNCRequestFromStaticNNC(nnc v1alpha.NodeNetworkConfig) (cns.CreateNetw NCVersion: int(nc.Version), } } - return cns.CreateNetworkContainerRequest{ + return &cns.CreateNetworkContainerRequest{ SecondaryIPConfigs: secondaryIPConfigs, NetworkContainerid: nc.ID, NetworkContainerType: cns.Docker, diff --git a/cns/singletenantcontroller/conversion_test.go b/cns/singletenantcontroller/conversion_test.go index 48acffbc9b..3df4e0fc98 100644 --- a/cns/singletenantcontroller/conversion_test.go +++ b/cns/singletenantcontroller/conversion_test.go @@ -16,6 +16,7 @@ const ( ipMalformed = "10.0.0.0.0" ncID = "160005ba-cd02-11ea-87d0-0242ac130003" primaryIP = "10.0.0.1" + overlayPrimaryIP = "10.0.0.1/30" subnetAddressSpace = "10.0.0.0/24" subnetName = "subnet1" subnetPrefixLen = 24 @@ -30,29 +31,30 @@ var invalidStatusMultiNC = v1alpha.NodeNetworkConfigStatus{ }, } -var validStatus = v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ +var validSwiftNC = v1alpha.NetworkContainer{ + ID: ncID, + AssignmentMode: v1alpha.Dynamic, + Type: v1alpha.VNET, + PrimaryIP: primaryIP, + IPAssignments: []v1alpha.IPAssignment{ { - PrimaryIP: primaryIP, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: uuid, - IP: testSecIP, - }, - }, - SubnetName: subnetName, - DefaultGateway: defaultGateway, - SubnetAddressSpace: subnetAddressSpace, - Version: version, + Name: uuid, + IP: testSecIP, }, }, - Scaler: v1alpha.Scaler{ - BatchSize: 1, + SubnetName: subnetName, + DefaultGateway: defaultGateway, + SubnetAddressSpace: subnetAddressSpace, + Version: version, +} + +var validSwiftStatus = v1alpha.NodeNetworkConfigStatus{ + NetworkContainers: []v1alpha.NetworkContainer{ + validSwiftNC, }, } -var validRequest = cns.CreateNetworkContainerRequest{ +var validSwiftRequest = &cns.CreateNetworkContainerRequest{ Version: strconv.FormatInt(version, 10), IPConfiguration: cns.IPConfiguration{ GatewayIPAddress: defaultGateway, @@ -71,143 +73,221 @@ var validRequest = cns.CreateNetworkContainerRequest{ }, } -func TestConvertNNCStatusToNCRequest(t *testing.T) { +var validOverlayNC = v1alpha.NetworkContainer{ + ID: ncID, + AssignmentMode: v1alpha.Static, + Type: v1alpha.Overlay, + PrimaryIP: overlayPrimaryIP, + SubnetName: subnetName, + SubnetAddressSpace: subnetAddressSpace, + Version: version, +} + +var validOverlayRequest = &cns.CreateNetworkContainerRequest{ + Version: strconv.FormatInt(version, 10), + IPConfiguration: cns.IPConfiguration{ + IPSubnet: cns.IPSubnet{ + PrefixLength: uint8(subnetPrefixLen), + IPAddress: primaryIP, + }, + }, + NetworkContainerid: ncID, + NetworkContainerType: cns.Docker, + SecondaryIPConfigs: map[string]cns.SecondaryIPConfig{ + "10.0.0.1": { + IPAddress: "10.0.0.1", + NCVersion: version, + }, + "10.0.0.2": { + IPAddress: "10.0.0.2", + NCVersion: version, + }, + "10.0.0.3": { + IPAddress: "10.0.0.3", + NCVersion: version, + }, + }, +} + +func TestCreateNCRequestFromDynamicNC(t *testing.T) { tests := []struct { name string - input v1alpha.NodeNetworkConfig - want cns.CreateNetworkContainerRequest + input v1alpha.NetworkContainer + want *cns.CreateNetworkContainerRequest wantErr bool }{ { - name: "valid", - input: v1alpha.NodeNetworkConfig{ - Status: validStatus, - }, + name: "valid swift", + input: validSwiftNC, wantErr: false, - want: validRequest, + want: validSwiftRequest, }, { - name: "no nc", - input: v1alpha.NodeNetworkConfig{}, - wantErr: false, - want: cns.CreateNetworkContainerRequest{}, + name: "malformed primary IP", + input: v1alpha.NetworkContainer{ + PrimaryIP: ipMalformed, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: testSecIP, + }, + }, + SubnetAddressSpace: subnetAddressSpace, + }, + + wantErr: true, }, { - name: ">1 nc", - input: v1alpha.NodeNetworkConfig{ - Status: invalidStatusMultiNC, + name: "malformed IP assignment", + input: v1alpha.NetworkContainer{ + PrimaryIP: primaryIP, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: ipMalformed, + }, + }, + SubnetAddressSpace: subnetAddressSpace, }, wantErr: true, }, { - name: "malformed primary IP", - input: v1alpha.NodeNetworkConfig{ - Status: v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: ipMalformed, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: uuid, - IP: testSecIP, - }, - }, - SubnetAddressSpace: subnetAddressSpace, - }, + name: "IP is CIDR", + input: v1alpha.NetworkContainer{ + PrimaryIP: ipIsCIDR, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: testSecIP, }, }, + SubnetName: subnetName, + DefaultGateway: defaultGateway, + SubnetAddressSpace: subnetAddressSpace, + Version: version, + }, + wantErr: false, + want: validSwiftRequest, + }, + { + name: "IP assignment is CIDR", + input: v1alpha.NetworkContainer{ + PrimaryIP: primaryIP, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: ipIsCIDR, + }, + }, + SubnetAddressSpace: subnetAddressSpace, }, wantErr: true, }, { - name: "malformed IP assignment", - input: v1alpha.NodeNetworkConfig{ - Status: v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: primaryIP, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: uuid, - IP: ipMalformed, - }, - }, - SubnetAddressSpace: subnetAddressSpace, - }, + name: "address space is not CIDR", + input: v1alpha.NetworkContainer{ + PrimaryIP: primaryIP, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: testSecIP, }, }, + SubnetAddressSpace: "10.0.0.0", // not a cidr range }, wantErr: true, }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + got, err := CreateNCRequestFromDynamicNC(tt.input) + if tt.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.EqualValues(t, tt.want, got) + }) + } +} + +func TestCreateNCRequestFromStaticNC(t *testing.T) { + tests := []struct { + name string + input v1alpha.NetworkContainer + want *cns.CreateNetworkContainerRequest + wantErr bool + }{ + { + name: "valid overlay", + input: validOverlayNC, + wantErr: false, + want: validOverlayRequest, + }, { - name: "IP is CIDR", - input: v1alpha.NodeNetworkConfig{ - Status: v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: ipIsCIDR, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: uuid, - IP: testSecIP, - }, - }, - SubnetName: subnetName, - DefaultGateway: defaultGateway, - SubnetAddressSpace: subnetAddressSpace, - Version: version, - }, + name: "malformed primary IP", + input: v1alpha.NetworkContainer{ + PrimaryIP: ipMalformed, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: testSecIP, }, - Scaler: v1alpha.Scaler{ - BatchSize: 1, + }, + SubnetAddressSpace: subnetAddressSpace, + }, + + wantErr: true, + }, + { + name: "malformed IP assignment", + input: v1alpha.NetworkContainer{ + PrimaryIP: primaryIP, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: ipMalformed, }, }, + SubnetAddressSpace: subnetAddressSpace, }, - wantErr: false, - want: validRequest, + wantErr: true, }, { name: "IP assignment is CIDR", - input: v1alpha.NodeNetworkConfig{ - Status: v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: primaryIP, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: uuid, - IP: ipIsCIDR, - }, - }, - SubnetAddressSpace: subnetAddressSpace, - }, + input: v1alpha.NetworkContainer{ + PrimaryIP: primaryIP, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: ipIsCIDR, }, }, + SubnetAddressSpace: subnetAddressSpace, }, wantErr: true, }, { name: "address space is not CIDR", - input: v1alpha.NodeNetworkConfig{ - Status: v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: primaryIP, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: uuid, - IP: testSecIP, - }, - }, - SubnetAddressSpace: "10.0.0.0", // not a cidr range - }, + input: v1alpha.NetworkContainer{ + PrimaryIP: primaryIP, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: testSecIP, }, }, + SubnetAddressSpace: "10.0.0.0", // not a cidr range }, wantErr: true, }, @@ -215,7 +295,7 @@ func TestConvertNNCStatusToNCRequest(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - got, err := CreateNCRequestFromDynamicNNC(tt.input) + got, err := CreateNCRequestFromStaticNC(tt.input) if tt.wantErr { assert.Error(t, err) return diff --git a/cns/singletenantcontroller/reconciler.go b/cns/singletenantcontroller/reconciler.go index ec68eff710..7ebbbdb308 100644 --- a/cns/singletenantcontroller/reconciler.go +++ b/cns/singletenantcontroller/reconciler.go @@ -4,7 +4,10 @@ import ( "context" "sync" + "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/logger" + "github.com/Azure/azure-container-networking/cns/restserver" + cnstypes "github.com/Azure/azure-container-networking/cns/types" "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" "github.com/pkg/errors" v1 "k8s.io/api/core/v1" @@ -18,8 +21,12 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) +type cnsClient interface { + CreateOrUpdateNetworkContainerInternal(*cns.CreateNetworkContainerRequest) cnstypes.ResponseCode +} + type nodeNetworkConfigListener interface { - Update(v1alpha.NodeNetworkConfig) error + Update(*v1alpha.NodeNetworkConfig) error } type nncGetter interface { @@ -28,26 +35,29 @@ type nncGetter interface { // Reconciler watches for CRD status changes type Reconciler struct { - nncListeners []nodeNetworkConfigListener - nnccli nncGetter - once sync.Once - started chan interface{} + cnscli cnsClient + ipampoolmonitorcli nodeNetworkConfigListener + nnccli nncGetter + once sync.Once + started chan interface{} } // NewReconciler creates a NodeNetworkConfig Reconciler which will get updates from the Kubernetes // apiserver for NNC events. // Provided nncListeners are passed the NNC after the Reconcile preprocesses it. Note: order matters! The // passed Listeners are notified in the order provided. -func NewReconciler(nnccli nncGetter, nncListeners ...nodeNetworkConfigListener) *Reconciler { +func NewReconciler(cnscli cnsClient, nnccli nncGetter, ipampoolmonitorcli nodeNetworkConfigListener) *Reconciler { return &Reconciler{ - nncListeners: nncListeners, - nnccli: nnccli, - started: make(chan interface{}), + cnscli: cnscli, + ipampoolmonitorcli: ipampoolmonitorcli, + nnccli: nnccli, + started: make(chan interface{}), } } // Reconcile is called on CRD status changes func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { + listenersToNotify := []nodeNetworkConfigListener{} nnc, err := r.nnccli.Get(ctx, req.NamespacedName) if err != nil { if apierrors.IsNotFound(err) { @@ -60,15 +70,44 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco logger.Printf("[cns-rc] CRD Spec: %v", nnc.Spec) - // If there are no network containers, don't continue to updating Listeners + // if there are no network containers, don't continue to updating Listeners if len(nnc.Status.NetworkContainers) == 0 { logger.Errorf("[cns-rc] Empty NetworkContainers") return reconcile.Result{}, nil } - // push the NNC to the registered NNC Sinks - for i := range r.nncListeners { - if err := r.nncListeners[i].Update(*nnc); err != nil { + ipAssignments := 0 + // for each NC, parse it in to a CreateNCRequest and forward it to the appropriate Listener + for i := range nnc.Status.NetworkContainers { + var req *cns.CreateNetworkContainerRequest + var err error + switch nnc.Status.NetworkContainers[i].AssignmentMode { + case v1alpha.Dynamic: + req, err = CreateNCRequestFromDynamicNC(nnc.Status.NetworkContainers[i]) + // in dynamic, we will also push this NNC to the IPAM Pool Monitor when we're done. + listenersToNotify = append(listenersToNotify, r.ipampoolmonitorcli) + case v1alpha.Static: + req, err = CreateNCRequestFromStaticNC(nnc.Status.NetworkContainers[i]) + default: + // unrecognized mode, fail out + err = errors.Errorf("unknown NetworkContainer AssignmentMode %s", string(nnc.Status.NetworkContainers[i].AssignmentMode)) + } + if err != nil { + return reconcile.Result{}, errors.Wrap(err, "failed to generate CreateNCRequest from NC") + } + responseCode := r.cnscli.CreateOrUpdateNetworkContainerInternal(req) + if err := restserver.ResponseCodeToError(responseCode); err != nil { + logger.Errorf("[cns-rc] Error creating or updating NC in reconcile: %v", err) + return reconcile.Result{}, errors.Wrap(err, "failed to create or update network container") + } + ipAssignments += len(req.SecondaryIPConfigs) + } + // record assigned IPs metric + allocatedIPs.Set(float64(ipAssignments)) + + // push the NNC to the registered NNC listeners. + for _, l := range listenersToNotify { + if err := l.Update(nnc); err != nil { return reconcile.Result{}, errors.Wrap(err, "nnc listener return error during update") } } diff --git a/cns/singletenantcontroller/reconciler_test.go b/cns/singletenantcontroller/reconciler_test.go index 368a990173..bb5619417f 100644 --- a/cns/singletenantcontroller/reconciler_test.go +++ b/cns/singletenantcontroller/reconciler_test.go @@ -19,23 +19,21 @@ import ( type cnsClientState struct { req *cns.CreateNetworkContainerRequest - nnc v1alpha.NodeNetworkConfig + nnc *v1alpha.NodeNetworkConfig } type mockCNSClient struct { state cnsClientState createOrUpdateNC func(*cns.CreateNetworkContainerRequest) cnstypes.ResponseCode - update func(v1alpha.NodeNetworkConfig) error + update func(*v1alpha.NodeNetworkConfig) error } -//nolint:gocritic // ignore hugeParam pls func (m *mockCNSClient) CreateOrUpdateNetworkContainerInternal(req *cns.CreateNetworkContainerRequest) cnstypes.ResponseCode { m.state.req = req return m.createOrUpdateNC(req) } -//nolint:gocritic //ignore hugeparam -func (m *mockCNSClient) Update(nnc v1alpha.NodeNetworkConfig) error { +func (m *mockCNSClient) Update(nnc *v1alpha.NodeNetworkConfig) error { m.state.nnc = nnc return m.update(nnc) } @@ -102,7 +100,7 @@ func TestReconcile(t *testing.T) { ncGetter: mockNCGetter{ get: func(context.Context, types.NamespacedName) (*v1alpha.NodeNetworkConfig, error) { return &v1alpha.NodeNetworkConfig{ - Status: validStatus, + Status: validSwiftStatus, }, nil }, }, @@ -113,7 +111,7 @@ func TestReconcile(t *testing.T) { }, wantErr: true, wantCNSClientState: cnsClientState{ - req: &validRequest, + req: validSwiftRequest, }, }, { @@ -121,7 +119,7 @@ func TestReconcile(t *testing.T) { ncGetter: mockNCGetter{ get: func(context.Context, types.NamespacedName) (*v1alpha.NodeNetworkConfig, error) { return &v1alpha.NodeNetworkConfig{ - Status: validStatus, + Status: validSwiftStatus, Spec: v1alpha.NodeNetworkConfigSpec{ RequestedIPCount: 1, }, @@ -132,15 +130,15 @@ func TestReconcile(t *testing.T) { createOrUpdateNC: func(*cns.CreateNetworkContainerRequest) cnstypes.ResponseCode { return cnstypes.Success }, - update: func(v1alpha.NodeNetworkConfig) error { + update: func(*v1alpha.NodeNetworkConfig) error { return nil }, }, wantErr: false, wantCNSClientState: cnsClientState{ - req: &validRequest, - nnc: v1alpha.NodeNetworkConfig{ - Status: validStatus, + req: validSwiftRequest, + nnc: &v1alpha.NodeNetworkConfig{ + Status: validSwiftStatus, Spec: v1alpha.NodeNetworkConfigSpec{ RequestedIPCount: 1, }, @@ -151,13 +149,13 @@ func TestReconcile(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - r := NewReconciler(&tt.ncGetter, SwiftNodeNetworkConfigListener(&tt.cnsClient), &tt.cnsClient) + r := NewReconciler(&tt.cnsClient, &tt.ncGetter, &tt.cnsClient) got, err := r.Reconcile(context.Background(), tt.in) if tt.wantErr { require.Error(t, err) - } else { - require.NoError(t, err) + return } + require.NoError(t, err) assert.Equal(t, tt.want, got) assert.Equal(t, tt.wantCNSClientState, tt.cnsClient.state) })