LogDog: Add prefix registration endpoint.

LogDog: Add prefix registration endpoint.

Add an prefix registration API and endpoint to LogDog coordinator. This
is exposed in a separate registration module. Implementation of this
endpoint is left to a future CL.

Add Coordinator awareness of project WRITE permission. This will be checked
in the RegisterPrefix endpoint and prefix creation/access is gated on
WRITE permission.

BUG=

Committed: https://github.com/luci/luci-go/commit/b1900dca518ac21c33e269fa7f289264826f4fbc

[dnj (Google), 2016-05-11 21:52:40]

dnj@google.com changed reviewers:
+ dnj@google.com, iannucci@chromium.org, vadimsh@chromium.org

[dnj (Google), 2016-05-11 21:52:40]

PTAL

[dnj, 2016-05-17 20:38:09]

Description was changed from

==========
LogDog: Add prefix registration endpoint.

Add an (unimplemented) prefix registration API and endpoint to LogDog
coordinator.

BUG=
==========

to

==========
LogDog: Add prefix registration endpoint.

Add an prefix registration API and endpoint to LogDog coordinator. This
is exposed in a separate registration module. Implementation of this
endpoint is left to a future CL.

Add Coordinator awareness of project WRITE permission. This will be checked
in the RegisterPrefix endpoint and prefix creation/access is gated on
WRITE permission.

BUG=
==========

[nodir, 2016-05-19 16:43:16]

nodir@chromium.org changed reviewers:
+ nodir@chromium.org

[nodir, 2016-05-19 16:43:17]

https://codereview.chromium.org/1970823005/diff/1/common/api/logdog_coordinat...
File common/api/logdog_coordinator/registration/v1/service.proto (right):

https://codereview.chromium.org/1970823005/diff/1/common/api/logdog_coordinat...
common/api/logdog_coordinator/registration/v1/service.proto:18: // The protobuf
version string for this stream.
please describe a scenario, in the comment, why do you need this field.

https://codereview.chromium.org/1970823005/diff/1/common/api/logdog_coordinat...
common/api/logdog_coordinator/registration/v1/service.proto:18: // The protobuf
version string for this stream.
is this a version of "desc" field? Please clarify comment

https://codereview.chromium.org/1970823005/diff/1/common/api/logdog_coordinat...
common/api/logdog_coordinator/registration/v1/service.proto:28: // Once closed,
new stream registration requests will no longer be accepted.
clarify if stream chunks can be sent after prefix expiration as long as stream
was registered before prefix expiration; or assert otherwise

https://codereview.chromium.org/1970823005/diff/1/common/api/logdog_coordinat...
common/api/logdog_coordinator/registration/v1/service.proto:41: // The name of
the Pub/Sub topic to publish butlerproto-formatted Butler log
"butlerproto" - typo? intentional? I am not sure

https://codereview.chromium.org/1970823005/diff/1/common/api/logdog_coordinat...
common/api/logdog_coordinator/registration/v1/service.proto:50: //
RegisterStream allows a Butler instance to register a log stream with the
s/RegisterStream/RegisterPrefix

https://codereview.chromium.org/1970823005/diff/1/common/proto/logdog/logpb/b...
File common/proto/logdog/logpb/butler.proto (right):

https://codereview.chromium.org/1970823005/diff/1/common/proto/logdog/logpb/b...
common/proto/logdog/logpb/butler.proto:57: string deprecated_source = 1;
I think the standard practice is to use "reserved" keyword for deprecated fields

https://codereview.chromium.org/1970823005/diff/20001/appengine/cmd/logdog_co...
File appengine/cmd/logdog_coordinator/register/doc.go (right):

https://codereview.chromium.org/1970823005/diff/20001/appengine/cmd/logdog_co...
appengine/cmd/logdog_coordinator/register/doc.go:5: // Package module is the
main entry point for the `register` LogDog AppEngine
why is it called module?

https://codereview.chromium.org/1970823005/diff/20001/appengine/cmd/logdog_co...
File appengine/cmd/logdog_coordinator/register/module-register.yaml (right):

https://codereview.chromium.org/1970823005/diff/20001/appengine/cmd/logdog_co...
appengine/cmd/logdog_coordinator/register/module-register.yaml:5: module:
register
I am not sure why you have to have a whole another appengine module for one
function. Why not same module? Why not same service? looks unnecessarily
complicated to me

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
File appengine/logdog/coordinator/context.go (right):

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/context.go:21: // NamespaceAccessNoAuth grants
unconditional access to a project's namespace.
please clarify that this simply skips the check

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/context.go:60: // MembershipError will be returned.
this comment needs an update

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/context.go:94: nc, err :=
info.Get(*c).Namespace(pns)
please add a check that namespace is not set already to a different value
because code calls WithProjectNamespace multiple times within the same request

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/logs/list.go (right):

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/logs/list.go:95: // This should work,
since the list would have rejected the namespace if
s/list/decorated service/

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/registration/service.go (right):

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/service.go:18: // server is
a Cloud Endpoint service supporting log stream registration.
It is not Cloud Endpoints

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/service.go:58:
log.WithError(err).Errorf(c, "User does not have WRITE access to project.")
I don't see why this is necessarily WRITE access. err probably contains that, so
maybe do not mention the kind of access

https://codereview.chromium.org/1970823005/diff/20001/common/api/logdog_coord...
File common/api/logdog_coordinator/registration/v1/gen.go (right):

https://codereview.chromium.org/1970823005/diff/20001/common/api/logdog_coord...
common/api/logdog_coordinator/registration/v1/gen.go:19: var _ = proto.Marshal
why do we still need this? I don't think I ever put this

https://codereview.chromium.org/1970823005/diff/20001/common/proto/logdog/log...
File common/proto/logdog/logpb/butler.proto (right):

https://codereview.chromium.org/1970823005/diff/20001/common/proto/logdog/log...
common/proto/logdog/logpb/butler.proto:57: string deprecated_source = 1;
use reserved

[dnj (Google), 2016-05-19 22:52:05]

https://codereview.chromium.org/1970823005/diff/20001/appengine/cmd/logdog_co...
File appengine/cmd/logdog_coordinator/register/doc.go (right):

https://codereview.chromium.org/1970823005/diff/20001/appengine/cmd/logdog_co...
appengine/cmd/logdog_coordinator/register/doc.go:5: // Package module is the
main entry point for the `register` LogDog AppEngine
On 2016/05/19 16:43:17, nodir wrote:
> why is it called module?

It's an AppEngine module. All of my GAE modules are called "module", kind of
like how other things are called "main".

https://codereview.chromium.org/1970823005/diff/20001/appengine/cmd/logdog_co...
File appengine/cmd/logdog_coordinator/register/module-register.yaml (right):

https://codereview.chromium.org/1970823005/diff/20001/appengine/cmd/logdog_co...
appengine/cmd/logdog_coordinator/register/module-register.yaml:5: module:
register
On 2016/05/19 16:43:17, nodir wrote:
> I am not sure why you have to have a whole another appengine module for one
> function. Why not same module? Why not same service? looks unnecessarily
> complicated to me

Well, currently I have the following modules:
"backend": Specifically backend services, admin/cron only.
"vmuser": A *managed VM* instance for user access.
"service": A classic GAE instance for only service access.

My thought was that all of these share ACL sets, so I'd create another separate
module space for the new ACL set (write). However, I guess I had a "one module
per service" mentality. I think I will put this into the "service" endpoint and
just have it export two services.

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
File appengine/logdog/coordinator/context.go (right):

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/context.go:21: // NamespaceAccessNoAuth grants
unconditional access to a project's namespace.
On 2016/05/19 16:43:17, nodir wrote:
> please clarify that this simply skips the check

Done.

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/context.go:60: // MembershipError will be returned.
On 2016/05/19 16:43:17, nodir wrote:
> this comment needs an update

Done.

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/context.go:94: nc, err :=
info.Get(*c).Namespace(pns)
On 2016/05/19 16:43:17, nodir wrote:
> please add a check that namespace is not set already to a different value
> because code calls WithProjectNamespace multiple times within the same request

That's allowed. This function only enters namespaces; it doesn't care where it
was coming from.

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/logs/list.go (right):

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/logs/list.go:95: // This should work,
since the list would have rejected the namespace if
On 2016/05/19 16:43:17, nodir wrote:
> s/list/decorated service/

Done.

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/registration/service.go (right):

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/service.go:18: // server is
a Cloud Endpoint service supporting log stream registration.
On 2016/05/19 16:43:17, nodir wrote:
> It is not Cloud Endpoints

Done.

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/service.go:58:
log.WithError(err).Errorf(c, "User does not have WRITE access to project.")
On 2016/05/19 16:43:17, nodir wrote:
> I don't see why this is necessarily WRITE access. err probably contains that,
so
> maybe do not mention the kind of access

I think it could be useful for debugging, so I'm going to leave this here.

https://codereview.chromium.org/1970823005/diff/20001/common/api/logdog_coord...
File common/api/logdog_coordinator/registration/v1/gen.go (right):

https://codereview.chromium.org/1970823005/diff/20001/common/api/logdog_coord...
common/api/logdog_coordinator/registration/v1/gen.go:19: var _ = proto.Marshal
On 2016/05/19 16:43:17, nodir wrote:
> why do we still need this? I don't think I ever put this

This was originally b/c it is possible for this directory to not have any
protobufs, and we wanted "go install" to still install "protoc" so "go generate"
wouldn't fail. Since we're using cproto now, it's definitely not necessary.

https://codereview.chromium.org/1970823005/diff/20001/common/proto/logdog/log...
File common/proto/logdog/logpb/butler.proto (right):

https://codereview.chromium.org/1970823005/diff/20001/common/proto/logdog/log...
common/proto/logdog/logpb/butler.proto:57: string deprecated_source = 1;
On 2016/05/19 16:43:17, nodir wrote:
> use reserved

I think "deprecated" is better here, since it communicates what the state of the
value is. Reserved suggests something else is saving it for usage.

[nodir, 2016-05-25 17:34:05]

lgtm

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
File appengine/logdog/coordinator/context.go (right):

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/context.go:94: nc, err :=
info.Get(*c).Namespace(pns)
On 2016/05/19 22:52:04, dnj (Google) wrote:
> On 2016/05/19 16:43:17, nodir wrote:
> > please add a check that namespace is not set already to a different value
> > because code calls WithProjectNamespace multiple times within the same
request
> 
> That's allowed. This function only enters namespaces; it doesn't care where it
> was coming from.

Acknowledged.

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/registration/service.go (right):

https://codereview.chromium.org/1970823005/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/service.go:58:
log.WithError(err).Errorf(c, "User does not have WRITE access to project.")
On 2016/05/19 22:52:04, dnj (Google) wrote:
> On 2016/05/19 16:43:17, nodir wrote:
> > I don't see why this is necessarily WRITE access. err probably contains
that,
> so
> > maybe do not mention the kind of access
> 
> I think it could be useful for debugging, so I'm going to leave this here.

Acknowledged.

[dnj (Google), 2016-05-26 16:35:06]

The CQ bit was checked by dnj@google.com

[commit-bot: I haz the power, 2016-05-26 16:35:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1970823005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1970823005/80001

[commit-bot: I haz the power, 2016-05-26 16:39:52]

Description was changed from

==========
LogDog: Add prefix registration endpoint.

Add an prefix registration API and endpoint to LogDog coordinator. This
is exposed in a separate registration module. Implementation of this
endpoint is left to a future CL.

Add Coordinator awareness of project WRITE permission. This will be checked
in the RegisterPrefix endpoint and prefix creation/access is gated on
WRITE permission.

BUG=
==========

to

==========
LogDog: Add prefix registration endpoint.

Add an prefix registration API and endpoint to LogDog coordinator. This
is exposed in a separate registration module. Implementation of this
endpoint is left to a future CL.

Add Coordinator awareness of project WRITE permission. This will be checked
in the RegisterPrefix endpoint and prefix creation/access is gated on
WRITE permission.

BUG=

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

[commit-bot: I haz the power, 2016-05-26 16:39:53]

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