CNI using CNS IPAM #597

matmerr · 2020-06-29T19:32:49Z

Reason for Change:

Using the CNS IPAM found in #584

Issue Fixed:

Requirements:

includes documentation
uses conventional commit messages
adds unit tests

Notes:

codecov · 2020-07-08T17:13:10Z

Codecov Report

Merging #597 into master will decrease coverage by 3.14%.
The diff coverage is 8.69%.

@@            Coverage Diff             @@
##           master     #597      +/-   ##
==========================================
- Coverage   41.72%   38.57%   -3.15%     
==========================================
  Files          72       78       +6     
  Lines        9474    10416     +942     
==========================================
+ Hits         3953     4018      +65     
- Misses       5052     5907     +855     
- Partials      469      491      +22

cni/network/network.go

cni/network/invoker_cns.go

cni/network/network.go

paulgmiller · 2020-07-31T16:19:26Z

cni/network/invoker.go

+	cniTypesCurr "github.com/containernetworking/cni/pkg/types/current"
+)
+
+type IPAMInvoker interface {


Document your interface? What is deletepoolonerrr? for example?

Here's a question from left field. Is there a future where we want other cni's invoking cns for ipam? Or would we want them to use the crd? If we think they might want to hit up cns making this interface really clear might pay future dividedends.

Also that first result is ipv4 second is ipv6. (hopefully I die before there is ipv8).

paulgmiller · 2020-07-31T16:20:52Z

cni/network/invoker_azure.go

+	cniTypesCurr "github.com/containernetworking/cni/pkg/types/current"
+)
+
+type AzureInvoker struct {


nit: AzureIpamInvoker or does it do more?

paulgmiller · 2020-07-31T16:25:27Z

cni/network/network.go

 		}
 	}

+	switch nwCfg.Ipam.Type {


This is coming from the json that gets laid down with the cni plugin I assume?

Are we allowed to look for the extiance of any other files to make this decision?

paulgmiller · 2020-07-31T16:48:35Z

cni/network/invoker_azure.go

+		nwCfg6.Ipam.Type = ipamV6
+
+		if len(nwInfo.Subnets) > 1 {
+			nwCfg6.Ipam.Subnet = nwInfo.Subnets[1].Prefix.String()


magic number index 1. Know this is old code you're just moving.

Do we have some backlog item to try and prove ipv6 works with cns?

paulgmiller · 2020-07-31T16:51:26Z

cni/network/invoker_cns.go

+		return err
+	}
+
+	return invoker.cnsClient.ReleaseIPAddress(orchestratorContext)


what happens on err? Do we leak, retry, does cns notices and free on its own?

cni/network/network.go

paulgmiller · 2020-07-31T16:55:13Z

cni/network/invoker_azure.go

+
+	if address.IP.To4() != nil {
+
+		if isDeletePoolOnError {


kinda of curious why this is important.

paulgmiller

Thanks this help me leared some. Not sure I'm qualified to sign off but looking close in my eyes :)

tamilmani1989 · 2020-08-03T17:34:16Z

cni/network/invoker.go

+)
+
+type IPAMInvoker interface {
+	Add(args *cniSkel.CmdArgs, nwCfg *cni.NetworkConfig, nwInfo network.NetworkInfo, isDeletePoolOnError bool) (*cniTypesCurr.Result, *cniTypesCurr.Result, error)


networkinfo is more specific to azure case. can we intialize that only as part of azureinvoker and remove from interface calls?

can we pass something like options map[string]string?

added an options map, but nwInfo seems pretty ubiquitous throughout the ADD/DELETE calls

tamilmani1989 · 2020-08-03T17:44:30Z

cni/network/invoker_cns.go

+	result.IPs = make([]*cniTypesCurr.IPConfig, 1)
+	result.IPs[0] = &cniTypesCurr.IPConfig{
+		Address: resultIPnet,
+	}


instead of doing this way, can we initialize cniTypesCurr.IPConfig and then append to array..so that it can be extensible

ipconfig := &cniTypesCurr.IPConfig{} var ipconfigs []*cniTypesCurr.IPConfig ipconfigs = append(ipconfigs, ipconfig)

updating to add gwip and dns

cni/network/invoker_cns.go

thatmattlong

Can we have the CNS client constructed to talk to CNS over the Node IP instead of localhost? @pjohnst5 's PR for the CNS Helm chart has CNS listening on the Node IP only, and doesn't not bind to 127.0.0.1.

matmerr · 2020-08-27T21:07:55Z

cni/network/invoker_cns.go

+	}
+
+	// set host ip interface name
+	hostInterfaceName, err := getHostInterfaceName(hostIPNet, hostIP)


reuse FindMasterInterface

matmerr · 2020-08-27T21:21:26Z

cni/network/invoker_cns.go

+}
+
+// SetNCAddressSpaceOnHostBrige Add's the NC subnet space to the primary interface
+func SetNCAddressSpaceOnHostBrige(ncSubnetAddressSpace string, ncSubnetPrefix uint8, hostPrimaryIfName string) error {


Sent Route for NC subnet on host bridge

pjohnst5 · 2020-09-24T18:45:19Z

cni/installer/job-agent.yaml

+              - matchExpressions:
+                  - key: node-role.kubernetes.io/agent
+                    operator: Exists
+      tolerations:


I don't think you'll need this toleration on the agent installer

tamilmani1989

gave feedback that will be addressed in different PR

ramiro-gamarra · 2020-09-25T15:01:59Z

cni/installer/Dockerfile

@@ -0,0 +1,14 @@
+FROM golang as build


This is the closest I could comment on this :). There's a busybox blob being checked in which should be removed.

configuring CNS to listen localhost is going to be configured with the deployment

* Configure CNI to use CNS IPAM

matmerr added work-in-progress cni Related to CNI. labels Jun 29, 2020

matmerr marked this pull request as ready for review July 17, 2020 01:23

matmerr commented Jul 20, 2020

View reviewed changes

cni/network/network.go Outdated Show resolved Hide resolved

matmerr commented Jul 20, 2020

View reviewed changes

cni/network/network.go Outdated Show resolved Hide resolved

matmerr commented Jul 21, 2020

View reviewed changes

cni/network/invoker_cns.go Outdated Show resolved Hide resolved

matmerr commented Jul 21, 2020

View reviewed changes

cni/network/invoker_cns.go Outdated Show resolved Hide resolved

matmerr commented Jul 22, 2020

View reviewed changes

cni/network/network.go Outdated Show resolved Hide resolved

paulgmiller reviewed Jul 31, 2020

View reviewed changes

cni/network/network.go Outdated Show resolved Hide resolved

paulgmiller reviewed Jul 31, 2020

View reviewed changes

tamilmani1989 requested changes Aug 3, 2020

View reviewed changes

matmerr force-pushed the cniwithcnsipam branch from 6eb5bec to 0129d6f Compare August 3, 2020 22:37

thatmattlong reviewed Aug 4, 2020

View reviewed changes

cni/network/invoker_cns.go Outdated Show resolved Hide resolved

thatmattlong previously requested changes Aug 4, 2020

View reviewed changes

matmerr force-pushed the cniwithcnsipam branch 2 times, most recently from 9c747c5 to aa653c5 Compare August 17, 2020 15:48

matmerr requested review from tamilmani1989 and thatmattlong August 19, 2020 03:53

matmerr force-pushed the cniwithcnsipam branch 3 times, most recently from 4e59e03 to 936eb1a Compare August 27, 2020 20:34

matmerr commented Aug 27, 2020

View reviewed changes

matmerr added 16 commits September 23, 2020 10:33

remove extra logging statements

b15aec0

pass options to invoker

91cbea6

use podipinfo

2bad2ab

snatting and routes

790f57f

address feedback and minor refactor

834dfb2

fix test

5b4b2a9

move route calls

69e8339

snat dns

3ce720b

remove snat rules

8ed7053

remove unused code

0bb825f

installer with ipam type

8e083d8

installer tune excempt bins

6773b21

update job image version

76482a2

installers

3e63683

remove unused installer test

e432488

ds installer

fdcdec7

pjohnst5 reviewed Sep 24, 2020

View reviewed changes

matmerr force-pushed the cniwithcnsipam branch from f61f54f to 997657a Compare September 24, 2020 22:56

tamilmani1989 previously approved these changes Sep 24, 2020

View reviewed changes

matmerr dismissed tamilmani1989’s stale review via a181bed September 24, 2020 23:08

ramiro-gamarra reviewed Sep 25, 2020

View reviewed changes

address feedback and update vendor

3ce65c2

matmerr force-pushed the cniwithcnsipam branch 2 times, most recently from a73662f to 240ea0f Compare September 25, 2020 23:21

subnet

c887c70

matmerr force-pushed the cniwithcnsipam branch from 240ea0f to c887c70 Compare September 25, 2020 23:21

tamilmani1989 approved these changes Sep 29, 2020

View reviewed changes

matmerr merged commit 41232c1 into Azure:master Sep 29, 2020

neaggarwMS pushed a commit to neaggarwMS/azure-container-networking that referenced this pull request Nov 13, 2020

CNI using CNS IPAM (Azure#597)

e75616c

* Configure CNI to use CNS IPAM

CNI using CNS IPAM #597

CNI using CNS IPAM #597

Uh oh!

Conversation

matmerr commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulgmiller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thatmattlong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tamilmani1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

matmerr commented Jun 29, 2020 •

edited

Loading

codecov bot commented Jul 8, 2020 •

edited

Loading