patch-operator icon indicating copy to clipboard operation
patch-operator copied to clipboard

Remove applied patch when Patch CR

Open LDL-GH opened this issue 3 years ago • 8 comments

Hi

I've used the patch operator with great success and am glad to be able to apply patches in a declarative way.

I was however wondering if it would be possible to (maybe optionally) configure it in a way that when the Patch CR is removed the applied patch is undone as well.

This may not be applicable to more advanced patches, but currently we have a few patches that just add some annotations to certain services. It would be nice these annotations would disappear when the Patch CR that added them is removed.

LDL-GH avatar Mar 24 '22 14:03 LDL-GH

as you have correctly identified there does not seem to be a deterministic way to undo a patch. In terms of declarative configuration what you ask is correct, but I don't know of a way to implement it.

raffaelespazzoli avatar Mar 24 '22 22:03 raffaelespazzoli

Hello there, The right way would be the verbose way at the same time: storing into the Patch CR, the last entire original CR. Honestly, I think this is a dirty way of doing it. I would not implement this but would treat to solve this with better GitOps, for example, remove and reconcile the original CR without applying the Patch CR.

achetronic avatar Apr 10 '22 10:04 achetronic

a CR manifest size is limited. This approach would severely limit the number of objects that can be patched by one patch cR. What would the use case be for undoing a patch, besides perfect declarative semantics?

On Sun, Apr 10, 2022 at 6:58 AM Alby Hernández @.***> wrote:

Hello there, The right way would be the verbose way at the same time: storing into the Patch CR, the last entire original CR. Honestly, I think this is a dirty way of doing it. I would not implement this but would treat to solve this with better GitOps, for example, remove and reconcile the original CR without applying the Patch CR.

— Reply to this email directly, view it on GitHub https://github.com/redhat-cop/patch-operator/issues/14#issuecomment-1094245462, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPERXBKZXGVUHZ4QHCQSB3VEKX3NANCNFSM5RRKCMUQ . You are receiving this because you commented.Message ID: @.***>

-- ciao/bye Raffaele

raffaelespazzoli avatar Apr 11 '22 01:04 raffaelespazzoli

this issue was mistakenly closed.

raffaelespazzoli avatar Apr 14 '22 12:04 raffaelespazzoli

Honestly, I thing it is better not to implement the feature this way commented. Anyway I can not find a better way to do it. What would be your proposal, @LDL-GH?

achetronic avatar Apr 21 '22 16:04 achetronic

We currently use the Patch operator for one specific intended purpose, to patch existing openshift objects. For example to add annotations to the openshift-etcd service (required for dynatrace monitoring) or to patch a custom certificate for ingress.

This usually involves a single object being patched with minimal config, so might not be impacted by the CR manifest size limit. However this is only a small part of what the patch operator is capable of and any implemented solution would better cover the whole scope.

Since objects like proxy/cluster or the ingresscontroller.operator are (to some degree) managed by the cluster itself, I'm not sure what will happen if I'd delete them first to recreate with my custom config added or what would happen during an upgrade?

I personally can't find a better way to implement a possible solution either though.

ref: documentation to replace ingress cert

LDL-GH avatar Apr 22 '22 10:04 LDL-GH

We currently use the Patch operator for one specific intended purpose, to patch existing openshift objects. For example to add annotations to the openshift-etcd service (required for dynatrace monitoring) or to patch a custom certificate for ingress.

This usually involves a single object being patched with minimal config, so might not be impacted by the CR manifest size limit. However this is only a small part of what the patch operator is capable of and any implemented solution would better cover the whole scope.

Since objects like proxy/cluster or the ingresscontroller.operator are (to some degree) managed by the cluster itself, I'm not sure what will happen if I'd delete them first to recreate with my custom config added or what would happen during an upgrade?

I personally can't find a better way to implement a possible solution either though.

ref: documentation to replace ingress cert

I have been thinking about the problem. What about imolementing another CR kinded PatchRevision which stores a copy of the resource before being patches and enough metadata to rollback when needed from a Patch? In the Patch CR could exist a parameter with with the ID of the revision and if the Patch is deleted, it re-apply the PatchRevision it refers to. What do you think about this approach?

achetronic avatar May 02 '22 04:05 achetronic

We currently use the Patch operator for one specific intended purpose, to patch existing openshift objects. For example to add annotations to the openshift-etcd service (required for dynatrace monitoring) or to patch a custom certificate for ingress. This usually involves a single object being patched with minimal config, so might not be impacted by the CR manifest size limit. However this is only a small part of what the patch operator is capable of and any implemented solution would better cover the whole scope. Since objects like proxy/cluster or the ingresscontroller.operator are (to some degree) managed by the cluster itself, I'm not sure what will happen if I'd delete them first to recreate with my custom config added or what would happen during an upgrade? I personally can't find a better way to implement a possible solution either though. ref: documentation to replace ingress cert

I have been thinking about the problem. What about imolementing another CR kinded PatchRevision which stores a copy of the resource before being patches and enough metadata to rollback when needed from a Patch? In the Patch CR could exist a parameter with with the ID of the revision and if the Patch is deleted, it re-apply the PatchRevision it refers to. What do you think about this approach?

Wouldn't this still be impacted by the CR manifest size when you're patching multiple resources? The copy of the original resource should also be updated every time the patched resource is updated. e.g. if a label is added to the resource afterwards I still expect that label to be there if I remove the patch.

LDL-GH avatar May 10 '22 08:05 LDL-GH