-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add support for cluster-scoped AdminNetworkPolicy resource #2522
Conversation
94168a7
to
4d4bc51
Compare
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
d48e417
to
9bf46b6
Compare
Sorry for the Comment bomb, I squashed a commit to fix CLA |
I'm lifting this out because I think this is getting awkward with the comments: Rather than trying to specify complicated ordering interactions between ANP and NP, it seems like we can achieve all the user stories (Namely, use-case number 5) by, instead, adding a fourth action. This action would "default-deny unless there is a NP specifically allowing it". Call it "Defer"? |
We've definitely had this conversation before with community members. At one point we proposed to have a priority-less model with 4 actions, and that captures all the user stories by having implicit priorities among the actions (the "defer" action also has an implicit priority). This idea wasn't well received by the end however, as I believe most people still prefer writing prioritized rules (narrower rules at top, wider rules on the bottom) and not overloading "actions". |
PRR looks fine now, thanks. Since there is no in-tree implementation it's as safe as anything can be. |
I'll add my approval after SIG approval is done. |
@Dyanngg gotcha, sorry to open up old discussions. While I'm not proposing getting rid of priority entirely (just the interaction between priority and NP), I didn't mean to re-litigate everything. |
Ack Thanks a ton @johnbelamaric @squeed I would definitely like to keep this conversation going, I think there's alot of room for this API to grow and evolve over the course of alpha |
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 good with the primary content. A few things should be called out explicitly ass UNRESOLVED, but otherwise this feels pretty good to proceed.
And the good news is that, as a CRD, we don't REALLY have to hit the KEp freeze :)
be considered as highest precedence. Any positive priority, however, will have | ||
higher precedence over the namespaced NetworkPolicy instances in the cluster. | ||
|
||
Additionally, the special priority "0" can be used in the priority field to indicate |
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.
One question that comes up with Pass and prio 0 is : what happens if I set a prio 0 rule with action Pass? Do we just not allow it?
If I interpret @squeed here, we could make "PassAllow" and "PassDeny" which maybe obviates the need for a "special" case at all. Pass* sends traffic to NP and the default disposition is clearly stated right there.
We can haggle this after KEP freeze, but it's not a terrible idea...
be considered as highest precedence. Any positive priority, however, will have | ||
higher precedence over the namespaced NetworkPolicy instances in the cluster. | ||
|
||
Additionally, the special priority "0" can be used in the priority field to indicate |
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.
maybe add an UNRESOLVED here
Add some unresolved sections, finish up PRR Signed-off-by: astoycos <astoycos@redhat.com>
I went ahead and addressed the final few comments here, thanks for all the help everyone! |
// its fields are set, they should assume an option they are not aware of has | ||
// been specified and fail closed. | ||
type NamespaceSet struct { | ||
// Self cannot be "false" when it is set. |
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.
These is a nitpick/question, but maybe is for non-english people, I find this hard to read, is not the same as?
Self can only be empty or true, it can not be false
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.
Totally understandable, Will definitely try to make this easier to understand when we implement the CRD
+1 |
/lgtm plenty of niggles to follow up on, but OK to start in on the API in earnest. |
Just need the OK from @johnbelamaric, thanks Tim! |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhiraut, johnbelamaric, thockin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Not sure if it's too late to comment given this is merged, but from experience with Calico's datamodel, I think it'd be a mistake to map "lower priority than NetworkPolicy" onto a single value of Priority. You'll immediately get users wanting to order the policies that are "after" network policies. How about something like this to make it explicit:
or
|
It's not too late at all @fasaxc I'm starting to work on the API PR today, thanks for the input, it's really helpful! |
This KEP aims to introduce new resources to the
netpol.networking.k8s.io
group to secure traffic at a cluster level aimed at solving cluster administrator's use cases, i.e. Cluster scoped AdminNetworkPolicy for administrators.Enhancement Issue: #2091