diff --git a/api/v1/configurationpolicy_types.go b/api/v1/configurationpolicy_types.go index 9411ab40..e0a07f67 100644 --- a/api/v1/configurationpolicy_types.go +++ b/api/v1/configurationpolicy_types.go @@ -11,6 +11,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" runtime "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -199,16 +200,48 @@ type ObjectTemplate struct { 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". + // cluster and the objectDefinition in the policy. Defaults to "None" when the object kind is + // ConfigMap, OAuthAccessToken, OAuthAuthorizeTokens, Route, or Secret. Defaults to "InStatus" otherwise. RecordDiff RecordDiff `json:"recordDiff,omitempty"` } -// +kubebuilder:validation:Enum=Log;None +func (o *ObjectTemplate) RecordDiffWithDefault() RecordDiff { + if o.RecordDiff != "" { + return o.RecordDiff + } + + _, gvk, err := unstructured.UnstructuredJSONScheme.Decode(o.ObjectDefinition.Raw, nil, nil) + if err != nil { + return o.RecordDiff + } + + switch gvk.Group { + case "": + if gvk.Kind == "ConfigMap" || gvk.Kind == "Secret" { + return RecordDiffCensored + } + case "oauth.openshift.io": + if gvk.Kind == "OAuthAccessToken" || gvk.Kind == "OAuthAuthorizeTokens" { + return RecordDiffCensored + } + case "route.openshift.io": + if gvk.Kind == "Route" { + return RecordDiffCensored + } + } + + return RecordDiffInStatus +} + +// +kubebuilder:validation:Enum=Log;InStatus;None type RecordDiff string const ( - RecordDiffLog RecordDiff = "Log" - RecordDiffNone RecordDiff = "None" + RecordDiffLog RecordDiff = "Log" + RecordDiffInStatus RecordDiff = "InStatus" + RecordDiffNone RecordDiff = "None" + // Censored is only used as an internal value to indicate a diff shouldn't be automatically generated. + RecordDiffCensored RecordDiff = "Censored" ) // ConfigurationPolicyStatus defines the observed state of ConfigurationPolicy @@ -355,7 +388,8 @@ type ObjectProperties struct { // Whether the object was created by the parent policy CreatedByPolicy *bool `json:"createdByPolicy,omitempty"` // Store object UID to help track object ownership for deletion - UID string `json:"uid,omitempty"` + UID string `json:"uid,omitempty"` + Diff string `json:"diff,omitempty"` } func init() { diff --git a/api/v1/configurationpolicy_types_test.go b/api/v1/configurationpolicy_types_test.go new file mode 100644 index 00000000..edba5598 --- /dev/null +++ b/api/v1/configurationpolicy_types_test.go @@ -0,0 +1,86 @@ +// Copyright Contributors to the Open Cluster Management project + +package v1 + +import ( + "fmt" + "testing" + + "k8s.io/apimachinery/pkg/runtime" +) + +func TestRecordDiffWithDefault(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + apiVersion string + kind string + recordDiff RecordDiff + expected RecordDiff + }{ + "Namespace-recordDiff-unset": { + apiVersion: "v1", + kind: "Namespace", + expected: RecordDiffInStatus, + }, + "Namespace-recordDiff-set": { + apiVersion: "v1", + kind: "Namespace", + recordDiff: RecordDiffLog, + expected: RecordDiffLog, + }, + "Configmap-recordDiff-unset": { + apiVersion: "v1", + kind: "ConfigMap", + expected: RecordDiffCensored, + }, + "Secret-recordDiff-unset": { + apiVersion: "v1", + kind: "Secret", + expected: RecordDiffCensored, + }, + "Secret-recordDiff-set": { + apiVersion: "v1", + kind: "Secret", + recordDiff: RecordDiffInStatus, + expected: RecordDiffInStatus, + }, + "OAuthAccessToken-recordDiff-unset": { + apiVersion: "oauth.openshift.io/v1", + kind: "OAuthAccessToken", + expected: RecordDiffCensored, + }, + "OAuthAuthorizeTokens-recordDiff-unset": { + apiVersion: "oauth.openshift.io/v1", + kind: "OAuthAuthorizeTokens", + expected: RecordDiffCensored, + }, + "Route-recordDiff-unset": { + apiVersion: "route.openshift.io/v1", + kind: "Route", + expected: RecordDiffCensored, + }, + } + + for testName, test := range tests { + test := test + + t.Run( + testName, + func(t *testing.T) { + t.Parallel() + + objTemp := ObjectTemplate{ + ObjectDefinition: runtime.RawExtension{ + Raw: []byte(fmt.Sprintf(`{"apiVersion": "%s", "kind": "%s"}`, test.apiVersion, test.kind)), + }, + RecordDiff: test.recordDiff, + } + + if objTemp.RecordDiffWithDefault() != test.expected { + t.Fatalf("Expected %s but got %s", test.expected, objTemp.RecordDiffWithDefault()) + } + }, + ) + } +} diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index 7226b9bb..fdcb8fce 100644 --- a/controllers/configurationpolicy_controller.go +++ b/controllers/configurationpolicy_controller.go @@ -15,6 +15,7 @@ import ( "sync" "time" + "github.com/go-logr/logr" gocmp "github.com/google/go-cmp/cmp" "github.com/prometheus/client_golang/prometheus" templates "github.com/stolostron/go-template-utils/v4/pkg/templates" @@ -1030,11 +1031,34 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi return } + if resolvedTemplate.HasSensitiveData { + for i := range objTemps { + if objTemps[i].RecordDiff == "" { + log.V(1).Info( + "Not automatically turning on recordDiff due to templates interacting with "+ + "sensitive data", + "objectTemplateIndex", i, + ) + + objTemps[i].RecordDiff = policyv1.RecordDiffCensored + } + } + } + plc.Spec.ObjectTemplates = objTemps break } + if plc.Spec.ObjectTemplates[i].RecordDiff == "" && resolvedTemplate.HasSensitiveData { + log.V(1).Info( + "Not automatically turning on recordDiff due to templates interacting with sensitive data", + "objectTemplateIndex", i, + ) + + plc.Spec.ObjectTemplates[i].RecordDiff = policyv1.RecordDiffCensored + } + // Otherwise, set the resolved data for use in further processing plc.Spec.ObjectTemplates[i].ObjectDefinition.Raw = resolvedTemplate.ResolvedJSON } else if isRawObjTemplate { @@ -1306,7 +1330,8 @@ func (r *ConfigurationPolicyReconciler) sortRelatedObjectsAndUpdate( newEntry.Properties.CreatedByPolicy != nil && !(*newEntry.Properties.CreatedByPolicy) { // Use the old properties if they existed and this is not a newly created resource - related[i].Properties = oldEntry.Properties + related[i].Properties.CreatedByPolicy = oldEntry.Properties.CreatedByPolicy + related[i].Properties.UID = oldEntry.Properties.UID if collectMetrics { found[objKey] = true @@ -1552,9 +1577,9 @@ func (r *ConfigurationPolicyReconciler) handleObjects( log.V(2).Info("Handling a single object template") - var creationInfo *policyv1.ObjectProperties + var objectProperties *policyv1.ObjectProperties - result, creationInfo = r.handleSingleObj(singObj, remediation, exists, objectT) + result, objectProperties = r.handleSingleObj(singObj, remediation, exists, objectT) if len(result.events) != 0 { event := result.events[len(result.events)-1] @@ -1566,7 +1591,7 @@ func (r *ConfigurationPolicyReconciler) handleObjects( objDetails.isNamespaced, result.objectNames, event.reason, - creationInfo, + objectProperties, ) } } else { // This case only occurs when the desired object is not named @@ -1668,7 +1693,7 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj( objectT *policyv1.ObjectTemplate, ) ( result objectTmplEvalResult, - creationInfo *policyv1.ObjectProperties, + objectProperties *policyv1.ObjectProperties, ) { objLog := log.WithValues("object", obj.name, "policy", obj.policy.Name, "index", obj.index) @@ -1706,7 +1731,7 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj( objLog.Error(err, "Could not handle missing musthave object") } else { created := true - creationInfo = &policyv1.ObjectProperties{ + objectProperties = &policyv1.ObjectProperties{ CreatedByPolicy: &created, UID: uid, } @@ -1745,14 +1770,25 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj( log.V(2).Info("The object already exists. Verifying the object fields match what is desired.") var throwSpecViolation, triedUpdate, updatedObj bool - var msg string + var msg, diff string + + uid := string(obj.existingObj.GetUID()) if evaluated, compliant := r.alreadyEvaluated(obj.policy, obj.existingObj); evaluated { log.V(1).Info("Skipping object comparison since the resourceVersion hasn't changed") + for _, relatedObj := range obj.policy.Status.RelatedObjects { + if relatedObj.Properties != nil && relatedObj.Properties.UID == uid { + // Retain the diff from the previous evaluation + diff = relatedObj.Properties.Diff + + break + } + } + throwSpecViolation = !compliant } else { - throwSpecViolation, msg, triedUpdate, updatedObj = r.checkAndUpdateResource( + throwSpecViolation, msg, diff, triedUpdate, updatedObj = r.checkAndUpdateResource( obj, objectT, remediation, ) } @@ -1762,6 +1798,8 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj( result.events = append(result.events, objectTmplEvalEvent{false, reasonWantFoundNoMatch, ""}) } + created := false + if throwSpecViolation { var resultReason, resultMsg string @@ -1772,6 +1810,12 @@ 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 @@ -1781,10 +1825,11 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj( } else { result.events = append(result.events, objectTmplEvalEvent{true, reasonWantFoundExists, ""}) } - created := false - creationInfo = &policyv1.ObjectProperties{ + + objectProperties = &policyv1.ObjectProperties{ CreatedByPolicy: &created, - UID: "", + UID: uid, + Diff: diff, } } else { result.events = append(result.events, objectTmplEvalEvent{true, reasonWantFoundExists, ""}) @@ -2524,7 +2569,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( obj singleObject, objectT *policyv1.ObjectTemplate, remediation policyv1.RemediationAction, -) (throwSpecViolation bool, message string, updateNeeded bool, updateSucceeded bool) { +) (throwSpecViolation bool, message string, diff string, updateNeeded bool, updateSucceeded bool) { complianceType := strings.ToLower(string(objectT.ComplianceType)) mdComplianceType := strings.ToLower(string(objectT.MetadataComplianceType)) @@ -2552,7 +2597,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( if obj.existingObj == nil { log.Info("Skipping update: Previous object retrieval from the API server failed") - return false, "", false, false + return false, "", "", false, false } var res dynamic.ResourceInterface @@ -2570,18 +2615,14 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( obj.desiredObj, obj.existingObj, existingObjectCopy, complianceType, mdComplianceType, !r.DryRunSupported, ) if message != "" { - return true, message, true, false + return true, message, "", true, false } + recordDiff := objectT.RecordDiffWithDefault() + if updateNeeded { mismatchLog := "Detected value mismatch" - // Add a configuration breadcrumb for users that might be looking in the logs for a diff - if objectT.RecordDiff != policyv1.RecordDiffLog { - mismatchLog += " (Diff disabled. To log the diff, " + - "set 'spec.object-tempates[].recordDiff' to 'Log' for this object-template.)" - } - log.Info(mismatchLog) // FieldValidation is supported in k8s 1.25 as beta release @@ -2590,10 +2631,12 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( if err := r.validateObject(obj.existingObj); err != nil { message := fmt.Sprintf("Error validating the object %s, the error is `%v`", obj.name, err) - return true, message, updateNeeded, false + return true, message, "", updateNeeded, false } } + isInform := remediation.IsInform() + // If the cluster supports dry run requests, verify that the API server agrees with the local comparison logic. // It's possible the dry run request shows the object does match. This can happen if the ConfigurationPolicy // specifies an empty map and the API server omits it from the return value. @@ -2610,7 +2653,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( r.setEvaluatedObject(obj.policy, obj.existingObj, false) - return true, "", false, false + return true, "", "", false, false } // If it's a conflict, refetch the object and try again. @@ -2634,7 +2677,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( ) } - return true, message, updateNeeded, false + return true, message, "", updateNeeded, false } removeFieldsForComparison(dryRunUpdatedObj) @@ -2647,36 +2690,23 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( r.setEvaluatedObject(obj.policy, obj.existingObj, true) - return false, "", false, false + 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) - } - } - } else if objectT.RecordDiff == policyv1.RecordDiffLog { + diff = handleDiff(log, recordDiff, isInform, existingObjectCopy, dryRunUpdatedObj) + } else if recordDiff == policyv1.RecordDiffLog || (isInform && recordDiff == policyv1.RecordDiffInStatus) { // Generate and log the diff for when dryrun is unsupported (i.e. OCP v3.11) mergedObjCopy := obj.existingObj.DeepCopy() removeFieldsForComparison(mergedObjCopy) - 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) - } + diff = handleDiff(log, recordDiff, isInform, existingObjectCopy, mergedObjCopy) } // The object would have been updated, so if it's inform, return as noncompliant. - if remediation.IsInform() { + if isInform { r.setEvaluatedObject(obj.policy, obj.existingObj, false) - return true, "", false, false + return true, "", diff, false, false } // If it's not inform (i.e. enforce), update the object @@ -2702,7 +2732,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( message = fmt.Sprintf("Error updating the object `%v`, the error is `%v`", obj.name, err) } - return true, message, updateNeeded, false + return true, message, diff, updateNeeded, false } if !statusMismatch { @@ -2711,10 +2741,62 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( updateSucceeded = true } else { + if throwSpecViolation && recordDiff != policyv1.RecordDiffNone { + // The spec didn't require a change but throwSpecViolation indicates the status didn't match. Handle + // this diff for this case. + mergedObjCopy := obj.existingObj.DeepCopy() + removeFieldsForComparison(mergedObjCopy) + + // The provided isInform value is always true because the status checking can only be inform. + diff = handleDiff(log, recordDiff, true, existingObjectCopy, mergedObjCopy) + } + r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation) } - return throwSpecViolation, "", updateNeeded, updateSucceeded + return throwSpecViolation, "", diff, updateNeeded, updateSucceeded +} + +// handleDiff will generate the diff and then log it or return it based on the input recordDiff value. If recordDiff +// is set to None or is set to InStatus with enforce, no diff is generated. This is because the diff is not relevant +// after the object is updated. When recordDiff is set to Censored, a message indicating so is returned. +func handleDiff( + log logr.Logger, + recordDiff policyv1.RecordDiff, + isInform bool, + existingObject *unstructured.Unstructured, + mergedObject *unstructured.Unstructured, +) string { + if !isInform && recordDiff == policyv1.RecordDiffInStatus { + return "" + } + + var computedDiff string + + if recordDiff != policyv1.RecordDiffNone && recordDiff != policyv1.RecordDiffCensored { + var err error + + computedDiff, err = generateDiff(existingObject, mergedObject) + if err != nil { + log.Error(err, "Failed to generate the diff") + + return "" + } + } + + switch recordDiff { + case policyv1.RecordDiffNone: + return "" + case policyv1.RecordDiffLog: + log.Info("Logging the diff:\n" + computedDiff) + case policyv1.RecordDiffInStatus: + return computedDiff + case policyv1.RecordDiffCensored: + return `# This diff may contain sensitive data. The "recordDiff" field must be set to "InStatus" ` + + `to record a diff.` + } + + return "" } // handleKeys goes through all of the fields in the desired object and checks if the existing object diff --git a/controllers/configurationpolicy_utils.go b/controllers/configurationpolicy_utils.go index d6c64749..1bc89bcc 100644 --- a/controllers/configurationpolicy_utils.go +++ b/controllers/configurationpolicy_utils.go @@ -132,7 +132,8 @@ func updateRelatedObjectsStatus( currentObject.Object.Metadata.Namespace == relatedObject.Object.Metadata.Namespace { present = true - if currentObject.Compliant != relatedObject.Compliant { + if currentObject.Compliant != relatedObject.Compliant || + !reflect.DeepEqual(currentObject.Properties, relatedObject.Properties) { list[index] = relatedObject } } @@ -687,9 +688,10 @@ func generateDiff(existingObj, updatedObj *unstructured.Unstructured) (string, e return "", fmt.Errorf("failed to marshal existing object to YAML for diff: %w", err) } - existingYAMLName := existingObj.GetName() + " : existing" + name := existingObj.GetName() + if existingObj.GetNamespace() != "" { - existingYAMLName = existingObj.GetNamespace() + "/" + existingYAMLName + name = existingObj.GetNamespace() + "/" + name } updatedYAML, err := yaml.Marshal(updatedObj.Object) @@ -697,19 +699,14 @@ func generateDiff(existingObj, updatedObj *unstructured.Unstructured) (string, e 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, + FromFile: name + " : existing", B: difflib.SplitLines(string(updatedYAML)), - ToFile: updatedYAMLName, - Context: 1, + ToFile: name + " : updated", + Context: 5, } // Generate and return the diff @@ -718,5 +715,13 @@ func generateDiff(existingObj, updatedObj *unstructured.Unstructured) (string, e return "", fmt.Errorf("failed to generate diff: %w", err) } + splitDiff := strings.Split(diff, "\n") + // Keep a maxmium of 50 lines of diff + 3 lines for the header + if len(splitDiff) > 53 { + diff = fmt.Sprintf( + "# Truncated: showing 50/%d diff lines:\n%s", len(splitDiff), strings.Join(splitDiff[:53], "\n"), + ) + } + return diff, nil } diff --git a/controllers/configurationpolicy_utils_test.go b/controllers/configurationpolicy_utils_test.go index 571941b7..210738f5 100644 --- a/controllers/configurationpolicy_utils_test.go +++ b/controllers/configurationpolicy_utils_test.go @@ -256,7 +256,8 @@ func TestGenerateDiff(t *testing.T) { }, }, expectedDiff: ` -@@ -2,2 +2,3 @@ +@@ -1,3 +1,4 @@ + cities: - Raleigh +- Durham`, }, @@ -273,7 +274,8 @@ func TestGenerateDiff(t *testing.T) { }, }, expectedDiff: ` -@@ -2,3 +2,2 @@ +@@ -1,4 +1,3 @@ + cities: - Raleigh -- Durham`, }, 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 fa4c0edf..548f7575 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 @@ -166,9 +166,11 @@ spec: 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". + cluster and the objectDefinition in the policy. Defaults to "None" when the object kind is + ConfigMap, OAuthAccessToken, OAuthAuthorizeTokens, Route, or Secret. Defaults to "InStatus" otherwise. enum: - Log + - InStatus - None type: string required: @@ -318,6 +320,8 @@ spec: description: Whether the object was created by the parent policy type: boolean + diff: + type: string uid: description: Store object UID to help track object ownership for deletion diff --git a/deploy/crds/kustomize_operatorpolicy/kustomization.yaml b/deploy/crds/kustomize_operatorpolicy/kustomization.yaml index e836fde8..64eef583 100644 --- a/deploy/crds/kustomize_operatorpolicy/kustomization.yaml +++ b/deploy/crds/kustomize_operatorpolicy/kustomization.yaml @@ -15,3 +15,9 @@ patches: version: v1 kind: CustomResourceDefinition name: operatorpolicies.policy.open-cluster-management.io +- path: remove-diff.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 02cc5890..e57c28b6 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 @@ -261,6 +261,8 @@ spec: description: Whether the object was created by the parent policy type: boolean + diff: + type: string uid: description: Store object UID to help track object ownership for deletion diff --git a/deploy/crds/kustomize_operatorpolicy/remove-diff.json b/deploy/crds/kustomize_operatorpolicy/remove-diff.json new file mode 100644 index 00000000..6ff3b461 --- /dev/null +++ b/deploy/crds/kustomize_operatorpolicy/remove-diff.json @@ -0,0 +1,6 @@ +[ + { + "op": "remove", + "path": "/spec/versions/0/schema/openAPIV3Schema/properties/status/properties/relatedObjects/items/properties/properties/properties/diff" + } +] \ No newline at end of file diff --git a/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml b/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml index 6be90138..6f9a6761 100644 --- a/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml +++ b/deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml @@ -173,9 +173,11 @@ spec: 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". + cluster and the objectDefinition in the policy. Defaults to "None" when the object kind is + ConfigMap, OAuthAccessToken, OAuthAuthorizeTokens, Route, or Secret. Defaults to "InStatus" otherwise. enum: - Log + - InStatus - None type: string required: @@ -325,6 +327,8 @@ spec: description: Whether the object was created by the parent policy type: boolean + diff: + type: string uid: description: Store object UID to help track object ownership for deletion diff --git a/go.mod b/go.mod index 32231bb1..31acfa2c 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.21 require ( github.com/ghodss/yaml v1.0.1-0.20190212211648-25d852aebe32 + github.com/go-logr/logr v1.4.1 github.com/go-logr/zapr v1.3.0 github.com/google/go-cmp v0.6.0 github.com/onsi/ginkgo/v2 v2.17.1 @@ -13,7 +14,7 @@ require ( github.com/prometheus/client_golang v1.18.0 github.com/spf13/pflag v1.0.5 github.com/stolostron/go-log-utils v0.1.2 - github.com/stolostron/go-template-utils/v4 v4.1.0 + github.com/stolostron/go-template-utils/v4 v4.2.0 github.com/stolostron/kubernetes-dependency-watches v0.6.0 github.com/stretchr/testify v1.8.4 golang.org/x/mod v0.16.0 @@ -44,7 +45,6 @@ require ( github.com/exponent-io/jsonpath v0.0.0-20210407135951-1de76d718b3f // indirect github.com/fsnotify/fsnotify v1.7.0 // indirect github.com/go-errors/errors v1.5.1 // indirect - github.com/go-logr/logr v1.4.1 // indirect github.com/go-openapi/jsonpointer v0.20.0 // indirect github.com/go-openapi/jsonreference v0.20.2 // indirect github.com/go-openapi/swag v0.22.4 // indirect diff --git a/go.sum b/go.sum index d460ad9b..27a87138 100644 --- a/go.sum +++ b/go.sum @@ -142,8 +142,8 @@ github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA= github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= github.com/stolostron/go-log-utils v0.1.2 h1:7l1aJWvBqU2+DUyimcslT5SJpdygVY/clRDmX5sO29c= github.com/stolostron/go-log-utils v0.1.2/go.mod h1:8zrB8UJmp1rXhv3Ck9bBl5SpNfKk3SApeElbo96YRtQ= -github.com/stolostron/go-template-utils/v4 v4.1.0 h1:i7S6gTmFixZB25zY03pR5rC30PVJGDFfFuLi15Oqq6g= -github.com/stolostron/go-template-utils/v4 v4.1.0/go.mod h1:pEezMPxvuvllc68oLUejh9tfM8r/Yi7s10ZlZhVkyM4= +github.com/stolostron/go-template-utils/v4 v4.2.0 h1:HayLPKiSu3wGHjyPsplWKyEVTJ9Ju7pdzzI0/SsSPiA= +github.com/stolostron/go-template-utils/v4 v4.2.0/go.mod h1:pEezMPxvuvllc68oLUejh9tfM8r/Yi7s10ZlZhVkyM4= github.com/stolostron/kubernetes-dependency-watches v0.6.0 h1:FxXsslLNH5hNojVvKX467iefCEwigdMftfmlYX/tddg= github.com/stolostron/kubernetes-dependency-watches v0.6.0/go.mod h1:6v54aX8Bxx1m9YETZUxNGUrSHcRIArM/YnOqnUbIB/g= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= diff --git a/test/e2e/case20_delete_objects_test.go b/test/e2e/case20_delete_objects_test.go index 0f6d50a2..077a9461 100644 --- a/test/e2e/case20_delete_objects_test.go +++ b/test/e2e/case20_delete_objects_test.go @@ -70,7 +70,7 @@ var _ = Describe("Test Object deletion", Ordered, func() { properties := relatedObj.(map[string]interface{})["properties"].(map[string]interface{}) return properties["uid"].(string) - }, defaultTimeoutSeconds, 1).Should(Not(Equal(""))) + }, defaultTimeoutSeconds, 1).ShouldNot(BeEmpty()) }) It("should update status fields properly for non-created objects", func() { By("Creating " + case20ConfigPolicyNameExisting + " on managed") @@ -99,7 +99,7 @@ var _ = Describe("Test Object deletion", Ordered, func() { properties := relatedObj.(map[string]interface{})["properties"].(map[string]interface{}) return properties["uid"] - }, defaultTimeoutSeconds, 1).Should(BeNil()) + }, defaultTimeoutSeconds, 1).ShouldNot(BeEmpty()) }) It("should update status fields properly for edited objects", func() { By("Creating " + case20ConfigPolicyNameEdit + " on managed") @@ -128,7 +128,7 @@ var _ = Describe("Test Object deletion", Ordered, func() { properties := relatedObj.(map[string]interface{})["properties"].(map[string]interface{}) return properties["uid"] - }, defaultTimeoutSeconds, 1).Should(BeNil()) + }, defaultTimeoutSeconds, 1).ShouldNot(BeEmpty()) }) It("should not update status field for inform policies", func() { By("Creating " + case20ConfigPolicyNameInform + " on managed") diff --git a/test/e2e/case2_role_handling_test.go b/test/e2e/case2_role_handling_test.go index b843875c..4c73d3a8 100644 --- a/test/e2e/case2_role_handling_test.go +++ b/test/e2e/case2_role_handling_test.go @@ -4,6 +4,8 @@ package e2e import ( + "fmt" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -147,12 +149,41 @@ var _ = Describe("Test role obj template handling", Ordered, func() { plc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, configPolicyNameBindingEnforce, testNamespace, true, defaultTimeoutSeconds) Expect(plc).NotTo(BeNil()) + + var managedPlc *unstructured.Unstructured + Eventually(func() interface{} { - managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, + managedPlc = utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, configPolicyNameBindingEnforce, testNamespace, true, defaultTimeoutSeconds) return utils.GetComplianceState(managedPlc) }, defaultTimeoutSeconds, 1).Should(Equal("NonCompliant")) + + By("Verifying the diff in the status") + relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects") + Expect(err).ToNot(HaveOccurred()) + Expect(relatedObjects).To(HaveLen(1)) + + uid, _, _ := unstructured.NestedString(relatedObjects[0].(map[string]interface{}), "properties", "uid") + Expect(uid).ToNot(BeEmpty()) + + diff, _, _ := unstructured.NestedString(relatedObjects[0].(map[string]interface{}), "properties", "diff") + expectedDiff := fmt.Sprintf(`--- default/pod-reader-e2e-binding : existing ++++ default/pod-reader-e2e-binding : updated +@@ -8,10 +8,6 @@ + uid: %s + roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: pod-reader-e2e +-subjects: +-- apiGroup: rbac.authorization.k8s.io +- kind: Group +- name: system:authenticated:oauth + +`, uid) + Expect(diff).To(Equal(expectedDiff)) + By("patching policy spec.remediationAction = enforce") utils.Kubectl("patch", "configurationpolicy", configPolicyNameBindingEnforce, `--type=json`, `-p=[{"op":"replace","path":"/spec/remediationAction","value":"enforce"}]`, "-n", testNamespace) diff --git a/test/e2e/case39_diff_generation_test.go b/test/e2e/case39_diff_generation_test.go index 353c1b31..c929003a 100644 --- a/test/e2e/case39_diff_generation_test.go +++ b/test/e2e/case39_diff_generation_test.go @@ -11,6 +11,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "open-cluster-management.io/config-policy-controller/test/utils" ) @@ -85,8 +86,12 @@ var _ = Describe("Generate the diff", Ordered, func() { data: - fieldToUpdate: "1" + fieldToUpdate: "2" - kind: ConfigMap - {"policy": "case39-policy-cfgmap-create", "name": "case39-map", "namespace": "default", "resource": "configmaps"}`)) + kind: ConfigMap`)) + + Expect(diff).Should(ContainSubstring( + `{"policy": "case39-policy-cfgmap-create", "name": "case39-map", "namespace": "default", ` + + `"resource": "configmaps"}`, + )) }) AfterAll(func() { @@ -94,3 +99,160 @@ var _ = Describe("Generate the diff", Ordered, func() { utils.Kubectl("delete", "configmap", "case39-map", "--ignore-not-found") }) }) + +var _ = Describe("Diff generation with sensitive input", Ordered, func() { + const ( + noDiffObjTemplatesRaw = "case39-no-diff-object-templates-raw" + noDiffObjTemplatesRawYAML = "../resources/case39_diff_generation/case39-no-diff-object-templates-raw.yaml" + noDiffObjTemplates = "case39-no-diff-object-templates" + noDiffObjTemplatesYAML = "../resources/case39_diff_generation/case39-no-diff-object-templates.yaml" + noDiffOnSecret = "case39-no-diff-on-secret" + noDiffOnSecretYAML = "../resources/case39_diff_generation/case39-no-diff-on-secret.yaml" + secretName = "case39-secret" + ) + + BeforeAll(func() { + By("Creating " + secretName + " in the default namespace") + utils.Kubectl("create", "-f", "../resources/case39_diff_generation/case39-secret.yaml") + }) + + AfterAll(func() { + deleteConfigPolicies([]string{noDiffObjTemplatesRaw, noDiffObjTemplates, noDiffOnSecret}) + utils.Kubectl("-n", "default", "delete", "secret", secretName, "--ignore-not-found") + }) + + It("Does not automatically generate a diff when using fromSecret (object-templates-raw)", func() { + By("Creating " + noDiffObjTemplatesRaw + " on managed") + utils.Kubectl("apply", "-f", noDiffObjTemplatesRawYAML, "-n", testNamespace) + + var managedPlc *unstructured.Unstructured + + Eventually(func() interface{} { + managedPlc = utils.GetWithTimeout( + clientManagedDynamic, + gvrConfigPolicy, + noDiffObjTemplatesRaw, + testNamespace, + true, + defaultTimeoutSeconds, + ) + + return utils.GetComplianceState(managedPlc) + }, defaultTimeoutSeconds, 1).Should(Equal("NonCompliant")) + + By("Verifying the diff in the status contains instructions to set recordDiff") + relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects") + Expect(err).ToNot(HaveOccurred()) + Expect(relatedObjects).To(HaveLen(1)) + + diff, _, _ := unstructured.NestedString(relatedObjects[0].(map[string]interface{}), "properties", "diff") + Expect(diff).To(Equal( + `# This diff may contain sensitive data. The "recordDiff" field must be set to "InStatus" ` + + `to record a diff.`, + )) + }) + + It("Does not automatically generate a diff when using fromSecret (object-templates)", func() { + By("Creating " + noDiffObjTemplates + " on managed") + utils.Kubectl("apply", "-f", noDiffObjTemplatesYAML, "-n", testNamespace) + + var managedPlc *unstructured.Unstructured + + Eventually(func() interface{} { + managedPlc = utils.GetWithTimeout( + clientManagedDynamic, + gvrConfigPolicy, + noDiffObjTemplates, + testNamespace, + true, + defaultTimeoutSeconds, + ) + + return utils.GetComplianceState(managedPlc) + }, defaultTimeoutSeconds, 1).Should(Equal("NonCompliant")) + + By("Verifying the diff in the status contains instructions to set recordDiff") + relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects") + Expect(err).ToNot(HaveOccurred()) + Expect(relatedObjects).To(HaveLen(1)) + + diff, _, _ := unstructured.NestedString(relatedObjects[0].(map[string]interface{}), "properties", "diff") + Expect(diff).To(Equal( + `# This diff may contain sensitive data. The "recordDiff" field must be set to "InStatus" ` + + `to record a diff.`, + )) + }) + + It("Does not automatically generate a diff when configuring a Secret", func() { + By("Creating " + noDiffOnSecret + " on managed") + utils.Kubectl("apply", "-f", noDiffOnSecretYAML, "-n", testNamespace) + + var managedPlc *unstructured.Unstructured + + Eventually(func() interface{} { + managedPlc = utils.GetWithTimeout( + clientManagedDynamic, + gvrConfigPolicy, + noDiffOnSecret, + testNamespace, + true, + defaultTimeoutSeconds, + ) + + return utils.GetComplianceState(managedPlc) + }, defaultTimeoutSeconds, 1).Should(Equal("NonCompliant")) + + By("Verifying the diff in the status contains instructions to set recordDiff") + relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects") + Expect(err).ToNot(HaveOccurred()) + Expect(relatedObjects).To(HaveLen(1)) + + diff, _, _ := unstructured.NestedString(relatedObjects[0].(map[string]interface{}), "properties", "diff") + Expect(diff).To(Equal( + `# This diff may contain sensitive data. The "recordDiff" field must be set to "InStatus" ` + + `to record a diff.`, + )) + }) +}) + +var _ = Describe("Diff generation that is truncated", Ordered, func() { + const ( + policyTruncatedDiff = "case39-truncated" + policyTruncatedDiffYAML = "../resources/case39_diff_generation/case39-truncated.yaml" + ) + + AfterAll(func() { + deleteConfigPolicies([]string{policyTruncatedDiff}) + }) + + It("Does not automatically generate a diff when configuring a Secret", func() { + By("Creating " + policyTruncatedDiff + " on managed") + utils.Kubectl("apply", "-f", policyTruncatedDiffYAML, "-n", testNamespace) + + var managedPlc *unstructured.Unstructured + + Eventually(func() interface{} { + managedPlc = utils.GetWithTimeout( + clientManagedDynamic, + gvrConfigPolicy, + policyTruncatedDiff, + testNamespace, + true, + defaultTimeoutSeconds, + ) + + return utils.GetComplianceState(managedPlc) + }, defaultTimeoutSeconds, 1).Should(Equal("NonCompliant")) + + By("Verifying the diff in the status is truncated") + relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects") + Expect(err).ToNot(HaveOccurred()) + Expect(relatedObjects).To(HaveLen(1)) + + diff, _, _ := unstructured.NestedString(relatedObjects[0].(map[string]interface{}), "properties", "diff") + Expect(diff).To(HavePrefix( + "# Truncated: showing 50/68 diff lines:\n--- default : existing\n+++ default : updated", + )) + Expect(diff).To(HaveSuffix("+ message46: message")) + }) +}) diff --git a/test/e2e/case8_status_check_test.go b/test/e2e/case8_status_check_test.go index bcf4c7e5..cb7e966b 100644 --- a/test/e2e/case8_status_check_test.go +++ b/test/e2e/case8_status_check_test.go @@ -6,6 +6,7 @@ package e2e import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "open-cluster-management.io/config-policy-controller/test/utils" ) @@ -58,12 +59,22 @@ var _ = Describe("Test pod obj template handling", func() { plc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, case8ConfigPolicyNameCheckFail, testNamespace, true, defaultTimeoutSeconds) Expect(plc).NotTo(BeNil()) + + var managedPlc *unstructured.Unstructured + Eventually(func() interface{} { - managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, + managedPlc = utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, case8ConfigPolicyNameCheckFail, testNamespace, true, defaultTimeoutSeconds) return utils.GetComplianceState(managedPlc) }, defaultTimeoutSeconds, 1).Should(Equal("NonCompliant")) + + relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects") + Expect(err).ToNot(HaveOccurred()) + Expect(relatedObjects).To(HaveLen(1)) + + diff, _, _ := unstructured.NestedString(relatedObjects[0].(map[string]interface{}), "properties", "diff") + Expect(diff).To(ContainSubstring("- compliant: Compliant\n+ compliant: NonCompliant")) }) It("should return nonCompliant if status does not match (enforce)", func() { By("Creating " + case8ConfigPolicyNameEnforceFail + " on managed") @@ -71,12 +82,22 @@ var _ = Describe("Test pod obj template handling", func() { plc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, case8ConfigPolicyNameEnforceFail, testNamespace, true, defaultTimeoutSeconds) Expect(plc).NotTo(BeNil()) + + var managedPlc *unstructured.Unstructured + Eventually(func() interface{} { - managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, + managedPlc = utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, case8ConfigPolicyNameEnforceFail, testNamespace, true, defaultTimeoutSeconds) return utils.GetComplianceState(managedPlc) }, defaultTimeoutSeconds, 1).Should(Equal("NonCompliant")) + + relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects") + Expect(err).ToNot(HaveOccurred()) + Expect(relatedObjects).To(HaveLen(1)) + + diff, _, _ := unstructured.NestedString(relatedObjects[0].(map[string]interface{}), "properties", "diff") + Expect(diff).To(ContainSubstring("- compliant: Compliant\n+ compliant: NonCompliant")) }) AfterAll(func() { policies := []string{ diff --git a/test/resources/case39_diff_generation/case39-no-diff-object-templates-raw.yaml b/test/resources/case39_diff_generation/case39-no-diff-object-templates-raw.yaml new file mode 100644 index 00000000..04573788 --- /dev/null +++ b/test/resources/case39_diff_generation/case39-no-diff-object-templates-raw.yaml @@ -0,0 +1,15 @@ +apiVersion: policy.open-cluster-management.io/v1 +kind: ConfigurationPolicy +metadata: + name: case39-no-diff-object-templates-raw +spec: + remediationAction: inform + object-templates-raw: | + - complianceType: musthave + objectDefinition: + apiVersion: v1 + kind: Namespace + metadata: + name: default + annotations: + message: '{{ fromSecret "default" "case39-secret" "message" }}' diff --git a/test/resources/case39_diff_generation/case39-no-diff-object-templates.yaml b/test/resources/case39_diff_generation/case39-no-diff-object-templates.yaml new file mode 100644 index 00000000..ef806fb2 --- /dev/null +++ b/test/resources/case39_diff_generation/case39-no-diff-object-templates.yaml @@ -0,0 +1,15 @@ +apiVersion: policy.open-cluster-management.io/v1 +kind: ConfigurationPolicy +metadata: + name: case39-no-diff-object-templates +spec: + remediationAction: inform + object-templates: + - complianceType: musthave + objectDefinition: + apiVersion: v1 + kind: Namespace + metadata: + name: default + annotations: + message: '{{ fromSecret "default" "case39-secret" "message" }}' diff --git a/test/resources/case39_diff_generation/case39-no-diff-on-secret.yaml b/test/resources/case39_diff_generation/case39-no-diff-on-secret.yaml new file mode 100644 index 00000000..14f13a48 --- /dev/null +++ b/test/resources/case39_diff_generation/case39-no-diff-on-secret.yaml @@ -0,0 +1,17 @@ +apiVersion: policy.open-cluster-management.io/v1 +kind: ConfigurationPolicy +metadata: + name: case39-no-diff-on-secret +spec: + remediationAction: inform + object-templates: + - complianceType: musthave + objectDefinition: + apiVersion: v1 + kind: Secret + metadata: + name: case39-secret + namespace: default + annotations: + riddle: "If I have it, I don't share it. If I share it, I don't have it. What am I?" + hint: "See the value of the kind field" diff --git a/test/resources/case39_diff_generation/case39-secret.yaml b/test/resources/case39_diff_generation/case39-secret.yaml new file mode 100644 index 00000000..3bdadd28 --- /dev/null +++ b/test/resources/case39_diff_generation/case39-secret.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: Secret +metadata: + name: case39-secret + namespace: default +type: Opaque +data: + message: VGhlcmUncyBhIHNuYWtlIGluIG15IGJvb3Qh diff --git a/test/resources/case39_diff_generation/case39-truncated.yaml b/test/resources/case39_diff_generation/case39-truncated.yaml new file mode 100644 index 00000000..b0c98965 --- /dev/null +++ b/test/resources/case39_diff_generation/case39-truncated.yaml @@ -0,0 +1,69 @@ +apiVersion: policy.open-cluster-management.io/v1 +kind: ConfigurationPolicy +metadata: + name: case39-truncated +spec: + remediationAction: inform + object-templates: + - complianceType: musthave + objectDefinition: + apiVersion: v1 + kind: Namespace + metadata: + name: default + annotations: + message1: message + message2: message + message3: message + message4: message + message5: message + message6: message + message7: message + message8: message + message9: message + message10: message + message11: message + message12: message + message13: message + message14: message + message15: message + message16: message + message17: message + message18: message + message19: message + message20: message + message21: message + message22: message + message23: message + message24: message + message25: message + message26: message + message27: message + message28: message + message29: message + message30: message + message31: message + message32: message + message33: message + message34: message + message35: message + message36: message + message37: message + message38: message + message39: message + message40: message + message41: message + message42: message + message43: message + message44: message + message45: message + message46: message + message47: message + message48: message + message49: message + message50: message + message51: message + message52: message + message53: message + message54: message + message55: message