[BUG] OpenAPI Spec is missing many k8s fields during generation

[drewmullen]

When the OAS Doc is generated and it gets to ObjectMeta, a subfield of VirtualMachine, it misses most of the values:

https://github.com/harvester/harvester/blob/master/api/openapi-spec/swagger.json#L10548-L10553

      "k8s.io.v1.ObjectMeta": {
...
        "properties": {
          "name": {
            "type": "string"
          },
          "namespace": {
            "type": "string"
          }
        }
      },

Compare that to what is defined in the vendored k8s types:

type ObjectMeta struct {
    Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"`
    GenerateName string `json:"generateName,omitempty" protobuf:"bytes,2,opt,name=generateName"`
    Namespace string `json:"namespace,omitempty" protobuf:"bytes,3,opt,name=namespace"`
    SelfLink string `json:"selfLink,omitempty" protobuf:"bytes,4,opt,name=selfLink"`
    UID types.UID `json:"uid,omitempty" protobuf:"bytes,5,opt,name=uid,casttype=k8s.io/kubernetes/pkg/types.UID"`
    ResourceVersion string `json:"resourceVersion,omitempty" protobuf:"bytes,6,opt,name=resourceVersion"`
    Generation int64 `json:"generation,omitempty" protobuf:"varint,7,opt,name=generation"`
    CreationTimestamp Time `json:"creationTimestamp,omitempty" protobuf:"bytes,8,opt,name=creationTimestamp"`
    DeletionTimestamp *Time `json:"deletionTimestamp,omitempty" protobuf:"bytes,9,opt,name=deletionTimestamp"`
    DeletionGracePeriodSeconds *int64 `json:"deletionGracePeriodSeconds,omitempty" protobuf:"varint,10,opt,name=deletionGracePeriodSeconds"`
    Labels map[string]string `json:"labels,omitempty" protobuf:"bytes,11,rep,name=labels"`
    Annotations map[string]string `json:"annotations,omitempty" protobuf:"bytes,12,rep,name=annotations"`
    OwnerReferences []OwnerReference `json:"ownerReferences,omitempty" patchStrategy:"merge" patchMergeKey:"uid" protobuf:"bytes,13,rep,name=ownerReferences"`
    Finalizers []string `json:"finalizers,omitempty" patchStrategy:"merge" protobuf:"bytes,14,rep,name=finalizers"`
    ManagedFields []ManagedFieldsEntry `json:"managedFields,omitempty" protobuf:"bytes,17,rep,name=managedFields"`
}

This problem existed before the upgrade to version 3 this week. Here is the same object missing from the v2 swagger doc, pre upgrade: https://github.com/harvester/harvester/blob/056ec34a91636864465e4bbea671de10a1c894e9/api/openapi-spec/swagger.json#L7762-L7775

[drewmullen]

Looks like the only developers who have worked on this script is @gitlawr , @guangbochen , and @starbops

Do any of yall have any idea why the script might be skipping so many fields for ObjectMeta?

[drewmullen]

Getting closer... looks like some fields are being ignored: https://github.com/harvester/harvester/blob/0802338b72d93cd31c1d85ee1e64fd12ed12713b/pkg/genswagger/rest/definitions.go#L8

[drewmullen]

@chrisho #4444 is now causing issues when we try to use the swagger file. because its incomplete we're not actually able to use it with generated SDKs: https://github.com/drewmullen/harvester-go-sdk/issues/2

I believe this abandoned PR in kube-openapi gen would solve the root issue in #4443

I'm wondering how much work it would be to temporarily remove the ignoreFieldsObjectMeta so we can have a working swagger file?

[w13915984028]

@Yu-Jack @m-ildefons Are you working on the related topics recently?

[drewmullen]

I have also discovered that requiring name can cause problems.

https://github.com/harvester/harvester/blob/0802338b72d93cd31c1d85ee1e64fd12ed12713b/pkg/genswagger/rest/definitions.go#L44

For example, kubevirt's spec.template.metadata uses the same ObjectMeta as the top level metadata object: https://github.com/kubevirt/api/blob/main/core/v1/types.go#L47

however, at least in my testing, the ListNamespacedVirtualMachine does not return a name. Here is an incomplete sample of my payload where no name was passed.

"spec": {
                "runStrategy": "RerunOnFailure",
                "template": {
                    "metadata": {
                        "annotations": {
                            "harvesterhci.io/sshNames": "[\"default/test\"]"
                        },
                        "creationTimestamp": null,
                        "labels": {
                            "harvesterhci.io/vmName": "imagetest"
                        }
                    },

If you want to require name in the top level metadata payload - thats probably better to do via validation or custom json marshaler. I'm not exactly where that would be in yall's codebase.

[m-ildefons]

@drewmullen a lot of the ObjectMeta fields aren't meant to be set by the user as they will be updated by one of the K8s controllers. E.g. CreationTimestamp, DeletionTimestamp, ResourceVersion, and more. Keep in mind that some of these fields are also mutually exclusive. Afaik the OpenAPI spec is primarily used to generate documentation, thus it's a good thing these fields are removed as this de-clutters the docs by hiding fields that shouldn't be touched by users.

I think it's right that ListNamespacedVirtualMachine doesn't return a .metadata.name property within its .spec.template property, since this is a template and not yet instantiated. The resulting VirtualMachineInstance object should have a .metadata.name property at its toplevel. That being said, it's indeed not correct to always require a name in the ObjectMeta structure and this is also documented upstream, as it can be replaced e.g. with a .metadata.generateName or as in your example may be omitted entirely in a template. As a result, the documentation actually falsely specifies this property as required in the VirtualMachine resource's .spec.template.metadata structure.

@w13915984028 yes, I've been digging into API documentation (and its bugs) these days. I needed to use the API and looking things up in the docs made it obvious, that our API docs haven't seen a whole lot of love recently.

[drewmullen]

@m-ildefons Hi and thanks for taking the time to reply. Its good to know that you're only currently using the OpenAPI Spec (OAS) doc for documentation. Let me tell you a little about my intended use and why these missing fields are a problem.

There are actually several other uses for OAS docs! For example, OAS docs can be used to for code generation. Using your OAS doc you can easily provide client libraries for many languages. I wanted to interact with Harvester via go client but it looks like the one provided by the team is no longer updated. For that reason we looked to using the oas doc to generate a client, which I've done successfully!

As I'm sure you can understand that the client will not work if the models are incomplete. The missing fields in ObjectMeta prevented me from fully unmarshaling payloads to go structs in items[#].metadata. The hard requirement of name was preventing me unmarshaling payloads in spec.template.metadata (which doesnt return a name). Hence my issue and subsequent PR #5512

Now... to the reason you deleted those fields. Is the documentation above from kubevirt or from harvester? OAS does have a notion of readOnly. However, I believe that would cause similar strange issues because the ObjectMeta type is used by several kubevirt primitives... If the docs are from kubevirt, have you considered opening an issue with them to have them consider a fix?

[drewmullen]

I added readOnly to the creationTimestamp property to see if it would effect much. In my initial tests it doesnt seem to effect the generated SDK. It also seems to update the docs for the file. Not sure if this change would filter through to the docs you showed above... can you comment on that?

Here is a PR to show the comparison: https://github.com/drewmullen/harvester-go-sdk/pull/3/files

Obviously, if we were going to implement in harvester/harvester we would have to edit the genswagger package but i just wanted to test it as easily as possible first

[Yu-Jack]

@w13915984028 I opened an issue related to namespace https://github.com/harvester/harvester/issues/5497, some APIs don't need the namespace, but it still shows required in the documentation. That's because there are few issue in generator in our repo.

[m-ildefons]

The ObjectMeta type originates from the Kubernetes API meta/v1 namespace. It is used by every K8s object to store metadata.

The OAS docs are not the source of truth though, they are generated from the API object type definitions (which might be the wrong way around). This is probably because the docs need to include parts of the API which are sourced from different projects (e.g. KubeVirt). Perhaps there are also other reasons for doing it this way.

The Harvester API docs are quite strange anyways and I'm not 100% sure they are referring to the K8s API, or if there is a different API endpoint for them. They certainly aren't always correct, e.g. kubevirt.io/v1/VirtualMachine.spec.template.spec.domain is a required property, but kubevirt.io/v1/VirtualMachine.spec.template.spec isn't, so I'm not sure what's going on there.

I can't tell you why the go-harvester client library isn't maintained anymore, but I understand that it causes a big pain for you. Note that Harvester's K8s API is quite complicated as it relies on API object from other projects as well, e.g. K8s itself and KubeVirt. So you'll have to be careful generating an API client library from the swagger file, as you don't want to include the parts of the API from K8s or KubeVirt, which already have a good client library and which probably aren't pictured accurately in the OAS definition file. I'm not sure how your fix for the missing API fields is going to work. Staying with your example of listing VirtualMachine objects and not having the .spec.template.metadata.name field, if the fields doesn't exist on the API server, the reply from the API wouldn't contain that field, so having it in the OAS doc wouldn't help, right? That being said, it's probably indeed incorrect to require this .metadata.name field as K8s has different mechanisms, like .metadata.generateName as well.

Hope you can make some sense out of these monday-morning ramblings ;-)

[drewmullen]

Morning and happy monday, @m-ildefons & @Yu-Jack !

you don't want to include the parts of the API from K8s or KubeVirt, which already have a good client library and which probably aren't pictured accurately in the OAS definition file

So my plan is to use this harvester client to manage image builds using HashiCorp Packer Plugin (which im writing). That means Harvester's API is the only interface I'll interact with. So even if some of the models are fed from upstream sources, all I'll care about is Harvesters inputs / outputs. Keeping that in mind, you may notice the issues i've opened up had to deal with the harvester swagger doc not accurately defining either inputs or outputs from a specific API.

Ok so lets summarize... we all agree:

1. The docs are currently not accurate: a. namespace parameter is being applied to every list action, even when not appropriate b. ObjectMeta is missing most of the fields it needs, resulting in incomplete response definitions in docs (and downstream generated code) c. name should not be a required property for ObjectMeta
2. We want the docs to be as accurate as possible

Based on these 2 assertions, do we have enough agreement to merge #5512 and we can look at how the docs come out after that? Side question... is it possible to generate and host the docs locally before theyre published?

[bk201]

Side question... is it possible to generate and host the docs locally before theyre published?

I usually just clone https://github.com/harvester/docs and run a local dev server (note it takes a while to build API pages).

yarn install
yarn start -h <bind-IP>

[m-ildefons]

the issues i've opened up had to deal with the harvester swagger doc not accurately defining either inputs or outputs from a specific API.

:disappointed: the docs are indeed in a bit of a sorry state. We maintain the contents in harvester/docs. The readme file there gives a few hints how to get started with a local dev environment. I'm not sure how familiar with Docusaurus or the JavaScript ecosystem you are, but there are additional command lines for re-generating the API docs specifically: yarn gen-api-docs and yarn clean-api-docs

[drewmullen]

Sorry to beat up on #4444 but I found another unforseen consequence of enforcing apiVersion and kind as required. ReadVirtualMachineImage does not return kind or apiVersion. Heres the pic for the docs saying its required but those fields are NOT returned by the API

[m-ildefons]

I have to admit, marking a field in a response as "required" is kind of weird, but they are returned:

│ ~ │► kubectl -n default get virtualmachineimages image-mtcdd -oyaml
apiVersion: harvesterhci.io/v1beta1
kind: VirtualMachineImage
metadata:
  annotations:
    harvesterhci.io/storageClassName: harvester-longhorn
  creationTimestamp: "2024-04-12T07:05:48Z"
  finalizers:
  - wrangler.cattle.io/vm-image-controller
  generateName: image-
  generation: 40
  labels:
    harvesterhci.io/image-type: raw_qcow2
    harvesterhci.io/os-type: openSUSE
  name: image-mtcdd
  namespace: default
  resourceVersion: "9053717"
  uid: d17d5d7a-bf0c-4920-bd70-a4263841b3cd
spec:
  checksum: ""
  displayName: microos
  pvcName: ""
  pvcNamespace: ""
  retry: 3
  sourceType: download
  storageClassParameters:
    migratable: "true"
    numberOfReplicas: "3"
    staleReplicaTimeout: "30"
  url: https://download.opensuse.org/tumbleweed/appliances/openSUSE-MicroOS.x86_64-ContainerHost-OpenStack-Cloud.qcow2
status:
  appliedUrl: https://download.opensuse.org/tumbleweed/appliances/openSUSE-MicroOS.x86_64-ContainerHost-OpenStack-Cloud.qcow2
  conditions:
  - lastUpdateTime: "2024-04-12T07:06:00Z"
    reason: Importing
    status: Unknown
    type: Imported
  - lastUpdateTime: "2024-04-12T07:06:00Z"
    reason: Initialized
    status: "True"
    type: Initialized
  failed: 1
  lastFailedTime: "2024-04-12T07:05:49Z"
  progress: 50
  size: 494665728
  storageClassName: longhorn-image-mtcdd

Also works in the API explorer:

https://github.com/harvester/harvester/assets/17141774/24a2a673-488f-4eb0-bf78-cc8323d66518

[drewmullen]

You're absolutely right. I'm sorry about that last one @m-ildefons

Little jumpy on doc cleanup right now! I have a hammer and everything looks like a nail 🤣 / 😭