Skip to content
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

Add support for cluster-scoped AdminNetworkPolicy resource #2522

Merged
merged 39 commits into from
Feb 3, 2022

Conversation

abhiraut
Copy link
Contributor

@abhiraut abhiraut commented Feb 18, 2021

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

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 18, 2021
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/network Categorizes an issue or PR as relevant to SIG Network. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 18, 2021
@abhiraut abhiraut changed the title Add support for ClusterNetworkPolicy resources WIP: Add support for ClusterNetworkPolicy resources Feb 18, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 18, 2021
@abhiraut abhiraut force-pushed the cnp-kep branch 3 times, most recently from 94168a7 to 4d4bc51 Compare February 20, 2021 01:56
@k8s-ci-robot k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 24, 2021
@k8s-ci-robot
Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 24, 2021
@Dyanngg Dyanngg force-pushed the cnp-kep branch 2 times, most recently from d48e417 to 9bf46b6 Compare February 25, 2021 00:08
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 4, 2021
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 18, 2021
@abhiraut abhiraut requested a review from rikatz March 22, 2021 21:46
@k8s-ci-robot k8s-ci-robot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 22, 2021
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 1, 2022
@astoycos
Copy link

astoycos commented Feb 1, 2022

Sorry for the Comment bomb, I squashed a commit to fix CLA

@squeed
Copy link

squeed commented Feb 1, 2022

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"?

@Dyanngg
Copy link
Contributor

Dyanngg commented Feb 1, 2022

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".

@johnbelamaric
Copy link
Member

PRR looks fine now, thanks. Since there is no in-tree implementation it's as safe as anything can be.

@johnbelamaric
Copy link
Member

I'll add my approval after SIG approval is done.

@squeed
Copy link

squeed commented Feb 1, 2022

We've definitely had this conversation before with community members

@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.

@astoycos
Copy link

astoycos commented Feb 1, 2022

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

Copy link
Member

@thockin thockin left a 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 :)

keps/sig-network/2091-admin-network-policy/README.md Outdated Show resolved Hide resolved
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
Copy link
Member

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
Copy link
Member

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

keps/sig-network/2091-admin-network-policy/README.md Outdated Show resolved Hide resolved
Add some unresolved sections,
finish up PRR

Signed-off-by: astoycos <astoycos@redhat.com>
@astoycos
Copy link

astoycos commented Feb 2, 2022

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.
Copy link
Member

@aojea aojea Feb 2, 2022

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

Copy link

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

@aojea
Copy link
Member

aojea commented Feb 2, 2022

I went ahead and addressed the final few comments here, thanks for all the help everyone!

+1
👏

@thockin thockin added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 2, 2022
@thockin
Copy link
Member

thockin commented Feb 2, 2022

/lgtm
/approve

plenty of niggles to follow up on, but OK to start in on the API in earnest.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2022
@astoycos
Copy link

astoycos commented Feb 2, 2022

Just need the OK from @johnbelamaric, thanks Tim!

@johnbelamaric
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2022
@k8s-ci-robot k8s-ci-robot merged commit 9f5142a into kubernetes:master Feb 3, 2022
@fasaxc
Copy link

fasaxc commented Feb 25, 2022

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:

priorityClass: Override | Deferred
(or)           High | Low
(or)           AboveNetworkPolicy | BelowNetworkPolicy
(or)           OverrideNetworkPolicy | DeferToNetworkPolicy
priority: 0-1000

or

priorityVsNetworkPolicy: High | Low
priority: 0-1000

@astoycos
Copy link

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/network Categorizes an issue or PR as relevant to SIG Network. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.