[Critical Bug]unexpectedly deleted when pruneObjectBehavior is None #142

yiraeChristineKim · 2023-06-14T19:08:23Z

Description of problem:

When a ConfigurationPolicy stops managing an object, it is always deleted regardless of pruneObjectBehavior being set to None.

Below are a few examples of when a ConfigurationPolicy would stop managing an object:

The ConfigurationPolicy is updated to manage a different object.
The ConfigurationPolicy's namespaceSelector is changed.
The ConfigurationPolicy's templating resolves differently based on changing conditions to no longer manage an object.
Version-Release number of selected component (if applicable):
How reproducible:

Very reproducable

Steps to Reproduce:
Create a ConfigurationPolicy with pruneObjectBehavior set to "None" (default) that creates/updates a ConfigMap called "test"
Update the ConfigurationPolicy to create/update a ConfigMap called "not test"
Actual results:

After following the reproducing steps, the ConfigMap called "test" is deleted.

Expected results:

After following the reproducing steps, the ConfigMap called "test" should not be deleted.

Additional info:

This will cause unexpected data loss to customers.

Ref: https://issues.redhat.com/browse/ACM-5939

controllers/configurationpolicy_controller.go

JustinKuli

Other than the extra file, I think this looks right! Feel free to ignore my suggestion, I might have gotten a bit carried away...

JustinKuli · 2023-06-14T21:03:02Z

test/e2e/__debug_bin

We probably don't want this

JustinKuli · 2023-06-14T21:03:24Z

controllers/configurationpolicy_utils.go

- // should compare only object
- for _, r := range arr {
+func containRelated(related []policyv1.RelatedObject, input policyv1.RelatedObject) bool {
+ // should compare name, APIVersion, Kind and namepcace


Suggested change

// should compare name, APIVersion, Kind and namepcace

// should compare name, APIVersion, Kind and namespace

JustinKuli · 2023-06-14T21:08:36Z

controllers/configurationpolicy_controller.go

+func (r *ConfigurationPolicyReconciler) cleanUpChildObjects(plc policyv1.ConfigurationPolicy,
+ related []policyv1.RelatedObject,
+) []string {
 deletionFailures := []string{}

+ // When spec is updated and new related objects are created
+ if len(related) != 0 {
+ log.Info("preparing to delete detached objects...")
+
+ oldRelated := plc.Status.RelatedObjects
+ var objShouldRemoved []policyv1.RelatedObject
+
+ for _, oldR := range oldRelated {
+ exist := containRelated(related, oldR)
+
+ if !exist {
+ objShouldRemoved = append(objShouldRemoved, oldR)
+ }
+ }
+
+ plc.Status.RelatedObjects = objShouldRemoved
+ }
+
 for _, object := range plc.Status.RelatedObjects {
 // set up client for object deletion


I'm thinking out loud here in case this is cleaner (it is to me at least)... what do you think of this?

func (r *ConfigurationPolicyReconciler) cleanUpChildObjects(plc policyv1.ConfigurationPolicy, newRelated []policyv1.RelatedObject, ) []string { deletionFailures := []string{} objsToDelete := []policyv1.RelatedObject{} log.Info("preparing to delete detached objects...") if len(newRelated) == 0 { // delete all objsToDelete = plc.Status.RelatedObjects } else { // When spec is updated and new related objects are created oldRelated := plc.Status.RelatedObjects for _, oldR := range oldRelated { if !containRelated(newRelated, oldR) { objsToDelete = append(objsToDelete, oldR) } } } for _, object := range objsToDelete { // set up client for object deletion

Actually, the two cases can just work the same, because if the newRelated is empty, then containRelated will just always be false.

func (r *ConfigurationPolicyReconciler) cleanUpChildObjects(plc policyv1.ConfigurationPolicy, newRelated []policyv1.RelatedObject, ) []string { deletionFailures := []string{} objsToDelete := []policyv1.RelatedObject{} log.Info("preparing to delete detached objects...") oldRelated := plc.Status.RelatedObjects for _, oldR := range oldRelated { if !containRelated(newRelated, oldR) { objsToDelete = append(objsToDelete, oldR) } } for _, object := range objsToDelete { // set up client for object deletion

also logic is same but change is variable name right?

when newRelated = nil we have to fall in loop, unnecessary isn't it?

mprahl · 2023-06-15T00:06:39Z

controllers/configurationpolicy_controller.go

+ var objShouldRemoved []policyv1.RelatedObject
+
+ for _, oldR := range objsToDelete {
+ exist := containRelated(newRelated, oldR)
+
+ if !exist {
+ objShouldRemoved = append(objShouldRemoved, oldR)
+ }
+ }
+
+ objsToDelete = objShouldRemoved


Optional simplification

Suggested change

var objShouldRemoved []policyv1.RelatedObject

for _, oldR := range objsToDelete {

exist := containRelated(newRelated, oldR)

if !exist {

objShouldRemoved = append(objShouldRemoved, oldR)

}

}

objsToDelete = objShouldRemoved

objsToDelete = []policyv1.RelatedObject{}

for _, oldR := range plc.Status.RelatedObjects {

if !containRelated(newRelated, oldR) {

objsToDelete = append(objsToDelete, oldR)

}

}

mprahl · 2023-06-15T00:09:22Z

controllers/configurationpolicy_controller.go

+ // At here, relatedObjects is always []
+ failures := r.cleanUpChildObjects(plc, relatedObjects)


Maybe this is clearer?

Suggested change

// At here, relatedObjects is always []

failures := r.cleanUpChildObjects(plc, relatedObjects)

failures := r.cleanUpChildObjects(plc, nil)

mprahl · 2023-06-15T00:13:32Z

test/resources/case13_templatization/case13_fromsecret.yaml

@@ -3,6 +3,7 @@ kind: ConfigurationPolicy
 metadata:
 name: tmplt-policy-secret-duplicate
 spec:
+ pruneObjectBehavior: DeleteAll


Is this change needed?

pruneObjectBehavior : none template error always fall to needsDelete = false

It is more targeted test

I had requested this :)

If this test doesn't have pruneObjectBehavior set, then the template error definitely shouldn't cause anything to be removed (because now, things are only removed when that is set). The test is meant to answer the question "Will objects be removed when there is a templating error, and I generally want objects to be pruned?" with "No, objects will not be removed".

mprahl · 2023-06-15T00:21:51Z

controllers/configurationpolicy_controller.go

@@ -571,6 +591,8 @@ func (r *ConfigurationPolicyReconciler) cleanUpChildObjects(plc policyv1.Configu
 object.Properties.UID == uid {
 needsDelete = true
 }
+ } else { // None or not
+ needsDelete = false


This is happening too late in the method since the code above is performing API queries only for the return values to never get used.

My suggestion is to add something to this effect in the start of the method:

if !strings.EqualFold(string(plc.Spec.RemediationAction), "enforce") { return deletionFailures } if !(string(plc.Spec.PruneObjectBehavior) == "DeleteAll" || string(plc.Spec.PruneObjectBehavior) == "DeleteIfCreated") { return deletionFailures }

Afterwards, you can also remove the if statement of if strings.EqualFold(string(plc.Spec.RemediationAction), "enforce") { on line 581 of the diff.

mprahl

This looks good but just a performance concern and a few optional improvements.

mprahl · 2023-06-15T13:36:41Z

controllers/configurationpolicy_controller.go

+
+ // When spec is updated and new related objects are created
+ if len(newRelated) != 0 {
+ log.Info("preparing to delete detached objects...")


Optional:

Suggested change

log.Info("preparing to delete detached objects...")

log.Info("Preparing to delete detached objects...")

Shouldn't this log message only be shown if objsToDelete has entries?

um.... you right. let me delete it

mprahl · 2023-06-15T13:38:13Z

controllers/configurationpolicy_controller.go

@@ -767,7 +794,8 @@ 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)
+ // At here, relatedObjects is always []


Suggested change

// At here, relatedObjects is always []

mprahl

A few minor comments

Signed-off-by: Yi Rae Kim <yikim@redhat.com>

mprahl

/hold for review from Justin

JustinKuli · 2023-06-15T14:19:39Z

/unohold

openshift-ci · 2023-06-15T14:19:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JustinKuli, mprahl, yiraeChristineKim

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JustinKuli,mprahl,yiraeChristineKim]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mprahl · 2023-06-15T14:38:48Z

/unhold

openshift-ci bot added the dco-signoff: yes label Jun 14, 2023

openshift-ci bot requested review from gparvin and mprahl June 14, 2023 19:08

openshift-ci bot added the approved label Jun 14, 2023

yiraeChristineKim requested a review from JustinKuli June 14, 2023 19:08

yiraeChristineKim commented Jun 14, 2023

View reviewed changes

controllers/configurationpolicy_controller.go Outdated Show resolved Hide resolved

yiraeChristineKim requested a review from dhaiducek June 14, 2023 19:26

yiraeChristineKim force-pushed the ACM-5939 branch 4 times, most recently from 666e0b8 to e61fe12 Compare June 14, 2023 20:55

JustinKuli reviewed Jun 14, 2023

View reviewed changes

yiraeChristineKim requested a review from JustinKuli June 14, 2023 22:13

yiraeChristineKim force-pushed the ACM-5939 branch from e61fe12 to ca93716 Compare June 14, 2023 22:15

mprahl reviewed Jun 15, 2023

View reviewed changes

mprahl requested changes Jun 15, 2023

View reviewed changes

openshift-ci bot assigned mprahl Jun 15, 2023

yiraeChristineKim force-pushed the ACM-5939 branch from ca93716 to 26c1934 Compare June 15, 2023 12:00

yiraeChristineKim requested a review from mprahl June 15, 2023 12:09

yiraeChristineKim force-pushed the ACM-5939 branch from 26c1934 to cf76c20 Compare June 15, 2023 13:19

mprahl reviewed Jun 15, 2023

View reviewed changes

unexpectedly deleted when pruneObjectBehavior is None

cc4a248

Signed-off-by: Yi Rae Kim <yikim@redhat.com>

yiraeChristineKim force-pushed the ACM-5939 branch from cf76c20 to cc4a248 Compare June 15, 2023 14:00

yiraeChristineKim requested a review from mprahl June 15, 2023 14:03

mprahl approved these changes Jun 15, 2023

View reviewed changes

openshift-ci bot added do-not-merge/hold lgtm labels Jun 15, 2023

JustinKuli approved these changes Jun 15, 2023

View reviewed changes

openshift-ci bot assigned JustinKuli Jun 15, 2023

openshift-ci bot removed the do-not-merge/hold label Jun 15, 2023

openshift-merge-robot merged commit 418c94c into open-cluster-management-io:main Jun 15, 2023

magic-mirror-bot bot mentioned this pull request Jun 15, 2023

🤖 Sync from open-cluster-management-io/config-policy-controller: #142 stolostron/config-policy-controller#514

Merged

yiraeChristineKim deleted the ACM-5939 branch February 29, 2024 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Critical Bug]unexpectedly deleted when pruneObjectBehavior is None #142

[Critical Bug]unexpectedly deleted when pruneObjectBehavior is None #142

yiraeChristineKim commented Jun 14, 2023

JustinKuli left a comment

JustinKuli Jun 14, 2023

JustinKuli Jun 14, 2023

yiraeChristineKim Jun 14, 2023

JustinKuli Jun 14, 2023

JustinKuli Jun 14, 2023

yiraeChristineKim Jun 14, 2023

yiraeChristineKim Jun 14, 2023

mprahl Jun 15, 2023 •

edited

Loading

mprahl Jun 15, 2023

yiraeChristineKim Jun 15, 2023

mprahl Jun 15, 2023

yiraeChristineKim Jun 15, 2023

yiraeChristineKim Jun 15, 2023

yiraeChristineKim Jun 15, 2023 •

edited

Loading

JustinKuli Jun 15, 2023

mprahl Jun 15, 2023

mprahl left a comment

mprahl Jun 15, 2023

mprahl Jun 15, 2023

yiraeChristineKim Jun 15, 2023

mprahl Jun 15, 2023

mprahl left a comment

mprahl left a comment

JustinKuli commented Jun 15, 2023

openshift-ci bot commented Jun 15, 2023

mprahl commented Jun 15, 2023

	// should compare name, APIVersion, Kind and namepcace
	// should compare name, APIVersion, Kind and namespace

		// At here, relatedObjects is always []
		failures := r.cleanUpChildObjects(plc, relatedObjects)

	// At here, relatedObjects is always []
	failures := r.cleanUpChildObjects(plc, relatedObjects)
	failures := r.cleanUpChildObjects(plc, nil)

	log.Info("preparing to delete detached objects...")
	log.Info("Preparing to delete detached objects...")

[Critical Bug]unexpectedly deleted when pruneObjectBehavior is None #142

[Critical Bug]unexpectedly deleted when pruneObjectBehavior is None #142

Conversation

yiraeChristineKim commented Jun 14, 2023

JustinKuli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mprahl Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiraeChristineKim Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mprahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mprahl left a comment

Choose a reason for hiding this comment

mprahl left a comment

Choose a reason for hiding this comment

JustinKuli commented Jun 15, 2023

openshift-ci bot commented Jun 15, 2023

mprahl commented Jun 15, 2023

mprahl Jun 15, 2023 •

edited

Loading

yiraeChristineKim Jun 15, 2023 •

edited

Loading