Fix compliance when created resource has a status #161

JustinKuli · 2023-08-30T18:12:28Z

Previously, the template would always be marked as compliant after the resource it defines was created. But if the object definition included a status, then the object might not actually match what was desired. Now, in this case, the template will be marked as non-compliant.

Refs:

https://issues.redhat.com/browse/ACM-7020

JustinKuli · 2023-08-31T15:35:16Z

I can squash the refactor commits together if that would be better.

dhaiducek

Overall looks good, and thanks for the refactor work! The renamed variables at least will make it clearer to read/follow!

dhaiducek · 2023-08-31T16:14:59Z

controllers/configurationpolicy_controller.go

+			hasStatus := false
+			if tmplObj, err := unmarshalFromJSON(objectT.ObjectDefinition.Raw); err == nil {
+				_, hasStatus = tmplObj.Object["status"]
+			}
+
+			if completed && hasStatus {
+				msg += ", but has a status that might not be correct yet"
+				reason += ", but has a status that might not be correct yet"
+				result.events = append(result.events, objectTmplEvalEvent{false, reason, msg})
+			} else {
+				result.events = append(result.events, objectTmplEvalEvent{completed, reason, msg})
+			}


This is the creation flow, right? Does it still stay NonCompliant if a NonCompliant policy is updated and the status doesn't match or does some logic need to be there also?

Good catch, somehow I didn't think about the update flow having this same potential issue!

It looks like it might work correctly, but I'll adjust the test to be sure.

Updated test: https://github.com/open-cluster-management-io/config-policy-controller/compare/eceb957c80fbc032d898a463b02be38fbd6fa600..3421b2310929e5f8b3e6aafc17034c1352487cc3

Initially, the CI here failed, but from the logs it looked like that was because the config-policy-controller was a little overwhelmed with the parallel tests. Because this test uses the status of another configurationpolicy, it can take extra loops to do what is expected... so bumping the timeouts to the default (1 minute) fixed it.

mprahl · 2023-09-06T18:11:20Z

controllers/configurationpolicy_controller.go

+			}
+
+			if completed && hasStatus {
+				msg += ", but has a status that might not be correct yet"


How about this?

Suggested change

msg += ", but has a status that might not be correct yet"

msg += ". The status will be validated on the next evaluation."

Instead, perhaps msg += ". The status will be verified on the next evaluation."

Good suggestion! I've reworded it slightly again, and I wanted to keep the comma: if a policy has multiple objects, the messages will be combined with semicolons, so using a period in the middle of a message seemed like it could be weird.

I also adjusted the reason to be shorter now.

mprahl · 2023-09-06T18:13:11Z

test/resources/case34_enforce_w_status/config-policy.yaml

+                metadata:
+                  name: default
+        status:
+          lastEvaluatedGeneration: 3


Haha--I thought the same thing! I had to give it a double-take.

I like config policies, so I put a config policy in my config policy, so I can test config policies with config policies.

mprahl

This looks good! The only thing is to consider changing the noncompliant message.

yiraeChristineKim · 2023-09-07T13:58:43Z

test/e2e/case34_enforce_w_status_test.go

@@ -0,0 +1,78 @@
+// Copyright (c) 2023 Red Hat, Inc.
+// Copyright Contributors to the Open Cluster Management project


the file name what does "w" mean?

"with" - as in "enforce with status"

Previously, the template would always be marked as compliant after the resource it defines was created. But if the object definition included a `status`, then the object might not actually match what was desired. Now, in this case, the template will be marked as non-compliant. Refs: - https://issues.redhat.com/browse/ACM-7020 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

The `object` and `unstruct` fields were not self-explanatory. Now they are `existingObj` and `desiredObj` respectively. This will help readers understand why they are used the way they are. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

In both instances, the object is already assured to be non-nil, so there are no errors that can occur here. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

By keeping track of which branch we're in, it became obvious that the `generateSingleObjReason` calls were actually just static lookups. So that function has been removed. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

Putting this in the function itself means that the function could be called from multiple places, and the metrics would still be recorded. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

openshift-ci · 2023-09-07T15:31:56Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

openshift-ci bot added do-not-merge/work-in-progress dco-signoff: yes approved labels Aug 30, 2023

JustinKuli marked this pull request as ready for review August 31, 2023 15:34

openshift-ci bot removed the do-not-merge/work-in-progress label Aug 31, 2023

openshift-ci bot requested review from dhaiducek and yiraeChristineKim August 31, 2023 15:34

dhaiducek reviewed Aug 31, 2023

View reviewed changes

JustinKuli force-pushed the 7020-enforced-w-status-compliance branch 2 times, most recently from 3421b23 to d9736b5 Compare August 31, 2023 19:02

JustinKuli requested a review from dhaiducek September 1, 2023 18:13

mprahl self-requested a review September 6, 2023 17:42

mprahl reviewed Sep 6, 2023

View reviewed changes

yiraeChristineKim reviewed Sep 7, 2023

View reviewed changes

JustinKuli added 5 commits September 7, 2023 10:33

Refactor: better uid lookup

85c4b45

In both instances, the object is already assured to be non-nil, so there are no errors that can occur here. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

Refactor: clarify conditions in handleSingleObj

d5e47ee

By keeping track of which branch we're in, it became obvious that the `generateSingleObjReason` calls were actually just static lookups. So that function has been removed. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

Refactor: move metrics for checkAndUpdateResource

02cf4b5

Putting this in the function itself means that the function could be called from multiple places, and the metrics would still be recorded. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

JustinKuli force-pushed the 7020-enforced-w-status-compliance branch from d9736b5 to 02cf4b5 Compare September 7, 2023 14:34

mprahl approved these changes Sep 7, 2023

View reviewed changes

openshift-ci bot assigned mprahl Sep 7, 2023

openshift-ci bot added the lgtm label Sep 7, 2023

yiraeChristineKim approved these changes Sep 7, 2023

View reviewed changes

openshift-ci bot assigned yiraeChristineKim Sep 7, 2023

openshift-merge-robot merged commit 406ccf7 into open-cluster-management-io:main Sep 7, 2023

magic-mirror-bot bot mentioned this pull request Sep 7, 2023

🤖 Sync from open-cluster-management-io/config-policy-controller: #161 stolostron/config-policy-controller#575

Merged

JustinKuli deleted the 7020-enforced-w-status-compliance branch July 25, 2024 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compliance when created resource has a status #161

Fix compliance when created resource has a status #161

JustinKuli commented Aug 30, 2023

JustinKuli commented Aug 31, 2023

dhaiducek left a comment

dhaiducek Aug 31, 2023 •

edited

Loading

JustinKuli Aug 31, 2023

JustinKuli Aug 31, 2023

JustinKuli Aug 31, 2023

JustinKuli Aug 31, 2023

mprahl Sep 6, 2023

mprahl Sep 6, 2023

JustinKuli Sep 7, 2023

mprahl Sep 6, 2023

dhaiducek Sep 6, 2023

JustinKuli Sep 7, 2023

mprahl left a comment

yiraeChristineKim Sep 7, 2023

JustinKuli Sep 7, 2023

openshift-ci bot commented Sep 7, 2023

	msg += ", but has a status that might not be correct yet"
	msg += ". The status will be validated on the next evaluation."

		@@ -0,0 +1,78 @@
		// Copyright (c) 2023 Red Hat, Inc.
		// Copyright Contributors to the Open Cluster Management project

Fix compliance when created resource has a status #161

Fix compliance when created resource has a status #161

Conversation

JustinKuli commented Aug 30, 2023

JustinKuli commented Aug 31, 2023

dhaiducek left a comment

Choose a reason for hiding this comment

dhaiducek Aug 31, 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

mprahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 7, 2023

dhaiducek Aug 31, 2023 •

edited

Loading