Closed MOZGIII closed 4 years ago
The openapi spec says that PodSpec::containers
is not optional, so it's emitted as containers: Vec<...>
. In your JSON, it's null
, hence the error.
On that basis, the JSON sent by the API server is violating the spec and it would count as an API server bug or a spec bug. (Yes, the spec says it's "required", not that it's non-nullable, but historically the generator that Kubernetes uses uses the former to mean the latter.)
But that said, it seems more logical to me to consider that bookmark events will only have kind
, apiVersion
and metadata.resourceVersion
set to useful values anyway, since everything else is unnecessary. So it would probably make sense to make a custom type with just those properties instead of using the original T
that the WatchEvent
is parameterized on.
On that basis, the JSON sent by the API server is violating the spec and it would count as an API server bug or a spec bug. (Yes, the spec says it's "required", not that it's non-nullable, but historically the generator that Kubernetes uses uses the former to mean the latter.)
:frowning: I suspected this would be the issue.
One of the more generic workarounds would be to generally relax serde deserializer strictness to be more like Go decoder. It seems like it's something that could be tweaked for the generic case, and some other potential issues with similar effect.
But that said, it seems more logical to me to consider that bookmark events will only have
kind
,apiVersion
andmetadata.resourceVersion
set to useful values anyway, since everything else is unnecessary. So it would probably make sense to make a custom type with just those properties instead of using the originalT
that theWatchEvent
is parameterized on.
The type of the event is now known upfront, so it'd have to be something like PatchedWatchEvent<T>
or WatchEvent<Wrapped<T>>
for it to work.
Just noting that people have also reported this a similar issue with kind
: https://github.com/clux/kube-rs/issues/216
One of the more generic workarounds would be to generally relax serde deserializer strictness to be more like Go decoder. It seems like it's something that could be tweaked for the generic case, and some other potential issues with similar effect.
It's not a matter of "relaxing". A Vec
fundamentally cannot be deserialized from a null
. The thing that needs to change is not the deserializer but the deserialized type, namely to either make PodSpec::containers
an Option<Vec>
or to make it default to an empty Vec
, both of which are bad solutions. The former is bad because that means every field in the whole crate that is not an Option
today will need to be an Option
, because we're ignoring "required"-ness from the openapi spec. The latter is bad because it's a lie.
to be more like Go decoder
Golang is able to deserialize this type not because the decoder is special, but because golang is not type-safe enough to have non-nullable types. PodSpec.Containers
in golang is a slice, and golang slices can be null, so there's no deserialization failure.
The type of the event is now known upfront, so it'd have to be something like
PatchedWatchEvent<T>
orWatchEvent<Wrapped<T>>
for it to work.
I'm saying that we can change the definition from:
enum WatchEvent<T> {
...,
Bookmark(T),
}
to:
enum WatchEvent<T> {
...,
Bookmark(Bookmark),
}
struct Bookmark {
kind: String,
api_version: String,
metadata: ObjectMeta,
}
@clux Your issue I'm more inclined to consider is an openapi spec bug regarding the nullability of ContainerImage::names
. Unlike OP's case there is no special consideration like "it's a bookmark event so it's expected to have fewer fields set". So your issue should be reported to github.com/kubernetes/kubernetes (and can be fixup'd in this repo's codegen in the mean time).
@MOZGIII Just to confirm, do all pod watch bookmark events have object.spec.containers
set to null
? Or do they generally have the field set?
@MOZGIII Just to confirm, do all pod watch bookmark events have
object.spec.containers
set tonull
? Or do they generally have the field set?
The ones I saw are like that, and I haven't seen other bookmarks, so I guess it's safe to assume they're all like that (with null
).
Actually, kind
and api_version
would presumably always match the T
, so they probably don't need to be there either. And ObjectMeta::resource_version
is optional, but the bookmark is always going to have it. Furthermore none of the other fields in ObjectMeta
are going to be set.
So I'm inclined to make it Bookmark { resource_version: String }
This could become problematic in the future if bookmark events start getting more fields outside the flattened ObjectMeta
, but I don't think it's particularly likely. If it does happen we can worry about it then.
@MOZGIII Can you test with https://github.com/Arnavion/k8s-openapi/compare/fix-70 ?
Unit test I have at the parsing module on our end now passes! I'll run our E2E suite tomorrow to be sure, but so far looks good. :+1:
This bug no longer appears at our E2E tests. :+1:
Are there any plans to ship this?
I'm getting an error
when parsing this data:
with
Pod::try_from_parts
.I get this set of bytes from the K8s API as part of the watch request stream.
Expected output:
Versions:
serde
1.0.104serde_json
1.0.52k8s-openapi
0.7.1