npm: enable debug feature parity for v2 #1324

matmerr · 2022-04-07T17:24:17Z

Reason for Change:

Enables basic debug feature parity with V1 and V2. Followup PR will add bubble up iptables comments along with gettuples.

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests

Notes:

…ing into npmcliv2

rbtr · 2022-04-13T17:50:33Z

@vakalapa you might want to update the reviewers team 🙂

huntergregory · 2022-04-13T21:37:34Z

npm/pkg/controlplane/goalstateprocessor/goalstateprocessor.go

+	}
+
 	if appendedIPSets == nil {
-		toDeleteIPSets = cachedIPSetNames


don't think we'll be using the gsp anytime soon but this used to have prefixed set names and now it has hashed set names

huntergregory · 2022-04-13T21:39:32Z

npm/util/ioutil/restore.go

 			sameNew = "updated"
 		}
-		msg := fmt.Sprintf("on try number %d, failed to run command [%s]. Rerunning with %s file. Had error [%s].\nUsed file:\n%s", creator.tryCount, commandString, sameNew, err.Error(), fileString)
+		msg := fmt.Sprintf("on try number %d, failed to run command [%s]. Rerunning with %s file. Had error [%s].Used file:%s", creator.tryCount, commandString, sameNew, err.Error(), fileString)


nit: easier to read the save file if there's a newline after the colon? The save file is multilined anyways

logs shouldn't be multiline. one log per line pls

huntergregory · 2022-04-13T22:03:01Z

npm/pkg/controlplane/controllers/v2/cache.go

+
+func (c *Cache) GetListMap() map[string]string {
+	listMap := make(map[string]string, 0)
+	// get all lists


nit: maybe comment that lists are kept in the setmap in v2?

huntergregory · 2022-04-13T22:07:04Z

npm/pkg/controlplane/controllers/v2/namespaceController.go

 type Namespace struct {
-	name      string
+	Name      string
 	LabelsMap map[string]string // Namespace labels


noticed that you extracted the Pod struct into the common pkg. Not necessary, but should we do that here for Namespace?

huntergregory · 2022-04-13T22:09:25Z

npm/pkg/controlplane/controllers/common/cacheinterface.go

+)
+
+// Tuple struct
+type Tuple struct {


nit: maybe Tuple stuff belongs in its own file?

this can be moved to pkg/dataplane/debug

huntergregory · 2022-04-13T22:14:16Z

npm/cmd/gettuples.go

+				err := viper.Unmarshal(config)
 				if err != nil {
-					return fmt.Errorf("%w", err)
+					log.Printf("failed to load config with err ")


nit: missing err

huntergregory · 2022-04-13T22:20:41Z

npm/npm_test.go

+	npmMgr := NetworkPolicyManager{
+		config: npmconfig.Config{
+			Toggles: npmconfig.Toggles{
+				EnableV2NPM: true,


add same test for v1?

huntergregory · 2022-04-13T22:57:45Z

npm/pkg/dataplane/debug/converter.go

+
+	}
+
+	return settype, setmetadata.GetSetKind()


introducing a getSetKind() method for SetType in my current PR. Would make this a little cleaner than having to use SetMetadata

huntergregory · 2022-04-13T22:59:14Z

npm/pkg/dataplane/debug/converter.go

+
+	if c.EnableV2NPM {
+		setInfo.Name = c.SetMap[ipsetHashedName]
+		setInfo.Type, _ = c.getSetTypeV2(setInfo.Name)


return error if it doesn't exist?

huntergregory

converter logic looks good now

huntergregory · 2022-06-02T17:25:50Z

npm/azure-npm.yaml

      containers:
        - name: azure-npm
-          image: mcr.microsoft.com/containernetworking/azure-npm:v1.4.21
+          image: acnpublic.azurecr.io/azure-npm:v1.4.22-24-g0df7a4d7-dirty-v3


nit: do we want to have acnpublic here?

huntergregory · 2022-06-02T17:35:35Z

npm/cmd/gettuples.go

+				c := &debug.Converter{
+					NPMDebugEndpointHost: "http://localhost",
+					NPMDebugEndpointPort: api.DefaultHttpPort,
+					EnableV2NPM:          config.Toggles.EnableV2NPM, // todo: pass this a different way than param to this


any plans for the todo?

huntergregory · 2022-06-02T17:39:44Z

npm/npm.go

-// matmerr: todo: really not a fan of sniping the marshaljson and returing different marshalled type,
-// makes very difficult to predict marshalled type when used as a client
+// Dear Time Traveler:
+// This is the server end of the debug dragons den. Several of these properties of the


huntergregory · 2022-06-02T17:59:57Z

npm/pkg/dataplane/debug/trafficanalyzer.go

+			continue
+		}
+		/*tuple.Tuple.Direction == "EGRESS" {
+			fmt.Printf("\tProtocol: %s, Port: %s\n, Chain: %v", tuple.Tuple.Protocol, tuple.Tuple.SrcPort, tuple.Rule.Chain)


would prefer removing dead code, but that doesn't need to be a blocker

the only remaining item from the requested changes seems to be a whitespace nit

* cache interface * retrieve cache common interface

matmerr added 10 commits March 4, 2022 15:44

pretty print

abd49ff

netpol files

0b49cfe

parser type

bcbdeb2

cache/pod interface

918acf2

cache interface

b409a9c

retrieve cache common interface

5de4db7

Merge branch 'master' into npmcliv2

4d45146

cachev2 tuple

a6fdea9

set map retrieval

061d768

add test files and start test

7ef3678

matmerr force-pushed the npmcliv2 branch from 5f1f0c5 to 7ef3678 Compare April 7, 2022 18:11

matmerr added 2 commits April 8, 2022 16:04

simplified rules

90d8fb2

remove log lines

12bd4da

matmerr force-pushed the npmcliv2 branch 2 times, most recently from d7e393c to 39e7fc9 Compare April 12, 2022 18:52

lint

2adecf7

matmerr force-pushed the npmcliv2 branch from 39e7fc9 to 2adecf7 Compare April 12, 2022 18:54

lint

d2ace83

matmerr marked this pull request as ready for review April 13, 2022 16:19

matmerr requested a review from a team as a code owner April 13, 2022 16:19

matmerr requested review from rbtr and removed request for a team April 13, 2022 16:19

matmerr added 2 commits April 13, 2022 09:25

Merge branch 'npmcliv2' of github.com:matmerr/azure-container-network…

c38d471

…ing into npmcliv2

remove old tests

43a73f9

rbtr requested review from a team and removed request for rbtr April 13, 2022 17:51

huntergregory added the npm Related to NPM. label Apr 13, 2022

huntergregory reviewed Apr 13, 2022

View reviewed changes

remove log lines

742e468

matmerr force-pushed the npmcliv2 branch from 198206d to 7c3fd3f Compare May 11, 2022 21:50

matmerr added 6 commits May 11, 2022 15:01

Merge branch 'master' into npmcliv2

37cacc7

better match conditions

214ba18

ipsm v1 hashing serversize for set+listmap

2d4ada2

replace old cache files

ab609af

tests

180bc47

update controller tools for ci

fa51304

matmerr force-pushed the npmcliv2 branch from 32a4538 to fa51304 Compare May 12, 2022 22:25

matmerr added 7 commits May 12, 2022 15:54

simplify generic cache signature

da2be05

parent string

912ebd0

nestedlabel prefix

427c1d0

match keylabelns

92cef04

printing formatting

656c2ef

update tests

c96a6cb

lint

b3602a0

matmerr requested a review from a team as a code owner May 26, 2022 23:58

matmerr requested review from rsagasthya and removed request for a team May 26, 2022 23:58

Merge branch 'master' into npmcliv2

0ea0170

matmerr force-pushed the npmcliv2 branch from 6d2b0d7 to 0ea0170 Compare May 27, 2022 20:45

huntergregory reviewed Jun 2, 2022

View reviewed changes

matmerr enabled auto-merge (squash) June 10, 2022 20:47

matmerr added 4 commits June 10, 2022 13:52

Merge branch 'master' into npmcliv2

a9c74dd

nnc

7ea845e

build tools conflict

a6a4346

fix crdgen

56ddd95

huntergregory approved these changes Jun 14, 2022

View reviewed changes

matmerr merged commit 1b57030 into Azure:master Jun 14, 2022

matmerr added a commit to matmerr/azure-container-networking that referenced this pull request Jun 29, 2022

npm: enable debug feature parity for v2 (Azure#1324)

cee6aca

* cache interface * retrieve cache common interface

npm: enable debug feature parity for v2 #1324

npm: enable debug feature parity for v2 #1324

Uh oh!

Conversation

matmerr commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbtr commented Apr 13, 2022

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!

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!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

matmerr commented Apr 7, 2022 •

edited

Loading