Send authorization headers when calling Swarming backend.

Send authorization headers when calling Swarming backend.

What headers to send are decided by bot_config.py. It allows different bots to
use different authentication mechanisms (e.g. some bots may use machine tokens,
some may use GCE service accounts, some may use predeployed JSON keys, etc).

R=maruel@chromium.org
BUG=

Committed: https://github.com/luci/luci-py/commit/3285ba3b48309de808817a4ee9d61466bc0e5485

[Vadim Sh., 2016-06-01 02:29:56]

This CL is not finished yet, but I'd like to get your feedback before finishing
it (to avoid wasting time).

I picked a file on disk as a simplest cross-platform mechanism for IPC between
bot_main and task_runner. The downside is that bot_main now spawns a parallel
thread (that updates file with tokens) while waiting for task_running to finish.

In theory it can be avoided by doing polling subprocess status periodically
(instead of blocking in proc.wait(hard_timeout) forever). But the code there is
already sort of complicated, I didn't want to make it even more complicated...

[M-A Ruel, 2016-06-01 17:08:02]

On 2016/06/01 02:29:56, Vadim Sh. wrote:
> This CL is not finished yet, but I'd like to get your feedback before
finishing
> it (to avoid wasting time).
> 
> I picked a file on disk as a simplest cross-platform mechanism for IPC between
> bot_main and task_runner. The downside is that bot_main now spawns a parallel
> thread (that updates file with tokens) while waiting for task_running to
finish.

I don't mind the thread.


> In theory it can be avoided by doing polling subprocess status periodically
> (instead of blocking in proc.wait(hard_timeout) forever). But the code there
is
> already sort of complicated, I didn't want to make it even more complicated...

Fine.

I guess there aren't a lot of ways to make it works sanely, this means this file
will have to be sent to run_isolated too.

I don't have better ideas than this.

https://codereview.chromium.org/2024313003/diff/1/appengine/swarming/swarming...
File appengine/swarming/swarming_bot/bot_code/bot_main.py (right):

https://codereview.chromium.org/2024313003/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/bot_main.py:182: return func(botobj) if
func else (None, None)
wrap that to catch all exceptions like the other functions accessing bot_config.

[Vadim Sh., 2016-06-03 01:15:32]

This is finished now, PTAL.

https://codereview.chromium.org/2024313003/diff/1/appengine/swarming/swarming...
File appengine/swarming/swarming_bot/bot_code/bot_main.py (right):

https://codereview.chromium.org/2024313003/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/bot_main.py:182: return func(botobj) if
func else (None, None)
On 2016/06/01 17:08:02, M-A Ruel wrote:
> wrap that to catch all exceptions like the other functions accessing
bot_config.

Exceptions are handled in RemoteClient, where it knows how to do retry loop.
Added a comment to doc string.

https://codereview.chromium.org/2024313003/diff/60001/appengine/swarming/swar...
File appengine/swarming/swarming_bot/bot_code/bot_main.py (right):

https://codereview.chromium.org/2024313003/diff/60001/appengine/swarming/swar...
appengine/swarming/swarming_bot/bot_code/bot_main.py:177: Doesn't catch
exceptions.
it's needed to be able to implement retry loop in RemoteClient.initialize.

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

https://codereview.chromium.org/2024313003/diff/60001/appengine/swarming/swar...
appengine/swarming/swarming_bot/bot_code/remote_client.py:85: return
self._get_headers_or_throw()
the idea here is to never to give up retrying exceptions

initialize() will try to grab the headers for some amount of time and eventually
give up, but bot_main would still attempt to run (probably failing in
poll_server). But during each call it still will attempt to grab the headers. So
if problem eventually fixes itself, poll_server would get the headers and
succeed.

https://codereview.chromium.org/2024313003/diff/60001/appengine/swarming/swar...
File appengine/swarming/swarming_bot/bot_code/task_runner.py (right):

https://codereview.chromium.org/2024313003/diff/60001/appengine/swarming/swar...
appengine/swarming/swarming_bot/bot_code/task_runner.py:284: raise
ValueError(last_error)
ValueError is what's being thrown and caught by other functions in this file.

[M-A Ruel, 2016-06-03 20:00:49]

https://codereview.chromium.org/2024313003/diff/80001/appengine/swarming/swar...
File appengine/swarming/swarming_bot/bot_code/file_refresher.py (right):

https://codereview.chromium.org/2024313003/diff/80001/appengine/swarming/swar...
appengine/swarming/swarming_bot/bot_code/file_refresher.py:37: target=self._run,
normally +4

https://codereview.chromium.org/2024313003/diff/80001/appengine/swarming/swar...
appengine/swarming/swarming_bot/bot_code/file_refresher.py:86: def _run(self,
signal):
why have you made it reusable, e.g. we can start()/stop() repeatedly. I don't
think this is needed and this could quite simplify the code.

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

https://codereview.chromium.org/2024313003/diff/80001/appengine/swarming/swar...
appengine/swarming/swarming_bot/bot_code/remote_client.py:86: except Exception:
I wish the kind of exceptions trapped was scoped.

https://codereview.chromium.org/2024313003/diff/80001/appengine/swarming/swar...
File appengine/swarming/swarming_bot/bot_code/task_runner.py (right):

https://codereview.chromium.org/2024313003/diff/80001/appengine/swarming/swar...
appengine/swarming/swarming_bot/bot_code/task_runner.py:232: headers =
read_auth_headers(auth_headers_file) if auth_headers_file else {}
I think it'd prefer to have it update the token in the background too, so that
post_update() is not blocked on disk I/O

[Vadim Sh., 2016-06-03 23:37:55]

https://codereview.chromium.org/2024313003/diff/80001/appengine/swarming/swar...
File appengine/swarming/swarming_bot/bot_code/file_refresher.py (right):

https://codereview.chromium.org/2024313003/diff/80001/appengine/swarming/swar...
appengine/swarming/swarming_bot/bot_code/file_refresher.py:37: target=self._run,
On 2016/06/03 20:00:49, M-A Ruel wrote:
> normally +4

Done.

https://codereview.chromium.org/2024313003/diff/80001/appengine/swarming/swar...
appengine/swarming/swarming_bot/bot_code/file_refresher.py:86: def _run(self,
signal):
On 2016/06/03 20:00:49, M-A Ruel wrote:
> why have you made it reusable, e.g. we can start()/stop() repeatedly. I don't
> think this is needed and this could quite simplify the code.

I didn't intend it to be reusable, I just happened to write it that way... Made
it less reusable :-/ Not sure code is  much simpler.

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

https://codereview.chromium.org/2024313003/diff/80001/appengine/swarming/swar...
appengine/swarming/swarming_bot/bot_code/remote_client.py:86: except Exception:
On 2016/06/03 20:00:49, M-A Ruel wrote:
> I wish the kind of exceptions trapped was scoped.

bot_config.py callback can through whatever it wants :(

https://codereview.chromium.org/2024313003/diff/80001/appengine/swarming/swar...
File appengine/swarming/swarming_bot/bot_code/task_runner.py (right):

https://codereview.chromium.org/2024313003/diff/80001/appengine/swarming/swar...
appengine/swarming/swarming_bot/bot_code/task_runner.py:232: headers =
read_auth_headers(auth_headers_file) if auth_headers_file else {}
On 2016/06/03 20:00:49, M-A Ruel wrote:
> I think it'd prefer to have it update the token in the background too, so that
> post_update() is not blocked on disk I/O

Done. I made it symmetrical to FileRefresherThread. Now there's
FileReaderThread.

[Vadim Sh., 2016-06-03 23:51:40]

The CQ bit was checked by vadimsh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-03 23:51:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2024313003/120001

[commit-bot: I haz the power, 2016-06-03 23:54:55]

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

[commit-bot: I haz the power, 2016-06-03 23:54:56]

Dry run: This issue passed the CQ dry run.

[Vadim Sh., 2016-06-06 18:07:06]

ping

[M-A Ruel, 2016-06-06 21:18:37]

small nits

https://codereview.chromium.org/2024313003/diff/120001/appengine/swarming/swa...
File appengine/swarming/swarming_bot/bot_code/file_reader.py (right):

https://codereview.chromium.org/2024313003/diff/120001/appengine/swarming/swa...
appengine/swarming/swarming_bot/bot_code/file_reader.py:75: except (OSError,
IOError, ValueError) as e:
IOError, OSError, ValueError

https://codereview.chromium.org/2024313003/diff/120001/appengine/swarming/swa...
appengine/swarming/swarming_bot/bot_code/file_reader.py:80: time.sleep(0.05)
I'd prefer using:
self._signal.get(timeout=0.05)

so line 56 hangs less if there's an issue with the file.

https://codereview.chromium.org/2024313003/diff/120001/appengine/swarming/swa...
File appengine/swarming/swarming_bot/bot_code/file_refresher.py (right):

https://codereview.chromium.org/2024313003/diff/120001/appengine/swarming/swa...
appengine/swarming/swarming_bot/bot_code/file_refresher.py:33: def start(self):
optional nit: I'm not sure start() is really needed, I'd just merge this
function into __init__(), same for FileReaderThread. But I don't mind.

https://codereview.chromium.org/2024313003/diff/120001/appengine/swarming/swa...
appengine/swarming/swarming_bot/bot_code/file_refresher.py:81: time.sleep(0.1)
self._signal.get(timeout=0.05)

https://codereview.chromium.org/2024313003/diff/120001/appengine/swarming/swa...
File appengine/swarming/swarming_bot/bot_code/task_runner.py (right):

https://codereview.chromium.org/2024313003/diff/120001/appengine/swarming/swa...
appengine/swarming/swarming_bot/bot_code/task_runner.py:260: def
read_auth_headers(path):
dead code?

[Vadim Sh., 2016-06-06 22:17:09]

https://codereview.chromium.org/2024313003/diff/120001/appengine/swarming/swa...
File appengine/swarming/swarming_bot/bot_code/file_reader.py (right):

https://codereview.chromium.org/2024313003/diff/120001/appengine/swarming/swa...
appengine/swarming/swarming_bot/bot_code/file_reader.py:75: except (OSError,
IOError, ValueError) as e:
On 2016/06/06 21:18:36, M-A Ruel wrote:
> IOError, OSError, ValueError

Done.

https://codereview.chromium.org/2024313003/diff/120001/appengine/swarming/swa...
appengine/swarming/swarming_bot/bot_code/file_reader.py:80: time.sleep(0.05)
On 2016/06/06 21:18:36, M-A Ruel wrote:
> I'd prefer using:
> self._signal.get(timeout=0.05)
> 
> so line 56 hangs less if there's an issue with the file.

Done.

https://codereview.chromium.org/2024313003/diff/120001/appengine/swarming/swa...
File appengine/swarming/swarming_bot/bot_code/file_refresher.py (right):

https://codereview.chromium.org/2024313003/diff/120001/appengine/swarming/swa...
appengine/swarming/swarming_bot/bot_code/file_refresher.py:33: def start(self):
On 2016/06/06 21:18:36, M-A Ruel wrote:
> optional nit: I'm not sure start() is really needed, I'd just merge this
> function into __init__(), same for FileReaderThread. But I don't mind.

Having explicit 'start' is better. It gives a chance to "register" the object in
all proper places before starting it.

https://codereview.chromium.org/2024313003/diff/120001/appengine/swarming/swa...
appengine/swarming/swarming_bot/bot_code/file_refresher.py:81: time.sleep(0.1)
On 2016/06/06 21:18:36, M-A Ruel wrote:
> self._signal.get(timeout=0.05)

Done.

https://codereview.chromium.org/2024313003/diff/120001/appengine/swarming/swa...
File appengine/swarming/swarming_bot/bot_code/task_runner.py (right):

https://codereview.chromium.org/2024313003/diff/120001/appengine/swarming/swa...
appengine/swarming/swarming_bot/bot_code/task_runner.py:260: def
read_auth_headers(path):
On 2016/06/06 21:18:36, M-A Ruel wrote:
> dead code?

oops, removed

[M-A Ruel, 2016-06-06 22:48:53]

lgtm

[Vadim Sh., 2016-06-07 22:22:59]

Please take another look at the latest patchset. 

I made it more "future friendly" in preparation for adding per-task service
account support.

https://codereview.chromium.org/2024313003/diff/160001/appengine/swarming/swa...
File appengine/swarming/swarming_bot/bot_code/bot_auth.py (right):

https://codereview.chromium.org/2024313003/diff/160001/appengine/swarming/swa...
appengine/swarming/swarming_bot/bot_code/bot_auth.py:9: AuthParams =
collections.namedtuple('AuthParams', [
This is expected to grow in the future to include:

1. Special token that identifies service account the task is running under. It
can be used to get a real OAuth token for that service account on demand.
2. URL of token-server to use to grab the access token.

It will be used not only by task_runner, but also by the tasks themselves, if
they support Swarming auth. That's why I put it into environment variable.

[M-A Ruel, 2016-06-07 22:41:19]

I like it more, lgtm

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

https://codereview.chromium.org/2024313003/diff/160001/appengine/swarming/doc...
appengine/swarming/doc/Magic-Values.md:36: - `SWARMING_HEADLESS=1` is always
set.
We should probably get rid of it one day but I think we depend on it somewhere.

https://codereview.chromium.org/2024313003/diff/160001/appengine/swarming/doc...
appengine/swarming/doc/Magic-Values.md:38: - `SWARMING_TASK_ID` is set to the
task id.
Thanks! I had totally forgot to document it.

[Vadim Sh., 2016-06-07 22:45:08]

The CQ bit was checked by vadimsh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-07 22:45:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2024313003/160001

[commit-bot: I haz the power, 2016-06-07 22:48:20]

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

[commit-bot: I haz the power, 2016-06-07 22:48:21]

Dry run: This issue passed the CQ dry run.

[Vadim Sh., 2016-06-07 23:41:28]

The CQ bit was checked by vadimsh@chromium.org

[Vadim Sh., 2016-06-07 23:41:29]

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/2024313003/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-07 23:41:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2024313003/180001

[commit-bot: I haz the power, 2016-06-07 23:44:58]

Description was changed from

==========
Send authorization headers when calling Swarming backend.

What headers to send are decided by bot_config.py. It allows different bots to
use different authentication mechanisms (e.g. some bots may use machine tokens,
some may use GCE service accounts, some may use predeployed JSON keys, etc).

R=maruel@chromium.org
BUG=
==========

to

==========
Send authorization headers when calling Swarming backend.

What headers to send are decided by bot_config.py. It allows different bots to
use different authentication mechanisms (e.g. some bots may use machine tokens,
some may use GCE service accounts, some may use predeployed JSON keys, etc).

R=maruel@chromium.org
BUG=

Committed:
https://github.com/luci/luci-py/commit/3285ba3b48309de808817a4ee9d61466bc0e5485
==========

[commit-bot: I haz the power, 2016-06-07 23:44:59]

Committed patchset #10 (id:180001) as
https://github.com/luci/luci-py/commit/3285ba3b48309de808817a4ee9d61466bc0e5485