Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Are we using OpenAPI? #166

Closed
lkatalin opened this issue Nov 18, 2022 · 7 comments
Closed

Are we using OpenAPI? #166

lkatalin opened this issue Nov 18, 2022 · 7 comments
Labels
question Further information is requested

Comments

@lkatalin
Copy link
Contributor

Quick question about our rekor submodule... a lot of the files (example here) have a header stating they are generated by OpenAPI, meaning they should not be edited manually. But I don't see any .yaml files included at all. Are we meant to have an openapi.yaml file somewhere to do this? Or are we intending to edit these files manually, and the header was left in by mistake?

@lkatalin lkatalin added the question Further information is requested label Nov 18, 2022
@lukehinds
Copy link
Member

lukehinds commented Nov 19, 2022

We should be, ideally that is, but its a bit of a dumpster file and needed some post generation manual intervention.

I recommend we maybe wait on the protobuf templates being available when I think we can scrap openapi

@znewman01 , can you sanity check my above statements?

@znewman01
Copy link

but its a bit of a dumpster file

Heartily endorse that conclusion 😂


There are three (currently AFAICT) potential sources of generated code for a typical Sigstore client (including sigstore-rs):

  1. The Rekor API client: currently specified using OpenAPI, in the Rekor repo
  2. The Fulcio API client: currently specified using proto+gRPC, in the Fulcio repo
  3. "Protobuf specs": the "bundle format", "trust root", and others: currently specified using proto

Long-run, what we'd like is for each of these sources to provide a generated proto client released in a language-idiomatic way (we're in the process of doing this for protobuf-specs for Python and Java, and would accept a Rust PR; that said, it's not urgent). So for Rust, you'd be able to add a dependency on sigstore-rekor-gen, sigstore-fulcio-gen, and sigstore-base-gen (or something, names TBD) to Cargo.toml and have access to those clients.

You will be able to hand-code clients against these specs using JSON etc., but I don't recommend it. These will be generated code clients, and not terribly idiomatic to use; I'd expect sigstore-rs to expose (maybe as a separate crate, maybe as part of one big crate) Rekor and Fulcio clients that were a little bit nicer.

We also want the Rekor API to move over to gRPC+proto for continuity here. However, I expect it to be difficult to do that while keeping compatibility with OpenAPI, so I don't imagine that will happen until Rekor API v2.


I would probably advise in the short term that we keep any kludgy thing that works here, and that as we cut everything over to the "long-term" solution of language-idiomatic libraries containing generated client code that's when you'd cut out those hacks.

Hopefully that makes sense, we can consider alternatives if not

@lkatalin
Copy link
Contributor Author

Thank you for the informative discussion! I appreciate the level of detail, which will help guide the work on this stuff. I opened a sigstore-rs-specific issue for it here.

It seems there are actually two levels of problems:

  1. OpenAPI is not great and we'd like to move away from it in the future
  2. Assuming that OpenAPI was used to generate the rekor-rs models, which seems to be the case, we have no way to update the models since we lack the original .yaml file for some reason

Re: the latter, I can think of the following courses of action:

  • Update the models manually, ignoring OpenAPI
  • Try to reverse engineer the .yaml file, or write a new .yaml file (this may break a lot of functionality that is currently working)
  • Put a freeze on updating the models until we move away from OpenAPI

Any thoughts?

Some of the work on better error handling in #116 is affected by this.

@znewman01
Copy link

Assuming that OpenAPI was used to generate the rekor-rs models, which seems to be the case, we have no way to update the models since we lack the original .yaml file for some reason

I'm a little confused by this. Do they not match sigstore/rekor:openapi.yaml (or maybe an earlier version of the same)?

If they don't, my vote is to try to generate the models from that YAML file.

@lkatalin
Copy link
Contributor Author

@znewman01 That actually makes sense, as both versions of Rekor would need to have the same interface and structures. I think it threw me for a loop that there was no copy of the file or pointer to the original in this repo. That does raise an interesting dilemma around making edits to the models for some Rust-specific purpose. It seems like we should leave the models alone for now.

@znewman01
Copy link

That does raise an interesting dilemma around making edits to the models for some Rust-specific purpose. It seems like we should leave the models alone for now.

Definitely let's avoid that 😄 If you need changes made, upstream will probably be willing to accept them as long as they don't break any other clients.

@lkatalin
Copy link
Contributor Author

Sounds good, thanks for the insights @znewman01 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants