MAISTRA-1541: Fix panic when controller not set

[knrc]

No description provided.

[luksa]

I'm not sure this code is correct.

While an object normally has only one owner, this isn't the case when users or other controllers add additional owner references to the object (a controller could in theory do this to prevent this object from being garbage collected until the additional owner is deleted).

Given the following scenario:

X is the controller of Y is (a non-controller) owner of C
A is the controller of B is the controller of C

C has two owners. One is a controller, the other is not. This code returns X as the root controller of C, but the actual root controller is A. At the very least, the code should give priority to the owner that is marked as a controller, and try to find a root controller among its ancestors.

But even in the case of a single path such as

A is the controller of B is a non-controller owner of C

according to the definition of owners and controllers (see below), A is not the root controller of C. If the code really needs to return A, the function name should be changed, as it doesn't match what the code does.

An owner is an owner of another object only in the sense that it prevents the other object from being garbage collected.

A controller is an owner that manages the other object. (The first object's controller is actually the controller that manages the other object, but to simplify things, we simply say that the first object is the controller of the second).

So, while this change might function properly in normal scenarios, it may break when users add additional owners.

[knrc]

@luksa I'm not sure I agree with this, the only relaxation I introduced was with respect to owners who have the kind 'ReplicationController' or 'ReplicaSet' and this is likely to be the desirable behaviour since they should have the appropriate name. The issue with the OpenShift implementation is that there is a ReplicationController added as an owner without it being tagged as a Controller.

The only problem I could see would be if someone added multiple ReplicaSet/ReplicaController owners in which case we would be taking the first.

[luksa]

Oh, I missed the fact that you only traverse up if the owner is a rs or rc, but my other point still stands.

If a user deploys a Pod by creating a Deployment and then adds another ReplicaSet/ReplicationController as an additional owner to this pod, the code might return the wrong root controller. Although the user's action is nonsensical, adding an additional owner like this is in accordance with the semantics of owner references.

I'll approve the PR so it can be merged, but I feel the behavior is not completely correct.

[knrc]

@luksa I agree with the general point. I think the impact would be minimal especially as we already know OpenShift doesn't set controller in this particular circumstance and the root controller is only used to derive the name of the workload for completing the destination/source attributes. The bigger issue is the panic when this condition is encountered.

[knrc]

Note the merging of this PR is on hold for now