New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 809155 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Apr 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Feature

Blocked on:
issue 809645

Blocking:
issue 758632



Sign in to add a comment

Generating oauth tokens in perf src side upload script

Project Member Reported by eyaich@chromium.org, Feb 5 2018

Issue description

Where is the correct place to generate the oauth token for uploading to the dashboard.  

Right now we do it in the recipe and pass it to the upload script.  You can see how we currently do this in chromium_tests for perf: 

https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium_tests/steps.py?l=1709

We are trying to determine what the correct API to our src side upload script it. vadimsh@ indicated that it would best easiest to generate it from the recipe side and pass it into the upload script, but I wanted to create a bug to talk about the options. 

vadimsh@ thoughts:

1.  Who/what will be calling the new script in src/tools/perf? Is it always going to be build bots (through recipes)? If so, the simplest solution would be to keep generating the token in the recipes and plumb --oauth-token-file as argument to the new script.

2. If you want the new script to be completely self-sufficient, this is more complicated... The canonical way of using service account keys from Python is through oauth2client library (this specifically), but it is not directly in chromium/src (only through catapult), and it is not entirely trivial to add (the library is huge and have a bunch of dependencies).

3 . If growing a dependency on depot_tools is acceptable, we can reimplement the same logic the recipes are doing for generating access tokens. They call external program "authutil" that does the work. We can make this program be part of depot_tools, so it is available on developer's machines. 


Therefore, I think we have to talk about the users of this script and whether or not we require the oauth token or generate it for them based on the bot the test is running on.

If we go with option 1 where we require it in the api we are going to have to modify the merge script api again to take a flag to optionally generate the oauth token.

https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/swarming/resources/collect_task.py?l=125

If we want to be a more self sufficient upload script we are going to have to figure out how to hook in this catapult library to do this for us.  Since this upload script isn't running on the swarming bot itself, I don't think a large dependency is completely unreasonable since it isn't impacting the size of our isolate.  

Any thoughts on the correct API from the perf side?  I think this is based on what users we think we will have from outside of the recipe code.  

Ned or Simon can you speak to that?



 
Blocking: 758632
Cc: jbudorick@chromium.org
Vadim can you talk more to what is involved in #2 above if we were to use import this library from catapult to try and generate them on our own?

Adding jbudorick@ to get his thoughts on adding this to the merge script api as an optional flag to generate this on the recipe side and pass it through.  
Having dependency on depot_tools is ok. I actually also planning to switch all our benchmark script to use vpython, which assumes users have depot_tools installed.

Given this, it seems to me option (3) is cleanest, as we can remove special code path in recipe to generate the oauth token?

Cc: vadimsh@chromium.org
Vadim isn't actually on this bug, so, adding him.

I spoke offline with Emily/Ashley about this, just wanted to chime in with a few thoughts.

It turns out that the puppet_service_account recipe module actually has a get_key_path method: https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/puppet_service_account/api.py

which can be used to get the path to the private key.

From there, it would be feasible to do the depot_tools thing, which sounds like it may not be a bad idea. I don't really have a strong opinion about which way we go based on the fact that puppet_service_account has a way to return the private key path. Each of these alternatives trades off between how much work will have to be done and how convenient it is for people not using recipes to upload stuff.
I think option (1) (aka passing the token from outside) is the simplest, assuming the script will ALWAYS be called by something that can generate tokens (e.g. recipes or some more advanced wrapping script).

Both options (2) (aka 'oauth2client') and (3) (aka 'authutil') require the script to be supplied with a path to a service account key (as JSON). 

Main differences between them:

1) 'oauth2client' is a python library with a lot of dependencies. But if catapult already has all of them, then using it is relatively straightforward. Here's an example from catapult itself: https://cs.chromium.org/chromium/src/third_party/catapult/experimental/qbot/qbot/api.py?q=ServiceAccountCredentials.from_json&sq=package:chromium&l=42

2) 'authutil' is a standalone statically linked Go binary. It can produce tokens using "authutil token -service-account-json <path-to-a-key>". Some wrapping code is still needed to call it and cache the token for its validity duration (if the script is long-running).

3) It is relatively simple to use 'authutil' with end-user credentials (e.g. @chromium.org accounts). Two steps: a developer runs "authutil login" first and goes through OAuth consent screen in the browser (need to do it only once). Then the script just calls "authutil token" to get the user's token. Doing similar thing using oauth2client requires more coding. Not sure it matters for your case though.

4) On LUCI infrastructure "authutil token" transparently returns a token associated with a task service account (on LUCI each task runs in a context of some service account). So 'authutil' approach will continue to work on LUCI bots mostly unaltered. Same is true for (1) approach, since recipes use authutil internally too. oauth2client won't work and will require special code path for LUCI environment. There are NO json keys on LUCI (but there's a replacement that authutil already can use).

The bad part about authutil approach is that it adds one more "hidden" dependency on depot_tools :( We can perhaps make it less hidden by adding 'authutil' package to DEPS (since gclient supports depending on binary packages now). But then it is one more place to roll, and more crap to fetch when checking out chromium.

---

I guess in order of convenience for the infrastructure it is (1), (3), (2). In order of convenience for users I suspect it is more like (2), (3), (1) :)
Cc: -jbudorick@chromium.org
Labels: -Type-Bug -Pri-3 Pri-1 Type-Feature
I think we have decided to push this into our script so that we don't have to be dependent on an api from infra.  Removing jbudorick@ since it won't impact the merge script api at all. 

Now we have to weigh oauth2client vs authutil.




Given we are on src side, I think we should try to start with authutil in depot tools instead of tryin to reinvent the wheel. 

We will still need a flag to our script to tell us the name of the service account (which we want to be separate from the task service account on luci).  See this CL here (https://chrome-internal-review.googlesource.com/c/infra/puppet/+/414488) for the chrome-perf-histograms account we created for the infra bots.   If we have the name of that name, we should be able to find the key and then run authutil directly ((2) from comment #5).    

Ethan indicated there might be a way to get the key path if we have the name passed in : It turns out that the puppet_service_account recipe module actually has a get_key_path method: https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/puppet_service_account/api.py

Vadim, is that the correct way to get the path to the key to pass into authutil?  That module says it is deprecated.
> Given we are on src side, I think we should try to start with authutil in depot tools instead of tryin to reinvent the wheel.

authutil is not in depot_tools yet, and I still feel conflicted about adding it there (don't want unexpected scripts somewhere grow an implicit dependency on it). Would you mind if I try adding it to chromium/src instead via DEPS?

> Vadim, is that the correct way to get the path to the key to pass into authutil?  That module says it is deprecated.

Yes, this is correct. It is deprecated in a sense of "we are actively working towards removing this" (by switching away from Buildbot to LUCI). Scripts using 'authutil' will continue to work (we'll just need to stop passing -service-account-json flag, to indicate it should pick up a default ambient account instead).
Vadim, this is maybe a little off-topic for this bug, but can you say more about the ambient account model in LUCI?

Currently, with respect to the perf dashboard /add_histograms upload API, we had imagined that different groups of uploaders would use different service accounts. This would mean that if some client we do not control started abusing our API we could revoke their permission to upload while we figured out what was going on. For example, we control chromium.perf and chromium.perf.fyi, but there are also many other types of tests that we don't control that upload to the perf dashboard.

Do you expect that such a thing would still be possible in the post-LUCI world?
> Would you mind if I try adding it to chromium/src instead via DEPS?

I had a discussion with iannucci@ and tandrii@. We decided to put it in depot_tools after all, but rename it to luci-auth first, because 'authutil' is goo generic name to be in PATH.

> Vadim, this is maybe a little off-topic for this bug, but can you say more about the ambient account model in LUCI?

In LUCI each individual build runs with an authority of some service account, specified in the builder configuration (like so https://chromium.googlesource.com/chromium/src/+/infra/config/cr-buildbucket.cfg#189). It is "ambient" in a sense all tools (git, gsutil, gcloud, all LUCI tools) use it transparently by default unless asked do use something else.

In terms of implementation it looks similar to GCE service accounts. Swarming bot runs a local TCP server in 127.0.0.1 port that serves OAuth tokens on demand. authutil knows how to use this protocol.

> Do you expect that such a thing would still be possible in the post-LUCI world?

Yes. Currently we do the granularity of service accounts be on approximately (project, flavor) level, where 'flavor' is "try" (for tryjobs) or "ci" (for post-commit builders). E.g. here are some accounts we already have:

chromium-ci-builder@chops-service-accounts.iam.gserviceaccount.com
chromium-try-builder@chops-service-accounts.iam.gserviceaccount.com
v8-ci-builder@chops-service-accounts.iam.gserviceaccount.com
v8-try-builder@chops-service-accounts.iam.gserviceaccount.com
cobalt-ci-builder@fuchsia-infra.iam.gserviceaccount.com
infra-ci-builder@fuchsia-infra.iam.gserviceaccount.com
...

It means that e.g V8 CI builders will be using v8-ci-builder@chops-service-accounts.iam.gserviceaccount.com for perf dashabord API calls and you can whitelist this account in the perf dashboard ACLs to allow V8 to upload stuff.

More granular accounts are also possible, it's a matter of configuration. In extreme case each individual builder can have its own account. We don't do this, because it entails a lot of management overhead (we'll need to add each individual account to all various ACLs, like for git, google storage, ...).
Blockedon: 809645
luci-auth is in depot_tools now.

To see how it works (if you have a service account key):
$ update_depot_tools  # only once
$ luci-auth token -service-account-json ~/.vadimshtesting-key.json -json-output -
{"token":"ya29.c.El....","expiry":1517966733}

You can also use your own account for local development:
$ luci-auth login
# follow instructions
$ {"token":"ya29.Gl....","expiry":1517966842}

In this mode "luci-auth login" sets up a refresh token and intended to be called from outside of any automated scripts. And "luci-auth token" uses the established refresh token. This is similar to how it would work on LUCI bots, except "luci-auth login" is kind of 'already done' there before build starts.

"luci-auth token" will never block or wait for user input. If something is not right, it will just fail with an error (for example, "Not logged in. Run 'luci-auth login'.").
 
Scripts are highly discouraged from calling "luci-auth login" automatically, since it often leads to blocking interactive prompts in the middle of a build run (highly confusing). Instead scripts are encouraged to propagate the failure to the end user, so they can react (either by using "luci-auth login" directly, or maybe calling "<script-name> login" wrapper if luci-auth should be hidden for UX reasons).

I hope this helps...
Thanks everyone!  We are going to use luci-auth in depot tools.

Ethan we are testing this on the FYI waterfall . Is the chromium-perf-histograms service account deployed to all bots on the chromium.perf and chromium.perf.fyi bots?  What is the process for getting service accounts on the bots?  
Summary: Generating oauth tokens in perf src side upload script (was: Correct API for src side upload script)
Vadim where do you recommend running luci-auth token until we are migrated to LUCI?  You said it required user interaction and it has to be re-run for every build so we are trying to figure out how to incorporate this.
If you pass it -service-account-json pointing to a key predeployed on bots, it doesn't require user interaction. I suggest you to make your scripts accept --service-account-json argument and pass it to 'luci-auth' as is. 

Currently recipes have this snippet:

try:
  oauth_token = api.puppet_service_account.get_access_token(
      'chromium-perf-histograms')
# Only bots uploading histograms have/need these creds.
except api.step.StepFailure:  # pragma: no cover
  oauth_token = None

It will be replaced by this one:

perf_upload_creds = api.puppet_service_account.get_key_path('chromium-perf-histograms')
...
self.m.step('upload perf', [...., '--service-account-json', perf_upload_creds])

On LUCI we will just stop passing --service-account-json and 'luci-auth' will pick up the default task account automatically.

---

The advantage here is that perf upload script can be used outside recipes. Anyone can create a new service account in their project and get a key for it.
https://chrome-internal-review.googlesource.com/c/infra/puppet/+/414488

Is where this service account was added to the masters.  

It is not currently on master.tryserver.chromium.perf where our trybot lives.
Project Member

Comment 18 by bugdroid1@chromium.org, Feb 20 2018

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/infra/puppet/+/79995473b80dda076b1e6bfde531bfac98faf55c

commit 79995473b80dda076b1e6bfde531bfac98faf55c
Author: Vadim Shtayura <vadimsh@chromium.org>
Date: Tue Feb 20 19:38:37 2018

Project Member

Comment 19 by bugdroid1@chromium.org, Feb 20 2018

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/infra/puppet/+/850a350592d240fb0dbeef0bfab29918b4f1656d

commit 850a350592d240fb0dbeef0bfab29918b4f1656d
Author: Vadim Shtayura <vadimsh@chromium.org>
Date: Tue Feb 20 22:33:43 2018

Status: Fixed (was: Untriaged)
I'm going to go ahead and close this bug since I added the oauth generation code to src/tools/perf here: https://cs.chromium.org/chromium/src/tools/perf/core/oauth_api.py

Sign in to add a comment