Monitoring proxy for time series data

Introducing the bare bones monitoring proxy.

BUG=461996
R=agable@chromium.org,stip@chromium.org
CC=pgervais@chromium.org

Committed: https://chromium.googlesource.com/infra/infra/+/73d6305244bd455777957602e1aa1d9bb7634a27

[Sergey Berezin, 2015-02-18 03:10:39]

This is NOT ready for review. I'm uploading this CL to warm you up for a
somewhat longish review.
Current state: 
 * No tests (will write soon);
 * Admin UI is super-primitive, but it works - updates credentials in datastore;
 * Authentication is nonexistent yet, I'll likely use the "auth" module, even
though it's still under swarming sub-repo. Or, to simplify this CL, might use
endpoint's built-in authentication.
 * Signing with monacq credentials is not implemented yet; VM pipeline is still
PoC just to test the data flow. To be implemented (should be simple).
 * The Cloud project is all setup with NAT boxes, static IPs are whitelisted,
Stackdriver dashboards and alerts are setup to monitor all the VMs.
 * Custom load balancer needs to be implemented; there is a stub in main.py.
 * Health checks can hopefully be reused from Stackdriver API and cached in
datastore / memcache (not implemented yet - can be postponed to later CLs)

[Sergey Berezin, 2015-03-05 03:11:54]

FYI, I uploaded a new version. Still no tests, and I'm still fighting an
httplib2 bug that fails to send a binary body (d'oh!), even though it happily
propagates all the way through the rest of the pipeline.

The code is getting closer to its final form, and modulo bug fixes and tests,
shouldn't change too much.

[pgervais, 2015-03-05 17:33:43]

On 2015/03/05 03:11:54, Sergey Berezin wrote:
> FYI, I uploaded a new version. Still no tests, and I'm still fighting an
> httplib2 bug that fails to send a binary body (d'oh!), even though it happily
> propagates all the way through the rest of the pipeline.
> 
Why not using requests, then :-)?

> The code is getting closer to its final form, and modulo bug fixes and tests,
> shouldn't change too much.

[Sergey Berezin, 2015-03-05 18:17:43]

On 2015/03/05 17:33:43, pgervais wrote:
> On 2015/03/05 03:11:54, Sergey Berezin wrote:
> > FYI, I uploaded a new version. Still no tests, and I'm still fighting an
> > httplib2 bug that fails to send a binary body (d'oh!), even though it
happily
> > propagates all the way through the rest of the pipeline.
> > 
> Why not using requests, then :-)?

I would use something else (like urllib2 which works just fine), if I could
figure out how to use it with oauth2 and Google credentials. The docs are
clearly biased towards httplib2 :-)

[Sergey Berezin, 2015-03-06 03:04:36]

Added auth module, and now authentication works all the way through the pipeline
(using the /test endpoint instead of the real endpoint).

The last snag: the data is corrupted in the last step somewhere.

[Sergey Berezin, 2015-03-10 00:47:28]

+pgervais FYI

Fixed the mangling of data in httplib2 (had to set content-type to
application/octet-stream).

I've invited you to the project (chrome-infra-mon-proxy), you should be able to
access the /auth endpoint.
I'll also change the /admin endpoint to authenticate through the same /auth
groups.

Will deploy this version tomorrow, and we can start sending data.

Next up - tests.

[Sergey Berezin, 2015-04-01 00:55:22]

PTAL: this is ready for review, complete with tests.

[Sergey Berezin, 2015-04-01 01:26:39]

Hmm, weird. Locally, tests show 100% coverage and pass cleanly. On the bot, I'm
missing a few branches. Will look closer tomorrow, must be some nondeterminism
in tests.

[pgervais, 2015-04-01 16:36:13]

We also hit some coverage flakiness in the past few days (all tests pass,
coverage is not 100%).

On Tue, Mar 31, 2015 at 6:26 PM, <sergeyberezin@chromium.org> wrote:

> Hmm, weird. Locally, tests show 100% coverage and pass cleanly. On the
> bot, I'm
> missing a few branches. Will look closer tomorrow, must be some
> nondeterminism
> in tests.
>
>
> https://codereview.chromium.org/928043005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sergey Berezin, 2015-04-02 19:26:38]

The tests now pass. Please start reviewing - I'd appreciate comments, even if
it's only one small file at a time :-) Thanks!

[Sergey Berezin, 2015-04-10 23:10:14]

sergeyberezin@chromium.org changed reviewers:
+ pgervais@chromium.org

[Sergey Berezin, 2015-04-10 23:10:15]

Small changes to update VM counts.

[Sergey Berezin, 2015-04-11 01:54:08]

Ran another load test up to ~650 qps today. Got GAE task queues to buckle... The
proxy seems to handle ~400 QPS more or less fine, is stretched at 500, and can't
keep up beyond that.
Interestingly, 3 NAT boxes (2-core each) can handle all of this traffic and more
just fine.

Also kubernetes (underlying managed VMs) seems to create many invalid VMs when
it auto-scales. At some point, I had 8 good and 8 bad VMs for one of the
modules. There is nothing one can do with the bad VMs, since they don't actually
exist. The only way to get rid of them is to drain the module and let the
auto-scale remove them.

I may need to create more task queues to handle the load, more modules to even
out the load, and a way to drain modules independently. And maybe set the number
of instances for each module manually (so the auto-scaler doesn't screw it up).

And maybe rewrite it in Go, so we don't need as many VMs... But not in this
iteration :-)

[ghost stip (do not use), 2015-04-14 00:36:44]

this code was pleasant to review, despite being a large change. thank you for
writing such clear code!

most of my comments center around how you're serving up the human-visible
webpages. I think there is a lot of cleanup that can be done by leveraging jinja
and some app.yaml settings.

I'm hesitant to check in the loads scripts as-is, especially since one writes to
your local crontab :). maybe we can move the scripts to an experimental
directory since they're most likely only to be used when we do capacity
re-evaluations

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
File appengine/chrome_infra_mon_proxy/admin_handler.py (right):

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/admin_handler.py:39: def serialize(value):
I'm not entirely sure what these are used for. this is just for decoding text
entry?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/admin_handler.py:78:
self.response.write(open('templates/set_credentials.html').read() % params)
you should use jinja

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/admin_handler.py:98:
self.response.headers["Content-Type"] = 'text/html'
I think this is set by default?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/admin_handler.py:109:
open('templates/set_credentials.html').read() % params)
again, jinja. from what I'm reading, you can replace lines 87-109 with a 3 line
jinja call, and remove serialize and deserialize entirely. note that jinja
templates can do if statements and iteration over lists/dicts

 take a look at like
https://chromium.googlesource.com/infra/infra/+/master/appengine/cr_rev/appen....
you can copy  BaseHandler() as boilerplate that sets up a cached jinja instance
(one that looks for templates in templates/). then look at how ridiculously
simple the code in MainPage() is

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/admin_handler.py:117: class
AdminDispatch(webapp2.RequestHandler):
I think you can remove lines 112 - 128 if you just set login:admin in the
app.yaml

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
File appengine/chrome_infra_mon_proxy/app.yaml (right):

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/app.yaml:11: 
- url: /admin/*
  secure:always
  login:admin
  script: main.app

unless you have a custom admin list?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/app.yaml:15: - name: jinja2
lol

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
File appengine/chrome_infra_mon_proxy/common.py (right):

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/common.py:1: # Copyright 2015 The Chromium
Authors. All rights reserved.
rename to something more meaningful like 'credentials'

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/common.py:15: def is_development_server():
there is another implementation of this, meaning it's *not* common :). inline it
back to the place that calls it?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/common.py:19: class MonAcqData(ndb.Model):
MonAcqCredentials

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/common.py:27: def get_data():
def _get_datastore_credentials()

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/common.py:35: def
get_credentials(credentials_dict, scopes):
def get_credentials(credentials_dict=None, scopes=None)

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/common.py:38: 
credentials_dict = credentials_dict or
self._get_datastore_credentials()['credentials']
scopes = scopes or self._get_datastore_credentials()['scopes']

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
File appengine/chrome_infra_mon_proxy/main.py (right):

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/main.py:26: VM_MODULES = ['vm1', 'vm2', 'vm3']
oppan dijkstra style:

VM_MODULE_COUNT = 3
VM_MODULES = ['vm%d' % i for i in range(VM_MODULE_COUNT)]

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/main.py:56: # Seconds since UNIX epoch when
last health check was made.
probably want to remove now until we figure out how to check NAT health?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/main.py:70: return random.randint(0,
len(modules)-1)
return random.choice(modules)

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/main.py:75: # logging.debug('Forwarding data:
%s', common.payload_stats(data))
why commented out? seems legit

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/main.py:78: queue_name =
VM_MODULES[lb.choose_module()]
queue_name = lb.choose_module()

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/main.py:86: xsrf_token_enforce_on = [] #
pragma: no cover
nit: two spaces before comment

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/main.py:91: self.response.write('GET requests
are not allowed.')
I think if you don't provide a get function to begin with the person gets a 405
automatically

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/main.py:92: self.response.set_status(403)
at least make it 405

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/main.py:106: msg = 'There is nothing here
yet.\n'
make this a little more friendly!

msg = 'Welcome to the monitoring proxy! Nothing to see here (yet).'

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/main.py:107:
self.response.headers['Content-Type'] = 'text/plain'
is this needed?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
File appengine/chrome_infra_mon_proxy/queue.yaml (right):

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/queue.yaml:10: task_age_limit: 1s
this is cool? we don't lose data because of this?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
File appengine/chrome_infra_mon_proxy/scripts/loadtest_docker/Dockerfile_base
(right):

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/scripts/loadtest_docker/Dockerfile_base:23: #
RUN pip install --upgrade httplib2
remove lines 22 / 23

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
File appengine/chrome_infra_mon_proxy/scripts/loadtest_setup.sh (right):

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/scripts/loadtest_setup.sh:2: # Run this script
from its directory, to correctly pick up loadtest_startup.sh:
might be a good idea to move all the loadtest scripts to a separate directory

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
File appengine/chrome_infra_mon_proxy/scripts/loadtest_startup.sh (right):

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/scripts/loadtest_startup.sh:4: echo "* * * * *
/bin/bash -c \"sudo docker run --rm
gcr.io/chrome_infra_mon_proxy/loadtest_08bb1f6c6e43\"" >> crontab
oh my

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
File appengine/chrome_infra_mon_proxy/scripts/nat_setup.sh (right):

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/scripts/nat_setup.sh:2: # Run this script from
its directory, to correctly pick up nat_startup.sh:
this is intended for humans, not some automated thing correct?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
File appengine/chrome_infra_mon_proxy/scripts/nat_startup.sh (right):

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/scripts/nat_startup.sh:3: sudo iptables -t nat
-A POSTROUTING -o eth0 -j MASQUERADE
friedman, does this look OK?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/scripts/nat_startup.sh:8: python -m
SimpleHTTPServer 8000 &
nice

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
File appengine/chrome_infra_mon_proxy/test.yaml (right):

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/test.yaml:2: # version: 2
might as well just remove?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/test.yaml:11: script: test_module.test
so this is a module on the app just for the loadtest?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
File appengine/chrome_infra_mon_proxy/test/__init__.py (right):

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/test/__init__.py:4: 
¯\_(ツ)_/¯

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
File appengine/chrome_infra_mon_proxy/test/admin_handler_test.py (right):

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/test/admin_handler_test.py:16: class
AdminTest(testing.AppengineTestCase):
nit: insert another newline

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
File appengine/chrome_infra_mon_proxy/test/common_test.py (right):

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/test/common_test.py:33: 'private_key':
'deadbeafyoudneverguess',
beef? :p

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
File appengine/chrome_infra_mon_proxy/test/test_module_test.py (right):

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/test/test_module_test.py:29: #   
self.mock(test_module.TestHandler, 'xsrf_token_enforce_on', [])
these are commented out. should we just delete the whole setUp() function?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
File appengine/chrome_infra_mon_proxy/test_module.py (right):

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/test_module.py:28: To start a load test, set
qps to a positive value.
I'm not sure exactly how this tests works. Does it spin up a local server, and
the unit test system hits it? does it test a live server?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
File appengine/chrome_infra_mon_proxy/vm_module.py (right):

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/vm_module.py:7: import httplib2
nit: alphabetize

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/vm_module.py:16: import common
you should rename this to be more specific. common what?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/vm_module.py:23: class
VMHandler(webapp2.RequestHandler):
it pleases me how small this file is :)

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/vm_module.py:27: self.response.out.write(msg)
self.response.set_status(403) ?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/vm_module.py:30: requester_id =
self.request.headers.get('X-Appengine-Inbound-Appid', None)
isn't None default? I think you can just do
self.request.headers.get('X-Appengine-Inbound-Appid')

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/vm_module.py:31: task_name =
self.request.headers.get('X-AppEngine-TaskName', None)
same

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/vm_module.py:33: authorized =
is_development_server() or task_name or requester_id == my_id
nit: might want to make the 'is me' check more obvious:

def requester_is_me(requester_id):
  return requester_id == app_identity.get_application_id()

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/vm_module.py:36: # logging.debug('Received POST
/vm: %s',
nit: remove commented code

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/vm_module.py:38: data = common.get_data()
can you rename get_data to be more specific? what does it do? where does the
data come from?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/vm_module.py:47: credentials =
common.get_credentials(data['credentials'], data['scopes'])
if data came from common, why  do I have to pass it back into common again?
shouldn't I be able to just call common.get_credentials() ?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/vm_module.py:49: def callback(_response,
_content):
I'm surprised this is needed. can you just set the callback to be None? do we
want to check response for error and log it?

https://chromiumcodereview.appspot.com/928043005/diff/380001/appengine/chrome...
appengine/chrome_infra_mon_proxy/vm_module.py:69: (r'/vm', VMHandler),
probably don't need the r here

[Sergey Berezin (google), 2015-04-14 00:38:02]

Heads up: after another round of loadtests, I decided to switch back to
synchronous requests, removing the task queues. They have lots of problems of
their own, and I found a way to keep the number of instances smaller. A patch
will follow tomorrow.

[Sergey Berezin (google), 2015-04-14 00:41:17]

On 2015/04/14 00:36:44, stip wrote:
> this code was pleasant to review, despite being a large change. thank you for
> writing such clear code!

Yay, thanks for review! Will respond tomorrow.

[agable, 2015-04-14 21:46:25]

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/admin_handler.py (right):

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:32:
self.response.write(open('templates/admin.html').read())
Would be way better to use a real templating engine like jinja2 or django.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:39: def serialize(value):
Not sure what the point of this function is. What it does is pretty
straightforward, but the *why* isn't.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:46: return 'Unsupported type'
Why shouldn't this be a ValueError as well?

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:71: def get(self):
This function is kinda odd. So, it presents a form to the user, where the fields
of that form are dynamically determined by the union of self.fields and the
fields on the CREDENTIALS_KEY datastore object? Since the datastore has a real
schema, we should maybe just use that to store exactly the fields needed by the
credential key, and nothing else.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:85: updated_fields = []
Why do we care about updated fields? Seems like updating the credential should
update the whole thing. Create a whole new datastore object with the new
credentials, and optionally move the old one aside and save it for the sake of
archaeology.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:117: class
AdminDispatch(webapp2.RequestHandler):
I don't see the purpose of the AdminCommand base class; both AdminPage and
SetCredentials should inherit directly from webapp2.RequestHandler and have
'login: admin' set in app.yaml.

Alternatively, they should inherit from a base class called AdminHandler which
enforces admin stuff, and they should super() call its methods before continuing
with their real work.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/app.yaml (right):

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/app.yaml:15: - name: jinja2
Yes! Use jinja2 :)

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/common.py (right):

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/common.py:12: CREDENTIALS_KEY =
'credentials_key'
Either this should be 'data_key' or the object should be 'MonAcqCredentials'

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/common.py:19: class MonAcqData(ndb.Model):
I would have a wholly separate datastore model just for the json p8 credentials.
That way we can just create a new datastore entry when the credentials are
updated, and keep the old ones around so we can see the history of updates if we
want to. Updating the credentials shouldn't modify just a portion of this global
config object.

That said, this object seems to mostly just be a credentials object, not a
general data object, so maybe just the name and update mechanism could be
changed.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/common.py:29: logging.info('get_data(): entity
= %r', data_entity)
Seems inconsistent to log when we get_data, but not when the admin handler
updates the data.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/common.py:40: # However, it insists to load
only from a file. So, here's a hack.
Why can't we read the json credentials from the datastore into a CStringIO
StringIO object, which behaves like a file, and let GoogleCredentials read from
that? Does it detect that and complain? Python appengine does have the StringIO
library available.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/main.py (right):

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:26: VM_MODULES = ['vm1', 'vm2', 'vm3']
Sucks a bit (or a lot) to hardcode this here. Would be really nice to
autogenerate everything about N different VMs just from a single integer
somewhere.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:29: def is_group_member(group_name):
'require_group_membership()' might be a better name.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:79: logging.info('Selected queue: %s',
queue_name)
nit: more context in logging. To quote a TotT: "Every log message should contain
enough context that if that were the only log line you received, you could still
start investigating."

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:92: self.response.set_status(403)
Why do this at all? If you don't implement the get() method, AppEngine will
automatically make it come back as an unsupported method (405).

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:97: # logging.info('Received POST
/monacq: %s',
This seems like useful logging to have around.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:107:
self.response.headers['Content-Type'] = 'text/plain'
Let's use html and a real template here.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/scripts/loadtest_setup.sh (right):

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/scripts/loadtest_setup.sh:53: if false; then
So, this script has been neutered. Why land it in this state?

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/vm2.yaml (right):

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm2.yaml:1: application: chrome-infra-mon-proxy
Unfortunate that we have 3 files for the 3 vms. Prevents us from landing a
1-line change to scale up from 3 to 5, or something similar. Is there any way to
abstract across a single template yaml file? If not, no problem, but maybe worth
investigating.

[Sergey Berezin (google), 2015-04-15 01:38:07]

sergeyberezin@google.com changed reviewers:
+ sergeyberezin@google.com

[Sergey Berezin (google), 2015-04-15 01:38:10]

Switched to jinja2 in admin UI, responded to comments in admin_handler. Thanks
for reviews! I'll continue working on your comments tomorrow.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/admin_handler.py (right):

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:39: def serialize(value):
On 2015/04/14 00:36:42, stip wrote:
> I'm not entirely sure what these are used for. this is just for decoding text
> entry?

Function removed.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:78:
self.response.write(open('templates/set_credentials.html').read() % params)
On 2015/04/14 00:36:41, stip wrote:
> you should use jinja

Done.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:98:
self.response.headers["Content-Type"] = 'text/html'
On 2015/04/14 00:36:42, stip wrote:
> I think this is set by default?

True. And now irrelevant (jinja!)

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:109:
open('templates/set_credentials.html').read() % params)
On 2015/04/14 00:36:42, stip wrote:
> again, jinja. from what I'm reading, you can replace lines 87-109 with a 3
line
> jinja call, and remove serialize and deserialize entirely. note that jinja
> templates can do if statements and iteration over lists/dicts
> 
>  take a look at like
>
https://chromium.googlesource.com/infra/infra/+/master/appengine/cr_rev/appen....
> you can copy  BaseHandler() as boilerplate that sets up a cached jinja
instance
> (one that looks for templates in templates/). then look at how ridiculously
> simple the code in MainPage() is

Done. Thanks for the pointers!

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:117: class
AdminDispatch(webapp2.RequestHandler):
On 2015/04/14 00:36:41, stip wrote:
> I think you can remove lines 112 - 128 if you just set login:admin in the
> app.yaml

First, I'd really like to switch to auth groups here, but don't have time to
debug it in this version. Next CL.

Also, I just tested app.yaml, and it is sensitive to the order in which you
specify the endpoints. If /admin/.* is (mistakenly) specified after /.*, it will
happily serve sensitive data to anyone. It's not easy to test for, so I'll add
login:admin, but also keep these checks, just to be sure.

Also, I plan to have more commands for the admin/ endpoint (like setting load
level to individual modules), so I'd keep the dispatcher code.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/app.yaml (right):

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/app.yaml:11: 
On 2015/04/14 00:36:42, stip wrote:
> - url: /admin/*
>   secure:always
>   login:admin
>   script: main.app
> 
> unless you have a custom admin list?

Done - for now. Though I'm thinking of using an auth group for this endpoint,
once I understand its use of xsrf tokens. Deferred to future CLs.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/app.yaml:15: - name: jinja2
On 2015/04/14 00:36:42, stip wrote:
> lol

Yeah, I know :-) It's used by the auth module...

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/common.py (right):

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/common.py:1: # Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/04/14 00:36:42, stip wrote:
> rename to something more meaningful like 'credentials'

It is actually a common set of functions, not just credentials. I'll rename the
key to be more generic, since it's no longer just credentials.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/common.py:15: def is_development_server():
On 2015/04/14 00:36:42, stip wrote:
> there is another implementation of this, meaning it's *not* common :). inline
it
> back to the place that calls it?

I fixed that, it's now the only (common) implementation. Thanks for catching!

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/common.py:19: class MonAcqData(ndb.Model):
On 2015/04/14 00:36:42, stip wrote:
> MonAcqCredentials

It's not only creds, it's also the URL for the endpoint, the scopes, and the
additional headers. I'm planning to add possibly load percentages for the
modules. So, I think Data is more appropriate.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/common.py:27: def get_data():
On 2015/04/14 00:36:42, stip wrote:
> def _get_datastore_credentials()

I'm moving this to vm_module.py, it's the only place it's used.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/admin_handler.py (right):

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:32:
self.response.write(open('templates/admin.html').read())
On 2015/04/14 21:46:24, agable wrote:
> Would be way better to use a real templating engine like jinja2 or django.

Done.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:39: def serialize(value):
On 2015/04/14 21:46:24, agable wrote:
> Not sure what the point of this function is. What it does is pretty
> straightforward, but the *why* isn't.

Removed it, replaced with setParams(), which should be more straightforward.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:46: return 'Unsupported type'
On 2015/04/14 21:46:24, agable wrote:
> Why shouldn't this be a ValueError as well?

No longer relevant - function removed.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:71: def get(self):
On 2015/04/14 21:46:24, agable wrote:
> This function is kinda odd. So, it presents a form to the user, where the
fields
> of that form are dynamically determined by the union of self.fields and the
> fields on the CREDENTIALS_KEY datastore object? Since the datastore has a real
> schema, we should maybe just use that to store exactly the fields needed by
the
> credential key, and nothing else.

I've changed it to be more explicit. The fields are actually fixed both in the
Datastore model, and in the form template, so no point making this function too
generic.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:85: updated_fields = []
On 2015/04/14 21:46:24, agable wrote:
> Why do we care about updated fields? Seems like updating the credential should
> update the whole thing. 

True, it's useless, removed the list, replaced with a boolean (to display the
message when successfully updated something).

> Create a whole new datastore object with the new
> credentials, and optionally move the old one aside and save it for the sake of
> archaeology.

Note, that in the future, not all fields may be updated in the POST request, in
which case the other fields should stay the same. For instance, we may decide to
update credentials in their own form, separate from other fields.

Saving history may be interesting, but I don't see it as critical for the
initial version. That's why just a single key, to keep it simple.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:117: class
AdminDispatch(webapp2.RequestHandler):
On 2015/04/14 21:46:24, agable wrote:
> I don't see the purpose of the AdminCommand base class; both AdminPage and
> SetCredentials should inherit directly from webapp2.RequestHandler and have
> 'login: admin' set in app.yaml.

I expect to add a few more admin functions in the near future (e.g. setting the
load percentage for each module, to drain and gradually bring up modules in a
rolling update).

With your approach, I'd have to touch two files instead of one, and app.yaml is
untestable. Also, each new handler has to implement authorization correctly (I
don't want to rely just on app.yaml - too easy to make a mistake). Plus,
login:admin is a temporary plug; I'd actually like to use auth group for that,
but don't have time to debug it in this version (it fails for me in POST).

> Alternatively, they should inherit from a base class called AdminHandler which
> enforces admin stuff, and they should super() call its methods before
continuing
> with their real work.

Here, forgetting to call the base class may leave the endpoint unprotected
(again, I don't trust app.yaml - I previously made stupid mistakes leaving
endpoints wide open). And there is no good single test for all such
authentications.

Going through AdminDispatch as the only handler guarantees the correct
authentication, enforced in one central place. So, it's fairly easy to use
correctly, and somewhat hard to use incorrectly - my favorite combination :-)

[Sergey Berezin (google), 2015-04-15 16:16:29]

Uploaded the new admin UI version:
https://chrome-infra-mon-proxy.appspot.com/admin/ (you should all have access)

[agable, 2015-04-15 17:14:54]

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/admin_handler.py (right):

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:85: updated_fields = []
> Note, that in the future, not all fields may be updated in the POST request,
in which case the other fields should stay the same. For instance, we may decide
to update credentials in their own form, separate from other fields.
> 
> Saving history may be interesting, but I don't see it as critical for the
initial version. That's why just a single key, to keep it simple.

The fact that we might be updating credentials separate from the other fields is
why I was arguing that there should be a separate Credentials datastore object
in my comments on common.py

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:117: class
AdminDispatch(webapp2.RequestHandler):
On 2015/04/15 at 01:38:09, sergeyberezin wrote:
> On 2015/04/14 21:46:24, agable wrote:
> > I don't see the purpose of the AdminCommand base class; both AdminPage and
> > SetCredentials should inherit directly from webapp2.RequestHandler and have
> > 'login: admin' set in app.yaml.
> 
> I expect to add a few more admin functions in the near future (e.g. setting
the load percentage for each module, to drain and gradually bring up modules in
a rolling update).
> 
> With your approach, I'd have to touch two files instead of one, 

Nope, still just one, since app.yaml can apply the 'login: admin' to *every*
path under /admin/

> and app.yaml is untestable. Also, each new handler has to implement
authorization correctly (I don't want to rely just on app.yaml - too easy to
make a mistake). Plus, login:admin is a temporary plug; I'd actually like to use
auth group for that, but don't have time to debug it in this version (it fails
for me in POST).

This is a valid reason: letting people who are not AppEngine admins make
administrative changes.

> 
> > Alternatively, they should inherit from a base class called AdminHandler
which
> > enforces admin stuff, and they should super() call its methods before
continuing
> > with their real work.
> 
> Here, forgetting to call the base class may leave the endpoint unprotected
(again, I don't trust app.yaml - I previously made stupid mistakes leaving
endpoints wide open). And there is no good single test for all such
authentications.
> 
> Going through AdminDispatch as the only handler guarantees the correct
authentication, enforced in one central place. So, it's fairly easy to use
correctly, and somewhat hard to use incorrectly - my favorite combination :-)

Except that the *real* handlers you're implementing have no base class to mimic
and no well-defined API to implement. They just inherit from (object) and then
have some get() and post() methods for the heck of it. That seems 'easy to get
wrong' to me.

[Sergey Berezin (google), 2015-04-16 04:39:09]

Responded to all the comments, and did some refactoring - the code definitely
improved - thanks! PTAL.

New in this version:
 * No more task queues; we are back to synchronous processing. Will do loadtests
tomorrow to confirm it works as expected.
 * All rendered pages are jinja2 templates.
 * Root page is now meaningful.
 * A bunch of refactoring, cleanups, etc.

Heads up: I'm thinking of dropping all the loadtest infrastructure and landing
it later as a separate app. It doesn't really belong here. OTOH, we still need
it for later enhancements and future loadtesting.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/common.py (right):

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/common.py:35: def
get_credentials(credentials_dict, scopes):
On 2015/04/14 00:36:42, stip wrote:
> def get_credentials(credentials_dict=None, scopes=None)

Moved to vm_module. I don't see the need for defaults here, since the function
is only used once.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/common.py:38: 
On 2015/04/14 00:36:42, stip wrote:
> credentials_dict = credentials_dict or
> self._get_datastore_credentials()['credentials']
> scopes = scopes or self._get_datastore_credentials()['scopes']

Same as above.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/main.py (right):

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:26: VM_MODULES = ['vm1', 'vm2', 'vm3']
On 2015/04/14 00:36:42, stip wrote:
> oppan dijkstra style:
> 
> VM_MODULE_COUNT = 3
> VM_MODULES = ['vm%d' % i for i in range(VM_MODULE_COUNT)]

I feel the temptation :-) But this suggests the number of modules can be
parameterized, and honestly, I want to fix it to exactly 3, and make it hard to
add more. It's already too many as it is for maintenance. I need N+1 redundancy,
and N=2 is the minimum viable in this case (avoiding SPOF and just handling
load). So I'm pulling an exception to the generalization rule. And it's 2 lines
instead of 1 :-)

Context: the number of modules is dictated by the number of NAT boxes I need to
have. I couldn't make those load balance separately, so I have to load balance
at the level of modules, each with its own NAT box. 3 us-central1 zones, 3 NAT
boxes.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:56: # Seconds since UNIX epoch when
last health check was made.
On 2015/04/14 00:36:42, stip wrote:
> probably want to remove now until we figure out how to check NAT health?

Done.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:70: return random.randint(0,
len(modules)-1)
On 2015/04/14 00:36:42, stip wrote:
> return random.choice(modules)

Done. Nice, thanks!

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:75: # logging.debug('Forwarding data:
%s', common.payload_stats(data))
On 2015/04/14 00:36:42, stip wrote:
> why commented out? seems legit

It computes md5, and now that I debugged the headers, I don't really need it
anymore. Saves CPU.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:78: queue_name =
VM_MODULES[lb.choose_module()]
On 2015/04/14 00:36:42, stip wrote:
> queue_name = lb.choose_module()

Done.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:86: xsrf_token_enforce_on = [] #
pragma: no cover
On 2015/04/14 00:36:43, stip wrote:
> nit: two spaces before comment

Done.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:91: self.response.write('GET requests
are not allowed.')
On 2015/04/14 00:36:42, stip wrote:
> I think if you don't provide a get function to begin with the person gets a
405
> automatically

Indeed - removed.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:92: self.response.set_status(403)
On 2015/04/14 00:36:42, stip wrote:
> at least make it 405

N/A - removed def get().

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:106: msg = 'There is nothing here
yet.\n'
On 2015/04/14 00:36:42, stip wrote:
> make this a little more friendly!
> 
> msg = 'Welcome to the monitoring proxy! Nothing to see here (yet).'

Done - created an actual page with some useful content.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:107:
self.response.headers['Content-Type'] = 'text/plain'
On 2015/04/14 00:36:42, stip wrote:
> is this needed?

Nope - it's all gone now.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/queue.yaml (right):

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/queue.yaml:10: task_age_limit: 1s
On 2015/04/14 00:36:43, stip wrote:
> this is cool? we don't lose data because of this?

We may, but with timeseries we'll lose data even on Monarch side.

In any case, my tests show that task queues are not appropriate for proxy, I'll
be removing this file altogether.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/scripts/loadtest_docker/Dockerfile_base
(right):

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/scripts/loadtest_docker/Dockerfile_base:23: #
RUN pip install --upgrade httplib2
On 2015/04/14 00:36:43, stip wrote:
> remove lines 22 / 23

Done.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/scripts/loadtest_startup.sh (right):

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/scripts/loadtest_startup.sh:4: echo "* * * * *
/bin/bash -c \"sudo docker run --rm
gcr.io/chrome_infra_mon_proxy/loadtest_08bb1f6c6e43\"" >> crontab
On 2015/04/14 00:36:43, stip wrote:
> oh my

Oh yeah :-) There is a long and sad story behind this too. The gist of it is,
oauth token refresh fails for me after about a minute, for unknown reason (DoS
protection?), but restarting a process somehow fixes it. So I had to set up a
cron job to start a new process reliably. Of course, this is the fastest hack I
could think of, and but actually works.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/scripts/nat_setup.sh (right):

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/scripts/nat_setup.sh:2: # Run this script from
its directory, to correctly pick up nat_startup.sh:
On 2015/04/14 00:36:43, stip wrote:
> this is intended for humans, not some automated thing correct?

Yes. This is how I'd fix a fried NAT box when I get a Stackdriver alert at 2am.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/scripts/nat_startup.sh (right):

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/scripts/nat_startup.sh:8: python -m
SimpleHTTPServer 8000 &
On 2015/04/14 00:36:43, stip wrote:
> nice

:-)

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/test.yaml (right):

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/test.yaml:2: # version: 2
On 2015/04/14 00:36:43, stip wrote:
> might as well just remove?

Done.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/test.yaml:11: script: test_module.test
On 2015/04/14 00:36:43, stip wrote:
> so this is a module on the app just for the loadtest?

Yes, that's right. Arguably, I could've used a whole different app. Maybe I
should actually do that... But it was convenient to have it in one place.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/test/__init__.py (right):

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/test/__init__.py:4: 
On 2015/04/14 00:36:43, stip wrote:
> ¯\_(ツ)_/¯

I totally have a copyright on a blank line!

All of this because someone at some point complained that empty files were
"bad"... Whatever.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/test/admin_handler_test.py (right):

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/test/admin_handler_test.py:16: class
AdminTest(testing.AppengineTestCase):
On 2015/04/14 00:36:43, stip wrote:
> nit: insert another newline

Done.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/test/common_test.py (right):

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/test/common_test.py:33: 'private_key':
'deadbeafyoudneverguess',
On 2015/04/14 00:36:43, stip wrote:
> beef? :p

because chicken is not hex?

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/test/test_module_test.py (right):

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/test/test_module_test.py:29: #   
self.mock(test_module.TestHandler, 'xsrf_token_enforce_on', [])
On 2015/04/14 00:36:43, stip wrote:
> these are commented out. should we just delete the whole setUp() function?

Done.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/test_module.py (right):

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/test_module.py:28: To start a load test, set
qps to a positive value.
On 2015/04/14 00:36:43, stip wrote:
> I'm not sure exactly how this tests works. Does it spin up a local server, and
> the unit test system hits it? does it test a live server?

The /loadtest endpoint does one simple thing - stores the desired qps and
payload size, and serves it to the client (scripts/loadtest_docker/loadtest.py)
that sends the data of that size to the live server at the requested qps. The
endpoint URL is hardcoded into loadtest.py, and the actual data is random.

I then spin up N VMs running this client, so my actual loadtest is qps * N
requests of that size. When VMs are live, I can change the load dynamically by
updating the numbers through /loadtest.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/vm_module.py (right):

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:7: import httplib2
On 2015/04/14 00:36:43, stip wrote:
> nit: alphabetize

Done.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:16: import common
On 2015/04/14 00:36:44, stip wrote:
> you should rename this to be more specific. common what?

I agree it's a lousy name. I just can't think of a better one right now :-)
common_routines? utils? (well, it's not just utils). Any suggestions?

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:23: class
VMHandler(webapp2.RequestHandler):
On 2015/04/14 00:36:44, stip wrote:
> it pleases me how small this file is :)

You should've seen the perl version :-)

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:27: self.response.out.write(msg)
On 2015/04/14 00:36:43, stip wrote:
> self.response.set_status(403) ?

Actually, I just removed get(). It was there for debugging.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:30: requester_id =
self.request.headers.get('X-Appengine-Inbound-Appid', None)
On 2015/04/14 00:36:44, stip wrote:
> isn't None default? I think you can just do
> self.request.headers.get('X-Appengine-Inbound-Appid')

Done.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:31: task_name =
self.request.headers.get('X-AppEngine-TaskName', None)
On 2015/04/14 00:36:44, stip wrote:
> same

Done.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:33: authorized =
is_development_server() or task_name or requester_id == my_id
On 2015/04/14 00:36:43, stip wrote:
> nit: might want to make the 'is me' check more obvious:
> 
> def requester_is_me(requester_id):
>   return requester_id == app_identity.get_application_id()

Done.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:36: # logging.debug('Received POST
/vm: %s',
On 2015/04/14 00:36:43, stip wrote:
> nit: remove commented code

Done.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:38: data = common.get_data()
On 2015/04/14 00:36:43, stip wrote:
> can you rename get_data to be more specific? what does it do? where does the
> data come from?

Renamed, and moved the function to vm_module.py.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:47: credentials =
common.get_credentials(data['credentials'], data['scopes'])
On 2015/04/14 00:36:43, stip wrote:
> if data came from common, why  do I have to pass it back into common again?
> shouldn't I be able to just call common.get_credentials() ?

This function is now also in vm_module.py. It's not used anywhere else.

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:49: def callback(_response,
_content):
On 2015/04/14 00:36:44, stip wrote:
> I'm surprised this is needed. can you just set the callback to be None? do we
> want to check response for error and log it?

Good idea, added logging for an error in response. I don't remember about None,
I think I tried it and it crashed...

https://codereview.chromium.org/928043005/diff/380001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:69: (r'/vm', VMHandler),
On 2015/04/14 00:36:44, stip wrote:
> probably don't need the r here

Done.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/admin_handler.py (right):

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:85: updated_fields = []
On 2015/04/15 17:14:54, agable wrote:
> > Note, that in the future, not all fields may be updated in the POST request,
> in which case the other fields should stay the same. For instance, we may
decide
> to update credentials in their own form, separate from other fields.
> > 
> > Saving history may be interesting, but I don't see it as critical for the
> initial version. That's why just a single key, to keep it simple.
> 
> The fact that we might be updating credentials separate from the other fields
is
> why I was arguing that there should be a separate Credentials datastore object
> in my comments on common.py

I don't yet see the advantage of a separate model, except for organizing. But I
do see a potential efficiency penalty in fetching 2 objects instead of one. When
you need to handle 500 qps, every ms counts.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:117: class
AdminDispatch(webapp2.RequestHandler):
On 2015/04/15 17:14:54, agable wrote:
> On 2015/04/15 at 01:38:09, sergeyberezin wrote:
> > On 2015/04/14 21:46:24, agable wrote:
> > > I don't see the purpose of the AdminCommand base class; both AdminPage and
> > > SetCredentials should inherit directly from webapp2.RequestHandler and
have
> > > 'login: admin' set in app.yaml.
> > 
> > I expect to add a few more admin functions in the near future (e.g. setting
> the load percentage for each module, to drain and gradually bring up modules
in
> a rolling update).
> > 
> > With your approach, I'd have to touch two files instead of one, 
> 
> Nope, still just one, since app.yaml can apply the 'login: admin' to *every*
> path under /admin/
> 
> > and app.yaml is untestable. Also, each new handler has to implement
> authorization correctly (I don't want to rely just on app.yaml - too easy to
> make a mistake). Plus, login:admin is a temporary plug; I'd actually like to
use
> auth group for that, but don't have time to debug it in this version (it fails
> for me in POST).
> 
> This is a valid reason: letting people who are not AppEngine admins make
> administrative changes.
> 
> > 
> > > Alternatively, they should inherit from a base class called AdminHandler
> which
> > > enforces admin stuff, and they should super() call its methods before
> continuing
> > > with their real work.
> > 
> > Here, forgetting to call the base class may leave the endpoint unprotected
> (again, I don't trust app.yaml - I previously made stupid mistakes leaving
> endpoints wide open). And there is no good single test for all such
> authentications.
> > 
> > Going through AdminDispatch as the only handler guarantees the correct
> authentication, enforced in one central place. So, it's fairly easy to use
> correctly, and somewhat hard to use incorrectly - my favorite combination :-)
> 
> Except that the *real* handlers you're implementing have no base class to
mimic
> and no well-defined API to implement. They just inherit from (object) and then
> have some get() and post() methods for the heck of it. That seems 'easy to get
> wrong' to me.

OK, I added a comment to the base class that get() and post() are expected. In
practice, not implementing those will immediate crash your app, so it's hard to
miss those :-) If you insist, I can also add dummy stubs to the base class. It
would add some extra code and tests, but there'll be a template to follow.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/app.yaml (right):

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/app.yaml:15: - name: jinja2
On 2015/04/14 21:46:24, agable wrote:
> Yes! Use jinja2 :)

Done.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/common.py (right):

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/common.py:12: CREDENTIALS_KEY =
'credentials_key'
On 2015/04/14 21:46:25, agable wrote:
> Either this should be 'data_key' or the object should be 'MonAcqCredentials'

Renamed to config_data_key.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/common.py:19: class MonAcqData(ndb.Model):
On 2015/04/14 21:46:24, agable wrote:
> I would have a wholly separate datastore model just for the json p8
credentials.
> That way we can just create a new datastore entry when the credentials are
> updated, and keep the old ones around so we can see the history of updates if
we
> want to. Updating the credentials shouldn't modify just a portion of this
global
> config object.

I'd postpone implementing a history of updates to a later CL. This one is
already big as it is :-)
Also, I need all the data from this record in every single request, and making
two calls to the datastore is slower than one, even if it's memcached.

But if I miss some other design consideration, please let me know.

> That said, this object seems to mostly just be a credentials object, not a
> general data object, so maybe just the name and update mechanism could be
> changed.

Credentials is only one field; it also holds the endpoint URL, the scopes, and
the headers. I consider it a complete config set.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/common.py:29: logging.info('get_data(): entity
= %r', data_entity)
On 2015/04/14 21:46:24, agable wrote:
> Seems inconsistent to log when we get_data, but not when the admin handler
> updates the data.

Added logging to admin_handler.

Also, moved this function to vm_module.py.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/common.py:40: # However, it insists to load
only from a file. So, here's a hack.
On 2015/04/14 21:46:25, agable wrote:
> Why can't we read the json credentials from the datastore into a CStringIO
> StringIO object, which behaves like a file, and let GoogleCredentials read
from
> that? Does it detect that and complain? Python appengine does have the
StringIO
> library available.

GoogleCredentials expects a file path as string... :-)

BTW, I moved this function to vm_module.py.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/main.py (right):

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:26: VM_MODULES = ['vm1', 'vm2', 'vm3']
On 2015/04/14 21:46:25, agable wrote:
> Sucks a bit (or a lot) to hardcode this here. Would be really nice to
> autogenerate everything about N different VMs just from a single integer
> somewhere.

Agreed, but I can't easily auto-generate NAT boxes with static IPs + network
routes. And from my tests, it appears 3 modules is a good N+1 redundancy choice,
so I intend to stick with it long term.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:29: def is_group_member(group_name):
On 2015/04/14 21:46:25, agable wrote:
> 'require_group_membership()' might be a better name.

Renamed.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:79: logging.info('Selected queue: %s',
queue_name)
On 2015/04/14 21:46:25, agable wrote:
> nit: more context in logging. To quote a TotT: "Every log message should
contain
> enough context that if that were the only log line you received, you could
still
> start investigating."

Rephrased: 'Forwarding request to module: %s'

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:92: self.response.set_status(403)
On 2015/04/14 21:46:25, agable wrote:
> Why do this at all? If you don't implement the get() method, AppEngine will
> automatically make it come back as an unsupported method (405).

Done.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:97: # logging.info('Received POST
/monacq: %s',
On 2015/04/14 21:46:25, agable wrote:
> This seems like useful logging to have around.

It runs md5, which is somewhat heavy. I've only used it to investigate how
various pipeline stages pass the data through, and now removed it. We still have
request logs from GAE.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:107:
self.response.headers['Content-Type'] = 'text/plain'
On 2015/04/14 21:46:25, agable wrote:
> Let's use html and a real template here.

Done.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/scripts/loadtest_setup.sh (right):

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/scripts/loadtest_setup.sh:53: if false; then
On 2015/04/14 21:46:25, agable wrote:
> So, this script has been neutered. Why land it in this state?

The intended use is to enable the required portion locally before running, and
keep it a no-op default, for safety.

Since this is a load test, and not a proper part of the proxy, I'm considering
removing it from this CL, and keeping it in the experimental repo.

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/vm2.yaml (right):

https://codereview.chromium.org/928043005/diff/400001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm2.yaml:1: application: chrome-infra-mon-proxy
On 2015/04/14 21:46:25, agable wrote:
> Unfortunate that we have 3 files for the 3 vms. Prevents us from landing a
> 1-line change to scale up from 3 to 5, or something similar. Is there any way
to
> abstract across a single template yaml file? If not, no problem, but maybe
worth
> investigating.

Note, that it's not 3 VMs, but 3 modules, one per NAT box. Each module can scale
the number of instances (VMs) independently.

Making the design generic in the number of NAT boxes gets ugly very fast, and
IMHO is not worth the trouble. Three NAT boxes can handle likely >1000 qps, so I
don't expect needing more. More NAT boxes also means more static IPs to
whitelist, so it's already a hassle.

[Sergey Berezin (google), 2015-04-20 23:04:06]

Small adjustments in *.yaml files for performance: these seem to stabilize the
app, and it can now reasonably handle up to 400 qps. Front-end latency is
300-1000 ms (average under 500ms).

Higher qps is blocked on the IP quota bump (approved, but not applied). I expect
to reach close to 1000 qps at peaks once I have the quota.

[Sergey Berezin (google), 2015-04-22 01:29:17]

Another small adjustment: VMs are now 1-core with target CPU usage at 80%. This
allows 500 qps to be handled by 27-30 cores, which is half the available
capacity.

Next patch will strip off the loadtest code - I'm working on moving it to a
separate app.

[Sergey Berezin (google), 2015-04-22 03:19:09]

The loadtest moved a separate app: http://chrome-infra-loadtest.appspot.com

The code is temporarily checked in here: 
https://chromium.googlesource.com/infra/experimental/+/master/appengine/chrom...

I'll removed it from this CL tomorrow, and later will have a separate CL to move
it to infra.git.

[Sergey Berezin (google), 2015-04-22 17:25:26]

OK, PTAL, it's ready to land.

This is the final working version of the proxy, with load test code removed, and
I believe all the comments addressed. It is now live at
chrome-infra-mon-proxy.appspot.com, receiving traffic from a few slaves and
sending it to a testing endpoint at chrome-infra-loadtest.appspot.com.

It's been load tested at 500 qps using about half the available VMs, so it may
potentially handle a peak of 1 Kqps. Expected cost at a steady 500 qps is about
$100/day.

Its backend IPs are whitelisted for a staging Acq. SRE endpoint that would send
data to Monarch, and we can switch to it any moment.

[agable, 2015-04-23 19:29:38]

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/admin_handler.py (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:49: def setParams(params,
data):
This should be a staticmethod inside SetCredentials. Could be better named or
accompanied by a docstring as well.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:58: 'scopes': lambda v: [l for
l in v.splitlines() if l],
Might want to strip whitespace too

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:116: if not
users.is_current_user_admin():
If the user is logged in, but not an admin, you should 403 directly rather than
sending them to yet another login url.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:119:
commands[command](self).get()
This dispatch system breaks the ability for url handlers in the WSGIApplication
to accept regular expressions and pass the capture groups on to the handlers.
Since you're only calling with (self), no additional url components can be
captured and passed on.

Why not instead put in app.yaml:

handlers:
- url: /admin/.*
  secure: always
  login: admin
  script: admin.app

And then handle the authentication and dispatch within an admin-only
WSGIApplication in this file?

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/common.py (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/common.py:23: credentials = ndb.JsonProperty()
I still think it makes sense for the credentials json to be its own standalone
datastore object, but this is fine for now.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/main.py (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:27: """Skip authorization for dev
appserver."""
nit: Odd docstring. First say what it *does*, then say the exceptions in which
it doesn't do that.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/scripts/nat_setup.sh (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/scripts/nat_setup.sh:7: REGION="us-central1"
What do we do when we want to have both frontends and backends in multiple
continental zones?

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/scripts/nat_setup.sh:43: if false; then
Still not clear on the functionality here, with "if false".

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/scripts/nat_startup.sh (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/scripts/nat_startup.sh:5: # Setup a health
check endpoint.
Best health check ever.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/templates/main.html (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/templates/main.html:16: <p>Send your protobufs
to the /monacq endpoint (POST only) using
nit: using the

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/templates/main.html:18:
target="_blank">infra/libs/ts_mon</a> library, or
nit: or the

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/templates/main.html:23: <a
We don't generally enforce 80-character limits in HTML templates. Especially
when it results in unreadable stuff like an "<a" on a line by itself.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/templates/set_credentials.html (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/templates/set_credentials.html:16: <p><a
href="/admin/">&lt; Admin</a>
Would be nice to have real breadcrumbs at some point, but this works for now.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/test/__init__.py (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/test/__init__.py:1: # Copyright 2015 The
Chromium Authors. All rights reserved.
Haven't thoroughly reviewed the tests yet. How's the coverage? 100%?

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/vm_module.py (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:49: def requester_is_task(self):
This doesn't return a boolean, as the name and usage suggest.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:61: if not all(f in data for f in
['credentials', 'url']):
We do checking here to make sure the admin data contains credentials, but we
never do checking on the credentials to make sure they contain the required
fields (we'll just IndexError in _get_credentials if the json is wrong).

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:69: success = 200 <=
response.status and response.status <= 299
Dammit, I really miss requests.codes.ok.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:83: logging.debug('Done sending
the payload.')
Maybe note that execution of the callback is done synchronously before this line
is executed.

[Sergey Berezin (google), 2015-04-24 19:09:53]

PTAL. The app is now running the latest version by default:
https://chrome-infra-mon-proxy.appspot.com/

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/admin_handler.py (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:49: def setParams(params,
data):
On 2015/04/23 19:29:37, agable wrote:
> This should be a staticmethod inside SetCredentials. Could be better named or
> accompanied by a docstring as well.

Moved both this and _parsers inside SetCredentials, and added a doc string. I
couldn't come up with a better name (it used to be "serialize", but others asked
to change that too :-)

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:58: 'scopes': lambda v: [l for
l in v.splitlines() if l],
On 2015/04/23 19:29:37, agable wrote:
> Might want to strip whitespace too

Done - good catch, thanks.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:116: if not
users.is_current_user_admin():
On 2015/04/23 19:29:37, agable wrote:
> If the user is logged in, but not an admin, you should 403 directly rather
than
> sending them to yet another login url.

Done.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:119:
commands[command](self).get()
On 2015/04/23 19:29:37, agable wrote:
> This dispatch system breaks the ability for url handlers in the
WSGIApplication
> to accept regular expressions and pass the capture groups on to the handlers.
> Since you're only calling with (self), no additional url components can be
> captured and passed on.

Honestly, I don't need this functionality now, and I don't expect to need it in
the future. So I'm fine with losing it.

But what I gain is a single place where authentication is implemented, and
command handlers don't need to bother with it.

> 
> Why not instead put in app.yaml:
> 
> handlers:
> - url: /admin/.*
>   secure: always
>   login: admin
>   script: admin.app
> 
> And then handle the authentication and dispatch within an admin-only
> WSGIApplication in this file?

Done (just because it eliminates a couple of imports :-). Though I still don't
trust "login: admin" in app.yaml, as it is untestable, and is easy to mess up. I
also plan to switch to auth groups for /admin/.*, and will have to strip login:
admin. Added a TODO to that effect.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/common.py (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/common.py:23: credentials = ndb.JsonProperty()
On 2015/04/23 19:29:37, agable wrote:
> I still think it makes sense for the credentials json to be its own standalone
> datastore object, but this is fine for now.

Acknowledged.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/main.py (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:27: """Skip authorization for dev
appserver."""
On 2015/04/23 19:29:37, agable wrote:
> nit: Odd docstring. First say what it *does*, then say the exceptions in which
> it doesn't do that.

Done.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/scripts/nat_setup.sh (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/scripts/nat_setup.sh:7: REGION="us-central1"
On 2015/04/23 19:29:37, agable wrote:
> What do we do when we want to have both frontends and backends in multiple
> continental zones?

Zones or regions? NAT boxes are already spread out to 3 different zones, to
cover for possible single-zone outages. I don't think zones make much difference
for proximity.

For regions (proximity), we only care about front-end, because our backend is an
Apiary endpoint, which I'm pretty sure has hosting in the US. The front-end,
however, is a regular sandboxed module, which hopefully handles regions
automatically (I don't think we have any control over it).

More relevant for us, managed VMs are always hosted in us-central1 (currently,
us-central1-f zone only), and we send lots of traffic between VMs and NAT boxes.
It's cheaper and faster within the same region.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/scripts/nat_setup.sh:43: if false; then
On 2015/04/23 19:29:37, agable wrote:
> Still not clear on the functionality here, with "if false".

Made the script take parameters, so it makes more sense.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/scripts/nat_startup.sh (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/scripts/nat_startup.sh:5: # Setup a health
check endpoint.
On 2015/04/23 19:29:37, agable wrote:
> Best health check ever.

Acknowledged.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/templates/main.html (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/templates/main.html:16: <p>Send your protobufs
to the /monacq endpoint (POST only) using
On 2015/04/23 19:29:37, agable wrote:
> nit: using the

Done.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/templates/main.html:18:
target="_blank">infra/libs/ts_mon</a> library, or
On 2015/04/23 19:29:37, agable wrote:
> nit: or the

Done.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/templates/main.html:23: <a
On 2015/04/23 19:29:37, agable wrote:
> We don't generally enforce 80-character limits in HTML templates. Especially
> when it results in unreadable stuff like an "<a" on a line by itself.

Done (damn html-mode in emacs :-)

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/templates/set_credentials.html (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/templates/set_credentials.html:16: <p><a
href="/admin/">&lt; Admin</a>
On 2015/04/23 19:29:37, agable wrote:
> Would be nice to have real breadcrumbs at some point, but this works for now.

Acknowledged.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/test/__init__.py (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/test/__init__.py:1: # Copyright 2015 The
Chromium Authors. All rights reserved.
On 2015/04/23 19:29:37, agable wrote:
> Haven't thoroughly reviewed the tests yet. How's the coverage? 100%?

Yes. I ran a builder, it passed test.py step. Will run another one on a new
patch.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/vm_module.py (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:49: def requester_is_task(self):
On 2015/04/23 19:29:37, agable wrote:
> This doesn't return a boolean, as the name and usage suggest.

Added bool(...) cast. It is indeed supposed to be a predicate, and worked in
python as is.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:61: if not all(f in data for f in
['credentials', 'url']):
On 2015/04/23 19:29:37, agable wrote:
> We do checking here to make sure the admin data contains credentials, but we
> never do checking on the credentials to make sure they contain the required
> fields (we'll just IndexError in _get_credentials if the json is wrong).

Added a try: except around _get_credentials, and print an error log with useful
info if this happens.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:69: success = 200 <=
response.status and response.status <= 299
On 2015/04/23 19:29:37, agable wrote:
> Dammit, I really miss requests.codes.ok.

Acknowledged.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:83: logging.debug('Done sending
the payload.')
On 2015/04/23 19:29:37, agable wrote:
> Maybe note that execution of the callback is done synchronously before this
line
> is executed.

Done.

[agable, 2015-04-27 20:51:07]

LGTM!

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/scripts/nat_setup.sh (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/scripts/nat_setup.sh:7: REGION="us-central1"
On 2015/04/24 at 19:09:52, sergeyberezin wrote:
> On 2015/04/23 19:29:37, agable wrote:
> > What do we do when we want to have both frontends and backends in multiple
> > continental zones?
> 
> Zones or regions? NAT boxes are already spread out to 3 different zones, to
cover for possible single-zone outages. I don't think zones make much difference
for proximity.
> 
> For regions (proximity), we only care about front-end, because our backend is
an Apiary endpoint, which I'm pretty sure has hosting in the US. The front-end,
however, is a regular sandboxed module, which hopefully handles regions
automatically (I don't think we have any control over it).
> 
> More relevant for us, managed VMs are always hosted in us-central1 (currently,
us-central1-f zone only), and we send lots of traffic between VMs and NAT boxes.
It's cheaper and faster within the same region.

I'm not clear on the distinction between zones and regions, sorry. I just mean
"what happens when we want to really be running this worldwide and want to
deploy some of this in asia-east-1 or something? How do we configure that?" It's
not a problem right now, I'm just trying to think about the future of infra
deployments in multiple continents.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/vm_module.py (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:49: def requester_is_task(self):
On 2015/04/24 at 19:09:52, sergeyberezin wrote:
> On 2015/04/23 19:29:37, agable wrote:
> > This doesn't return a boolean, as the name and usage suggest.
> 
> Added bool(...) cast. It is indeed supposed to be a predicate, and worked in
python as is.

Yeah I know it worked, I just also know that because it didn't actually return a
boolean someday someone would abuse it :P

https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/main.py (right):

https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:65: def forward_data(data):
Not in this CL, but immediately after: I'd like the ability (temporarily) to
split traffic between various forwarding locations. In particular so that I can
spin up the entire linux fleet sending data to the app to make sure it works,
but then still be able to slowly ramp up traffic to the real mon-api. So we
could send 90% to /dev/null and 10% to the real endpoint, and then 50/50, etc.

[Sergey Berezin, 2015-04-27 21:27:08]

sergeyberezin@chromium.org changed reviewers:
- sergeyberezin@google.com

[Sergey Berezin, 2015-04-27 21:45:51]

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/scripts/nat_setup.sh (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/scripts/nat_setup.sh:7: REGION="us-central1"
On 2015/04/27 20:51:07, agable wrote:
> On 2015/04/24 at 19:09:52, sergeyberezin wrote:
> > On 2015/04/23 19:29:37, agable wrote:
> > > What do we do when we want to have both frontends and backends in multiple
> > > continental zones?
> > 
> > Zones or regions? NAT boxes are already spread out to 3 different zones, to
> cover for possible single-zone outages. I don't think zones make much
difference
> for proximity.
> > 
> > For regions (proximity), we only care about front-end, because our backend
is
> an Apiary endpoint, which I'm pretty sure has hosting in the US. The
front-end,
> however, is a regular sandboxed module, which hopefully handles regions
> automatically (I don't think we have any control over it).
> > 
> > More relevant for us, managed VMs are always hosted in us-central1
(currently,
> us-central1-f zone only), and we send lots of traffic between VMs and NAT
boxes.
> It's cheaper and faster within the same region.
> 
> I'm not clear on the distinction between zones and regions, sorry. I just mean
> "what happens when we want to really be running this worldwide and want to
> deploy some of this in asia-east-1 or something? How do we configure that?"
It's
> not a problem right now, I'm just trying to think about the future of infra
> deployments in multiple continents.

The best answer is
https://pantheon.corp.google.com/project/chrome-infra-mon-proxy/compute/zones...
There are basically 3 regions (us-central1, asia-east1, europe-west1) - the
fourth us-central2 is going away - and you pick the closest. The HTTP load
balancer with a world-wide IP can handle that automatically, if we set up
machines in all of these regions.
Zones within each region are for maintenance windows; I'm assuming they try not
to bring down all zones at once in each region, so the service can migrate
around as needed.
But, of course, this is not yet relevant for this proxy. And we can't control
regions / zones for managed VMs, only for pure GCE instances.

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/scripts/nat_startup.sh (right):

https://codereview.chromium.org/928043005/diff/560001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/scripts/nat_startup.sh:5: # Setup a health
check endpoint.
On 2015/04/23 19:29:37, agable wrote:
> Best health check ever.

Acknowledged.

https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/main.py (right):

https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/main.py:65: def forward_data(data):
On 2015/04/27 20:51:07, agable wrote:
> Not in this CL, but immediately after: I'd like the ability (temporarily) to
> split traffic between various forwarding locations. In particular so that I
can
> spin up the entire linux fleet sending data to the app to make sure it works,
> but then still be able to slowly ramp up traffic to the real mon-api. So we
> could send 90% to /dev/null and 10% to the real endpoint, and then 50/50, etc.

Good idea, I'll add it to the list. I was thinking about a similar feature to
spin down / ramp up traffic to individual VM modules, so we can gracefully
rollout updates.

[Sergey Berezin, 2015-04-27 21:56:00]

The CQ bit was checked by sergeyberezin@chromium.org

[commit-bot: I haz the power, 2015-04-27 21:56:12]

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

[commit-bot: I haz the power, 2015-04-27 21:56:36]

Committed patchset #30 (id:580001) as
https://chromium.googlesource.com/infra/infra/+/73d6305244bd455777957602e1aa1...

[Sergey Berezin, 2015-04-27 21:57:37]

On 2015/04/27 21:56:36, I haz the power (commit-bot) wrote:
> Committed patchset #30 (id:580001) as
>
https://chromium.googlesource.com/infra/infra/+/73d6305244bd455777957602e1aa1...

Woohoo!! Thanks!

[Vadim Sh., 2015-04-28 18:10:42]

vadimsh@chromium.org changed reviewers:
+ vadimsh@chromium.org

[Vadim Sh., 2015-04-28 18:10:43]

Stumbled on this CL in Snippets.

https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/admin_handler.py (right):

https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:128: def post(self, command):
All POSTs here need XSRF protection, otherwise following scenario is possible
0) You are logged in the site (e.g. visited it earlier today).
1) Someone sends you a link "Look, pictures of cute kittens". Or more
realistically, post to chrome-troopers "OMG, everything is broken" with
URLshortened link.
2) You open the link, observe the kittens.
3) Meanwhile invisible POST form on the page posts to /admin/set-credentials
abusing cookies your browser has. Boom, everything is broken.

If you use auth.AuthenticationHandler, adding XSRF protection is rather trivia
(just put xsrf_token hidden field in the form, e.g
https://github.com/luci/luci-py/blob/master/appengine/swarming/handlers_front...)

https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/vm_module.py (right):

https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:70: http_auth =
credentials.authorize(http_auth)
this thing does RSA signing and calls google servers (via urlfetch) to grab
access_token _in each request_, no wonder it is slow

SignedJwtAssertionCredentials object caches access_token internally. It is
mutable stateful object. You need to cache it across requests, so that its
access token is reused. Then you'll pay the penalty of token refresh only once
an hour (when access token expires). Be careful with multithreading though, it
is not thread safe (but request handlers are).

(For high QPS it should be made even more carefully, to avoid multiple requests
all start refreshing the token at once when it expiration comes).

Also oauth2client library is not the best kind of library for writing this sort
of logic in my opinion. It is very flexible and supports all kinds of flows and
storage mechanisms, but when you need to implement something that works fast in
multithreaded environment in very specific conditions (e.g. your case), the
flexibility gets in the way.

[Sergey Berezin, 2015-04-28 19:35:27]

https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/admin_handler.py (right):

https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/admin_handler.py:128: def post(self, command):
Awesome! Thanks, Vadim, you just answered my not-yet-asked question on how to
finish my TODO above. I'll do that in the very next CL.

https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/vm_module.py (right):

https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:70: http_auth =
credentials.authorize(http_auth)
Thanks, this is great info. I'll definitely think about this, and will fix it in
future CLs. It is not mission-critical at the moment though, so I'd say it can
wait for a bit.

Can you suggest alternative libraries? Otherwise, I'll look for a way to store
the token in the Datastore, and reconstruct SignedJwtAssertionCredentials with a
cached token. Refreshing a token at 500 qps without a race storm will indeed be
an interesting challenge. I'll think about it.

[pgervais, 2015-04-28 19:48:50]

On 2015/04/28 19:35:27, Sergey Berezin wrote:
>
https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
> File appengine/chrome_infra_mon_proxy/admin_handler.py (right):
> 
>
https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
> appengine/chrome_infra_mon_proxy/admin_handler.py:128: def post(self,
command):
> Awesome! Thanks, Vadim, you just answered my not-yet-asked question on how to
> finish my TODO above. I'll do that in the very next CL.
> 
>
https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
> File appengine/chrome_infra_mon_proxy/vm_module.py (right):
> 
>
https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
> appengine/chrome_infra_mon_proxy/vm_module.py:70: http_auth =
> credentials.authorize(http_auth)
> Thanks, this is great info. I'll definitely think about this, and will fix it
in
> future CLs. It is not mission-critical at the moment though, so I'd say it can
> wait for a bit.
> 
> Can you suggest alternative libraries? Otherwise, I'll look for a way to store
> the token in the Datastore, and reconstruct SignedJwtAssertionCredentials with
a
> cached token. Refreshing a token at 500 qps without a race storm will indeed
be
> an interesting challenge. I'll think about it.

If you've stored the token in the datastore, refresh it in the background one
minute before it expires.

[Vadim Sh., 2015-04-28 20:28:52]

https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
File appengine/chrome_infra_mon_proxy/vm_module.py (right):

https://codereview.chromium.org/928043005/diff/580001/appengine/chrome_infra_...
appengine/chrome_infra_mon_proxy/vm_module.py:70: http_auth =
credentials.authorize(http_auth)
On 2015/04/28 19:35:27, Sergey Berezin wrote:
> Thanks, this is great info. I'll definitely think about this, and will fix it
in
> future CLs. It is not mission-critical at the moment though, so I'd say it can
> wait for a bit.
> 
> Can you suggest alternative libraries? Otherwise, I'll look for a way to store
> the token in the Datastore, and reconstruct SignedJwtAssertionCredentials with
a
> cached token. Refreshing a token at 500 qps without a race storm will indeed
be
> an interesting challenge. I'll think about it.

Swarming components have simple "get_access_token(...)" function that works with
service accounts and that uses memcache to store access token. It can be
slightly adjusted to use local memory as yet another caching layer (for existing
use cases memcache was more than enough, for long lived managed VM local cache
is justifiable).

We also have wrapper around URLfetch that knows how to use get_access_token and
that implements retries.

Roughly, the usage would be:

from components import auth
from components import net

service_account_key = auth.ServiceAccountKey(
  client_email=...,
  private_key=...,
  private_key_id=...)

response = net.request(
  url=url, 
  method='POST', 
  payload=self.request.body,
  headers={'Content-Type': 'application/x-protobuf'},
  scopes=data['scopes'],
  service_account_key=service_account_key)

(succeeds on  200 <= code <= 299)
(raises net.Error on 300 <= code < 500)
(retries >= 500 a bunch of times)

net.py:
https://github.com/luci/luci-py/blob/master/appengine/components/components/n...
service_account.py:
https://github.com/luci/luci-py/blob/master/appengine/components/components/a...