Add NPEP for new CIDRGroup object peer
#183
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
✅ Deploy Preview for kubernetes-sigs-network-policy-api ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tssurya The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/M
tssurya
force-pushed
the
cidr-object-npep
branch
from
aef81b5 to
7e30032
Compare
k8s-ci-robot
added
size/L
tssurya
changed the title
CIDRGroup object peer
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
tssurya
force-pushed
the
cidr-object-npep
branch
from
7e30032 to
930064a
Compare
Signed-off-by: Surya Seetharaman <[email protected]>
tssurya
force-pushed
the
cidr-object-npep
branch
from
930064a to
b6673d7
Compare
|
@joestringer and @networkop PTAL, I have tried to include the uses cases you had proposed to see if we can also have this way of expressing CIDR peers. |
| The speed of IP churn is bounded by the BGP advertisement interval and can be | ||
| further reduced by the BGP controller implementation. | ||
|
|
||
| * As a cluster administrator I want to to ensure that pods can reach |
There was a problem hiding this comment.
Just playing devil's advocate with maybe a really bad idea:
What if we try to use the EndpointSlice API to solve this set of problems?
Pros:
- Fewer network policy
objectsCRDS - Make headway into defining the Service selector API
Cons:
- Abusing(?) the EndpointSlice idiom
There was a problem hiding this comment.
@rahulkjoshi can you specify IP prefixes or only IP addresses in EndpointSlice?
There was a problem hiding this comment.
😦 yeah you're right only individual addresses in EndpointSlice
My underlying thought with this situation was whether there's overlap with a more general service discovery problem. My thinking was that whatever application the rule protects also needs to discover the IPs it's talking to.
I was hoping that plugging into that system might be more ergonomic? But it's more likely to just be incredibly complicated and not easily generalizable.
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
k8s-ci-robot
removed
the
lifecycle/stale
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. In response to this:
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-sigs/prow repository. |
|
/reopen |
|
@tssurya: Reopened this PR. In response to this:
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-sigs/prow repository. |
|
/remove-lifecycle rotten |
k8s-ci-robot
removed
the
lifecycle/rotten
| NOTE: Second use case is not possible today using NetworkPolicy resource | ||
| since we only have `ipBlocks` as a peer however this is definitely a useful | ||
| case to keep in mind for having a CIDR Group. | ||
|
|
There was a problem hiding this comment.
Calico has a two similar resources:
- GlobalNetworkSet <- identical in all but name?
- NamespacedNetworkSet <- namespaced version of the above
The feature is popular. It's much nicer to write policies as allow to role = 'external-db' and to update the external DB network set. And it's much easier to automate update of a network set rather than a policy. It also helps Calico with optimisation; we render selectors into IP sets, which are efficient to change.
I think the namespaced version of the Calico resource came about to allow tenants/teams to define these resources within their own namespace. Often a particular team has some particular external servers that they need to access and it become unmanageable to go through the platform owner to manage those network sets.
Arguably, a mistake we made in our model was to put NetowrkSets in the same selector scope as Pods in the namespace. This proposal makes it explicit what you're matching, which I think is better. Folks expect "allow from all within namespace" to mean "allow all my workloads", not "allow all my workloads and network sets".
| In order to ensure it is coexisting with inline CIDR peers without confusion, | ||
| we propose to change the type of `networks` peer from `string` to a struct of type | ||
| `NetworkPeer`: |
There was a problem hiding this comment.
Given the API is headed for beta, would this breaking change be OK? I'd have thought we want to add a new top-level peer type to make it easier to adopt in non-breaking fashion.
This NPEP captures the use cases for having a better CIDRGrouper for easier management of CIDR peers in addition to inline CIDR peers
Closes #182