From c99b08ec05a5931482843cda268db3d297d84180 Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Tue, 7 Sep 2021 16:45:16 -0500 Subject: [PATCH 01/13] create nodenetworkconfig client Signed-off-by: Evan Baker --- crd/nodenetworkconfig/client.go | 78 ++++++++++++++++++++++++++--- crd/nodenetworkconfig/embed.go | 3 +- crd/nodenetworkconfig/embed_test.go | 15 +++--- crd/nodenetworkconfig/json.go | 19 +++++++ crd/nodenetworkconfig/json_test.go | 40 +++++++++++++++ 5 files changed, 141 insertions(+), 14 deletions(-) create mode 100644 crd/nodenetworkconfig/json.go create mode 100644 crd/nodenetworkconfig/json_test.go diff --git a/crd/nodenetworkconfig/client.go b/crd/nodenetworkconfig/client.go index e588d89652..1742373049 100644 --- a/crd/nodenetworkconfig/client.go +++ b/crd/nodenetworkconfig/client.go @@ -5,36 +5,66 @@ import ( "reflect" "github.com/Azure/azure-container-networking/crd" + "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" "github.com/pkg/errors" v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" typedv1 "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" + ctrlcli "sigs.k8s.io/controller-runtime/pkg/client" ) +// Scheme is a runtime scheme containing the client-go scheme and the NodeNetworkConfig scheme. +var Scheme = runtime.NewScheme() + +func init() { + _ = clientgoscheme.AddToScheme(Scheme) + _ = v1alpha.AddToScheme(Scheme) +} + +// Client is provided to interface with the NodeNetworkConfig CRDs. type Client struct { - crd typedv1.CustomResourceDefinitionInterface + nnccli ctrlcli.Client + crdcli typedv1.CustomResourceDefinitionInterface } -func NewClientWithConfig(c *rest.Config) (*Client, error) { +// NewClient creates a new NodeNetworkConfig client from the passed k8s Config. +func NewClient(c *rest.Config) (*Client, error) { crdCli, err := crd.NewCRDClient(c) + if err != nil { + return nil, errors.Wrap(err, "failed to init crd client") + } + opts := ctrlcli.Options{ + Scheme: Scheme, + } + nnnCli, err := ctrlcli.New(c, opts) if err != nil { return nil, errors.Wrap(err, "failed to init nnc client") } return &Client{ - crd: crdCli, + crdcli: crdCli, + nnccli: nnnCli, }, nil } func (c *Client) create(ctx context.Context, res *v1.CustomResourceDefinition) (*v1.CustomResourceDefinition, error) { - res, err := c.crd.Create(ctx, res, metav1.CreateOptions{}) + res, err := c.crdcli.Create(ctx, res, metav1.CreateOptions{}) if err != nil { return nil, errors.Wrap(err, "failed to create nnc crd") } return res, nil } +// Get returns the NodeNetworkConfig identified by the NamespacedName. +func (c *Client) Get(ctx context.Context, key types.NamespacedName) (*v1alpha.NodeNetworkConfig, error) { + nodeNetworkConfig := &v1alpha.NodeNetworkConfig{} + return nodeNetworkConfig, errors.Wrapf(c.nnccli.Get(ctx, key, nodeNetworkConfig), "failed to get nnc %v", key) +} + // Install installs the embedded NodeNetworkConfig CRD definition in the cluster. func (c *Client) Install(ctx context.Context) (*v1.CustomResourceDefinition, error) { nnc, err := GetNodeNetworkConfigs() @@ -44,6 +74,7 @@ func (c *Client) Install(ctx context.Context) (*v1.CustomResourceDefinition, err return c.create(ctx, nnc) } +// InstallOrUpdate installs the embedded NodeNetworkConfig CRD definition in the cluster or updates it if present. func (c *Client) InstallOrUpdate(ctx context.Context) (*v1.CustomResourceDefinition, error) { nnc, err := GetNodeNetworkConfigs() if err != nil { @@ -54,7 +85,7 @@ func (c *Client) InstallOrUpdate(ctx context.Context) (*v1.CustomResourceDefinit return current, err } if current == nil { - current, err = c.crd.Get(ctx, nnc.Name, metav1.GetOptions{}) + current, err = c.crdcli.Get(ctx, nnc.Name, metav1.GetOptions{}) if err != nil { return nil, errors.Wrap(err, "failed to get existing nnc crd") } @@ -62,10 +93,45 @@ func (c *Client) InstallOrUpdate(ctx context.Context) (*v1.CustomResourceDefinit if !reflect.DeepEqual(nnc.Spec.Versions, current.Spec.Versions) { nnc.SetResourceVersion(current.GetResourceVersion()) previous := *current - current, err = c.crd.Update(ctx, nnc, metav1.UpdateOptions{}) + current, err = c.crdcli.Update(ctx, nnc, metav1.UpdateOptions{}) if err != nil { return &previous, errors.Wrap(err, "failed to update existing nnc crd") } } return current, nil } + +// PatchSpec performs a server-side patch of the passed NodeNetworkConfigSpec to the NodeNetworkConfig specified by the NamespacedName. +func (c *Client) PatchSpec(ctx context.Context, key types.NamespacedName, spec *v1alpha.NodeNetworkConfigSpec) (*v1alpha.NodeNetworkConfig, error) { + obj := &v1alpha.NodeNetworkConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: key.Name, + Namespace: key.Namespace, + }, + } + + patch, err := specToJSON(spec) + if err != nil { + return nil, err + } + + if err := c.nnccli.Patch(ctx, obj, ctrlcli.RawPatch(types.ApplyPatchType, patch)); err != nil { + return nil, errors.Wrap(err, "failed to patch nnc") + } + + return obj, nil +} + +// UpdateSpec does a fetch, deepcopy, and update of the NodeNetworkConfig with the passed spec. +// Deprecated: UpdateSpec is deprecated and usage should migrate to PatchSpec. +func (c *Client) UpdateSpec(ctx context.Context, key types.NamespacedName, spec *v1alpha.NodeNetworkConfigSpec) (*v1alpha.NodeNetworkConfig, error) { + nnc, err := c.Get(ctx, key) + if err != nil { + return nil, errors.Wrap(err, "failed to update nnc") + } + spec.DeepCopyInto(&nnc.Spec) + if err := c.nnccli.Update(ctx, nnc); err != nil { + return nil, errors.Wrap(err, "failed to update nnc") + } + return nnc, nil +} diff --git a/crd/nodenetworkconfig/embed.go b/crd/nodenetworkconfig/embed.go index 0da7837287..c4132cfce6 100644 --- a/crd/nodenetworkconfig/embed.go +++ b/crd/nodenetworkconfig/embed.go @@ -5,6 +5,7 @@ import ( // import the manifests package so that caller of this package have the manifests compiled in as a side-effect. _ "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/manifests" + "github.com/pkg/errors" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "sigs.k8s.io/yaml" ) @@ -17,5 +18,5 @@ var NodeNetworkConfigsYAML []byte // to a CustomResourceDefinition and returns it or an unmarshalling error. func GetNodeNetworkConfigs() (*apiextensionsv1.CustomResourceDefinition, error) { nodeNetworkConfigs := &apiextensionsv1.CustomResourceDefinition{} - return nodeNetworkConfigs, yaml.Unmarshal(NodeNetworkConfigsYAML, &nodeNetworkConfigs) + return nodeNetworkConfigs, errors.Wrap(yaml.Unmarshal(NodeNetworkConfigsYAML, &nodeNetworkConfigs), "error unmarshalling embedded nnc") } diff --git a/crd/nodenetworkconfig/embed_test.go b/crd/nodenetworkconfig/embed_test.go index 0fda4d23e4..a8d26f17bf 100644 --- a/crd/nodenetworkconfig/embed_test.go +++ b/crd/nodenetworkconfig/embed_test.go @@ -2,19 +2,20 @@ package nodenetworkconfig import ( "os" - "reflect" "testing" + + "github.com/stretchr/testify/assert" ) const filename = "manifests/acn.azure.com_nodenetworkconfigs.yaml" func TestEmbed(t *testing.T) { b, err := os.ReadFile(filename) - if err != nil { - t.Error(err) - } + assert.NoError(t, err) + assert.Equal(t, b, NodeNetworkConfigsYAML) +} - if !reflect.DeepEqual(NodeNetworkConfigsYAML, b) { - t.Errorf("embedded file did not match file on disk") - } +func TestGetNodeNetworkConfigs(t *testing.T) { + _, err := GetNodeNetworkConfigs() + assert.NoError(t, err) } diff --git a/crd/nodenetworkconfig/json.go b/crd/nodenetworkconfig/json.go new file mode 100644 index 0000000000..30e8c262d3 --- /dev/null +++ b/crd/nodenetworkconfig/json.go @@ -0,0 +1,19 @@ +package nodenetworkconfig + +import ( + "encoding/json" + + "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" + "github.com/pkg/errors" +) + +func specToJSON(spec *v1alpha.NodeNetworkConfigSpec) ([]byte, error) { + m := map[string]*v1alpha.NodeNetworkConfigSpec{ + "spec": spec, + } + b, err := json.Marshal(m) + if err != nil { + return nil, errors.Wrap(err, "failed to marshal nnc spec") + } + return b, nil +} diff --git a/crd/nodenetworkconfig/json_test.go b/crd/nodenetworkconfig/json_test.go new file mode 100644 index 0000000000..ccf488018c --- /dev/null +++ b/crd/nodenetworkconfig/json_test.go @@ -0,0 +1,40 @@ +package nodenetworkconfig + +import ( + "reflect" + "testing" + + "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" +) + +func TestSpecToJSON(t *testing.T) { + tests := []struct { + name string + spec *v1alpha.NodeNetworkConfigSpec + want []byte + wantErr bool + }{ + { + name: "good", + spec: &v1alpha.NodeNetworkConfigSpec{ + RequestedIPCount: 13, + IPsNotInUse: []string{"abc", "def"}, + }, + want: []byte(`{"spec":{"requestedIPCount":13,"ipsNotInUse":["abc","def"]}}`), + wantErr: false, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + got, err := specToJSON(tt.spec) + if (err != nil) != tt.wantErr { + t.Errorf("specToJSON() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("specToJSON() = %s, want %s", got, tt.want) + } + }) + } +} From db8e3b47fd1e17662bc30feab339e7c0187cd03b Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Tue, 7 Sep 2021 16:45:55 -0500 Subject: [PATCH 02/13] add envnodename to config Signed-off-by: Evan Baker --- cns/configuration/env.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 cns/configuration/env.go diff --git a/cns/configuration/env.go b/cns/configuration/env.go new file mode 100644 index 0000000000..04844e2199 --- /dev/null +++ b/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 +} From 827b0da97a5f2cbe0bdcc21632c2bc0f04e12a3d Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Tue, 7 Sep 2021 16:48:08 -0500 Subject: [PATCH 03/13] rewrite and fully test nnc to nc conversion Signed-off-by: Evan Baker --- cns/singletenantcontroller/conversion.go | 67 +++++ cns/singletenantcontroller/conversion_test.go | 198 ++++++++++++++ .../kubecontroller/crdtranslator.go | 78 ------ .../kubecontroller/crdtranslator_test.go | 242 ------------------ 4 files changed, 265 insertions(+), 320 deletions(-) create mode 100644 cns/singletenantcontroller/conversion.go create mode 100644 cns/singletenantcontroller/conversion_test.go delete mode 100644 cns/singletenantcontroller/kubecontroller/crdtranslator.go delete mode 100644 cns/singletenantcontroller/kubecontroller/crdtranslator_test.go diff --git a/cns/singletenantcontroller/conversion.go b/cns/singletenantcontroller/conversion.go new file mode 100644 index 0000000000..35b2dd491a --- /dev/null +++ b/cns/singletenantcontroller/conversion.go @@ -0,0 +1,67 @@ +package kubecontroller + +import ( + "fmt" + "net" + "strconv" + + "github.com/Azure/azure-container-networking/cns" + "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" + "github.com/pkg/errors" +) + +var ErrUnsupportedNCQuantity = errors.New("unsupported number of network containers") + +// CRDStatusToNCRequest translates a crd status to createnetworkcontainer request +func CRDStatusToNCRequest(status v1alpha.NodeNetworkConfigStatus) (cns.CreateNetworkContainerRequest, error) { + // if NNC has no NC, return an empty request + if len(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)) + } + + nc := status.NetworkContainers[0] + + ip := net.ParseIP(nc.PrimaryIP) + if ip == nil { + return cns.CreateNetworkContainerRequest{}, fmt.Errorf("invalid PrimaryIP %s", nc.PrimaryIP) + } + + _, ipNet, err := net.ParseCIDR(nc.SubnetAddressSpace) + if err != nil { + return cns.CreateNetworkContainerRequest{}, errors.Wrapf(err, "invalid SubnetAddressSpace %s", nc.SubnetAddressSpace) + } + + size, _ := ipNet.Mask.Size() + + subnet := cns.IPSubnet{ + IPAddress: ip.String(), + PrefixLength: uint8(size), + } + + secondaryIPConfigs := map[string]cns.SecondaryIPConfig{} + for _, ipAssignment := range nc.IPAssignments { + secondaryIP := net.ParseIP(ipAssignment.IP) + if secondaryIP == nil { + return cns.CreateNetworkContainerRequest{}, fmt.Errorf("invalid SecondaryIP %s", ipAssignment.IP) + } + secondaryIPConfigs[ipAssignment.Name] = cns.SecondaryIPConfig{ + IPAddress: secondaryIP.String(), + NCVersion: int(nc.Version), + } + } + return cns.CreateNetworkContainerRequest{ + SecondaryIPConfigs: secondaryIPConfigs, + NetworkContainerid: nc.ID, + NetworkContainerType: cns.Docker, + Version: strconv.FormatInt(nc.Version, 10), + IPConfiguration: cns.IPConfiguration{ + IPSubnet: subnet, + GatewayIPAddress: nc.DefaultGateway, + }, + }, nil +} diff --git a/cns/singletenantcontroller/conversion_test.go b/cns/singletenantcontroller/conversion_test.go new file mode 100644 index 0000000000..2ec095ef12 --- /dev/null +++ b/cns/singletenantcontroller/conversion_test.go @@ -0,0 +1,198 @@ +package kubecontroller + +import ( + "reflect" + "strconv" + "testing" + + "github.com/Azure/azure-container-networking/cns" + "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" +) + +const ( + uuid = "539970a2-c2dd-11ea-b3de-0242ac130004" + defaultGateway = "10.0.0.2" + ipInCIDR = "10.0.0.1/32" + ipMalformed = "10.0.0.0.0" + ncID = "160005ba-cd02-11ea-87d0-0242ac130003" + primaryIP = "10.0.0.1" + subnetAddressSpace = "10.0.0.0/24" + subnetName = "subnet1" + subnetPrefixLen = 24 + testSecIP = "10.0.0.2" + version = 1 +) + +func TestConvertNNCStatusToNCRequest(t *testing.T) { + tests := []struct { + name string + status v1alpha.NodeNetworkConfigStatus + ncreq cns.CreateNetworkContainerRequest + wantErr bool + }{ + { + name: "no nc", + status: v1alpha.NodeNetworkConfigStatus{}, + wantErr: false, + ncreq: cns.CreateNetworkContainerRequest{}, + }, + { + name: ">1 nc", + status: v1alpha.NodeNetworkConfigStatus{ + NetworkContainers: []v1alpha.NetworkContainer{ + {}, + {}, + }, + }, + wantErr: true, + }, + { + name: "malformed primary IP", + status: v1alpha.NodeNetworkConfigStatus{ + NetworkContainers: []v1alpha.NetworkContainer{ + { + PrimaryIP: ipMalformed, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: testSecIP, + }, + }, + SubnetAddressSpace: subnetAddressSpace, + }, + }, + }, + wantErr: true, + }, + { + name: "malformed IP assignment", + status: v1alpha.NodeNetworkConfigStatus{ + NetworkContainers: []v1alpha.NetworkContainer{ + { + PrimaryIP: primaryIP, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: ipMalformed, + }, + }, + SubnetAddressSpace: subnetAddressSpace, + }, + }, + }, + wantErr: true, + }, + { + name: "IP is CIDR", + status: v1alpha.NodeNetworkConfigStatus{ + NetworkContainers: []v1alpha.NetworkContainer{ + { + PrimaryIP: ipInCIDR, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: testSecIP, + }, + }, + SubnetAddressSpace: subnetAddressSpace, + }, + }, + }, + wantErr: true, + }, + { + name: "IP assignment is CIDR", + status: v1alpha.NodeNetworkConfigStatus{ + NetworkContainers: []v1alpha.NetworkContainer{ + { + PrimaryIP: primaryIP, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: ipInCIDR, + }, + }, + SubnetAddressSpace: subnetAddressSpace, + }, + }, + }, + wantErr: true, + }, + { + name: "address space is not CIDR", + 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 + }, + }, + }, + wantErr: true, + }, + { + name: "valid", + status: v1alpha.NodeNetworkConfigStatus{ + NetworkContainers: []v1alpha.NetworkContainer{ + { + PrimaryIP: primaryIP, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: testSecIP, + }, + }, + SubnetName: subnetName, + DefaultGateway: defaultGateway, + SubnetAddressSpace: subnetAddressSpace, + Version: version, + }, + }, + }, + wantErr: false, + ncreq: cns.CreateNetworkContainerRequest{ + Version: strconv.FormatInt(version, 10), + IPConfiguration: cns.IPConfiguration{ + GatewayIPAddress: defaultGateway, + IPSubnet: cns.IPSubnet{ + PrefixLength: uint8(subnetPrefixLen), + IPAddress: primaryIP, + }, + }, + NetworkContainerid: ncID, + NetworkContainerType: cns.Docker, + SecondaryIPConfigs: map[string]cns.SecondaryIPConfig{ + uuid: { + IPAddress: testSecIP, + NCVersion: version, + }, + }, + }, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + got, err := CRDStatusToNCRequest(tt.status) + if (err != nil) != tt.wantErr { + t.Errorf("ConvertNNCStatusToNCRequest() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.ncreq) { + t.Errorf("ConvertNNCStatusToNCRequest()\nhave: %+v\n want: %+v", got, tt.ncreq) + } + }) + } +} diff --git a/cns/singletenantcontroller/kubecontroller/crdtranslator.go b/cns/singletenantcontroller/kubecontroller/crdtranslator.go deleted file mode 100644 index 82c9e4c9e6..0000000000 --- a/cns/singletenantcontroller/kubecontroller/crdtranslator.go +++ /dev/null @@ -1,78 +0,0 @@ -package kubecontroller - -import ( - "fmt" - "net" - "strconv" - - "github.com/Azure/azure-container-networking/cns" - "github.com/Azure/azure-container-networking/cns/logger" - "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" -) - -// CRDStatusToNCRequest translates a crd status to createnetworkcontainer request -func CRDStatusToNCRequest(crdStatus v1alpha.NodeNetworkConfigStatus) (cns.CreateNetworkContainerRequest, error) { - var ( - ncRequest cns.CreateNetworkContainerRequest - nc v1alpha.NetworkContainer - secondaryIPConfig cns.SecondaryIPConfig - ipSubnet cns.IPSubnet - ipAssignment v1alpha.IPAssignment - err error - ip net.IP - ipNet *net.IPNet - size int - numNCsSupported int - numNCs int - ) - - numNCsSupported = 1 - numNCs = len(crdStatus.NetworkContainers) - - // Right now we're only supporing one NC per node, but in the future we will support multiple NCs per node - if numNCs > numNCsSupported { - return ncRequest, fmt.Errorf("Number of network containers is not supported. Got %v number of ncs, supports %v", numNCs, numNCsSupported) - } - - for _, nc = range crdStatus.NetworkContainers { - ncRequest.SecondaryIPConfigs = make(map[string]cns.SecondaryIPConfig) - ncRequest.NetworkContainerid = nc.ID - ncRequest.NetworkContainerType = cns.Docker - ncRequest.Version = strconv.FormatInt(nc.Version, 10) - - if ip = net.ParseIP(nc.PrimaryIP); ip == nil { - return ncRequest, fmt.Errorf("Invalid PrimaryIP %s:", nc.PrimaryIP) - } - - if _, ipNet, err = net.ParseCIDR(nc.SubnetAddressSpace); err != nil { - return ncRequest, fmt.Errorf("Invalid SubnetAddressSpace %s:, err:%s", nc.SubnetAddressSpace, err) - } - - size, _ = ipNet.Mask.Size() - ipSubnet.IPAddress = ip.String() - ipSubnet.PrefixLength = uint8(size) - ncRequest.IPConfiguration.IPSubnet = ipSubnet - ncRequest.IPConfiguration.GatewayIPAddress = nc.DefaultGateway - var ncVersion int - if ncVersion, err = strconv.Atoi(ncRequest.Version); err != nil { - return ncRequest, fmt.Errorf("Invalid ncRequest.Version is %s in CRD, err:%s", ncRequest.Version, err) - } - - for _, ipAssignment = range nc.IPAssignments { - if ip = net.ParseIP(ipAssignment.IP); ip == nil { - return ncRequest, fmt.Errorf("Invalid SecondaryIP %s:", ipAssignment.IP) - } - secondaryIPConfig = cns.SecondaryIPConfig{ - IPAddress: ip.String(), - NCVersion: ncVersion, - } - ncRequest.SecondaryIPConfigs[ipAssignment.Name] = secondaryIPConfig - logger.Debugf("Seconday IP Configs got set, name is %s, config is %v", ipAssignment.Name, secondaryIPConfig) - } - logger.Printf("Set NC request info with NetworkContainerid %s, NetworkContainerType %s, NC Version %s", - ncRequest.NetworkContainerid, ncRequest.NetworkContainerType, ncRequest.Version) - } - - // Only returning the first network container for now, later we will return a list - return ncRequest, nil -} diff --git a/cns/singletenantcontroller/kubecontroller/crdtranslator_test.go b/cns/singletenantcontroller/kubecontroller/crdtranslator_test.go deleted file mode 100644 index 62b7075965..0000000000 --- a/cns/singletenantcontroller/kubecontroller/crdtranslator_test.go +++ /dev/null @@ -1,242 +0,0 @@ -package kubecontroller - -import ( - "testing" - - "github.com/Azure/azure-container-networking/cns" - "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" -) - -const ( - ncID = "160005ba-cd02-11ea-87d0-0242ac130003" - primaryIp = "10.0.0.1" - ipInCIDR = "10.0.0.1/32" - ipMalformed = "10.0.0.0.0" - defaultGateway = "10.0.0.2" - subnetName = "subnet1" - subnetAddressSpace = "10.0.0.0/24" - subnetPrefixLen = 24 - testSecIp1 = "10.0.0.2" - version = 1 -) - -func TestStatusToNCRequestMalformedPrimaryIP(t *testing.T) { - var ( - status v1alpha.NodeNetworkConfigStatus - err error - ) - - status = v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: ipMalformed, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: allocatedUUID, - IP: testSecIp1, - }, - }, - SubnetAddressSpace: subnetAddressSpace, - }, - }, - } - - // Test with malformed primary ip - _, err = CRDStatusToNCRequest(status) - - if err == nil { - t.Fatalf("Expected translation of CRD status with malformed ip to fail.") - } -} - -func TestStatusToNCRequestMalformedIPAssignment(t *testing.T) { - var ( - status v1alpha.NodeNetworkConfigStatus - err error - ) - - status = v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: primaryIp, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: allocatedUUID, - IP: ipMalformed, - }, - }, - SubnetAddressSpace: subnetAddressSpace, - }, - }, - } - - // Test with malformed ip assignment - _, err = CRDStatusToNCRequest(status) - - if err == nil { - t.Fatalf("Expected translation of CRD status with malformed ip assignment to fail.") - } -} - -func TestStatusToNCRequestPrimaryIPInCIDR(t *testing.T) { - var ( - status v1alpha.NodeNetworkConfigStatus - err error - ) - - status = v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: ipInCIDR, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: allocatedUUID, - IP: testSecIp1, - }, - }, - SubnetAddressSpace: subnetAddressSpace, - }, - }, - } - - // Test with primary ip not in CIDR form - _, err = CRDStatusToNCRequest(status) - - if err == nil { - t.Fatalf("Expected translation of CRD status with primary ip not CIDR, to fail.") - } -} - -func TestStatusToNCRequestIPAssignmentNotCIDR(t *testing.T) { - var ( - status v1alpha.NodeNetworkConfigStatus - err error - ) - - status = v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: primaryIp, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: allocatedUUID, - IP: ipInCIDR, - }, - }, - SubnetAddressSpace: subnetAddressSpace, - }, - }, - } - - // Test with ip assignment not in CIDR form - _, err = CRDStatusToNCRequest(status) - - if err == nil { - t.Fatalf("Expected translation of CRD status with ip assignment not CIDR, to fail.") - } -} - -func TestStatusToNCRequestWithIncorrectSubnetAddressSpace(t *testing.T) { - var ( - status v1alpha.NodeNetworkConfigStatus - err error - ) - - status = v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: primaryIp, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: allocatedUUID, - IP: testSecIp1, - }, - }, - SubnetAddressSpace: "10.0.0.0", // not a cidr range - }, - }, - } - - // Test with ip assignment not in CIDR form - _, err = CRDStatusToNCRequest(status) - - if err == nil { - t.Fatalf("Expected translation of CRD status with ip assignment not CIDR, to fail.") - } -} - -func TestStatusToNCRequestSuccess(t *testing.T) { - var ( - status v1alpha.NodeNetworkConfigStatus - ncRequest cns.CreateNetworkContainerRequest - secondaryIPs map[string]cns.SecondaryIPConfig - secondaryIP cns.SecondaryIPConfig - ok bool - err error - ) - - status = v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: primaryIp, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: allocatedUUID, - IP: testSecIp1, - }, - }, - SubnetName: subnetName, - DefaultGateway: defaultGateway, - SubnetAddressSpace: subnetAddressSpace, - Version: version, - }, - }, - } - - // Test with ips formed correctly as CIDRs - ncRequest, err = CRDStatusToNCRequest(status) - - if err != nil { - t.Fatalf("Expected translation of CRD status to succeed, got error :%v", err) - } - - if ncRequest.IPConfiguration.IPSubnet.IPAddress != primaryIp { - t.Fatalf("Expected ncRequest's ipconfiguration to have the ip %v but got %v", primaryIp, ncRequest.IPConfiguration.IPSubnet.IPAddress) - } - - if ncRequest.IPConfiguration.IPSubnet.PrefixLength != uint8(subnetPrefixLen) { - t.Fatalf("Expected ncRequest's ipconfiguration prefix length to be %v but got %v", subnetPrefixLen, ncRequest.IPConfiguration.IPSubnet.PrefixLength) - } - - if ncRequest.IPConfiguration.GatewayIPAddress != defaultGateway { - t.Fatalf("Expected ncRequest's ipconfiguration gateway to be %s but got %s", defaultGateway, ncRequest.IPConfiguration.GatewayIPAddress) - } - - if ncRequest.NetworkContainerid != ncID { - t.Fatalf("Expected ncRequest's network container id to equal %v but got %v", ncID, ncRequest.NetworkContainerid) - } - - if ncRequest.NetworkContainerType != cns.Docker { - t.Fatalf("Expected ncRequest's network container type to be %v but got %v", cns.Docker, ncRequest.NetworkContainerType) - } - - secondaryIPs = ncRequest.SecondaryIPConfigs - - if secondaryIP, ok = secondaryIPs[allocatedUUID]; !ok { - t.Fatalf("Expected there to be a secondary ip with the key %v but found nothing", allocatedUUID) - } - - if secondaryIP.IPAddress != testSecIp1 { - t.Fatalf("Expected %v as the secondary IP config but got %v", testSecIp1, secondaryIP.IPAddress) - } - - if secondaryIP.NCVersion != version { - t.Fatalf("Expected %d as the secondary IP config NC version but got %v", version, secondaryIP.NCVersion) - } -} From ec9c7305e1f5f3730725acdfe5c83be64009f3d6 Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Tue, 7 Sep 2021 16:51:03 -0500 Subject: [PATCH 04/13] use nnc clients to separate reconciler and ipampoolmonitor Signed-off-by: Evan Baker --- cns/fakes/requestcontrollerfake.go | 36 ++--- cns/ipampoolmonitor/ipampoolmonitor.go | 17 ++- cns/ipampoolmonitor/ipampoolmonitor_test.go | 11 +- cns/service/main.go | 131 +++++++++++++----- cns/singletenantcontroller/cnsclient.go | 13 ++ .../{kubecontroller => }/eventfilter.go | 0 .../kubecontroller/directapiclient.go | 52 ------- .../kubecontroller/directcrdclient.go | 54 -------- .../kubecontroller/kubeclientinterface.go | 25 ---- .../{kubecontroller => }/metrics.go | 0 .../crdreconciler.go => reconciler.go} | 41 +++--- .../requestcontrollerinterface.go | 15 -- cns/singletenantcontroller/scopedclient.go | 33 +++++ 13 files changed, 202 insertions(+), 226 deletions(-) create mode 100644 cns/singletenantcontroller/cnsclient.go rename cns/singletenantcontroller/{kubecontroller => }/eventfilter.go (100%) delete mode 100644 cns/singletenantcontroller/kubecontroller/directapiclient.go delete mode 100644 cns/singletenantcontroller/kubecontroller/directcrdclient.go delete mode 100644 cns/singletenantcontroller/kubecontroller/kubeclientinterface.go rename cns/singletenantcontroller/{kubecontroller => }/metrics.go (100%) rename cns/singletenantcontroller/{kubecontroller/crdreconciler.go => reconciler.go} (68%) delete mode 100644 cns/singletenantcontroller/requestcontrollerinterface.go create mode 100644 cns/singletenantcontroller/scopedclient.go diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index bacc369dfb..ab7e3870d1 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/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 } diff --git a/cns/ipampoolmonitor/ipampoolmonitor.go b/cns/ipampoolmonitor/ipampoolmonitor.go index 7c48ba0707..54ad7a998f 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor.go +++ b/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 diff --git a/cns/ipampoolmonitor/ipampoolmonitor_test.go b/cns/ipampoolmonitor/ipampoolmonitor_test.go index 60b91b500b..cf39ecf5c1 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor_test.go +++ b/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 diff --git a/cns/service/main.go b/cns/service/main.go index 83fa7070ef..21daf0553f 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -21,6 +21,7 @@ import ( "github.com/Azure/azure-container-networking/cnm/ipam" "github.com/Azure/azure-container-networking/cnm/network" "github.com/Azure/azure-container-networking/cns" + "github.com/Azure/azure-container-networking/cns/client/httpapi" cnscli "github.com/Azure/azure-container-networking/cns/cmd/cli" "github.com/Azure/azure-container-networking/cns/cnireconciler" cni "github.com/Azure/azure-container-networking/cns/cnireconciler" @@ -34,14 +35,19 @@ import ( "github.com/Azure/azure-container-networking/cns/multitenantcontroller/multitenantoperator" "github.com/Azure/azure-container-networking/cns/nmagentclient" "github.com/Azure/azure-container-networking/cns/restserver" - "github.com/Azure/azure-container-networking/cns/singletenantcontroller" - "github.com/Azure/azure-container-networking/cns/singletenantcontroller/kubecontroller" + kubecontroller "github.com/Azure/azure-container-networking/cns/singletenantcontroller" acn "github.com/Azure/azure-container-networking/common" + "github.com/Azure/azure-container-networking/crd" + "github.com/Azure/azure-container-networking/crd/nodenetworkconfig" + "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" "github.com/Azure/azure-container-networking/log" "github.com/Azure/azure-container-networking/platform" localtls "github.com/Azure/azure-container-networking/server/tls" "github.com/Azure/azure-container-networking/store" + "github.com/pkg/errors" + "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" ) const ( @@ -773,18 +779,75 @@ func InitializeMultiTenantController(ctx context.Context, httpRestService cns.HT return nil } -// initializeCRD state -func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cnsconfig configuration.CNSConfig) error { - var requestController singletenantcontroller.RequestController +type nodeNetworkConfigGetter interface { + Get(context.Context) (*v1alpha.NodeNetworkConfig, error) +} - logger.Printf("[Azure CNS] Starting request controller") +type ncStateReconciler interface { + ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo, scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) error +} - kubeConfig, err := kubecontroller.GetKubeConfig() +// TODO(rbtr) where should this live?? +// InitCNS initializes cns by passing pods and a createnetworkcontainerrequest +func initCNS(ctx context.Context, cli nodeNetworkConfigGetter, ncReconciler ncStateReconciler) error { + // Get nnc using direct client + nnc, err := cli.Get(ctx) if err != nil { - logger.Errorf("[Azure CNS] Failed to get kubeconfig for request controller: %v", err) + // If the CRD is not defined, exit + if crd.IsNotDefined(err) { + logger.Errorf("CRD is not defined on cluster: %v", err) + os.Exit(1) + } + + if nnc == nil { + logger.Errorf("NodeNetworkConfig is not present on cluster") + return nil + } + + // If instance of crd is not found, pass nil to CNSClient + if client.IgnoreNotFound(err) == nil { + //nolint:wrapcheck + return ncReconciler.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec) + } + + // If it's any other error, log it and return + logger.Errorf("Error when getting nodeNetConfig using direct client when initializing cns state: %v", err) + return err + } + + // If there are no NCs, pass nil to CNSClient + if len(nnc.Status.NetworkContainers) == 0 { + //nolint:wrapcheck + return ncReconciler.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec) + } + + // Convert to CreateNetworkContainerRequest + ncRequest, err := kubecontroller.CRDStatusToNCRequest(nnc.Status) + if err != nil { + logger.Errorf("Error when converting nodeNetConfig status into CreateNetworkContainerRequest: %v", err) return err } + // rebuild CNS state from CNI + logger.Printf("initializing CNS from CNI") + podInfoByIPProvider, err := cnireconciler.NewCNIPodInfoProvider() + if err != nil { + return err + } + podInfoByIP, err := podInfoByIPProvider.PodInfoByIP() + if err != nil { + return errors.Wrap(err, "err in CNS initialization") + } + + // errors.Wrap provides additional context, and return nil if the err input arg is nil + // Call cnsclient init cns passing those two things. + return errors.Wrap(ncReconciler.ReconcileNCState(&ncRequest, podInfoByIP, nnc.Status.Scaler, nnc.Spec), "err in CNS reconciliation") +} + +// initializeCRD state +func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cnsconfig configuration.CNSConfig) error { + logger.Printf("[Azure CNS] Starting request controller") + // convert interface type to implementation type httpRestServiceImplementation, ok := httpRestService.(*restserver.HTTPRestService) if !ok { @@ -799,32 +862,48 @@ func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cn } httpRestServiceImplementation.SetNodeOrchestrator(&orchestrator) - // Get crd implementation of request controller - requestController, err = kubecontroller.New( - kubecontroller.Config{ - InitializeFromCNI: cnsconfig.InitializeFromCNI, - KubeConfig: kubeConfig, - MetricsBindAddress: cnsconfig.MetricsBindAddress, - Service: httpRestServiceImplementation, - }) + kubeConfig, err := ctrl.GetConfig() + if err != nil { + logger.Errorf("[Azure CNS] Failed to get kubeconfig for request controller: %v", err) + return err + } + nnccli, err := nodenetworkconfig.NewClient(kubeConfig) if err != nil { - logger.Errorf("[Azure CNS] Failed to make crd request controller :%v", err) return err } + nodeName, err := configuration.NodeName() + if err != nil { + return err + } + // TODO(rbtr): nodename and namespace should be in the cns config + scopedcli := kubecontroller.NewScopedClient(nnccli, types.NamespacedName{Namespace: "kube-system", Name: nodeName}) // initialize the ipam pool monitor - httpRestServiceImplementation.IPAMPoolMonitor = ipampoolmonitor.NewCNSIPAMPoolMonitor(httpRestServiceImplementation, requestController) + httpRestServiceImplementation.IPAMPoolMonitor = ipampoolmonitor.NewCNSIPAMPoolMonitor(httpRestServiceImplementation, scopedcli) + cnscli := &httpapi.Client{ + RestService: httpRestServiceImplementation, + } + if err := initCNS(ctx, scopedcli, cnscli); err != nil { + return errors.Wrap(err, "failed to initialize CNS state") + } - err = requestController.Init(ctx) + manager, err := ctrl.NewManager(kubeConfig, ctrl.Options{ + Scheme: nodenetworkconfig.Scheme, + MetricsBindAddress: cnsconfig.MetricsBindAddress, + Namespace: "kube-system", // TODO(rbtr): namespace should be in the cns config + }) if err != nil { - logger.Errorf("[Azure CNS] Failed to initialized cns state :%v", err) + return err + } + reconciler := kubecontroller.New(nnccli, cnscli) + if err := reconciler.SetupWithManager(manager, nodeName); err != nil { return err } // Start the RequestController which starts the reconcile loop go func() { for { - if err := requestController.Start(ctx); err != nil { + if err := manager.Start(ctx); err != nil { logger.Errorf("[Azure CNS] Failed to start request controller: %v", err) // retry to start the request controller // todo: add a CNS metric to count # of failures @@ -838,16 +917,6 @@ func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cn } }() - for { - if requestController.IsStarted() { - logger.Printf("RequestController is started") - break - } - - logger.Printf("Waiting for requestController to start...") - time.Sleep(time.Millisecond * 500) - } - logger.Printf("Starting IPAM Pool Monitor") go func() { for { diff --git a/cns/singletenantcontroller/cnsclient.go b/cns/singletenantcontroller/cnsclient.go new file mode 100644 index 0000000000..f377568477 --- /dev/null +++ b/cns/singletenantcontroller/cnsclient.go @@ -0,0 +1,13 @@ +package kubecontroller + +import ( + "github.com/Azure/azure-container-networking/cns" + "github.com/Azure/azure-container-networking/cns/types" + "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" +) + +type cnsclient interface { + ReconcileNCState(*cns.CreateNetworkContainerRequest, map[string]cns.PodInfo, v1alpha.Scaler, v1alpha.NodeNetworkConfigSpec) types.ResponseCode + CreateOrUpdateNetworkContainerInternal(*cns.CreateNetworkContainerRequest) types.ResponseCode + UpdateIPAMPoolMonitor(scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) +} diff --git a/cns/singletenantcontroller/kubecontroller/eventfilter.go b/cns/singletenantcontroller/eventfilter.go similarity index 100% rename from cns/singletenantcontroller/kubecontroller/eventfilter.go rename to cns/singletenantcontroller/eventfilter.go diff --git a/cns/singletenantcontroller/kubecontroller/directapiclient.go b/cns/singletenantcontroller/kubecontroller/directapiclient.go deleted file mode 100644 index 0863f56b0f..0000000000 --- a/cns/singletenantcontroller/kubecontroller/directapiclient.go +++ /dev/null @@ -1,52 +0,0 @@ -package kubecontroller - -import ( - "context" - - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/kubernetes" - "k8s.io/client-go/rest" -) - -// APIDirectClient implements DirectAPIClient interface -var _ DirectAPIClient = &APIDirectClient{} - -// APIDirectClient is a direct client to a kubernetes API server -type APIDirectClient struct { - clientset *kubernetes.Clientset -} - -// ListPods lists all pods in the given namespace and node -func (apiClient *APIDirectClient) ListPods(ctx context.Context, namespace, node string) (*corev1.PodList, error) { - var ( - pods *corev1.PodList - err error - ) - - pods, err = apiClient.clientset.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{ - FieldSelector: "spec.nodeName=" + node, - }) - - if err != nil { - return nil, err - } - - return pods, nil -} - -// NewAPIDirectClient creates a new APIDirectClient -func NewAPIDirectClient(kubeconfig *rest.Config) (*APIDirectClient, error) { - var ( - clientset *kubernetes.Clientset - err error - ) - - if clientset, err = kubernetes.NewForConfig(kubeconfig); err != nil { - return nil, err - } - - return &APIDirectClient{ - clientset: clientset, - }, nil -} diff --git a/cns/singletenantcontroller/kubecontroller/directcrdclient.go b/cns/singletenantcontroller/kubecontroller/directcrdclient.go deleted file mode 100644 index a2b2dd7344..0000000000 --- a/cns/singletenantcontroller/kubecontroller/directcrdclient.go +++ /dev/null @@ -1,54 +0,0 @@ -package kubecontroller - -import ( - "context" - - "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" - "k8s.io/apimachinery/pkg/runtime/schema" - clientgoscheme "k8s.io/client-go/kubernetes/scheme" - "k8s.io/client-go/rest" -) - -// Implements DirectCRDClient interface -var _ DirectCRDClient = &CRDDirectClient{} - -// CRDDirectClient is a direct client to CRDs in the API Server. -type CRDDirectClient struct { - restClient *rest.RESTClient -} - -// Get gets a crd -func (crdClient *CRDDirectClient) Get(ctx context.Context, name, namespace, typeName string) (*v1alpha.NodeNetworkConfig, error) { - var ( - nodeNetConfig *v1alpha.NodeNetworkConfig - err error - ) - - nodeNetConfig = &v1alpha.NodeNetworkConfig{} - if err = crdClient.restClient.Get().Namespace(namespace).Resource(crdTypeName).Name(name).Do(ctx).Into(nodeNetConfig); err != nil { - return nil, err - } - - return nodeNetConfig, nil -} - -// NewCRDDirectClient creates a new direct crd client to the api server -func NewCRDDirectClient(kubeconfig *rest.Config, groupVersion *schema.GroupVersion) (*CRDDirectClient, error) { - var ( - config rest.Config - restClient *rest.RESTClient - err error - ) - - config = *kubeconfig - config.GroupVersion = groupVersion - config.APIPath = "/apis" - config.NegotiatedSerializer = clientgoscheme.Codecs.WithoutConversion() - if restClient, err = rest.RESTClientFor(&config); err != nil { - return nil, err - } - - return &CRDDirectClient{ - restClient: restClient, - }, nil -} diff --git a/cns/singletenantcontroller/kubecontroller/kubeclientinterface.go b/cns/singletenantcontroller/kubecontroller/kubeclientinterface.go deleted file mode 100644 index 798a96c2fc..0000000000 --- a/cns/singletenantcontroller/kubecontroller/kubeclientinterface.go +++ /dev/null @@ -1,25 +0,0 @@ -package kubecontroller - -import ( - "context" - - "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" - corev1 "k8s.io/api/core/v1" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -// KubeClient is an interface that talks to the API server -type KubeClient interface { - Get(ctx context.Context, key client.ObjectKey, obj client.Object) error - Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error -} - -// DirectCRDClient is an interface to get CRDs directly, without cache -type DirectCRDClient interface { - Get(ctx context.Context, name, namespace, typeName string) (*v1alpha.NodeNetworkConfig, error) -} - -// DirectAPIClient is an interface to talk directly with API Server without cache -type DirectAPIClient interface { - ListPods(ctx context.Context, namespace, node string) (*corev1.PodList, error) -} diff --git a/cns/singletenantcontroller/kubecontroller/metrics.go b/cns/singletenantcontroller/metrics.go similarity index 100% rename from cns/singletenantcontroller/kubecontroller/metrics.go rename to cns/singletenantcontroller/metrics.go diff --git a/cns/singletenantcontroller/kubecontroller/crdreconciler.go b/cns/singletenantcontroller/reconciler.go similarity index 68% rename from cns/singletenantcontroller/kubecontroller/crdreconciler.go rename to cns/singletenantcontroller/reconciler.go index 9981355fc1..d4a56dad09 100644 --- a/cns/singletenantcontroller/kubecontroller/crdreconciler.go +++ b/cns/singletenantcontroller/reconciler.go @@ -3,9 +3,9 @@ package kubecontroller import ( "context" - "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/logger" "github.com/Azure/azure-container-networking/cns/restserver" + "github.com/Azure/azure-container-networking/crd/nodenetworkconfig" "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" apierrors "k8s.io/apimachinery/pkg/api/errors" ctrl "sigs.k8s.io/controller-runtime" @@ -13,26 +13,29 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -// CrdReconciler watches for CRD status changes -type CrdReconciler struct { - KubeClient KubeClient - NodeName string - CNSRestService *restserver.HTTPRestService - IPAMPoolMonitor cns.IPAMPoolMonitor +// Reconciler watches for CRD status changes +type Reconciler struct { + cnscli cnsclient + nnccli *nodenetworkconfig.Client +} + +func New(nnccli *nodenetworkconfig.Client, cnscli cnsclient) *Reconciler { + return &Reconciler{ + cnscli: cnscli, + nnccli: nnccli, + } } // Reconcile is called on CRD status changes -func (r *CrdReconciler) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { - // Get the CRD object - var nnc v1alpha.NodeNetworkConfig - if err := r.KubeClient.Get(ctx, request.NamespacedName, &nnc); err != nil { +func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { + nnc, err := r.nnccli.Get(ctx, request.NamespacedName) + if err != nil { if apierrors.IsNotFound(err) { logger.Printf("[cns-rc] CRD not found, ignoring %v", err) return reconcile.Result{}, client.IgnoreNotFound(err) - } else { - logger.Errorf("[cns-rc] Error retrieving CRD from cache : %v", err) - return reconcile.Result{}, err } + logger.Errorf("[cns-rc] Error retrieving CRD from cache : %v", err) + return reconcile.Result{}, err } logger.Printf("[cns-rc] CRD Spec: %v", nnc.Spec) @@ -62,7 +65,7 @@ func (r *CrdReconciler) Reconcile(ctx context.Context, request reconcile.Request return reconcile.Result{}, err } - responseCode := r.CNSRestService.CreateOrUpdateNetworkContainerInternal(&ncRequest) + responseCode := r.cnscli.CreateOrUpdateNetworkContainerInternal(&ncRequest) err = restserver.ResponseCodeToError(responseCode) if err != nil { logger.Errorf("[cns-rc] Error creating or updating NC in reconcile: %v", err) @@ -70,17 +73,17 @@ func (r *CrdReconciler) Reconcile(ctx context.Context, request reconcile.Request return reconcile.Result{}, err } - r.CNSRestService.IPAMPoolMonitor.Update(nnc.Status.Scaler, nnc.Spec) + r.cnscli.UpdateIPAMPoolMonitor(nnc.Status.Scaler, nnc.Spec) // record assigned IPs metric assignedIPs.Set(float64(len(nnc.Status.NetworkContainers[0].IPAssignments))) return reconcile.Result{}, nil } -// SetupWithManager Sets up the reconciler with a new manager, filtering using NodeNetworkConfigFilter -func (r *CrdReconciler) SetupWithManager(mgr ctrl.Manager) error { +// SetupWithManager Sets up the reconciler with a new manager, filtering using NodeNetworkConfigFilter on nodeName. +func (r *Reconciler) SetupWithManager(mgr ctrl.Manager, nodeName string) error { return ctrl.NewControllerManagedBy(mgr). For(&v1alpha.NodeNetworkConfig{}). - WithEventFilter(NodeNetworkConfigFilter{nodeName: r.NodeName}). + WithEventFilter(NodeNetworkConfigFilter{nodeName: nodeName}). Complete(r) } diff --git a/cns/singletenantcontroller/requestcontrollerinterface.go b/cns/singletenantcontroller/requestcontrollerinterface.go deleted file mode 100644 index 4d9d8b5871..0000000000 --- a/cns/singletenantcontroller/requestcontrollerinterface.go +++ /dev/null @@ -1,15 +0,0 @@ -package singletenantcontroller - -import ( - "context" - - "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" -) - -// RequestController interface for cns to interact with the request controller -type RequestController interface { - Init(context.Context) error - Start(context.Context) error - UpdateCRDSpec(context.Context, v1alpha.NodeNetworkConfigSpec) error - IsStarted() bool -} diff --git a/cns/singletenantcontroller/scopedclient.go b/cns/singletenantcontroller/scopedclient.go new file mode 100644 index 0000000000..3c30027a50 --- /dev/null +++ b/cns/singletenantcontroller/scopedclient.go @@ -0,0 +1,33 @@ +package kubecontroller + +import ( + "context" + + "github.com/Azure/azure-container-networking/crd/nodenetworkconfig" + "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" + "k8s.io/apimachinery/pkg/types" +) + +// ScopedClient is provided to interface with a single configured NodeNetworkConfig. +type ScopedClient struct { + types.NamespacedName + *nodenetworkconfig.Client +} + +// NewScopedClient returns a NodeNetworkConfig client scoped to a single NodeNetworkConfig. +func NewScopedClient(cli *nodenetworkconfig.Client, key types.NamespacedName) *ScopedClient { + return &ScopedClient{ + NamespacedName: key, + Client: cli, + } +} + +// Get returns the NodeNetworkConfig that this scoped client is associated to. +func (sc *ScopedClient) Get(ctx context.Context) (*v1alpha.NodeNetworkConfig, error) { + return sc.Client.Get(ctx, sc.NamespacedName) +} + +// UpdateSpec updates the associated NodeNetworkConfig with the passed NodeNetworkConfigSpec. +func (sc *ScopedClient) UpdateSpec(ctx context.Context, spec *v1alpha.NodeNetworkConfigSpec) (*v1alpha.NodeNetworkConfig, error) { + return sc.Client.UpdateSpec(ctx, sc.NamespacedName, spec) +} From e9251de60333fd0a2654bbfb76d1b75bb681125d Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Tue, 14 Sep 2021 19:15:35 -0500 Subject: [PATCH 05/13] inline reconciler event filters Signed-off-by: Evan Baker --- cns/service/main.go | 2 +- cns/singletenantcontroller/cnsclient.go | 13 - cns/singletenantcontroller/conversion.go | 1 + cns/singletenantcontroller/eventfilter.go | 36 - .../kubecontroller/crdrequestcontroller.go | 359 ---------- .../crdrequestcontroller_test.go | 677 ------------------ cns/singletenantcontroller/reconciler.go | 35 +- 7 files changed, 34 insertions(+), 1089 deletions(-) delete mode 100644 cns/singletenantcontroller/cnsclient.go delete mode 100644 cns/singletenantcontroller/eventfilter.go delete mode 100644 cns/singletenantcontroller/kubecontroller/crdrequestcontroller.go delete mode 100644 cns/singletenantcontroller/kubecontroller/crdrequestcontroller_test.go diff --git a/cns/service/main.go b/cns/service/main.go index 21daf0553f..b12a4230f4 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -844,7 +844,7 @@ func initCNS(ctx context.Context, cli nodeNetworkConfigGetter, ncReconciler ncSt return errors.Wrap(ncReconciler.ReconcileNCState(&ncRequest, podInfoByIP, nnc.Status.Scaler, nnc.Spec), "err in CNS reconciliation") } -// initializeCRD state +// InitializeCRDState builds and starts the CRD controllers. func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cnsconfig configuration.CNSConfig) error { logger.Printf("[Azure CNS] Starting request controller") diff --git a/cns/singletenantcontroller/cnsclient.go b/cns/singletenantcontroller/cnsclient.go deleted file mode 100644 index f377568477..0000000000 --- a/cns/singletenantcontroller/cnsclient.go +++ /dev/null @@ -1,13 +0,0 @@ -package kubecontroller - -import ( - "github.com/Azure/azure-container-networking/cns" - "github.com/Azure/azure-container-networking/cns/types" - "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" -) - -type cnsclient interface { - ReconcileNCState(*cns.CreateNetworkContainerRequest, map[string]cns.PodInfo, v1alpha.Scaler, v1alpha.NodeNetworkConfigSpec) types.ResponseCode - CreateOrUpdateNetworkContainerInternal(*cns.CreateNetworkContainerRequest) types.ResponseCode - UpdateIPAMPoolMonitor(scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) -} diff --git a/cns/singletenantcontroller/conversion.go b/cns/singletenantcontroller/conversion.go index 35b2dd491a..f4f0bdfb3a 100644 --- a/cns/singletenantcontroller/conversion.go +++ b/cns/singletenantcontroller/conversion.go @@ -10,6 +10,7 @@ import ( "github.com/pkg/errors" ) +// ErrUnsupoortedNCQuantity indicates that the node has an unsupported nummber of Network Containers attached. var ErrUnsupportedNCQuantity = errors.New("unsupported number of network containers") // CRDStatusToNCRequest translates a crd status to createnetworkcontainer request diff --git a/cns/singletenantcontroller/eventfilter.go b/cns/singletenantcontroller/eventfilter.go deleted file mode 100644 index eac761656e..0000000000 --- a/cns/singletenantcontroller/eventfilter.go +++ /dev/null @@ -1,36 +0,0 @@ -package kubecontroller - -import ( - "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/predicate" -) - -type NodeNetworkConfigFilter struct { - predicate.Funcs - nodeName string -} - -// Returns true if request is to be processed by Reconciler -// Checks that old generation equals new generation because status changes don't change generation number -func (n NodeNetworkConfigFilter) Update(e event.UpdateEvent) bool { - isNodeName := n.isNodeName(e.ObjectOld.GetName()) - oldGeneration := e.ObjectOld.GetGeneration() - newGeneration := e.ObjectOld.GetGeneration() - return (oldGeneration == newGeneration) && isNodeName -} - -// Only process create events if CRD name equals this host's name -func (n NodeNetworkConfigFilter) Create(e event.CreateEvent) bool { - return n.isNodeName(e.Object.GetName()) -} - -// Delete is a noop filter to ignore all delete events. -// TODO: Decide what deleting crd means with DNC -func (n NodeNetworkConfigFilter) Delete(e event.DeleteEvent) bool { - return false -} - -// Given a string, returns if that string equals the nodename running this program -func (n NodeNetworkConfigFilter) isNodeName(metaName string) bool { - return metaName == n.nodeName -} diff --git a/cns/singletenantcontroller/kubecontroller/crdrequestcontroller.go b/cns/singletenantcontroller/kubecontroller/crdrequestcontroller.go deleted file mode 100644 index b1d7830f85..0000000000 --- a/cns/singletenantcontroller/kubecontroller/crdrequestcontroller.go +++ /dev/null @@ -1,359 +0,0 @@ -package kubecontroller - -import ( - "context" - "fmt" - "os" - "sync" - - "github.com/Azure/azure-container-networking/cns" - "github.com/Azure/azure-container-networking/cns/cnireconciler" - "github.com/Azure/azure-container-networking/cns/logger" - "github.com/Azure/azure-container-networking/cns/restserver" - "github.com/Azure/azure-container-networking/cns/singletenantcontroller" - "github.com/Azure/azure-container-networking/cns/types" - "github.com/Azure/azure-container-networking/crd" - "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" - "github.com/pkg/errors" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/runtime" - clientgoscheme "k8s.io/client-go/kubernetes/scheme" - "k8s.io/client-go/rest" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/manager" -) - -const ( - nodeNameEnvVar = "NODENAME" - k8sNamespace = "kube-system" - crdTypeName = "nodenetworkconfigs" - allNamespaces = "" -) - -// Config has crdRequestController options -type Config struct { - // InitializeFromCNI whether or not to initialize CNS state from k8s/CRDs - InitializeFromCNI bool - KubeConfig *rest.Config - MetricsBindAddress string - Service *restserver.HTTPRestService -} - -var _ singletenantcontroller.RequestController = (*requestController)(nil) - -type cnsrestservice interface { - ReconcileNCState(*cns.CreateNetworkContainerRequest, map[string]cns.PodInfo, v1alpha.Scaler, v1alpha.NodeNetworkConfigSpec) types.ResponseCode - CreateOrUpdateNetworkContainerInternal(*cns.CreateNetworkContainerRequest) types.ResponseCode -} - -// requestController -// - watches CRD status changes -// - updates CRD spec -type requestController struct { - cfg Config - mgr manager.Manager // Manager starts the reconcile loop which watches for crd status changes - KubeClient KubeClient // KubeClient is a cached client which interacts with API server - directAPIClient DirectAPIClient // Direct client to interact with API server - directCRDClient DirectCRDClient // Direct client to interact with CRDs on API server - CNSRestService cnsrestservice - nodeName string // name of node running this program - Reconciler *CrdReconciler - initialized bool - Started bool - lock sync.Mutex -} - -// GetKubeConfig precedence -// * --kubeconfig flag pointing at a file at this cmd line -// * KUBECONFIG environment variable pointing at a file -// * In-cluster config if running in cluster -// * $HOME/.kube/config if exists -func GetKubeConfig() (*rest.Config, error) { - k8sconfig, err := ctrl.GetConfig() - if err != nil { - return nil, err - } - return k8sconfig, nil -} - -// New builds a requestController struct given a reference to CNS's HTTPRestService state. -func New(cfg Config) (*requestController, error) { - // Check that logger package has been intialized - if logger.Log == nil { - return nil, errors.New("Must initialize logger before calling") - } - - // Check that NODENAME environment variable is set. NODENAME is name of node running this program - nodeName := os.Getenv(nodeNameEnvVar) - if nodeName == "" { - return nil, errors.New("Must declare " + nodeNameEnvVar + " environment variable.") - } - - // Add client-go scheme to runtime sheme so manager can recognize it - scheme := runtime.NewScheme() - if err := clientgoscheme.AddToScheme(scheme); err != nil { - return nil, errors.New("Error adding client-go scheme to runtime scheme") - } - - // Add CRD scheme to runtime sheme so manager can recognize it - if err := v1alpha.AddToScheme(scheme); err != nil { - return nil, errors.New("Error adding NodeNetworkConfig scheme to runtime scheme") - } - - // Create a direct client to the API server which we use to list pods when initializing cns state before reconcile loop - directAPIClient, err := NewAPIDirectClient(cfg.KubeConfig) - if err != nil { - return nil, fmt.Errorf("Error creating direct API Client: %v", err) - } - - // Create a direct client to the API server configured to get nodenetconfigs to get nnc for same reason above - directCRDClient, err := NewCRDDirectClient(cfg.KubeConfig, &v1alpha.GroupVersion) - if err != nil { - return nil, fmt.Errorf("Error creating direct CRD client: %v", err) - } - - // Create manager for CrdRequestController - // MetricsBindAddress is the tcp address that the controller should bind to - // for serving prometheus metrics, set to "0" to disable - mgr, err := ctrl.NewManager(cfg.KubeConfig, ctrl.Options{ - Scheme: scheme, - MetricsBindAddress: cfg.MetricsBindAddress, - Namespace: k8sNamespace, - }) - if err != nil { - logger.Errorf("[cns-rc] Error creating new request controller manager: %v", err) - return nil, err - } - - // Create reconciler - crdreconciler := &CrdReconciler{ - KubeClient: mgr.GetClient(), - NodeName: nodeName, - CNSRestService: cfg.Service, - } - - // Setup manager with reconciler - if err := crdreconciler.SetupWithManager(mgr); err != nil { - logger.Errorf("[cns-rc] Error creating new CrdRequestController: %v", err) - return nil, err - } - - // Create the requestController - rc := requestController{ - cfg: cfg, - mgr: mgr, - KubeClient: mgr.GetClient(), - directAPIClient: directAPIClient, - directCRDClient: directCRDClient, - CNSRestService: cfg.Service, - nodeName: nodeName, - Reconciler: crdreconciler, - } - - return &rc, nil -} - -// Init will initialize/reconcile the CNS state -func (rc *requestController) Init(ctx context.Context) error { - logger.Printf("InitRequestController") - - rc.lock.Lock() - defer rc.lock.Unlock() - - if err := rc.initCNS(ctx); err != nil { - logger.Errorf("[cns-rc] Error initializing cns state: %v", err) - return err - } - - rc.initialized = true - return nil -} - -// Start starts the Reconciler loop which watches for CRD status updates -func (rc *requestController) Start(ctx context.Context) error { - logger.Printf("StartRequestController") - - rc.lock.Lock() - if !rc.initialized { - rc.lock.Unlock() - return fmt.Errorf("Failed to start requestController, state is not initialized [%v]", rc) - } - - // Setting the started state - rc.Started = true - rc.lock.Unlock() - - logger.Printf("Starting reconcile loop") - if err := rc.mgr.Start(ctx); err != nil { - if crd.IsNotDefined(err) { - logger.Errorf("[cns-rc] CRD is not defined on cluster, starting reconcile loop failed: %v", err) - os.Exit(1) - } - - return err - } - - return nil -} - -// return if RequestController is started -func (rc *requestController) IsStarted() bool { - rc.lock.Lock() - defer rc.lock.Unlock() - return rc.Started -} - -// InitCNS initializes cns by passing pods and a createnetworkcontainerrequest -func (rc *requestController) initCNS(ctx context.Context) error { - // Get nnc using direct client - nnc, err := rc.getNodeNetConfigDirect(ctx, rc.nodeName, k8sNamespace) - if err != nil { - // If the CRD is not defined, exit - if crd.IsNotDefined(err) { - logger.Errorf("CRD is not defined on cluster: %v", err) - os.Exit(1) - } - - if nnc == nil { - logger.Errorf("NodeNetworkConfig is not present on cluster") - return nil - } - - // If instance of crd is not found, pass nil to CNSRestService - if client.IgnoreNotFound(err) == nil { - //nolint:wrapcheck - return restserver.ResponseCodeToError(rc.CNSRestService.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec)) - } - - // If it's any other error, log it and return - logger.Errorf("Error when getting nodeNetConfig using direct client when initializing cns state: %v", err) - return err - } - - // If there are no NCs, pass nil to CNSRestService - if len(nnc.Status.NetworkContainers) == 0 { - //nolint:wrapcheck - return restserver.ResponseCodeToError(rc.CNSRestService.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec)) - } - - // Convert to CreateNetworkContainerRequest - ncRequest, err := CRDStatusToNCRequest(nnc.Status) - if err != nil { - logger.Errorf("Error when converting nodeNetConfig status into CreateNetworkContainerRequest: %v", err) - return err - } - - var podInfoByIPProvider cns.PodInfoByIPProvider - - if rc.cfg.InitializeFromCNI { - // rebuild CNS state from CNI - logger.Printf("initializing CNS from CNI") - podInfoByIPProvider, err = cnireconciler.NewCNIPodInfoProvider() - if err != nil { - return err - } - } else { - logger.Printf("initializing CNS from apiserver") - // Get all pods using direct client - pods, err := rc.getAllPods(ctx, rc.nodeName) - if err != nil { - logger.Errorf("error when getting all pods when initializing cns: %v", err) - return err - } - podInfoByIPProvider = cns.PodInfoByIPProviderFunc(func() (map[string]cns.PodInfo, error) { - return rc.kubePodsToPodInfoByIP(pods.Items) - }) - } - - podInfoByIP, err := podInfoByIPProvider.PodInfoByIP() - if err != nil { - return errors.Wrap(err, "err in CNS initialization") - } - - // errors.Wrap provides additional context, and return nil if the err input arg is nil - // Call CNSRestService init cns passing those two things. - return errors.Wrap(restserver.ResponseCodeToError(rc.CNSRestService.ReconcileNCState(&ncRequest, podInfoByIP, nnc.Status.Scaler, nnc.Spec)), "err in CNS reconciliation") -} - -// kubePodsToPodInfoByIP maps kubernetes pods to cns.PodInfos by IP -func (rc *requestController) kubePodsToPodInfoByIP(pods []corev1.Pod) (map[string]cns.PodInfo, error) { - podInfoByIP := map[string]cns.PodInfo{} - for _, pod := range pods { - if !pod.Spec.HostNetwork { - if _, ok := podInfoByIP[pod.Status.PodIP]; ok { - return nil, errors.Wrap(cns.ErrDuplicateIP, pod.Status.PodIP) - } - podInfoByIP[pod.Status.PodIP] = cns.NewPodInfo("", "", pod.Name, pod.Namespace) - } - } - return podInfoByIP, nil -} - -// UpdateCRDSpec updates the CRD spec -func (rc *requestController) UpdateCRDSpec(ctx context.Context, nnc v1alpha.NodeNetworkConfigSpec) error { - nodeNetworkConfig, err := rc.getNodeNetConfig(ctx, rc.nodeName, k8sNamespace) - if err != nil { - logger.Errorf("[cns-rc] Error getting CRD when updating spec %v", err) - return err - } - - logger.Printf("[cns-rc] Received update for IP count %+v", nnc) - - // Update the CRD spec - nnc.DeepCopyInto(&nodeNetworkConfig.Spec) - - logger.Printf("[cns-rc] After deep copy %+v", nodeNetworkConfig.Spec) - - // Send update to API server - if err := rc.updateNodeNetConfig(ctx, nodeNetworkConfig); err != nil { - logger.Errorf("[cns-rc] Error updating CRD spec %v", err) - return err - } - - // record IP metrics - requestedIPs.Set(float64(nnc.RequestedIPCount)) - unusedIPs.Set(float64(len(nnc.IPsNotInUse))) - return nil -} - -// getNodeNetConfig gets the nodeNetworkConfig CRD given the name and namespace of the CRD object -func (rc *requestController) getNodeNetConfig(ctx context.Context, name, namespace string) (*v1alpha.NodeNetworkConfig, error) { - nodeNetworkConfig := &v1alpha.NodeNetworkConfig{} - - err := rc.KubeClient.Get(ctx, client.ObjectKey{ - Namespace: namespace, - Name: name, - }, nodeNetworkConfig) - if err != nil { - return nil, err - } - - return nodeNetworkConfig, nil -} - -// getNodeNetConfigDirect gets the nodeNetworkConfig CRD using a direct client -func (rc *requestController) getNodeNetConfigDirect(ctx context.Context, name, namespace string) (*v1alpha.NodeNetworkConfig, error) { - //nolint:wrapcheck - return rc.directCRDClient.Get(ctx, name, namespace, crdTypeName) -} - -// updateNodeNetConfig updates the nodeNetConfig object in the API server with the given nodeNetworkConfig object -func (rc *requestController) updateNodeNetConfig(ctx context.Context, nnc *v1alpha.NodeNetworkConfig) error { - //nolint:wrapcheck - return rc.KubeClient.Update(ctx, nnc) -} - -// getAllPods gets all pods running on the node using the direct API client -func (rc *requestController) getAllPods(ctx context.Context, node string) (*corev1.PodList, error) { - var ( - pods *corev1.PodList - err error - ) - - if pods, err = rc.directAPIClient.ListPods(ctx, allNamespaces, node); err != nil { - return nil, err - } - - return pods, nil -} diff --git a/cns/singletenantcontroller/kubecontroller/crdrequestcontroller_test.go b/cns/singletenantcontroller/kubecontroller/crdrequestcontroller_test.go deleted file mode 100644 index fc42b91089..0000000000 --- a/cns/singletenantcontroller/kubecontroller/crdrequestcontroller_test.go +++ /dev/null @@ -1,677 +0,0 @@ -package kubecontroller - -import ( - "context" - "errors" - "fmt" - "os" - "reflect" - "strings" - "testing" - - "github.com/Azure/azure-container-networking/cns" - "github.com/Azure/azure-container-networking/cns/logger" - "github.com/Azure/azure-container-networking/cns/types" - "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -const ( - existingNNCName = "nodenetconfig_1" - existingPodName = "pod_1" - hostNetworkPodName = "pod_hostNet" - allocatedPodIP = "10.0.0.2" - allocatedUUID = "539970a2-c2dd-11ea-b3de-0242ac130004" - allocatedUUID2 = "01a5dd00-cd5d-11ea-87d0-0242ac130003" - networkContainerID = "24fcd232-0364-41b0-8027-6e6ef9aeabc6" - existingNamespace = k8sNamespace - nonexistingNNCName = "nodenetconfig_nonexisting" - nonexistingNamespace = "namespace_nonexisting" - ncPrimaryIP = "10.0.0.1" - subnetRange = "10.0.0.0/24" -) - -// MockAPI is a mock of kubernete's API server -type MockAPI struct { - nodeNetConfigs map[MockKey]*v1alpha.NodeNetworkConfig - pods map[MockKey]*corev1.Pod -} - -// MockKey is the key to the mockAPI, namespace+"/"+name like in API server -type MockKey struct { - Namespace string - Name string -} - -// MockKubeClient implements KubeClient interface -type MockKubeClient struct { - mockAPI *MockAPI -} - -// Mock implementation of the KubeClient interface Get method -// Mimics that of controller-runtime's client.Client -func (mc MockKubeClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object) error { - mockKey := MockKey{ - Namespace: key.Namespace, - Name: key.Name, - } - - nodeNetConfig, ok := mc.mockAPI.nodeNetConfigs[mockKey] - if !ok { - return errors.New("Node Net Config not found in mock store") - } - nodeNetConfig.DeepCopyInto(obj.(*v1alpha.NodeNetworkConfig)) - - return nil -} - -// Mock implementation of the KubeClient interface Update method -// Mimics that of controller-runtime's client.Client -func (mc MockKubeClient) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { - nodeNetConfig := obj.(*v1alpha.NodeNetworkConfig) - - mockKey := MockKey{ - Namespace: nodeNetConfig.ObjectMeta.Namespace, - Name: nodeNetConfig.ObjectMeta.Name, - } - - _, ok := mc.mockAPI.nodeNetConfigs[mockKey] - - if !ok { - return errors.New("Node Net Config not found in mock store") - } - - nodeNetConfig.DeepCopyInto(mc.mockAPI.nodeNetConfigs[mockKey]) - - return nil -} - -// MockCNSRestServer implements CNSRestServer interface -type MockCNSRestService struct { - MockCNSUpdated bool - MockCNSInitialized bool - Pods map[string]cns.PodInfo - NCRequest *cns.CreateNetworkContainerRequest -} - -// we're just testing that reconciler interacts with CNS on Reconcile(). -func (m *MockCNSRestService) CreateOrUpdateNetworkContainerInternal(ncRequest *cns.CreateNetworkContainerRequest) types.ResponseCode { - m.MockCNSUpdated = true - return types.Success -} - -func (m *MockCNSRestService) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, - podInfoByIP map[string]cns.PodInfo, - scalar v1alpha.Scaler, - spec v1alpha.NodeNetworkConfigSpec) types.ResponseCode { - m.MockCNSInitialized = true - m.Pods = podInfoByIP - m.NCRequest = ncRequest - return types.Success -} - -// MockDirectCRDClient implements the DirectCRDClient interface -var _ DirectCRDClient = &MockDirectCRDClient{} - -type MockDirectCRDClient struct { - mockAPI *MockAPI -} - -func (mc *MockDirectCRDClient) Get(ctx context.Context, name, namespace, typeName string) (*v1alpha.NodeNetworkConfig, error) { - var ( - mockKey MockKey - nodeNetConfig *v1alpha.NodeNetworkConfig - ok bool - ) - - mockKey = MockKey{ - Namespace: namespace, - Name: name, - } - - if nodeNetConfig, ok = mc.mockAPI.nodeNetConfigs[mockKey]; !ok { - return nil, fmt.Errorf("No nnc by that name in mock client") - } - - return nodeNetConfig, nil -} - -// MockDirectAPIClient implements the DirectAPIClient interface -var _ DirectAPIClient = &MockDirectAPIClient{} - -type MockDirectAPIClient struct { - mockAPI *MockAPI -} - -func (mc *MockDirectAPIClient) ListPods(ctx context.Context, namespace, node string) (*corev1.PodList, error) { - var ( - pod *corev1.Pod - pods corev1.PodList - ) - - for _, pod = range mc.mockAPI.pods { - if namespace == "" || namespace == pod.ObjectMeta.Namespace { - if pod.Spec.NodeName == node { - pods.Items = append(pods.Items, *pod) - } - } - } - - if len(pods.Items) == 0 { - return nil, errors.New("No pods found") - } - - return &pods, nil -} - -func TestNewCrdRequestController(t *testing.T) { - // Test making request controller without logger initialized, should fail - _, err := New(Config{}) - if err == nil { - t.Fatalf("Expected error when making NewCrdRequestController without initializing logger, got nil error") - } else if !strings.Contains(err.Error(), "logger") { - t.Fatalf("Expected logger error when making NewCrdRequestController without initializing logger, got: %+v", err) - } - - // Initialize logger - logger.InitLogger("Azure CRD Request Controller", 3, 3, "") - - // Test making request controller without NODENAME env var set, should fail - // Save old value though - nodeName, found := os.LookupEnv(nodeNameEnvVar) - os.Unsetenv(nodeNameEnvVar) - defer func() { - if found { - os.Setenv(nodeNameEnvVar, nodeName) - } - }() - - _, err = New(Config{}) - if err == nil { - t.Fatalf("Expected error when making NewCrdRequestController without setting " + nodeNameEnvVar + " env var, got nil error") - } else if !strings.Contains(err.Error(), nodeNameEnvVar) { - t.Fatalf("Expected error when making NewCrdRequestController without setting "+nodeNameEnvVar+" env var, got: %+v", err) - } - - // TODO: Create integration tests with minikube -} - -func TestGetNonExistingNodeNetConfig(t *testing.T) { - nodeNetConfig := &v1alpha.NodeNetworkConfig{ - ObjectMeta: v1.ObjectMeta{ - Name: existingNNCName, - Namespace: existingNamespace, - }, - } - mockNNCKey := MockKey{ - Namespace: existingNamespace, - Name: existingNNCName, - } - mockAPI := &MockAPI{ - nodeNetConfigs: map[MockKey]*v1alpha.NodeNetworkConfig{ - mockNNCKey: nodeNetConfig, - }, - } - mockKubeClient := MockKubeClient{ - mockAPI: mockAPI, - } - rc := &requestController{ - KubeClient: mockKubeClient, - } - logger.InitLogger("Azure CNS RequestController", 0, 0, "") - - // Test getting nonexisting NodeNetconfig obj - _, err := rc.getNodeNetConfig(context.Background(), nonexistingNNCName, nonexistingNamespace) - if err == nil { - t.Fatalf("Expected error when getting nonexisting nodenetconfig obj. Got nil error.") - } -} - -func TestGetExistingNodeNetConfig(t *testing.T) { - nodeNetConfig := &v1alpha.NodeNetworkConfig{ - ObjectMeta: v1.ObjectMeta{ - Name: existingNNCName, - Namespace: existingNamespace, - }, - } - mockNNCKey := MockKey{ - Namespace: existingNamespace, - Name: existingNNCName, - } - mockAPI := &MockAPI{ - nodeNetConfigs: map[MockKey]*v1alpha.NodeNetworkConfig{ - mockNNCKey: nodeNetConfig, - }, - } - mockKubeClient := MockKubeClient{ - mockAPI: mockAPI, - } - rc := &requestController{ - KubeClient: mockKubeClient, - } - logger.InitLogger("Azure CNS RequestController", 0, 0, "") - - // Test getting existing NodeNetConfig obj - nodeNetConfig, err := rc.getNodeNetConfig(context.Background(), existingNNCName, existingNamespace) - if err != nil { - t.Fatalf("Expected no error when getting existing NodeNetworkConfig: %+v", err) - } - - if !reflect.DeepEqual(nodeNetConfig, mockAPI.nodeNetConfigs[mockNNCKey]) { - t.Fatalf("Expected fetched node net config to equal one in mock store") - } -} - -func TestUpdateNonExistingNodeNetConfig(t *testing.T) { - nodeNetConfig := &v1alpha.NodeNetworkConfig{ - ObjectMeta: v1.ObjectMeta{ - Name: existingNNCName, - Namespace: existingNamespace, - }, - } - mockNNCKey := MockKey{ - Namespace: existingNamespace, - Name: existingNNCName, - } - mockAPI := &MockAPI{ - nodeNetConfigs: map[MockKey]*v1alpha.NodeNetworkConfig{ - mockNNCKey: nodeNetConfig, - }, - } - mockKubeClient := MockKubeClient{ - mockAPI: mockAPI, - } - rc := &requestController{ - KubeClient: mockKubeClient, - } - logger.InitLogger("Azure CNS RequestController", 0, 0, "") - - // Test updating non existing NodeNetworkConfig obj - nodeNetConfigNonExisting := &v1alpha.NodeNetworkConfig{ObjectMeta: metav1.ObjectMeta{ - Name: nonexistingNNCName, - Namespace: nonexistingNamespace, - }} - - err := rc.updateNodeNetConfig(context.Background(), nodeNetConfigNonExisting) - - if err == nil { - t.Fatalf("Expected error when updating non existing NodeNetworkConfig. Got nil error") - } -} - -func TestUpdateExistingNodeNetConfig(t *testing.T) { - nodeNetConfig := &v1alpha.NodeNetworkConfig{ - ObjectMeta: v1.ObjectMeta{ - Name: existingNNCName, - Namespace: existingNamespace, - }, - } - mockNNCKey := MockKey{ - Namespace: existingNamespace, - Name: existingNNCName, - } - mockAPI := &MockAPI{ - nodeNetConfigs: map[MockKey]*v1alpha.NodeNetworkConfig{ - mockNNCKey: nodeNetConfig, - }, - } - mockKubeClient := MockKubeClient{ - mockAPI: mockAPI, - } - rc := &requestController{ - nodeName: existingNNCName, - KubeClient: mockKubeClient, - } - logger.InitLogger("Azure CNS RequestController", 0, 0, "") - - // Update an existing NodeNetworkConfig obj from the mock API - nodeNetConfigUpdated := mockAPI.nodeNetConfigs[mockNNCKey].DeepCopy() - nodeNetConfigUpdated.ObjectMeta.ClusterName = "New cluster name" - - err := rc.updateNodeNetConfig(context.Background(), nodeNetConfigUpdated) - if err != nil { - t.Fatalf("Expected no error when updating existing NodeNetworkConfig, got :%v", err) - } - - // See that NodeNetworkConfig in mock store was updated - if !reflect.DeepEqual(nodeNetConfigUpdated, mockAPI.nodeNetConfigs[mockNNCKey]) { - t.Fatal("Update of existing NodeNetworkConfig did not get passed along") - } -} - -func TestUpdateSpecOnNonExistingNodeNetConfig(t *testing.T) { - nodeNetConfig := &v1alpha.NodeNetworkConfig{ - ObjectMeta: v1.ObjectMeta{ - Name: existingNNCName, - Namespace: existingNamespace, - }, - } - mockNNCKey := MockKey{ - Namespace: existingNamespace, - Name: existingNNCName, - } - mockAPI := &MockAPI{ - nodeNetConfigs: map[MockKey]*v1alpha.NodeNetworkConfig{ - mockNNCKey: nodeNetConfig, - }, - } - mockKubeClient := MockKubeClient{ - mockAPI: mockAPI, - } - rc := &requestController{ - nodeName: nonexistingNNCName, - KubeClient: mockKubeClient, - } - logger.InitLogger("Azure CNS RequestController", 0, 0, "") - - spec := v1alpha.NodeNetworkConfigSpec{ - RequestedIPCount: int64(10), - IPsNotInUse: []string{ - allocatedUUID, - allocatedUUID2, - }, - } - - // Test updating spec for existing NodeNetworkConfig - err := rc.UpdateCRDSpec(context.Background(), spec) - - if err == nil { - t.Fatalf("Expected error when updating spec on non-existing crd") - } -} - -func TestUpdateSpecOnExistingNodeNetConfig(t *testing.T) { - nodeNetConfig := &v1alpha.NodeNetworkConfig{ - ObjectMeta: v1.ObjectMeta{ - Name: existingNNCName, - Namespace: existingNamespace, - }, - } - mockNNCKey := MockKey{ - Namespace: existingNamespace, - Name: existingNNCName, - } - mockAPI := &MockAPI{ - nodeNetConfigs: map[MockKey]*v1alpha.NodeNetworkConfig{ - mockNNCKey: nodeNetConfig, - }, - } - mockKubeClient := MockKubeClient{ - mockAPI: mockAPI, - } - rc := &requestController{ - nodeName: existingNNCName, - KubeClient: mockKubeClient, - } - logger.InitLogger("Azure CNS RequestController", 0, 0, "") - - spec := v1alpha.NodeNetworkConfigSpec{ - RequestedIPCount: int64(10), - IPsNotInUse: []string{ - allocatedUUID, - allocatedUUID2, - }, - } - - // Test update spec for existing NodeNetworkConfig - err := rc.UpdateCRDSpec(context.Background(), spec) - if err != nil { - t.Fatalf("Expected no error when updating spec on existing crd, got :%v", err) - } - - if !reflect.DeepEqual(mockAPI.nodeNetConfigs[mockNNCKey].Spec, spec) { - t.Fatalf("Expected Spec to equal requested spec update") - } -} - -// test get nnc directly -func TestGetExistingNNCDirectClient(t *testing.T) { - nodeNetConfigFill := &v1alpha.NodeNetworkConfig{ - ObjectMeta: v1.ObjectMeta{ - Name: existingNNCName, - Namespace: existingNamespace, - }, - } - mockNNCKey := MockKey{ - Namespace: existingNamespace, - Name: existingNNCName, - } - mockAPI := &MockAPI{ - nodeNetConfigs: map[MockKey]*v1alpha.NodeNetworkConfig{ - mockNNCKey: nodeNetConfigFill, - }, - } - mockCRDDirectClient := &MockDirectCRDClient{ - mockAPI: mockAPI, - } - rc := &requestController{ - directCRDClient: mockCRDDirectClient, - } - - nodeNetConfigFetched, err := rc.getNodeNetConfigDirect(context.Background(), existingNNCName, existingNamespace) - if err != nil { - t.Fatalf("Expected to be able to get existing nodenetconfig with directCRD client: %v", err) - } - - if !reflect.DeepEqual(nodeNetConfigFill, nodeNetConfigFetched) { - t.Fatalf("Expected fetched nodenetconfig to be equal to one we loaded into store") - } -} - -// test get nnc directly non existing -func TestGetNonExistingNNCDirectClient(t *testing.T) { - nodeNetConfigFill := &v1alpha.NodeNetworkConfig{ - ObjectMeta: v1.ObjectMeta{ - Name: existingNNCName, - Namespace: existingNamespace, - }, - } - mockNNCKey := MockKey{ - Namespace: existingNamespace, - Name: existingNNCName, - } - mockAPI := &MockAPI{ - nodeNetConfigs: map[MockKey]*v1alpha.NodeNetworkConfig{ - mockNNCKey: nodeNetConfigFill, - }, - } - mockCRDDirectClient := &MockDirectCRDClient{ - mockAPI: mockAPI, - } - rc := &requestController{ - directCRDClient: mockCRDDirectClient, - } - - _, err := rc.getNodeNetConfigDirect(context.Background(), nonexistingNNCName, nonexistingNamespace) - - if err == nil { - t.Fatalf("Expected error when getting non-existing nodenetconfig with direct crd client.") - } -} - -// test get all pods on node -func TestGetPodsExistingNodeDirectClient(t *testing.T) { - mockPodKey := MockKey{ - Namespace: existingNamespace, - Name: existingPodName, - } - mockPod := &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: existingPodName, - Namespace: existingNamespace, - }, - Status: corev1.PodStatus{ - PodIP: allocatedPodIP, - }, - Spec: corev1.PodSpec{ - NodeName: existingNNCName, - HostNetwork: false, - }, - } - mockAPI := &MockAPI{ - pods: map[MockKey]*corev1.Pod{ - mockPodKey: mockPod, - }, - } - mockAPIDirectClient := &MockDirectAPIClient{ - mockAPI: mockAPI, - } - rc := &requestController{ - directAPIClient: mockAPIDirectClient, - } - - pods, err := rc.getAllPods(context.Background(), existingNNCName) - if err != nil { - t.Fatalf("Expected to be able to get all pods given correct node name") - } - - if !reflect.DeepEqual(pods.Items[0], *mockPod) { - t.Fatalf("Expected pods to equal each other when getting all pods on node") - } -} - -func TestGetPodsNonExistingNodeDirectClient(t *testing.T) { - mockPodKey := MockKey{ - Namespace: existingNamespace, - Name: existingPodName, - } - mockPod := &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: existingPodName, - Namespace: existingNamespace, - }, - Status: corev1.PodStatus{ - PodIP: allocatedPodIP, - }, - Spec: corev1.PodSpec{ - NodeName: existingNNCName, - HostNetwork: false, - }, - } - mockAPI := &MockAPI{ - pods: map[MockKey]*corev1.Pod{ - mockPodKey: mockPod, - }, - } - mockAPIDirectClient := &MockDirectAPIClient{ - mockAPI: mockAPI, - } - rc := &requestController{ - directAPIClient: mockAPIDirectClient, - } - - _, err := rc.getAllPods(context.Background(), nonexistingNNCName) - - if err == nil { - t.Fatalf("Expected failure when getting pods of non-existant node") - } -} - -// test that cns init gets called -func TestInitRequestController(t *testing.T) { - nodeNetConfigFill := &v1alpha.NodeNetworkConfig{ - ObjectMeta: v1.ObjectMeta{ - Name: existingNNCName, - Namespace: existingNamespace, - }, - Status: v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: ncPrimaryIP, - ID: networkContainerID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: allocatedUUID, - IP: allocatedPodIP, - }, - }, - SubnetAddressSpace: subnetRange, - Version: 1, - }, - }, - }, - } - mockNNCKey := MockKey{ - Namespace: existingNamespace, - Name: existingNNCName, - } - mockPodKey := MockKey{ - Namespace: existingNamespace, - Name: existingPodName, - } - mockPod := &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: existingPodName, - Namespace: existingNamespace, - }, - Status: corev1.PodStatus{ - PodIP: allocatedPodIP, - }, - Spec: corev1.PodSpec{ - NodeName: existingNNCName, - HostNetwork: false, - }, - } - mockPodKeyHostNetwork := MockKey{ - Namespace: existingNamespace, - Name: hostNetworkPodName, - } - mockPodHostNetwork := &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: hostNetworkPodName, - Namespace: existingNamespace, - }, - Spec: corev1.PodSpec{ - NodeName: existingNNCName, - HostNetwork: true, - }, - } - mockAPI := &MockAPI{ - nodeNetConfigs: map[MockKey]*v1alpha.NodeNetworkConfig{ - mockNNCKey: nodeNetConfigFill, - }, - pods: map[MockKey]*corev1.Pod{ - mockPodKey: mockPod, - mockPodKeyHostNetwork: mockPodHostNetwork, - }, - } - mockAPIDirectClient := &MockDirectAPIClient{ - mockAPI: mockAPI, - } - mockCRDDirectClient := &MockDirectCRDClient{ - mockAPI: mockAPI, - } - mockCNSRestService := &MockCNSRestService{} - rc := &requestController{ - cfg: Config{}, - directAPIClient: mockAPIDirectClient, - directCRDClient: mockCRDDirectClient, - CNSRestService: mockCNSRestService, - nodeName: existingNNCName, - } - - logger.InitLogger("Azure CNS RequestController", 0, 0, "") - - if err := rc.initCNS(context.Background()); err != nil { - t.Fatalf("Expected no failure to init cns when given mock clients") - } - - if !mockCNSRestService.MockCNSInitialized { - t.Fatalf("MockCNSClient should have been initialized on request controller init") - } - - if _, ok := mockCNSRestService.Pods[mockPodHostNetwork.Status.PodIP]; ok { - t.Fatalf("Init shouldn't pass cns pods that are part of host network") - } - - if _, ok := mockCNSRestService.Pods[mockPod.Status.PodIP]; !ok { - t.Fatalf("Init should pass cns pods that aren't part of host network") - } - - if _, ok := mockCNSRestService.NCRequest.SecondaryIPConfigs[allocatedUUID]; !ok { - t.Fatalf("Expected secondary ip config to be in ncrequest") - } -} diff --git a/cns/singletenantcontroller/reconciler.go b/cns/singletenantcontroller/reconciler.go index d4a56dad09..06cdf70fa3 100644 --- a/cns/singletenantcontroller/reconciler.go +++ b/cns/singletenantcontroller/reconciler.go @@ -3,23 +3,37 @@ package kubecontroller import ( "context" + "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/logger" "github.com/Azure/azure-container-networking/cns/restserver" "github.com/Azure/azure-container-networking/crd/nodenetworkconfig" "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) +type cnsclient interface { + ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo, scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) error + CreateOrUpdateNC(ncRequest cns.CreateNetworkContainerRequest) error + UpdateIPAMPoolMonitor(scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) +} + +type nncgetter interface { + Get(ctx context.Context, key types.NamespacedName) (*v1alpha.NodeNetworkConfig, error) +} + // Reconciler watches for CRD status changes type Reconciler struct { cnscli cnsclient - nnccli *nodenetworkconfig.Client + nnccli nncgetter } -func New(nnccli *nodenetworkconfig.Client, cnscli cnsclient) *Reconciler { +func New(nnccli nncgetter, cnscli cnsclient) *Reconciler { return &Reconciler{ cnscli: cnscli, nnccli: nnccli, @@ -84,6 +98,21 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( func (r *Reconciler) SetupWithManager(mgr ctrl.Manager, nodeName string) error { return ctrl.NewControllerManagedBy(mgr). For(&v1alpha.NodeNetworkConfig{}). - WithEventFilter(NodeNetworkConfigFilter{nodeName: nodeName}). + WithEventFilter(predicate.Funcs{ + // ignore delete events. + DeleteFunc: func(event.DeleteEvent) bool { + return false + }, + }). + WithEventFilter(predicate.NewPredicateFuncs(func(object client.Object) bool { + // match on node name for all other events. + return nodeName == object.GetName() + })). + WithEventFilter(predicate.Funcs{ + // check that the generation is the same - status changes don't update generation. + UpdateFunc: func(ue event.UpdateEvent) bool { + return ue.ObjectOld.GetGeneration() == ue.ObjectNew.GetGeneration() + }, + }). Complete(r) } From 97b1e66adf28110a8e9cd54685ebbef4cd166788 Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Tue, 14 Sep 2021 19:20:51 -0500 Subject: [PATCH 06/13] delint Signed-off-by: Evan Baker --- cns/singletenantcontroller/conversion.go | 2 +- cns/singletenantcontroller/reconciler.go | 7 ++++++- cns/singletenantcontroller/scopedclient.go | 7 +++++-- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/cns/singletenantcontroller/conversion.go b/cns/singletenantcontroller/conversion.go index f4f0bdfb3a..85b8aa7477 100644 --- a/cns/singletenantcontroller/conversion.go +++ b/cns/singletenantcontroller/conversion.go @@ -10,7 +10,7 @@ import ( "github.com/pkg/errors" ) -// ErrUnsupoortedNCQuantity indicates that the node has an unsupported nummber of Network Containers attached. +// ErrUnsupportedNCQuantity indicates that the node has an unsupported nummber of Network Containers attached. var ErrUnsupportedNCQuantity = errors.New("unsupported number of network containers") // CRDStatusToNCRequest translates a crd status to createnetworkcontainer request diff --git a/cns/singletenantcontroller/reconciler.go b/cns/singletenantcontroller/reconciler.go index 06cdf70fa3..a12d9e7284 100644 --- a/cns/singletenantcontroller/reconciler.go +++ b/cns/singletenantcontroller/reconciler.go @@ -8,6 +8,7 @@ import ( "github.com/Azure/azure-container-networking/cns/restserver" "github.com/Azure/azure-container-networking/crd/nodenetworkconfig" "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" + "github.com/pkg/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" @@ -96,7 +97,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( // SetupWithManager Sets up the reconciler with a new manager, filtering using NodeNetworkConfigFilter on nodeName. func (r *Reconciler) SetupWithManager(mgr ctrl.Manager, nodeName string) error { - return ctrl.NewControllerManagedBy(mgr). + err := ctrl.NewControllerManagedBy(mgr). For(&v1alpha.NodeNetworkConfig{}). WithEventFilter(predicate.Funcs{ // ignore delete events. @@ -115,4 +116,8 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager, nodeName string) error { }, }). Complete(r) + if err != nil { + return errors.Wrap(err, "failed to set up reconciler with manager") + } + return nil } diff --git a/cns/singletenantcontroller/scopedclient.go b/cns/singletenantcontroller/scopedclient.go index 3c30027a50..b124dda5b8 100644 --- a/cns/singletenantcontroller/scopedclient.go +++ b/cns/singletenantcontroller/scopedclient.go @@ -5,6 +5,7 @@ import ( "github.com/Azure/azure-container-networking/crd/nodenetworkconfig" "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" + "github.com/pkg/errors" "k8s.io/apimachinery/pkg/types" ) @@ -24,10 +25,12 @@ func NewScopedClient(cli *nodenetworkconfig.Client, key types.NamespacedName) *S // Get returns the NodeNetworkConfig that this scoped client is associated to. func (sc *ScopedClient) Get(ctx context.Context) (*v1alpha.NodeNetworkConfig, error) { - return sc.Client.Get(ctx, sc.NamespacedName) + nnc, err := sc.Client.Get(ctx, sc.NamespacedName) + return nnc, errors.Wrapf(err, "failed to get nnc %v", sc.NamespacedName) } // UpdateSpec updates the associated NodeNetworkConfig with the passed NodeNetworkConfigSpec. func (sc *ScopedClient) UpdateSpec(ctx context.Context, spec *v1alpha.NodeNetworkConfigSpec) (*v1alpha.NodeNetworkConfig, error) { - return sc.Client.UpdateSpec(ctx, sc.NamespacedName, spec) + nnc, err := sc.Client.UpdateSpec(ctx, sc.NamespacedName, spec) + return nnc, errors.Wrapf(err, "failed to update nnc %v", sc.NamespacedName) } From a40e6682c645b04b65e0d297446b499f26a6005e Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Tue, 14 Sep 2021 19:26:15 -0500 Subject: [PATCH 07/13] test env config Signed-off-by: Evan Baker --- cns/configuration/env.go | 2 +- cns/configuration/env_test.go | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) create mode 100644 cns/configuration/env_test.go diff --git a/cns/configuration/env.go b/cns/configuration/env.go index 04844e2199..63f3b028a6 100644 --- a/cns/configuration/env.go +++ b/cns/configuration/env.go @@ -12,7 +12,7 @@ const ( ) // ErrNodeNameUnset indicates the the $EnvNodeName variable is unset in the environment. -var ErrNodeNameUnset = errors.Errorf("Must declare %s environment variable", EnvNodeName) +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) { diff --git a/cns/configuration/env_test.go b/cns/configuration/env_test.go new file mode 100644 index 0000000000..df06b6d610 --- /dev/null +++ b/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) +} From c04bf69c7b0297ae5f8d6864d592176abf4ccd8f Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Wed, 15 Sep 2021 15:03:49 -0500 Subject: [PATCH 08/13] wrap errs Signed-off-by: Evan Baker --- cns/service/main.go | 37 +++++++++------ cns/singletenantcontroller/conversion.go | 17 ++++--- cns/singletenantcontroller/conversion_test.go | 2 +- cns/singletenantcontroller/reconciler.go | 14 +++--- cns/singletenantcontroller/reconciler_test.go | 46 +++++++++++++++++++ crd/nodenetworkconfig/embed.go | 5 +- 6 files changed, 91 insertions(+), 30 deletions(-) create mode 100644 cns/singletenantcontroller/reconciler_test.go diff --git a/cns/service/main.go b/cns/service/main.go index b12a4230f4..f12ece87b6 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -562,7 +562,7 @@ func main() { } logger.Printf("Set GlobalPodInfoScheme %v", cns.GlobalPodInfoScheme) - err = InitializeCRDState(rootCtx, httpRestService, *cnsconfig) + err = InitializeCRDState(rootCtx, httpRestService, cnsconfig) if err != nil { logger.Errorf("Failed to start CRD Controller, err:%v.\n", err) return @@ -806,8 +806,11 @@ func initCNS(ctx context.Context, cli nodeNetworkConfigGetter, ncReconciler ncSt // If instance of crd is not found, pass nil to CNSClient if client.IgnoreNotFound(err) == nil { - //nolint:wrapcheck - return ncReconciler.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec) + err = ncReconciler.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec) + if err != nil { + return errors.Wrap(err, "failed to reconcile NC state") + } + return nil } // If it's any other error, log it and return @@ -817,22 +820,25 @@ func initCNS(ctx context.Context, cli nodeNetworkConfigGetter, ncReconciler ncSt // If there are no NCs, pass nil to CNSClient if len(nnc.Status.NetworkContainers) == 0 { - //nolint:wrapcheck - return ncReconciler.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec) + err = ncReconciler.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec) + if err != nil { + return errors.Wrap(err, "failed to reconcile NC state") + } + return nil } // Convert to CreateNetworkContainerRequest - ncRequest, err := kubecontroller.CRDStatusToNCRequest(nnc.Status) + ncRequest, err := kubecontroller.CRDStatusToNCRequest(&nnc.Status) if err != nil { logger.Errorf("Error when converting nodeNetConfig status into CreateNetworkContainerRequest: %v", err) - return err + return errors.Wrap(err, "failed to convert NNC status to network container request") } // rebuild CNS state from CNI logger.Printf("initializing CNS from CNI") podInfoByIPProvider, err := cnireconciler.NewCNIPodInfoProvider() if err != nil { - return err + return errors.Wrap(err, "failed to create CNI PodInfoProvider") } podInfoByIP, err := podInfoByIPProvider.PodInfoByIP() if err != nil { @@ -845,7 +851,7 @@ func initCNS(ctx context.Context, cli nodeNetworkConfigGetter, ncReconciler ncSt } // InitializeCRDState builds and starts the CRD controllers. -func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cnsconfig configuration.CNSConfig) error { +func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cnsconfig *configuration.CNSConfig) error { logger.Printf("[Azure CNS] Starting request controller") // convert interface type to implementation type @@ -869,21 +875,22 @@ func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cn } nnccli, err := nodenetworkconfig.NewClient(kubeConfig) if err != nil { - return err + return errors.Wrap(err, "failed to create NNC client") } nodeName, err := configuration.NodeName() if err != nil { - return err + return errors.Wrap(err, "failed to get NodeName") } // TODO(rbtr): nodename and namespace should be in the cns config scopedcli := kubecontroller.NewScopedClient(nnccli, types.NamespacedName{Namespace: "kube-system", Name: nodeName}) // initialize the ipam pool monitor httpRestServiceImplementation.IPAMPoolMonitor = ipampoolmonitor.NewCNSIPAMPoolMonitor(httpRestServiceImplementation, scopedcli) - cnscli := &httpapi.Client{ + cnsclient := &httpapi.Client{ RestService: httpRestServiceImplementation, } - if err := initCNS(ctx, scopedcli, cnscli); err != nil { + err = initCNS(ctx, scopedcli, cnsclient) + if err != nil { return errors.Wrap(err, "failed to initialize CNS state") } @@ -893,9 +900,9 @@ func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cn Namespace: "kube-system", // TODO(rbtr): namespace should be in the cns config }) if err != nil { - return err + return errors.Wrap(err, "failed to create manager") } - reconciler := kubecontroller.New(nnccli, cnscli) + reconciler := kubecontroller.New(nnccli, cnsclient) if err := reconciler.SetupWithManager(manager, nodeName); err != nil { return err } diff --git a/cns/singletenantcontroller/conversion.go b/cns/singletenantcontroller/conversion.go index 85b8aa7477..9b6577f558 100644 --- a/cns/singletenantcontroller/conversion.go +++ b/cns/singletenantcontroller/conversion.go @@ -1,7 +1,6 @@ package kubecontroller import ( - "fmt" "net" "strconv" @@ -10,11 +9,17 @@ import ( "github.com/pkg/errors" ) -// ErrUnsupportedNCQuantity indicates that the node has an unsupported nummber of Network Containers attached. -var ErrUnsupportedNCQuantity = errors.New("unsupported number of network containers") +var ( + // ErrInvalidPrimaryIP indicates the NC primary IP is invalid. + ErrInvalidPrimaryIP = errors.New("invalid primary IP") + // ErrInvalidSecondaryIP indicates that a secondary IP on the NC is invalid. + ErrInvalidSecondaryIP = errors.New("invalid secondary IP") + // ErrUnsupportedNCQuantity indicates that the node has an unsupported nummber of Network Containers attached. + ErrUnsupportedNCQuantity = errors.New("unsupported number of network containers") +) // CRDStatusToNCRequest translates a crd status to createnetworkcontainer request -func CRDStatusToNCRequest(status v1alpha.NodeNetworkConfigStatus) (cns.CreateNetworkContainerRequest, error) { +func CRDStatusToNCRequest(status *v1alpha.NodeNetworkConfigStatus) (cns.CreateNetworkContainerRequest, error) { // if NNC has no NC, return an empty request if len(status.NetworkContainers) == 0 { return cns.CreateNetworkContainerRequest{}, nil @@ -29,7 +34,7 @@ func CRDStatusToNCRequest(status v1alpha.NodeNetworkConfigStatus) (cns.CreateNet ip := net.ParseIP(nc.PrimaryIP) if ip == nil { - return cns.CreateNetworkContainerRequest{}, fmt.Errorf("invalid PrimaryIP %s", nc.PrimaryIP) + return cns.CreateNetworkContainerRequest{}, errors.Wrapf(ErrInvalidPrimaryIP, "IP: %s", nc.PrimaryIP) } _, ipNet, err := net.ParseCIDR(nc.SubnetAddressSpace) @@ -48,7 +53,7 @@ func CRDStatusToNCRequest(status v1alpha.NodeNetworkConfigStatus) (cns.CreateNet for _, ipAssignment := range nc.IPAssignments { secondaryIP := net.ParseIP(ipAssignment.IP) if secondaryIP == nil { - return cns.CreateNetworkContainerRequest{}, fmt.Errorf("invalid SecondaryIP %s", ipAssignment.IP) + return cns.CreateNetworkContainerRequest{}, errors.Wrapf(ErrInvalidSecondaryIP, "IP: %s", ipAssignment.IP) } secondaryIPConfigs[ipAssignment.Name] = cns.SecondaryIPConfig{ IPAddress: secondaryIP.String(), diff --git a/cns/singletenantcontroller/conversion_test.go b/cns/singletenantcontroller/conversion_test.go index 2ec095ef12..63383ada75 100644 --- a/cns/singletenantcontroller/conversion_test.go +++ b/cns/singletenantcontroller/conversion_test.go @@ -185,7 +185,7 @@ func TestConvertNNCStatusToNCRequest(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - got, err := CRDStatusToNCRequest(tt.status) + got, err := CRDStatusToNCRequest(&tt.status) if (err != nil) != tt.wantErr { t.Errorf("ConvertNNCStatusToNCRequest() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/cns/singletenantcontroller/reconciler.go b/cns/singletenantcontroller/reconciler.go index a12d9e7284..38bc484d58 100644 --- a/cns/singletenantcontroller/reconciler.go +++ b/cns/singletenantcontroller/reconciler.go @@ -42,15 +42,15 @@ func New(nnccli nncgetter, cnscli cnsclient) *Reconciler { } // Reconcile is called on CRD status changes -func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { - nnc, err := r.nnccli.Get(ctx, request.NamespacedName) +func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { + nnc, err := r.nnccli.Get(ctx, req.NamespacedName) if err != nil { if apierrors.IsNotFound(err) { logger.Printf("[cns-rc] CRD not found, ignoring %v", err) - return reconcile.Result{}, client.IgnoreNotFound(err) + return reconcile.Result{}, errors.Wrapf(client.IgnoreNotFound(err), "NodeNetworkConfig %v not found", req.NamespacedName) } logger.Errorf("[cns-rc] Error retrieving CRD from cache : %v", err) - return reconcile.Result{}, err + return reconcile.Result{}, errors.Wrapf(err, "failed to get NodeNetworkConfig %v", req.NamespacedName) } logger.Printf("[cns-rc] CRD Spec: %v", nnc.Spec) @@ -73,11 +73,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( len(networkContainer.IPAssignments)) // Otherwise, create NC request and hand it off to CNS - ncRequest, err := CRDStatusToNCRequest(nnc.Status) + ncRequest, err := CRDStatusToNCRequest(&nnc.Status) if err != nil { logger.Errorf("[cns-rc] Error translating crd status to nc request %v", err) // requeue - return reconcile.Result{}, err + return reconcile.Result{}, errors.Wrap(err, "failed to convert NNC status to network container request") } responseCode := r.cnscli.CreateOrUpdateNetworkContainerInternal(&ncRequest) @@ -85,7 +85,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( if err != nil { logger.Errorf("[cns-rc] Error creating or updating NC in reconcile: %v", err) // requeue - return reconcile.Result{}, err + return reconcile.Result{}, errors.Wrap(err, "failed to create or update network container") } r.cnscli.UpdateIPAMPoolMonitor(nnc.Status.Scaler, nnc.Spec) diff --git a/cns/singletenantcontroller/reconciler_test.go b/cns/singletenantcontroller/reconciler_test.go new file mode 100644 index 0000000000..b074f2f31a --- /dev/null +++ b/cns/singletenantcontroller/reconciler_test.go @@ -0,0 +1,46 @@ +package kubecontroller + +import ( + "context" + "reflect" + "testing" + + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +func TestReconciler_Reconcile(t *testing.T) { + type fields struct { + cnscli cnsclient + nnccli nncgetter + } + type args struct { + ctx context.Context + request reconcile.Request + } + tests := []struct { + name string + fields fields + args args + want reconcile.Result + wantErr bool + }{ + // TODO: Add test cases. + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + r := &Reconciler{ + cnscli: tt.fields.cnscli, + nnccli: tt.fields.nnccli, + } + got, err := r.Reconcile(tt.args.ctx, tt.args.request) + if (err != nil) != tt.wantErr { + t.Errorf("Reconciler.Reconcile() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("Reconciler.Reconcile() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/crd/nodenetworkconfig/embed.go b/crd/nodenetworkconfig/embed.go index c4132cfce6..30d07b3145 100644 --- a/crd/nodenetworkconfig/embed.go +++ b/crd/nodenetworkconfig/embed.go @@ -18,5 +18,8 @@ var NodeNetworkConfigsYAML []byte // to a CustomResourceDefinition and returns it or an unmarshalling error. func GetNodeNetworkConfigs() (*apiextensionsv1.CustomResourceDefinition, error) { nodeNetworkConfigs := &apiextensionsv1.CustomResourceDefinition{} - return nodeNetworkConfigs, errors.Wrap(yaml.Unmarshal(NodeNetworkConfigsYAML, &nodeNetworkConfigs), "error unmarshalling embedded nnc") + if err := yaml.Unmarshal(NodeNetworkConfigsYAML, &nodeNetworkConfigs); err != nil { + return nil, errors.Wrap(err, "error unmarshalling embedded nnc") + } + return nodeNetworkConfigs, nil } From ad201bfba1787955e7c1e1d4c5de233ad5c0a1e4 Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Wed, 15 Sep 2021 16:52:54 -0500 Subject: [PATCH 09/13] test reconciler Signed-off-by: Evan Baker --- cns/singletenantcontroller/conversion_test.go | 97 +++++----- cns/singletenantcontroller/reconciler.go | 20 +-- cns/singletenantcontroller/reconciler_test.go | 169 +++++++++++++++--- 3 files changed, 199 insertions(+), 87 deletions(-) diff --git a/cns/singletenantcontroller/conversion_test.go b/cns/singletenantcontroller/conversion_test.go index 63383ada75..c02725c027 100644 --- a/cns/singletenantcontroller/conversion_test.go +++ b/cns/singletenantcontroller/conversion_test.go @@ -23,6 +23,54 @@ const ( version = 1 ) +var invalidStatusMultiNC = v1alpha.NodeNetworkConfigStatus{ + NetworkContainers: []v1alpha.NetworkContainer{ + {}, + {}, + }, +} + +var validStatus = v1alpha.NodeNetworkConfigStatus{ + NetworkContainers: []v1alpha.NetworkContainer{ + { + PrimaryIP: primaryIP, + ID: ncID, + IPAssignments: []v1alpha.IPAssignment{ + { + Name: uuid, + IP: testSecIP, + }, + }, + SubnetName: subnetName, + DefaultGateway: defaultGateway, + SubnetAddressSpace: subnetAddressSpace, + Version: version, + }, + }, + Scaler: v1alpha.Scaler{ + BatchSize: 1, + }, +} + +var validRequest = cns.CreateNetworkContainerRequest{ + Version: strconv.FormatInt(version, 10), + IPConfiguration: cns.IPConfiguration{ + GatewayIPAddress: defaultGateway, + IPSubnet: cns.IPSubnet{ + PrefixLength: uint8(subnetPrefixLen), + IPAddress: primaryIP, + }, + }, + NetworkContainerid: ncID, + NetworkContainerType: cns.Docker, + SecondaryIPConfigs: map[string]cns.SecondaryIPConfig{ + uuid: { + IPAddress: testSecIP, + NCVersion: version, + }, + }, +} + func TestConvertNNCStatusToNCRequest(t *testing.T) { tests := []struct { name string @@ -37,13 +85,8 @@ func TestConvertNNCStatusToNCRequest(t *testing.T) { ncreq: cns.CreateNetworkContainerRequest{}, }, { - name: ">1 nc", - status: v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - {}, - {}, - }, - }, + name: ">1 nc", + status: invalidStatusMultiNC, wantErr: true, }, { @@ -142,44 +185,10 @@ func TestConvertNNCStatusToNCRequest(t *testing.T) { wantErr: true, }, { - name: "valid", - status: v1alpha.NodeNetworkConfigStatus{ - NetworkContainers: []v1alpha.NetworkContainer{ - { - PrimaryIP: primaryIP, - ID: ncID, - IPAssignments: []v1alpha.IPAssignment{ - { - Name: uuid, - IP: testSecIP, - }, - }, - SubnetName: subnetName, - DefaultGateway: defaultGateway, - SubnetAddressSpace: subnetAddressSpace, - Version: version, - }, - }, - }, + name: "valid", + status: validStatus, wantErr: false, - ncreq: cns.CreateNetworkContainerRequest{ - Version: strconv.FormatInt(version, 10), - IPConfiguration: cns.IPConfiguration{ - GatewayIPAddress: defaultGateway, - IPSubnet: cns.IPSubnet{ - PrefixLength: uint8(subnetPrefixLen), - IPAddress: primaryIP, - }, - }, - NetworkContainerid: ncID, - NetworkContainerType: cns.Docker, - SecondaryIPConfigs: map[string]cns.SecondaryIPConfig{ - uuid: { - IPAddress: testSecIP, - NCVersion: version, - }, - }, - }, + ncreq: validRequest, }, } for _, tt := range tests { diff --git a/cns/singletenantcontroller/reconciler.go b/cns/singletenantcontroller/reconciler.go index 38bc484d58..28170a61c5 100644 --- a/cns/singletenantcontroller/reconciler.go +++ b/cns/singletenantcontroller/reconciler.go @@ -19,13 +19,12 @@ import ( ) type cnsclient interface { - ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo, scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) error - CreateOrUpdateNC(ncRequest cns.CreateNetworkContainerRequest) error - UpdateIPAMPoolMonitor(scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) + CreateOrUpdateNC(cns.CreateNetworkContainerRequest) error + UpdateIPAMPoolMonitor(v1alpha.Scaler, v1alpha.NodeNetworkConfigSpec) } type nncgetter interface { - Get(ctx context.Context, key types.NamespacedName) (*v1alpha.NodeNetworkConfig, error) + Get(context.Context, types.NamespacedName) (*v1alpha.NodeNetworkConfig, error) } // Reconciler watches for CRD status changes @@ -61,18 +60,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco return reconcile.Result{}, nil } - networkContainer := nnc.Status.NetworkContainers[0] - logger.Printf("[cns-rc] CRD Status: NcId: [%s], Version: [%d], podSubnet: [%s], Subnet CIDR: [%s], "+ - "Gateway Addr: [%s], Primary IP: [%s], SecondaryIpsCount: [%d]", - networkContainer.ID, - networkContainer.Version, - networkContainer.SubnetName, - networkContainer.SubnetAddressSpace, - networkContainer.DefaultGateway, - networkContainer.PrimaryIP, - len(networkContainer.IPAssignments)) - - // Otherwise, create NC request and hand it off to CNS + // Create NC request and hand it off to CNS ncRequest, err := CRDStatusToNCRequest(&nnc.Status) if err != nil { logger.Errorf("[cns-rc] Error translating crd status to nc request %v", err) diff --git a/cns/singletenantcontroller/reconciler_test.go b/cns/singletenantcontroller/reconciler_test.go index b074f2f31a..f77636ff16 100644 --- a/cns/singletenantcontroller/reconciler_test.go +++ b/cns/singletenantcontroller/reconciler_test.go @@ -2,45 +2,160 @@ package kubecontroller import ( "context" - "reflect" "testing" + "github.com/Azure/azure-container-networking/cns" + "github.com/Azure/azure-container-networking/cns/logger" + "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" + "github.com/pkg/errors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -func TestReconciler_Reconcile(t *testing.T) { - type fields struct { - cnscli cnsclient - nnccli nncgetter - } - type args struct { - ctx context.Context - request reconcile.Request - } +type cnsClientState struct { + req cns.CreateNetworkContainerRequest + scaler v1alpha.Scaler + spec v1alpha.NodeNetworkConfigSpec +} + +type mockCNSClient struct { + state cnsClientState + createOrUpdateNC func(cns.CreateNetworkContainerRequest) error + updateIPAMPoolMonitor func(v1alpha.Scaler, v1alpha.NodeNetworkConfigSpec) +} + +//nolint:gocritic // ignore hugeParam pls +func (m *mockCNSClient) CreateOrUpdateNC(req cns.CreateNetworkContainerRequest) error { + m.state.req = req + return m.createOrUpdateNC(req) +} + +func (m *mockCNSClient) UpdateIPAMPoolMonitor(scaler v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) { + m.state.scaler = scaler + m.state.spec = spec + m.updateIPAMPoolMonitor(scaler, spec) +} + +type mockNCGetter struct { + get func(context.Context, types.NamespacedName) (*v1alpha.NodeNetworkConfig, error) +} + +func (m *mockNCGetter) Get(ctx context.Context, key types.NamespacedName) (*v1alpha.NodeNetworkConfig, error) { + return m.get(ctx, key) +} + +func TestReconcile(t *testing.T) { + logger.InitLogger("", 0, 0, "") tests := []struct { - name string - fields fields - args args - want reconcile.Result - wantErr bool + name string + in reconcile.Request + ncGetter mockNCGetter + cnsClient mockCNSClient + want reconcile.Result + wantCNSClientState cnsClientState + wantErr bool }{ - // TODO: Add test cases. + { + name: "unknown get err", + ncGetter: mockNCGetter{ + get: func(context.Context, types.NamespacedName) (*v1alpha.NodeNetworkConfig, error) { + return nil, errors.New("") + }, + }, + wantErr: true, + }, + { + name: "not found", + ncGetter: mockNCGetter{ + get: func(context.Context, types.NamespacedName) (*v1alpha.NodeNetworkConfig, error) { + return nil, apierrors.NewNotFound(schema.GroupResource{}, "") + }, + }, + wantErr: false, + }, + { + name: "no NCs", + ncGetter: mockNCGetter{ + get: func(context.Context, types.NamespacedName) (*v1alpha.NodeNetworkConfig, error) { + return &v1alpha.NodeNetworkConfig{}, nil + }, + }, + wantErr: false, + }, + { + name: "invalid NCs", + ncGetter: mockNCGetter{ + get: func(context.Context, types.NamespacedName) (*v1alpha.NodeNetworkConfig, error) { + return &v1alpha.NodeNetworkConfig{ + Status: invalidStatusMultiNC, + }, nil + }, + }, + wantErr: true, + }, + { + name: "err in CreateOrUpdateNC", + ncGetter: mockNCGetter{ + get: func(context.Context, types.NamespacedName) (*v1alpha.NodeNetworkConfig, error) { + return &v1alpha.NodeNetworkConfig{ + Status: validStatus, + }, nil + }, + }, + cnsClient: mockCNSClient{ + createOrUpdateNC: func(cns.CreateNetworkContainerRequest) error { + return errors.New("") + }, + }, + wantErr: true, + wantCNSClientState: cnsClientState{ + req: validRequest, + }, + }, + { + name: "success", + ncGetter: mockNCGetter{ + get: func(context.Context, types.NamespacedName) (*v1alpha.NodeNetworkConfig, error) { + return &v1alpha.NodeNetworkConfig{ + Status: validStatus, + Spec: v1alpha.NodeNetworkConfigSpec{ + RequestedIPCount: 1, + }, + }, nil + }, + }, + cnsClient: mockCNSClient{ + createOrUpdateNC: func(cns.CreateNetworkContainerRequest) error { + return nil + }, + updateIPAMPoolMonitor: func(v1alpha.Scaler, v1alpha.NodeNetworkConfigSpec) {}, + }, + wantErr: false, + wantCNSClientState: cnsClientState{ + req: validRequest, + scaler: validStatus.Scaler, + spec: v1alpha.NodeNetworkConfigSpec{ + RequestedIPCount: 1, + }, + }, + }, } for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - r := &Reconciler{ - cnscli: tt.fields.cnscli, - nnccli: tt.fields.nnccli, - } - got, err := r.Reconcile(tt.args.ctx, tt.args.request) - if (err != nil) != tt.wantErr { - t.Errorf("Reconciler.Reconcile() error = %v, wantErr %v", err, tt.wantErr) - return - } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("Reconciler.Reconcile() = %v, want %v", got, tt.want) + r := New(&tt.ncGetter, &tt.cnsClient) + got, err := r.Reconcile(context.Background(), tt.in) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) } + assert.Equal(t, tt.want, got) + assert.Equal(t, tt.wantCNSClientState, tt.cnsClient.state) }) } } From df52a79dcdf43b8a9ed7e4818b49efbcffb72023 Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Fri, 17 Sep 2021 12:13:10 -0500 Subject: [PATCH 10/13] address review comments Signed-off-by: Evan Baker --- cns/service/main.go | 10 +++------- crd/nodenetworkconfig/client.go | 5 +++-- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/cns/service/main.go b/cns/service/main.go index f12ece87b6..a9a5d36033 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -793,10 +793,9 @@ func initCNS(ctx context.Context, cli nodeNetworkConfigGetter, ncReconciler ncSt // Get nnc using direct client nnc, err := cli.Get(ctx) if err != nil { - // If the CRD is not defined, exit + if crd.IsNotDefined(err) { - logger.Errorf("CRD is not defined on cluster: %v", err) - os.Exit(1) + return errors.Wrap(err, "failed to get NNC during init CNS state") } if nnc == nil { @@ -807,10 +806,7 @@ func initCNS(ctx context.Context, cli nodeNetworkConfigGetter, ncReconciler ncSt // If instance of crd is not found, pass nil to CNSClient if client.IgnoreNotFound(err) == nil { err = ncReconciler.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec) - if err != nil { - return errors.Wrap(err, "failed to reconcile NC state") - } - return nil + return errors.Wrap(err, "failed to reconcile NC state") } // If it's any other error, log it and return diff --git a/crd/nodenetworkconfig/client.go b/crd/nodenetworkconfig/client.go index 1742373049..29f0bcbc42 100644 --- a/crd/nodenetworkconfig/client.go +++ b/crd/nodenetworkconfig/client.go @@ -62,7 +62,8 @@ func (c *Client) create(ctx context.Context, res *v1.CustomResourceDefinition) ( // Get returns the NodeNetworkConfig identified by the NamespacedName. func (c *Client) Get(ctx context.Context, key types.NamespacedName) (*v1alpha.NodeNetworkConfig, error) { nodeNetworkConfig := &v1alpha.NodeNetworkConfig{} - return nodeNetworkConfig, errors.Wrapf(c.nnccli.Get(ctx, key, nodeNetworkConfig), "failed to get nnc %v", key) + err := c.nnccli.Get(ctx, key, nodeNetworkConfig) + return nodeNetworkConfig, errors.Wrapf(err, "failed to get nnc %v", key) } // Install installs the embedded NodeNetworkConfig CRD definition in the cluster. @@ -127,7 +128,7 @@ func (c *Client) PatchSpec(ctx context.Context, key types.NamespacedName, spec * func (c *Client) UpdateSpec(ctx context.Context, key types.NamespacedName, spec *v1alpha.NodeNetworkConfigSpec) (*v1alpha.NodeNetworkConfig, error) { nnc, err := c.Get(ctx, key) if err != nil { - return nil, errors.Wrap(err, "failed to update nnc") + return nil, errors.Wrap(err, "failed to get nnc") } spec.DeepCopyInto(&nnc.Spec) if err := c.nnccli.Update(ctx, nnc); err != nil { From 74d2f55961d335a5d0d314ed9e6344d75680a82a Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Mon, 27 Sep 2021 17:10:38 -0500 Subject: [PATCH 11/13] rebase cleanup Signed-off-by: Evan Baker --- cns/service/main.go | 18 ++++++++---------- cns/singletenantcontroller/reconciler.go | 23 ++++++++++++++--------- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/cns/service/main.go b/cns/service/main.go index a9a5d36033..0c56627bd7 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -21,7 +21,6 @@ import ( "github.com/Azure/azure-container-networking/cnm/ipam" "github.com/Azure/azure-container-networking/cnm/network" "github.com/Azure/azure-container-networking/cns" - "github.com/Azure/azure-container-networking/cns/client/httpapi" cnscli "github.com/Azure/azure-container-networking/cns/cmd/cli" "github.com/Azure/azure-container-networking/cns/cnireconciler" cni "github.com/Azure/azure-container-networking/cns/cnireconciler" @@ -36,6 +35,7 @@ import ( "github.com/Azure/azure-container-networking/cns/nmagentclient" "github.com/Azure/azure-container-networking/cns/restserver" kubecontroller "github.com/Azure/azure-container-networking/cns/singletenantcontroller" + cnstypes "github.com/Azure/azure-container-networking/cns/types" acn "github.com/Azure/azure-container-networking/common" "github.com/Azure/azure-container-networking/crd" "github.com/Azure/azure-container-networking/crd/nodenetworkconfig" @@ -784,7 +784,7 @@ type nodeNetworkConfigGetter interface { } type ncStateReconciler interface { - ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo, scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) error + ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo, scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) cnstypes.ResponseCode } // TODO(rbtr) where should this live?? @@ -805,7 +805,7 @@ func initCNS(ctx context.Context, cli nodeNetworkConfigGetter, ncReconciler ncSt // If instance of crd is not found, pass nil to CNSClient if client.IgnoreNotFound(err) == nil { - err = ncReconciler.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec) + err = restserver.ResponseCodeToError(ncReconciler.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec)) return errors.Wrap(err, "failed to reconcile NC state") } @@ -816,7 +816,7 @@ func initCNS(ctx context.Context, cli nodeNetworkConfigGetter, ncReconciler ncSt // If there are no NCs, pass nil to CNSClient if len(nnc.Status.NetworkContainers) == 0 { - err = ncReconciler.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec) + err = restserver.ResponseCodeToError(ncReconciler.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec)) if err != nil { return errors.Wrap(err, "failed to reconcile NC state") } @@ -843,7 +843,8 @@ func initCNS(ctx context.Context, cli nodeNetworkConfigGetter, ncReconciler ncSt // errors.Wrap provides additional context, and return nil if the err input arg is nil // Call cnsclient init cns passing those two things. - return errors.Wrap(ncReconciler.ReconcileNCState(&ncRequest, podInfoByIP, nnc.Status.Scaler, nnc.Spec), "err in CNS reconciliation") + err = restserver.ResponseCodeToError(ncReconciler.ReconcileNCState(&ncRequest, podInfoByIP, nnc.Status.Scaler, nnc.Spec)) + return errors.Wrap(err, "err in CNS reconciliation") } // InitializeCRDState builds and starts the CRD controllers. @@ -882,10 +883,7 @@ func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cn // initialize the ipam pool monitor httpRestServiceImplementation.IPAMPoolMonitor = ipampoolmonitor.NewCNSIPAMPoolMonitor(httpRestServiceImplementation, scopedcli) - cnsclient := &httpapi.Client{ - RestService: httpRestServiceImplementation, - } - err = initCNS(ctx, scopedcli, cnsclient) + err = initCNS(ctx, scopedcli, httpRestServiceImplementation) if err != nil { return errors.Wrap(err, "failed to initialize CNS state") } @@ -898,7 +896,7 @@ func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cn if err != nil { return errors.Wrap(err, "failed to create manager") } - reconciler := kubecontroller.New(nnccli, cnsclient) + reconciler := kubecontroller.New(nnccli, httpRestServiceImplementation, httpRestServiceImplementation.IPAMPoolMonitor) if err := reconciler.SetupWithManager(manager, nodeName); err != nil { return err } diff --git a/cns/singletenantcontroller/reconciler.go b/cns/singletenantcontroller/reconciler.go index 28170a61c5..a5f38a3d2b 100644 --- a/cns/singletenantcontroller/reconciler.go +++ b/cns/singletenantcontroller/reconciler.go @@ -6,7 +6,7 @@ import ( "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/logger" "github.com/Azure/azure-container-networking/cns/restserver" - "github.com/Azure/azure-container-networking/crd/nodenetworkconfig" + cnstypes "github.com/Azure/azure-container-networking/cns/types" "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" "github.com/pkg/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -19,8 +19,11 @@ import ( ) type cnsclient interface { - CreateOrUpdateNC(cns.CreateNetworkContainerRequest) error - UpdateIPAMPoolMonitor(v1alpha.Scaler, v1alpha.NodeNetworkConfigSpec) + CreateOrUpdateNetworkContainerInternal(*cns.CreateNetworkContainerRequest) cnstypes.ResponseCode +} + +type ipampoolmonitorclient interface { + Update(scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) } type nncgetter interface { @@ -29,14 +32,16 @@ type nncgetter interface { // Reconciler watches for CRD status changes type Reconciler struct { - cnscli cnsclient - nnccli nncgetter + cnscli cnsclient + ipampoolmonitorcli ipampoolmonitorclient + nnccli nncgetter } -func New(nnccli nncgetter, cnscli cnsclient) *Reconciler { +func New(nnccli nncgetter, cnscli cnsclient, ipampipampoolmonitorcli ipampoolmonitorclient) *Reconciler { return &Reconciler{ - cnscli: cnscli, - nnccli: nnccli, + cnscli: cnscli, + ipampoolmonitorcli: ipampipampoolmonitorcli, + nnccli: nnccli, } } @@ -76,7 +81,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco return reconcile.Result{}, errors.Wrap(err, "failed to create or update network container") } - r.cnscli.UpdateIPAMPoolMonitor(nnc.Status.Scaler, nnc.Spec) + r.ipampoolmonitorcli.Update(nnc.Status.Scaler, nnc.Spec) // record assigned IPs metric assignedIPs.Set(float64(len(nnc.Status.NetworkContainers[0].IPAssignments))) From 64388849e516dd0f6c5f9bbec463d86216fa1c10 Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Tue, 5 Oct 2021 15:17:18 -0500 Subject: [PATCH 12/13] fix test Signed-off-by: Evan Baker --- cns/restserver/internalapi.go | 4 +-- cns/singletenantcontroller/reconciler_test.go | 31 ++++++++++--------- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/cns/restserver/internalapi.go b/cns/restserver/internalapi.go index 41f84e8fce..ae96b03318 100644 --- a/cns/restserver/internalapi.go +++ b/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 diff --git a/cns/singletenantcontroller/reconciler_test.go b/cns/singletenantcontroller/reconciler_test.go index f77636ff16..b416a0915f 100644 --- a/cns/singletenantcontroller/reconciler_test.go +++ b/cns/singletenantcontroller/reconciler_test.go @@ -6,6 +6,7 @@ import ( "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/logger" + cnstypes "github.com/Azure/azure-container-networking/cns/types" "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" "github.com/pkg/errors" "github.com/stretchr/testify/assert" @@ -17,27 +18,27 @@ import ( ) type cnsClientState struct { - req cns.CreateNetworkContainerRequest + req *cns.CreateNetworkContainerRequest scaler v1alpha.Scaler spec v1alpha.NodeNetworkConfigSpec } type mockCNSClient struct { - state cnsClientState - createOrUpdateNC func(cns.CreateNetworkContainerRequest) error - updateIPAMPoolMonitor func(v1alpha.Scaler, v1alpha.NodeNetworkConfigSpec) + state cnsClientState + createOrUpdateNC func(*cns.CreateNetworkContainerRequest) cnstypes.ResponseCode + update func(v1alpha.Scaler, v1alpha.NodeNetworkConfigSpec) } //nolint:gocritic // ignore hugeParam pls -func (m *mockCNSClient) CreateOrUpdateNC(req cns.CreateNetworkContainerRequest) error { +func (m *mockCNSClient) CreateOrUpdateNetworkContainerInternal(req *cns.CreateNetworkContainerRequest) cnstypes.ResponseCode { m.state.req = req return m.createOrUpdateNC(req) } -func (m *mockCNSClient) UpdateIPAMPoolMonitor(scaler v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) { +func (m *mockCNSClient) Update(scaler v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) { m.state.scaler = scaler m.state.spec = spec - m.updateIPAMPoolMonitor(scaler, spec) + m.update(scaler, spec) } type mockNCGetter struct { @@ -107,13 +108,13 @@ func TestReconcile(t *testing.T) { }, }, cnsClient: mockCNSClient{ - createOrUpdateNC: func(cns.CreateNetworkContainerRequest) error { - return errors.New("") + createOrUpdateNC: func(*cns.CreateNetworkContainerRequest) cnstypes.ResponseCode { + return cnstypes.UnexpectedError }, }, wantErr: true, wantCNSClientState: cnsClientState{ - req: validRequest, + req: &validRequest, }, }, { @@ -129,14 +130,14 @@ func TestReconcile(t *testing.T) { }, }, cnsClient: mockCNSClient{ - createOrUpdateNC: func(cns.CreateNetworkContainerRequest) error { - return nil + createOrUpdateNC: func(*cns.CreateNetworkContainerRequest) cnstypes.ResponseCode { + return cnstypes.Success }, - updateIPAMPoolMonitor: func(v1alpha.Scaler, v1alpha.NodeNetworkConfigSpec) {}, + update: func(v1alpha.Scaler, v1alpha.NodeNetworkConfigSpec) {}, }, wantErr: false, wantCNSClientState: cnsClientState{ - req: validRequest, + req: &validRequest, scaler: validStatus.Scaler, spec: v1alpha.NodeNetworkConfigSpec{ RequestedIPCount: 1, @@ -147,7 +148,7 @@ func TestReconcile(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - r := New(&tt.ncGetter, &tt.cnsClient) + r := New(&tt.ncGetter, &tt.cnsClient, &tt.cnsClient) got, err := r.Reconcile(context.Background(), tt.in) if tt.wantErr { require.Error(t, err) From 13809f6635d9a5e88474348128facd779f39d1f5 Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Wed, 6 Oct 2021 13:38:56 -0500 Subject: [PATCH 13/13] address review feedback Signed-off-by: Evan Baker --- cns/service/main.go | 16 +++------------- cns/singletenantcontroller/reconciler.go | 14 +++++++------- cns/singletenantcontroller/reconciler_test.go | 2 +- 3 files changed, 11 insertions(+), 21 deletions(-) diff --git a/cns/service/main.go b/cns/service/main.go index 0c56627bd7..7a6fdfa146 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -798,11 +798,6 @@ func initCNS(ctx context.Context, cli nodeNetworkConfigGetter, ncReconciler ncSt return errors.Wrap(err, "failed to get NNC during init CNS state") } - if nnc == nil { - logger.Errorf("NodeNetworkConfig is not present on cluster") - return nil - } - // If instance of crd is not found, pass nil to CNSClient if client.IgnoreNotFound(err) == nil { err = restserver.ResponseCodeToError(ncReconciler.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec)) @@ -810,23 +805,18 @@ func initCNS(ctx context.Context, cli nodeNetworkConfigGetter, ncReconciler ncSt } // If it's any other error, log it and return - logger.Errorf("Error when getting nodeNetConfig using direct client when initializing cns state: %v", err) - return err + return errors.Wrap(err, "error getting NodeNetworkConfig when initializing CNS state") } // If there are no NCs, pass nil to CNSClient if len(nnc.Status.NetworkContainers) == 0 { err = restserver.ResponseCodeToError(ncReconciler.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec)) - if err != nil { - return errors.Wrap(err, "failed to reconcile NC state") - } - return nil + return errors.Wrap(err, "failed to reconcile NC state") } // Convert to CreateNetworkContainerRequest ncRequest, err := kubecontroller.CRDStatusToNCRequest(&nnc.Status) if err != nil { - logger.Errorf("Error when converting nodeNetConfig status into CreateNetworkContainerRequest: %v", err) return errors.Wrap(err, "failed to convert NNC status to network container request") } @@ -896,7 +886,7 @@ func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cn if err != nil { return errors.Wrap(err, "failed to create manager") } - reconciler := kubecontroller.New(nnccli, httpRestServiceImplementation, httpRestServiceImplementation.IPAMPoolMonitor) + reconciler := kubecontroller.NewReconciler(nnccli, httpRestServiceImplementation, httpRestServiceImplementation.IPAMPoolMonitor) if err := reconciler.SetupWithManager(manager, nodeName); err != nil { return err } diff --git a/cns/singletenantcontroller/reconciler.go b/cns/singletenantcontroller/reconciler.go index a5f38a3d2b..99249efb8a 100644 --- a/cns/singletenantcontroller/reconciler.go +++ b/cns/singletenantcontroller/reconciler.go @@ -18,26 +18,26 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -type cnsclient interface { +type cnsClient interface { CreateOrUpdateNetworkContainerInternal(*cns.CreateNetworkContainerRequest) cnstypes.ResponseCode } -type ipampoolmonitorclient interface { +type ipamPoolMonitorClient interface { Update(scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) } -type nncgetter interface { +type nncGetter interface { Get(context.Context, types.NamespacedName) (*v1alpha.NodeNetworkConfig, error) } // Reconciler watches for CRD status changes type Reconciler struct { - cnscli cnsclient - ipampoolmonitorcli ipampoolmonitorclient - nnccli nncgetter + cnscli cnsClient + ipampoolmonitorcli ipamPoolMonitorClient + nnccli nncGetter } -func New(nnccli nncgetter, cnscli cnsclient, ipampipampoolmonitorcli ipampoolmonitorclient) *Reconciler { +func NewReconciler(nnccli nncGetter, cnscli cnsClient, ipampipampoolmonitorcli ipamPoolMonitorClient) *Reconciler { return &Reconciler{ cnscli: cnscli, ipampoolmonitorcli: ipampipampoolmonitorcli, diff --git a/cns/singletenantcontroller/reconciler_test.go b/cns/singletenantcontroller/reconciler_test.go index b416a0915f..c82ee24755 100644 --- a/cns/singletenantcontroller/reconciler_test.go +++ b/cns/singletenantcontroller/reconciler_test.go @@ -148,7 +148,7 @@ func TestReconcile(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - r := New(&tt.ncGetter, &tt.cnsClient, &tt.cnsClient) + r := NewReconciler(&tt.ncGetter, &tt.cnsClient, &tt.cnsClient) got, err := r.Reconcile(context.Background(), tt.in) if tt.wantErr { require.Error(t, err)