gnostic icon indicating copy to clipboard operation
gnostic copied to clipboard

Support buf build deps

Open renyijiu opened this issue 3 years ago • 22 comments
trafficstars

In the example code of protoc-gen-openapi, I see that option (openapi.v3.documentation) is already supported, but when using it in the proto file of other projects, the user needs to manually introduce import "openapiv3/annotations.proto"; associated annotations.proto file to be able to compile successfully. Can you consider uploading it to buf.build, similar to https://buf.build/googleapis/googleapis, to make it easier for other users to introduce dependencies for use, thanks to this great project.

#buf.yaml

version: v1
deps:
    - buf.build/googleapis/googleapis
lint:
  use:
    - DEFAULT
breaking:
  use:
    - FILE

renyijiu avatar Apr 06 '22 12:04 renyijiu

I would also like to have this, though as far as I know https://buf.build/googleapis/googleapis is actually maintained by buf themselves, and not google. I'm not sure if google has, or ever will, provide stuff on buf.

Though, given that @timburks gave your request a thumbs up, I remain hopeful that this might actually happen. :)

jeffsawatzky avatar Apr 06 '22 23:04 jeffsawatzky

I've been aware of buf but not using it. I'm not surprised that the googleapis repo on buf is maintained by buf directly, but I've been curious about buf so this seems like a good opportunity to try it. I've signed into buf and found that I could create a gnostic organization. Looking at googleapis as an example, it seems that we would want to have a "gnostic" repo with subdirectories for "openapiv3", etc.

I've uploaded a draft repo to buf.build/gnostic/gnostic. Please take a look and, if you're a buf user, please try it and comment here. To pass the buf linter, I made some changes to file names and paths that you can review at timburks/gnostic/tree/buf/buf.

timburks avatar Apr 09 '22 17:04 timburks

I think the change in the package name is good (as I think it follows proto naming conventions more closely), though it means that we can't use those options with protoc-gen-openapi anymore. Is your intention that these would be the new package names going forward and used everywhere?

jeffsawatzky avatar Apr 09 '22 23:04 jeffsawatzky

I add deps in buf.yaml

# buf.yaml

deps:
  - buf.build/gnostic/gnostic

and buf.gen.yaml is as follows.

# buf.gen.yaml
version: v1beta1
plugins:
  - name: openapi
    out: gen/openapi
    strategy: all
    opt:
      - title=Openapiv3

Add the relevant definitions to the proto file

import "gnostic/openapi/v3/annotations.proto";

option (gnostic.openapi.v3.document) = {
  info: {
    title: "Title from annotation";
    version: "Version from annotation";
    description: "Description from annotation";
    contact: {
      name: "Contact Name";
      url: "https://github.com/google/gnostic";
      email: "[email protected]";
    }
    license: {
      name: "Apache License";
      url: "https://github.com/google/gnostic/blob/master/LICENSE";
    }
  }
  components: {
    security_schemes: {
      additional_properties: [
        {
          name: "BasicAuth";
          value: {
            security_scheme: {
              type: "http";
              scheme: "basic";
            }
          }
        }
      ]
    }
  }
};

Run the buf generate command, then you can generate openapi.yaml, but it does not take effect, there is no security_schemes in the yaml file. If you change it to this option (openapi.v3.document), buf generate will report an error: unknown extension openapi.v3.schema. I don't know how to handle this, but I've provided a simple demo to test and verify.

renyijiu avatar Apr 10 '22 12:04 renyijiu

@renyijiu its because protoc-gen-openapi is built against a different version of the protos which use a different package name. Right now you can't use the ones on buf.build with protoc-gen-openapi...unless you build your own version of it using the protos on buf.build.

For now I've just copied the OpenApiv3 protos and annotations into its own local buf module.

jeffsawatzky avatar Apr 10 '22 21:04 jeffsawatzky

I also can't use proto-gen-openapi to generate openapi.v3.document property in the openapi.yaml. Why is this?@jeffsawatzky @renyijiu

yzhaoyu avatar Apr 23 '22 05:04 yzhaoyu

I'd like to move this repo to the proto structure in buf, or something similar that's better aligned with common practice for larger-scale proto-based projects. Because kubectl pulls protos from this repo, I've been wanting to wait until we had an alternate source for them to use for their gnostic protos, which I've just set up in github.com/google/gnostic-models -- I've previously gotten complaints about changes despite encouraging clients to used tagged versions, so this isolated distribution seemed best, and also addresses concerns about dependencies discussed in #317.

I've passed information about the new repo to the kubectl team, and when that gets updated, I think we can safely restructure the protos in this repo.

timburks avatar Apr 26 '22 17:04 timburks

I just tried this and really love it. I wasn't able to use protoc-gen-openapi as part of buf generate pipeline and had to roll 'protoc' manually.

Could protoc-gen-openapi be published as a buf plugin? Then generating openapi specs would be as trivial as:

# buf.gen.yaml
plugins:
- remote: buf.build/gnostic/plugins/openapiv3:v0.6.8
  out: gen/openapi
  opt:
  - fq_schema_naming=true
  - naming=proto
  - enum_type=string

rauanmayemir avatar Sep 06 '22 12:09 rauanmayemir

This is largely working on my end with the buf.build repo, thanks @timburks

However, since it hasn't been updated in a while it doesn't have this fix, which is causing issues for me. https://github.com/google/gnostic/pull/344 any chance the fork can be updated with the main branch here?

What would it take to get this to a more mature state? My team is starting to get into Buf pretty heavily, and generating OpenAPI v3 schemas would be really helpful.

mgebundy avatar Sep 20 '22 21:09 mgebundy

@timburks any chance you can push the latest updates to https://buf.build/gnostic/gnostic? Similar to the previous comment, I believe I'm running into an issue fixed by https://github.com/google/gnostic/pull/344.

When I execute buf generate I get an invalid import in the generated protobuf files.

import (
	_ "./openapiv3"
	_ "google.golang.org/genproto/googleapis/api/annotations"
...

The correct import is

import (
	_ "github.com/google/gnostic/openapiv3"
	_ "google.golang.org/genproto/googleapis/api/annotations"
...

I am getting around this currently by sed replacing the import post generation.

ericdstein avatar Mar 28 '23 03:03 ericdstein

@timburks I noticed that when we install protoc-gen-openapi using:

go get github.com/google/gnostic/cmd/protoc-gen-openapi

We can use the proto definitions from buf, the ones that use gnostic.openapi.v3 as the package name, for the options in the protocol file (so I can add extra options).

However, looking at this repo, it is still using the openapi.v3 package name. So I am very confused as to how this is working for us. Did you make a special build using updated packages? I'm clearly missing something.

EDIT: My best guess is that since we are using buf to generate the openapi, and buf uses the buf.build definitions to pass to protoc, that protoc is able to parse the proto file using the buf package names, and when the openapi plugin gets the input it is already pre-parsed and uses something other than the package name to look up the extensions...like the field id perhaps?

In other words, here where it looks up the extensions: https://github.com/google/gnostic/blob/main/cmd/protoc-gen-openapi/generator/generator.go#L124 It does so by something other than package name, perhaps the Field 1143 from here: https://github.com/google/gnostic/blob/main/openapiv3/annotations.pb.go#L75

jeffsawatzky avatar Apr 11 '23 20:04 jeffsawatzky

@jeffsawatzky I don't recall making any special builds. I think these other definitions are working because their field numbers and types are the same, so deserializing with those protos works the same as the originals. AFAIK, the package name isn't used anywhere in the serialized protos (i.e. the binaries often stored as .pb files).

timburks avatar Apr 13 '23 23:04 timburks

Catching up (and being dense), assuming the go_package change in the two protos in #344 is what we need to push to buf...

timburks avatar Apr 13 '23 23:04 timburks

Yeah I've been using tx7do's bsr instead and it works fine:

https://buf.build/tx7do/gnostic/file/main:gnostic/openapi/v3/openapiv3.proto#L45

jrc2139 avatar Apr 14 '23 00:04 jrc2139

How's this? https://buf.build/timburks/gnostic-test

If this looks good I'll push it to gnostic/gnostic

timburks avatar Apr 14 '23 00:04 timburks

I'll take that as LGTM - pushed to https://buf.build/gnostic/gnostic. All four protos were updated.

timburks avatar Apr 14 '23 00:04 timburks

Sorry to dig this out -- I've been working with, and noticed reflection does not work in my buf setup (mostly using https://github.com/SchwarzIT/go-template):

$ grpcurl -plaintext -vv localhost:9090 list api.v1.ApiService
Failed to list methods for service "api.v1.ApiService": Symbol not found: api.v1.ApiService
caused by: File not found: gnostic/openapi/v3/annotations.proto

after some debugging, I found out that the file needs to be imported with import "gnostic/openapi/v3/annotations.proto" according to your buf.build push.

However, in the protobuf reflection API, it registers itself as openapiv3/annotations.proto. I can verify that by using the reflection package directly:

protoregistry.GlobalFiles.FindFileByPath("openapiv3/annotations.proto") # works
protoregistry.GlobalFiles.FindFileByPath("gnostic/openapi/v3/annotations.proto") # worksn't 🙁

This seems far-fetched, but maybe we should align these import paths? It's a bit unfortunate, since I also haven't found any way yet to re-map them manually as a workaround.

cfstras avatar Mar 14 '24 19:03 cfstras

Thanks, @cfstras, this helped me debug the issue where my gRPC client was unable to connect to my server using reflect. I published the OpenAPI specs under the correct path in my buf repo (buf.build/jeroenrinzema/openapi). This dependency resolved the reflection issue on my side.

jeroenrinzema avatar Apr 04 '24 14:04 jeroenrinzema

As another workaround, I figured I can keep using gnostic if I omit the option (gnostic.openapi.v3.*) annotations from my proto definitions. This makes reflection work, and I don't particularly need the extra description fields they offer at this point.

cfstras avatar Apr 05 '24 12:04 cfstras

I add deps in buf.yaml

# buf.yaml

deps:
  - buf.build/gnostic/gnostic

and buf.gen.yaml is as follows.

# buf.gen.yaml
version: v1beta1
plugins:
  - name: openapi
    out: gen/openapi
    strategy: all
    opt:
      - title=Openapiv3

Add the relevant definitions to the proto file

import "gnostic/openapi/v3/annotations.proto";

option (gnostic.openapi.v3.document) = {
  info: {
    title: "Title from annotation";
    version: "Version from annotation";
    description: "Description from annotation";
    contact: {
      name: "Contact Name";
      url: "https://github.com/google/gnostic";
      email: "[email protected]";
    }
    license: {
      name: "Apache License";
      url: "https://github.com/google/gnostic/blob/master/LICENSE";
    }
  }
  components: {
    security_schemes: {
      additional_properties: [
        {
          name: "BasicAuth";
          value: {
            security_scheme: {
              type: "http";
              scheme: "basic";
            }
          }
        }
      ]
    }
  }
};

Run the buf generate command, then you can generate openapi.yaml, but it does not take effect, there is no security_schemes in the yaml file. If you change it to this option (openapi.v3.document), buf generate will report an error: unknown extension openapi.v3.schema. I don't know how to handle this, but I've provided a simple demo to test and verify.

I tried it again and now openapi.yaml is generated correctly, with fields such as security_schemes is correct. Thanks the great work! a simple demo to test and verify.

renyijiu avatar May 10 '24 09:05 renyijiu