diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index 56574138..57e4626f 100644 --- a/controllers/configurationpolicy_controller.go +++ b/controllers/configurationpolicy_controller.go @@ -2586,72 +2586,28 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( res = r.TargetK8sDynamicClient.Resource(obj.gvr) } - updateSucceeded = false // Use a copy since some values can be directly assigned to mergedObj in handleSingleKey. existingObjectCopy := obj.existingObj.DeepCopy() removeFieldsForComparison(existingObjectCopy) - var statusMismatch bool - - for key := range obj.desiredObj.Object { - isStatus := key == "status" - - // use metadatacompliancetype to evaluate metadata if it is set - keyComplianceType := complianceType - if key == "metadata" && mdComplianceType != "" { - keyComplianceType = mdComplianceType - } - - // check key for mismatch - errorMsg, keyUpdateNeeded, mergedObj, skipped := handleSingleKey( - key, obj.desiredObj, existingObjectCopy, keyComplianceType, !r.DryRunSupported, - ) - if errorMsg != "" { - log.Info(errorMsg) - - return true, errorMsg, true, false - } - - if mergedObj == nil && skipped { - continue - } - - // only look at labels and annotations for metadata - configurationPolicies do not update other metadata fields - if key == "metadata" { - // if it's not the right type, the map will be empty - mdMap, _ := mergedObj.(map[string]interface{}) + throwSpecViolation, message, updateNeeded, statusMismatch := handleKeys( + obj.desiredObj, obj.existingObj, existingObjectCopy, complianceType, mdComplianceType, !r.DryRunSupported, + ) + if message != "" { + return true, message, true, false + } - // if either isn't found, they'll just be empty - mergedAnnotations, _, _ := unstructured.NestedStringMap(mdMap, "annotations") - mergedLabels, _, _ := unstructured.NestedStringMap(mdMap, "labels") + if updateNeeded { + mismatchLog := "Detected value mismatch" - obj.existingObj.SetAnnotations(mergedAnnotations) - obj.existingObj.SetLabels(mergedLabels) - } else { - obj.existingObj.UnstructuredContent()[key] = mergedObj + // 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.)" } - if keyUpdateNeeded { - if isStatus { - throwSpecViolation = true - statusMismatch = true - - log.Info("Ignoring an update to the object status", "key", key) - } else { - updateNeeded = true + log.Info(mismatchLog) - mismatchLog := "Detected value mismatch for object key: " + key - // 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) - } - } - } - - if updateNeeded { // 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 { @@ -2783,6 +2739,68 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( return throwSpecViolation, "", updateNeeded, updateSucceeded } +// handleKeys goes through all of the fields in the desired object and checks if the existing object +// matches. When a field is a map or slice, the value in the existing object will be updated with +// the result of merging its current value with the desired value. +func handleKeys( + desiredObj unstructured.Unstructured, + existingObj *unstructured.Unstructured, + existingObjectCopy *unstructured.Unstructured, + compType string, + mdCompType string, + zeroValueEqualsNil bool, +) (throwSpecViolation bool, message string, updateNeeded bool, statusMismatch bool) { + for key := range desiredObj.Object { + isStatus := key == "status" + + // use metadatacompliancetype to evaluate metadata if it is set + keyComplianceType := compType + if key == "metadata" && mdCompType != "" { + keyComplianceType = mdCompType + } + + // check key for mismatch + errorMsg, keyUpdateNeeded, mergedObj, skipped := handleSingleKey( + key, desiredObj, existingObjectCopy, keyComplianceType, zeroValueEqualsNil, + ) + if errorMsg != "" { + log.Info(errorMsg) + + return true, errorMsg, true, statusMismatch + } + + if mergedObj == nil && skipped { + continue + } + + // only look at labels and annotations for metadata - configurationPolicies do not update other metadata fields + if key == "metadata" { + // if it's not the right type, the map will be empty + mdMap, _ := mergedObj.(map[string]interface{}) + + // if either isn't found, they'll just be empty + mergedAnnotations, _, _ := unstructured.NestedStringMap(mdMap, "annotations") + mergedLabels, _, _ := unstructured.NestedStringMap(mdMap, "labels") + + existingObj.SetAnnotations(mergedAnnotations) + existingObj.SetLabels(mergedLabels) + } else { + existingObj.UnstructuredContent()[key] = mergedObj + } + + if keyUpdateNeeded { + if isStatus { + throwSpecViolation = true + statusMismatch = true + } else { + updateNeeded = true + } + } + } + + return +} + func removeFieldsForComparison(obj *unstructured.Unstructured) { unstructured.RemoveNestedField(obj.Object, "metadata", "managedFields") unstructured.RemoveNestedField( diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go index 051ec999..6a2dda8b 100644 --- a/controllers/operatorpolicy_controller.go +++ b/controllers/operatorpolicy_controller.go @@ -6,6 +6,7 @@ package controllers import ( "context" "encoding/json" + "errors" "fmt" "reflect" @@ -14,6 +15,7 @@ import ( depclient "github.com/stolostron/kubernetes-dependency-watches/client" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -173,22 +175,24 @@ func (r *OperatorPolicyReconciler) handleOpGroup(ctx context.Context, policy *po case 1: opGroup := foundOpGroups[0] - if policy.Spec.OperatorGroup == nil { - // There is one operator group in the namespace, but the policy doesn't specify what it should look like. - // FUTURE: check if the one operator group is compatible with the desired subscription. - // For an initial implementation, assume if an OperatorGroup already exists, then it's a good one. - err := r.updateStatus(ctx, policy, opGroupPreexistingCond, matchedObj(&opGroup)) - if err != nil { - return fmt.Errorf("error updating the status for an OperatorGroup that matches: %w", err) - } + // Check if what's on the cluster matches what the policy wants (whether it's specified or not) - return nil - } - - // OperatorGroup is specified in the policy, check if what's on the cluster matches. emptyNameMatch := desiredOpGroup.Name == "" && opGroup.GetGenerateName() == desiredOpGroup.GenerateName if !(opGroup.GetName() == desiredOpGroup.Name || emptyNameMatch) { + if policy.Spec.OperatorGroup == nil { + // The policy doesn't specify what the OperatorGroup should look like, but what is already + // there is not the default one the policy would create. + // FUTURE: check if the one operator group is compatible with the desired subscription. + // For an initial implementation, assume if an OperatorGroup already exists, then it's a good one. + err := r.updateStatus(ctx, policy, opGroupPreexistingCond, matchedObj(&opGroup)) + if err != nil { + return fmt.Errorf("error updating the status for a pre-existing OperatorGroup: %w", err) + } + + return nil + } + // There is an OperatorGroup in the namespace that does not match the name of what is in the policy. // Just creating a new one would cause the "TooManyOperatorGroups" failure. // So, just report a NonCompliant status. @@ -209,9 +213,14 @@ func (r *OperatorPolicyReconciler) handleOpGroup(ctx context.Context, policy *po return fmt.Errorf("error converting desired OperatorGroup to an Unstructured: %w", err) } - // NOTE: this is more like mustonlyhave than musthave, and won't quite work. - // something from config-policy will be needed to check if the specs match. - if reflect.DeepEqual(desiredUnstruct["spec"], opGroup.Object["spec"]) { + updateNeeded, skipUpdate, err := r.mergeObjects( + ctx, desiredUnstruct, &opGroup, string(policy.Spec.ComplianceType), + ) + if err != nil { + return fmt.Errorf("error checking if the OperatorGroup needs an update: %w", err) + } + + if !updateNeeded { // Everything relevant matches! err := r.updateStatus(ctx, policy, matchesCond("OperatorGroup"), matchedObj(&opGroup)) if err != nil { @@ -221,6 +230,30 @@ func (r *OperatorPolicyReconciler) handleOpGroup(ctx context.Context, policy *po return nil } + // Specs don't match. + + if policy.Spec.OperatorGroup == nil { + // The policy doesn't specify what the OperatorGroup should look like, but what is already + // there is not the default one the policy would create. + // FUTURE: check if the one operator group is compatible with the desired subscription. + // For an initial implementation, assume if an OperatorGroup already exists, then it's a good one. + err := r.updateStatus(ctx, policy, opGroupPreexistingCond, matchedObj(&opGroup)) + if err != nil { + return fmt.Errorf("error updating the status for a pre-existing OperatorGroup: %w", err) + } + + return nil + } + + if policy.Spec.RemediationAction.IsEnforce() && skipUpdate { + err = r.updateStatus(ctx, policy, mismatchCondUnfixable("OperatorGroup"), mismatchedObj(&opGroup)) + if err != nil { + return fmt.Errorf("error updating status for an unenforceable mismatched OperatorGroup: %w", err) + } + + return nil + } + // The names match, but the specs don't: report NonCompliance err = r.updateStatus(ctx, policy, mismatchCond("OperatorGroup"), mismatchedObj(&opGroup)) if err != nil { @@ -367,9 +400,12 @@ func (r *OperatorPolicyReconciler) handleSubscription(ctx context.Context, polic return fmt.Errorf("error converting desired Subscription to an Unstructured: %w", err) } - // NOTE: this is more like mustonlyhave than musthave, and won't quite work. - // something from config-policy will be needed to check if the specs match. - if reflect.DeepEqual(desiredUnstruct["spec"], foundSub.Object["spec"]) { + updateNeeded, skipUpdate, err := r.mergeObjects(ctx, desiredUnstruct, foundSub, string(policy.Spec.ComplianceType)) + if err != nil { + return fmt.Errorf("error checking if the Subscription needs an update: %w", err) + } + + if !updateNeeded { // FUTURE: Check more details about the *status* of the Subscription // For now, just mark it as compliant err := r.updateStatus(ctx, policy, matchesCond("Subscription"), matchedObj(foundSub)) @@ -381,15 +417,22 @@ func (r *OperatorPolicyReconciler) handleSubscription(ctx context.Context, polic } // Specs don't match. + if policy.Spec.RemediationAction.IsEnforce() && skipUpdate { + err = r.updateStatus(ctx, policy, mismatchCondUnfixable("Subscription"), mismatchedObj(foundSub)) + if err != nil { + return fmt.Errorf("error updating status for a mismatched Subscription that can't be enforced: %w", err) + } + + return nil + } + err = r.updateStatus(ctx, policy, mismatchCond("Subscription"), mismatchedObj(foundSub)) if err != nil { return fmt.Errorf("error updating status for a mismatched Subscription: %w", err) } if policy.Spec.RemediationAction.IsEnforce() { - desiredSub.ResourceVersion = foundSub.GetResourceVersion() - - err := r.Update(ctx, desiredSub) + err := r.Update(ctx, foundSub) if err != nil { return fmt.Errorf("error updating the Subscription: %w", err) } @@ -448,3 +491,49 @@ func opPolIdentifier(namespace, name string) depclient.ObjectIdentifier { Name: name, } } + +// mergeObjects takes fields from the desired object and sets/merges them on the +// existing object. It checks and returns whether an update is really necessary +// with a server-side dry-run. +func (r *OperatorPolicyReconciler) mergeObjects( + ctx context.Context, + desired map[string]interface{}, + existing *unstructured.Unstructured, + complianceType string, +) (updateNeeded, updateIsForbidden bool, err error) { + desiredObj := unstructured.Unstructured{Object: desired} + + // Use a copy since some values can be directly assigned to mergedObj in handleSingleKey. + existingObjectCopy := existing.DeepCopy() + removeFieldsForComparison(existingObjectCopy) + + _, errMsg, updateNeeded, _ := handleKeys( + desiredObj, existing, existingObjectCopy, complianceType, "", false, + ) + if errMsg != "" { + return updateNeeded, false, errors.New(errMsg) + } + + if updateNeeded { + err := r.Update(ctx, existing, client.DryRunAll) + if err != nil { + if k8serrors.IsForbidden(err) { + // This indicates the update would make a change, but the change is not allowed, + // for example, the changed field might be immutable. + // The policy should be marked as noncompliant, but an enforcement update would fail. + return true, true, nil + } + + return updateNeeded, false, err + } + + removeFieldsForComparison(existing) + + if reflect.DeepEqual(existing.Object, existingObjectCopy.Object) { + // The dry run indicates that there is not *really* a mismatch. + updateNeeded = false + } + } + + return updateNeeded, false, nil +} diff --git a/controllers/operatorpolicy_status.go b/controllers/operatorpolicy_status.go index c1d43d19..e89372c5 100644 --- a/controllers/operatorpolicy_status.go +++ b/controllers/operatorpolicy_status.go @@ -334,6 +334,17 @@ func mismatchCond(kind string) metav1.Condition { } } +// mismatchCondUnfixable returns a NonCompliant condition with a Reason like '____Mismatch', +// and a Message like 'the ____ found on the cluster does not match the policy and can't be enforced' +func mismatchCondUnfixable(kind string) metav1.Condition { + return metav1.Condition{ + Type: condType(kind), + Status: metav1.ConditionFalse, + Reason: kind + "Mismatch", + Message: "the " + kind + " found on the cluster does not match the policy and can't be enforced", + } +} + // updatedCond returns a Compliant condition, with a Reason like'____Updated', // and a Message like 'the ____ was updated to match the policy' func updatedCond(kind string) metav1.Condition { diff --git a/test/e2e/case38_install_operator_test.go b/test/e2e/case38_install_operator_test.go index 29f67b65..9b863b50 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/test/e2e/case38_install_operator_test.go @@ -175,8 +175,11 @@ var _ = Describe("Test installing an operator from OperatorPolicy", Ordered, fun "there is more than one OperatorGroup in the namespace", ) }) - It("Should match the OperatorGroup when the extra one is deleted", func() { - utils.Kubectl("delete", "operatorgroup", extraOpGroupName, "-n", opPolTestNS) + It("Should warn about the OperatorGroup when it doesn't match the default", func() { + utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p", + `[{"op": "replace", "path": "/spec/remediationAction", "value": "inform"}]`) + utils.Kubectl("delete", "operatorgroup", "-n", opPolTestNS, "--all") + utils.Kubectl("apply", "-f", extraOpGroupYAML, "-n", opPolTestNS) check( opPolName, false, @@ -189,12 +192,13 @@ var _ = Describe("Test installing an operator from OperatorPolicy", Ordered, fun Reason: "Resource found as expected", }}, metav1.Condition{ - Type: "OperatorGroupCompliant", - Status: metav1.ConditionTrue, - Reason: "OperatorGroupMatches", - Message: "the OperatorGroup matches what is required by the policy", + Type: "OperatorGroupCompliant", + Status: metav1.ConditionTrue, + Reason: "PreexistingOperatorGroupFound", + Message: "the policy does not specify an OperatorGroup but one already exists in the namespace" + + " - assuming that OperatorGroup is correct", }, - "the OperatorGroup matches what is required by the policy", + "assuming that OperatorGroup is correct", ) }) }) @@ -401,7 +405,7 @@ var _ = Describe("Test installing an operator from OperatorPolicy", Ordered, fun metav1.Condition{ Type: "SubscriptionCompliant", Status: metav1.ConditionTrue, - Reason: "SubscriptionCreated", + Reason: "SubscriptionMatches", Message: "the Subscription matches what is required by the policy", }, "the Subscription required by the policy was created",