Bug: Objects are pruned on templating errors #139
Conversation
| @@ -967,7 +969,7 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi | |||
| encryptionConfig, usedKeyCache, err = r.getEncryptionConfig(plc, true) | |||
|
|
|||
| if err != nil { | |||
| addTemplateErrorViolation("", err.Error()) | |||
| addTemplateErrorViolation("", err.Error(), true) | |||
There was a problem hiding this comment.
There are more addTemplateErrorViolation I am actually not sure which one should I add "skip removing resources"... Would anyone give me advice?
There was a problem hiding this comment.
Right now the instances I see where you don't set the parameter are:
- when
getEncryptionConfighas an error - when
templates.NewResolverhas an error - when
ObjectTemplatesRawis used, but doesn't have any templates, and thenyaml.Unmarshalhas an error.
I can't think of a situation where I would want things related objects removed when one of these errors occurs. So I wonder if there are really any template errors where we should consider removing resources - I think that when any error occurs, we probably shouldn't clean anything up, just in case.
If addTemplateErrorViolation always passed a deleteDetachedObjs = false parameter (see other comments) to checkRelatedAndUpdate, I think that would be a bit cleaner.
There was a problem hiding this comment.
@JustinKuli Did I miss anything?!
yiraeChristineKim
force-pushed
the
ACM-5301
branch
from
49b57b4
to
b01efd1
Compare
| @@ -967,7 +969,7 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi | |||
| encryptionConfig, usedKeyCache, err = r.getEncryptionConfig(plc, true) | |||
|
|
|||
| if err != nil { | |||
| addTemplateErrorViolation("", err.Error()) | |||
| addTemplateErrorViolation("", err.Error(), true) | |||
There was a problem hiding this comment.
Right now the instances I see where you don't set the parameter are:
- when
getEncryptionConfighas an error - when
templates.NewResolverhas an error - when
ObjectTemplatesRawis used, but doesn't have any templates, and thenyaml.Unmarshalhas an error.
I can't think of a situation where I would want things related objects removed when one of these errors occurs. So I wonder if there are really any template errors where we should consider removing resources - I think that when any error occurs, we probably shouldn't clean anything up, just in case.
If addTemplateErrorViolation always passed a deleteDetachedObjs = false parameter (see other comments) to checkRelatedAndUpdate, I think that would be a bit cleaner.
| @@ -1193,8 +1195,14 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi | |||
| // checkRelatedAndUpdate checks the related objects field and triggers an update on the ConfigurationPolicy | |||
| func (r *ConfigurationPolicyReconciler) checkRelatedAndUpdate( | |||
| plc policyv1.ConfigurationPolicy, related, oldRelated []policyv1.RelatedObject, sendEvent bool, | |||
| isTemplateParseErr ...bool, | |||
There was a problem hiding this comment.
I think I would prefer this to not be variadic, because it makes it easier to review that we've considered each time that this function is called. I would also prefer it to be called something like deleteDetachedObjs to describe what it does, rather than where it's coming from, since there could be other cases where we don't want to do this cleanup, but the reason has nothing to do with template errors.
There was a problem hiding this comment.
Looking at other times checkRelatedAndUpdate is called, I think there are more error cases where we don't want to delete "detached" objects.
There was a problem hiding this comment.
Final note (sorry), I think the function signature is long enough now that each parameter should be on a separate line, like https://github.com/open-cluster-management-io/config-policy-controller/blob/main/controllers/configurationpolicy_controller.go#L1394. But that's just a style thing.
yiraeChristineKim
force-pushed
the
ACM-5301
branch
from
b01efd1
to
215e61b
Compare
Signed-off-by: Yi Rae Kim <yikim@redhat.com>
yiraeChristineKim
force-pushed
the
ACM-5301
branch
from
215e61b
to
8cb8be1
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JustinKuli, 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:
Approvers can indicate their approval by writing |
Description:
Deploying a enforced ConfigurationPolicy with a valid template and then updating the template to be invalid caused the created object to be pruned. (I did it with hub templates, but I'd suspect managed cluster templates would have a similar behavior.)
expected: Not perform any deletion while in this error state.
Ref: https://issues.redhat.com/browse/ACM-5301
Signed-off-by: Yi Rae Kim yikim@redhat.com