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

Routing headers are not passed to the operations client #732

Closed
busunkim96 opened this issue Jan 14, 2021 · 6 comments · Fixed by googleapis/python-api-core#133
Closed
Assignees
Labels
🚨 This issue needs some love. triage me I really want to be triaged.

Comments

@busunkim96
Copy link
Contributor

See internal issue 173104871 for more details.

Some APIs require routing headers (location info inx-goog-request-params) for successful requests. https://google.aip.dev/client-libraries/4222

# Certain fields should be provided within the metadata header;
# add these here.
metadata = tuple(metadata) + (
gapic_v1.routing_header.to_grpc_metadata((
{%- for field_header in method.field_headers %}
{%- if not method.client_streaming %}
('{{ field_header }}', request.{{ field_header }}),
{%- endif %}
{%- endfor %}
)),
)
{%- endif %}
# Send the request.
{% if not method.void %}response = {% endif %}rpc(
{%- if not method.client_streaming %}
request,
{%- else %}
requests,
{%- endif %}
retry=retry,
timeout=timeout,
metadata=metadata,
)

These headers are added to normal requests to the API but are missing in calls to the Operations API. This results in requests failing with permission denied errors.

{%- if method.lro %}
# Wrap the response in an operation future.
response = {{ method.client_output.ident.module_alias or method.client_output.ident.module }}.from_gapic(
response,
self._transport.operations_client,
{{ method.lro.response_type.ident }},
metadata_type={{ method.lro.metadata_type.ident }},
)

A fix will require:

  1. new version of google-api-core with an operations client that accepts and uses gRPC metadata
  2. An update to the generator templates.
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Jan 14, 2021
@noahdietz
Copy link
Contributor

FWIW the Go GAPICs use an LRO GAPIC under the hood. The LRO GAPIC is made using the google/longrunning/operations.proto which have google.api.http annotations on the RPCs that declare the name field as a request param here. So then, per https://google.aip.dev/client-libraries/4222, the Go gapic generator makes sure to include the code that injects the name field of the Operation as x-goog-request-params. Thus, all LRO RPCs have the name=... request param. This was the missing header necessary in the internal bug, but why tests with the Go gapic succeeded when Python's failed.

@alexander-fenster
Copy link
Member

I understand why it may require name field, but why location? Would it be enough to just pass the name request header for LRO?

@noahdietz
Copy link
Contributor

noahdietz commented Jan 15, 2021

I had the same thought last night. I think you are right, @alexander-fenster, passing the value of the Operation.name should be sufficient. That is what the Go GAPICs do and it works properly.

I think location came up in discussion because the routing rules were parsing out the Location "parent" part of the Operation.name.

This could be straight forward as passing the Operation.name to x-goog-request-params on every request.

cc: @viacheslav-rostovtsev

@alexander-fenster
Copy link
Member

OK, thank you @noahdietz – then for TypeScript it's just a quick fix to the operations client that is in google-gax (it was originally a generated GAPIC but never updated, for some reason it does not pass x-goog-request-params, maybe it was frozen before that became a thing).

@busunkim96
Copy link
Contributor Author

I think I can just make this fix in Python's gax then - google-api-core. Thanks for the clarification @noahdietz @alexander-fenster! 🙏

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Jan 19, 2021
@busunkim96
Copy link
Contributor Author

Opened googleapis/python-api-core#133

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 This issue needs some love. triage me I really want to be triaged.
Projects
None yet
4 participants