-
Notifications
You must be signed in to change notification settings - Fork 39
Remove same-not-same-labels #196
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
Remove same-not-same-labels #196
Conversation
|
Skipping CI for Draft Pull Request. |
✅ Deploy Preview for kubernetes-sigs-network-policy-api ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
/hold |
ac70784 to
1d780b7
Compare
|
/hold cancel |
1d780b7 to
70b035a
Compare
|
/hold Until the work as part of the Tenancy NPEP ends up as an actual API change PR, I'll let @npinaeva remove this hold |
danwinship
left a 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.
eh, we know we don't want this tenancy API, so let's get rid of it
apis/v1alpha1/shared_types.go
Outdated
| // | ||
| // +optional | ||
| Namespaces *NamespacedPeer `json:"namespaces,omitempty"` | ||
| Namespaces *metav1.LabelSelector `json:"namespaceSelector,omitempty"` |
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.
Field name (Namespaces) and json field name (namespaceSelector) don't match.
With this PR, we have:
NamespacesinAdminNetworkPolicySubjectNamespaceSelectorinNamespacedPod- (not clear which one you meant) in
AdminNetworkPolicyIngressPeer/AdminNetworkPolicyEgressPeer
This seems like it could be confusing... it was definitely less confusing with NamespacedPeer there because then a peer had either
- namespaces:
namespaceSelector: ...
or
- pods:
podSelector: ...
Maybe we should keep NamespacedPeer even though it's mostly vestigial, just to preserve the symmetry? (Or maybe not? Maybe it's annoying of us to force users to have to include that extra level?)
(If we do that here, we should probably do the same thing in Subject).
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.
yeah so I wanted to make this the same as what's in the subject today.. so don't make users go through that extra step/hoop for no reason,
btw nice catch on the naming diff ..
Let me rebase this PR
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.
al'right so in the latest update this is what I did:
subject and peer are symmetric now in how we express namespaces and pods:
namespaces:
matchLabels:
pods:
namespaceSelector:
matchLabels:
podSelector:
matchLabels:
70b035a to
6493a47
Compare
|
PR needs rebase. DetailsInstructions 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. |
Signed-off-by: Surya Seetharaman <[email protected]>
6493a47 to
4d5dd78
Compare
|
/lgtm not doing "/hold cancel" because I'm not totally sure why it's held, but I think it's ok to merge |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, tssurya The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel Let's get rid of this unwanted API /lgtm thanks @tssurya |
Brings in: 1) kubernetes-sigs/network-policy-api#209 2) kubernetes-sigs/network-policy-api#196 3) kubernetes-sigs/network-policy-api#213 Signed-off-by: Surya Seetharaman <[email protected]>
Brings in: 1) kubernetes-sigs/network-policy-api#209 2) kubernetes-sigs/network-policy-api#196 3) kubernetes-sigs/network-policy-api#213 Signed-off-by: Surya Seetharaman <[email protected]>
Brings in: 1) kubernetes-sigs/network-policy-api#209 2) kubernetes-sigs/network-policy-api#196 3) kubernetes-sigs/network-policy-api#213 Signed-off-by: Surya Seetharaman <[email protected]>
Brings in: 1) kubernetes-sigs/network-policy-api#209 2) kubernetes-sigs/network-policy-api#196 3) kubernetes-sigs/network-policy-api#213 Signed-off-by: Surya Seetharaman <[email protected]>
Brings in: 1) kubernetes-sigs/network-policy-api#209 2) kubernetes-sigs/network-policy-api#196 3) kubernetes-sigs/network-policy-api#213 Signed-off-by: Surya Seetharaman <[email protected]>
Brings in: 1) kubernetes-sigs/network-policy-api#209 2) kubernetes-sigs/network-policy-api#196 3) kubernetes-sigs/network-policy-api#213 Signed-off-by: Surya Seetharaman <[email protected]> (cherry picked from commit 57bca707c5f866a24c79767b4af1244ec2463570)
Brings in: 1) kubernetes-sigs/network-policy-api#209 2) kubernetes-sigs/network-policy-api#196 3) kubernetes-sigs/network-policy-api#213 Signed-off-by: Surya Seetharaman <[email protected]>
Brings in: 1) kubernetes-sigs/network-policy-api#209 2) kubernetes-sigs/network-policy-api#196 3) kubernetes-sigs/network-policy-api#213 Signed-off-by: Surya Seetharaman <[email protected]>
Brings in: 1) kubernetes-sigs/network-policy-api#209 2) kubernetes-sigs/network-policy-api#196 3) kubernetes-sigs/network-policy-api#213 Signed-off-by: Surya Seetharaman <[email protected]>
Brings in: 1) kubernetes-sigs/network-policy-api#209 2) kubernetes-sigs/network-policy-api#196 3) kubernetes-sigs/network-policy-api#213 Signed-off-by: Surya Seetharaman <[email protected]>
Brings in: 1) kubernetes-sigs/network-policy-api#209 2) kubernetes-sigs/network-policy-api#196 3) kubernetes-sigs/network-policy-api#213 Signed-off-by: Surya Seetharaman <[email protected]>
Brings in: 1) kubernetes-sigs/network-policy-api#209 2) kubernetes-sigs/network-policy-api#196 3) kubernetes-sigs/network-policy-api#213 Signed-off-by: Surya Seetharaman <[email protected]>
We have been extensively re-designing our tenancy use cases and its clear we won't be using same and notSame labels: #178 (comment)
Let's remove this from our API before it hits Beta.
See FUP issues that need to be fixed once this merges in : #197
We had a certain asymmetry around how
namespacesinsubjectandnamespacesinpeersare used. This was because thenamespacesin subject was a simplenamespaceSelectorwhile the one in the peer was a struct type withnamespaceSelector,sameLabelsandnotSameLabels. However since we are removingsameLabelsandnotSameLabelsthere is no need fornamespacesin peer to be a struct, we can just make thisnamespaceSelectorthus bringing it closer to how it looks in the subject.and
if that makes it confusing ^ we can think of alternatives