Generating Multiple Path Bindings for Single gRPC Call

[jon-whit]

I have a field in a protobuf message called "parent". It looks like this:

message ListRolesRequest {
    // The resource name of the parent resource in one of the following formats:
    // `` (empty string) -- this refers to curated roles.
    // `organizations/{organization_id}`
    // `tenants/{tenant_id}`
    string parent = 1;
  ...
}

And a gRPC method that looks like this:

rpc ListRoles(ListRolesRequest) returns (ListRolesResponse) {
      option (google.api.http) = {
        get: "/auth/iam/roles"
        additional_bindings {
          get: "/auth/iam/{parent=organizations/*}/roles"
        }
        additional_bindings {
          get: "/auth/iam/{parent=tenants/*}/roles"
        }
      };
}

The parent can take the form organizations/<org_id> or tenants/<tenant_id> or it can be empty. I'd like the generated swagger definitions to match all three. For example:

GET /auth/iam/roles
GET /auth/iam/organizations/<org_id>/roles
GET /auth/iam/tenants/<tenant_id>/roles

The swagger generator currently only generates two paths:

GET /auth/iam/roles
GET /auth/iam/{parent}/roles

{parent} only matches a single path parameter, and since the parent can include a slash you can't get a match.

What's the correct way of doing this?

[jon-whit]

Was the use case outlined above addressed in #702, because I just pulled the latest and I don't see any differences in the generated swagger output.

Looping in @ch3rub1m

[ch3rub1m]

@jon-whit #702 is not about additional_bindings. It is for the resource name in path. I pulled the latest and it works well.

By the way, this maybe work in your case:

rpc ListRoles(ListRolesRequest) returns (ListRolesResponse) {
      option (google.api.http) = {
        get: "/auth/iam{parent=*}/roles"
      };
}

[jon-whit]

@ch3rub1m that doesn't even compile with the latest version of protoc. the { and} characters have to be preceded and followed by / characters.

[ch3rub1m]

@jon-whit How about this:

rpc ListRoles(ListRolesRequest) returns (ListRolesResponse) {
      option (google.api.http) = {
        get: "/auth/{parent=iam*}/roles"
      };
}

[jon-whit]

@ch3rub1m sorry man, I feel like we're clashing on this one... The problem with your example above is it doesn't adhere to my needs as stated above.

I need parent to match organizations/<org_id> or tenants/<tenant_id> or an empty string. Your example above would match iam/<something>.

[ch3rub1m]

@jon-whit I understood. I just propose a compromise between current features and what you want.

[jon-whit]

Is there anyone else that can speak to this? I guess I'm not fundamentally understanding why

/auth/iam/{parent=organizations/*}/roles

doesn't get transformed to something like

/auth/iam/organizations/{id}/roles

Could someone help me understand why this design choice was made?

[johanbrandhorst]

I'm afraid that information may be hard to come by, perhaps @achew22 will know?

[jon-whit]

@johanbrandhorst thanks for looping in here. I've been trying to figure out a solution to this issue for a while now, so any assistance is much appreciated :).

Thanks!

[jon-whit]

@achew22 Any ideas here?

[achew22]

@jon-whit unfortunately I don't have any specific ideas here. Step one is going to be create a failing test case. Do you think you could send in a PR that demonstrates this reliably?

[jon-whit]

@achew22 maybe I misunderstand.. The example above would be one such case..? What do you mean?

[hbagdi]

Does the following solve the issue?

grpc-gateway/examples/internal/proto/examplepb/a_bit_of_everything.proto

Lines 500 to 511 in 1dd92c9

	additional_bindings: [
	{
	patch: "/v2/example/a_bit_of_everything/{abe.uuid}"
	body: "abe"
	},
	{
	patch: "/v2a/example/a_bit_of_everything/{abe.uuid}"
	body: "*"
	}
	]
	};
	}

[yinzara]

Is there anyone else that can speak to this? I guess I'm not fundamentally understanding why

/auth/iam/{parent=organizations/*}/roles

doesn't get transformed to something like

/auth/iam/organizations/{id}/roles

Could someone help me understand why this design choice was made?

This follows the design of the Google gRPC guidelines that they use for their own APIs.

https://cloud.google.com/apis/design

The path parameters are described in http.proto and in RFC 6570

By using a "name" based syntax that qualifies all identifiers, it allows fields that reference more than one potential type of entity to specify that.

For example, if you take Google "Resource Manager" API, it has a top level element called Organization, a branch structure called Folder, and a leaf called Project. Project and Folder both have a "parent" field that is the "name" of the Organization or Folder that is the parent for the given resource.
https://github.com/googleapis/googleapis/blob/master/google/cloud/resourcemanager/v3/organizations.proto
https://github.com/googleapis/googleapis/blob/master/google/cloud/resourcemanager/v3/folders.proto
https://github.com/googleapis/googleapis/blob/master/google/cloud/resourcemanager/v3/projects.proto

If they had used IDs everywhere, they would have had a "parent_organization_id" and a "parent_folder_id" fields with some constraint around them. Not a great alternative

Unfortunately the group that manages the OpenAPI specification has not yet agreed to support RFC 6570 so currently they could not support this type of syntax.

See (and related issues):
OAI/OpenAPI-Specification#1459
OAI/OpenAPI-Specification#892

That means that if we would like to support this style of syntax, grpc-gateway would need to translate templated path parameters like "/v1/{name=organizations/}" into a OpenAPI permitted syntax of the path /v1/{name} with a "pattern" of "^organizations/.$" in the protoc-gen-openapiv2 and then properly route when an escaped path is passed.

[johanbrandhorst]

Thanks for all of that context, that makes a lot of sense! In particular the last part is very useful for anyone who wants to tackle this issue - it sounds like it should be totally possible, though we'll need to figure out how to parse these variables in the openapi generator. We obviously do the parsing already in the grpc-gateway generator but I'm not sure if it's there in the openapi one today.

[yinzara]

So I've been reading the source and debugging a lot more recently and reading old issues.

It appears that Issue #702 made a change that doesn't strip the "=organizations/*" from the path parameter if its called "name" or "parent". Specifically line 820 in protoc-gen-openapiv1/internal/genopenapi/template.go, the "isResourceName" function.

The original issue was raised because if you have two endpoints that share the stripped name, they couldn't be represented in OpenAPI as the path and method is the unique identifier for the endpoint.

For example if you had two GET endpoints defined as:
/v1/{name=organizations/*)/roles
and
/v1/{name=folders/*)/roles

Those would both resolve to /v1/{name}/roles which can't be represented in OpenAPI.

So @rcgoodfellow when he implemented the fix, just said "when the path parameter is called 'name' or 'parent', then don't strip the qualifier". I feel like this solution was actually not an appropriate one as it technically is what caused #407 (i.e. we now have an invalid swagger specification because it has path parameters specified using a syntax that OpenAPI does not support (i.e. {name=foo/*} instead of {name}). This makes tools like Swagger UI completely non functional (they can't even determine the name of the placeholder as it violates the standards in OpenAPI.

Unfortunately I can't really think of a way that it could be fixed that isn't some other hack. We could rename the path parameter to a different name (maybe add a _1 suffix) when another endpoint is found with the same stripped name. This would only really be a change to the protoc-gen-openapiv2.

However we still have the problem of tools sending the escaped "/" (i.e. "$2F") based on the swagger specification (as all path parameters have to be escaped before they're replaced). Right now we have the newly added Unescaping configuration however it currently doesn't unescape path segments before it attempts to divide them (Line 268 in runtime/mux.go) so we'd have to do additional work there as well.

[rcgoodfellow]

@yinzara I believe I've been mistagged here, my contributions to this repository to not appear to be related to the issues you are referencing.

[betmix-matt]

Totally apologize @rcgoodfellow , it was @ch3rub1m that changed the lines I was looking at (yours were later in the file).

I've got a PR now #2461

Love some feedback!

[oyvindwe]

I've read this and related issues with great interest, as we also have gRPC APIs that follows the Google gRPC API guidelines with the resource name syntax (e.g. ("/v1/{name=foos/*}") in the HTTP mapping, and would like to automate the generation of OpenAPI. I've tested version 2.7.2, and it seems to be a great step in right direction.

First, I've found what looks like a bug I'm happy to report in a separate issue: If I have multiple services in the same API with similar prefixes (e.g. FooService with /v1/{name=foos/*} and BarService with /v1/{name=bars/*}), and these services have multiple methods for these paths (e.g. both GET and DELETE), a lot extra paths are generated in the OpenAPI file; each service in the proto file gets one extra set of paths per method per number of services below with the same path.

Second, I'd like to to get input to some thoughts on the original problem described in this issue. I would really like to have a OpenAPI file that lists the different paths (using OPs example):

• GET /auth/iam/roles
• GET /auth/iam/organizations/{organization_id}/roles
• GET /auth/iam/tenants/{tenant_id}/roles

I see two immediate problems with generating this:

1. Where to get the path parameter names (organization_id and tenant_id) from? They are not formally specified in the proto file.
2. Where to get the documentation for the parameter names from? The documentation from the proto field (parent) cannot be used, as it describes all 3 variants, and includes the prefixes in the values.

To solve this, would it make sense to allow specifying the OpenAPI friendly paths and path parameters in the openapiv2_operation option? It would have to be a repeated field in case of additional bindings being used. E.g. something like this:

option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_operation) = {
  paths: {
    path: {
      template: "/auth/iam/roles"
    }
    path: {
      template: "/auth/iam/organizations/{organization_id}/roles"
      parameters: {
        parameter: {
          name: "organization_id",
          description: "The organization id"
        }
      }
    }
    path: {
      template: "/auth/iam/tenants/{tenant_id}/roles"
      parameters: {
        parameter: {
          name: "tenant_id",
          description: "The tenant id"
        }
      }
    }
  }
};

As a path may contain multiple parameters, this also must be a repeated field.

[johanbrandhorst]

Thanks for your thoughtful input! I noticed you raised #2489 for the first issue, thanks for that!

As for the proposal, I worry about having another source of truth for the template path and parameters, with obvious maintenance issues. I don't think this would be the first such case but we really should exhaust other possibilities before compromising there.

I'm confused by the problem statement too, in the example GET /auth/iam/organizations/{organization_id}/roles, couldn't that be written as get: "/auth/iam/{organization_id=organizations/*}/roles"? Do you want to separate the parameter name from the message field name?

[oyvindwe]

Thanks for the feedback! My idea is not in ideal, it's merely a workaround the lack of support for this in the standards (OpenAPI and gRPC HTTP mapping).

I get your idea with using different gRPC fields for the alternative paths. That will require a change to any gRPC message to use in this way, as OPs example would have to be changed to (possibly put the alternative parent ids in a oneof):

message ListRolesRequest {
  …
  // The resource name of the parent resource. Only allowed format is `organizations/{organization_id}`
  string organization_id = 2;
  // The resource name of the parent resource. Only allowed format is in the format `tenants/{tenant_id}`
  string tenant_id = 3:
  ...
}

rpc ListRoles(ListRolesRequest) returns (ListRolesResponse) {
  option (google.api.http) = {
    get: "/auth/iam/roles"
    additional_bindings {
      get: "/auth/iam/{organization_id=organizations/*}/roles"
    }
    additional_bindings {
      get: "/auth/iam/{tenant_id=tenants/*}/roles"
    }
  };
}

This puts an extra burden on the gRPC service implementation that I think should be possible to solve with parameter mapping. Also, if you do the above, the path is very short to ignore the Google API guidelines completely:

message ListRolesRequest {
  // The parent resource. If not set, this refers to curated roles
  oneof parent {
    // The organization id of the parent resource
    string organization_id = 2;
    // The tentant_id of the parent resource
    string tenant_id = 3:
  }
  ...
}

rpc ListRoles(ListRolesRequest) returns (ListRolesResponse) {
  option (google.api.http) = {
    get: "/auth/iam/roles"
    additional_bindings {
      get: "/auth/iam/organizations/{organization_id}/roles"
    }
    additional_bindings {
      get: "/auth/iam/tenants/{tenant_id}/roles"
    }
  };
}

However, this doesn't solve the problem on how to generate OpenAPI specification when following the Google API guidelines for resource names.

Another possibility is to extend google.api.http to support OpenAPI compatible path templates, but it will still require both the OpenAPI path parameter name and it's descriptions. E.g. something like get: "/auth/iam/{parent=organizations/{organization_id}}/roles", "The parent's organization id". Unfortunately, I think this is a longer path to a solution, as the scope is beyond grpc-gateway.

[johanbrandhorst]

I see the problem. I'm hesitant to adopt something like this without a clear need from the community - it would be a pretty dangerous change to make and I want to know that there's enough benefit to take on the cost, if it means having to add this alternative path specification option. Let me know if you can think of some other way to solve this though!

[oyvindwe]

I have implemented a simpler suggestion in PR #2562

[oyvindwe]

I seem to have missed a concept in the Google AIPs that can be used to generate paths based on resource names - the pattern property of the google.api.resource annotation as explained in AIP 123.

Examples are provided in AIP 122. Note that there are two variants that must be supported, depending on when the resource type is used in request message, and when the request only contains a reference (by resource name) to the actual resource (option google.api.resource_reference), so there's potentially a lot of lookups that must be implemented.

It is still not possible to convey type information, description, or examples of the individual path segments though.

Would it make sense to add a runtime option to protoc-gen-openapiv2 to generate path parameters based on these annotations?

[johanbrandhorst]

Would we need to add an option? Is it something we could just turn on? Do you think it would risk breaking users?

[oyvindwe]

Would we need to add an option? Is it something we could just turn on? Do you think it would risk breaking users?

I have a feeling nobody are using protoc-gen-openapiv2 with google.api.resource annotations, since nobody suggested to use that previously. It wouldn't break our flow at least, that's for sure!

[oyvindwe]

@yinzara I'd be very happy to get your input on the idea to generate path parameters from google.api.resource annotations.

[betmix-matt]

Yeah and I think this is an unrelated issue to this issue. If we want to support `google.api.resource` annotations instead of using `google.api.http` then we should have a separate defect opened. From: Øyvind Matheson Wergeland ***@***.***> Date: Monday, March 28, 2022 at 8:00 AM To: grpc-ecosystem/grpc-gateway ***@***.***> Cc: Matthew Vance ***@***.***>, Comment ***@***.***> Subject: Re: [grpc-ecosystem/grpc-gateway] Generating Multiple Path Bindings for Single gRPC Call (#720) Would we need to add an option? Is it something we could just turn on? Do you think it would risk breaking users? I have a feeling nobody are using protoc-gen-openapiv2 with google.api.resource annotations, since nobody suggested to use that previously. It wouldn't break our flow at least, that's for sure! — Reply to this email directly, view it on GitHub<#720 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AWEFJMX3XESEGOHQZEOY27DVCHCO7ANCNFSM4FOT23AQ>. You are receiving this because you commented.Message ID: ***@***.***>

[oyvindwe]

Yeah and I think this is an unrelated issue to this issue. If we want to support google.api.resource annotations instead of using google.api.http then we should have a separate defect opened.

The way I understand this is that google.api.resource would be used in combination with google.api.http. The http annotation would give the complete path to an operation, while the resource annotation would break down the resource name or parent path parameter into the separate path segments, and possibly also into multiple path parameters.

See https://github.com/googleapis/googleapis/blob/f59df18148fd44d2ead03109c391a742a9c99e48/google/api/resource.proto#L75-L83 for an example on how to apply multiple resource paths for a resource.

OPs example would need to be expanded to something like this (note that I'm not sure how to support an empty parent):

rpc ListRoles(ListRolesRequest) returns (ListRolesResponse) {
  option (google.api.http) = {
    get: "/auth/iam/roles"
    additional_bindings {
      get: "/auth/iam/{parent=organizations/*}/roles"
    }
    additional_bindings {
      get: "/auth/iam/{parent=tenants/*}/roles"
    }
  };
}

message ListRolesRequest {
  // The resource name of the parent resource. Not set refers to curated roles.
  // Format:
  //  - organizations/{organization}
  //  - tenants/{tenant}
  string parent = 1 [(google.api.resource_reference) = {
    child_type: "api.example.com/Role"
  }];
}

message Role {
  option (google.api.resource) = {
    type: "api.example.com/Role"
    pattern: "roles/{role}"
    pattern: "organizations/{organization}/roles/{role}"
    pattern: "tenants/{tenant}/roles/{role}"
  };

  // The resource name of the role.
  string name = 1;

  // Other fields...
}

This should generate the following paths in the OAS file:

• /auth/iam/roles
• /auth/iam/organizations/{organization}/roles
• /auth/iam/tenants/{tenant}/roles

An implementation will have to handle several cases, e.g. for supporting multiple parent resources, each alternative path must be matched with the binding, and for parent references using multiple parent types, the path matching must be on the pattern prefix of the alternative patterns for the child_type.

Expanding OPs example with GetRoleRequest, we see that a single {name} parameter expands to multiple path parameters:

rpc GetRole(GetRoleRequest) returns (Role) {
  option (google.api.http) = {
    get: "/auth/iam/{name=roles/*}"
    additional_bindings {
      get: "/auth/iam/{name=organizations/*/roles/*}"
    }
    additional_bindings {
      get: "/auth/iam/{name=tenants/*/roles/*}"
    }
  };
}

message GetRoleRequest {
  // The resource name of the resource.
  // Format:
  //  - roles/{role}
  //  - organizations/{organization}/roles/{role}
  //  - tenants/{tenant}/roles/{role}
  string name = 1 [(google.api.resource_reference) = {
    type: "api.example.com/Role"
  }];
}

This should generate the following paths in the OAS file:

• /auth/iam/roles/{role}
• /auth/iam/organizations/{organization}/roles/{role}
• /auth/iam/tenants/{tenant}/roles/{role}

(EDIT: corrected typo tentant -> tenant)

[yinzara]

So that makes no sense to me.

That would mean that you'd have to have translate the GetRoleRequest message into 3 separate OpenAPI resources: one for each API endpoint that could be supported by it as you would no longer have a name parameter, but instead a organization or tenant or role parameters all with different combinations depending on which endpoint. That would further distance the OAS from the proto specification.

That also breaks the whole abstraction as "tenant" and "role" really don't exist. They're just a name for a placeholder in the URL/resource name. You aren't supposed to deconstruct resource names into their parts. They are a name and not an id for a reason.

There is no way to have /auth/iam/organizations/{organization}/roles/{role} be the path in the OAS without rewriting the name path parameter into multiple different parameters.

[oyvindwe]

@yinzara thank you for your input!

I think there would still only be one resource (Role), but 3 different paths to get to that resource. That is already supported now by the generation of these 3 paths in the OAS file (for the GetRole example above):

• GET /auth/iam/{name} where path parameter name has pattern roles/[^/]+
• GET /auth/iam/{name_1} where path parameter name_1 has pattern organizations/[^/]+/roles/[^/]+
• GET /auth/iam/{name_2} where path parameter name_2 has pattern tenants/[^/]+/roles/[^/]+

So from a REST consumer point of view, I think it is preferable to rather expand the path than work with these patterns, and google.api.resource provides the necessary information to do that, albeit on a minimum level, i.e. no place to document each variable path segment.

[yinzara]

I disagree. I think you're making assumptions about the consumer of your API.

When you use resource names in an API, you expect to be passing those resource names around and for fields of entities to use those resource names when referring to other entities.

You shouldn't be parsing those names into those parts and then passing those parts to your APIs as you don't have enough information to know if your parsing is actual valid.

Google APIs must represent resource names using plain strings, unless backward compatibility is an issue. Resource names should be handled like normal file paths. When a resource name is passed between different components, it must be treated as an atomic value and must not have any data loss.

If I have a RoleBinding message that has a string role where it can have any of the known forms of a Role name, it would then be the burden of the client side implementation to parse the resource names (possibly incorrectly) so it could call your now modified APIs.

https://cloud.google.com/apis/design/resource_names#resource_name_as_string

[oyvindwe]

When testing tools used to render documentation from OAS files(readme.com and Swagger UI), we noticed that path parameters containing / are URL encoded in generated code. Tracking down this issue, I realized that allowReserved in OpenAPI 3.0 is only allowed for query parameters: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.2.md#parameterObject

OpenAPI 2 doesn't include allowReserved, and the spec doesn't explicitly specify if / is allowed or not in path parameters, but it feels a bit implied when reading it.

Allowing / in path parameters in OpenAPI is being discussed here: OAI/OpenAPI-Specification#892

This implies that the current approach in protoc-gen-openapiv2 generates illegal OAS.

My conclusion so far is that we'll most likely do some post processing on the generated OAS file to replace path parameters with /.

[johanbrandhorst]

Thanks for the research there, do you think we need to make a behavioral change in the generator?

[oyvindwe]

Yes I do think that, but that takes us some steps back to previous discussions - we need to introduce annotations to specify the extra path parameters per http binding, including description and type information. I think this is a fairly large extension of the generator.

[johanbrandhorst]

Introducing more annotations is leaving a bad taste in my mouth, it's already complex as it is. Right now my main concern is that we generate valid OAS documents. Should we revert the previous changes to behavior?

[oyvindwe]

Sorry, let me rephrase; the OAS file isn't syntactically invalid, but when used on path parameters that contains /, the generated OAS is a mismatch with what's expected according to the gRPC HTTP bindings, so it's not usable.

As I understand it, there are no previous version of the generator that supports path parameters with / correct according to OpenAPI, so there's nothing to revert.

One possibility is to log a warning in these cases, but that doesn't solve the problem on how to generate a valid and usable OAS file based on a proto file when following the Google AIPs.

As outlined above, it looks like it may be possible to get the path parameter names from google.api.resource annotations by mapping the variable (*) segments to the variable names from the pattern properties of the resource, but we still need at least description, and preferably also type information.

We're currently working around this by providing a custom mapping file and post-processing the generated OAS files with a script that replaces the path parameters. This way, we avoid hand coding the bulk of the OAS files, so the generator is still usable in our pipeline.

[johanbrandhorst]

I see, thanks for the clarification. So the attempts to fix the handling of parameters containing / was unfortunately insufficient, and we still don't support it. It seems we're left with two choices;

• Continue down the path of trying to handle these corner cases properly, which may mean introducing extra annotations.
• Calling it a day on this effort, document the workarounds (as you describe) and log warnings for this case. Ideally an error in the generator would've been nice (this seems like an almost fundamental compatibility issue between OAS and google.api.http).

What do you think?

[oyvindwe]

I'm leaning towards calling it a day. I would be very happy if the openapiv2 generator could produce an OAS that's directly usable, but for us it's currently not worth the effort. I'd rather spend some time fixing other bugs within the current behaviour.

Also, template.go is 2765 lines, and template_test.go is more than 6000 lines, so to add new features like this probably warrants restructuring the code.

[johanbrandhorst]

I agree. Would you be interested in adding a warning log for these cases, so that a user that runs into this issue can understand why it may not work as expected?

[oyvindwe]

Would you be interested in adding a warning log for these cases, so that a user that runs into this issue can understand why it may not work as expected?

Done in PR #2697

[oyvindwe]

OpenAPI 3.1 explicitly forbids / in path parameters:

The value for these path parameters MUST NOT contain any unescaped “generic syntax” characters described by [RFC3986]: forward slashes (/), question marks (?), or hashes (#).

https://spec.openapis.org/oas/v3.1.0#path-templating