[NPM Lite] Default Deny CNS Changes

[rejain456]

Reason for Change:

As part of adding default deny so pods can't communicate with one another when network policies are not present, this pr is part 2 which updates the cns code, creating default deny acl and sending it to cni.

Issue Fixed:

Requirements:

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

Notes:

[timraymond]

I'm not excited about adding a fourth return value here, and I generally don't like to return bools (since they carry no documentation about their meaning).

I would almost be of a mind to do the k.Cli.Get(ctx, mtpncNamespacedName, &mtpnc) to find out the status of the defaultDenyACL one level up the call stack where we want to use that value, but I understand that incurs a penalty for the second network call to the api server.

The only alternate proposal I have is caching these values on *K8sSWIFTv2Middleware, but all the usual hazards of caching apply here. For something applying network policy, I'm leery of doing this.

I suppose this is okay for now unless we can think of an alternative that avoids it.

[rbtr]

I think this method has become too overloaded. validate- means tell me if this input is valid. It should only return a bool.

[timraymond]

Any reason not to do:

Suggested change

	for i := range ipConfigsResp.PodIPInfo {
	ipInfo := &ipConfigsResp.PodIPInfo[i]
	for _, ipInfo := range ipConfigsResp.PodIPInfo {

[rejain456]

Hi, there are 2 reasons why I did not go that route

• I see the following error: rangeValCopy: each iteration copies 256 bytes (consider pointers or indexing) (gocritic)

• This line of code: _, ipInfo := range ipConfigsResp.PodIPInfo creates a copy of ipInfo and &ipInfo creates a pointer to the copy; however, does not change the slice itself.

Please feel free to correct me on this, but that was my thinking.

[timraymond]

It feels like premature optimization to me. I'd prefer the more readable suggestion unless we can point at some benchmarks / profiling that proves this large copy is a performance bottleneck.

[rejain456]

@timraymond, can you please elaborate on which readable suggestion you are referring to?

[timraymond]

Sorry @rejain456 , when I refer to "the suggestion" here, I mean the code suggestion in the top comment here: #3286 (comment)

[rejain456]

@timraymond , got it thanks for clarifying. Regarding that I believe this line of code: _, ipInfo := range ipConfigsResp.PodIPInfo creates a copy of ipInfo and &ipInfo creates a pointer to the copy; however, does not change the slice itself. In that case, the list PodIPInfo won't get changed. Whereas the line:
for i := range ipConfigsResp.PodIPInfo { ipInfo := &ipConfigsResp.PodIPInfo[i] creates a pointer to the original PodIPInfo list and will change the properties in it.

Please let me know if any part of that is incorrect

[rbtr]

I think this method has become too overloaded. validate- means tell me if this input is valid. It should only return a bool.

[rbtr]

we shouldn't call this from common code if it requires a stub in an _os file. we should be able to abstract in such a way that we don't do this.

[thatmattlong]

I'm actually ok with this, I feel it's better to have an os stub than to branch in the common code based on runtime.GOOS

[rbtr]

I don't want us to check runtime.GOOS either. But we can do a better job using the _<os>.go pattern...I feel like I have done this several times recently, but I can't find a good example immediately.

At k8sSwiftV2_linux.go L105 right above here, there's a stub for addDefaultRoute which noops. If we only need to set a default route on Windows, putting addDefaultRoute in the OS abstraction is the wrong place - what are we doing when we need to addDefaultRoute? That's the thing that actually has different behavior per-OS, and so that's the thing that should be in the OS specific files.

If we need these stubs, the abstraction is wrong...or effectively doesn't exist. We're not abstracting away the OSes, we're actually coding them all together and branching implicitly based on the GOOS. The if just moves out of the code and in to my head, because now I need to know that if GOOS=linux then this method call is noop when I'm reading the common calling code.

[thatmattlong]

That's the thing that actually has different behavior per-OS, and so that's the thing that should be in the OS specific files.

While this is probably true, I don't want this PR to become too much about refactoring that. There's a lot of stuff in the calling code which we'd have to duplicate and/or refactor which I would rather not do now, and I feel this doesn't put us in much worse a situation for when (if) we do take up that work.