LogDog: Add signed GS URL fetching.

LogDog: Add signed GS URL fetching.

Allow a "Get" RPC request to additionally request a signed URL to the
archive log entry stream, if available.

To really support this, we restructure (for the better) the internal
service API and the Coordinator test framework to enable fetching bound
storage instances instead of the motley assortment of composite services
that was previously implemented.

BUG=chromium:669994
TEST=unit

Committed: https://github.com/luci/luci-go/commit/2568e271f92ed046ea2c10318e90c2f53147eda1

[dnj, 2016-11-30 19:06:18]

dnj@chromium.org changed reviewers:
+ estaab@chromium.org, vadimsh@chromium.org

[dnj, 2016-11-30 19:06:19]

PTAL

[Vadim Sh., 2016-11-30 21:03:53]

lgtm with nits

https://codereview.chromium.org/2538203002/diff/1/logdog/api/endpoints/coordi...
File logdog/api/endpoints/coordinator/logs/v1/logs.proto (right):

https://codereview.chromium.org/2538203002/diff/1/logdog/api/endpoints/coordi...
logdog/api/endpoints/coordinator/logs/v1/logs.proto:73: // The signed URL will
have an expiration that is <= the requested expiration.
you mean >=?

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
File logdog/appengine/coordinator/endpoints/logs/get.go (right):

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
logdog/appengine/coordinator/endpoints/logs/get.go:159: if signURLLifetime > 0 {
is a request with 'tail == true' and 'signURLLifetime > 0' valid?

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
File logdog/appengine/coordinator/service.go (right):

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
logdog/appengine/coordinator/service.go:43: // maxSignedURLLifetime is the
maximum allowed signed URL lifetime.
mention this limit in *.proto doc too

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
logdog/appengine/coordinator/service.go:273: gs = nil // Don't close in defer.
nit: well.. there's only one exit point from this function (line 270). It would
be simpler just to put gs.Close there instead of doing a defer and then "disarm"
it.

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
logdog/appengine/coordinator/service.go:417: if url, err =
gcst.SignedURL(gs.stream.Bucket(), gs.stream.Filename(), &opts); err != nil {
consider caching the result in the future. SignBytes is RPC to google backends,
it is relatively expensive (compared to memcache) and has a separate quota. 

e.g. if Logdog viewer switches to signed URLs exclusively, there'll be a LOT of
SignBytes requests.

[Vadim Sh., 2016-11-30 21:09:09]

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
File logdog/appengine/coordinator/service.go (right):

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
logdog/appengine/coordinator/service.go:390: acct, err := info.ServiceAccount(c)
oh, btw this is RPC to the backend too:
https://github.com/golang/appengine/blob/master/identity.go#L121

Consider using
https://github.com/luci/luci-go/blob/master/appengine/gaeauth/server/gaesigne....
It caches account name already. 

s := gaesigner.Signer{}
info, _ := s.ServiceInfo(ctx)
info.ServiceAccountName

[dnj, 2016-12-01 17:39:31]

Thanks!

https://codereview.chromium.org/2538203002/diff/1/logdog/api/endpoints/coordi...
File logdog/api/endpoints/coordinator/logs/v1/logs.proto (right):

https://codereview.chromium.org/2538203002/diff/1/logdog/api/endpoints/coordi...
logdog/api/endpoints/coordinator/logs/v1/logs.proto:73: // The signed URL will
have an expiration that is <= the requested expiration.
On 2016/11/30 21:03:52, Vadim Sh. wrote:
> you mean >=?

I mean <=, as in if you request 24 days and the server caps at 1 day, you'll get
1 day.'

The expiration here is a duration, not a time, so it probably will always be <
in practice b/c of delay.

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
File logdog/appengine/coordinator/endpoints/logs/get.go (right):

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
logdog/appengine/coordinator/endpoints/logs/get.go:159: if signURLLifetime > 0 {
On 2016/11/30 21:03:52, Vadim Sh. wrote:
> is a request with 'tail == true' and 'signURLLifetime > 0' valid?

Nope, b/c that parameter is not part of the TailRequest RPC :)

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
File logdog/appengine/coordinator/service.go (right):

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
logdog/appengine/coordinator/service.go:43: // maxSignedURLLifetime is the
maximum allowed signed URL lifetime.
On 2016/11/30 21:03:52, Vadim Sh. wrote:
> mention this limit in *.proto doc too

I don't want to put constraints in the proto doc, as I consider this an
implementation detail. The RPC returns the expiration, and the proto mentions
that the return expiration will be <= the requested, so I think everything is
covered.

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
logdog/appengine/coordinator/service.go:273: gs = nil // Don't close in defer.
On 2016/11/30 21:03:52, Vadim Sh. wrote:
> nit: well.. there's only one exit point from this function (line 270). It
would
> be simpler just to put gs.Close there instead of doing a defer and then
"disarm"
> it.

I suppose I gravitate towards defer to protect against panics, too.

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
logdog/appengine/coordinator/service.go:390: acct, err := info.ServiceAccount(c)
On 2016/11/30 21:09:09, Vadim Sh. wrote:
> oh, btw this is RPC to the backend too:
> https://github.com/golang/appengine/blob/master/identity.go#L121
> 
> Consider using
>
https://github.com/luci/luci-go/blob/master/appengine/gaeauth/server/gaesigne....
> It caches account name already. 
> 
> s := gaesigner.Signer{}
> info, _ := s.ServiceInfo(ctx)
> info.ServiceAccountName

Done.

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
logdog/appengine/coordinator/service.go:417: if url, err =
gcst.SignedURL(gs.stream.Bucket(), gs.stream.Filename(), &opts); err != nil {
On 2016/11/30 21:03:52, Vadim Sh. wrote:
> consider caching the result in the future. SignBytes is RPC to google
backends,
> it is relatively expensive (compared to memcache) and has a separate quota. 
> 
> e.g. if Logdog viewer switches to signed URLs exclusively, there'll be a LOT
of
> SignBytes requests.

This gets tricky, since the actual signed URLs have individual expiration times
associated with them. For now, I think I'll leave this as-is. My intention is
not to have the log viewer web UI use this endpoint (since it needs to stream,
too); rather, I'll implement its use in the CLI. I'll note that this can be
expensive, though.

[dnj, 2016-12-01 17:56:58]

The CQ bit was checked by dnj@chromium.org

[dnj, 2016-12-01 17:56:58]

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/2538203002/#ps20001
(title: "Allow index signing, use gaesigner.")

[commit-bot: I haz the power, 2016-12-01 17:57:06]

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-12-01 18:03:50]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480615018380350,
"parent_rev": "982d8b5f9e60122df55521188b9f884c615fd5e9", "commit_rev":
"2568e271f92ed046ea2c10318e90c2f53147eda1"}

[commit-bot: I haz the power, 2016-12-01 18:03:52]

Description was changed from

==========
LogDog: Add signed GS URL fetching.

Allow a "Get" RPC request to additionally request a signed URL to the
archive log entry stream, if available.

To really support this, we restructure (for the better) the internal
service API and the Coordinator test framework to enable fetching bound
storage instances instead of the motley assortment of composite services
that was previously implemented.

BUG=chromium:669994
TEST=unit
==========

to

==========
LogDog: Add signed GS URL fetching.

Allow a "Get" RPC request to additionally request a signed URL to the
archive log entry stream, if available.

To really support this, we restructure (for the better) the internal
service API and the Coordinator test framework to enable fetching bound
storage instances instead of the motley assortment of composite services
that was previously implemented.

BUG=chromium:669994
TEST=unit

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

[commit-bot: I haz the power, 2016-12-01 18:03:53]

Committed patchset #2 (id:20001) as
https://github.com/luci/luci-go/commit/2568e271f92ed046ea2c10318e90c2f53147eda1

[Vadim Sh., 2016-12-01 19:32:13]

https://codereview.chromium.org/2538203002/diff/1/logdog/api/endpoints/coordi...
File logdog/api/endpoints/coordinator/logs/v1/logs.proto (right):

https://codereview.chromium.org/2538203002/diff/1/logdog/api/endpoints/coordi...
logdog/api/endpoints/coordinator/logs/v1/logs.proto:73: // The signed URL will
have an expiration that is <= the requested expiration.
On 2016/12/01 17:39:30, dnj wrote:
> On 2016/11/30 21:03:52, Vadim Sh. wrote:
> > you mean >=?
> 
> I mean <=, as in if you request 24 days and the server caps at 1 day, you'll
get
> 1 day.'
> 
> The expiration here is a duration, not a time, so it probably will always be <
> in practice b/c of delay.

Oh, I thought it would return an URL that lives at least
"sign_entry_url_lifetime" (and possibly more).

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
File logdog/appengine/coordinator/service.go (right):

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
logdog/appengine/coordinator/service.go:43: // maxSignedURLLifetime is the
maximum allowed signed URL lifetime.
On 2016/12/01 17:39:31, dnj wrote:
> On 2016/11/30 21:03:52, Vadim Sh. wrote:
> > mention this limit in *.proto doc too
> 
> I don't want to put constraints in the proto doc, as I consider this an
> implementation detail. The RPC returns the expiration, and the proto mentions
> that the return expiration will be <= the requested, so I think everything is
> covered.

1. The doc doesn't mention it anymore.
2. "<=" is very vague. A cap at 1 min is also <=, does it mean all callers
should expect very short-lived URLs? I think it is reasonable to mention that
lifetime is limited by 1 hour. Or as alternative set an expectation of what
callers should pass, e.g. "sign_entry_url_lifetime should usually be several
minutes or tens of minutes".

https://codereview.chromium.org/2538203002/diff/1/logdog/appengine/coordinato...
logdog/appengine/coordinator/service.go:417: if url, err =
gcst.SignedURL(gs.stream.Bucket(), gs.stream.Filename(), &opts); err != nil {
On 2016/12/01 17:39:31, dnj wrote:
> On 2016/11/30 21:03:52, Vadim Sh. wrote:
> > consider caching the result in the future. SignBytes is RPC to google
> backends,
> > it is relatively expensive (compared to memcache) and has a separate quota. 
> > 
> > e.g. if Logdog viewer switches to signed URLs exclusively, there'll be a LOT
> of
> > SignBytes requests.
> 
> This gets tricky, since the actual signed URLs have individual expiration
times
> associated with them.
That's why I assumed >= in sign_entry_url_lifetime doc: it makes sense if
coordinator reuses signed URLs.

https://codereview.chromium.org/2538203002/diff/20001/logdog/api/endpoints/co...
File logdog/api/endpoints/coordinator/logs/v1/logs.proto (right):

https://codereview.chromium.org/2538203002/diff/20001/logdog/api/endpoints/co...
logdog/api/endpoints/coordinator/logs/v1/logs.proto:136: //
"sign_entry_url_lifetime".
"sign_entry_url_lifetime" is no longer defined