Add diff logging

Adds a `recordDiff` enum parameter to the
`ConfigurationPolicy`. When set to `Log`, it uses
the `go-difflib` package to compare the YAML
marshalled into strings. While `go-difflib` is
unmaintained, it's extensively imported, in
particular by the `stretchr/testify` package here.

For simplicity, logging the diff for
objectDefinitions without a name specified are
not logged.

ref: https://issues.redhat.com/browse/ACM-9072
Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com>

api/v1/configurationpolicy_types.go

@@ -187,8 +187,21 @@ type ObjectTemplate struct {
 	// ObjectDefinition defines required fields for the object
 	// +kubebuilder:pruning:PreserveUnknownFields
 	ObjectDefinition runtime.RawExtension `json:"objectDefinition"`
+
+	// RecordDiff specifies whether (and where) to log the diff between the object on the
+	// cluster and the objectDefinition in the policy. Defaults to "None".
+	// +kubebuilder:default:=None
+	RecordDiff RecordDiff `json:"recordDiff,omitempty"`
 }
 
+// +kubebuilder:validation:Enum=Log;None
+type RecordDiff string
+
+const (
+	RecordDiffLog  = "Log"
+	RecordDiffNone = "None"
+)
+
 // ConfigurationPolicyStatus defines the observed state of ConfigurationPolicy
 type ConfigurationPolicyStatus struct {
 	ComplianceState   ComplianceState  `json:"compliant,omitempty"`         // Compliant/NonCompliant/UnknownCompliancy
@@ -211,9 +224,6 @@ type CompliancePerClusterStatus struct {
 // ComplianceMap map to hold CompliancePerClusterStatus objects
 type ComplianceMap map[string]*CompliancePerClusterStatus
 
-// ResourceState genric description of a state
-type ResourceState string
-
 //+kubebuilder:object:root=true
 //+kubebuilder:subresource:status
 //+kubebuilder:printcolumn:name="Compliance state",type="string",JSONPath=".status.compliant"

controllers/configurationpolicy_controller.go

@@ -1775,11 +1775,8 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
 
 			throwSpecViolation = !compliant
 		} else {
-			compType := strings.ToLower(string(objectT.ComplianceType))
-			mdCompType := strings.ToLower(string(objectT.MetadataComplianceType))
-
 			throwSpecViolation, msg, triedUpdate, updatedObj = r.checkAndUpdateResource(
-				obj, compType, mdCompType, remediation,
+				obj, objectT, remediation,
 			)
 		}
 
@@ -2549,10 +2546,12 @@ type cachedEvaluationResult struct {
 // successfully.
 func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 	obj singleObject,
-	complianceType string,
-	mdComplianceType string,
+	objectT *policyv1.ObjectTemplate,
 	remediation policyv1.RemediationAction,
 ) (throwSpecViolation bool, message string, updateNeeded bool, updateSucceeded bool) {
+	complianceType := strings.ToLower(string(objectT.ComplianceType))
+	mdComplianceType := strings.ToLower(string(objectT.MetadataComplianceType))
+
 	log := log.WithValues(
 		"policy", obj.policy.Name, "name", obj.name, "namespace", obj.namespace, "resource", obj.gvr.Resource,
 	)
@@ -2696,7 +2695,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 					if getErr == nil {
 						obj.existingObj = rv
 
-						return r.checkAndUpdateResource(obj, complianceType, mdComplianceType, remediation)
+						return r.checkAndUpdateResource(obj, objectT, remediation)
 					}
 				}
 
@@ -2725,6 +2724,16 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 				return false, "", false, false
 			}
 
+			// Generate and log the diff
+			if objectT.RecordDiff == policyv1.RecordDiffLog {
+				diff, err := generateDiff(existingObjectCopy, dryRunUpdatedObj)
+				if err != nil {
+					log.Info("Failed to generate the diff: " + err.Error())
+				} else {
+					log.Info("Logging the diff:\n" + diff)
+				}
+			}
+
 			// The object would have been updated, so if it's inform, return as noncompliant.
 			if strings.EqualFold(string(remediation), string(policyv1.Inform)) {
 				r.setEvaluatedObject(obj.policy, obj.existingObj, false)
@@ -2744,7 +2753,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 				if getErr == nil {
 					obj.existingObj = rv
 
-					return r.checkAndUpdateResource(obj, complianceType, mdComplianceType, remediation)
+					return r.checkAndUpdateResource(obj, objectT, remediation)
 				}
 			}
 

controllers/configurationpolicy_utils.go

@@ -11,11 +11,13 @@ import (
 	"strings"
 
 	gocmp "github.com/google/go-cmp/cmp"
+	"github.com/pmezard/go-difflib/difflib"
 	apiRes "k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/json"
+	"sigs.k8s.io/yaml"
 
 	policyv1 "open-cluster-management.io/config-policy-controller/api/v1"
 )
@@ -676,3 +678,44 @@ func containRelated(related []policyv1.RelatedObject, input policyv1.RelatedObje
 
 	return false
 }
+
+func generateDiff(existingObj, updatedObj *unstructured.Unstructured) (string, error) {
+	// Marshal YAML to []byte and parse object names for logging
+	existingYAML, err := yaml.Marshal(existingObj.Object)
+	if err != nil {
+		return "", fmt.Errorf("failed to marshal existing object to YAML for diff: %w", err)
+	}
+
+	existingYAMLName := existingObj.GetName() + " : existing"
+	if existingObj.GetNamespace() != "" {
+		existingYAMLName = existingObj.GetNamespace() + "/" + existingYAMLName
+	}
+
+	updatedYAML, err := yaml.Marshal(updatedObj.Object)
+	if err != nil {
+		return "", fmt.Errorf("failed to marshal updated object to YAML for diff: %w", err)
+	}
+
+	updatedYAMLName := updatedObj.GetName() + " : updated"
+	if updatedObj.GetNamespace() != "" {
+		updatedYAMLName = updatedObj.GetNamespace() + "/" + updatedYAMLName
+	}
+
+	// Set the diffing configuration
+	// See https://pkg.go.dev/github.com/pmezard/go-difflib/difflib#UnifiedDiff
+	unifiedDiff := difflib.UnifiedDiff{
+		A:        difflib.SplitLines(string(existingYAML)),
+		FromFile: existingYAMLName,
+		B:        difflib.SplitLines(string(updatedYAML)),
+		ToFile:   updatedYAMLName,
+		Context:  1,
+	}
+
+	// Generate and return the diff
+	diff, err := difflib.GetUnifiedDiffString(unifiedDiff)
+	if err != nil {
+		return "", fmt.Errorf("failed to generate diff: %w", err)
+	}
+
+	return diff, nil
+}

controllers/configurationpolicy_utils_test.go

@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
 	policyv1 "open-cluster-management.io/config-policy-controller/api/v1"
 )
@@ -197,3 +198,112 @@ func TestEqualObjWithSortEmptyMap(t *testing.T) {
 	assert.True(t, equalObjWithSort(mergedObj, oldObj, true))
 	assert.False(t, equalObjWithSort(mergedObj, oldObj, false))
 }
+
+func TestGenerateDiff(t *testing.T) {
+	t.Parallel()
+
+	tests := map[string]struct {
+		existingObj  map[string]interface{}
+		updatedObj   map[string]interface{}
+		expectedDiff string
+		expectedErr  string
+	}{
+		"same object generates no diff": {
+			existingObj: map[string]interface{}{
+				"cities": map[string]interface{}{},
+			},
+			updatedObj: map[string]interface{}{
+				"cities": map[string]interface{}{},
+			},
+		},
+		"object with new child key": {
+			existingObj: map[string]interface{}{
+				"cities": map[string]interface{}{},
+			},
+			updatedObj: map[string]interface{}{
+				"cities": map[string]interface{}{
+					"raleigh": map[string]interface{}{},
+				},
+			},
+			expectedDiff: `
+@@ -1,2 +1,3 @@
+-cities: {}
++cities:
++  raleigh: {}`,
+		},
+		"object with new key": {
+			existingObj: map[string]interface{}{
+				"cities": map[string]interface{}{},
+			},
+			updatedObj: map[string]interface{}{
+				"cities": map[string]interface{}{},
+				"states": map[string]interface{}{},
+			},
+			expectedDiff: `
+@@ -1,2 +1,3 @@
+ cities: {}
++states: {}`,
+		},
+		"array with added item": {
+			existingObj: map[string]interface{}{
+				"cities": []string{
+					"Raleigh",
+				},
+			},
+			updatedObj: map[string]interface{}{
+				"cities": []string{
+					"Raleigh",
+					"Durham",
+				},
+			},
+			expectedDiff: `
+@@ -2,2 +2,3 @@
+ - Raleigh
++- Durham`,
+		},
+		"array with removed item": {
+			existingObj: map[string]interface{}{
+				"cities": []string{
+					"Raleigh",
+					"Durham",
+				},
+			},
+			updatedObj: map[string]interface{}{
+				"cities": []string{
+					"Raleigh",
+				},
+			},
+			expectedDiff: `
+@@ -2,3 +2,2 @@
+ - Raleigh
+-- Durham`,
+		},
+	}
+
+	for testName, test := range tests {
+		test := test
+		t.Run(testName, func(t *testing.T) {
+			t.Parallel()
+
+			existingObj := &unstructured.Unstructured{
+				Object: test.existingObj,
+			}
+			updatedObj := &unstructured.Unstructured{
+				Object: test.updatedObj,
+			}
+
+			diff, err := generateDiff(existingObj, updatedObj)
+			if err != nil {
+				assert.EqualError(t, err, test.expectedErr)
+			}
+
+			// go-diff adds a trailing newline and whitespace, which gets
+			// chomped when logging, so adding it here just for the test,
+			// along with the common prefix
+			if test.expectedDiff != "" {
+				test.expectedDiff = "---  : existing\n+++  : updated" + test.expectedDiff + "\n \n"
+			}
+			assert.Equal(t, test.expectedDiff, diff)
+		})
+	}
+}

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

@@ -161,6 +161,15 @@ spec:
                         object
                       type: object
                       x-kubernetes-preserve-unknown-fields: true
+                    recordDiff:
+                      default: None
+                      description: Record diff specifies whether (and where) to log
+                        the diff between the object on the cluster and the objectDefinition
+                        in the policy.
+                      enum:
+                      - Log
+                      - None
+                      type: string
                   required:
                   - complianceType
                   - objectDefinition

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

@@ -168,6 +168,15 @@ spec:
                         object
                       type: object
                       x-kubernetes-preserve-unknown-fields: true
+                    recordDiff:
+                      default: None
+                      description: Record diff specifies whether (and where) to log
+                        the diff between the object on the cluster and the objectDefinition
+                        in the policy.
+                      enum:
+                      - Log
+                      - None
+                      type: string
                   required:
                   - complianceType
                   - objectDefinition

go.mod

@@ -9,6 +9,7 @@ require (
 	github.com/onsi/ginkgo/v2 v2.13.0
 	github.com/onsi/gomega v1.28.1
 	github.com/operator-framework/api v0.17.6
+	github.com/pmezard/go-difflib v1.0.0
 	github.com/prometheus/client_golang v1.17.0
 	github.com/spf13/pflag v1.0.5
 	github.com/stolostron/go-log-utils v0.1.2
@@ -69,7 +70,6 @@ require (
 	github.com/monochromegane/go-gitignore v0.0.0-20200626010858-205db1a8cc00 // indirect
 	github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
 	github.com/pkg/errors v0.9.1 // indirect
-	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/prometheus/client_model v0.5.0 // indirect
 	github.com/prometheus/common v0.45.0 // indirect
 	github.com/prometheus/procfs v0.12.0 // indirect