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

Add alternative to reroute_to_grpc_interface #536

Closed
busunkim96 opened this issue Jul 17, 2020 · 5 comments · Fixed by #545
Closed

Add alternative to reroute_to_grpc_interface #536

busunkim96 opened this issue Jul 17, 2020 · 5 comments · Fixed by #545
Assignees
Labels
conversion blocking A surface delta, bug, missing feature, or other issue that prevents API conversion type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@busunkim96
Copy link
Contributor

Pub/Sub and KMS use the reroute_to_grpc_interface option in the GAPIC yaml to add additional IAM methods with the monolithic generator. This adds the methods get_iam_policy(), set_iam_policy(), test_iam_permission(). There is no equivalent in the microgenerator.

IAM v1 is a full-fledged API and can be generated with the existing protos/configs here.

Options:

  • Publish the google/iam/v1* client somewhere. Possible locations: grpc-google-iam-v1, google-cloud-iam, google-api-core, some new package.
  • Add an option to the generator to add IAM methods.

Thoughts?

Here is what NodeJS did for reference: googleapis/gax-nodejs#762 and googleapis/gapic-generator-typescript#375

CC @plamut @software-dov

@plamut
Copy link
Contributor

plamut commented Jul 17, 2020

I'm fine with either option, whatever makes the future maintenance easier. Or, if one option is significantly faster to implement than the other, let's go with it and possibly make a switch to the other option later.

Background: Having the IAM methods in PubSub client is a prerequisite for the upcoming major release. We currently put all other PRs on hold to not interfere with these changes, thus we would like to complete the major release in the near future, if possible.

@software-dov
Copy link
Contributor

My kneejerk reaction is to publish google/iam/v1*. Do we have a good estimate on how much initial/recurring effort that would involve?

@busunkim96
Copy link
Contributor Author

I did some investigation to figure out why this is only used for two APIs. reroute_to_grpc_interface seems to be a hack (see internal thread 1 and 2 ). The recommended way to add IAM is as a mix-in API. This is what the vast majority of APIs do. The IAM methods are copied into the service protos and the generator happily produces those methods.

If we publish a separate client, users have to instantiate a separate IAM client for only Pub/Sub and KMS, which doesn't seem like a good experience.

I don't think it will be too terrible to do this in the generator (the 3 methods don't seem to change from API to API). I'm going to take a stab at a PR, but if it starts to get out of hand we can do something more similar to what Node did.

Threads from other microgenerators:

@busunkim96 busunkim96 added conversion blocking A surface delta, bug, missing feature, or other issue that prevents API conversion type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed triage me I really want to be triaged. labels Jul 17, 2020
@busunkim96 busunkim96 self-assigned this Jul 17, 2020
@software-dov
Copy link
Contributor

Spitballing: would copying the requisite messages/rpcs to a separate proto file and then importing that in KMS/PubSub/et al. be sufficient?

@busunkim96
Copy link
Contributor Author

@software-dov The main issue seems to be that the IAM calls need to go to iam.googleapis.com instead of pubsub.googleapis.com or kms.googleapis.com, since these APIs don't know what to do with these method calls. Making that happen in the proto files would be the cleanest solution. (I assumed it wasn't an option since Ruby and Node seem to have changed their generators to accommodate this.)

KMS transport vs container analysis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conversion blocking A surface delta, bug, missing feature, or other issue that prevents API conversion type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants