feat: CNS RequestIPAddress branching for MT/V2 #2300

nddq · 2023-10-17T19:26:08Z

Reason for Change:

Same as 2114 but from a branch on the main repo to circumvent ongoing pipeline issues.

ipam.go -> requestConfigHandlerHelper releases default ipconfig immediately on failure + more UTs
swiftv2.go -> Add routes for node CIDRs + parse MTPNC Primary IP as a CIDR + toggle SkipDefaultRoutes for InfraNIC to true
Move CNS RBAC roles for SWIFT v2 to a different file
Add loggers

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
relevant PR labels added

Notes:

Signed-off-by: Quang Nguyen <nddangquang@gmail.com>

isaac-dasan · 2023-10-27T19:50:26Z

cns/restserver/ipam.go

 	ErrNoNCs            = errors.New("No NCs found in the CNS internal state")
 )

-// requestIPConfigHandlerHelper validates the request, assigns IPs, and returns a response


Can we keep or enhance the comments?

isaac-dasan · 2023-10-27T20:03:23Z

cns/restserver/ipam_test.go

+	assert.False(t, podIPInfo[2].SkipDefaultRoutes)
+}
+
+func TestIPAMGetSWIFTv2IPFailure(t *testing.T) {


Not sure what we are asserting here and what is failing, can you please explain with some comments at the beginning?

isaac-dasan

What is the ongoing pipelines issue that we are trying to circumvent?

rbtr · 2023-10-24T21:35:25Z

cns/middlewares/swiftV2.go

+	mtpnc := v1alpha1.MultitenantPodNetworkConfig{}
+	mtpncNamespacedName := k8stypes.NamespacedName{Namespace: podInfo.Namespace(), Name: podInfo.Name()}
+	if err := m.Cli.Get(ctx, mtpncNamespacedName, &mtpnc); err != nil {
+		return cns.PodIpInfo{}, fmt.Errorf("failed to get pod's mtpnc from cache : %w", err)


we should use errors.Wrap/Wrapf everywhere, never fmt.Errorf

I noticed that the package github.com/pkg/errors have been made obsolete since go 1.13 (see answer here, and also the repo has been archived since 2021. Should we still be using it?

we are still using it.

rbtr · 2023-10-24T21:36:43Z

cns/middlewares/swiftV2.go

+	return nil
+}
+
+// parseCIDRs parses the semicolons separated CIDRs string and returns the IPv4 and IPv6 CIDRs.


semicolons

comment needs update

rbtr · 2023-10-27T20:40:56Z

cns/azure-cns-swiftv2.yaml

+          effect: NoSchedule
+      containers:
+        - name: cns-container
+          image: mcr.microsoft.com/containernetworking/azure-cns:v1.4.7


i don't think we need this file at all?

rbtr · 2023-10-27T20:42:04Z

cns/azure-cns.yaml

@@ -1,32 +1,4 @@
 apiVersion: v1
-kind: ServiceAccount


revert this file, why did you change it?

cns/middlewares/mock/mockSWIFTv2.go

rbtr · 2023-10-27T20:50:10Z

cns/restserver/internalapi_test.go

+	SWIFTv2IP           = "192.168.0.1"
+	SWIFTv2MAC          = "00:00:00:00:00:00"
+	SWIFTv2GatewayIP    = "10.0.0.1"
+	SWIFTv2NCID         = "testncid"


the linter will not warn you about unused Exported fields, only private ones. because they could be used by another package which the linter is not scanning

cns/restserver/ipam.go

rbtr · 2023-10-27T21:03:42Z

cns/api.go

+	SetRoutes(*PodIpInfo) error
+}
+
+type IPConfigsRequestValidator func(context.Context, *IPConfigsRequest) (types.ResponseCode, string)


does this func need to return (types.ResponseCode, string)? If it is just a validator, returning err or nil should be plenty?

I see from the impl that this is not actually a pure function, as -Validator implies.
It also isn't really middleware/decorator - that would require the input to the middleware func to the next func to call in the chain...this is just a dependent object.
Since it mutates the input *IPConfigsRequest, the field in the "Middleware" object should be renamed to something like SetHasSecondaryInterface. I don't have a good idea for what the interface def should be renamed to, but validator it is not.

fyi @miguelgoms, we had a discussion regarding this offline. Currently the way I implement the logic behind SWIFTv2Middleware cherry-picked middleware behaviors, but it is not "true middleware". It is a design review so it is not blocking, we can still go ahead with this since everything is working now, but I can go back to this after we got everything merged to revise the design.

nddq · 2023-10-27T22:20:35Z

What is the ongoing pipelines issue that we are trying to circumvent?

We can't trigger build from forks anymore, so I had to make a PR from a branch directly from the main ACN repo.

Signed-off-by: Quang Nguyen <nguyenquang@microsoft.com>

rbtr

there's some missed usages of fmt.Error, and util packages are generally discouraged. not blocking

rbtr · 2023-10-31T15:14:37Z

cns/middlewares/utils/utils.go

no utils packages please

neaggarwMS · 2023-10-31T17:38:52Z

cns/configuration/env.go

+	LabelPodSwiftV2 = "kubernetes.azure.com/pod-network"
+	EnvPodCIDRs     = "POD_CIDRs"
+	EnvServiceCIDRs = "SERVICE_CIDRs"
+	EnvNodeCIDRs    = "NODE_CIDRs"


This will change to infraVnet CIDR in future PR

nddq and others added 30 commits July 28, 2023 17:25

fix overlay IPAM not reporting version

ace96ff

Merge branch 'master' into master

f0b4f19

revert file and var naming, add correct path to makefile

f1b5dcb

Merge branch 'Azure:master' into master

eebfbc8

Merge branch 'Azure:master' into master

84abe01

Merge branch 'Azure:master' into master

994dc04

Merge branch 'Azure:master' into master

8475126

Merge branch 'Azure:master' into master

29c8206

Merge branch 'Azure:master' into master

62e6df8

proposal design for multitenant IPAM flow

20988bd

change podipinfo + linter issue

2e6842f

pointer issues for printf

227fd89

update IPAM branching

d5b00db

remove comments

e53f1e7

pod client placeholder

f99525c

address lint issue for httpservicefake

c7f1bca

getting pod info in validator

f2359ac

linter issue

fa2cac5

Merge branch 'master' into swift2-cns-ipam-branching

79aef2e

Signed-off-by: Quang Nguyen <nddangquang@gmail.com>

Merge branch 'master' into swift2-cns-ipam-branching

fd2803f

Merge branch 'master' into swift2-cns-ipam-branching

dc08bfe

Signed-off-by: Quang Nguyen <nddangquang@gmail.com>

Merge branch 'master' into swift2-cns-ipam-branching

7792ac4

Merge branch 'master' into swift2-cns-ipam-branching

dfd8764

Merge branch 'master' into swift2-cns-ipam-branching

a60f9e1

Merge branch 'master' into swift2-cns-ipam-branching

c58fef7

Merge branch 'master' into swift2-cns-ipam-branching

50deef4

update network container contract

cc91071

renaming

0674e06

mtpnc changes

fd03f7c

Merge branch 'Azure:master' into master

baeb65b

isaac-dasan reviewed Oct 27, 2023

View reviewed changes

isaac-dasan requested changes Oct 27, 2023

View reviewed changes

rbtr requested changes Oct 27, 2023

View reviewed changes

Merge branch 'master' into featSWIFTv2-IPAM-branching

382708d

Signed-off-by: Quang Nguyen <nguyenquang@microsoft.com>

This comment was marked as off-topic.

Sign in to view

isaac-dasan previously approved these changes Oct 28, 2023

View reviewed changes

nddq and others added 4 commits October 28, 2023 13:46

Merge branch 'master' into featSWIFTv2-IPAM-branching

ae5d28d

Signed-off-by: Quang Nguyen <nguyenquang@microsoft.com>

fix: move parseCIDRs to a common package, use net/netip instead of net

2d305e1

fix: exhaustive all switch case for nic type

54aa8be

fix: exhaustive all switch case for nic type

86b9e7d

nddq dismissed stale reviews from miguelgoms and isaac-dasan via 86b9e7d October 28, 2023 17:59

nddq and others added 4 commits October 30, 2023 01:22

Merge branch 'master' into featSWIFTv2-IPAM-branching

9417618

Signed-off-by: Quang Nguyen <nguyenquang@microsoft.com>

refractor: change fmt.Errorf to errors.Wrapf

8fed7b9

fix: add mtpnc status check in validator + use netip package

d57e6b3

fix: minor

04f08fe

miguelgoms previously approved these changes Oct 30, 2023

View reviewed changes

nddq and others added 4 commits October 30, 2023 15:12

Merge branch 'master' into featSWIFTv2-IPAM-branching

790e787

Signed-off-by: Quang Nguyen <nguyenquang@microsoft.com>

Merge branch 'master' into featSWIFTv2-IPAM-branching

0688baf

Signed-off-by: Quang Nguyen <nguyenquang@microsoft.com>

Merge branch 'master' into featSWIFTv2-IPAM-branching

82016cd

Signed-off-by: Quang Nguyen <nguyenquang@microsoft.com>

revert: azure-cns.yaml

113021d

nddq dismissed miguelgoms’s stale review via 113021d October 31, 2023 15:18

rbtr approved these changes Oct 31, 2023

View reviewed changes

cns/middlewares/utils/utils.go

Copy link

Collaborator

rbtr Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no utils packages please

miguelgoms approved these changes Oct 31, 2023

View reviewed changes

neaggarwMS approved these changes Oct 31, 2023

View reviewed changes

nddq merged commit d34c61f into master Oct 31, 2023

nddq deleted the featSWIFTv2-IPAM-branching branch October 31, 2023 17:45

nddq mentioned this pull request Oct 31, 2023

feat: CNS RequestIPAddress branching for MT/V2 #2114

Closed

4 tasks

feat: CNS RequestIPAddress branching for MT/V2 #2300

feat: CNS RequestIPAddress branching for MT/V2 #2300

Uh oh!

Conversation

nddq commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isaac-dasan 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

nddq commented Oct 27, 2023

Uh oh!

This comment was marked as off-topic.

Uh oh!

rbtr 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nddq commented Oct 17, 2023 •

edited

Loading