diff --git a/Makefile b/Makefile index a510a49d..d4c2d928 100644 --- a/Makefile +++ b/Makefile @@ -225,10 +225,6 @@ e2e-test: e2e-dependencies e2e-test-coverage: E2E_TEST_ARGS = --json-report=report_e2e.json --label-filter='!hosted-mode && !running-in-cluster' --output-dir=. e2e-test-coverage: e2e-run-instrumented e2e-test e2e-stop-instrumented -.PHONY: e2e-test-coverage-foreground -e2e-test-coverage-foreground: LOG_REDIRECT = -e2e-test-coverage-foreground: e2e-test-coverage - .PHONY: e2e-test-hosted-mode-coverage e2e-test-hosted-mode-coverage: E2E_TEST_ARGS = --json-report=report_e2e_hosted_mode.json --label-filter="hosted-mode && !running-in-cluster" --output-dir=. e2e-test-hosted-mode-coverage: COVERAGE_E2E_OUT = coverage_e2e_hosted_mode.out @@ -244,9 +240,8 @@ e2e-build-instrumented: go test -covermode=atomic -coverpkg=$(shell cat go.mod | head -1 | cut -d ' ' -f 2)/... -c -tags e2e ./ -o build/_output/bin/$(IMG)-instrumented .PHONY: e2e-run-instrumented -LOG_REDIRECT ?= &>build/_output/controller.log e2e-run-instrumented: e2e-build-instrumented - WATCH_NAMESPACE="$(WATCH_NAMESPACE)" ./build/_output/bin/$(IMG)-instrumented -test.run "^TestRunMain$$" -test.coverprofile=$(COVERAGE_E2E_OUT) $(LOG_REDIRECT) & + WATCH_NAMESPACE="$(WATCH_NAMESPACE)" ./build/_output/bin/$(IMG)-instrumented -test.run "^TestRunMain$$" -test.coverprofile=$(COVERAGE_E2E_OUT) 2>&1 | tee ./build/_output/controller.log & .PHONY: e2e-stop-instrumented e2e-stop-instrumented: diff --git a/api/v1/configurationpolicy_types.go b/api/v1/configurationpolicy_types.go index 93450aa0..323743dd 100644 --- a/api/v1/configurationpolicy_types.go +++ b/api/v1/configurationpolicy_types.go @@ -187,8 +187,20 @@ 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". + RecordDiff RecordDiff `json:"recordDiff,omitempty"` } +// +kubebuilder:validation:Enum=Log;None +type RecordDiff string + +const ( + RecordDiffLog RecordDiff = "Log" + RecordDiffNone RecordDiff = "None" +) + // ConfigurationPolicyStatus defines the observed state of ConfigurationPolicy type ConfigurationPolicyStatus struct { ComplianceState ComplianceState `json:"compliant,omitempty"` // Compliant/NonCompliant/UnknownCompliancy @@ -211,9 +223,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" diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index 2de015a7..60e35fec 100644 --- a/controllers/configurationpolicy_controller.go +++ b/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, ) @@ -2633,16 +2632,6 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( } if keyUpdateNeeded { - isInform := strings.EqualFold(string(remediation), string(policyv1.Inform)) - - // If a key didn't match but the cluster supports dry run mode, then continue merging the object - // and then run a dry run update request to see if the Kubernetes API agrees with the assesment. - if isInform && !r.DryRunSupported { - r.setEvaluatedObject(obj.policy, obj.existingObj, false) - - return true, "", false, false - } - if isStatus { throwSpecViolation = true statusMismatch = true @@ -2651,16 +2640,12 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( } else { updateNeeded = true - if !isInform { - log.Info("Queuing an update for the object due to a value mismatch", "key", key) - } + log.Info("Detected value mismatch for object key: " + key) } } } if updateNeeded { - log.V(2).Info("Updating the object based on the template definition") - // FieldValidation is supported in k8s 1.25 as beta release // so if the version is below 1.25, we need to use client side validation to validate the object if semver.Compare(r.serverVersion, "v1.25.0") < 0 { @@ -2690,13 +2675,13 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( // If it's a conflict, refetch the object and try again. if k8serrors.IsConflict(err) { - log.Info("The object updating during the evaluation. Trying again.") + log.Info("The object was updating during the evaluation. Trying again.") rv, getErr := res.Get(context.TODO(), obj.existingObj.GetName(), metav1.GetOptions{}) if getErr == nil { obj.existingObj = rv - return r.checkAndUpdateResource(obj, complianceType, mdComplianceType, remediation) + return r.checkAndUpdateResource(obj, objectT, remediation) } } @@ -2725,14 +2710,44 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( return false, "", false, false } - // 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) + // 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) + } + } + } else if objectT.RecordDiff == policyv1.RecordDiffLog { + // Generate and log the diff for when dryrun is unsupported (i.e. OCP v3.11) + mergedObjCopy := obj.existingObj.DeepCopy() + removeFieldsForComparison(mergedObjCopy) - return true, "", false, false + diff, err := generateDiff(existingObjectCopy, mergedObjCopy) + if err != nil { + log.Info("Failed to generate the diff: " + err.Error()) + } else { + log.Info("Logging the diff:\n" + diff) } } + // Log a configuration breadcrumb for users that might be looking in the logs for a diff + if objectT.RecordDiff != policyv1.RecordDiffLog { + log.Info("(Diff disabled. To log the diff, " + + "set 'spec.object-tempates[].objectDefinition.recordDiff' to 'Log' for this object-template).") + } + + // 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) + + return true, "", false, false + } + + // If it's not inform (i.e. enforce), update the object + log.Info("Updating the object based on the template definition") + updatedObj, err := res.Update(context.TODO(), obj.existingObj, metav1.UpdateOptions{ FieldValidation: metav1.FieldValidationStrict, }) @@ -2744,7 +2759,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( if getErr == nil { obj.existingObj = rv - return r.checkAndUpdateResource(obj, complianceType, mdComplianceType, remediation) + return r.checkAndUpdateResource(obj, objectT, remediation) } } diff --git a/controllers/configurationpolicy_utils.go b/controllers/configurationpolicy_utils.go index 0df60d2a..bfd8460e 100644 --- a/controllers/configurationpolicy_utils.go +++ b/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,45 @@ func containRelated(related []policyv1.RelatedObject, input policyv1.RelatedObje return false } + +// generateDiff takes two unstructured objects and returns the diff between the two embedded objects +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 +} diff --git a/controllers/configurationpolicy_utils_test.go b/controllers/configurationpolicy_utils_test.go index 914da825..571941b7 100644 --- a/controllers/configurationpolicy_utils_test.go +++ b/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 + }{ + "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 { + t.Fatal(fmt.Errorf("Encountered unexpected error: %w", err)) + } + + // 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) + }) + } +} 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 f4a23a79..910c6915 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 @@ -161,6 +161,14 @@ spec: object type: object x-kubernetes-preserve-unknown-fields: true + recordDiff: + description: RecordDiff specifies whether (and where) to log + the diff between the object on the cluster and the objectDefinition + in the policy. Defaults to "None". + enum: + - Log + - None + type: string required: - complianceType - objectDefinition diff --git a/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml b/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml index 0e323db3..4a2ed4de 100644 --- a/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml +++ b/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml @@ -168,6 +168,14 @@ spec: object type: object x-kubernetes-preserve-unknown-fields: true + recordDiff: + description: RecordDiff specifies whether (and where) to log + the diff between the object on the cluster and the objectDefinition + in the policy. Defaults to "None". + enum: + - Log + - None + type: string required: - complianceType - objectDefinition diff --git a/go.mod b/go.mod index bfb711c7..a320cfdc 100644 --- a/go.mod +++ b/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 diff --git a/test/e2e/case27_showupdateinstatus_test.go b/test/e2e/case27_showupdateinstatus_test.go index 61317c91..6ef725da 100644 --- a/test/e2e/case27_showupdateinstatus_test.go +++ b/test/e2e/case27_showupdateinstatus_test.go @@ -11,7 +11,7 @@ import ( ) const ( - case27ConfigPolicyName string = "policy-cfgmap-create" + case27ConfigPolicyName string = "case27-policy-cfgmap-create" case27CreateYaml string = "../resources/case27_showupdateinstatus/case27-create-cfgmap-policy.yaml" case27UpdateYaml string = "../resources/case27_showupdateinstatus/case27-update-cfgmap-policy.yaml" ) diff --git a/test/e2e/case39_diff_generation_test.go b/test/e2e/case39_diff_generation_test.go new file mode 100644 index 00000000..353c1b31 --- /dev/null +++ b/test/e2e/case39_diff_generation_test.go @@ -0,0 +1,96 @@ +// Copyright (c) 2020 Red Hat, Inc. +// Copyright Contributors to the Open Cluster Management project + +package e2e + +import ( + "bufio" + "fmt" + "os" + "strings" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "open-cluster-management.io/config-policy-controller/test/utils" +) + +var _ = Describe("Generate the diff", Ordered, func() { + const ( + logPath string = "../../build/_output/controller.log" + configPolicyName string = "case39-policy-cfgmap-create" + createYaml string = "../resources/case39_diff_generation/case39-create-cfgmap-policy.yaml" + updateYaml string = "../resources/case39_diff_generation/case39-update-cfgmap-policy.yaml" + ) + + BeforeAll(func() { + _, err := os.Stat(logPath) + if err != nil { + Skip(fmt.Sprintf("Skipping. Failed to find log file %s: %s", logPath, err.Error())) + } + }) + + It("configmap should be created properly on the managed cluster", func() { + By("Creating " + configPolicyName + " on managed") + utils.Kubectl("apply", "-f", createYaml, "-n", testNamespace) + plc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, + configPolicyName, testNamespace, true, defaultTimeoutSeconds) + Expect(plc).NotTo(BeNil()) + Eventually(func() interface{} { + managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, + configPolicyName, testNamespace, true, defaultTimeoutSeconds) + + return utils.GetStatusMessage(managedPlc) + }, 120, 1).Should(Equal("configmaps [case39-map] found as specified in namespace default")) + }) + + It("configmap and status should be updated properly on the managed cluster", func() { + By("Updating " + configPolicyName + " on managed") + utils.Kubectl("apply", "-f", updateYaml, "-n", testNamespace) + Eventually(func() interface{} { + managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, + configPolicyName, testNamespace, true, defaultTimeoutSeconds) + + return utils.GetStatusMessage(managedPlc) + }, 30, 0.5).Should(Equal("configmaps [case39-map] was updated successfully in namespace default")) + }) + + It("diff should be logged by the controller", func() { + By("Checking the controller logs") + logFile, err := os.Open(logPath) + Expect(err).ToNot(HaveOccurred()) + defer logFile.Close() + + diff := "" + foundDiff := false + logScanner := bufio.NewScanner(logFile) + logScanner.Split(bufio.ScanLines) + for logScanner.Scan() { + line := logScanner.Text() + if foundDiff && strings.HasPrefix(line, "\t{") { + foundDiff = false + } else if foundDiff || strings.Contains(line, "Logging the diff:") { + foundDiff = true + } else { + continue + } + + diff += line + "\n" + } + + Expect(diff).Should(ContainSubstring(`Logging the diff: +--- default/case39-map : existing ++++ default/case39-map : updated +@@ -2,3 +2,3 @@ + data: +- fieldToUpdate: "1" ++ fieldToUpdate: "2" + kind: ConfigMap + {"policy": "case39-policy-cfgmap-create", "name": "case39-map", "namespace": "default", "resource": "configmaps"}`)) + }) + + AfterAll(func() { + deleteConfigPolicies([]string{configPolicyName}) + utils.Kubectl("delete", "configmap", "case39-map", "--ignore-not-found") + }) +}) diff --git a/test/resources/case27_showupdateinstatus/case27-create-cfgmap-policy.yaml b/test/resources/case27_showupdateinstatus/case27-create-cfgmap-policy.yaml index 0868ce52..fc55f08e 100644 --- a/test/resources/case27_showupdateinstatus/case27-create-cfgmap-policy.yaml +++ b/test/resources/case27_showupdateinstatus/case27-create-cfgmap-policy.yaml @@ -1,7 +1,7 @@ apiVersion: policy.open-cluster-management.io/v1 kind: ConfigurationPolicy metadata: - name: policy-cfgmap-create + name: case27-policy-cfgmap-create spec: remediationAction: enforce namespaceSelector: diff --git a/test/resources/case27_showupdateinstatus/case27-update-cfgmap-policy.yaml b/test/resources/case27_showupdateinstatus/case27-update-cfgmap-policy.yaml index 6ed25c06..b1e7031b 100644 --- a/test/resources/case27_showupdateinstatus/case27-update-cfgmap-policy.yaml +++ b/test/resources/case27_showupdateinstatus/case27-update-cfgmap-policy.yaml @@ -1,7 +1,7 @@ apiVersion: policy.open-cluster-management.io/v1 kind: ConfigurationPolicy metadata: - name: policy-cfgmap-create + name: case27-policy-cfgmap-create spec: remediationAction: enforce namespaceSelector: diff --git a/test/resources/case39_diff_generation/case39-create-cfgmap-policy.yaml b/test/resources/case39_diff_generation/case39-create-cfgmap-policy.yaml new file mode 100644 index 00000000..347ce28a --- /dev/null +++ b/test/resources/case39_diff_generation/case39-create-cfgmap-policy.yaml @@ -0,0 +1,19 @@ +apiVersion: policy.open-cluster-management.io/v1 +kind: ConfigurationPolicy +metadata: + name: case39-policy-cfgmap-create +spec: + remediationAction: enforce + namespaceSelector: + exclude: ["kube-*"] + include: ["default"] + object-templates: + - complianceType: musthave + recordDiff: Log + objectDefinition: + apiVersion: v1 + kind: ConfigMap + metadata: + name: case39-map + data: + fieldToUpdate: "1" diff --git a/test/resources/case39_diff_generation/case39-update-cfgmap-policy.yaml b/test/resources/case39_diff_generation/case39-update-cfgmap-policy.yaml new file mode 100644 index 00000000..40afd05a --- /dev/null +++ b/test/resources/case39_diff_generation/case39-update-cfgmap-policy.yaml @@ -0,0 +1,19 @@ +apiVersion: policy.open-cluster-management.io/v1 +kind: ConfigurationPolicy +metadata: + name: case39-policy-cfgmap-create +spec: + remediationAction: enforce + namespaceSelector: + exclude: ["kube-*"] + include: ["default"] + object-templates: + - complianceType: musthave + recordDiff: Log + objectDefinition: + apiVersion: v1 + kind: ConfigMap + metadata: + name: case39-map + data: + fieldToUpdate: "2"