firecracker icon indicating copy to clipboard operation
firecracker copied to clipboard

Consider specifying /mmds get schema as `string` rather than `object`

Open samuelkarp opened this issue 4 years ago • 5 comments

Hello!

Currently the MMDS get operation is defined as follows:

  /mmds:
    #  ...snip...
    get:
      summary: Get the MMDS data store.
      responses:
        200:
          description: The MMDS data store JSON.
          schema:
            type: object
        400:
          description: Cannot get the MMDS data store due to bad input.
          schema:
            $ref: "#/definitions/Error"
        default:
          description: Internal server error
          schema:
            $ref: "#/definitions/Error"

The schema is defined as object, which is parallel to the schema of the put and patch operations as well. In the Firecracker SDK for Go, we've received a pull request to add a GetMetadata method to retrieve the metadata from the MMDS. However, the definition of the schema as object (and the way that go-openapi generates a client) makes that difficult to work with.

When object is defined, our Swagger client generator gives us an interface{}, which is Go's equivalent of a void pointer; we then need figure out some way of converting that to a useful value. One mechanism is to implement dynamic type discovery, which is a lot of boilerplate and error-prone code. Another is to send this back through another serialization-deserialization round-trip to use Go's built-in JSON decoding, but that will be inefficient especially for large objects. Or there are other approaches that involve digging beneath the Swagger client library's abstractions.

Would you be willing to change this to a string (and format: binary) instead of object? If this were defined as a string, we would get the serialized data back and can deserialize it ourselves instead of needing these workarounds.

If this isn't a change you'd be willing to make, we do have some options:

  1. Dynamic type discovery
  2. Reserialize-deserialize round-trip
  3. Patch the swagger spec ourselves
  4. Implement a separate Client with a separate runtime.ClientResponseReader just for this operation

But these approaches are all more complicated, and (though I haven't looked) I suspect that other Swagger client generators in other programming languages will have similar challenges with a returned object.

Thanks, Sam

samuelkarp avatar Aug 26 '19 22:08 samuelkarp

@samuelkarp We are using type object on all requests, not just MMDS. The code that parses requests from client into json is generic and the same for all requests. Changing only one of the request would introduce messy code in the api_server (which we also work on changing any way).

Considering this, I would like us to avoid changing the request type at least until we replace the api_server with our custom implementation. I do find it a bit weird that it's so hard to get a JSON from type object because reading through the Swagger documentation this is the only way I can find to represent a JSON object. Isn't this a miss of the Go SDK?

andreeaflorescu avatar Sep 02 '19 10:09 andreeaflorescu

We are using type object on all requests, not just MMDS.

As far as I can tell, /mmds is the only API that directly uses type: object. All other usage of type: object is in defining the structure of a specific object in the definitions section.

I do find it a bit weird that it's so hard to get a JSON from type object because reading through the Swagger documentation this is the only way I can find to represent a JSON object.

Does MMDS depend on it specifically being JSON? My understanding is that MMDS is accepting and serving arbitrary data.

Isn't this a miss of the Go SDK?

It's specifically an implementation detail of the go-openapi/go-swagger client generator. The Firecracker Go SDK is an additional library on top of the generated client.

However, I suspect that other Swagger client generators (in other languages) are going to have similar issues.

samuelkarp avatar Sep 03 '19 17:09 samuelkarp

I do find it a bit weird that it's so hard to get a JSON from type object because reading through the Swagger documentation this is the only way I can find to represent a JSON object.

For the other type: object types that are specified in the spec, the client generator is able to generate real types and deserialize the JSON into objects of those types. However, when we look at the MMDS, the lack of defined fields for the type: object return value means that the client generator can't generate types and instead we get an interface{}, which is not usable without additional work.

Yes, we can handle this in the Firecracker Go SDK through one of the workarounds I listed above. But we suspect that other clients in other languages will need similar workarounds, and we'd prefer to not use them. If the MMDS is really holding arbitrary data/arbitrary bytes, can it just return those bytes and let the client deal with it?

samuelkarp avatar Sep 03 '19 17:09 samuelkarp

I came around this while looking at this issue: https://github.com/firecracker-microvm/firecracker-go-sdk/issues/109, which was fixed by this PR: https://github.com/firecracker-microvm/firecracker-go-sdk/pull/114.

@samuelkarp , I am wondering if this issue is still relevant?

Related to your previous question:

If the MMDS is really holding arbitrary data/arbitrary bytes, can it just return those bytes and let the client deal with it?

MMDS is not holding arbitrary data/arbitrary bytes, but valid JSON structured bytes.

iulianbarbu avatar Apr 22 '20 16:04 iulianbarbu

@kzys is this still of interest for go-sdk?

raduiliescu avatar Feb 24 '22 17:02 raduiliescu

We have not heard back from so closing this for the moment. @kzys @samuelkarp feel free to re-open if necessary. Thanks!

dianpopa avatar Apr 04 '23 08:04 dianpopa