Add RelatedObject property when compliant dry run overrides policy mismatch

[jan-law]

When configuration policies detect desired vs actual object mismatches, the dry run update validation sometimes responds with a "no-op", aka "no changes required" due to apiserver webhooks or mutating admission controllers modifying requests. The policy is assumed to be compliant without indicating any potential mismatch to users.

Since the requests are modified at the apiserver, we do not have other means of validating the policy before making the dry run request. To improve user experience when troubleshooting this scenario, this PR adds the property DryRunNoOpOverride (edit: MatchesAfterDryRun) when the policy detects a mismatch AND the no-op dry run update overrode the policy assessment.

The RelatedObject Properties object will appear for policies in any remediation mode. Previously, all properties were hidden for all compliant objects in Inform mode.

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

[jan-law]

Note: Creating a policy with this dry run behaviour always emits two events, a "violation" followed by an immediate "compliant" notification. Since we intend to modify the rate of policy history evaluation, I did not investigate the cause of these double events.

[yiraeChristineKim]

/cc @JustinKuli

[yiraeChristineKim]

What is no-op mean?

[yiraeChristineKim]

Are you going to add e2e test?

[yiraeChristineKim]

Can you explain me "!throwSpecViolation", according to " // The spec didn't require a change but throwSpecViolation indicates the status didn't match. Handle" We set compliant when there is mismatch(changed by kube api, mutator) I am confused

[jan-law]

throwSpecViolation is initially true when handleKeys() notices a difference between the policy fields and the actual object fields.

Afterwards, the dry run validation handles edge cases where handleKeys found a difference between unset and empty fields. If the dry run Update did not update any fields, the policy is assumed to be compliant, and throwSpecViolation is inverted.

config-policy-controller/controllers/configurationpolicy_controller.go

Line 3252 in b390ed7

// There are situations where updateNeeded is wrong in either direction: an update might not be

In this case ACM-14577, the bad scc annotation in the policy is technically invalid, but the dry run validation can’t detect the error. During the dry run update request, an admission controller intercepts the request body and silently fixes the invalid annotation before the api server returns a response. This means the dry run update thinks the object matches the policy.

[yiraeChristineKim]

!throwSpecViolation => noncompliant in setEvaluatedObject func right?

[yiraeChristineKim]

Also can you add comment

[jan-law]

In ACM-14577, !throwSpecViolation evaluates to true, compliant

[yiraeChristineKim]

okie Can you add comment for this

[JustinKuli]

Maybe MatchesAfterDryRun would be a better name? "Override" here makes me feel more like it's an option that the user could specify.

[JustinKuli]

Also, this might not need to be a pointer.

[yiraeChristineKim]

@JustinKuli What do you think about skipping the E2E test for this? We could use a Gatekeeper mutation to test it instead, but if you think that's too much, I'm okay with skipping the E2E test.

[JustinKuli]

@yiraeChristineKim if creating an e2e test for this in a KinD cluster is too much of a burden, I would be ok with a test that just runs in our framework tests against an openshift cluster, since the openshift SCC annotation example would be relatively simple to test.

However, I'd want us to try to make a test here first. I think that k8s will default the type field in a Service to ClusterIP. So I think we can make a test that takes an existing Service in the cluster of that type, and copy all of its info into a mustonlyhave policy, except for the type. I would expect that to trigger this behavior, and for the new property to be marked in the related object (although I could be misunderstanding this exact case).

Or, @jan-law could find a better example for a test.

[jan-law]

Question: Could you help me understand why the policy in case8 with metadataComplianceType: musthave is NonCompliant when both the policy and the service yaml do not contain any annotations:?

See diff below when I removed lines 6-7 annotations: test:test from the service.

 compliant: NonCompliant
  lastEvaluated: "2025-08-11T14:41:05Z"
  lastEvaluatedGeneration: 1
  relatedObjects:
  - compliant: NonCompliant
    object:
      apiVersion: v1
      kind: Service
      metadata:
        name: grc-policy-propagator-metrics
        namespace: managed
    properties:
      createdByPolicy: false
      diff: |
        --- managed/grc-policy-propagator-metrics : existing
        +++ managed/grc-policy-propagator-metrics : updated
        @@ -1,9 +1,8 @@
         apiVersion: v1
         kind: Service
         metadata:
        -  annotations: {}
           creationTimestamp: "2025-08-11T14:40:35Z"
           name: grc-policy-propagator-metrics
           namespace: managed
           resourceVersion: "2513"
           uid: ddf71564-cba9-41fa-809f-b742bd85ab50

[JustinKuli]

That's very odd!

Maybe https://github.com/open-cluster-management-io/config-policy-controller/blob/main/controllers/configurationpolicy_controller.go#L3524 is to blame... it seems like that could make the existing object "look like" it has an empty annotations object there when it really doesn't. Maybe the SetAnnotations and SetLabels calls should only be done when there are annotations/labels.

I think we should remove the annotation from your test YAML, and make sure that this situation works as we'd expect. But I'm surprised that no other tests are hitting this odd behavior.

[JustinKuli]

Thanks to your test, I was able to narrow down what seems to be happening. When the service annotations are empty, the reflect.DeepEqual(dryRunUpdatedObj.Object, existingObjectCopy.Object) is coming back false because one of the objects has an empty annotations map, and the other has no annotations map. I don't exactly know why they are different, and especially why the same situation is not happening with the empty labels.

But adjusting this existing function seems like it could handle it (I've added the last 3 lines):

func removeFieldsForComparison(obj *unstructured.Unstructured) {
	unstructured.RemoveNestedField(obj.Object, "metadata", "managedFields")
	unstructured.RemoveNestedField(
		obj.Object, "metadata", "annotations", "kubectl.kubernetes.io/last-applied-configuration",
	)
	// The generation might actually bump but the API output might be the same.
	unstructured.RemoveNestedField(obj.Object, "metadata", "generation")

	if len(obj.GetAnnotations()) == 0 {
		unstructured.RemoveNestedField(obj.Object, "metadata", "annotations")
	}
}

[JustinKuli]

Excellent tests, and nice work adding that field! I'm suggesting changing many of the return cases in checkAndUpdateResource to default to "false" for the new field, but maybe I'm missing a reason for it to be true?

[JustinKuli]

OperatorPolicy doesn't need this field, does it?

We can add an additional patch to the CRD to remove it, see deploy/crds/kustomize_operatorpolicy/remove-diff.json (and it will need to be referenced in deploy/crds/kustomize_operatorpolicy/kustomization.yaml)

[JustinKuli]

LGTM! Thanks for the diligent work here!

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jan-law, JustinKuli

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,jan-law]

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