Remove duplicated compliance in Gatekeeper synced policy message #76
Conversation
|
@zyjjay Please add the Jira issue to the commit message so that the message propagates to |
controllers/utils/events.go
Outdated
| @@ -32,8 +32,6 @@ func (c *ComplianceEventSender) SendEvent( | |||
| msg string, | |||
| compliance policyv1.ComplianceState, | |||
| ) error { | |||
| msg = string(compliance) + "; " + msg | |||
|
|
|||
There was a problem hiding this comment.
I think removing it here will remove the status from ALL messages (ConfigurationPolicy, IamPolicy, CertificatePolicy, Gatekeeper) rather than just the duplicated Gatekeeper messages.
There was a problem hiding this comment.
Assuming that's the case and this is affecting all policies, the cause might be somewhere in the Gatekeeper sync code instead: https://github.com/open-cluster-management-io/governance-policy-framework-addon/blob/main/controllers/gatekeepersync/gatekeeper_constraint_sync.go
There was a problem hiding this comment.
From what I can gather, the cause seems to come from calls to the emitTemplateEvent() function in template_sync.go. The msgMeta field is supplied with the extra compliance text and concatenated with the compliance state in the subsequent call to sendEvent() in events.go. I do realize now that my current fix may cause unwanted behaviour, though.
There was a problem hiding this comment.
There is also a similar function in gatekeeper_contraint_sync.go called sendComplianceEvent (https://github.com/open-cluster-management-io/governance-policy-framework-addon/blob/main/controllers/gatekeepersync/gatekeeper_constraint_sync.go#L347) that wraps around sendEvent. In here, the msg field is not supplied with a compliance prefix.
There was a problem hiding this comment.
Ah, I see what you mean about the SendEvent function is appending the compliance state, so I'm thinking this isn't just limited to Gatekeeper policies and that's just a coincidence I noticed it there. 🤔
It might be best to stick a check or two in an E2E to check these messages (if they're not checked already) since it feels like this code is arriving here from a few different places.
Also emitTemplateError/Success/Pending could instead pass policiesv1.ComplianceState directly instead of msgMeta, and emitTemplateError could add the template-error; string to the msg instead of msgMeta?
There was a problem hiding this comment.
I was actually thinking about moving the template-error; string to the msg field but wasn't sure if I should since it seemed like a piece of "metadata". I'll go ahead and make those changes now.
There was a problem hiding this comment.
I agree with Dale, I think the problem is more likely in something gatekeeper-specific, otherwise I feel like we'd have noticed "NonCompliant; NonCompliant; template-error;" in other messages.
I could be convinced if there was a test for this: maybe something using this checkForEvent function? https://github.com/open-cluster-management-io/governance-policy-framework-addon/blob/main/test/e2e/case10_error_test.go#L311. I'm just brainstorming
| @@ -1249,7 +1249,7 @@ func overrideRemediationAction(instance *policiesv1.Policy, tObjectUnstructured | |||
| func (r *PolicyReconciler) emitTemplateSuccess( | |||
| ctx context.Context, pol *policiesv1.Policy, tIndex int, tName string, clusterScoped bool, msg string, | |||
| ) error { | |||
| err := r.emitTemplateEvent(ctx, pol, tIndex, tName, clusterScoped, "Normal", "Compliant; ", msg) | |||
| err := r.emitTemplateEvent(ctx, pol, tIndex, tName, clusterScoped, "Normal", string(policiesv1.Compliant), msg) | |||
There was a problem hiding this comment.
Instead, could you please change emitTemplateEvent to accept a compliance state argument instead of msgMeta? It seems only in the case of emitTemplateError does it add an additional prefix, so this can just be added to the message instead.
| @@ -1314,10 +1314,10 @@ func (r *PolicyReconciler) emitTemplatePending( | |||
| // msg are concatenated without spaces, so any spacing should be included inside the msgMeta string. | |||
There was a problem hiding this comment.
Could you please update this comment since it still mentions msgMeta?
|
Since it seems to be a more general fix than just with Gatekeeper, could you please change the commit title to something like |
| @@ -251,6 +251,22 @@ var _ = Describe("Test Gatekeeper ConstraintTemplate and constraint sync", Order | |||
| ) | |||
| }, gkAuditFrequency*3, 1).Should(Succeed()) | |||
|
|
|||
| By("Checking if policy status history is correctly formatted") | |||
There was a problem hiding this comment.
Without the code change, I would think this test would still pass because the issue seems to be in the template-sync's event emitting as opposed to the Gatekeeper sync controller. The original bug report mentioned Gatekeeper but that error actually originated from the template-sync when it was trying to create the Gatekeeper constraint from the policy-templates array.
I would adjust the existing test in case10 to make it more precise.
The compliance state is contenated twice to a message before it is emitted as an event. To fix this, the compliance state is removed from the message and now only passed as an argument. ref: https://issues.redhat.com/browse/ACM-6346 Signed-off-by: Jason Zhang <jaszhang@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mprahl, 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:
Approvers can indicate their approval by writing |
openshift-merge-robot
merged commit 747b933
into
open-cluster-management-io:main
ref: https://issues.redhat.com/browse/ACM-6346