From db17f5cccb7673c4ebc3ef03af2e8c783882d58f Mon Sep 17 00:00:00 2001 From: Janelle Law Date: Tue, 12 Aug 2025 12:45:51 -0700 Subject: [PATCH 1/4] Store dry run validation result in Status ref: https://issues.redhat.com/browse/ACM-14577 Adds boolean in the relatedObject properties, indicating when configuration policies detect desired vs actual object mismatches, but dry run updates produce no changes due to webhooks or mutating admission controllers modifying Update requests. Related object properties were previously only set for enforced policies and non-compliant objects, causing compliant objects in Inform policies to have nil properties and inconsistent status reporting. This commit displays related object properties on compliant objects regardless of remediation action. Signed-off-by: Janelle Law --- api/v1/configurationpolicy_types.go | 5 ++ controllers/configurationpolicy_controller.go | 58 ++++++++++--------- ...r-management.io_configurationpolicies.yaml | 6 ++ ...luster-management.io_operatorpolicies.yaml | 6 ++ ...r-management.io_configurationpolicies.yaml | 6 ++ ...luster-management.io_operatorpolicies.yaml | 6 ++ ...r-management.io_configurationpolicies.yaml | 6 ++ 7 files changed, 66 insertions(+), 27 deletions(-) diff --git a/api/v1/configurationpolicy_types.go b/api/v1/configurationpolicy_types.go index 43ea0ec4..3307f147 100644 --- a/api/v1/configurationpolicy_types.go +++ b/api/v1/configurationpolicy_types.go @@ -417,6 +417,11 @@ type ObjectProperties struct { // Diff stores the difference between the `objectDefinition` in the policy and the object on the // cluster. Diff string `json:"diff,omitempty"` + + // MatchesAfterDryRun indicates whether the dry run update matches the policy assessment. If false, + // there was an initial mismatch between the policy and object, but the dry run update produced + // a compliant result. + MatchesAfterDryRun bool `json:"matchesAfterDryRun,omitempty"` } // RelatedObject contains the details of an object matched by the policy. diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index 8370ab03..d683be9b 100644 --- a/controllers/configurationpolicy_controller.go +++ b/controllers/configurationpolicy_controller.go @@ -2388,7 +2388,7 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj( if exists && obj.shouldExist { log.V(2).Info("The object already exists. Verifying the object fields match what is desired.") - var throwSpecViolation, triedUpdate bool + var throwSpecViolation, triedUpdate, matchesAfterDryRun bool var msg, diff string var updatedObj *unstructured.Unstructured @@ -2400,8 +2400,9 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj( for _, relatedObj := range obj.policy.Status.RelatedObjects { if relatedObj.Properties != nil && relatedObj.Properties.UID == uid { - // Retain the diff from the previous evaluation + // Retain the properties from the previous evaluation diff = relatedObj.Properties.Diff + matchesAfterDryRun = relatedObj.Properties.MatchesAfterDryRun break } @@ -2410,7 +2411,7 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj( throwSpecViolation = !compliant msg = cachedMsg } else { - throwSpecViolation, msg, diff, triedUpdate, updatedObj = r.checkAndUpdateResource( + throwSpecViolation, msg, diff, triedUpdate, updatedObj, matchesAfterDryRun = r.checkAndUpdateResource( obj, objectT, remediation, ) @@ -2435,12 +2436,6 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj( resultReason = reasonWantFoundNoMatch } - objectProperties = &policyv1.ObjectProperties{ - CreatedByPolicy: &created, - UID: uid, - Diff: diff, - } - result.events = append(result.events, objectTmplEvalEvent{false, resultReason, resultMsg}) } else { // it is a must have and it does exist, so it is compliant @@ -2450,16 +2445,17 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj( } else { result.events = append(result.events, objectTmplEvalEvent{true, reasonWantFoundExists, ""}) } - - objectProperties = &policyv1.ObjectProperties{ - CreatedByPolicy: &created, - UID: uid, - Diff: diff, - } } else { result.events = append(result.events, objectTmplEvalEvent{true, reasonWantFoundExists, ""}) } } + + objectProperties = &policyv1.ObjectProperties{ + CreatedByPolicy: &created, + UID: uid, + Diff: diff, + MatchesAfterDryRun: matchesAfterDryRun, + } } return @@ -3162,7 +3158,12 @@ type cachedEvaluationResult struct { func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( obj singleObject, objectT *policyv1.ObjectTemplate, remediation policyv1.RemediationAction, ) ( - throwSpecViolation bool, message string, diff string, updateNeeded bool, updatedObj *unstructured.Unstructured, + throwSpecViolation bool, + message string, + diff string, + updateNeeded bool, + updatedObj *unstructured.Unstructured, + matchesAfterDryRun bool, ) { log := log.WithValues( "policy", obj.policy.Name, "name", obj.name, "namespace", obj.namespace, "resource", obj.scopedGVR.Resource, @@ -3188,7 +3189,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( if obj.existingObj == nil { log.Info("Skipping update: Previous object retrieval from the API server failed") - return false, "", "", false, nil + return false, "", "", false, nil, true } var res dynamic.ResourceInterface @@ -3210,7 +3211,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( objectT.MetadataComplianceType, ) if errMsg != "" { - return true, errMsg, "", true, nil + return true, errMsg, "", true, nil, true } recordDiff := objectT.RecordDiffWithDefault() @@ -3230,7 +3231,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation, "") - return throwSpecViolation, "", diff, updateNeeded, updatedObj + return throwSpecViolation, "", diff, updateNeeded, updatedObj, true } if updateNeeded { @@ -3274,7 +3275,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( r.setEvaluatedObject(obj.policy, obj.existingObj, false, message) } - return true, message, "", updateNeeded, nil + return true, message, "", updateNeeded, nil, true } // If an update is invalid (i.e. modifying Pod spec fields), then return noncompliant since that @@ -3300,7 +3301,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( r.setEvaluatedObject(obj.policy, obj.existingObj, false, message) - return true, message, diff, false, nil + return true, message, diff, false, nil, true } mergedObjCopy := obj.existingObj.DeepCopy() @@ -3323,9 +3324,12 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( diff = handleDiff(log, recordDiff, existingObjectCopy, mergedObjCopy, r.FullDiffs) } + // Assume object is compliant by inverting the value of throwSpecViolation r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation, "") - return throwSpecViolation, "", diff, updateNeeded, updatedObj + matchesAfterDryRun = false + + return throwSpecViolation, "", diff, updateNeeded, updatedObj, matchesAfterDryRun } diff = handleDiff(log, recordDiff, existingObjectCopy, dryRunUpdatedObj, r.FullDiffs) @@ -3335,7 +3339,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( if isInform { r.setEvaluatedObject(obj.policy, obj.existingObj, false, "") - return true, "", diff, false, nil + return true, "", diff, false, nil, true } // If it's not inform (i.e. enforce), update the object @@ -3355,7 +3359,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( if err != nil && !k8serrors.IsNotFound(err) { message = fmt.Sprintf(`%s failed to delete when recreating with the error %v`, getMsgPrefix(&obj), err) - return true, message, "", updateNeeded, nil + return true, message, "", updateNeeded, nil, true } attempts := 0 @@ -3373,7 +3377,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( message = getMsgPrefix(&obj) + " timed out waiting for the object to delete during recreate, " + "will retry on the next policy evaluation" - return true, message, "", updateNeeded, nil + return true, message, "", updateNeeded, nil, true } time.Sleep(time.Second) @@ -3409,14 +3413,14 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( message = fmt.Sprintf("%s failed to %s with the error `%v`", getMsgPrefix(&obj), action, err) } - return true, message, diff, updateNeeded, nil + return true, message, diff, updateNeeded, nil, true } if !statusMismatch { r.setEvaluatedObject(obj.policy, updatedObj, true, message) } - return throwSpecViolation, "", diff, updateNeeded, updatedObj + return throwSpecViolation, "", diff, updateNeeded, updatedObj, true } func getMsgPrefix(obj *singleObject) string { diff --git a/deploy/crds/kustomize_configurationpolicy/policy.open-cluster-management.io_configurationpolicies.yaml b/deploy/crds/kustomize_configurationpolicy/policy.open-cluster-management.io_configurationpolicies.yaml index 9e808332..f1e6a4a9 100644 --- a/deploy/crds/kustomize_configurationpolicy/policy.open-cluster-management.io_configurationpolicies.yaml +++ b/deploy/crds/kustomize_configurationpolicy/policy.open-cluster-management.io_configurationpolicies.yaml @@ -477,6 +477,12 @@ spec: Diff stores the difference between the `objectDefinition` in the policy and the object on the cluster. type: string + matchesAfterDryRun: + description: |- + MatchesAfterDryRun indicates whether the dry run update matches the policy assessment. If false, + there was an initial mismatch between the policy and object, but the dry run update produced + a compliant result. + type: boolean uid: description: |- UID stores the object UID to help track object ownership for deletion when pruning is diff --git a/deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml b/deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml index c5b9eae3..e71929d0 100644 --- a/deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml +++ b/deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml @@ -384,6 +384,12 @@ spec: Diff stores the difference between the `objectDefinition` in the policy and the object on the cluster. type: string + matchesAfterDryRun: + description: |- + MatchesAfterDryRun indicates whether the dry run update matches the policy assessment. If false, + there was an initial mismatch between the policy and object, but the dry run update produced + a compliant result. + type: boolean uid: description: |- UID stores the object UID to help track object ownership for deletion when pruning is diff --git a/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml b/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml index a23b98d8..5ba2ec69 100644 --- a/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml +++ b/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml @@ -484,6 +484,12 @@ spec: Diff stores the difference between the `objectDefinition` in the policy and the object on the cluster. type: string + matchesAfterDryRun: + description: |- + MatchesAfterDryRun indicates whether the dry run update matches the policy assessment. If false, + there was an initial mismatch between the policy and object, but the dry run update produced + a compliant result. + type: boolean uid: description: |- UID stores the object UID to help track object ownership for deletion when pruning is diff --git a/deploy/crds/policy.open-cluster-management.io_operatorpolicies.yaml b/deploy/crds/policy.open-cluster-management.io_operatorpolicies.yaml index 7026a80f..3f676522 100644 --- a/deploy/crds/policy.open-cluster-management.io_operatorpolicies.yaml +++ b/deploy/crds/policy.open-cluster-management.io_operatorpolicies.yaml @@ -381,6 +381,12 @@ spec: CreatedByPolicy reports whether the object was created by the configuration policy, which is important when pruning is configured. type: boolean + matchesAfterDryRun: + description: |- + MatchesAfterDryRun indicates whether the dry run update matches the policy assessment. If false, + there was an initial mismatch between the policy and object, but the dry run update produced + a compliant result. + type: boolean uid: description: |- UID stores the object UID to help track object ownership for deletion when pruning is diff --git a/pkg/dryrun/policy.open-cluster-management.io_configurationpolicies.yaml b/pkg/dryrun/policy.open-cluster-management.io_configurationpolicies.yaml index a23b98d8..5ba2ec69 100644 --- a/pkg/dryrun/policy.open-cluster-management.io_configurationpolicies.yaml +++ b/pkg/dryrun/policy.open-cluster-management.io_configurationpolicies.yaml @@ -484,6 +484,12 @@ spec: Diff stores the difference between the `objectDefinition` in the policy and the object on the cluster. type: string + matchesAfterDryRun: + description: |- + MatchesAfterDryRun indicates whether the dry run update matches the policy assessment. If false, + there was an initial mismatch between the policy and object, but the dry run update produced + a compliant result. + type: boolean uid: description: |- UID stores the object UID to help track object ownership for deletion when pruning is From f353b3a454f3a4fe6681901e4ca9133f65950f49 Mon Sep 17 00:00:00 2001 From: Janelle Law Date: Tue, 12 Aug 2025 12:48:05 -0700 Subject: [PATCH 2/4] Test related object properties present for any remediation action ref: https://issues.redhat.com/browse/ACM-14577 Tests scenarios where the policy reports a mismatched object, but the dry run validation reports a compliant object due to the apiserver adding or modifying fields in the request body. This commit also prevents timeouts in case40 by wrapping assertions with Eventually. Signed-off-by: Janelle Law --- .../configurationpolicy_controller_test.go | 69 +++++++++++ test/e2e/case20_delete_objects_test.go | 32 +++-- test/e2e/case40_recreate_option_test.go | 48 ++++---- test/e2e/case8_status_check_test.go | 114 ++++++++++++++++++ .../case8_status_check/case8_service.yaml | 23 ++++ .../case8_service_inform.yaml | 36 ++++++ 6 files changed, 290 insertions(+), 32 deletions(-) create mode 100644 test/resources/case8_status_check/case8_service.yaml create mode 100644 test/resources/case8_status_check/case8_service_inform.yaml diff --git a/controllers/configurationpolicy_controller_test.go b/controllers/configurationpolicy_controller_test.go index d9694717..777576db 100644 --- a/controllers/configurationpolicy_controller_test.go +++ b/controllers/configurationpolicy_controller_test.go @@ -705,6 +705,75 @@ func TestUpdatedRelatedObjects(t *testing.T) { assert.Equal(t, "bar", relatedList[0].Object.Metadata.Name) } +func TestAddRelatedObjectProperties(t *testing.T) { + compliant := true + scopedGVR := depclient.ScopedGVR{ + GroupVersionResource: policyv1.SchemeBuilder.GroupVersion.WithResource("ConfigurationPolicy"), + Namespaced: true, + } + namespace := "default" + name := "foo" + reason := "reason" + + // Create test data for ObjectProperties + createdByPolicy := true + testUID := "test-uid-12345" + testDiff := "test diff content" + dryRunMatches := true + + creationInfo := &policyv1.ObjectProperties{ + CreatedByPolicy: &createdByPolicy, + UID: testUID, + Diff: testDiff, + MatchesAfterDryRun: dryRunMatches, + } + + relatedList := addRelatedObjects( + compliant, scopedGVR, "ConfigurationPolicy", namespace, []string{name}, reason, creationInfo, + ) + related := relatedList[0] + + // Validate that the properties are correctly set + assert.NotNil(t, related.Properties, "Properties should not be nil when creationInfo is provided") + assert.NotNil(t, related.Properties.CreatedByPolicy, "CreatedByPolicy should not be nil") + assert.Equal(t, createdByPolicy, *related.Properties.CreatedByPolicy) + assert.Equal(t, testUID, related.Properties.UID) + assert.Equal(t, testDiff, related.Properties.Diff) + assert.Equal(t, dryRunMatches, related.Properties.MatchesAfterDryRun) + + // add the same object and make sure the existing one is overwritten + reason = "new" + compliant = false + + // Create new test data for ObjectProperties with different values + newCreatedByPolicy := false + newTestDiff := "updated diff content" + newDryRunMatches := false + + newCreationInfo := &policyv1.ObjectProperties{ + CreatedByPolicy: &newCreatedByPolicy, + UID: testUID, + Diff: newTestDiff, + MatchesAfterDryRun: newDryRunMatches, + } + + relatedList = addRelatedObjects( + compliant, scopedGVR, "ConfigurationPolicy", namespace, []string{name}, reason, newCreationInfo) + related = relatedList[0] + + assert.Len(t, relatedList, 1) + assert.Equal(t, string(policyv1.NonCompliant), related.Compliant) + assert.Equal(t, "new", related.Reason) + + // Validate that the properties were updated with new values + assert.NotNil(t, related.Properties, "Properties should not be nil when newCreationInfo is provided") + assert.NotNil(t, related.Properties.CreatedByPolicy, "CreatedByPolicy should not be nil") + assert.Equal(t, newCreatedByPolicy, *related.Properties.CreatedByPolicy) + assert.Equal(t, testUID, related.Properties.UID) + assert.Equal(t, newTestDiff, related.Properties.Diff) + assert.Equal(t, newDryRunMatches, related.Properties.MatchesAfterDryRun) +} + func TestCreateStatus(t *testing.T) { testcases := []struct { testName string diff --git a/test/e2e/case20_delete_objects_test.go b/test/e2e/case20_delete_objects_test.go index 60b567f1..c41e8858 100644 --- a/test/e2e/case20_delete_objects_test.go +++ b/test/e2e/case20_delete_objects_test.go @@ -130,26 +130,40 @@ var _ = Describe("Test Object deletion", Ordered, func() { return properties["uid"] }, defaultTimeoutSeconds, 1).ShouldNot(BeEmpty()) }) - It("should not update status field for inform policies", func() { + It("should update status field for inform policies", func() { By("Creating " + case20ConfigPolicyNameInform + " on managed") utils.Kubectl("apply", "-f", case20PolicyYamlInform, "-n", testNamespace) plc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, case20ConfigPolicyNameInform, testNamespace, true, defaultTimeoutSeconds) Expect(plc).NotTo(BeNil()) + + var managedPlc *unstructured.Unstructured + Eventually(func(g Gomega) { - managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, + managedPlc = utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, case20ConfigPolicyNameInform, testNamespace, true, defaultTimeoutSeconds) utils.CheckComplianceStatus(g, managedPlc, "Compliant") }, defaultTimeoutSeconds, 1).Should(Succeed()) - Eventually(func() interface{} { - managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, - case20ConfigPolicyNameInform, testNamespace, true, defaultTimeoutSeconds) - relatedObj := managedPlc.Object["status"].(map[string]interface{})["relatedObjects"].([]interface{})[0] - properties := relatedObj.(map[string]interface{})["properties"] - return properties - }, defaultTimeoutSeconds, 1).Should(BeNil()) + By("Verifying the related object properties") + relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects") + Expect(err).ToNot(HaveOccurred()) + Expect(relatedObjects).To(HaveLen(1)) + + relatedObj := relatedObjects[0].(map[string]interface{}) + + createdByPolicy, found, _ := unstructured.NestedBool(relatedObj, "properties", "createdByPolicy") + Expect(found).To(BeTrue()) + Expect(createdByPolicy).To(BeFalse()) + + uid, found, _ := unstructured.NestedString(relatedObj, "properties", "uid") + Expect(found).To(BeTrue()) + Expect(uid).ToNot(BeEmpty()) + + matchesAfterDryRun, found, _ := unstructured.NestedBool(relatedObj, "properties", "matchesAfterDryRun") + Expect(found).To(BeTrue()) + Expect(matchesAfterDryRun).To(BeTrue()) }) AfterAll(func() { policies := []string{ diff --git a/test/e2e/case40_recreate_option_test.go b/test/e2e/case40_recreate_option_test.go index c86b50f2..2913a848 100644 --- a/test/e2e/case40_recreate_option_test.go +++ b/test/e2e/case40_recreate_option_test.go @@ -117,7 +117,7 @@ var _ = Describe("Recreate options", Ordered, func() { By("Verifying the ConfigurationPolicy is Compliant") var managedPlc *unstructured.Unstructured - + var propsUID string Eventually(func(g Gomega) { managedPlc = utils.GetWithTimeout( clientManagedDynamic, @@ -129,37 +129,39 @@ var _ = Describe("Recreate options", Ordered, func() { ) utils.CheckComplianceStatus(g, managedPlc, "Compliant") - }, defaultTimeoutSeconds, 1).Should(Succeed()) - By("Verifying the diff is not set") - relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects") - Expect(err).ToNot(HaveOccurred()) - Expect(relatedObjects).To(HaveLen(1)) + relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(relatedObjects).To(HaveLen(1)) - relatedObject := relatedObjects[0].(map[string]interface{}) + relatedObject := relatedObjects[0].(map[string]interface{}) - diff, _, _ := unstructured.NestedString(relatedObject, "properties", "diff") - Expect(diff).To(BeEmpty()) + diff, _, _ := unstructured.NestedString(relatedObject, "properties", "diff") + g.Expect(diff).To(BeEmpty()) - propsUID, _, _ := unstructured.NestedString(relatedObject, "properties", "uid") - Expect(propsUID).ToNot(BeEmpty()) + propsUID, _, _ = unstructured.NestedString(relatedObject, "properties", "uid") + g.Expect(propsUID).ToNot(BeEmpty()) - createdByPolicy, _, _ := unstructured.NestedBool(relatedObject, "properties", "createdByPolicy") - Expect(createdByPolicy).To(BeTrue()) + createdByPolicy, _, _ := unstructured.NestedBool(relatedObject, "properties", "createdByPolicy") + g.Expect(createdByPolicy).To(BeTrue()) - deployment, err = clientManagedDynamic.Resource(gvrDeployment).Namespace("default").Get( - ctx, "case40", metav1.GetOptions{}, - ) - Expect(err).ToNot(HaveOccurred()) + deployment, err = clientManagedDynamic.Resource(gvrDeployment).Namespace("default").Get( + ctx, "case40", metav1.GetOptions{}, + ) + g.Expect(err).ToNot(HaveOccurred()) + }, defaultTimeoutSeconds, 1).Should(Succeed()) By("Verifying the Deployment was recreated") - Expect(deployment.GetUID()).ToNot(Equal(uid), "Expected a new UID on the Deployment after it got recreated") - Expect(propsUID).To( - BeEquivalentTo(deployment.GetUID()), "Expect the object properties UID to match the new Deployment", - ) + Eventually(func(g Gomega) { + g.Expect(deployment.GetUID()).ToNot( + Equal(uid), "Expected a new UID on the Deployment after it got recreated") + g.Expect(propsUID).To( + BeEquivalentTo(deployment.GetUID()), "Expect the object properties UID to match the new Deployment", + ) - selector, _, _ := unstructured.NestedString(deployment.Object, "spec", "selector", "matchLabels", "app") - Expect(selector).To(Equal("case40-2")) + selector, _, _ := unstructured.NestedString(deployment.Object, "spec", "selector", "matchLabels", "app") + g.Expect(selector).To(Equal("case40-2")) + }, defaultTimeoutSeconds, 1).Should(Succeed()) deleteConfigPolicies([]string{"case40"}) }) diff --git a/test/e2e/case8_status_check_test.go b/test/e2e/case8_status_check_test.go index c245e230..c14c0cc9 100644 --- a/test/e2e/case8_status_check_test.go +++ b/test/e2e/case8_status_check_test.go @@ -171,3 +171,117 @@ var _ = Describe("Test pod obj template handling", func() { }) }) }) + +var _ = Describe("Test related object property status", Ordered, func() { + Describe("Create a policy missing a field added by kubernetes", Ordered, func() { + const ( + policyName = "policy-service" + serviceName = "grc-policy-propagator-metrics" + policyYAML = "../resources/case8_status_check/case8_service_inform.yaml" + serviceYAML = "../resources/case8_status_check/case8_service.yaml" + ) + + It("Should be compliant when the inform policy omits a field added by the apiserver", func() { + By("Creating a Service with explicit type: ClusterIP") + utils.Kubectl("apply", "-f", serviceYAML) + + By("Creating the " + policyName + " policy that omits the type field)") + utils.Kubectl("apply", "-f", policyYAML, "-n", testNamespace) + + By("Verifying that the " + policyName + " policy is compliant") + Eventually(func(g Gomega) { + managedPlc := utils.GetWithTimeout( + clientManagedDynamic, gvrConfigPolicy, policyName, testNamespace, true, defaultTimeoutSeconds, + ) + + utils.CheckComplianceStatus(g, managedPlc, "Compliant") + + relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(relatedObjects).To(HaveLen(1)) + + relatedObj := relatedObjects[0].(map[string]interface{}) + matchesAfterDryRun, _, _ := unstructured.NestedBool(relatedObj, "properties", "matchesAfterDryRun") + + g.Expect(matchesAfterDryRun).To(BeFalse()) + }, defaultTimeoutSeconds, 1).Should(Succeed()) + }) + + It("Should be compliant when the enforce policy omits a field added by the apiserver", func() { + By("Changing the policy to enforce mode") + utils.EnforceConfigurationPolicy(policyName, testNamespace) + + By("Verifying that the " + policyName + " policy is compliant") + Eventually(func(g Gomega) { + managedPlc := utils.GetWithTimeout( + clientManagedDynamic, gvrConfigPolicy, policyName, testNamespace, true, defaultTimeoutSeconds, + ) + + utils.CheckComplianceStatus(g, managedPlc, "Compliant") + + relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(relatedObjects).To(HaveLen(1)) + + relatedObj := relatedObjects[0].(map[string]interface{}) + matchesAfterDryRun, _, _ := unstructured.NestedBool(relatedObj, "properties", "matchesAfterDryRun") + + g.Expect(matchesAfterDryRun).To(BeFalse()) + }, defaultTimeoutSeconds, 1).Should(Succeed()) + }) + + It("Should be compliant when the enforce policy includes all fields", func() { + By("Patching the " + policyName + " policy with the Service type field)") + utils.Kubectl("patch", "configurationpolicy", policyName, "-n", testNamespace, "--type=json", "-p", + `[{"op": "add", "path": "/spec/object-templates/0/objectDefinition/spec/type", "value": "ClusterIP"}]`) + + By("Verifying that the " + policyName + " policy is compliant") + Eventually(func(g Gomega) { + managedPlc := utils.GetWithTimeout( + clientManagedDynamic, gvrConfigPolicy, policyName, testNamespace, true, defaultTimeoutSeconds, + ) + + utils.CheckComplianceStatus(g, managedPlc, "Compliant") + + relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(relatedObjects).To(HaveLen(1)) + + relatedObj := relatedObjects[0].(map[string]interface{}) + matchesAfterDryRun, _, _ := unstructured.NestedBool(relatedObj, "properties", "matchesAfterDryRun") + + g.Expect(matchesAfterDryRun).To(BeTrue()) + }, defaultTimeoutSeconds, 1).Should(Succeed()) + }) + + It("Should be compliant when the inform policy includes all fields", func() { + By("Changing the policy to inform mode") + utils.Kubectl("patch", "configurationpolicy", policyName, `--type=json`, + `-p=[{"op":"replace","path":"/spec/remediationAction","value":"inform"}]`, "-n", testNamespace) + + By("Verifying that the " + policyName + " policy is compliant") + Eventually(func(g Gomega) { + managedPlc := utils.GetWithTimeout( + clientManagedDynamic, gvrConfigPolicy, policyName, testNamespace, true, defaultTimeoutSeconds, + ) + + utils.CheckComplianceStatus(g, managedPlc, "Compliant") + + relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(relatedObjects).To(HaveLen(1)) + + relatedObj := relatedObjects[0].(map[string]interface{}) + matchesAfterDryRun, _, _ := unstructured.NestedBool(relatedObj, "properties", "matchesAfterDryRun") + + g.Expect(matchesAfterDryRun).To(BeTrue()) + }, defaultTimeoutSeconds, 1).Should(Succeed()) + }) + + AfterAll(func() { + deleteConfigPolicies([]string{policyName, policyName}) + + utils.KubectlDelete("service", serviceName, "-n", "managed") + }) + }) +}) diff --git a/test/resources/case8_status_check/case8_service.yaml b/test/resources/case8_status_check/case8_service.yaml new file mode 100644 index 00000000..639af8dd --- /dev/null +++ b/test/resources/case8_status_check/case8_service.yaml @@ -0,0 +1,23 @@ +apiVersion: v1 +kind: Service +metadata: + name: grc-policy-propagator-metrics + namespace: managed + annotations: + test: test +spec: + internalTrafficPolicy: Cluster + ipFamilies: + - IPv4 + ipFamilyPolicy: SingleStack + ports: + - name: metrics-https + port: 8443 + protocol: TCP + targetPort: 8443 + selector: + app: grc + component: ocm-policy-propagator + release: grc + sessionAffinity: None + type: ClusterIP diff --git a/test/resources/case8_status_check/case8_service_inform.yaml b/test/resources/case8_status_check/case8_service_inform.yaml new file mode 100644 index 00000000..37f3dd4e --- /dev/null +++ b/test/resources/case8_status_check/case8_service_inform.yaml @@ -0,0 +1,36 @@ +apiVersion: policy.open-cluster-management.io/v1 +kind: ConfigurationPolicy +metadata: + name: policy-service + namespace: managed +spec: + remediationAction: inform + severity: low + object-templates: + - complianceType: mustonlyhave + metadataComplianceType: musthave + objectDefinition: + apiVersion: v1 + kind: Service + metadata: + name: grc-policy-propagator-metrics + namespace: managed + spec: # Intentionally omitting 'type' field - K8s will default to ClusterIP + clusterIP: '{{ (lookup "v1" "Service" "managed" "grc-policy-propagator-metrics").spec.clusterIP }}' + clusterIPs: + - '{{ (lookup "v1" "Service" "managed" "grc-policy-propagator-metrics").spec.clusterIP }}' + internalTrafficPolicy: Cluster + ipFamilies: + - IPv4 + ipFamilyPolicy: SingleStack + ports: + - name: metrics-https + port: 8443 + protocol: TCP + targetPort: 8443 + selector: + app: grc + component: ocm-policy-propagator + release: grc + sessionAffinity: None + From ec772e60860f519676739d6010bf7225d5cd6f23 Mon Sep 17 00:00:00 2001 From: Janelle Law Date: Wed, 13 Aug 2025 13:51:15 -0700 Subject: [PATCH 3/4] Omit dry run matching field when not in use ref: https://issues.redhat.com/browse/ACM-14577 Signed-off-by: Janelle Law --- api/v1/configurationpolicy_types.go | 2 +- controllers/configurationpolicy_controller.go | 22 +++++++++---------- ...r-management.io_configurationpolicies.yaml | 2 +- .../kustomization.yaml | 6 +++++ ...luster-management.io_operatorpolicies.yaml | 2 +- .../remove-matches-after-dry-run.json | 6 +++++ ...r-management.io_configurationpolicies.yaml | 2 +- ...luster-management.io_operatorpolicies.yaml | 6 ----- ...r-management.io_configurationpolicies.yaml | 2 +- test/e2e/case20_delete_objects_test.go | 4 ---- test/e2e/case8_status_check_test.go | 8 +++---- 11 files changed, 32 insertions(+), 30 deletions(-) create mode 100644 deploy/crds/kustomize_operatorpolicy/remove-matches-after-dry-run.json diff --git a/api/v1/configurationpolicy_types.go b/api/v1/configurationpolicy_types.go index 3307f147..4359cf9f 100644 --- a/api/v1/configurationpolicy_types.go +++ b/api/v1/configurationpolicy_types.go @@ -418,7 +418,7 @@ type ObjectProperties struct { // cluster. Diff string `json:"diff,omitempty"` - // MatchesAfterDryRun indicates whether the dry run update matches the policy assessment. If false, + // MatchesAfterDryRun indicates whether the object matches the policy after the dry run update. If true, // there was an initial mismatch between the policy and object, but the dry run update produced // a compliant result. MatchesAfterDryRun bool `json:"matchesAfterDryRun,omitempty"` diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index d683be9b..b39438aa 100644 --- a/controllers/configurationpolicy_controller.go +++ b/controllers/configurationpolicy_controller.go @@ -3189,7 +3189,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( if obj.existingObj == nil { log.Info("Skipping update: Previous object retrieval from the API server failed") - return false, "", "", false, nil, true + return false, "", "", false, nil, false } var res dynamic.ResourceInterface @@ -3211,7 +3211,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( objectT.MetadataComplianceType, ) if errMsg != "" { - return true, errMsg, "", true, nil, true + return true, errMsg, "", true, nil, false } recordDiff := objectT.RecordDiffWithDefault() @@ -3231,7 +3231,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation, "") - return throwSpecViolation, "", diff, updateNeeded, updatedObj, true + return throwSpecViolation, "", diff, updateNeeded, updatedObj, false } if updateNeeded { @@ -3275,7 +3275,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( r.setEvaluatedObject(obj.policy, obj.existingObj, false, message) } - return true, message, "", updateNeeded, nil, true + return true, message, "", updateNeeded, nil, false } // If an update is invalid (i.e. modifying Pod spec fields), then return noncompliant since that @@ -3301,7 +3301,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( r.setEvaluatedObject(obj.policy, obj.existingObj, false, message) - return true, message, diff, false, nil, true + return true, message, diff, false, nil, false } mergedObjCopy := obj.existingObj.DeepCopy() @@ -3327,7 +3327,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( // Assume object is compliant by inverting the value of throwSpecViolation r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation, "") - matchesAfterDryRun = false + matchesAfterDryRun = true return throwSpecViolation, "", diff, updateNeeded, updatedObj, matchesAfterDryRun } @@ -3339,7 +3339,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( if isInform { r.setEvaluatedObject(obj.policy, obj.existingObj, false, "") - return true, "", diff, false, nil, true + return true, "", diff, false, nil, matchesAfterDryRun } // If it's not inform (i.e. enforce), update the object @@ -3359,7 +3359,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( if err != nil && !k8serrors.IsNotFound(err) { message = fmt.Sprintf(`%s failed to delete when recreating with the error %v`, getMsgPrefix(&obj), err) - return true, message, "", updateNeeded, nil, true + return true, message, "", updateNeeded, nil, matchesAfterDryRun } attempts := 0 @@ -3377,7 +3377,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( message = getMsgPrefix(&obj) + " timed out waiting for the object to delete during recreate, " + "will retry on the next policy evaluation" - return true, message, "", updateNeeded, nil, true + return true, message, "", updateNeeded, nil, matchesAfterDryRun } time.Sleep(time.Second) @@ -3413,14 +3413,14 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( message = fmt.Sprintf("%s failed to %s with the error `%v`", getMsgPrefix(&obj), action, err) } - return true, message, diff, updateNeeded, nil, true + return true, message, diff, updateNeeded, nil, matchesAfterDryRun } if !statusMismatch { r.setEvaluatedObject(obj.policy, updatedObj, true, message) } - return throwSpecViolation, "", diff, updateNeeded, updatedObj, true + return throwSpecViolation, "", diff, updateNeeded, updatedObj, matchesAfterDryRun } func getMsgPrefix(obj *singleObject) string { diff --git a/deploy/crds/kustomize_configurationpolicy/policy.open-cluster-management.io_configurationpolicies.yaml b/deploy/crds/kustomize_configurationpolicy/policy.open-cluster-management.io_configurationpolicies.yaml index f1e6a4a9..315e3a86 100644 --- a/deploy/crds/kustomize_configurationpolicy/policy.open-cluster-management.io_configurationpolicies.yaml +++ b/deploy/crds/kustomize_configurationpolicy/policy.open-cluster-management.io_configurationpolicies.yaml @@ -479,7 +479,7 @@ spec: type: string matchesAfterDryRun: description: |- - MatchesAfterDryRun indicates whether the dry run update matches the policy assessment. If false, + MatchesAfterDryRun indicates whether the object matches the policy after the dry run update. If true, there was an initial mismatch between the policy and object, but the dry run update produced a compliant result. type: boolean diff --git a/deploy/crds/kustomize_operatorpolicy/kustomization.yaml b/deploy/crds/kustomize_operatorpolicy/kustomization.yaml index f9e9a83e..0a020aaa 100644 --- a/deploy/crds/kustomize_operatorpolicy/kustomization.yaml +++ b/deploy/crds/kustomize_operatorpolicy/kustomization.yaml @@ -14,3 +14,9 @@ patches: version: v1 kind: CustomResourceDefinition name: operatorpolicies.policy.open-cluster-management.io +- path: remove-matches-after-dry-run.json + target: + group: apiextensions.k8s.io + version: v1 + kind: CustomResourceDefinition + name: operatorpolicies.policy.open-cluster-management.io diff --git a/deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml b/deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml index e71929d0..f842ae70 100644 --- a/deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml +++ b/deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml @@ -386,7 +386,7 @@ spec: type: string matchesAfterDryRun: description: |- - MatchesAfterDryRun indicates whether the dry run update matches the policy assessment. If false, + MatchesAfterDryRun indicates whether the object matches the policy after the dry run update. If true, there was an initial mismatch between the policy and object, but the dry run update produced a compliant result. type: boolean diff --git a/deploy/crds/kustomize_operatorpolicy/remove-matches-after-dry-run.json b/deploy/crds/kustomize_operatorpolicy/remove-matches-after-dry-run.json new file mode 100644 index 00000000..aa963617 --- /dev/null +++ b/deploy/crds/kustomize_operatorpolicy/remove-matches-after-dry-run.json @@ -0,0 +1,6 @@ +[ + { + "op": "remove", + "path": "/spec/versions/0/schema/openAPIV3Schema/properties/status/properties/relatedObjects/items/properties/properties/properties/matchesAfterDryRun" + } +] diff --git a/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml b/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml index 5ba2ec69..7b6e1a43 100644 --- a/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml +++ b/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml @@ -486,7 +486,7 @@ spec: type: string matchesAfterDryRun: description: |- - MatchesAfterDryRun indicates whether the dry run update matches the policy assessment. If false, + MatchesAfterDryRun indicates whether the object matches the policy after the dry run update. If true, there was an initial mismatch between the policy and object, but the dry run update produced a compliant result. type: boolean diff --git a/deploy/crds/policy.open-cluster-management.io_operatorpolicies.yaml b/deploy/crds/policy.open-cluster-management.io_operatorpolicies.yaml index 3f676522..7026a80f 100644 --- a/deploy/crds/policy.open-cluster-management.io_operatorpolicies.yaml +++ b/deploy/crds/policy.open-cluster-management.io_operatorpolicies.yaml @@ -381,12 +381,6 @@ spec: CreatedByPolicy reports whether the object was created by the configuration policy, which is important when pruning is configured. type: boolean - matchesAfterDryRun: - description: |- - MatchesAfterDryRun indicates whether the dry run update matches the policy assessment. If false, - there was an initial mismatch between the policy and object, but the dry run update produced - a compliant result. - type: boolean uid: description: |- UID stores the object UID to help track object ownership for deletion when pruning is diff --git a/pkg/dryrun/policy.open-cluster-management.io_configurationpolicies.yaml b/pkg/dryrun/policy.open-cluster-management.io_configurationpolicies.yaml index 5ba2ec69..7b6e1a43 100644 --- a/pkg/dryrun/policy.open-cluster-management.io_configurationpolicies.yaml +++ b/pkg/dryrun/policy.open-cluster-management.io_configurationpolicies.yaml @@ -486,7 +486,7 @@ spec: type: string matchesAfterDryRun: description: |- - MatchesAfterDryRun indicates whether the dry run update matches the policy assessment. If false, + MatchesAfterDryRun indicates whether the object matches the policy after the dry run update. If true, there was an initial mismatch between the policy and object, but the dry run update produced a compliant result. type: boolean diff --git a/test/e2e/case20_delete_objects_test.go b/test/e2e/case20_delete_objects_test.go index c41e8858..b71d6dd6 100644 --- a/test/e2e/case20_delete_objects_test.go +++ b/test/e2e/case20_delete_objects_test.go @@ -160,10 +160,6 @@ var _ = Describe("Test Object deletion", Ordered, func() { uid, found, _ := unstructured.NestedString(relatedObj, "properties", "uid") Expect(found).To(BeTrue()) Expect(uid).ToNot(BeEmpty()) - - matchesAfterDryRun, found, _ := unstructured.NestedBool(relatedObj, "properties", "matchesAfterDryRun") - Expect(found).To(BeTrue()) - Expect(matchesAfterDryRun).To(BeTrue()) }) AfterAll(func() { policies := []string{ diff --git a/test/e2e/case8_status_check_test.go b/test/e2e/case8_status_check_test.go index c14c0cc9..15e0dbf4 100644 --- a/test/e2e/case8_status_check_test.go +++ b/test/e2e/case8_status_check_test.go @@ -203,7 +203,7 @@ var _ = Describe("Test related object property status", Ordered, func() { relatedObj := relatedObjects[0].(map[string]interface{}) matchesAfterDryRun, _, _ := unstructured.NestedBool(relatedObj, "properties", "matchesAfterDryRun") - g.Expect(matchesAfterDryRun).To(BeFalse()) + g.Expect(matchesAfterDryRun).To(BeTrue()) }, defaultTimeoutSeconds, 1).Should(Succeed()) }) @@ -226,7 +226,7 @@ var _ = Describe("Test related object property status", Ordered, func() { relatedObj := relatedObjects[0].(map[string]interface{}) matchesAfterDryRun, _, _ := unstructured.NestedBool(relatedObj, "properties", "matchesAfterDryRun") - g.Expect(matchesAfterDryRun).To(BeFalse()) + g.Expect(matchesAfterDryRun).To(BeTrue()) }, defaultTimeoutSeconds, 1).Should(Succeed()) }) @@ -250,7 +250,7 @@ var _ = Describe("Test related object property status", Ordered, func() { relatedObj := relatedObjects[0].(map[string]interface{}) matchesAfterDryRun, _, _ := unstructured.NestedBool(relatedObj, "properties", "matchesAfterDryRun") - g.Expect(matchesAfterDryRun).To(BeTrue()) + g.Expect(matchesAfterDryRun).To(BeFalse()) }, defaultTimeoutSeconds, 1).Should(Succeed()) }) @@ -274,7 +274,7 @@ var _ = Describe("Test related object property status", Ordered, func() { relatedObj := relatedObjects[0].(map[string]interface{}) matchesAfterDryRun, _, _ := unstructured.NestedBool(relatedObj, "properties", "matchesAfterDryRun") - g.Expect(matchesAfterDryRun).To(BeTrue()) + g.Expect(matchesAfterDryRun).To(BeFalse()) }, defaultTimeoutSeconds, 1).Should(Succeed()) }) From 981f733dbe4e1706356b5cac285c1a1ab0c8c559 Mon Sep 17 00:00:00 2001 From: Janelle Law Date: Wed, 13 Aug 2025 13:52:12 -0700 Subject: [PATCH 4/4] Treat missing and empty annotations as equivalent While testing https://issues.redhat.com/browse/ACM-14577 , some objects reported missing annotations as an empty map while other objects omitted the annotations key. Handle empty annotations before comparing objects. Signed-off-by: Janelle Law --- controllers/configurationpolicy_controller.go | 4 ++++ test/resources/case8_status_check/case8_service.yaml | 2 -- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index b39438aa..04885605 100644 --- a/controllers/configurationpolicy_controller.go +++ b/controllers/configurationpolicy_controller.go @@ -3575,6 +3575,10 @@ func removeFieldsForComparison(obj *unstructured.Unstructured) { ) // 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") + } } // setEvaluatedObject updates the cache to indicate that the ConfigurationPolicy has evaluated this diff --git a/test/resources/case8_status_check/case8_service.yaml b/test/resources/case8_status_check/case8_service.yaml index 639af8dd..eaa0b3d2 100644 --- a/test/resources/case8_status_check/case8_service.yaml +++ b/test/resources/case8_status_check/case8_service.yaml @@ -3,8 +3,6 @@ kind: Service metadata: name: grc-policy-propagator-metrics namespace: managed - annotations: - test: test spec: internalTrafficPolicy: Cluster ipFamilies: