Milo: Internal buildbot masters support

Milo: Internal buildbot masters support

This CL finds all the places where the buildbot build or master entries are being fetched, and performs an ACL if the "Internal" flag on the build is set to see if the current user is part of the magic "buildbot-private" project.
404's are the errors of choice for internal buildbot builds.

BUG=637375
Committed: https://github.com/luci/luci-go/commit/63a9cfa4f98f04ab34e84f38c5304945575e5975

[Ryan Tseng, 2016-08-22 23:09:14]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 23:09:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 23:16:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 23:16:30]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2016-08-23 18:06:20]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 18:06:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Tseng, 2016-08-23 18:07:38]

Description was changed from

==========
Milo: Internal buildbot masters support

BUG=637375
==========

to

==========
Milo: Internal buildbot masters support

This CL finds all the places where the buildbot build or master entries are
being fetched, and performs an ACL if the "Internal" flag on the build is set to
see if the current user is part of the magic "buildbot-private" project.

BUG=637375
==========

[Ryan Tseng, 2016-08-23 18:07:38]

hinoka@google.com changed reviewers:
+ vadimsh@chromium.org

[Ryan Tseng, 2016-08-23 18:08:30]

Description was changed from

==========
Milo: Internal buildbot masters support

This CL finds all the places where the buildbot build or master entries are
being fetched, and performs an ACL if the "Internal" flag on the build is set to
see if the current user is part of the magic "buildbot-private" project.

BUG=637375
==========

to

==========
Milo: Internal buildbot masters support

This CL finds all the places where the buildbot build or master entries are
being fetched, and performs an ACL if the "Internal" flag on the build is set to
see if the current user is part of the magic "buildbot-private" project.
404's are the errors of choice for internal buildbot builds.

BUG=637375
==========

[Ryan Tseng, 2016-08-23 18:09:09]

ptal, this utilizes settings.IsAllowed() from the last CL

[commit-bot: I haz the power, 2016-08-23 18:09:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 18:09:53]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30d1669cc468be10)
  Luci-go Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30d1669d277a9210)

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

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
File milo/appengine/buildbot/build.go (right):

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:32: // getBuild fetches a build from BuildBot.
... and checks ACL.

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:46: return nil, errBuildNotFound
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.

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:51: allowed, err := settings.IsAllowed(c,
"buildbot-internal")
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")
}

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:55: return nil, errBuildNotFound
same here, it should be HTTP 500, not HTTP 404.

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
File milo/appengine/buildbot/builder.go (right):

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/builder.go:134: return nil, errMasterNotFound
same here

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
File milo/appengine/buildbot/master.go (right):

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/master.go:76: if !settings.IsAllowed(c,
"buildbot-private") {
errors like this is why its better to deduplicate magic strings :) In other
places it is "buildbot-internal".

Also IsAllowed returns tuple (bool, error). This would not compile :-/

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
File milo/appengine/buildbot/pubsub.go (right):

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/pubsub.go:31: publicSubName   =
"projects/luci-milo/subscriptions/buildbot-public"
hardcoded project names :(

[Ryan Tseng, 2016-08-23 21:55:35]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 21:55:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Also just did s/log/logging/g in the files, as a small refactoring.

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
File milo/appengine/buildbot/build.go (right):

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:32: // getBuild fetches a build from BuildBot.
On 2016/08/23 18:53:06, Vadim Sh. wrote:
> ... and checks ACL.

Done.

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:46: return nil, errBuildNotFound
On 2016/08/23 18:53:06, Vadim Sh. wrote:
> 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.

Done.

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:51: allowed, err := settings.IsAllowed(c,
"buildbot-internal")
On 2016/08/23 18:53:06, Vadim Sh. wrote:
> 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")
> }

Done.

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:55: return nil, errBuildNotFound
On 2016/08/23 18:53:06, Vadim Sh. wrote:
> same here, it should be HTTP 500, not HTTP 404.

Done.

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
File milo/appengine/buildbot/builder.go (right):

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/builder.go:134: return nil, errMasterNotFound
On 2016/08/23 18:53:06, Vadim Sh. wrote:
> same here

Done.

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
File milo/appengine/buildbot/master.go (right):

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/master.go:76: if !settings.IsAllowed(c,
"buildbot-private") {
On 2016/08/23 18:53:06, Vadim Sh. wrote:
> errors like this is why its better to deduplicate magic strings :) In other
> places it is "buildbot-internal".
> 
> Also IsAllowed returns tuple (bool, error). This would not compile :-/

doh

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
File milo/appengine/buildbot/pubsub.go (right):

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/pubsub.go:31: publicSubName   =
"projects/luci-milo/subscriptions/buildbot-public"
On 2016/08/23 18:53:06, Vadim Sh. wrote:
> hardcoded project names :(

Probably will move this to a settings page... wdyt? This is a global setting, so
I think settings might make more sense than luci-cfg.

on the other hand, luci-cfg is being used for acl'ing internal access, but
that's only because there's a pre-existing ACL mechanism via luci-cfg.

[Ryan Tseng, 2016-08-23 22:01:50]

Small changes

[Ryan Tseng, 2016-08-23 22:01:51]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 22:01:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 22:11:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 22:11:51]

Dry run: This issue passed the CQ dry run.

[Vadim Sh., 2016-08-23 22:35:46]

lgtm with nit

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
File milo/appengine/buildbot/pubsub.go (right):

https://codereview.chromium.org/2271453002/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/pubsub.go:31: publicSubName   =
"projects/luci-milo/subscriptions/buildbot-public"
On 2016/08/23 22:00:15, Ryan Tseng wrote:
> On 2016/08/23 18:53:06, Vadim Sh. wrote:
> > hardcoded project names :(
> 
> Probably will move this to a settings page... wdyt? This is a global setting,
so
> I think settings might make more sense than luci-cfg.
> 
Don't know. I'd expect stuff like that to be in luci-config. It's different from
analytics, since it's not shared reusable code, but a thing specific to Milo,
and Milo has luci-config integration already.

> on the other hand, luci-cfg is being used for acl'ing internal access, but
> that's only because there's a pre-existing ACL mechanism via luci-cfg.

https://codereview.chromium.org/2271453002/diff/60001/milo/appengine/buildbot...
File milo/appengine/buildbot/build.go (right):

https://codereview.chromium.org/2271453002/diff/60001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:48: switch err {
nit: I usually do

switch {
  case err == datastore.ErrNo....:
    ...
  case err != nil:
    return nil, err
}
// success 

it's simpler

[Ryan Tseng, 2016-08-24 21:06:28]

nit fix

[Ryan Tseng, 2016-08-24 21:06:29]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 21:06:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Tseng, 2016-08-24 21:07:20]

The CQ bit was unchecked by hinoka@google.com

[Ryan Tseng, 2016-08-24 21:07:24]

https://codereview.chromium.org/2271453002/diff/60001/milo/appengine/buildbot...
File milo/appengine/buildbot/build.go (right):

https://codereview.chromium.org/2271453002/diff/60001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:48: switch err {
On 2016/08/23 22:35:46, Vadim Sh. wrote:
> nit: I usually do
> 
> switch {
>   case err == datastore.ErrNo....:
>     ...
>   case err != nil:
>     return nil, err
> }
> // success 
> 
> it's simpler

Done.

[Ryan Tseng, 2016-08-24 21:07:29]

The CQ bit was checked by hinoka@google.com

[Ryan Tseng, 2016-08-24 21:07:29]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/2271453002/#ps80001
(title: "nit fix")

[commit-bot: I haz the power, 2016-08-24 21:07:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 21:13:33]

Description was changed from

==========
Milo: Internal buildbot masters support

This CL finds all the places where the buildbot build or master entries are
being fetched, and performs an ACL if the "Internal" flag on the build is set to
see if the current user is part of the magic "buildbot-private" project.
404's are the errors of choice for internal buildbot builds.

BUG=637375
==========

to

==========
Milo: Internal buildbot masters support

This CL finds all the places where the buildbot build or master entries are
being fetched, and performs an ACL if the "Internal" flag on the build is set to
see if the current user is part of the magic "buildbot-private" project.
404's are the errors of choice for internal buildbot builds.

BUG=637375

Committed:
https://github.com/luci/luci-go/commit/63a9cfa4f98f04ab34e84f38c5304945575e5975
==========

[commit-bot: I haz the power, 2016-08-24 21:13:34]

Committed patchset #5 (id:80001) as
https://github.com/luci/luci-go/commit/63a9cfa4f98f04ab34e84f38c5304945575e5975