Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KubernetesResourceList resources are not handled properly by AbstractWatchManager #4496

Closed
StupidScience opened this issue Oct 13, 2022 · 7 comments · Fixed by #4506
Closed
Milestone

Comments

@StupidScience
Copy link

Describe the bug

In #4365 was introduced change that in a nutshell changed from this:

if (object instanceof KubernetesResourceList) {}
else if (object instanceof HasMetadata) {}

to

if (object instanceof HasMetadata) {
  if (object instanceof KubernetesResourceList) {}
}

But AFAIU KubernetesResourceList doesn't implement HasMetadata so any event will end up with error io.fabric8.kubernetes.client.WatcherException: Invalid object received: ...

Fabric8 Kubernetes Client version

5.12.4

Steps to reproduce

  1. Send event with any List resource to watcher
  2. Got an exception

Expected behavior

List resources expected to be handled

Runtime

Kubernetes (vanilla)

Kubernetes API Server version

other (please specify in additional context)

Environment

other (please specify in additional context)

Fabric8 Kubernetes Client Logs

No response

Additional context

No response

@shawkins
Copy link
Contributor

@StupidScience what watch is this happening with specifically? I can see in the history that the list handling was added for api group watches - and I don't see that a mock or a integration test was ever specifically added.

@StupidScience
Copy link
Author

@shawkins not sure that I get your question. Do you mean what specific resource watcher? If yes then in our case it is for Endpoints.

@shawkins
Copy link
Contributor

@StupidScience what kubernetes version are you on?

@StupidScience
Copy link
Author

@shawkins it occurred in our tests actually with mock KubernetesServer after update to 5.12.4. Rollback to 5.12.3 fix the issue.

@shawkins
Copy link
Contributor

shawkins commented Oct 13, 2022

@StupidScience from what I can see the api server does not (at least with v1.22.2) respond with a list object when you watch Endpoints. It would be great to capture definitively where this is still happening.

In looking at the go client their watch logic is tolerant to list objects being returned, but it won't flatten them into separate events - which means their informer logic can't handle that either. My hope is that this list object handling is no longer relevant.

@manusa
Copy link
Member

manusa commented Oct 18, 2022

I agree with Steven, the following code:

HasMetadata hasMetadata = (HasMetadata) object;
updateResourceVersion(hasMetadata.getMetadata().getResourceVersion());
if (object instanceof KubernetesResourceList) {
// Dirty cast - should always be valid though
@SuppressWarnings({ "rawtypes" })
KubernetesResourceList list = (KubernetesResourceList) hasMetadata;
@SuppressWarnings("unchecked")
List<HasMetadata> items = list.getItems();
if (items != null) {
for (HasMetadata item : items) {
eventReceived(action, item);
}
}
} else {
eventReceived(action, hasMetadata);
}

should be simply replaced to:

        HasMetadata hasMetadata = (HasMetadata) object;
        updateResourceVersion(hasMetadata.getMetadata().getResourceVersion());
        eventReceived(action, hasMetadata);

@StupidScience
Copy link
Author

Let’s simplify then? Since current code is misleading and I assume with this kind of nesting there are no chances to fall into lists related block anyway

shawkins added a commit to shawkins/kubernetes-client that referenced this issue Oct 18, 2022
manusa pushed a commit to shawkins/kubernetes-client that referenced this issue Oct 19, 2022
@manusa manusa added this to the 6.2.0 milestone Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants