diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index 9fd9b8de..b6b382e1 100644 --- a/controllers/configurationpolicy_controller.go +++ b/controllers/configurationpolicy_controller.go @@ -1136,14 +1136,15 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi func (r *ConfigurationPolicyReconciler) checkRelatedAndUpdate( plc policyv1.ConfigurationPolicy, related, oldRelated []policyv1.RelatedObject, sendEvent bool, ) { - sortRelatedObjectsAndUpdate(&plc, related, oldRelated, r.EnableMetrics) + r.sortRelatedObjectsAndUpdate(&plc, related, oldRelated, r.EnableMetrics) // An update always occurs to account for the lastEvaluated status field r.addForUpdate(&plc, sendEvent) } // helper function to check whether related objects has changed -func sortRelatedObjectsAndUpdate( - plc *policyv1.ConfigurationPolicy, related, oldRelated []policyv1.RelatedObject, collectMetrics bool, +func (r *ConfigurationPolicyReconciler) sortRelatedObjectsAndUpdate( + plc *policyv1.ConfigurationPolicy, related, oldRelated []policyv1.RelatedObject, + collectMetrics bool, ) { sort.SliceStable(related, func(i, j int) bool { if related[i].Object.Kind != related[j].Object.Kind { @@ -1156,8 +1157,6 @@ func sortRelatedObjectsAndUpdate( return related[i].Object.Metadata.Name < related[j].Object.Metadata.Name }) - update := false - // Instantiate found objects for the related object metric found := map[string]bool{} @@ -1215,19 +1214,34 @@ func sortRelatedObjectsAndUpdate( } } - if len(oldRelated) == len(related) { - for i, entry := range oldRelated { - if !gocmp.Equal(entry, related[i]) { - update = true - } + if !gocmp.Equal(related, oldRelated) { + r.deleteDetachedObj(*plc, related, oldRelated) + 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) } - } else { - update = true } - if update { - plc.Status.RelatedObjects = related + 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 diff --git a/controllers/configurationpolicy_controller_test.go b/controllers/configurationpolicy_controller_test.go index d68f9dd4..58d095fd 100644 --- a/controllers/configurationpolicy_controller_test.go +++ b/controllers/configurationpolicy_controller_test.go @@ -381,6 +381,8 @@ func TestAddRelatedObject(t *testing.T) { } func TestSortRelatedObjectsAndUpdate(t *testing.T) { + r := &ConfigurationPolicyReconciler{} + policy := &policyv1.ConfigurationPolicy{ ObjectMeta: metav1.ObjectMeta{ Name: "foo", @@ -412,14 +414,14 @@ func TestSortRelatedObjectsAndUpdate(t *testing.T) { empty := []policyv1.RelatedObject{} - sortRelatedObjectsAndUpdate(policy, relatedList, empty, false) + r.sortRelatedObjectsAndUpdate(policy, relatedList, empty, false) assert.True(t, relatedList[0].Object.Metadata.Name == "bar") // append another object named bar but also with namespace bar - relatedList = append(relatedList, addRelatedObjects(true, rsrc, "ConfigurationPolicy", "bar", - true, []string{name}, "reason", nil)...) + relatedList = append(relatedList, addRelatedObjects(true, rsrc, + "ConfigurationPolicy", "bar", true, []string{name}, "reason", nil)...) - sortRelatedObjectsAndUpdate(policy, relatedList, empty, false) + r.sortRelatedObjectsAndUpdate(policy, relatedList, empty, false) assert.True(t, relatedList[0].Object.Metadata.Namespace == "bar") // clear related objects and test sorting with no namespace @@ -430,7 +432,7 @@ func TestSortRelatedObjectsAndUpdate(t *testing.T) { relatedList = append(relatedList, addRelatedObjects(true, rsrc, "ConfigurationPolicy", "", false, []string{name}, "reason", nil)...) - sortRelatedObjectsAndUpdate(policy, relatedList, empty, false) + r.sortRelatedObjectsAndUpdate(policy, relatedList, empty, false) assert.True(t, relatedList[0].Object.Metadata.Name == "bar") } @@ -754,3 +756,384 @@ func TestShouldEvaluatePolicy(t *testing.T) { ) } } + +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 da5e4bb6..93784b00 100644 --- a/controllers/configurationpolicy_utils.go +++ b/controllers/configurationpolicy_utils.go @@ -9,6 +9,7 @@ import ( "sort" "strings" + gocmp "github.com/google/go-cmp/cmp" apiRes "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -560,3 +561,14 @@ func removeObjFinalizer(obj metav1.Object, finalizer string) []string { return result } + +func containRelated(arr []policyv1.RelatedObject, input policyv1.RelatedObject) bool { + // should compare only object + for _, r := range arr { + if gocmp.Equal(r.Object, input.Object) { + return true + } + } + + return false +} diff --git a/test/e2e/case20_delete_objects_test.go b/test/e2e/case20_delete_objects_test.go index 2115a06a..277f210c 100644 --- a/test/e2e/case20_delete_objects_test.go +++ b/test/e2e/case20_delete_objects_test.go @@ -6,6 +6,7 @@ package e2e import ( "context" "errors" + "fmt" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -606,3 +607,91 @@ var _ = Describe("Test objects are not deleted when the CRD is removed", Ordered }, defaultTimeoutSeconds, 1).Should(Equal(oldPodUID)) }) }) + +var _ = Describe("Clean up old object when configuraionpolicy is changed", Ordered, func() { + const ( + oldPodName string = "case29-name-changed-pod" + newPodName string = "case29-name-changed-new" + configplcName string = "case29-name-changed" + case20ChangeConfigYaml string = "../resources/case20_delete_objects/case20_change_config_policy.yaml" + ) + cleanup := func() { + policies := []string{ + configplcName, + } + deleteConfigPolicies(policies) + + pods := []string{oldPodName, newPodName} + namespaces := []string{testNamespace, "default"} + deletePods(pods, namespaces) + } + AfterEach(cleanup) + It("check old pod is removed when name is changed in configpolicy ", func() { + utils.Kubectl("apply", "-f", case20ChangeConfigYaml, "-n", testNamespace) + + 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) + + oldPod = utils.GetWithTimeout(clientManagedDynamic, gvrPod, + oldPodName, "default", false, defaultTimeoutSeconds) + Expect(oldPod).Should(BeNil()) + + newPod := utils.GetWithTimeout(clientManagedDynamic, gvrPod, + newPodName, "default", true, defaultTimeoutSeconds) + Expect(newPod).ShouldNot(BeNil()) + }) + It("check old pod is removed when namespace is changed in configpolicy ", func() { + utils.Kubectl("apply", "-f", case20ChangeConfigYaml, "-n", testNamespace) + + 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) + + oldPod = utils.GetWithTimeout(clientManagedDynamic, gvrPod, + oldPodName, "default", false, defaultTimeoutSeconds) + Expect(oldPod).Should(BeNil()) + + newPod := utils.GetWithTimeout(clientManagedDynamic, gvrPod, + oldPodName, testNamespace, true, defaultTimeoutSeconds) + Expect(newPod).ShouldNot(BeNil()) + }) + It("check old pod and new pod is removed when namespace is changed in configpolicy ", func() { + utils.Kubectl("apply", "-f", case20ChangeConfigYaml, "-n", testNamespace) + + oldPod := utils.GetWithTimeout(clientManagedDynamic, gvrPod, + oldPodName, "default", true, defaultTimeoutSeconds) + Expect(oldPod).ShouldNot(BeNil()) + + By("Changing complianceType and podname at the sametime, both pods should not exist") + patch := fmt.Sprintf(`[ + {"op":"replace", "path": "/spec/object-templates/0/complianceType", "value": %s}, + {"op":"replace", "path": "/spec/object-templates/0/objectDefinition/metadata/name", "value": %s} + ]`, "mustnothave", newPodName) + utils.Kubectl("patch", "configurationpolicy", configplcName, "-n", testNamespace, + "--type=json", "-p", patch) + + oldPod = utils.GetWithTimeout(clientManagedDynamic, gvrPod, + oldPodName, "default", false, defaultTimeoutSeconds) + Expect(oldPod).Should(BeNil()) + + newPod := utils.GetWithTimeout(clientManagedDynamic, gvrPod, + newPodName, "default", false, defaultTimeoutSeconds) + Expect(newPod).Should(BeNil()) + }) +}) diff --git a/test/e2e/e2e_suite_test.go b/test/e2e/e2e_suite_test.go index 29728457..572ae8f0 100644 --- a/test/e2e/e2e_suite_test.go +++ b/test/e2e/e2e_suite_test.go @@ -29,7 +29,6 @@ import ( var ( testNamespace string - gvrPolicy schema.GroupVersionResource defaultTimeoutSeconds int kubeconfigManaged string clientManaged kubernetes.Interface @@ -44,6 +43,7 @@ var ( gvrClusterClaim schema.GroupVersionResource gvrConfigMap schema.GroupVersionResource gvrDeployment schema.GroupVersionResource + gvrPolicy schema.GroupVersionResource defaultImageRegistry string ) @@ -199,3 +199,24 @@ func deleteConfigPolicies(policyNames []string) { ) } } + +func deletePods(podNames []string, namespaces []string) { + for _, podName := range podNames { + for _, ns := range namespaces { + err := clientManagedDynamic.Resource(gvrPod).Namespace(ns).Delete( + context.TODO(), podName, metav1.DeleteOptions{}, + ) + if !errors.IsNotFound(err) { + Expect(err).To(BeNil()) + } + } + } + + for _, podName := range podNames { + for _, ns := range namespaces { + _ = utils.GetWithTimeout( + clientManagedDynamic, gvrPod, podName, ns, false, defaultTimeoutSeconds, + ) + } + } +} diff --git a/test/resources/case20_delete_objects/case20_change_config_policy.yaml b/test/resources/case20_delete_objects/case20_change_config_policy.yaml new file mode 100644 index 00000000..bb3fc28d --- /dev/null +++ b/test/resources/case20_delete_objects/case20_change_config_policy.yaml @@ -0,0 +1,23 @@ +apiVersion: policy.open-cluster-management.io/v1 +kind: ConfigurationPolicy +metadata: + name: case29-name-changed + namespace: managed +spec: + remediationAction: enforce + pruneObjectBehavior: DeleteAll + object-templates: + - complianceType: musthave + objectDefinition: + apiVersion: v1 + kind: Pod + metadata: + name: case29-name-changed-pod + namespace: default + spec: + containers: + - name: nginx + imagePullPolicy: Never + image: nginx:1.7.9 + ports: + - containerPort: 80 \ No newline at end of file