watcher: return NoResourceVersion error if resource version is empty
#1259
Conversation
Check if resource version returned by the API to the watcher is empty and return a `NoResourceVersion` error if it is. Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1259 +/- ##
==========================================
- Coverage 73.19% 73.16% -0.03%
==========================================
Files 75 75
Lines 6025 6030 +5
==========================================
+ Hits 4410 4412 +2
- Misses 1615 1618 +3
|
| let resource_version = obj.resource_version().unwrap_or_default(); | ||
| if resource_version.is_empty() { | ||
| (Some(Err(Error::NoResourceVersion)), State::default()) |
There was a problem hiding this comment.
One minor observation here. Before this change in the watching stage, we would actually be somewhat resilient to missing resource versions here (where resource versions got transmitted as empty strings) because the resource version is only used in the next watch, and Kubernetes seem to accept empty strings in the watch params 🙃
It is technically possible that this change will lead to more re-lists because of some apiserver returning "" as resource versions. Still, that's kind of an impossibly defensive game to play against what is meant to be a stable api, so it's probably best to assume they are reasonably well behaved here, and raise bugs upstream otherwise.
There was a problem hiding this comment.
yes its theoretically possible, but as far as i'm aware, this should practically not happen since the resource version in the response is never blank. From https://kubernetes.io/docs/reference/using-api/api-concepts/#resourceversion-in-metadata:
v1.meta/ListMeta - The metadata.resourceVersion of a resource collection (the response to a list) identifies the resource version at which the collection was constructed.
clux
changed the title
NoResourceVersion error if resource version is emptywatcher: return NoResourceVersion error if resource version is empty
Motivation
Atm, the watcher don't check if the returned resource version is empty before taking its next step, which effectively makes it a hard error (potential panics since we use
unwrap()in some places). Instead of treating them as hard errors, we should bubble them up.Solution
Check if resource version returned by the API to the watcher is empty and return a
NoResourceVersionerror if it is.Fixes #1251