Add support for google.api.VisibilityRules annotations to hide APIs and fields

[BCook98]

References to other Issues or PRs

#1791

Have you read the Contributing Guidelines?

Yes

Brief description of what is fixed or changed

This PR adds support for annotations to set the visibility restrictions of fields, services, methods and enum values. These rules can be used in combination with a new option visibility_restriction_selectors to hide fields and APIs from the generated API output.

Google's google.api.VisibilityRules are used as a standard annotation.

This is useful when generating documentation where protobuf APIs are used both internally and externally, where some fields may need to be hidden. Or if an API field isn't fully baked yet it can be tagged with BETA or PREVIEW and only generated to internal or private documentation.

Example

opt: visibility_restriction_selectors=PREVIEW will result in:

Input Example:

service Echo {
    rpc EchoInternal(VisibilityRuleSimpleMessage) returns (VisibilityRuleSimpleMessage) {
        option (google.api.method_visibility).restriction = "INTERNAL";
        option (google.api.http) = {
            get: "/v1/example/echo_internal"
        };
    }
    rpc EchoInternalAndPreview(VisibilityRuleSimpleMessage) returns (VisibilityRuleSimpleMessage) {
        option (google.api.method_visibility).restriction = "INTERNAL,PREVIEW";
        option (google.api.http) = {
            get: "/v1/example/echo_internal_and_preview"
        };
    }
}

message VisibilityRuleSimpleMessage {
     enum VisibilityEnum {
          UNSPECIFIED = 0;
          VISIBLE = 1;
          INTERNAL = 2 [(google.api.value_visibility).restriction = "INTERNAL"];
          PREVIEW = 3 [(google.api.value_visibility).restriction = "INTERNAL,PREVIEW"];
     }
     
     string internal_field = 1 [(google.api.field_visibility).restriction = "INTERNAL"];
     string preview_field = 2 [(google.api.field_visibility).restriction = "INTERNAL,PREVIEW"];
     VisibilityEnum an_enum = 3;
}

Output json:

{
    "paths": {
        "/v1/example/echo_internal_and_preview": {
            "get": {
                "summary": "EchoInternalAndPreview is a internal and preview API that should be visibile in the OpenAPI spec.",
                "operationId": "VisibilityRuleEchoService_EchoInternalAndPreview",
                "responses": {
                    "200": {
                        "description": "A successful response.",
                        "schema": {
                        "$ref": "#/definitions/examplepbVisibilityRuleSimpleMessage"
                        }
                    },
                    "default": {
                        "description": "An unexpected error response.",
                        "schema": {
                            "$ref": "#/definitions/rpcStatus"
                        }
                    }
                },
                "parameters": [
                    {
                        "name": "previewField",
                        "in": "query",
                        "required": false,
                        "type": "string"
                    },
                    {
                        "name": "anEnum",
                        "in": "query",
                        "required": false,
                        "type": "string",
                        "enum": [
                            "UNSPECIFIED",
                            "VISIBLE",
                            "PREVIEW"
                        ],
                        "default": "UNSPECIFIED"
                    }
                ],
                "tags": [
                    "VisibilityRuleEchoService"
                ]
            }
        }
    }
}

Other comments

This PR doesn't add support for selectors in visibility rules, just using them as annotations. See https://github.com/googleapis/googleapis/blob/master/google/api/visibility.proto#L71 for more detail. This is useful if you want to provide an external list of rules.

Support is also missing for adding annotations to messages and enums, this was excluded due to complexity and potential edge cases when excluding a type that is required somewhere else in the OpenAPI Schema.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

[johanbrandhorst]

This is very cool, I will need some time to review, but this is a commonly requested feature. Thanks!

[johanbrandhorst]

Closing and reopening to trigger CI runs

[johanbrandhorst]

Closing and reopening to trigger CI... not sure why this is happening 😬.

[johanbrandhorst]

This is a really top notch contribution, even including docs 😍. I have some initial questions:

• It appears that the visibility labels themselves can be arbitrary strings, both in these examples and in the Google protobuf sources. Because of this, I think we need to make it clear that there is no implicit hierarchy in the labels (which it seemed like to me initially), and the visibility selector will only match those rules that are provided exactly.
• Another consequence of this is that unless you intend to invoke the plugin with different options, you only really need the INTERNAL label, probably. Maybe we should have the example include an invocation without a selector, with INTERNAL and with PREVIEW respectively. We might also need an example with more than one selector. Maybe only one with more than one selector actually. What do you think?

[johanbrandhorst]

I don't see us doing any case folding anywhere, which means that this matching is case sensitive, which I think makes sense, just wanted to check that this was intentional?

[BCook98]

Yep this is intentionally case sensitive. Though there is no real justification for it being that way, I can't someone wanting internal and INTERNAL to mean different things.

But something doesn't feel right about making this case insensitive.

[BCook98]

Thanks for your feedback, much appreciated 🙏

Let me take some time to revisit the docs and make it clear that these selectors are completely arbitrary and add some more examples around it!

---

I may need your help with Bazel and those imports... From what I can see I don't think I am doing the wrong thing with https://github.com/grpc-ecosystem/grpc-gateway/pull/2578/files#diff-c5fc7051bb345b50fbe0e92d76c0eadf61359cb387e689308ecc1ade85cf96c7R106. In fact bazel test //... requires them in that format over what it is suggesting, otherwise it fails.

[BCook98]

flag doesn't work, the last supplied flag is the one that is used. But using StringArray from https://github.com/spf13/pflag will allow for multiple flags to be supplied.

Happy to also use ; as a delimiter instead if you'd like to avoid adding additional dependencies.

diff --git a/examples/internal/proto/examplepb/visibility_rule_preview_and_internal_echo_service.buf.gen.yaml b/examples/internal/proto/examplepb/visibility_rule_preview_and_internal_echo_service.buf.gen.yaml
new file mode 100644
index 0000000..0a2a741
--- /dev/null
+++ b/examples/internal/proto/examplepb/visibility_rule_preview_and_internal_echo_service.buf.gen.yaml
@@ -0,0 +1,7 @@
+version: v1
+plugins:
+  - name: openapiv2
+    out: .
+    opt:
+      - visibility_restriction_selectors=PREVIEW
+      - visibility_restriction_selectors=INTERNAL
diff --git a/go.mod b/go.mod
index afa6531..1fc926f 100644
--- a/go.mod
+++ b/go.mod
@@ -16,6 +16,7 @@ require (
 )
 
 require (
+	github.com/spf13/pflag v1.0.5 // indirect
 	golang.org/x/net v0.0.0-20220127200216-cd36cc0744dd // indirect
 	golang.org/x/sys v0.0.0-20211216021012-1d35b9e2eb4e // indirect
 	golang.org/x/text v0.3.7 // indirect
diff --git a/go.sum b/go.sum
index 71b3f14..881fa88 100644
--- a/go.sum
+++ b/go.sum
@@ -133,6 +133,8 @@ github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:
 github.com/rogpeppe/fastuuid v1.2.0 h1:Ppwyp6VYCF1nvBTXL3trRso7mXMlRrw9ooo375wvi2s=
 github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ=
 github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4=
+github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA=
+github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
 github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA=
diff --git a/internal/descriptor/registry.go b/internal/descriptor/registry.go
index 87a2cfe..7be4576 100644
--- a/internal/descriptor/registry.go
+++ b/internal/descriptor/registry.go
@@ -576,9 +576,12 @@ func (r *Registry) GetOmitEnumDefaultValue() bool {
 }
 
 // SetVisibilityRestrictionSelectors sets the visibility restriction selectors.
-func (r *Registry) SetVisibilityRestrictionSelectors(selectors string) {
+func (r *Registry) SetVisibilityRestrictionSelectors(selectors *[]string) {
 	r.visibilityRestrictionSelectors = make(map[string]bool)
-	for _, selector := range strings.Split(selectors, ",") {
+	if selectors == nil {
+		return
+	}
+	for _, selector := range *selectors {
 		r.visibilityRestrictionSelectors[strings.TrimSpace(selector)] = true
 	}
 }
diff --git a/protoc-gen-openapiv2/main.go b/protoc-gen-openapiv2/main.go
index 5a290f3..801c59a 100644
--- a/protoc-gen-openapiv2/main.go
+++ b/protoc-gen-openapiv2/main.go
@@ -1,7 +1,6 @@
 package main
 
 import (
-	"flag"
 	"fmt"
 	"os"
 	"strings"
@@ -10,6 +9,7 @@ import (
 	"github.com/grpc-ecosystem/grpc-gateway/v2/internal/codegenerator"
 	"github.com/grpc-ecosystem/grpc-gateway/v2/internal/descriptor"
 	"github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv2/internal/genopenapi"
+	flag "github.com/spf13/pflag"
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/types/pluginpb"
 )
@@ -37,7 +37,7 @@ var (
 	generateUnboundMethods         = flag.Bool("generate_unbound_methods", false, "generate swagger metadata even for RPC methods that have no HttpRule annotation")
 	recursiveDepth                 = flag.Int("recursive-depth", 1000, "maximum recursion count allowed for a field type")
 	omitEnumDefaultValue           = flag.Bool("omit_enum_default_value", false, "if set, omit default enum value")
-	visibilityRestrictionSelectors = flag.String("visibility_restriction_selectors", "", "comma separated list of `google.api.VisibilityRule` visibility labels to include in the generated output when a visibility annotation is defined. Elements without visibility annotations are unaffected by this setting.")
+	visibilityRestrictionSelectors = flag.StringArray("visibility_restriction_selectors", []string{}, "comma separated list of `google.api.VisibilityRule` visibility labels to include in the generated output when a visibility annotation is defined. Elements without visibility annotations are unaffected by this setting.")
 )
 
 // Variables set by goreleaser at build time
@@ -115,7 +115,7 @@ func main() {
 	reg.SetGenerateUnboundMethods(*generateUnboundMethods)
 	reg.SetRecursiveDepth(*recursiveDepth)
 	reg.SetOmitEnumDefaultValue(*omitEnumDefaultValue)
-	reg.SetVisibilityRestrictionSelectors(*visibilityRestrictionSelectors)
+	reg.SetVisibilityRestrictionSelectors(visibilityRestrictionSelectors)
 	if err := reg.SetRepeatedPathParamSeparator(*repeatedPathParamSeparator); err != nil {
 		emitError(err)
 		return

[johanbrandhorst]

Instead of adding a dependency, what about a custom flag variable like this: https://stackoverflow.com/a/28323276? I think it'll be important for us to emphasize how to use multiple values correctly (remove any mention of comma separation from all the docs).

[johanbrandhorst]

Suggested change

	// Elements with an overlap on `visibility_restriction_selectors`, those without are not visible.
	// Elements with no visibility rule or elements with an overlap on `visibility_restriction_selectors` are visible, while those with no overlap are not visible.

[johanbrandhorst]

Almost there, just a few nits left!

[johanbrandhorst]

LGTM 🎉

[BCook98]

Thanks for the feedback! I've addressed all of those comments.

Just running a quick manual test on our API protos to make sure it's all still working as expected.

[johanbrandhorst]

Looks like we need another generation after the master merge

[BCook98]

I believe I have fixed all of the test/lint failures.

Also a manual test across our APIs gives us the expected output 🎉.

[johanbrandhorst]

🤞🏻

[johanbrandhorst]

Thank you for this incredible contribution ❤️