Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NPM Lite] Default Deny CNS Changes #3286
base: master
Are you sure you want to change the base?
[NPM Lite] Default Deny CNS Changes #3286
Changes from 3 commits
d951865
3489f9c
7b01c07
8eff31c
38cf5be
6bfd495
f2bd968
f6b6206
c51e568
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to do:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, there are 2 reasons why I did not go that route
Please feel free to correct me on this, but that was my thinking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timraymond, can you please elaborate on which readable suggestion you are referring to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @rejain456 , when I refer to "the suggestion" here, I mean the code suggestion in the top comment here: #3286 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this method has become too overloaded.
validate-
means tell me if this input is valid. It should only return a bool.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, puttingaddDefaultRoute
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk, if there's only one OS that needs special handling, and the rest are all NOPs, I find
runtime.GOOS
more readable. It works better with static analysis too, since you don't need to set GOOS for the toolchain to realize there is an entire separate implementation.Now if we had wholly different implementations, each with their own considerations, I'd consider build tags more necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works if the special handling is entirely your code and doesn't work if it's platform specific things like using registry package which is compiled for and can only be referenced from code compiled for Windows. It doesn't work here because the special extra logic for Windows is interacting with HCN and hcsshim/hcn is not importable outside of a Windows built file.
We do a lot of this - Linux iptables, netns, etc vs Windows HCS, registry, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough--if there's compile-time reasons, that's certainly a good reason. I guess lately I've seen a few instances where it's just
exec.Cmd
s for eitherpowershell.exe
or their Linux equivalent. In those circumstances, the specific string can be decided at runtime.Check failure on line 67 in cns/middlewares/k8sSwiftV2_windows.go
GitHub Actions / Lint (1.22.x, windows-latest)
Check failure on line 67 in cns/middlewares/k8sSwiftV2_windows.go
GitHub Actions / Lint (1.23.x, windows-latest)