Remove same-not-same-labels #196

tssurya · 2024-02-20T20:01:56Z

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 namespaces in subject and namespaces in peers are used. This was because the namespaces in subject was a simple namespaceSelector while the one in the peer was a struct type with namespaceSelector, sameLabels and notSameLabels. However since we are removing sameLabels and notSameLabels there is no need for namespaces in peer to be a struct, we can just make this namespaceSelector thus bringing it closer to how it looks in the subject.

namespaces:
   matchLabels:

and

pods:
   namespaceSelector:
     matchLabels:
   podSelector:
     matchLabels:

if that makes it confusing ^ we can think of alternatives

k8s-ci-robot · 2024-02-20T20:01:59Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

netlify · 2024-02-20T20:02:16Z

✅ Deploy Preview for kubernetes-sigs-network-policy-api ready!

Name	Link
🔨 Latest commit	`4d5dd78`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-network-policy-api/deploys/6601cdc9ffb5d30008fc09f3
😎 Deploy Preview	https://deploy-preview-196--kubernetes-sigs-network-policy-api.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

tssurya · 2024-02-20T20:02:43Z

/hold
need to remove conformance plumbing and need to bring it up in upstream meeting before merging this.

tssurya · 2024-02-21T20:46:31Z

/hold cancel

astoycos · 2024-02-27T17:12:32Z

/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

eh, we know we don't want this tenancy API, so let's get rid of it

danwinship · 2024-03-07T18:08:52Z

apis/v1alpha1/shared_types.go

@@ -133,15 +133,15 @@ type AdminNetworkPolicyIngressPeer struct {
 	// Support: Core
 	//
 	// +optional
-	Namespaces *NamespacedPeer `json:"namespaces,omitempty"`
+	Namespaces *metav1.LabelSelector `json:"namespaceSelector,omitempty"`


Field name (Namespaces) and json field name (namespaceSelector) don't match.

With this PR, we have:

Namespaces in AdminNetworkPolicySubject

NamespaceSelector in NamespacedPod

(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).

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

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:

k8s-ci-robot · 2024-03-11T23:36:59Z

PR needs rebase.

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.

Signed-off-by: Surya Seetharaman <[email protected]>

danwinship · 2024-03-25T20:08:51Z

/lgtm
/approve

not doing "/hold cancel" because I'm not totally sure why it's held, but I think it's ok to merge

k8s-ci-robot · 2024-03-25T20:09:00Z

[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

Needs approval from an approver in each of these files:

~~OWNERS~~ [danwinship]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

astoycos · 2024-03-25T20:13:35Z

/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]> (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]>

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 20, 2024

k8s-ci-robot requested review from astoycos and danwinship February 20, 2024 20:02

tssurya marked this pull request as ready for review February 20, 2024 20:02

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 20, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 20, 2024

tssurya force-pushed the remove-same-not-same-labels branch from ac70784 to 1d780b7 Compare February 21, 2024 20:41

tssurya mentioned this pull request Feb 21, 2024

[BUG] Cleanup references to same and notSameLabels once Tenancy API is finalized #197

Open

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 21, 2024

tssurya force-pushed the remove-same-not-same-labels branch from 1d780b7 to 70b035a Compare February 21, 2024 20:53

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2024

danwinship reviewed Mar 7, 2024

View reviewed changes

tssurya force-pushed the remove-same-not-same-labels branch from 70b035a to 6493a47 Compare March 8, 2024 11:35

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2024

Remove sameLabels and notSameLabels from experimental

4d5dd78

Signed-off-by: Surya Seetharaman <[email protected]>

tssurya force-pushed the remove-same-not-same-labels branch from 6493a47 to 4d5dd78 Compare March 25, 2024 19:17

danwinship removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2024

k8s-ci-robot assigned danwinship Mar 25, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 25, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 25, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 25, 2024

k8s-ci-robot merged commit f53b85f into kubernetes-sigs:main Mar 25, 2024
8 checks passed

huntergregory mentioned this pull request Mar 26, 2024

[Policy Assistant] remove support for SameLabels and NotSameLabels #212

Open

tssurya mentioned this pull request Mar 28, 2024

Fix tests to reflect newAPI changes #214

Merged

tssurya mentioned this pull request Apr 10, 2024

[DNM] Pre merge testing server side apply; Unblock QE openshift/ovn-kubernetes#2116

Closed

huntergregory mentioned this pull request Oct 29, 2024

fix: [Policy Assistant] warn that SameLabels and NotSameLabels are deprecated #261

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove same-not-same-labels #196

Remove same-not-same-labels #196

tssurya commented Feb 20, 2024 •

edited

Loading

k8s-ci-robot commented Feb 20, 2024

netlify bot commented Feb 20, 2024 •

edited

Loading

tssurya commented Feb 20, 2024

tssurya commented Feb 21, 2024

astoycos commented Feb 27, 2024

danwinship left a comment

danwinship Mar 7, 2024

tssurya Mar 7, 2024

tssurya Mar 8, 2024 •

edited

Loading

k8s-ci-robot commented Mar 11, 2024

danwinship commented Mar 25, 2024

k8s-ci-robot commented Mar 25, 2024

astoycos commented Mar 25, 2024

Remove same-not-same-labels #196

Remove same-not-same-labels #196

Conversation

tssurya commented Feb 20, 2024 • edited Loading

k8s-ci-robot commented Feb 20, 2024

netlify bot commented Feb 20, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-network-policy-api ready!

tssurya commented Feb 20, 2024

tssurya commented Feb 21, 2024

astoycos commented Feb 27, 2024

danwinship left a comment

Choose a reason for hiding this comment

danwinship Mar 7, 2024

Choose a reason for hiding this comment

tssurya Mar 7, 2024

Choose a reason for hiding this comment

tssurya Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 11, 2024

danwinship commented Mar 25, 2024

k8s-ci-robot commented Mar 25, 2024

astoycos commented Mar 25, 2024

tssurya commented Feb 20, 2024 •

edited

Loading

netlify bot commented Feb 20, 2024 •

edited

Loading

tssurya Mar 8, 2024 •

edited

Loading