search

kubernetes / kube-openapi

Kubernetes OpenAPI spec generation & serving
Apache License 2.0
318 stars 205 forks source link

OpenAPI V3 references parsed as *proto.Arbitrary instead of *proto.Ref #477

Open tomleb opened 4 months ago

tomleb commented 4 months ago

The issue

I'm using proto.NewOpenAPIV3Data() to parse an OpenAPI V3 spec coming from the /openapi/v3 endpoints of the apiserver.

I'd expect fields such as .metadata.ownerReferences to be a *proto.Ref that refers to the io.k8s.apimachinery.pkg.apis.meta.v1.OwnerReference schema, however they are instead returned as *proto.Arbitrary.

I'm not sure if this is the intended behavior, but I believe it should return a *proto.Ref.

Investigation

I observed a difference between the testdata for pkg/util/proto and the actual spec returned by /openapi/v3.

ownerReferences field from testdata

The $ref is directly under items.

          ...
          "ownerReferences": {
            "description": "List of objects depended by this object. If ALL objects in the list have been deleted, this object will be garbage collected. If this object is managed by a controller, then an entry in this list will point to this controller, with the controller field set to true. There cannot be more than one managing controller.",
            "type": "array",
            "items": {
              "default": {},
              "$ref": "#/components/schemas/io.k8s.apimachinery.pkg.apis.meta.v1.OwnerReference"
            },
            "x-kubernetes-patch-merge-key": "uid",
            "x-kubernetes-patch-strategy": "merge"
          },
          ...

ownerReferences field from k3s 1.30.0 (and 1.28.6)

The $ref is under an allOf, under items.

          ...
          "ownerReferences": {
            "description": "List of objects depended by this object. If ALL objects in the list have been deleted, this object will be garbage collected. If this object is managed by a controller, then an entry in this list will point to this controller, with the controller field set to true. There cannot be more than one managing controller.",
            "type": "array",
            "items": {
              "default": {},
              "allOf": [
                {
                  "$ref": "#/components/schemas/io.k8s.apimachinery.pkg.apis.meta.v1.OwnerReference"
                }
              ]
            },
            "x-kubernetes-list-map-keys": [
              "uid"
            ],
            "x-kubernetes-list-type": "map",
            "x-kubernetes-patch-merge-key": "uid",
            "x-kubernetes-patch-strategy": "merge"
          },
          ...

Going further, this document is being parsed by gnostic's openapi_v3.ParseDocument. This results in the following different result (marshaled to JSON):

from testdata

items -> schema_or_reference -> oneof -> reference -> the ref

                     ...
                      "name": "ownerReferences",
                      "value": {
                        "Oneof": {
                          "Schema": {
                            "type": "array",
                            "items": {
                              "schema_or_reference": [
                                {
                                  "Oneof": {
                                    "Reference": {
                                      "_ref": "#/components/schemas/io.k8s.apimachinery.pkg.apis.meta.v1.OwnerReference"
                                    }
                                  }
                                }
                              ]
                            }
                          }
                        }
                      }
                     ...

from k3s 1.30.0

items -> schema_or_reference -> oneof -> allof ->oneof -> reference -> the ref

                     ...
                      "name": "ownerReferences",
                      "value": {
                        "Oneof": {
                          "Schema": {
                            "type": "array",
                            "items": {
                              "schema_or_reference": [
                                {
                                  "Oneof": {
                                    "Schema": {
                                      "all_of": [
                                        {
                                          "Oneof": {
                                            "Reference": {
                                              "_ref": "#/components/schemas/io.k8s.apimachinery.pkg.apis.meta.v1.OwnerReference"
                                            }
                                          }
                                        }
                                      ],
                                      "default": {
                                        "Oneof": null
                                      }
                                    }
                                  }
                                }
                              ]
                            }
                          }
                        }
                      }
                     ...

Looking at the code, the first case is handled correctly. The calls are: proto.ParseSchemaV3 -> proto.parseV3Array -> proto.ParseV3SchemaOrReference -> proto.ParseV3SchemaReference -> returns a Ref.

The second case fails to parse as a ref properly because once it finds the Oneof which has type schema, so proto.ParseSchemaV3 is called. That function expects a type field to be defined, but since there isn't one, it defaults to returning an arbitrary.

Solution

From the looks of it, it seems like proto.ParseSchemaV3 should first check if there's an AllOf, before checking if there's a type. I have a small proof of concept that makes it work for my use case, though I'm not knowledgeable enough on the OpenAPI spec side to know whether that's a bullet proof fix.

Reproducing

I created a repo for easily reproducing this issue: https://github.com/tomleb/kube-openapi-issue/blob/master/main_test.go. Simply run go test -v ./.... I included a OpenAPI V3 spec from v1.30.0, v1.28.6 and from this repo's testdata.

I'd appreciate some guidance on whether this is a truly a bug (as opposed to say, misuse of the library?) and thoughts on the fix proposed above. I'd be willing to work on the fix and contribute to the repo once that's confirmed.

tomleb commented 4 months ago

A quick glance at the closed PRs, it looks like this PR introduced the wrapped refs: https://github.com/kubernetes/kube-openapi/pull/298/. The reason for wrapping the refs is explained here: https://github.com/kubernetes/kubernetes/issues/106387#issue-1052139640.

Any members other than "$ref" in a JSON Reference object SHALL be ignored.

Aka we wouldn't be able to have description with a $ref, so instead we wrap it in an allOf.

k8s-triage-robot commented 1 month ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot commented 1 week ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten