diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go index 3cee26ab..cd6d0872 100644 --- a/controllers/operatorpolicy_controller.go +++ b/controllers/operatorpolicy_controller.go @@ -632,13 +632,15 @@ func (r *OperatorPolicyReconciler) mustnothaveOpGroup( return nil, changed, nil } - foundOpGroupName := "" + var foundOpGroupName string + var foundOpGroup *unstructured.Unstructured for _, opGroup := range foundOpGroups { emptyNameMatch := desiredOpGroup.Name == "" && opGroup.GetGenerateName() == desiredOpGroup.GenerateName if opGroup.GetName() == desiredOpGroup.Name || emptyNameMatch { foundOpGroupName = opGroup.GetName() + foundOpGroup = opGroup.DeepCopy() break } @@ -684,6 +686,11 @@ func (r *OperatorPolicyReconciler) mustnothaveOpGroup( } } + if foundOpGroup.GetDeletionTimestamp() != nil { + // No "early" condition because that would cause the status to flap + return nil, updateStatus(policy, deletingCond("OperatorGroup"), deletingObj(desiredOpGroup)), nil + } + earlyConds := []metav1.Condition{} if changed { @@ -872,6 +879,11 @@ func (r *OperatorPolicyReconciler) mustnothaveSubscription( return foundSub, nil, changed, nil } + if foundSub.GetDeletionTimestamp() != nil { + // No "early" condition because that would cause the status to flap + return foundSub, nil, updateStatus(policy, deletingCond("Subscription"), deletingObj(foundSub)), nil + } + earlyConds := []metav1.Condition{} if changed { @@ -1174,21 +1186,40 @@ func (r *OperatorPolicyReconciler) mustnothaveInstallPlan( earlyConds = append(earlyConds, calculateComplianceCondition(policy)) } - deletedInstallPlans := make([]policyv1.RelatedObject, 0, len(ownedInstallPlans)) + anyAlreadyDeleting := false for i := range ownedInstallPlans { + if ownedInstallPlans[i].GetDeletionTimestamp() != nil { + anyAlreadyDeleting = true + relatedInstallPlans[i] = deletingObj(&ownedInstallPlans[i]) + + continue + } + err := r.Delete(ctx, &ownedInstallPlans[i]) if err != nil { - changed := updateStatus(policy, foundNotWantedCond("InstallPlan"), deletedInstallPlans...) + changed := updateStatus(policy, foundNotWantedCond("InstallPlan"), relatedInstallPlans...) + + if anyAlreadyDeleting { + // reset the "early" conditions to avoid flapping. + earlyConds = []metav1.Condition{} + } return earlyConds, changed, fmt.Errorf("error deleting the InstallPlan: %w", err) } ownedInstallPlans[i].SetGroupVersionKind(installPlanGVK) // Delete stripped this information - deletedInstallPlans = append(deletedInstallPlans, deletedObj(&ownedInstallPlans[i])) + relatedInstallPlans[i] = deletedObj(&ownedInstallPlans[i]) + } + + if anyAlreadyDeleting { + // reset the "early" conditions to avoid flapping. + earlyConds = []metav1.Condition{} + + return earlyConds, updateStatus(policy, deletingCond("InstallPlan"), relatedInstallPlans...), nil } - updateStatus(policy, deletedCond("InstallPlan"), deletedInstallPlans...) + updateStatus(policy, deletedCond("InstallPlan"), relatedInstallPlans...) return earlyConds, true, nil } @@ -1285,21 +1316,40 @@ func (r *OperatorPolicyReconciler) mustnothaveCSV( earlyConds = append(earlyConds, calculateComplianceCondition(policy)) } - deletedCSVs := make([]policyv1.RelatedObject, 0, len(csvList)) + anyAlreadyDeleting := false for i := range csvList { + if csvList[i].GetDeletionTimestamp() != nil { + anyAlreadyDeleting = true + relatedCSVs[i] = deletingObj(&csvList[i]) + + continue + } + err := r.Delete(ctx, &csvList[i]) if err != nil { - changed := updateStatus(policy, foundNotWantedCond("ClusterServiceVersion"), deletedCSVs...) + changed := updateStatus(policy, foundNotWantedCond("ClusterServiceVersion"), relatedCSVs...) + + if anyAlreadyDeleting { + // reset the "early" conditions to avoid flapping + earlyConds = []metav1.Condition{} + } return earlyConds, changed, fmt.Errorf("error deleting ClusterServiceVersion: %w", err) } csvList[i].SetGroupVersionKind(clusterServiceVersionGVK) - deletedCSVs = append(deletedCSVs, deletedObj(&csvList[i])) + relatedCSVs[i] = deletedObj(&csvList[i]) + } + + if anyAlreadyDeleting { + // reset the "early" conditions to avoid flapping + earlyConds = []metav1.Condition{} + + return earlyConds, updateStatus(policy, deletingCond("ClusterServiceVersion"), relatedCSVs...), nil } - updateStatus(policy, deletedCond("ClusterServiceVersion"), deletedCSVs...) + updateStatus(policy, deletedCond("ClusterServiceVersion"), relatedCSVs...) return earlyConds, true, nil } @@ -1420,21 +1470,40 @@ func (r *OperatorPolicyReconciler) handleCRDs( earlyConds = append(earlyConds, calculateComplianceCondition(policy)) } - deletedCRDs := make([]policyv1.RelatedObject, 0, len(crdList)) + anyAlreadyDeleting := false for i := range crdList { + if crdList[i].GetDeletionTimestamp() != nil { + anyAlreadyDeleting = true + relatedCRDs[i] = deletingObj(&crdList[i]) + + continue + } + err := r.Delete(ctx, &crdList[i]) if err != nil { - changed := updateStatus(policy, foundNotWantedCond("CustomResourceDefinition"), deletedCRDs...) + changed := updateStatus(policy, foundNotWantedCond("CustomResourceDefinition"), relatedCRDs...) + + if anyAlreadyDeleting { + // reset the "early" conditions to avoid flapping. + earlyConds = []metav1.Condition{} + } return earlyConds, changed, fmt.Errorf("error deleting the CRD: %w", err) } crdList[i].SetGroupVersionKind(customResourceDefinitionGVK) - deletedCRDs = append(deletedCRDs, deletedObj(&crdList[i])) + relatedCRDs[i] = deletedObj(&crdList[i]) + } + + if anyAlreadyDeleting { + // reset the "early" conditions to avoid flapping. + earlyConds = []metav1.Condition{} + + return earlyConds, updateStatus(policy, deletingCond("CustomResourceDefinition"), relatedCRDs...), nil } - updateStatus(policy, deletedCond("CustomResourceDefinition"), deletedCRDs...) + updateStatus(policy, deletedCond("CustomResourceDefinition"), relatedCRDs...) return earlyConds, true, nil } diff --git a/controllers/operatorpolicy_status.go b/controllers/operatorpolicy_status.go index 3b2a6b6e..de222a85 100644 --- a/controllers/operatorpolicy_status.go +++ b/controllers/operatorpolicy_status.go @@ -459,6 +459,17 @@ func deletedCond(kind string) metav1.Condition { } } +// deletingCond returns a NonCompliant condition, with a Reason like '____Deleting', +// and a Message like 'the ____ has a deletion timestamp' +func deletingCond(kind string) metav1.Condition { + return metav1.Condition{ + Type: condType(kind), + Status: metav1.ConditionFalse, + Reason: kind + "Deleting", + Message: "the " + kind + " has a deletion timestamp", + } +} + // keptCond returns a Compliant condition, with a Reason like '____Kept', // and a Message like 'the policy specifies to keep the ____' func keptCond(kind string) metav1.Condition { @@ -839,6 +850,14 @@ func deletedObj(obj client.Object) policyv1.RelatedObject { } } +func deletingObj(obj client.Object) policyv1.RelatedObject { + return policyv1.RelatedObject{ + Object: policyv1.ObjectResourceFromObj(obj), + Compliant: string(policyv1.NonCompliant), + Reason: "The object is being deleted but has not been removed yet", + } +} + // matchedObj returns a Compliant RelatedObject with reason = 'Resource found as expected' func matchedObj(obj client.Object) policyv1.RelatedObject { return policyv1.RelatedObject{ diff --git a/test/e2e/case38_install_operator_test.go b/test/e2e/case38_install_operator_test.go index 08c5c58b..6a567886 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/test/e2e/case38_install_operator_test.go @@ -1902,8 +1902,58 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { utils.GetWithTimeout(clientManagedDynamic, gvrCRD, "quayregistries.quay.redhat.com", "", true, eventuallyTimeout) }) - It("Should remove things when enforced while set to Delete everything", func(ctx SpecContext) { - // Change the removal behaviors from Keep to Delete + It("Should report a special status when the resources are stuck", func(ctx SpecContext) { + By("Adding a finalizer to each of the resources") + pol, err := clientManagedDynamic.Resource(gvrOperatorPolicy). + Namespace(opPolTestNS).Get(ctx, opPolName, metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + Expect(pol).NotTo(BeNil()) + + relatedObjects, found, err := unstructured.NestedSlice(pol.Object, "status", "relatedObjects") + Expect(err).NotTo(HaveOccurred()) + Expect(found).To(BeTrue()) + + var opGroupName, installPlanName, csvName string + crdNames := make([]string, 0) + + for _, relatedObject := range relatedObjects { + relatedObj, ok := relatedObject.(map[string]interface{}) + Expect(ok).To(BeTrue()) + + objKind, found, err := unstructured.NestedString(relatedObj, "object", "kind") + Expect(err).NotTo(HaveOccurred()) + Expect(found).To(BeTrue()) + + objName, found, err := unstructured.NestedString(relatedObj, "object", "metadata", "name") + Expect(err).NotTo(HaveOccurred()) + Expect(found).To(BeTrue()) + + switch objKind { + case "OperatorGroup": + opGroupName = objName + case "Subscription": + // just do the finalizer; we already know the subscription name + case "ClusterServiceVersion": + csvName = objName + case "InstallPlan": + installPlanName = objName + case "CustomResourceDefinition": + crdNames = append(crdNames, objName) + default: + // skip adding / removing the finalizer for other types + continue + } + + utils.Kubectl("patch", objKind, objName, "-n", opPolTestNS, "--type=json", "-p", + `[{"op": "add", "path": "/metadata/finalizers", "value": ["donutdelete"]}]`) + DeferCleanup(func() { + By("removing the finalizer from " + objKind + " " + objName) + utils.Kubectl("patch", objKind, objName, "-n", opPolTestNS, "--type=json", "-p", + `[{"op": "remove", "path": "/metadata/finalizers"}]`) + }) + } + + By("Setting the removal behaviors to Delete") utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p", `[{"op": "replace", "path": "/spec/removalBehavior/operatorGroups", "value": "Delete"},`+ `{"op": "replace", "path": "/spec/removalBehavior/subscriptions", "value": "Delete"},`+ @@ -1911,6 +1961,126 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { `{"op": "replace", "path": "/spec/removalBehavior/installPlans", "value": "Delete"},`+ `{"op": "replace", "path": "/spec/removalBehavior/customResourceDefinitions", "value": "Delete"}]`) + check( + opPolName, + false, + []policyv1.RelatedObject{{ + Object: policyv1.ObjectResource{ + Kind: "OperatorGroup", + APIVersion: "operators.coreos.com/v1", + Metadata: policyv1.ObjectMetadata{ + Namespace: opPolTestNS, + Name: opGroupName, + }, + }, + Compliant: "NonCompliant", + Reason: "The object is being deleted but has not been removed yet", + }}, + metav1.Condition{ + Type: "OperatorGroupCompliant", + Status: metav1.ConditionFalse, + Reason: "OperatorGroupDeleting", + Message: "the OperatorGroup has a deletion timestamp", + }, + `the OperatorGroup was deleted`, + ) + check( + opPolName, + false, + []policyv1.RelatedObject{{ + Object: policyv1.ObjectResource{ + Kind: "Subscription", + APIVersion: "operators.coreos.com/v1alpha1", + Metadata: policyv1.ObjectMetadata{ + Namespace: opPolTestNS, + Name: "project-quay", + }, + }, + Compliant: "NonCompliant", + Reason: "The object is being deleted but has not been removed yet", + }}, + metav1.Condition{ + Type: "SubscriptionCompliant", + Status: metav1.ConditionFalse, + Reason: "SubscriptionDeleting", + Message: "the Subscription has a deletion timestamp", + }, + `the Subscription was deleted`, + ) + check( + opPolName, + false, + []policyv1.RelatedObject{{ + Object: policyv1.ObjectResource{ + Kind: "InstallPlan", + APIVersion: "operators.coreos.com/v1alpha1", + Metadata: policyv1.ObjectMetadata{ + Namespace: opPolTestNS, + Name: installPlanName, + }, + }, + Compliant: "NonCompliant", + Reason: "The object is being deleted but has not been removed yet", + }}, + metav1.Condition{ + Type: "InstallPlanCompliant", + Status: metav1.ConditionFalse, + Reason: "InstallPlanDeleting", + Message: "the InstallPlan has a deletion timestamp", + }, + `the InstallPlan was deleted`, + ) + check( + opPolName, + false, + []policyv1.RelatedObject{{ + Object: policyv1.ObjectResource{ + Kind: "ClusterServiceVersion", + APIVersion: "operators.coreos.com/v1alpha1", + Metadata: policyv1.ObjectMetadata{ + Namespace: opPolTestNS, + Name: csvName, + }, + }, + Compliant: "NonCompliant", + Reason: "The object is being deleted but has not been removed yet", + }}, + metav1.Condition{ + Type: "ClusterServiceVersionCompliant", + Status: metav1.ConditionFalse, + Reason: "ClusterServiceVersionDeleting", + Message: "the ClusterServiceVersion has a deletion timestamp", + }, + `the ClusterServiceVersion was deleted`, + ) + desiredCRDObjects := make([]policyv1.RelatedObject, 0) + for _, name := range crdNames { + desiredCRDObjects = append(desiredCRDObjects, policyv1.RelatedObject{ + Object: policyv1.ObjectResource{ + Kind: "CustomResourceDefinition", + APIVersion: "apiextensions.k8s.io/v1", + Metadata: policyv1.ObjectMetadata{ + Name: name, + }, + }, + Compliant: "NonCompliant", + Reason: "The object is being deleted but has not been removed yet", + }) + } + check( + opPolName, + false, + desiredCRDObjects, + metav1.Condition{ + Type: "CustomResourceDefinitionCompliant", + Status: metav1.ConditionFalse, + Reason: "CustomResourceDefinitionDeleting", + Message: "the CustomResourceDefinition has a deletion timestamp", + }, + `the CustomResourceDefinition was deleted`, + ) + }) + It("Should report things as gone after the finalizers are removed", func() { By("Checking that certain (named) resources are not there, indicating the removal was completed") utils.GetWithTimeout(clientManagedDynamic, gvrClusterServiceVersion, "quay-operator.v3.8.13", opPolTestNS, false, eventuallyTimeout)