Milo: Internal buildbot masters support

milo/appengine/buildbot/build.go

Index: milo/appengine/buildbot/build.go
diff --git a/milo/appengine/buildbot/build.go b/milo/appengine/buildbot/build.go
index ebbb68ef7f15bf6e5cf5ddad55dcd874508ab742..8d824fd9be25b3865147c153ba6623cec6cb3452 100644
--- a/milo/appengine/buildbot/build.go
+++ b/milo/appengine/buildbot/build.go
@@ -9,7 +9,6 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/http"
-	"os"
 	"path/filepath"
 	"regexp"
 	"sort"
@@ -18,29 +17,16 @@ import (
 	"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.

@@ -54,44 +40,26 @@ func getBuild(c context.Context, master, builder, buildNum string) (*buildbotBui
 	} 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)
-			}
-		}
-		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
+		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.

+		}
+		if !allowed {
+			return nil, errBuildNotFound
+		}
 	}
 
-	return result, json.Unmarshal(buf, result)
+	return result, nil
 }
 
 // result2Status translates a buildbot result integer into a resp.Status.
@@ -152,12 +120,8 @@ func getBanner(c context.Context, b *buildbotBuild, m *buildbotMaster) *resp.Log
 	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