token-server: Fix panic when generating machine token for unrecognized machine.

token-server: Fix panic when generating machine token for unrecognized machine.

In this case operation returns MintMachineTokenResponse{ErrorCode: ...} (since
there's no way to return structured error responses via grpc.Error).

BigQuery logging code attempted to log such response and paniced on nil
dereference when attempting to examine the token.

Fix this, add a test and a bunch of asserts.

R=iannucci@chromium.org, smut@google.com
BUG=732467
Review-Url: https://codereview.chromium.org/2943403003
Committed: https://github.com/luci/luci-go/commit/71aba0a6cd7d6264e0ecdd38afe5b69db8c8556f

[Vadim Sh., 2017-06-20 03:49:50]

PTAL

Usually Nodir reviews this code, but he's OOO.

https://codereview.chromium.org/2943403003/diff/40001/tokenserver/appengine/i...
File tokenserver/appengine/impl/machinetoken/rpc_mint_machine_token.go (right):

https://codereview.chromium.org/2943403003/diff/40001/tokenserver/appengine/i...
tokenserver/appengine/impl/machinetoken/rpc_mint_machine_token.go:148: case
resp.ErrorCode != 0: // logic-level error
this is the actual fix: don't try to log the token if ErrorCode != 0 (there's
nothing to log)

[Vadim Sh., 2017-06-20 03:50:12]

The CQ bit was checked by vadimsh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 03:50:18]

Dry run: CQ is trying da patch.

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

[Vadim Sh., 2017-06-20 03:52:33]

On a related note, we should probably add a gRPC interceptor that converts
panics into codes.Internal gRPC errors (before reporting them to tsmon).

These HTTP 500s are currently completely invisible from token-server's viceroy
dashboard :)

[commit-bot: I haz the power, 2017-06-20 03:56:08]

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

[commit-bot: I haz the power, 2017-06-20 03:56:09]

Dry run: This issue passed the CQ dry run.

[Vadim Sh., 2017-06-20 04:25:57]

On 2017/06/20 03:52:33, Vadim Sh. wrote:
> On a related note, we should probably add a gRPC interceptor that converts
> panics into codes.Internal gRPC errors (before reporting them to tsmon).
> 
> These HTTP 500s are currently completely invisible from token-server's viceroy
> dashboard :)

This is in https://codereview.chromium.org/2945013002

[smut, 2017-06-20 22:37:17]

https://codereview.chromium.org/2943403003/diff/40001/tokenserver/appengine/i...
File tokenserver/appengine/impl/machinetoken/bigquery_log_test.go (right):

https://codereview.chromium.org/2943403003/diff/40001/tokenserver/appengine/i...
tokenserver/appengine/impl/machinetoken/bigquery_log_test.go:22: ctx :=
testingContext(testingCA)
Where is testingCA defined?

[iannucci, 2017-06-20 22:43:19]

lgtm

https://codereview.chromium.org/2943403003/diff/40001/tokenserver/appengine/i...
File tokenserver/appengine/impl/machinetoken/rpc_mocks_test.go (right):

https://codereview.chromium.org/2943403003/diff/40001/tokenserver/appengine/i...
tokenserver/appengine/impl/machinetoken/rpc_mocks_test.go:97: var testingCA =
certconfig.CA{
@smut: here's the definition of testingCA

[Vadim Sh., 2017-06-20 22:46:02]

https://codereview.chromium.org/2943403003/diff/40001/tokenserver/appengine/i...
File tokenserver/appengine/impl/machinetoken/rpc_mocks_test.go (right):

https://codereview.chromium.org/2943403003/diff/40001/tokenserver/appengine/i...
tokenserver/appengine/impl/machinetoken/rpc_mocks_test.go:97: var testingCA =
certconfig.CA{
On 2017/06/20 22:43:19, iannucci wrote:
> @smut: here's the definition of testingCA

yeah... I have no good method of defining global testing constants so they are
discoverable... I myself just do text search in a package when I see a reference
to a global variable.

[smut, 2017-06-20 22:51:58]

lgtm

[Vadim Sh., 2017-06-20 22:58:56]

The CQ bit was checked by vadimsh@chromium.org

[commit-bot: I haz the power, 2017-06-20 22:58:59]

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, 2017-06-20 23:04:06]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497999536012090,
"parent_rev": "985d9cf9b317c12615370ca4ebdebe44d3658a81", "commit_rev":
"71aba0a6cd7d6264e0ecdd38afe5b69db8c8556f"}

[commit-bot: I haz the power, 2017-06-20 23:04:13]

Description was changed from

==========
token-server: Fix panic when generating machine token for unrecognized machine.

In this case operation returns MintMachineTokenResponse{ErrorCode: ...} (since
there's no way to return structured error responses via grpc.Error).

BigQuery logging code attempted to log such response and paniced on nil
dereference when attempting to examine the token.

Fix this, add a test and a bunch of asserts.

R=iannucci@chromium.org, smut@google.com
BUG=732467
==========

to

==========
token-server: Fix panic when generating machine token for unrecognized machine.

In this case operation returns MintMachineTokenResponse{ErrorCode: ...} (since
there's no way to return structured error responses via grpc.Error).

BigQuery logging code attempted to log such response and paniced on nil
dereference when attempting to examine the token.

Fix this, add a test and a bunch of asserts.

R=iannucci@chromium.org, smut@google.com
BUG=732467

Review-Url: https://codereview.chromium.org/2943403003
Committed:
https://github.com/luci/luci-go/commit/71aba0a6cd7d6264e0ecdd38afe5b69db8c8556f
==========

[commit-bot: I haz the power, 2017-06-20 23:04:14]

Committed patchset #3 (id:40001) as
https://github.com/luci/luci-go/commit/71aba0a6cd7d6264e0ecdd38afe5b69db8c8556f