LogDog: Implement RegisterPrefix RPC.

LogDog: Implement RegisterPrefix RPC.

Implement the register prefix RPC. Remove prefix creation from the
register stream RPC. Prefix creation and access to prefix namepace is
gated on WRITE ACL for the user.

Expiration is currently not retained or enforced, and will be implemented
in a future CL.

LogDog Coordinator is no longer usable by the current Butler. A future
CL will enable the Butler to call RegisterStream appropriately.

BUG=

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

[dnj (Google), 2016-05-13 04:47:36]

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

[dnj (Google), 2016-05-13 04:47:37]

PTAL

[nodir, 2016-05-17 02:46:00]

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

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:20:
"google.golang.org/grpc/codes"
this block should go before the luci one

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:34:
log.WithError(err).Errorf(c, "Invalid prefix.")
not sure about this. this is a user error, not a server error. Usually I do
warning for user errors, but it is up to you to decide

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:63: switch
t := cfg.Transport; t {
switch is overkill here. Also you would have less code nesting with if
saving cfg.Transport to t is unnecessary too

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:69: switch
tps := t.GetPubsub(); tps {
same here

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:91:
log.WithError(err).Warningf(c, "Failed to probe for missing hierarchy
components.")
now this is an error we want to notice as opposed to user errors

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:99: if err
:= hierarchy.PutMulti(di, comps); err != nil {
in hierarchy.go you need to update comment
"This entity is created at stream registration" for componentEntity

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:109:
secretSize, err := cryptorand.Read(c, []byte(secret))
secretSize is unnecessary because according to
https://godoc.org/github.com/luci/luci-go/common/cryptorand#Read secretSize !=
types.PrefixSecretLength only if err != nil

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:115:
secret = secret[:secretSize] // Should be no-op.
unnecessary

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:116: if
err := secret.Validate(); err != nil {
will always return nil because it checks only length and I cannot imagine any
other check to validate a secret blob

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:151:
log.WithError(err).Errorf(c, "Failed to register prefix (transactional).")
some cases of errors are already logged inside the transaction. Would this cause
double logging?

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:156:
Secret:         []byte(secret),
types.PrefixSecret has a comment 
"This is a Base64-encoded secret value." and here the secret is transferred as
raw bytes. That comment should be probably removed. It is confusing anyway

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/services/registerStream.go (right):

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/services/registerStream.go:117: return
nil, grpcutil.Errf(codes.FailedPrecondition, "prefix is not registered")
include the prefix in the error msg?

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/services/registerStream.go:122: // The
prefix secret much match the request secret. If it does, we know this
s/much/must

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/services/registerStream.go:128: 
check that prefix did not expire

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/services/registerStream.go:133: if err :=
di.GetMulti([]interface{}{ls, lst}); err != nil {
yeah.. GetMulti should have been made variadic. I am sure you and iannucci
already had discussions about this

https://codereview.chromium.org/1967273002/diff/20001/common/logdog/types/str...
File common/logdog/types/streamname.go (right):

https://codereview.chromium.org/1967273002/diff/20001/common/logdog/types/str...
common/logdog/types/streamname.go:168: // the remainder of hte path with the
supplied name.
s/hte/the

https://codereview.chromium.org/1967273002/diff/20001/common/logdog/types/str...
common/logdog/types/streamname.go:173: // If name is a valid StreamName, this
will construct a valid StreamPath. If s
"If name is a valid StreamName, this will construct a valid StreamPath" is
incorrect because s may be invalid

https://codereview.chromium.org/1967273002/diff/20001/common/proto/logdog/svc...
File common/proto/logdog/svcconfig/config.proto (right):

https://codereview.chromium.org/1967273002/diff/20001/common/proto/logdog/svc...
common/proto/logdog/svcconfig/config.proto:48: // Project or stream
configurations may override this by providing >= 0 values
s/stream/prefix/?

[dnj (Google), 2016-05-17 14:44:33]

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

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:20:
"google.golang.org/grpc/codes"
On 2016/05/17 02:45:59, nodir wrote:
> this block should go before the luci one

Done.

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:34:
log.WithError(err).Errorf(c, "Invalid prefix.")
On 2016/05/17 02:45:58, nodir wrote:
> not sure about this. this is a user error, not a server error. Usually I do
> warning for user errors, but it is up to you to decide

Done.

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:63: switch
t := cfg.Transport; t {
On 2016/05/17 02:45:58, nodir wrote:
> switch is overkill here. Also you would have less code nesting with if
> saving cfg.Transport to t is unnecessary too

Not sure that I agree here. Switch is a perfectly reasonable conditional, and I
prefer scoping the lifecycle of variables to their use. In this case and also
dislike reusing variables. I prefer switch to "if ... else" in this case because
I think it looks better, and I think I have the artistic license to make that
choice.

But I'll go ahead and rewrite this as if...else. Go lint prevents "if { ...
return;} else { ... }", saying that the else should just be removed, so these
variables will end up being scoped to the local function. I'm not terribly happy
about that, but it's fine.

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:91:
log.WithError(err).Warningf(c, "Failed to probe for missing hierarchy
components.")
On 2016/05/17 02:45:58, nodir wrote:
> now this is an error we want to notice as opposed to user errors

This is actually not a big deal, since hierarchy components are idempotent.
Failure to identify missing components just means we have to re-put them, which
is slightly wasteful but not critical.

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:99: if err
:= hierarchy.PutMulti(di, comps); err != nil {
On 2016/05/17 02:45:59, nodir wrote:
> in hierarchy.go you need to update comment
> "This entity is created at stream registration" for componentEntity

Done.

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:109:
secretSize, err := cryptorand.Read(c, []byte(secret))
On 2016/05/17 02:45:58, nodir wrote:
> secretSize is unnecessary because according to
> https://godoc.org/github.com/luci/luci-go/common/cryptorand#Read secretSize !=
> types.PrefixSecretLength only if err != nil

Done.

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:115:
secret = secret[:secretSize] // Should be no-op.
On 2016/05/17 02:45:58, nodir wrote:
> unnecessary

Done.

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:116: if
err := secret.Validate(); err != nil {
On 2016/05/17 02:45:59, nodir wrote:
> will always return nil because it checks only length and I cannot imagine any
> other check to validate a secret blob

Correct, but I am going to do it anyway, because if I ever change the validation
criteria, I want this check in place.

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:151:
log.WithError(err).Errorf(c, "Failed to register prefix (transactional).")
On 2016/05/17 02:45:58, nodir wrote:
> some cases of errors are already logged inside the transaction. Would this
cause
> double logging?

Yes, but that's fine.

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:156:
Secret:         []byte(secret),
On 2016/05/17 02:45:58, nodir wrote:
> types.PrefixSecret has a comment 
> "This is a Base64-encoded secret value." and here the secret is transferred as
> raw bytes. That comment should be probably removed. It is confusing anyway

Done.

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/services/registerStream.go (right):

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/services/registerStream.go:117: return
nil, grpcutil.Errf(codes.FailedPrecondition, "prefix is not registered")
On 2016/05/17 02:45:59, nodir wrote:
> include the prefix in the error msg?

I'm choosing not to. The user knows what they've submitted, and the prefix is
easy programmatically and visually to extract.

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/services/registerStream.go:122: // The
prefix secret much match the request secret. If it does, we know this
On 2016/05/17 02:45:59, nodir wrote:
> s/much/must

Done.

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/services/registerStream.go:128: 
On 2016/05/17 02:45:59, nodir wrote:
> check that prefix did not expire

Done.

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/services/registerStream.go:133: if err :=
di.GetMulti([]interface{}{ls, lst}); err != nil {
On 2016/05/17 02:45:59, nodir wrote:
> yeah.. GetMulti should have been made variadic. I am sure you and iannucci
> already had discussions about this

Yeah we're actually going to make "Get" variadic for 100% compatibility.

https://codereview.chromium.org/1967273002/diff/20001/common/logdog/types/str...
File common/logdog/types/streamname.go (right):

https://codereview.chromium.org/1967273002/diff/20001/common/logdog/types/str...
common/logdog/types/streamname.go:168: // the remainder of hte path with the
supplied name.
On 2016/05/17 02:45:59, nodir wrote:
> s/hte/the

Done.

https://codereview.chromium.org/1967273002/diff/20001/common/logdog/types/str...
common/logdog/types/streamname.go:173: // If name is a valid StreamName, this
will construct a valid StreamPath. If s
On 2016/05/17 02:45:59, nodir wrote:
> "If name is a valid StreamName, this will construct a valid StreamPath" is
> incorrect because s may be invalid

Done.

https://codereview.chromium.org/1967273002/diff/20001/common/proto/logdog/svc...
File common/proto/logdog/svcconfig/config.proto (right):

https://codereview.chromium.org/1967273002/diff/20001/common/proto/logdog/svc...
common/proto/logdog/svcconfig/config.proto:48: // Project or stream
configurations may override this by providing >= 0 values
On 2016/05/17 02:45:59, nodir wrote:
> s/stream/prefix/?

Done.

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

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/services/registerStream.go (right):

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/services/registerStream.go:128: 
On 2016/05/17 14:44:33, dnj (Google) wrote:
> On 2016/05/17 02:45:59, nodir wrote:
> > check that prefix did not expire
> 
> Done.

I don't see where is it done. You've added two prefix_expiration config values
and they are not used

https://codereview.chromium.org/1967273002/diff/40001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/registration/registerPrefix.go
(right):

https://codereview.chromium.org/1967273002/diff/40001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:10:
"google.golang.org/grpc/codes"
A tool that organizes blocks of imports will probably be a mort effective
solution to make imports in the repo consistent. Don't bother in this CL

https://codereview.chromium.org/1967273002/diff/40001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:62:
cfgTransport := cfg.Transport
Why do you introduce a new variable for that?

https://codereview.chromium.org/1967273002/diff/40001/common/logdog/types/str...
File common/logdog/types/streamsecret.go (right):

https://codereview.chromium.org/1967273002/diff/40001/common/logdog/types/str...
common/logdog/types/streamsecret.go:24: // The Prefix secreet is generated by
the Coordinator at prefix registration,
s/screet/secret

https://codereview.chromium.org/1967273002/diff/40001/common/logdog/types/str...
common/logdog/types/streamsecret.go:26: // the stream. The secret is asserted by
microservices and the Coordiantor
typo in Coordinator

[dnj (Google), 2016-05-17 16:32:54]

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/services/registerStream.go (right):

https://codereview.chromium.org/1967273002/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/services/registerStream.go:128: 
On 2016/05/17 16:19:47, nodir wrote:
> On 2016/05/17 14:44:33, dnj (Google) wrote:
> > On 2016/05/17 02:45:59, nodir wrote:
> > > check that prefix did not expire
> > 
> > Done.
> 
> I don't see where is it done. You've added two prefix_expiration config values
> and they are not used

Oh sorry meant to link the other CL: https://codereview.chromium.org/1972753002

This is implemented in a separate CL to keep review size down.

https://codereview.chromium.org/1967273002/diff/40001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/registration/registerPrefix.go
(right):

https://codereview.chromium.org/1967273002/diff/40001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:62:
cfgTransport := cfg.Transport
On 2016/05/17 16:19:47, nodir wrote:
> Why do you introduce a new variable for that?

I don't want to dereference the same thing three times.

https://codereview.chromium.org/1967273002/diff/40001/common/logdog/types/str...
File common/logdog/types/streamsecret.go (right):

https://codereview.chromium.org/1967273002/diff/40001/common/logdog/types/str...
common/logdog/types/streamsecret.go:24: // The Prefix secreet is generated by
the Coordinator at prefix registration,
On 2016/05/17 16:19:47, nodir wrote:
> s/screet/secret

Done.

https://codereview.chromium.org/1967273002/diff/40001/common/logdog/types/str...
common/logdog/types/streamsecret.go:26: // the stream. The secret is asserted by
microservices and the Coordiantor
On 2016/05/17 16:19:47, nodir wrote:
> typo in Coordinator

Done.

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

Description was changed from

==========
LogDog: Implement RegisterPrefix RPC.

Implement the register prefix RPC. Remove prefix creation from the
register stream RPC.

LogDog Coordinator is no longer usable by the current Butler. A future
CL will enable the Butler to call RegisterStream appropriately.

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

to

==========
LogDog: Implement RegisterPrefix RPC.

Implement the register prefix RPC. Remove prefix creation from the
register stream RPC. Prefix creation and access to prefix namepace is
gated on WRITE ACL for the user.

LogDog Coordinator is no longer usable by the current Butler. A future
CL will enable the Butler to call RegisterStream appropriately.

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

[dnj, 2016-05-17 20:40:49]

Description was changed from

==========
LogDog: Implement RegisterPrefix RPC.

Implement the register prefix RPC. Remove prefix creation from the
register stream RPC. Prefix creation and access to prefix namepace is
gated on WRITE ACL for the user.

LogDog Coordinator is no longer usable by the current Butler. A future
CL will enable the Butler to call RegisterStream appropriately.

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

to

==========
LogDog: Implement RegisterPrefix RPC.

Implement the register prefix RPC. Remove prefix creation from the
register stream RPC. Prefix creation and access to prefix namepace is
gated on WRITE ACL for the user.

Expiration is currently not retained or enforced, and will be implemented
in a future CL.

LogDog Coordinator is no longer usable by the current Butler. A future
CL will enable the Butler to call RegisterStream appropriately.

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

[nodir, 2016-05-19 00:48:32]

lgtm

https://codereview.chromium.org/1967273002/diff/40001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/registration/registerPrefix.go
(right):

https://codereview.chromium.org/1967273002/diff/40001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/registration/registerPrefix.go:62:
cfgTransport := cfg.Transport
On 2016/05/17 16:32:54, dnj (Google) wrote:
> On 2016/05/17 16:19:47, nodir wrote:
> > Why do you introduce a new variable for that?
> 
> I don't want to dereference the same thing three times.

without variable performance is about the same and there is less code...

[dnj (Google), 2016-05-26 16:40:11]

The CQ bit was checked by dnj@google.com

[dnj (Google), 2016-05-26 16:40:11]

The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org
Link to the patchset: https://codereview.chromium.org/1967273002/#ps120001
(title: "Updated patchset dependency")

[commit-bot: I haz the power, 2016-05-26 16:40:14]

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

[commit-bot: I haz the power, 2016-05-26 16:49:30]

Description was changed from

==========
LogDog: Implement RegisterPrefix RPC.

Implement the register prefix RPC. Remove prefix creation from the
register stream RPC. Prefix creation and access to prefix namepace is
gated on WRITE ACL for the user.

Expiration is currently not retained or enforced, and will be implemented
in a future CL.

LogDog Coordinator is no longer usable by the current Butler. A future
CL will enable the Butler to call RegisterStream appropriately.

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

to

==========
LogDog: Implement RegisterPrefix RPC.

Implement the register prefix RPC. Remove prefix creation from the
register stream RPC. Prefix creation and access to prefix namepace is
gated on WRITE ACL for the user.

Expiration is currently not retained or enforced, and will be implemented
in a future CL.

LogDog Coordinator is no longer usable by the current Butler. A future
CL will enable the Butler to call RegisterStream appropriately.

BUG=

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

[commit-bot: I haz the power, 2016-05-26 16:49:31]

Committed patchset #7 (id:120001) as
https://github.com/luci/luci-go/commit/f53ba305ee6fd50d62bfdf2abc02ea9c943b506a