Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log policy NonCompliance #136

Merged
merged 1 commit into from
Aug 9, 2023
Merged

Log policy NonCompliance #136

merged 1 commit into from
Aug 9, 2023

Conversation

gparvin
Copy link
Member

@gparvin gparvin commented May 25, 2023

Consistently log the policy noncompliance and compliance as updates are made. The goal is to allow tools that scrape logs to be able to obtain the violation message and the details on when compliance changes happen.

Refs:

@gparvin
Copy link
Member Author

gparvin commented May 25, 2023

Sample log entry:

2023-05-24T13:37:53.496-0400	info	configuration-policy-controller	controllers/configurationpolicy_controller.go:2915	Policy status message	{"name": "case23-invalid-field", "namespace": "managed", "status": "NonCompliant; violation - Error validating the object case23, the error is `ValidationError(ConfigMap): unknown field \"invalid\" in io.k8s.api.core.v1.ConfigMap`"}

@@ -2911,7 +2908,7 @@ func convertPolicyStatusToString(plc *policyv1.ConfigurationPolicy) (results str
}
}
}

policyLog.Info("Policy status message", "status", result)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the log inside this function might result in it being duplicated sometimes, but I think that is a better problem than missing it sometimes. I wonder if it should be behind V(1) or a special flag, to keep things from getting too noisy?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal is to have this logged by default so servicing systems that only have the GRC governance metrics and logs can determine the NonCompliance messages. Since the NonCompliance message isn't in a metric we need it to always be in log. I'll have to see if the V(1) logs show up by default. I'll do a duplication detection too to see if that happens -- it looks like I've cleaned up the logs I captured last week so will recollect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @JustinKuli. I think these could be very noisy and make the logs grow quickly--I'd prefer we ask that servicing systems bump the log level if they want the information present in the logs. It's not too difficult since we have a custom annotation that can be applied to the ManagedClusterAddon. Does SD have a requirement that the status message be present at default logging levels?

@gparvin
Copy link
Member Author

gparvin commented Jun 2, 2023

/hold
I changed to the V(1) logging -- but I'm not convinced there's a valid concern around log growth due to this change yet.

@gparvin
Copy link
Member Author

gparvin commented Aug 7, 2023

I rebuilt the changes on the latest code base. To get the message to display I had to use: --log-level debug. I couldn't tell that --v 1 does anything. Is that what we expect?

@mprahl
Copy link
Member

mprahl commented Aug 8, 2023

@gparvin could you please log it in updatePolicyStatus instead? Then it can be just a normal info log.

@@ -2826,11 +2826,14 @@ func (r *ConfigurationPolicyReconciler) updatePolicyStatus(
eventType = eventWarning
}

eventMessage := fmt.Sprintf("%s%s", policy.Status.ComplianceState, msg)
log.Info("Policy status message", "status", eventMessage)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logger here doesn't automatically include the policy name. Could you please include that as a key/value pair?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Now it has the policy name. Thanks

Consistently log the policy noncompliance and compliance as updates are
made. The goal is to allow tools that scrape logs to be able to obtain
the violation message and the details on when compliance changes happen.

Refs:
 - https://issues.redhat.com/browse/ACM-5568

Signed-off-by: Gus Parvin <gparvin@redhat.com>
@openshift-ci
Copy link

openshift-ci bot commented Aug 9, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gparvin, mprahl

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gparvin
Copy link
Member Author

gparvin commented Aug 9, 2023

/unhold

@openshift-merge-robot openshift-merge-robot merged commit 73f50ce into open-cluster-management-io:main Aug 9, 2023
2 checks passed
@openshift-cherrypick-robot

@gparvin: cannot checkout release-2.8: error checking out release-2.8: exit status 1. output: error: pathspec 'release-2.8' did not match any file(s) known to git

In response to this:

/cherry-pick release-2.8

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants