WatchNames: return errors via WebSocket

[moio]

Align behavior with plain Watch, which also does the same. UI code actually expects the same behavior in both cases.

Fixes rancher/rancher#41809

[rmweir]

LGTM

[MbolotSuse]

One comment on the safety of returning the error event without doing any processing of the object that it contains.

I also have an overall question on the solution: my first glance would say that the ideal solution would be to immediately return a resource.error event when we are given an invalid resource version, rather than a resource.start, then a resource.error, then a resource.stop. I think that the idea here is just to align this with standard watch rather than re-evaluate the API behavior, but wanted to ask just to be sure - would you mind elaborating on why you choose the start -> error -> stop flow here?

[moio]

I also have an overall question on the solution: my first glance would say that the ideal solution would be to immediately return a resource.error event when we are given an invalid resource version, rather than a resource.start, then a resource.error, then a resource.stop. I think that the idea here is just to align this with standard watch rather than re-evaluate the API behavior, but wanted to ask just to be sure - would you mind elaborating on why you choose the start -> error -> stop flow here?

As you mentioned, here I am proposing to align behavior with the Watch function. UI code does not differentiate the two cases, as from the UI perspective a watch call is a watch call - only Steve makes a distinction between Watch and WatchNames internally depending on permissions.

I think you are right in pointing out this particular flow could be optimized to be less chatty, but that would need to be done for Watch and WatchNames, aligning client UI code in the process. To me, that's a valid optimization/refactoring but it is a separate effort/PR from just fixing the bug. It should be done later, separately, and subject to an evaluation of importance - as this is could be a pretty rare case.

[moio]

@MbolotSuse following our discussions can you approve and merge this?

Relatedly: user reported success with the patch in the meantime.

[MbolotSuse]

@moio I've resolved the outstanding thread on the functionality (and I think that we can merge this PR without further changes there), however, I'm not sure that the tests are working. Would you mind giving that a look?

[moio]

@MbolotSuse please review once again and, if all looks OK,merge this directly. Thanks in advance

[moio]

Partially related, about your question which was left pending:

listAndWatch currently has a case that looks like it functions much the same as the case that you fixed does (i.e. debug the error and don't send on the channel). Would you mind seeing if you need to fix that as well?

I took a deeper look. To recap, listAndWatch is the function producing events, including errors, consumed by WatchNames, the the subject of this PR. WatchNames, in turn, forwards events to the UI and this PR is about forwarding errors to the UI too, so that the UI can act on them. Watch does the same.

Therefore when listAndWatch receives an error (from the Kubernetes watch operation) it has two options:

• log and ignore or
• send it to the UI, by forwarding it WatchNames/Watch

At this point listAndWatch is forwarding all errors that can be type-asserted to *v1.Status - which is recommended but not guaranteed by the API:

https://github.com/kubernetes/apimachinery/blob/f14778da5523847e4c07346e3161a4b4f6c9186e/pkg/watch/watch.go#L67-L69

I think that approach makes sense - if Steve is at least able to extract a string or a code from the error then there is a chance the UI can do something about it, therefore forward (eg. the "resource too old" situation that kicked off this PR).

For an error whose type is completely unknown, there is no way to tell if it makes sense to forward to the UI or not and even if it does, there is no way to extract a meaningful string or code that the UI could check and react on a priori. It will need intervention from a Steve programmer anyway to add code for a specific type when such a case is found, and the best way to serve that future programmer is to have a reliable log.

What could be argued, IMO, is to raise the log level from debug to info or even error to increase the visibility of those cases, if they are important at all. ATM I do not have reasons to change that, but others might.

Please help me double-check the reasoning above. If it makes sense, I will not open a follow-up PR.