Arnavion / k8s-openapi

Rust definitions of the resource types in the Kubernetes client API
Apache License 2.0
382 stars 40 forks source link

`MicroTime` can be `null` #137

Closed foriequal0 closed 1 year ago

foriequal0 commented 1 year ago

kubernetes apimachinary's MicroTime can be parsed from null https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/micro_time.go#L111 Event events.k8s.io/v1 that is converted from core/v1 usually have "eventTime": null e.g.

{
    "apiVersion": "v1",
    "count": 1,
    "eventTime": null,
    "firstTimestamp": "2023-02-16T17:41:58Z",
    "involvedObject": {
        "apiVersion": "v1",
        "kind": "Pod",
        "name": "some-pod",
        "namespace": "test-243550",
        "resourceVersion": "317717",
        "uid": "aa79cf26-7485-489a-a67e-a5561a8d4c98"
    },
    "kind": "Event",
    "lastTimestamp": "2023-02-16T17:41:58Z",
    "message": "Successfully assigned test-243550/some-pod to test-pgd-control-plane",
    "metadata": {
        "creationTimestamp": "2023-02-16T17:41:58Z",
        "name": "some-pod.17445ee9a7a6379d",
        "namespace": "test-243550",
        "resourceVersion": "317719",
        "uid": "7fbf643d-c04d-4e5b-84e8-136c4bd44ff9"
    },
    "reason": "Scheduled",
    "reportingComponent": "",
    "reportingInstance": "",
    "source": {
        "component": "default-scheduler"
    },
    "type": "Normal"
}
Arnavion commented 1 year ago

It can, but it produces a zero time.Time, which per https://pkg.go.dev/time#Time represents "January 1, year 1, 00:00:00.000000000 UTC".

k8s-openapi types the Event::event_time field as Option<MicroTime> because the spec says the field is optional.

Is 0001-01-01T00:00:00.000000000Z a more useful value than None ? In golang where nonsensical "zero" values for structs are the norm since optionals require pointer types, sure. But I'm not sure I want to carry that forward into Rust when we have a proper Option.

foriequal0 commented 1 year ago

core/v1 Event spec says eventTime is optional, but events.k8s.io/v1 Event spec says it's required. so api.get() or api.list() on events.k8s.io/v1 Event woud fail when there's any event that is converted from old type.

foriequal0 commented 1 year ago

Oh, I forgot to mention that events.k8s.io/v1 Event usually contains core/v1 Events that are automatically converted.

foriequal0 commented 1 year ago

I attached kubectl get event, not kubecl get events.events.k8s.io this is the latter.

{
    "apiVersion": "events.k8s.io/v1",
    "deprecatedCount": 1,
    "deprecatedFirstTimestamp": "2023-02-16T18:25:07Z",
    "deprecatedLastTimestamp": "2023-02-16T18:25:07Z",
    "deprecatedSource": {
        "component": "default-scheduler"
    },
    "eventTime": null,
    "kind": "Event",
    "metadata": {
        "creationTimestamp": "2023-02-16T18:25:07Z",
        "name": "some-pod.174461445d6d3cff",
        "namespace": "test-530513",
        "resourceVersion": "320932",
        "uid": "f3ac47c9-cc86-4193-a2ed-2e4428b7ad77"
    },
    "note": "Successfully assigned test-530513/some-pod to test-pgd-control-plane",
    "reason": "Scheduled",
    "regarding": {
        "apiVersion": "v1",
        "kind": "Pod",
        "name": "some-pod",
        "namespace": "test-530513",
        "resourceVersion": "320930",
        "uid": "ace87ee8-15fb-46ae-9dae-c77422c2010f"
    },
    "type": "Normal"
}
Arnavion commented 1 year ago

Does the API server accept a JSON blob that doesn't contain an eventTime field at all as an events.k8s.io/v1 Event ? Or does it require the field to be present?

Arnavion commented 1 year ago

Okay, from experimentation, it looks like the old core Events are translated into events/v1 Events with eventTime: null. Creating new Events (or editing existing ones) with eventTime: null or without eventTime is rejected by the API server. New events/v1 Events that were generated by the API server as such must have eventTime: non-null.

There is also the case of reportingController etc fields - they are required in new events/v1 Events, but unlike eventTime they are omitted entirely instead of being set to null for old core Events. Also unlike the eventTime field they are marked as optional in the OpenAPI spec.

So I think I'll treat this as an OpenAPI spec bug that should mark Event::eventTime as an optional field just like it did for reportingController etc, rather than allow MicroTime to be deserializable from null.