Skip to content

Commit c68bee4

Browse files
jan-lawjan-law
committed
Add property when compliant dry run overrides policy mismatch
ref: https://issues.redhat.com/browse/ACM-14577 Adds DryRunNoOpOverride boolean in the relatedObject properties. Indicates when configuration policies detect desired vs actual object mismatches, but dry run updates produce no changes due to webhooks or mutating admission controllers modifying apiserver requests. Signed-off-by: Janelle Law <[email protected]>
1 parent 9eedfbc commit c68bee4

11 files changed

+273
-25
lines changed

‎api/v1/configurationpolicy_types.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -417,6 +417,11 @@ type ObjectProperties struct {
417417
// Diff stores the difference between the `objectDefinition` in the policy and the object on the
418418
// cluster.
419419
Diff string `json:"diff,omitempty"`
420+
421+
// DryRunNoOpOverride indicates that a difference was detected between the policy and the
422+
// object, but a dry run update reported no changes. The dry run result overrides the policy
423+
// assessment, meaning the object is considered compliant despite the initial mismatch.
424+
DryRunNoOpOverride *bool `json:"dryRunNoOpOverride,omitempty"`
420425
}
421426

422427
// RelatedObject contains the details of an object matched by the policy.

‎api/v1/zz_generated.deepcopy.go‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎controllers/configurationpolicy_controller.go‎

Lines changed: 35 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -2393,8 +2393,9 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
23932393

23942394
created := false
23952395
uid := string(obj.existingObj.GetUID())
2396+
dryRunNoOpOverride := false
23962397

2397-
if evaluated, compliant, cachedMsg := r.alreadyEvaluated(obj.policy, obj.existingObj); evaluated {
2398+
if evaluated, compliant, cachedMsg, dryRunNoOp := r.alreadyEvaluated(obj.policy, obj.existingObj); evaluated {
23982399
log.V(1).Info("Skipping object comparison since the resourceVersion hasn't changed")
23992400

24002401
for _, relatedObj := range obj.policy.Status.RelatedObjects {
@@ -2408,6 +2409,7 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
24082409

24092410
throwSpecViolation = !compliant
24102411
msg = cachedMsg
2412+
dryRunNoOpOverride = dryRunNoOp
24112413
} else {
24122414
throwSpecViolation, msg, diff, triedUpdate, updatedObj = r.checkAndUpdateResource(
24132415
obj, objectT, remediation,
@@ -2435,9 +2437,10 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
24352437
}
24362438

24372439
objectProperties = &policyv1.ObjectProperties{
2438-
CreatedByPolicy: &created,
2439-
UID: uid,
2440-
Diff: diff,
2440+
CreatedByPolicy: &created,
2441+
UID: uid,
2442+
Diff: diff,
2443+
DryRunNoOpOverrodePolicy: &dryRunNoOpOverrodePolicy,
24412444
}
24422445

24432446
result.events = append(result.events, objectTmplEvalEvent{false, resultReason, resultMsg})
@@ -2451,9 +2454,10 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
24512454
}
24522455

24532456
objectProperties = &policyv1.ObjectProperties{
2454-
CreatedByPolicy: &created,
2455-
UID: uid,
2456-
Diff: diff,
2457+
CreatedByPolicy: &created,
2458+
UID: uid,
2459+
Diff: diff,
2460+
DryRunNoOpOverrodePolicy: &dryRunNoOpOverrodePolicy,
24572461
}
24582462
} else {
24592463
result.events = append(result.events, objectTmplEvalEvent{true, reasonWantFoundExists, ""})
@@ -3149,9 +3153,10 @@ func handleSingleKey(
31493153
}
31503154

31513155
type cachedEvaluationResult struct {
3152-
resourceVersion string
3153-
compliant bool
3154-
msg string
3156+
resourceVersion string
3157+
compliant bool
3158+
msg string
3159+
dryRunNoOpOverride bool
31553160
}
31563161

31573162
// checkAndUpdateResource checks each individual key of a resource and passes it to handleKeys to see if it
@@ -3227,7 +3232,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
32273232
diff = handleDiff(log, recordDiff, existingObjectCopy, mergedObjCopy, r.FullDiffs)
32283233
}
32293234

3230-
r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation, "")
3235+
r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation, "", false)
32313236

32323237
return throwSpecViolation, "", diff, updateNeeded, updatedObj
32333238
}
@@ -3270,7 +3275,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
32703275

32713276
// If the user specifies an unknown or invalid field, it comes back as a bad request.
32723277
if k8serrors.IsBadRequest(err) {
3273-
r.setEvaluatedObject(obj.policy, obj.existingObj, false, message)
3278+
r.setEvaluatedObject(obj.policy, obj.existingObj, false, message, false)
32743279
}
32753280

32763281
return true, message, "", updateNeeded, nil
@@ -3297,7 +3302,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
32973302
`you may set spec["object-templates"][].recreateOption to recreate the object`
32983303
}
32993304

3300-
r.setEvaluatedObject(obj.policy, obj.existingObj, false, message)
3305+
r.setEvaluatedObject(obj.policy, obj.existingObj, false, message, false)
33013306

33023307
return true, message, diff, false, nil
33033308
}
@@ -3322,7 +3327,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
33223327
diff = handleDiff(log, recordDiff, existingObjectCopy, mergedObjCopy, r.FullDiffs)
33233328
}
33243329

3325-
r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation, "")
3330+
r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation, "", true)
33263331

33273332
return throwSpecViolation, "", diff, updateNeeded, updatedObj
33283333
}
@@ -3332,7 +3337,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
33323337

33333338
// The object would have been updated, so if it's inform, return as noncompliant.
33343339
if isInform {
3335-
r.setEvaluatedObject(obj.policy, obj.existingObj, false, "")
3340+
r.setEvaluatedObject(obj.policy, obj.existingObj, false, "", false)
33363341

33373342
return true, "", diff, false, nil
33383343
}
@@ -3412,7 +3417,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
34123417
}
34133418

34143419
if !statusMismatch {
3415-
r.setEvaluatedObject(obj.policy, updatedObj, true, message)
3420+
r.setEvaluatedObject(obj.policy, updatedObj, true, message, false)
34163421
}
34173422

34183423
return throwSpecViolation, "", diff, updateNeeded, updatedObj
@@ -3575,7 +3580,11 @@ func removeFieldsForComparison(obj *unstructured.Unstructured) {
35753580
// setEvaluatedObject updates the cache to indicate that the ConfigurationPolicy has evaluated this
35763581
// object at its current resourceVersion.
35773582
func (r *ConfigurationPolicyReconciler) setEvaluatedObject(
3578-
policy *policyv1.ConfigurationPolicy, currentObject *unstructured.Unstructured, compliant bool, msg string,
3583+
policy *policyv1.ConfigurationPolicy,
3584+
currentObject *unstructured.Unstructured,
3585+
compliant bool,
3586+
msg string,
3587+
dryRunOverride bool,
35793588
) {
35803589
policyMap := &sync.Map{}
35813590

@@ -3587,9 +3596,10 @@ func (r *ConfigurationPolicyReconciler) setEvaluatedObject(
35873596
policyMap.Store(
35883597
currentObject.GetUID(),
35893598
cachedEvaluationResult{
3590-
resourceVersion: currentObject.GetResourceVersion(),
3591-
compliant: compliant,
3592-
msg: msg,
3599+
resourceVersion: currentObject.GetResourceVersion(),
3600+
compliant: compliant,
3601+
msg: msg,
3602+
dryRunNoOpOverride: dryRunOverride,
35933603
},
35943604
)
35953605
}
@@ -3598,29 +3608,29 @@ func (r *ConfigurationPolicyReconciler) setEvaluatedObject(
35983608
// resourceVersion.
35993609
func (r *ConfigurationPolicyReconciler) alreadyEvaluated(
36003610
policy *policyv1.ConfigurationPolicy, currentObject *unstructured.Unstructured,
3601-
) (evaluated bool, compliant bool, msg string) {
3611+
) (evaluated bool, compliant bool, msg string, dryRunNoOpOverrode bool) {
36023612
if policy == nil || currentObject == nil {
3603-
return false, false, ""
3613+
return false, false, "", false
36043614
}
36053615

36063616
loadedPolicyMap, loaded := r.processedPolicyCache.Load(policy.GetUID())
36073617
if !loaded {
3608-
return false, false, ""
3618+
return false, false, "", false
36093619
}
36103620

36113621
policyMap := loadedPolicyMap.(*sync.Map)
36123622

36133623
result, loaded := policyMap.Load(currentObject.GetUID())
36143624
if !loaded {
3615-
return false, false, ""
3625+
return false, false, "", false
36163626
}
36173627

36183628
resultTyped := result.(cachedEvaluationResult)
36193629

36203630
alreadyEvaluated := resultTyped.resourceVersion != "" &&
36213631
resultTyped.resourceVersion == currentObject.GetResourceVersion()
36223632

3623-
return alreadyEvaluated, resultTyped.compliant, resultTyped.msg
3633+
return alreadyEvaluated, resultTyped.compliant, resultTyped.msg, resultTyped.dryRunNoOpOverride
36243634
}
36253635

36263636
func getUpdateErrorMsg(err error, kind string, name string) string {

‎controllers/configurationpolicy_controller_test.go‎

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1566,3 +1566,162 @@ func TestShouldHandleSingleKeyFalse(t *testing.T) {
15661566
assert.False(t, skip)
15671567
}
15681568
}
1569+
1570+
// TestAlreadyEvaluated tests the core caching functionality for evaluated objects
1571+
func TestAlreadyEvaluated(t *testing.T) {
1572+
// Create a test ConfigurationPolicy
1573+
policy := &policyv1.ConfigurationPolicy{
1574+
ObjectMeta: metav1.ObjectMeta{
1575+
Name: "test-policy",
1576+
Namespace: "default",
1577+
UID: types.UID("test-policy-uid"),
1578+
},
1579+
}
1580+
1581+
// Create a test unstructured object
1582+
testObj := &unstructured.Unstructured{
1583+
Object: map[string]interface{}{
1584+
"apiVersion": "v1",
1585+
"kind": "ConfigMap",
1586+
"metadata": map[string]interface{}{
1587+
"name": "test-configmap",
1588+
"namespace": "default",
1589+
"uid": "test-object-uid",
1590+
"resourceVersion": "123",
1591+
},
1592+
},
1593+
}
1594+
1595+
// Create a reconciler with an empty cache
1596+
r := &ConfigurationPolicyReconciler{
1597+
processedPolicyCache: sync.Map{},
1598+
}
1599+
1600+
t.Run("Test caching dryRunNoOpOverride=true", func(t *testing.T) {
1601+
// Call setEvaluatedObject with dryRunNoOpOverride=true
1602+
r.setEvaluatedObject(policy, testObj, true, "test message", true)
1603+
1604+
// Verify the object was cached correctly
1605+
evaluated, compliant, msg, dryRunNoOpOverride := r.alreadyEvaluated(policy, testObj)
1606+
1607+
assert.True(t, evaluated, "Object should be marked as evaluated")
1608+
assert.True(t, compliant, "Object should be marked as compliant")
1609+
assert.Equal(t, "test message", msg, "Message should match")
1610+
assert.True(t, dryRunNoOpOverride, "dryRunNoOpOverride should be true")
1611+
})
1612+
1613+
t.Run("Test caching dryRunNoOpOverride=false", func(t *testing.T) {
1614+
// Create a new policy and object to avoid cache interference
1615+
policy2 := &policyv1.ConfigurationPolicy{
1616+
ObjectMeta: metav1.ObjectMeta{
1617+
Name: "test-policy-2",
1618+
Namespace: "default",
1619+
UID: types.UID("test-policy-uid-2"),
1620+
},
1621+
}
1622+
1623+
testObj2 := &unstructured.Unstructured{
1624+
Object: map[string]interface{}{
1625+
"apiVersion": "v1",
1626+
"kind": "ConfigMap",
1627+
"metadata": map[string]interface{}{
1628+
"name": "test-configmap-2",
1629+
"namespace": "default",
1630+
"uid": "test-object-uid-2",
1631+
"resourceVersion": "124",
1632+
},
1633+
},
1634+
}
1635+
1636+
// Call setEvaluatedObject with dryRunNoOpOverride=false
1637+
r.setEvaluatedObject(policy2, testObj2, false, "error message", false)
1638+
1639+
// Verify the object was cached correctly
1640+
evaluated, compliant, msg, dryRunNoOpOverride := r.alreadyEvaluated(policy2, testObj2)
1641+
1642+
assert.True(t, evaluated, "Object should be marked as evaluated")
1643+
assert.False(t, compliant, "Object should be marked as non-compliant")
1644+
assert.Equal(t, "error message", msg, "Message should match")
1645+
assert.False(t, dryRunNoOpOverride, "dryRunNoOpOverride should be false")
1646+
})
1647+
1648+
t.Run("Test not cached object", func(t *testing.T) {
1649+
// Create a completely new policy and object
1650+
policy3 := &policyv1.ConfigurationPolicy{
1651+
ObjectMeta: metav1.ObjectMeta{
1652+
Name: "test-policy-3",
1653+
Namespace: "default",
1654+
UID: types.UID("test-policy-uid-3"),
1655+
},
1656+
}
1657+
1658+
testObj3 := &unstructured.Unstructured{
1659+
Object: map[string]interface{}{
1660+
"apiVersion": "v1",
1661+
"kind": "ConfigMap",
1662+
"metadata": map[string]interface{}{
1663+
"name": "test-configmap-3",
1664+
"namespace": "default",
1665+
"uid": "test-object-uid-3",
1666+
"resourceVersion": "125",
1667+
},
1668+
},
1669+
}
1670+
1671+
// Call alreadyEvaluated without caching first
1672+
evaluated, compliant, msg, dryRunNoOpOverride := r.alreadyEvaluated(policy3, testObj3)
1673+
1674+
assert.False(t, evaluated, "Object should not be marked as evaluated")
1675+
assert.False(t, compliant, "Object should be marked as non-compliant")
1676+
assert.Empty(t, msg, "Message should be empty")
1677+
assert.False(t, dryRunNoOpOverride, "dryRunNoOpOverride should be false")
1678+
})
1679+
1680+
t.Run("Test resource version change invalidates cache", func(t *testing.T) {
1681+
// Use the same policy but change the resource version
1682+
testObjNewVersion := &unstructured.Unstructured{
1683+
Object: map[string]interface{}{
1684+
"apiVersion": "v1",
1685+
"kind": "ConfigMap",
1686+
"metadata": map[string]interface{}{
1687+
"name": "test-configmap",
1688+
"namespace": "default",
1689+
"uid": "test-object-uid",
1690+
"resourceVersion": "456", // Different resource version
1691+
},
1692+
},
1693+
}
1694+
1695+
// Call alreadyEvaluated - should return false due to different resource version
1696+
// but the cached values from the previous evaluation should still be returned
1697+
evaluated, compliant, msg, dryRunNoOpOverride := r.alreadyEvaluated(policy, testObjNewVersion)
1698+
1699+
assert.False(t, evaluated, "Object should not be marked as evaluated due to different resource version")
1700+
assert.True(t, compliant, "Should return the cached compliance state from previous evaluation")
1701+
assert.Equal(t, "test message", msg, "Should return the cached message from previous evaluation")
1702+
assert.True(t, dryRunNoOpOverride, "Should return the cached dryRunNoOpOverride from previous evaluation")
1703+
})
1704+
1705+
t.Run("Test nil inputs", func(t *testing.T) {
1706+
// Test with nil policy
1707+
evaluated, compliant, msg, dryRunNoOpOverride := r.alreadyEvaluated(nil, testObj)
1708+
assert.False(t, evaluated)
1709+
assert.False(t, compliant)
1710+
assert.Empty(t, msg)
1711+
assert.False(t, dryRunNoOpOverride)
1712+
1713+
// Test with nil object
1714+
evaluated, compliant, msg, dryRunNoOpOverride = r.alreadyEvaluated(policy, nil)
1715+
assert.False(t, evaluated)
1716+
assert.False(t, compliant)
1717+
assert.Empty(t, msg)
1718+
assert.False(t, dryRunNoOpOverride)
1719+
1720+
// Test with both nil
1721+
evaluated, compliant, msg, dryRunNoOpOverride = r.alreadyEvaluated(nil, nil)
1722+
assert.False(t, evaluated)
1723+
assert.False(t, compliant)
1724+
assert.Empty(t, msg)
1725+
assert.False(t, dryRunNoOpOverride)
1726+
})
1727+
}

‎deploy/crds/kustomize_configurationpolicy/policy.open-cluster-management.io_configurationpolicies.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -462,6 +462,12 @@ spec:
462462
Diff stores the difference between the `objectDefinition` in the policy and the object on the
463463
cluster.
464464
type: string
465+
dryRunNoOpOverride:
466+
description: |-
467+
DryRunNoOpOverride indicates that a difference was detected between the policy and the
468+
object, but a dry run update reported no changes. The dry run result overrides the policy
469+
assessment, meaning the object is considered compliant despite the initial mismatch.
470+
type: boolean
465471
uid:
466472
description: |-
467473
UID stores the object UID to help track object ownership for deletion when pruning is

‎deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -369,6 +369,12 @@ spec:
369369
Diff stores the difference between the `objectDefinition` in the policy and the object on the
370370
cluster.
371371
type: string
372+
dryRunNoOpOverride:
373+
description: |-
374+
DryRunNoOpOverride indicates that a difference was detected between the policy and the
375+
object, but a dry run update reported no changes. The dry run result overrides the policy
376+
assessment, meaning the object is considered compliant despite the initial mismatch.
377+
type: boolean
372378
uid:
373379
description: |-
374380
UID stores the object UID to help track object ownership for deletion when pruning is

0 commit comments

Comments
 (0)