git cl try uses buildbucket via OAuth2

git_cl.py

Index: git_cl.py
diff --git a/git_cl.py b/git_cl.py
index 4eee3f152853d4a746bc901c465da0355397c3e5..a3f4d006629e7028036bed7809eb076e794b08c6 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -11,6 +11,7 @@ from distutils.version import LooseVersion
 from multiprocessing.pool import ThreadPool
 import base64
 import glob
+import httplib
 import json
 import logging
 import optparse
@@ -21,6 +22,8 @@ import stat
 import sys
 import tempfile
 import textwrap
+import threading
+import time
 import urllib2
 import urlparse
 import webbrowser
@@ -33,13 +36,16 @@ except ImportError:
 
 
 from third_party import colorama
+from third_party import httplib2
 from third_party import upload
+from third_party.google_api_python_client import apiclient

[Vadim Sh., 2015/04/10 02:24:29]

it is not vendored properly:

Traceback (most recent call last):
  File "/Users/vadimsh/depot_tools/git_cl.py", line 36, in <module>
    from third_party import upload
  File "/Users/vadimsh/depot_tools/third_party/upload.py", line 75, in <module>
    import auth
  File "/Users/vadimsh/depot_tools/auth.py", line 26, in <module>
    from third_party.google_api_python_client import apiclient
  File
"/Users/vadimsh/depot_tools/third_party/google_api_python_client/apiclient/__init__.py",
line 3, in <module>
    import googleapiclient
ImportError: No module named googleapiclient

I suspect you have it installed in your global site-packages and it is imported
from there.

I personally think apiclient.discovery is not worth the trouble of adding
apiclient dependency (e.g. it is simpler to use httplib2 directly to send HTTP
requests to JSON endpoints). But as you prefer.

[nodir, 2015/04/13 16:04:53]

"import apiclient" should work, we use it in other places. If it doesn't
PYTHONPATH needs to be updated

[sheyang, 2015/04/14 01:30:22]

Per discussion with Nodir@ I added the directory to syspath to fix it.

 import breakpad  # pylint: disable=W0611
 import clang_format
 import dart_format
 import fix_encoding
 import gclient_utils
 import git_common
+import oauth2

[Vadim Sh., 2015/04/10 02:24:29]

this file has been replaced by auth.py in
https://codereview.chromium.org/1074673002/ (will commit it tomorrow).

auth.py is used by "git cl upload" and all other depot_tools script. See comment
below for how to use it for accessing buildbucket.

[sheyang, 2015/04/14 01:30:22]

Done.

 import owners
 import owners_finder
 import presubmit_support
@@ -61,6 +67,13 @@ REFS_THAT_ALIAS_TO_OTHER_REFS = {
     'refs/remotes/origin/lkcr': 'refs/remotes/origin/master',
 }
 
+# Buildbucket-related constants
+DISCOVERY_URL = (
+    'https://cr-buildbucket.appspot.com/_ah/api/discovery/v1/apis/'
+    '{api}/{apiVersion}/rest')
+DEFAULT_SCOPES = 'email'

[Vadim Sh., 2015/04/10 02:24:29]

not used

[sheyang, 2015/04/14 01:30:22]

Removed.

+BUILDSET_STR = 'patch/rietveld/{hostname}/{issue}/{patch}'
+
 # Valid extensions for files we want to lint.
 DEFAULT_LINT_REGEX = r"(.*\.cpp|.*\.cc|.*\.h)"
 DEFAULT_LINT_IGNORE_REGEX = r"$^"
@@ -215,6 +228,95 @@ def is_dirty_git_tree(cmd):
   return False
 
 
+def _prefix_master(master):
+  prefix = 'master.'
+  if master.startswith(prefix):
+    return master
+  else:
+    return '%s%s' % (prefix, master)
+
+
+def _get_buildbucket(credentials):

[Vadim Sh., 2015/04/10 02:24:29]

with auth.py it would be:

def _get_buildbucket(rietveld_url, auth_config):
  authenticator = auth.get_authenticator_for_host(rietveld_url, auth_config)
  http = httplib2.Http()
  authenticator.authorize(http)
  return ...

auth_config is already extracted from command line options CMDtry. rietveld_url
is changelist.GetRietveldServer().

[sheyang, 2015/04/14 01:30:22]

Done.

+  http = httplib2.Http()
+  http = credentials.authorize(http)
+  return apiclient.discovery.build(
+      'buildbucket', 'v1',
+      http=http,
+      discoveryServiceUrl=DISCOVERY_URL,
+  )
+
+
+def trigger_distributed_try_jobs(
+    credentials, changelist, options, masters, category):
+  buildbucket = _get_buildbucket(credentials)
+  creds_props = json.loads(credentials.to_json())
+  requester = creds_props['id_token']['email']

[Vadim Sh., 2015/04/10 02:24:29]

this is currently not exposed via auth.py, though I can add it. But I think it
is a wrong approach security-wise, see comment below.

[sheyang, 2015/04/14 01:30:22]

This field will be populated from oauth2 credential. Replaced it with user_agent
and the value is 'git+cl_try'.

+  issue_props = changelist.GetIssueProperties()
+  rietveld_host = urlparse.urlparse(changelist.GetRietveldServer()).hostname
+  issue = changelist.GetIssue()
+  patchset = changelist.GetMostRecentPatchset()
+  buildset = BUILDSET_STR.format(
+      hostname=rietveld_host,
+      issue=str(issue),
+      patch=str(patchset))
+  print 'Tried jobs on:'
+  for (master, builders_and_tests) in masters.iteritems():
+    print 'Master: %s' % master
+    bucket = _prefix_master(master)
+    for builder, tests in builders_and_tests.iteritems():
+      req = buildbucket.put(body={
+          'bucket': bucket,
+          'parameters_json': json.dumps({
+              'builder_name': builder,
+              'changes':[
+                  {'author': {'email': issue_props['owner_email']}},
+              ],
+              'properties': {
+                  'category': category,
+                  'clobber': options.clobber,
+                  'issue': issue,
+                  'master': master,
+                  'patch_project': issue_props['project'],
+                  'patch_storage': 'rietveld',
+                  'patchset': patchset,
+                  'reason': options.name,
+                  'requester': requester,

[Vadim Sh., 2015/04/10 02:24:29]

the server should derive it from oauth credentials itself, or at least verify if
corresponds to email associated with oauth token. Otherwise it is trivial to
trick server and request jobs on somebody else's behalf. +nodir to answer this
question.

[nodir, 2015/04/10 02:31:29]

buidbucket does not read this property. It is interpreted by a recipe, that does
not verify identity. Buildbot trusts buildbucket that its buckets are secure.
BuildBucket trusts that authorized users won't put garbage to properties.

requester is not always == created_by because CQ schedules builds on behalf of
its users, so "requester" "enables" impersonation (in insecure way I guess)

[nodir, 2015/04/13 16:04:53]

This part was wrong because CQ sets requester to "commit-bot@chromium.org", so
requester should be read from buildbucket.build.created_by property. In order to
not break the existing contract between buildbot and recipes, I think
buildbot-buildbucket integration should copy created_by to requester property.

[sheyang, 2015/04/14 01:30:22]

Acknowledged.

+                  'revision': options.revision,
+                  'rietveld': changelist.GetRietveldServer(),
+                  'testfilter': tests,
+              },
+          }),
+          'tags': ['buildset:%s' % buildset,
+                   'master:%s' % master,
+                   'builder:%s' % builder,
+                   'requester:%s' % requester]

[nodir, 2015/04/13 16:04:53]

requester tag is not the user, but a tool. Maybe we should rename it to
"requester_tool" 
https://cr-buildbucket.appspot.com/#docs/conventions
CQ specifies "cq" here. git-cl-try should probably specify "git-cl-try".
Rietveld will specify "rietveld"

[sheyang, 2015/04/14 01:30:22]

Use 'user_agent' instead.

+      })
+      wait = 1
+      try_count = 3
+      while try_count > 0:
+        try:
+          try_count -= 1
+          response = req.execute()
+          if response.get('error'):
+            msg = 'Error in response. Reason: %s. Message: %s.' % (
+                response['error'].get('reason', ''),
+                response['error'].get('message', ''))
+            raise BuildbucketResponseException(msg)
+          break
+        except apiclient.errors.HttpError as ex:
+          status = ex.resp.status if ex.resp else None
+          if status >= 500:
+            logging.debug('Transient errors when triggering tryjobs. '
+                          'Will retry in %d seconds.', wait)
+            time.sleep(wait)
+            wait *= 2
+            if try_count <= 0:
+              raise
+          else:
+            raise
+      print '  %s: %s' % (builder, tests)
+
+
 def MatchSvnGlob(url, base_url, glob_spec, allow_wildcards):
   """Return the corresponding git ref if |base_url| together with |glob_spec|
   matches the full |url|.
@@ -282,6 +384,10 @@ def print_stats(similarity, find_copies, args):
       stdout=stdout, env=env)
 
 
+class BuildbucketResponseException(Exception):
+  pass
+
+
 class Settings(object):
   def __init__(self):
     self.default_server = None
@@ -2736,6 +2842,7 @@ def CMDtry(parser, args):
   group.add_option(
       "-n", "--name", help="Try job name; default to current branch name")
   parser.add_option_group(group)
+  oauth2.add_oauth2_options(parser)
   options, args = parser.parse_args(args)
 
   if args:
@@ -2831,22 +2938,26 @@ def CMDtry(parser, args):
         'upload fail?\ngit-cl try always uses latest patchset from rietveld. '
         'Continuing using\npatchset %s.\n' % patchset)
   try:
-    cl.RpcServer().trigger_distributed_try_jobs(
-        cl.GetIssue(), patchset, options.name, options.clobber,
-        options.revision, masters)
-  except urllib2.HTTPError, e:
-    if e.code == 404:
-      print('404 from rietveld; '
-            'did you mean to use "git try" instead of "git cl try"?')
+    creds = oauth2.get_oauth2_creds(
+        options,
+        urlparse.urlparse(cl.GetRietveldServer()).hostname)
+    if not creds:
+      print 'Failed to fetch credentials. Aborting...'
       return 1
-  print('Tried jobs on:')
-
-  for (master, builders) in masters.iteritems():
-    if master:
-      print 'Master: %s' % master
-    length = max(len(builder) for builder in builders)
-    for builder in sorted(builders):
-      print '  %*s: %s' % (length, builder, ','.join(builders[builder]))
+    trigger_distributed_try_jobs(creds, cl, options, masters, 'git cl try')
+  except apiclient.errors.HttpError as ex:
+    status = ex.resp.status if ex.resp else None
+    if status == httplib.FORBIDDEN:
+      print 'ERROR: Access denied. Please verify you have tryjob access.'
+    else:
+      print 'ERROR: Tryjob request failed: %s.' % ex
+    return 1
+  except BuildbucketResponseException as ex:
+    print ex
+    return 1
+  except Exception as e:
+    print 'Unexpected error when trying to trigger tryjobs: %s.' % e
+    return 1
   return 0