Milo: Internal buildbot masters support

milo/appengine/buildbot/build.go

 // Copyright 2016 The LUCI Authors. All rights reserved.
 // Use of this source code is governed under the Apache License, Version 2.0
 // that can be found in the LICENSE file.
 
 package buildbot
 
 import (
         "encoding/json"
         "fmt"
         "io/ioutil"
         "net/http"
-        "os"
         "path/filepath"
         "regexp"
         "sort"
         "strconv"
         "strings"
         "time"
 
         "github.com/luci/gae/service/datastore"
-        "github.com/luci/gae/service/memcache"
         log "github.com/luci/luci-go/common/logging"
         "github.com/luci/luci-go/milo/api/resp"
-        "github.com/luci/luci-go/server/auth"
+        "github.com/luci/luci-go/milo/appengine/settings"
+        "github.com/luci/luci-go/milo/common/miloerror"
         "golang.org/x/net/context"
 )
 
-func getURL(c context.Context, URL string) ([]byte, error) {
-        fmt.Fprintf(os.Stderr, "Fetching %s\n", URL)
-        tr, err := auth.GetRPCTransport(c, auth.NoAuth)
-        if err != nil {
-                return nil, err
-        }
-        client := http.Client{Transport: tr}
-        resp, err := client.Get(URL)
-        if err != nil {
-                return nil, err
-        }
-        if resp.StatusCode != 200 {
-                return nil, fmt.Errorf("Failed to fetch %s, status code %d", URL, resp.StatusCode)
-        }
-        defer resp.Body.Close()
-        return ioutil.ReadAll(resp.Body)
+var errBuildNotFound = miloerror.Error{
+        Message: "Build not found",
+        Code:    http.StatusNotFound,
 }
 
 // getBuild fetches a build from BuildBot.

[Vadim Sh., 2016/08/23 18:53:06]

... and checks ACL.

[Ryan Tseng, 2016/08/23 22:00:15]

Done.

 func getBuild(c context.Context, master, builder, buildNum string) (*buildbotBuild, error) {
         result := &buildbotBuild{
                 Master:      master,
                 Buildername: builder,
         }
         if num, err := strconv.Atoi(buildNum); err != nil {
                 panic(fmt.Errorf("%s does not look like a number", buildNum))
         } else {
                 result.Number = num
         }
-        // Check memcache first.
-        mc := memcache.Get(c)
-        if item, err := mc.Get(result.getID()); err == nil {
-                log.Debugf(c, "Found in Memcache!")
-                json.Unmarshal(item.Value(), result)
-                return result, nil
-        }
-        // Then check local datastore.
         ds := datastore.Get(c)
         err := ds.Get(result)
-        if err == nil {
-                log.Debugf(c, "Found build in local cache!")
-                if result.Times[1] != nil {
-                        bs, ierr := json.Marshal(result)
-                        if ierr == nil {
-                                item := mc.NewItem(result.getID()).SetValue(bs)
-                                mc.Set(item)
-                        }
+        if err != nil {
+                return nil, errBuildNotFound

[Vadim Sh., 2016/08/23 18:53:06]

It is important to distinguish transient errors from fatal ones. Transient
errors should produce HTTP 500, so client knows to retry.

In this case only datastore.ErrNoSuchEntity is fatal error (and should result in
HTTP 404), the rest should result in HTTP 500.

[Ryan Tseng, 2016/08/23 22:00:14]

Done.

+        }
+        // If the build is internal, check to see if the user has access.  We use
+        // the magic project name "buildbot-internal" for membership check.
+        if result.Internal {
+                allowed, err := settings.IsAllowed(c, "buildbot-internal")

[Vadim Sh., 2016/08/23 18:53:06]

since we are hardcoding magic name anyway, perhaps better to do it as:

settings.CanAccessInternal(c)

func CanAccessInternal(c context.Context) (bool, error) {
  return IsAllowed(c, "buildbot-internal")
}

[Ryan Tseng, 2016/08/23 22:00:14]

Done.

+                if err != nil {
+                        log.WithError(err).Errorf(c,
+                                "Encountered error while checking buildbot membership, returning 404")
+                        return nil, errBuildNotFound

[Vadim Sh., 2016/08/23 18:53:06]

same here, it should be HTTP 500, not HTTP 404.

[Ryan Tseng, 2016/08/23 22:00:15]

Done.

                 }
-                return result, nil
-        }
-        log.Debugf(c, "Could not find log in local cache: %s", err.Error())
-        // Now check CBE.
-        cbeURL := fmt.Sprintf(
-                "https://chrome-build-extract.appspot.com/p/%s/builders/%s/builds/%s?json=1",
-                master, builder, buildNum)
-        if buf, err := getURL(c, cbeURL); err == nil {
-                return result, json.Unmarshal(buf, result)
-        }
-        // TODO(hinoka): Cache finished builds.
-        url := fmt.Sprintf(
-                "https://build.chromium.org/p/%s/json/builders/%s/builds/%s", master, builder, buildNum)
-        buf, err := getURL(c, url)
-        if err != nil {
-                return nil, err
+                if !allowed {
+                        return nil, errBuildNotFound
+                }
         }
 
-        return result, json.Unmarshal(buf, result)
+        return result, nil
 }
 
 // result2Status translates a buildbot result integer into a resp.Status.
 func result2Status(s *int) (status resp.Status) {
         if s == nil {
                 return resp.Running
         }
         switch *s {
         case 0:
                 status = resp.Success
@@ skipping 40 matching lines @@
 // getBanner fetches the banner information about the bot that the build
 // ran on.  This is best effort and:
 // * Will return an empty list of banners if the master is not found.
 // * May return incorrect data if the platform of the slave of the same name
 //   was changed recently, and this build was older than before the slave
 //   changed platforms.
 func getBanner(c context.Context, b *buildbotBuild, m *buildbotMaster) *resp.LogoBanner {
         logos := &resp.LogoBanner{}
         // Fetch the master info from datastore if not provided.
         if m == nil {
-                m1, i, _, err := getMasterJSON(c, b.Master)
+                m1, _, _, err := getMasterJSON(c, b.Master)
                 m = m1
-                if i {
-                        // TODO(hinoka): Support internal masters.
-                        return nil
-                }
                 if err != nil {
                         log.Warningf(c, "Failed to fetch master information for banners on master %s", b.Master)
                         return nil
                 }
         }
 
         s, ok := m.Slaves[b.Slave]
         if !ok {
                 log.Warningf(c, "Could not find slave %s in master %s", b.Slave, b.Master)
                 return nil
@@ skipping 339 matching lines @@
 
         // TODO(hinoka): Do all fields concurrently.
         return &resp.MiloBuild{
                 SourceStamp:   sourcestamp(c, b),
                 Summary:       summary(c, b),
                 Components:    components(b),
                 PropertyGroup: properties(b),
                 Blame:         blame(b),
         }, nil
 }