Garbage collector will delete objects when wrong GVK is specified in ownerRefs

[tnozicka]

Follow up on #14582

Upstream garbage collector already checks for and ignores objects it doesn't know.

What happens in OpenShift is that if you specify invalid resource it works the same way. But if you specify a valid OpenShift resource with API version (without group) and GC has a cold cache, it will delete it as an orphan. That's because RESTMapper won't raise an error but dynamic client hits url with /api instead of /oapi and get 404 as if the resource wouldn't exist thus deletes the "orphan". This will cause data loss. Even if we fix all the controllers to create a valid controllerRefs and tested it properly, user can still manually (or with some automation of his own) create an ownerReference and such object would be incorrectly deleted resulting in data loss.

It seems like we could switch RESTMapper for garbage collector to fix it. (See @deads2k comment)

[deads2k]

It seems like we could switch RESTMapper for garbage collector to fix it. (See @deads2k comment)

To be clear, the fix in this case means to fail harder and earlier, not actually tolerate such owner references.

[tnozicka]

Agreed. (To expand on that: it doesn't fail with an error at all at this point which is the biggest issue - it just deletes data incorrectly.)

[tnozicka]

cc: @mfojtik

[deads2k]

I don't think this is a blocker for 3.6. The ownerrefs are managed in code, we just have to have them set them correctly.

[tnozicka]

@deads2k For our objects, yes. For DC<-RC<-Po we set only 1 cotrollerRef in Po+RC. Users could set other owner references manually as well to achieve cascade deletion. Or have some scripts/controllers in place to do that.

ControllerRef is a special kind of OwnerRef (and a singleton) that we control and we can make sure to set it up correctly. OwnerRefs in general can be set up by anyone I think. Like you want to tie a lifetime of your DC to an IS or another DC or ... User can do that. I am aware that the probability of him doing that is quite low but it still results in data loss.

[deads2k]

ControllerRef is a special kind of OwnerRef (and a singleton) that we control and we can make sure to set it up correctly. OwnerRefs in general can be set up by anyone I think. Like you want to tie a lifetime of your DC to an IS or another DC or ... User can do that. I am aware that the probability of him doing that is quite low but it still results in data loss.

I'm not going to claim it's ideal, but performing an advanced operation incorrectly (and there is a very narrow path to doing it correctly) behaving consistently (every other mistake also results in deletion), ought not block a release.

[tnozicka]

@deads2k I am fine with letting it slip 1.6.0; just wanted to point out it's not only about our controllers

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[tnozicka]

/lifecycle frozen