diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index e71b0e2b..ace4336c 100644 --- a/controllers/configurationpolicy_controller.go +++ b/controllers/configurationpolicy_controller.go @@ -506,10 +506,37 @@ func (r *ConfigurationPolicyReconciler) getObjectTemplateDetails( return templateObjs, selectedNamespaces, false, nil } -func (r *ConfigurationPolicyReconciler) cleanUpChildObjects(plc policyv1.ConfigurationPolicy) []string { +func (r *ConfigurationPolicyReconciler) cleanUpChildObjects(plc policyv1.ConfigurationPolicy, + newRelated []policyv1.RelatedObject, +) []string { deletionFailures := []string{} - for _, object := range plc.Status.RelatedObjects { + if !strings.EqualFold(string(plc.Spec.RemediationAction), "enforce") { + return deletionFailures + } + + // PruneObjectBehavior = none case fall in here + if !(string(plc.Spec.PruneObjectBehavior) == "DeleteAll" || + string(plc.Spec.PruneObjectBehavior) == "DeleteIfCreated") { + return deletionFailures + } + + objsToDelete := plc.Status.RelatedObjects + + // When spec is updated and new related objects are created + if len(newRelated) != 0 { + var objShouldRemoved []policyv1.RelatedObject + + for _, oldR := range objsToDelete { + if !containRelated(newRelated, oldR) { + objShouldRemoved = append(objShouldRemoved, oldR) + } + } + + objsToDelete = objShouldRemoved + } + + for _, object := range objsToDelete { // set up client for object deletion gvk := schema.FromAPIVersionAndKind(object.Object.APIVersion, object.Object.Kind) @@ -555,22 +582,20 @@ func (r *ConfigurationPolicyReconciler) cleanUpChildObjects(plc policyv1.Configu continue } - if strings.EqualFold(string(plc.Spec.RemediationAction), "enforce") { - if string(plc.Spec.PruneObjectBehavior) == "DeleteAll" { + if string(plc.Spec.PruneObjectBehavior) == "DeleteAll" { + needsDelete = true + } else if string(plc.Spec.PruneObjectBehavior) == "DeleteIfCreated" { + // if prune behavior is DeleteIfCreated, we need to check whether createdByPolicy + // is true and the UID is not stale + uid, uidFound, err := unstructured.NestedString(existing.Object, "metadata", "uid") + + if !uidFound || err != nil { + log.Error(err, "Tried to pull UID from obj but the field did not exist or was not a string") + } else if object.Properties != nil && + object.Properties.CreatedByPolicy != nil && + *object.Properties.CreatedByPolicy && + object.Properties.UID == uid { needsDelete = true - } else { - // if prune behavior is DeleteIfCreated, we need to check whether createdByPolicy - // is true and the UID is not stale - uid, uidFound, err := unstructured.NestedString(existing.Object, "metadata", "uid") - - if !uidFound || err != nil { - log.Error(err, "Tried to pull UID from obj but the field did not exist or was not a string") - } else if object.Properties != nil && - object.Properties.CreatedByPolicy != nil && - *object.Properties.CreatedByPolicy && - object.Properties.UID == uid { - needsDelete = true - } } } @@ -618,7 +643,7 @@ func (r *ConfigurationPolicyReconciler) cleanUpChildObjects(plc policyv1.Configu continue } - log.Info("Object successfully deleted as part of child object pruning") + log.Info("Object successfully deleted as part of child object pruning or detached objects") } } } @@ -767,7 +792,7 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi if plc.ObjectMeta.DeletionTimestamp != nil { log.V(1).Info("Config policy has been deleted, handling child objects") - failures := r.cleanUpChildObjects(plc) + failures := r.cleanUpChildObjects(plc, nil) if len(failures) == 0 { log.V(1).Info("Objects have been successfully cleaned up, removing finalizer") @@ -1302,39 +1327,14 @@ func (r *ConfigurationPolicyReconciler) sortRelatedObjectsAndUpdate( } if !gocmp.Equal(related, oldRelated) { - // When it is hub or managed template parse error, it should not remove previous objects if deleteDetachedObjs { - r.deleteDetachedObj(*plc, related, oldRelated) + r.cleanUpChildObjects(*plc, related) } plc.Status.RelatedObjects = related } } -// helper function to delete unconnected objs -func (r *ConfigurationPolicyReconciler) deleteDetachedObj(plc policyv1.ConfigurationPolicy, - related, oldRelated []policyv1.RelatedObject, -) []policyv1.RelatedObject { - objShouldRemoved := []policyv1.RelatedObject{} - // Pick out only obj should be removed in oldRelated - for _, oldR := range oldRelated { - isContain := containRelated(related, oldR) - - if !isContain { - objShouldRemoved = append(objShouldRemoved, oldR) - } - } - - plc.Status.RelatedObjects = objShouldRemoved - - // removed objs which are not related(detached) anymore - if r != nil { - r.cleanUpChildObjects(plc) - } - // For now this is for unit test - return objShouldRemoved -} - // helper function that appends a condition (violation or compliant) to the status of a configurationpolicy // Set the index to -1 to signal that the status should be cleared. func addConditionToStatus( diff --git a/controllers/configurationpolicy_controller_test.go b/controllers/configurationpolicy_controller_test.go index 94d8668d..05c813f1 100644 --- a/controllers/configurationpolicy_controller_test.go +++ b/controllers/configurationpolicy_controller_test.go @@ -852,384 +852,3 @@ func TestShouldHandleSingleKeyFalse(t *testing.T) { assert.False(t, skip) } } - -func TestShouldDeleteDetachedObj(t *testing.T) { - policy := policyv1.ConfigurationPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "default", - }, - Spec: &policyv1.ConfigurationPolicySpec{ - Severity: "low", - NamespaceSelector: policyv1.Target{ - Exclude: []policyv1.NonEmptyString{"kube-system"}, - }, - RemediationAction: "inform", - ObjectTemplates: []*policyv1.ObjectTemplate{ - { - ComplianceType: "musthave", - ObjectDefinition: runtime.RawExtension{}, - }, - }, - }, - } - - r := ConfigurationPolicyReconciler{} - testTable := []map[string]interface{}{ - { - "incommingRelate": []policyv1.RelatedObject{ - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod1", - Namespace: "default", - }, - }, - Reason: "", - }, - }, - "oldRelate": []policyv1.RelatedObject{ - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod2", - Namespace: "default", - }, - }, - Reason: "", - }, - }, - "expectRelated": []policyv1.RelatedObject{ - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod2", - Namespace: "default", - }, - }, - Reason: "", - }, - }, - }, - // not diff, should delete nothing - { - "incommingRelate": []policyv1.RelatedObject{ - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod", - Namespace: "default", - }, - }, - Reason: "", - }, - }, - "oldRelate": []policyv1.RelatedObject{ - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod", - Namespace: "default", - }, - }, - Reason: "", - }, - }, - "expectRelated": []policyv1.RelatedObject{}, - }, - // only Kind diff, should delete one(all) old - { - "incommingRelate": []policyv1.RelatedObject{ - { - Object: policyv1.ObjectResource{ - Kind: "development", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod", - Namespace: "default", - }, - }, - Reason: "", - }, - }, - "oldRelate": []policyv1.RelatedObject{ - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod", - Namespace: "default", - }, - }, - Reason: "", - }, - }, - "expectRelated": []policyv1.RelatedObject{ - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod", - Namespace: "default", - }, - }, - Reason: "", - }, - }, - }, - // should delete all old when kind diff - { - "incommingRelate": []policyv1.RelatedObject{ - { - Object: policyv1.ObjectResource{ - Kind: "development", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "dvl", - Namespace: "default", - }, - }, - Reason: "", - }, - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod", - Namespace: "default", - }, - }, - Reason: "", - }, - }, - "oldRelate": []policyv1.RelatedObject{ - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod", - // namespace diff - Namespace: "managed", - }, - }, - Reason: "", - }, - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod-1", - Namespace: "default", - }, - }, - Reason: "", - }, - }, - "expectRelated": []policyv1.RelatedObject{ - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod", - Namespace: "managed", - }, - }, - Reason: "", - }, - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod-1", - Namespace: "default", - }, - }, - Reason: "", - }, - }, - }, - // should delete all old when namespace diff - { - "incommingRelate": []policyv1.RelatedObject{ - { - Object: policyv1.ObjectResource{ - Kind: "development", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "dvl", - Namespace: "default", - }, - }, - Reason: "", - }, - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod", - Namespace: "default", - }, - }, - Reason: "", - }, - }, - "oldRelate": []policyv1.RelatedObject{ - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod", - // namespace diff - Namespace: "managed", - }, - }, - Reason: "", - }, - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod-1", - Namespace: "default", - }, - }, - Reason: "", - }, - }, - "expectRelated": []policyv1.RelatedObject{ - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod", - Namespace: "managed", - }, - }, - Reason: "", - }, - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod-1", - Namespace: "default", - }, - }, - Reason: "", - }, - }, - }, - // should delete all old when new is empty - { - "incommingRelate": []policyv1.RelatedObject{}, - "oldRelate": []policyv1.RelatedObject{ - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod", - // namespace diff - Namespace: "managed", - }, - }, - Reason: "", - }, - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod2", - Namespace: "default", - }, - }, - Reason: "", - }, - }, - "expectRelated": []policyv1.RelatedObject{ - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod", - // namespace diff - Namespace: "managed", - }, - }, - Reason: "", - }, - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod2", - Namespace: "default", - }, - }, - Reason: "", - }, - }, - }, - // should delete nothing when old is empty - { - "incommingRelate": []policyv1.RelatedObject{ - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod", - // namespace diff - Namespace: "managed", - }, - }, - Reason: "", - }, - { - Object: policyv1.ObjectResource{ - Kind: "pod", - APIVersion: "v1", - Metadata: policyv1.ObjectMetadata{ - Name: "pod2", - Namespace: "default", - }, - }, - Reason: "", - }, - }, - "oldRelate": []policyv1.RelatedObject{}, - "expectRelated": []policyv1.RelatedObject{}, - }, - } - - for _, test := range testTable { - deletedRelated := r.deleteDetachedObj(policy, test["incommingRelate"].([]policyv1.RelatedObject), - test["oldRelate"].([]policyv1.RelatedObject)) - assert.Equal(t, deletedRelated, test["expectRelated"]) - } -} diff --git a/controllers/configurationpolicy_utils.go b/controllers/configurationpolicy_utils.go index bee3aaf0..f244c220 100644 --- a/controllers/configurationpolicy_utils.go +++ b/controllers/configurationpolicy_utils.go @@ -557,9 +557,9 @@ func removeObjFinalizerPatch(obj metav1.Object, finalizer string) []byte { return nil } -func containRelated(arr []policyv1.RelatedObject, input policyv1.RelatedObject) bool { - // should compare only object - for _, r := range arr { +func containRelated(related []policyv1.RelatedObject, input policyv1.RelatedObject) bool { + // should compare name, APIVersion, Kind and namespace + for _, r := range related { if gocmp.Equal(r.Object, input.Object) { return true } diff --git a/test/e2e/case20_delete_objects_test.go b/test/e2e/case20_delete_objects_test.go index b1642171..11a55966 100644 --- a/test/e2e/case20_delete_objects_test.go +++ b/test/e2e/case20_delete_objects_test.go @@ -611,9 +611,9 @@ var _ = Describe("Test objects are not deleted when the CRD is removed", Ordered var _ = Describe("Clean up old object when configurationpolicy is changed", Ordered, func() { const ( - oldPodName string = "case29-name-changed-pod" - newPodName string = "case29-name-changed-new" - configplcName string = "case29-name-changed" + oldPodName string = "case20-name-changed-pod" + newPodName string = "case20-name-changed-new" + configplcName string = "case20-name-changed" case20ChangeConfigYaml string = "../resources/case20_delete_objects/case20_change_config_policy.yaml" ) cleanup := func() { @@ -696,3 +696,99 @@ var _ = Describe("Clean up old object when configurationpolicy is changed", Orde Expect(newPod).Should(BeNil()) }) }) + +var _ = Describe("Object Should not be deleted", Ordered, func() { + const ( + oldPodName string = "case20-name-changed-pod" + newPodName string = "case20-name-changed-new" + configplcName string = "case20-name-changed" + case20ChangeConfigYaml string = "../resources/case20_delete_objects/case20_change_config_policy.yaml" + ) + BeforeEach(func() { + utils.Kubectl("apply", "-f", case20ChangeConfigYaml, "-n", testNamespace) + patch := `[{"op": "remove", "path": "/spec/pruneObjectBehavior"}]` + utils.Kubectl("patch", "configurationpolicy", configplcName, "-n", testNamespace, + "--type=json", "-p", patch) + }) + cleanup := func() { + policies := []string{ + configplcName, + } + deleteConfigPolicies(policies) + + pods := []string{oldPodName, newPodName} + namespaces := []string{testNamespace, "default"} + deletePods(pods, namespaces) + } + AfterEach(cleanup) + It("check pod is not removed when PruneObjectBehavior is none and name changed", func() { + oldPod := utils.GetWithTimeout(clientManagedDynamic, gvrPod, + oldPodName, "default", true, defaultTimeoutSeconds) + Expect(oldPod).ShouldNot(BeNil()) + + By("Changing the pod name") + + patch := fmt.Sprintf(`[ + {"op":"replace", "path": "/spec/object-templates/0/objectDefinition/metadata/name", "value": %s} + ]`, newPodName) + utils.Kubectl("patch", "configurationpolicy", configplcName, "-n", testNamespace, + "--type=json", "-p", patch) + + Consistently(func() interface{} { + oldPod = utils.GetWithTimeout(clientManagedDynamic, gvrPod, + oldPodName, "default", true, defaultTimeoutSeconds) + Expect(oldPod).ShouldNot(BeNil()) + + return oldPod + }, 20, 1).ShouldNot(BeNil()) + }) + It("check pod is not removed when PruneObjectBehavior is none and namespace changed", func() { + oldPod := utils.GetWithTimeout(clientManagedDynamic, gvrPod, + oldPodName, "default", true, defaultTimeoutSeconds) + Expect(oldPod).ShouldNot(BeNil()) + + By("Changing namespace, old-pod should not exist, newpod exist in new namepace with old name") + patch := fmt.Sprintf(`[ + {"op":"replace", "path": "/spec/object-templates/0/objectDefinition/metadata/namespace", "value": %s} + ]`, testNamespace) + utils.Kubectl("patch", "configurationpolicy", configplcName, "-n", testNamespace, + "--type=json", "-p", patch) + + Consistently(func() interface{} { + oldPod = utils.GetWithTimeout(clientManagedDynamic, gvrPod, + oldPodName, "default", true, defaultTimeoutSeconds) + Expect(oldPod).ShouldNot(BeNil()) + + return oldPod + }, 20, 1).ShouldNot(BeNil()) + }) + It("check pod is not removed when PruneObjectBehavior is DeleteAll and spec changed", func() { + By("Add PruneObjectBehavior is DeleteAll") + patch := fmt.Sprintf(`[ + {"op":"add", "path": "/spec/pruneObjectBehavior", "value": %s} + ]`, "DeleteAll") + utils.Kubectl("patch", "configurationpolicy", configplcName, "-n", testNamespace, + "--type=json", "-p", patch) + + utils.Kubectl("apply", "-f", case20ChangeConfigYaml, "-n", testNamespace) + + oldPod := utils.GetWithTimeout(clientManagedDynamic, gvrPod, + oldPodName, "default", true, defaultTimeoutSeconds) + Expect(oldPod).ShouldNot(BeNil()) + + By("Changing imagePullPolicy in spec in object-templates") + patch = fmt.Sprintf(`[ + {"op":"replace", "path": "/spec/object-templates/0/imagePullPolicy", "value": %s}, + ]`, "Always") + utils.Kubectl("patch", "configurationpolicy", configplcName, "-n", testNamespace, + "--type=json", "-p", patch) + + Consistently(func() interface{} { + oldPod = utils.GetWithTimeout(clientManagedDynamic, gvrPod, + oldPodName, "default", true, defaultTimeoutSeconds) + Expect(oldPod).ShouldNot(BeNil()) + + return oldPod + }, 20, 1).ShouldNot(BeNil()) + }) +}) diff --git a/test/resources/case13_templatization/case13_fromsecret.yaml b/test/resources/case13_templatization/case13_fromsecret.yaml index 7b857a3b..0b986c68 100644 --- a/test/resources/case13_templatization/case13_fromsecret.yaml +++ b/test/resources/case13_templatization/case13_fromsecret.yaml @@ -3,6 +3,7 @@ kind: ConfigurationPolicy metadata: name: tmplt-policy-secret-duplicate spec: + pruneObjectBehavior: DeleteAll remediationAction: enforce namespaceSelector: exclude: ["kube-*"] diff --git a/test/resources/case20_delete_objects/case20_change_config_policy.yaml b/test/resources/case20_delete_objects/case20_change_config_policy.yaml index bb3fc28d..e075bafc 100644 --- a/test/resources/case20_delete_objects/case20_change_config_policy.yaml +++ b/test/resources/case20_delete_objects/case20_change_config_policy.yaml @@ -1,7 +1,7 @@ apiVersion: policy.open-cluster-management.io/v1 kind: ConfigurationPolicy metadata: - name: case29-name-changed + name: case20-name-changed namespace: managed spec: remediationAction: enforce @@ -12,7 +12,7 @@ spec: apiVersion: v1 kind: Pod metadata: - name: case29-name-changed-pod + name: case20-name-changed-pod namespace: default spec: containers: