Parsing error when decoding bookmark

[MOZGIII]

I'm getting an error

Json(Error(\"invalid type: null, expected a sequence\", line: 2, column: 163))

when parsing this data:

\n{\"type\":\"BOOKMARK\",\"object\":{\"kind\":\"Pod\",\"apiVersion\":\"v1\",\"metadata\":{\"resourceVersion\":\"3845\",\"creationTimestamp\":null},\"spec\":{\"containers\":null},\"status\":{}}}\n

with Pod::try_from_parts.

I get this set of bytes from the K8s API as part of the watch request stream.

Expected output:

WatchEvent::Bookmark(Pod {
  metadata: Some(ObjectMeta {
      resource_version: Some("3845".into()),
      creation_timestamp: None,
      ..ObjectMeta::default()
  }),
  spec: Some(PodSpec {
      containers: vec![],
      ..PodSpec::default()
  }),
  ..Pod::default()
});

Versions:

• serde 1.0.104
• serde_json 1.0.52
• k8s-openapi 0.7.1

[Arnavion]

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.

[MOZGIII]

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.)

😦 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 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.

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.

[clux]

Just noting that people have also reported this a similar issue with kind: kube-rs/kube#216

[Arnavion]

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> or WatchEvent<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,
}

[Arnavion]

@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]

@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?

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).

[Arnavion]

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.

[Arnavion]

@MOZGIII Can you test with https://github.com/Arnavion/k8s-openapi/compare/fix-70 ?

[MOZGIII]

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. 👍

[MOZGIII]

This bug no longer appears at our E2E tests. 👍

[MOZGIII]

Are there any plans to ship this?