Refactor all gRPC proxy code into a single class.

Refactor all gRPC proxy code into a single class.

BUG=

Review-Url: https://codereview.chromium.org/2987333002
Committed: https://github.com/luci/luci-py/commit/a0cd80426292b7b4775ed2568d7f5130785f8c88

[aludwin, 2017-08-01 20:28:48]

aludwin@google.com changed reviewers:
+ maruel@chromium.org, vadimsh@chromium.org

[aludwin, 2017-08-01 20:28:49]

Hey Marc-Antoine, Vadim - this is mainly a refactoring to collect all
boilerplate gRPC code into one place, though I've also beefed up my error
checking at the same time.

I tested this using my local proxy both with an without errors. Let me know if
you think that unit tests are important; they'll essentially be able to show
that both successful operations and exceptions are let through the proxy
transparently, but little else, since I don't want to test the text that's
written to the logs. Let me know what you think.

[M-A Ruel, 2017-08-02 21:05:12]

https://codereview.chromium.org/2987333002/diff/20001/appengine/swarming/swar...
File appengine/swarming/swarming_bot/bot_code/remote_client.py (right):

https://codereview.chromium.org/2987333002/diff/20001/appengine/swarming/swar...
appengine/swarming/swarming_bot/bot_code/remote_client.py:46: grpc_proxy =
os.environ.get('SWARMING_GRPC_PROXY', grpc_proxy)
We'd need to document these under doc/

https://codereview.chromium.org/2987333002/diff/20001/client/utils/grpc_proxy.py
File client/utils/grpc_proxy.py (right):

https://codereview.chromium.org/2987333002/diff/20001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:1: #!/usr/bin/env python
This file is not executable.

https://codereview.chromium.org/2987333002/diff/20001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:27: # google.auth.transport
(https://stackoverflow.com/questions/24973326). So this
the sentence is incomplete

https://codereview.chromium.org/2987333002/diff/20001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:60:
(https://developers.google.com/identity/protocols/application-default-credentials).
() are unnecessary

https://codereview.chromium.org/2987333002/diff/20001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:98: m = re.search('^(https?):\/\/([^\/]+)/?(.*)$',
proxy)
I prefer to use urlparse + startswith(('http://', 'https://')), it's saner and
easier to understand.
https://docs.python.org/2/library/urlparse.html

https://codereview.chromium.org/2987333002/diff/20001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:183: sp = lambda x: stream_proxy(x, name)
I don't understand why you create a lambda just to call it right away, e.g.:

return stream_proxy(stream, name)

but even then, I don't see the value of this local function.

https://codereview.chromium.org/2987333002/diff/20001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:194: return self._wrap_grpc_operation(name, lambda:
method(request, timeout=GRPC_TIMEOUT_SEC))
wrap (Everywhere)

https://codereview.chromium.org/2987333002/diff/20001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:238: roots = os.environ.get('GRPC_PROXY_TLS_ROOTS')
We'd need to document these under doc/

[aludwin, 2017-08-04 01:44:27]

I'll re-test tomorrow before checking in.

https://codereview.chromium.org/2987333002/diff/20001/appengine/swarming/swar...
File appengine/swarming/swarming_bot/bot_code/remote_client.py (right):

https://codereview.chromium.org/2987333002/diff/20001/appengine/swarming/swar...
appengine/swarming/swarming_bot/bot_code/remote_client.py:46: grpc_proxy =
os.environ.get('SWARMING_GRPC_PROXY', grpc_proxy)
On 2017/08/02 21:05:11, M-A Ruel wrote:
> We'd need to document these under doc/

Done.

https://codereview.chromium.org/2987333002/diff/20001/client/utils/grpc_proxy.py
File client/utils/grpc_proxy.py (right):

https://codereview.chromium.org/2987333002/diff/20001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:1: #!/usr/bin/env python
On 2017/08/02 21:05:11, M-A Ruel wrote:
> This file is not executable.

Done.

https://codereview.chromium.org/2987333002/diff/20001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:27: # google.auth.transport
(https://stackoverflow.com/questions/24973326). So this
On 2017/08/02 21:05:11, M-A Ruel wrote:
> the sentence is incomplete

Done.

https://codereview.chromium.org/2987333002/diff/20001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:60:
(https://developers.google.com/identity/protocols/application-default-credentials).
On 2017/08/02 21:05:11, M-A Ruel wrote:
> () are unnecessary

Done.

https://codereview.chromium.org/2987333002/diff/20001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:98: m = re.search('^(https?):\/\/([^\/]+)/?(.*)$',
proxy)
On 2017/08/02 21:05:11, M-A Ruel wrote:
> I prefer to use urlparse + startswith(('http://', 'https://')), it's saner and
> easier to understand.
> https://docs.python.org/2/library/urlparse.html

Done.

https://codereview.chromium.org/2987333002/diff/20001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:183: sp = lambda x: stream_proxy(x, name)
On 2017/08/02 21:05:11, M-A Ruel wrote:
> I don't understand why you create a lambda just to call it right away, e.g.:
> 
> return stream_proxy(stream, name)
> 
> but even then, I don't see the value of this local function.

I created a lambda because I forgot how generators worked <blush>. Fixed.

The local function (actually, a local generator) simply ensures that every call
to next(stream) occurs inside _wrap_grpc_operation so that its exceptions can be
caught and processed in the way we want.

https://codereview.chromium.org/2987333002/diff/20001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:194: return self._wrap_grpc_operation(name, lambda:
method(request, timeout=GRPC_TIMEOUT_SEC))
On 2017/08/02 21:05:11, M-A Ruel wrote:
> wrap (Everywhere)

Done.

https://codereview.chromium.org/2987333002/diff/20001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:238: roots = os.environ.get('GRPC_PROXY_TLS_ROOTS')
On 2017/08/02 21:05:11, M-A Ruel wrote:
> We'd need to document these under doc/

Done.

[M-A Ruel, 2017-08-04 02:05:37]

lgtm with comments.

https://codereview.chromium.org/2987333002/diff/40001/appengine/swarming/doc/...
File appengine/swarming/doc/Magic-Values.md (right):

https://codereview.chromium.org/2987333002/diff/40001/appengine/swarming/doc/...
appengine/swarming/doc/Magic-Values.md:55: /client/utils/grpc_proxy.py for more
information.
you can link directly with:
 [/client/utils/grpc_proxy.py](../../../client/utils/grpc_proxy.py)

https://codereview.chromium.org/2987333002/diff/40001/client/utils/grpc_proxy.py
File client/utils/grpc_proxy.py (right):

https://codereview.chromium.org/2987333002/diff/40001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:114: self._debug_info = ['full proxy name: ' + proxy]
instead of _debug_info, you can create a local logger, e.g.

log = logging.getLogger('gprc.proxy')

then use log.info()...

https://codereview.chromium.org/2987333002/diff/40001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:119: @property
Most of these are not needed, you can add only once needed.

https://codereview.chromium.org/2987333002/diff/40001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:185: return wrap_grpc_iterator(stream, name)
There's still no value in wrap_grpc_iterator, you can rewrite the whole function
as a generator:

def get_stream(self, name, request):
  stream = self.call_no_retries(name, request)
  while True:
    response = self._wrap_grpc_operation(name + ' pull from stream', lambda:
next(stream, 1))
    if isinstance(response, int):
      return
    yield response


that's it! (plus comments)

https://codereview.chromium.org/2987333002/diff/40001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:229: # GRPC_PROXY_TLS_ROOTS to the "roots.crt" file
specifying the certificate for
wrap all

https://codereview.chromium.org/2987333002/diff/40001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:242: if roots is not None:
if roots:

https://codereview.chromium.org/2987333002/diff/40001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:250: if overd is not None:
if overd:

https://codereview.chromium.org/2987333002/diff/40001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:256: self._debug_info.append('Scopes are: %s' %
repr(scopes))
%r instead of repr

[aludwin, 2017-08-04 14:53:10]

https://codereview.chromium.org/2987333002/diff/40001/appengine/swarming/doc/...
File appengine/swarming/doc/Magic-Values.md (right):

https://codereview.chromium.org/2987333002/diff/40001/appengine/swarming/doc/...
appengine/swarming/doc/Magic-Values.md:55: /client/utils/grpc_proxy.py for more
information.
On 2017/08/04 02:05:37, M-A Ruel wrote:
> you can link directly with:
>  [/client/utils/grpc_proxy.py](../../../client/utils/grpc_proxy.py)

Done.

https://codereview.chromium.org/2987333002/diff/40001/client/utils/grpc_proxy.py
File client/utils/grpc_proxy.py (right):

https://codereview.chromium.org/2987333002/diff/40001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:114: self._debug_info = ['full proxy name: ' + proxy]
On 2017/08/04 02:05:37, M-A Ruel wrote:
> instead of _debug_info, you can create a local logger, e.g.
> 
> log = logging.getLogger('gprc.proxy')
> 
> then use log.info()...

The idea here is to *save* these bits of information unless/until something goes
wrong, and only then print them out. From what I've read, logging.Logger isn't
really a great fit for that - it seems pretty heavyweight for what I'm really
trying to do (delayed output).

https://codereview.chromium.org/2987333002/diff/40001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:119: @property
On 2017/08/04 02:05:37, M-A Ruel wrote:
> Most of these are not needed, you can add only once needed.

Done.

https://codereview.chromium.org/2987333002/diff/40001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:185: return wrap_grpc_iterator(stream, name)
On 2017/08/04 02:05:37, M-A Ruel wrote:
> There's still no value in wrap_grpc_iterator, you can rewrite the whole
function
> as a generator:
> 
> def get_stream(self, name, request):
>   stream = self.call_no_retries(name, request)
>   while True:
>     response = self._wrap_grpc_operation(name + ' pull from stream', lambda:
> next(stream, 1))
>     if isinstance(response, int):
>       return
>     yield response
> 
> 
> that's it! (plus comments)

Oh, right :P Thanks!

https://codereview.chromium.org/2987333002/diff/40001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:229: # GRPC_PROXY_TLS_ROOTS to the "roots.crt" file
specifying the certificate for
On 2017/08/04 02:05:37, M-A Ruel wrote:
> wrap all

Done.

https://codereview.chromium.org/2987333002/diff/40001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:242: if roots is not None:
On 2017/08/04 02:05:37, M-A Ruel wrote:
> if roots:

Done.

https://codereview.chromium.org/2987333002/diff/40001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:250: if overd is not None:
On 2017/08/04 02:05:37, M-A Ruel wrote:
> if overd:

Done.

https://codereview.chromium.org/2987333002/diff/40001/client/utils/grpc_proxy...
client/utils/grpc_proxy.py:256: self._debug_info.append('Scopes are: %s' %
repr(scopes))
On 2017/08/04 02:05:37, M-A Ruel wrote:
> %r instead of repr

Done.

[aludwin, 2017-08-04 18:06:27]

The CQ bit was checked by aludwin@google.com

[aludwin, 2017-08-04 18:06:27]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/2987333002/#ps80001
(title: "Test and tweak messages")

[commit-bot: I haz the power, 2017-08-04 18:06:33]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-04 18:15:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-04 18:15:42]

Try jobs failed on following builders:
  Luci-py Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/37c73f104f5ce710)

[aludwin, 2017-08-04 18:35:09]

The CQ bit was checked by aludwin@google.com

[aludwin, 2017-08-04 18:35:09]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/2987333002/#ps100001
(title: "Fix unit test failures")

[commit-bot: I haz the power, 2017-08-04 18:35:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-04 19:11:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-04 19:11:02]

Try jobs failed on following builders:
  Luci-py Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/37c7594fea9d1c10)

[aludwin, 2017-08-04 19:22:37]

The CQ bit was checked by aludwin@google.com

[commit-bot: I haz the power, 2017-08-04 19:22:43]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[M-A Ruel, 2017-08-04 19:26:54]

On 2017/08/04 19:11:02, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   Luci-py Presubmit on luci.infra.try (JOB_FAILED,
> https://luci-milo.appspot.com/swarming/task/37c7594fea9d1c10)

************* Module bot_code.remote_client_grpc_test
C: 34, 0: Line too long (89/80) (line-too-long)

[aludwin, 2017-08-04 19:31:28]

On 2017/08/04 19:26:54, M-A Ruel wrote:
> On 2017/08/04 19:11:02, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   Luci-py Presubmit on luci.infra.try (JOB_FAILED,
> > https://luci-milo.appspot.com/swarming/task/37c7594fea9d1c10)
> 
> ************* Module bot_code.remote_client_grpc_test
> C: 34, 0: Line too long (89/80) (line-too-long)

Oh, I missed that - I thought I just got one of the timeouts I frequently get
(does that really not happen for anyone else except me?)

[aludwin, 2017-08-04 19:35:27]

The CQ bit was checked by aludwin@google.com

[aludwin, 2017-08-04 19:35:28]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/2987333002/#ps120001
(title: "Wrap long line")

[commit-bot: I haz the power, 2017-08-04 19:35:31]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-04 19:44:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-04 19:44:29]

Try jobs failed on following builders:
  Luci-py Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/37c7908c39b57d10)

[aludwin, 2017-08-04 19:53:55]

The CQ bit was checked by aludwin@google.com

[aludwin, 2017-08-04 19:53:56]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/2987333002/#ps140001
(title: "Fix pylint errors")

[commit-bot: I haz the power, 2017-08-04 19:53:58]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-04 19:58:11]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1501876435448480,
"parent_rev": "aa0910e4251048df312ef8c652768081bd7f2cb4", "commit_rev":
"a0cd80426292b7b4775ed2568d7f5130785f8c88"}

[commit-bot: I haz the power, 2017-08-04 19:58:15]

Description was changed from

==========
Refactor all gRPC proxy code into a single class.

BUG=
==========

to

==========
Refactor all gRPC proxy code into a single class.

BUG=

Review-Url: https://codereview.chromium.org/2987333002
Committed:
https://github.com/luci/luci-py/commit/a0cd80426292b7b4775ed2568d7f5130785f8c88
==========

[commit-bot: I haz the power, 2017-08-04 19:58:17]

Committed patchset #8 (id:140001) as
https://github.com/luci/luci-py/commit/a0cd80426292b7b4775ed2568d7f5130785f8c88