Refactor object template status generation #143

mprahl · 2023-06-15T16:19:06Z

The code is ready for review but I will need to update some E2E tests.

This refactoring takes away the responsibility of individual methods of updating the status and instead they return one or more status events. The handleObjectTemplates method then generates status events using the entire context of results from the object-template to be able to consolidate messages and include different messages per namespace.

Relates:
https://issues.redhat.com/browse/ACM-5175

mprahl · 2023-06-15T16:19:13Z

/hold

mprahl · 2023-06-20T19:11:23Z

/unhold this is ready for review

JustinKuli

These changes haven't really "clicked" for me yet... I only have some initial comments, I need to look at it again.

JustinKuli · 2023-06-20T19:38:34Z

controllers/configurationpolicy_utils.go

+ var namesStr string
+
+ if len(namespaceToEvent[ns].result.objectNames) > 0 {
+ namesStr = " ["
+
+ for i, name := range namespaceToEvent[ns].result.objectNames {
+ namesStr += name
+ if i != len(namespaceToEvent[ns].result.objectNames)-1 {
+ namesStr += ", "
+ }
+ }
+
+ namesStr += "]"
+ }


Suggested change

var namesStr string

if len(namespaceToEvent[ns].result.objectNames) > 0 {

namesStr = " ["

for i, name := range namespaceToEvent[ns].result.objectNames {

namesStr += name

if i != len(namespaceToEvent[ns].result.objectNames)-1 {

namesStr += ", "

}

}

namesStr += "]"

}

namesStr := ""

if len(namespaceToEvent[ns].result.objectNames) > 0 {

namesStr = " [" + strings.Join(namespaceToEvent[ns].result.objectNames, ", ") + "]"

}

I know we have this same kind of loop somewhere else, but I think we should use the stdlib for this.

JustinKuli · 2023-06-20T20:38:28Z

controllers/configurationpolicy_controller.go

- r.Recorder.Event(policy, eventType, fmt.Sprintf(eventFmtStr, policy.GetName(), objDetails.name),
- convertPolicyStatusToString(policy))
+ result = objectResult{
+ []string{objName},


What happens when this objName is empty? 🤔

It ends up not being used because it's an error case, so it falls in the default switch case. Here is an example of the status:

conditions: - lastTransitionTime: "2023-06-21T12:24:28Z" message: namespaced object of kind Role has no namespace specified from the policy namespaceSelector nor the object metadata reason: K8s missing namespace status: "True" type: violation

I added a unit test for this situation to illustrate this. I hope this helps clarify it.

JustinKuli · 2023-06-20T20:43:27Z

controllers/configurationpolicy_controller.go

+type objectResult struct {
+ objectNames []string
+ namespace string
+ events []objectResultEvent
+}
+
+type objectResultEvent struct {
+ compliant bool
+ reason string
+ message string
+}
+
+type objectResultEventWithCtx struct {
+ result objectResult
+ event objectResultEvent
+}


I think these names are pretty confusing. I don't have anything better though...

I agree. I tried renaming them. Let me know if the new names are better.

JustinKuli · 2023-06-20T20:47:00Z

controllers/configurationpolicy_controller.go

+ result = objectResult{
+ objectNames: []string{obj.name},
+ namespace: obj.namespace,
+ events: append(result.events, objectResultEvent{completed, reason, msg}),
+ }


Why not just append to the events here (like 2 lines down) ?

Suggested change

result = objectResult{

objectNames: []string{obj.name},

namespace: obj.namespace,

events: append(result.events, objectResultEvent{completed, reason, msg}),

}

result.events = append(result.events, objectResultEvent{completed, reason, msg})

JustinKuli · 2023-06-20T20:59:34Z

controllers/configurationpolicy_controller.go

+ // Determine the applicable batch. For example, if there are 3 batches but 2 events, we need
+ // to skip the first batch and append to the second and third batches.


I definitely need more explanation for this comment. Why skip any batches? Why specifically the first?

I updated the comment with a real example. Does this help?

Signed-off-by: mprahl <mprahl@users.noreply.github.com>

yiraeChristineKim · 2023-06-23T13:44:14Z

Makefile

@@ -205,7 +205,7 @@ install-resources:

 .PHONY: e2e-test
 e2e-test: e2e-dependencies
- $(GINKGO) -v --fail-fast $(E2E_TEST_ARGS) test/e2e
+ $(GINKGO) -v --timeout=2h --fail-fast $(E2E_TEST_ARGS) test/e2e


We can remove this. I fixed flaky e2e test issue.

Once #145 is merged, I'll rebase this PR and remove the increased timeout.

yiraeChristineKim · 2023-06-23T15:11:13Z

Would you refactor the lines from 88 to 91 in configurationpolicy_utils. currentObject.Object.APIVersion == relatedObject.Object.APIVersion && currentObject.Object.Kind == relatedObject.Object.Kind && currentObject.Object.Metadata.Name == relatedObject.Object.Metadata.Name && currentObject.Object.Metadata.Namespace == relatedObject.Object.Metadata.Namespace. we can just compare the objects, such as relatedObject.Object vs relatedObject.Object

controllers/configurationpolicy_controller.go

yiraeChristineKim · 2023-06-23T16:16:20Z

controllers/configurationpolicy_utils.go

+
+ for namespace, eventWithCtx := range namespaceToEvent {
+ // If a noncompliant event is encountered, then reset the maps to only include noncompliant events.
+ if compliant && !eventWithCtx.event.compliant {


I love this part. Amazing logic flow!

yiraeChristineKim · 2023-06-23T16:18:51Z

controllers/configurationpolicy_controller.go

-// createMergedStatus updates the status field for a configurationpolicy with remediationAction=inform
-// based on how many compliant/noncompliant objects are found when processing the templates in the configurationpolicy
-// Or multiple namespaces and enforce use this
-func createMergedStatus(


lovely remove this part

mprahl · 2023-06-26T13:20:07Z

currentObject.Object.APIVersion ==

@yiraeChristineKim I think since that change is unrelated to this PR, I'd prefer it in a separate PR.

This refactoring takes away the responsibility of individual methods of updating the status and instead they return one or more status events. The handleObjectTemplates method then generates status events using the entire context of results from the object-template to be able to consolidate messages and include different messages per namespace. Relates: https://issues.redhat.com/browse/ACM-5175 Signed-off-by: mprahl <mprahl@users.noreply.github.com>

mprahl · 2023-06-26T15:25:58Z

/hold for reviews

dhaiducek

A couple questions about whether passing a pointer is necessary, but overall a great update. The batching pulls together similar actions nicely (though I'll be honest some of the more elaborate batching logic I kind of took your word it worked 😆 )

dhaiducek · 2023-06-27T11:57:35Z

controllers/configurationpolicy_controller.go

 policy *policyv1.ConfigurationPolicy,
 index int,
-) (mapping *meta.RESTMapping, updateNeeded bool) {
+) (mapping *meta.RESTMapping, result *objectTmplEvalResult) {


Nit: The policy argument no longer needs to be a pointer

The pointer is nice in that it prevents the policy data from being copied in memory, however, it does mean the function could unintentionally modify the policy.

What do you think @yiraeChristineKim and @JustinKuli?

If our controller is out of memory, I agree to use a pointer. Otherwise, mappingErrResult is only used
line 1159, so I vote for removing Pointer

mappingErrResult != nil { nsToResults[ns] = *mappingErrResult continue }

Oh no, getmapping(helper) is used only here... let's go with pointer to save memory

dhaiducek · 2023-06-27T12:50:45Z

controllers/configurationpolicy_controller.go

@@ -2931,7 +2867,7 @@ func (r *ConfigurationPolicyReconciler) sendComplianceEvent(instance *policyv1.C
 }

 // convertPolicyStatusToString to be able to pass the status as event
-func convertPolicyStatusToString(plc *policyv1.ConfigurationPolicy) (results string) {
+func convertPolicyStatusToString(plc *policyv1.ConfigurationPolicy) string {


Nit: I'm not sure the plc argument needs to be a pointer?

openshift-ci · 2023-06-27T13:46:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaiducek, 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~~ [dhaiducek,mprahl,yiraeChristineKim]

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

mprahl · 2023-06-27T13:56:26Z

/unhold

mprahl requested review from JustinKuli and yiraeChristineKim June 15, 2023 16:19

openshift-ci bot added do-not-merge/work-in-progress dco-signoff: yes labels Jun 15, 2023

openshift-ci bot requested a review from gparvin June 15, 2023 16:19

openshift-ci bot added do-not-merge/hold approved labels Jun 15, 2023

mprahl force-pushed the refactor-status branch 9 times, most recently from e4b0360 to ff294aa Compare June 20, 2023 17:40

mprahl changed the title ~~WIP: Refactor object template status generation~~ Refactor object template status generation Jun 20, 2023

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

JustinKuli reviewed Jun 20, 2023

View reviewed changes

mprahl force-pushed the refactor-status branch 6 times, most recently from f8ca607 to 7989040 Compare June 22, 2023 12:06

mprahl requested a review from JustinKuli June 22, 2023 13:28

Increase the ginkgo timeout to 2 hours

4f96d22

Signed-off-by: mprahl <mprahl@users.noreply.github.com>

mprahl force-pushed the refactor-status branch from 7989040 to 2193e79 Compare June 22, 2023 15:06

mprahl requested a review from dhaiducek June 22, 2023 15:15

yiraeChristineKim reviewed Jun 23, 2023

View reviewed changes

controllers/configurationpolicy_controller.go Outdated Show resolved Hide resolved

yiraeChristineKim reviewed Jun 23, 2023

View reviewed changes

mprahl force-pushed the refactor-status branch from 2193e79 to f07a1a9 Compare June 26, 2023 13:38

mprahl requested a review from yiraeChristineKim June 26, 2023 13:38

mprahl force-pushed the refactor-status branch from f07a1a9 to 3427ba8 Compare June 26, 2023 13:53

dhaiducek approved these changes Jun 27, 2023

View reviewed changes

openshift-ci bot assigned dhaiducek Jun 27, 2023

openshift-ci bot added the lgtm label Jun 27, 2023

yiraeChristineKim approved these changes Jun 27, 2023

View reviewed changes

openshift-ci bot assigned yiraeChristineKim Jun 27, 2023

openshift-ci bot removed the do-not-merge/hold label Jun 27, 2023

openshift-merge-robot merged commit 3237da5 into open-cluster-management-io:main Jun 27, 2023
2 checks passed

This was referenced Jun 27, 2023

🤖 Sync from open-cluster-management-io/config-policy-controller: #143 stolostron/config-policy-controller#524

Merged

😿 Failed to sync the upstream PRs: #143 stolostron/config-policy-controller#525

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor object template status generation #143

Refactor object template status generation #143

mprahl commented Jun 15, 2023

mprahl commented Jun 15, 2023

mprahl commented Jun 20, 2023

JustinKuli left a comment

JustinKuli Jun 20, 2023

JustinKuli Jun 20, 2023

mprahl Jun 21, 2023

mprahl Jun 21, 2023

JustinKuli Jun 20, 2023

mprahl Jun 21, 2023

yiraeChristineKim Jun 23, 2023

JustinKuli Jun 20, 2023

JustinKuli Jun 20, 2023

mprahl Jun 21, 2023

yiraeChristineKim Jun 23, 2023

mprahl Jun 26, 2023

yiraeChristineKim commented Jun 23, 2023

yiraeChristineKim Jun 23, 2023

yiraeChristineKim Jun 23, 2023

mprahl commented Jun 26, 2023

mprahl commented Jun 26, 2023

dhaiducek left a comment

dhaiducek Jun 27, 2023

mprahl Jun 27, 2023

yiraeChristineKim Jun 27, 2023

yiraeChristineKim Jun 27, 2023 •

edited

Loading

dhaiducek Jun 27, 2023

openshift-ci bot commented Jun 27, 2023

mprahl commented Jun 27, 2023

		// Determine the applicable batch. For example, if there are 3 batches but 2 events, we need
		// to skip the first batch and append to the second and third batches.

Refactor object template status generation #143

Refactor object template status generation #143

Conversation

mprahl commented Jun 15, 2023

mprahl commented Jun 15, 2023

mprahl commented Jun 20, 2023

JustinKuli left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiraeChristineKim commented Jun 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mprahl commented Jun 26, 2023

mprahl commented Jun 26, 2023

dhaiducek left a 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

yiraeChristineKim Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 27, 2023

mprahl commented Jun 27, 2023

yiraeChristineKim Jun 27, 2023 •

edited

Loading