Skip to content

feat: Determine URL protocol based on hostname #900

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.

Sign up for GitHub

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

Closed
vchudnov-g wants to merge 4 commits into from
Closed

feat: Determine URL protocol based on hostname #900

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7dc5ce3
feat: Determine URL protocol based on hostname
vchudnov-g May 19, 2021
f6e383f
chore: Implement code-review changes to protocol_for_host()
vchudnov-g May 21, 2021
02e5fd3
Merge branch 'master' into vchudnov-host-protocol
parthea Jul 26, 2021
0141d94
Merge branch 'master' into vchudnov-host-protocol
parthea Aug 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

{% block content %}

import re
import warnings
from typing import Callable, Dict, Optional, Sequence, Tuple

Expand Down Expand Up @@ -180,7 +181,8 @@ class {{ service.name }}RestTransport({{ service.name }}Transport):
{# TODO(yon-mg): Write helper method for handling grpc transcoding url #}
# TODO(yon-mg): need to handle grpc transcoding and parse url correctly
# current impl assumes basic case of grpc transcoding
url = 'https://{host}{{ method.http_opt['url'] }}'.format(
url = '{protocol}://{host}{{ method.http_opt['url'] }}'.format(
protocol=_protocol_for(self._host),
host=self._host,
{% for field in method.path_params %}
{{ field }}=request.{{ method.input.get_field(field).name }},
Expand Down Expand Up @@ -229,6 +231,29 @@ class {{ service.name }}RestTransport({{ service.name }}Transport):
{% endif %}
{% endfor %}

{# TODO: Allow client creation to explictly specify connection
parameters, such as secure/insecure connections, and have tests use
that. Then remove this function.
Tracking issue:
https://github.com/googleapis/gapic-generator-python/issues/901
#}
def _protocol_for(host: str) -> str:
"""Determine the URL protocol to use for connecting to the given host.

This function returns 'http' for hosts that match
`^localhost(:.*)?$` and `https` for all others. This is intended
Copy link
Contributor

@vam-google vam-google May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have anything like this in the other languages? Overall this looks suspicious I would prefer to not have something like this. Determining protocol from host name does not seem reliable and also we are modifying production code for testing purposes. Also what if the host is specified as an IP (land specifically 127.0.0.1)?

Copy link
Contributor Author

@vchudnov-g vchudnov-g May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This was discussed in an internal doc; I'll ping you the link. We're specifically looking for localhost because yes, this is a temporary special case to allow testing (see #901 for the long-term solution)

to allow integration testing over an insecure connection to a
locally running server.

Args:
host (str): The hostname for REST connection.

Returns:
str: The protocol (`http` or `https`) to use for connecting to `host`.
"""
if not hasattr(_protocol_for, 'regex'):
_protocol_for.regex = re.compile("^localhost(:.*)?$")
Copy link
Contributor

@vam-google vam-google May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this regex be executed for every call made? If yes, it will have some performance implications (not big probably, but here we sacrifice production code to support rare testing case, which is suboptimal).

Copy link
Contributor Author

@vchudnov-g vchudnov-g May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compilation of the regex will only happen once because I have it memoized via this attribute.

The matching will happen for every call for now, though we could refactor things so the protocol is only computed once per client and stored in an attribute. However, given that the performance hit is small, I'm inclined to think it's preferable to keep things simple.

return 'http' if _protocol_for.regex.match(host) else 'https'

__all__ = (
'{{ service.name }}RestTransport',
Expand Down