Empty label fields in policies are ignored #150

zyjjay · 2023-07-06T15:29:00Z

Fix parsing issue when policy label fields are set to empty strings

ref: https://issues.redhat.com/browse/ACM-5398

controllers/configurationpolicy_utils.go

yiraeChristineKim · 2023-08-15T21:35:10Z

test/e2e/case33_empty_label_test.go

+					found := false
+
+					_, ok := labelMap[case33StorageLabel]
+					if ok {
+						found = true
+					}
+
+					return found


Suggested change

found := false

_, ok := labelMap[case33StorageLabel]

if ok {

found = true

}

return found

_, ok := labelMap[case33StorageLabel]

if ok {

return true

}

return false

Alternatively, this would probably be better handled by the HaveKey() Gomega assertion rather than returning a vague boolean.

You could even use HaveKeyWithValue() to assert both the key and the value to combine both of this and the next test.

https://onsi.github.io/gomega/#havekeykey-interface

yiraeChristineKim · 2023-08-15T21:37:41Z

test/e2e/case33_empty_label_test.go

+				Eventually(func() interface{} {
+					node, _ := clientManaged.CoreV1().Nodes().Get(context.TODO(), case33NodeName, metav1.GetOptions{})
+					labelMap := node.GetLabels()
+					merged := labelMap[case33StorageLabel]


Suggested change

merged := labelMap[case33StorageLabel]

_, ok := labelMap[case33StorageLabel]

return ok

We wanted to verify whether the label exist or not

I verified it in the Eventually block above this. This check is to make sure the value of the field is an empty string.

yiraeChristineKim · 2023-08-15T21:44:11Z

kubeconfig_test-managed-control-plane

@@ -0,0 +1,17 @@
+apiVersion: v1


Where this file used? cannot find

Oh I have no idea what this is either, I will fix up my PR to remove this.

Its a kubeconfig file. This looks to me to be the result of naming the managed cluster test-managed instead of just managed and apparently we don't have that in our .gitignore. 🙂

dhaiducek · 2023-08-16T13:52:10Z

test/e2e/case33_empty_label_test.go

+const (
+	case33ConfigPolicy     = "../resources/case33_empty_label_handling/empty-labels-config.yaml"
+	case33ConfigPolicyName = "case33-empty-labels"
+	case33NodeName         = "test-managed-control-plane"
+	case33StorageLabel     = "cluster.ocs.openshift.io/openshift-storage"
+)
+
+var _ = Describe("Test resource creation when there are empty labels in configurationPolicy", Ordered, func() {
+	Describe("Appending empty labels to a Node", func() {
+		Context("When the Node already exists", func() {


We shouldn't be using global variables in tests unless they truly need to be global (it was an inadvisable practice that was used previously and hadn't been cleaned up yet).

Also, it seems to me the Describe() and Context() nodes aren't necessary with a single test suite.

Suggested change

const (

case33ConfigPolicy = "../resources/case33_empty_label_handling/empty-labels-config.yaml"

case33ConfigPolicyName = "case33-empty-labels"

case33NodeName = "test-managed-control-plane"

case33StorageLabel = "cluster.ocs.openshift.io/openshift-storage"

)

var _ = Describe("Test resource creation when there are empty labels in configurationPolicy", Ordered, func() {

Describe("Appending empty labels to a Node", func() {

Context("When the Node already exists", func() {

var _ = Describe("Test resource creation when there are empty labels in configurationPolicy", Ordered, func() {

const (

configPolicy = "../resources/case33_empty_label_handling/empty-labels-config.yaml"

configPolicyName = "case33-empty-labels"

nodeName = "test-managed-control-plane"

storageLabel = "cluster.ocs.openshift.io/openshift-storage"

)

dhaiducek · 2023-08-16T13:53:43Z

kubeconfig_test-managed-control-plane

@@ -0,0 +1,17 @@
+apiVersion: v1


Its a kubeconfig file. This looks to me to be the result of naming the managed cluster test-managed instead of just managed and apparently we don't have that in our .gitignore. 🙂

dhaiducek · 2023-08-16T14:11:03Z

test/resources/case33_empty_label_handling/empty-labels-config.yaml

+      metadata:
+        name: test-managed-control-plane
+        labels:
+          cluster.ocs.openshift.io/openshift-storage: ""


As a sanity check, could we also check using a policy that sets cluster.ocs.openshift.io/openshift-storage: null to make sure the policy doesn't remove the field? (Though I'm also uncertain what the behavior would be there in terms of compliance...)

It would be nice to have a test just to check what this currently does, and understand if that behavior has changed (or if we change it accidentally in the future)

Currently, having an unset label/setting it to nil results in the deletion of all the labels attached to the object. I'll probably make a separate ticket to address this issue as it's a bit more involved and possibly affect annotations as well (need to confirm this but they both use the same helper function to populate the fields).

JustinKuli · 2023-08-16T14:35:49Z

test/e2e/case33_empty_label_test.go

+const (
+	case33ConfigPolicy     = "../resources/case33_empty_label_handling/empty-labels-config.yaml"
+	case33ConfigPolicyName = "case33-empty-labels"
+	case33NodeName         = "test-managed-control-plane"


I'd prefer to use a different resource for the test. This relies on using a kind cluster of a specific name. If the test creates a configmap or something, the policy could just label that.

JustinKuli

/hold

Looks good to me! Thanks for the changes to the tests, it looks great now. Just holding for any other reviewer comments.

test/e2e/case33_empty_label_test.go

mprahl · 2023-08-22T16:18:27Z

@zyjjay could you please format your commit message title and description to match the contribution guidelines?
https://github.com/open-cluster-management-io/community/blob/main/sig-policy/contribution-guidelines.md#commit-messages

Also, ACM shouldn't be mentioned in the commit message since this is in the upstream Open Cluster Management community.

yiraeChristineKim · 2023-08-22T22:01:31Z

/lgtm

yiraeChristineKim · 2023-08-22T22:01:35Z

/hold

yiraeChristineKim · 2023-08-22T22:02:24Z

/hold
Until the commit message is changed

mprahl · 2023-08-23T13:12:55Z

@zyjjay do you need help with changing the commit message?

Fix parsing issue when policy label fields are set to empty strings ref: https://issues.redhat.com/browse/ACM-5398 Signed-off-by: Jason Zhang <jaszhang@redhat.com>

mprahl · 2023-08-23T14:33:49Z

@zyjjay the commit title should start with a verb in the imperative mood (i.e. Fix ....). Please do this next time.

mprahl

/unhold

openshift-ci · 2023-08-23T14:34:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JustinKuli, mprahl, yiraeChristineKim, zyjjay

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~~ [JustinKuli,mprahl,yiraeChristineKim]

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

yiraeChristineKim · 2023-08-30T20:32:11Z

/cherry-pick release-2.7

mprahl · 2023-08-30T20:39:48Z

/cherry-pick release-2.7

You have to do that on the Stolostron PR:
stolostron/config-policy-controller#568

openshift-cherrypick-robot · 2023-08-30T20:41:40Z

@yiraeChristineKim: only open-cluster-management-io org members may request cherry picks. You can still do the cherry-pick manually.

In response to this:

/cherry-pick release-2.7

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.

openshift-ci bot requested review from dhaiducek and yiraeChristineKim July 6, 2023 15:29

openshift-ci bot added the dco-signoff: yes label Jul 6, 2023

zyjjay force-pushed the ACM-5398 branch from c1d2f93 to 89db260 Compare July 6, 2023 15:50

openshift-ci bot added dco-signoff: no and removed dco-signoff: yes labels Aug 10, 2023

zyjjay force-pushed the ACM-5398 branch from e5e04f6 to 46528da Compare August 15, 2023 19:40

zyjjay changed the title ~~Bug: ACM-5398, ACM ignores Policies about empty label~~ ACM ignores Policies about empty label Aug 15, 2023

openshift-ci bot added dco-signoff: yes and removed dco-signoff: no labels Aug 15, 2023

yiraeChristineKim reviewed Aug 15, 2023

View reviewed changes

controllers/configurationpolicy_utils.go Show resolved Hide resolved

yiraeChristineKim requested changes Aug 15, 2023

View reviewed changes

openshift-ci bot assigned yiraeChristineKim Aug 15, 2023

yiraeChristineKim reviewed Aug 15, 2023

View reviewed changes

yiraeChristineKim requested a review from JustinKuli August 16, 2023 14:03

dhaiducek reviewed Aug 16, 2023

View reviewed changes

JustinKuli reviewed Aug 16, 2023

View reviewed changes

zyjjay force-pushed the ACM-5398 branch from 46528da to 07cd21b Compare August 16, 2023 21:04

openshift-ci bot added dco-signoff: no and removed dco-signoff: yes labels Aug 17, 2023

zyjjay force-pushed the ACM-5398 branch from 992589e to 443687d Compare August 17, 2023 14:59

openshift-ci bot added dco-signoff: yes and removed dco-signoff: no labels Aug 17, 2023

JustinKuli previously approved these changes Aug 21, 2023

View reviewed changes

openshift-ci bot assigned JustinKuli Aug 21, 2023

openshift-ci bot added do-not-merge/hold lgtm approved labels Aug 21, 2023

yiraeChristineKim reviewed Aug 21, 2023

View reviewed changes

test/e2e/case33_empty_label_test.go Show resolved Hide resolved

zyjjay dismissed JustinKuli’s stale review via 1c380b7 August 22, 2023 16:16

zyjjay force-pushed the ACM-5398 branch from 443687d to 1c380b7 Compare August 22, 2023 16:16

openshift-ci bot removed lgtm approved labels Aug 22, 2023

zyjjay changed the title ~~ACM ignores Policies about empty label~~ Empty label fields in policies are ignored Aug 22, 2023

openshift-ci bot added the lgtm label Aug 22, 2023

yiraeChristineKim approved these changes Aug 22, 2023

View reviewed changes

openshift-ci bot added the approved label Aug 22, 2023

Empty label fields in policies are ignored

33b5728

Fix parsing issue when policy label fields are set to empty strings ref: https://issues.redhat.com/browse/ACM-5398 Signed-off-by: Jason Zhang <jaszhang@redhat.com>

zyjjay force-pushed the ACM-5398 branch from 1c380b7 to 33b5728 Compare August 23, 2023 14:24

openshift-ci bot removed the lgtm label Aug 23, 2023

JustinKuli approved these changes Aug 23, 2023

View reviewed changes

openshift-ci bot added the lgtm label Aug 23, 2023

mprahl approved these changes Aug 23, 2023

View reviewed changes

openshift-ci bot removed the do-not-merge/hold label Aug 23, 2023

openshift-ci bot assigned mprahl Aug 23, 2023

openshift-merge-robot merged commit 7c94409 into open-cluster-management-io:main Aug 23, 2023
2 checks passed

This was referenced Aug 23, 2023

🤖 Sync from open-cluster-management-io/config-policy-controller: #150 stolostron/config-policy-controller#568

Merged

😿 Failed to sync the upstream PRs: #150 stolostron/config-policy-controller#569

Closed

zyjjay mentioned this pull request Sep 7, 2023

Application to become OCM member open-cluster-management-io/community#183

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Empty label fields in policies are ignored #150

Empty label fields in policies are ignored #150

zyjjay commented Jul 6, 2023 •

edited

Loading

yiraeChristineKim Aug 15, 2023 •

edited

Loading

dhaiducek Aug 16, 2023

yiraeChristineKim Aug 15, 2023 •

edited

Loading

zyjjay Aug 16, 2023

yiraeChristineKim Aug 15, 2023

zyjjay Aug 16, 2023

dhaiducek Aug 16, 2023

dhaiducek Aug 16, 2023

dhaiducek Aug 16, 2023

dhaiducek Aug 16, 2023

JustinKuli Aug 16, 2023

zyjjay Aug 16, 2023

JustinKuli Aug 16, 2023

JustinKuli left a comment

mprahl commented Aug 22, 2023

yiraeChristineKim commented Aug 22, 2023

yiraeChristineKim commented Aug 22, 2023

yiraeChristineKim commented Aug 22, 2023 •

edited

Loading

mprahl commented Aug 23, 2023

mprahl commented Aug 23, 2023

mprahl left a comment

openshift-ci bot commented Aug 23, 2023

yiraeChristineKim commented Aug 30, 2023

mprahl commented Aug 30, 2023

openshift-cherrypick-robot commented Aug 30, 2023

	merged := labelMap[case33StorageLabel]
	_, ok := labelMap[case33StorageLabel]
	return ok

Empty label fields in policies are ignored #150

Empty label fields in policies are ignored #150

Conversation

zyjjay commented Jul 6, 2023 • edited Loading

yiraeChristineKim Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiraeChristineKim Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinKuli left a comment

Choose a reason for hiding this comment

mprahl commented Aug 22, 2023

yiraeChristineKim commented Aug 22, 2023

yiraeChristineKim commented Aug 22, 2023

yiraeChristineKim commented Aug 22, 2023 • edited Loading

mprahl commented Aug 23, 2023

mprahl commented Aug 23, 2023

mprahl left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 23, 2023

yiraeChristineKim commented Aug 30, 2023

mprahl commented Aug 30, 2023

openshift-cherrypick-robot commented Aug 30, 2023

zyjjay commented Jul 6, 2023 •

edited

Loading

yiraeChristineKim Aug 15, 2023 •

edited

Loading

yiraeChristineKim Aug 15, 2023 •

edited

Loading

yiraeChristineKim commented Aug 22, 2023 •

edited

Loading