New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 825521 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Whitelisted group functionality (logged error: "Failed to create identity for , ..." ...)

Project Member Reported by qyears...@chromium.org, Mar 24 2018

Issue description

This is related to the whitelisted_group feature for filtering which CLs get analyzed.

Example log excerpt:

  2018-03-23 12:21:00.712 PDT
  Found tracked NEW change (garnet~master~I07741f69eeddf38605e835c45b77f15d302f4db6) with new revision; updating.
  ...
  2018-03-23 12:21:00.864 PDT
  Enqueue Analyze requests for 1 changes
  2018-03-23 12:21:00.864 PDT
  Failed to create identity for , err: auth: bad value "" for identity kind "user"

Related code:
https://cs.chromium.org/chromium/infra/go/src/infra/tricium/appengine/gerrit/poll.go?l=326
 
Cc: phosek@chromium.org
The above example log appears to explain why no analysis was started for https://fuchsia-review.googlesource.com/c/garnet/+/135473.
The related code is at https://chromium.googlesource.com/infra/infra/+/6173fbcb7bc5b94569c83691e4afeeaec5dbfe4f/go/src/infra/tricium/appengine/gerrit/poll.go#326.

The error indicates that c.Owner.Email is an empty string, where c is a Gerrit ChangeInfo struct (https://godoc.org/golang.org/x/build/gerrit#ChangeInfo).

Looking at the Gerrit REST API docs, it appears the owner email isn't included because "DETAILED_ACCOUNTS" (https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#detailed-accounts) isn't added to the Gerrit query when polling. I removed it in https://chromium-review.googlesource.com/c/936689/ thinking that it wasn't necessary, but apparently it was.
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/36a54493a96a3a136c7e4b59f51f49555c6748a9

commit 36a54493a96a3a136c7e4b59f51f49555c6748a9
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Sun Mar 25 21:29:01 2018

Re-add DETAILED_ACCOUNTS and add explanatory comments.

This reverts commit 8bc457f8c46e365f755fc5d215f682676fd9a502.
In addition to a revert, this change also adds comments.

Reason for revert: Account details actually appear to be required for
the "whitelisted_group" functionality.

Also the original change description was wrong, it should have said
"DETAILED_ACCOUNTS", not "ACCOUNT_DETAIL".

Original change's description:
> Remove ACCOUNT_DETAIL from Gerrit poll request
>
> I believe that account details are not needed when
> polling Gerrit changes, what we just use the Gerrit
> changes response for constructing analyze requests,
> and an analyze request doesn't include any details
> about the CL author or reviewers.
>
> The reason for removing this is just to avoid
> requesting unnecessary details when polling Gerrit.
>
> Change-Id: Ie83c76352fba4fee446621b2d3cd0c17be553ed1
> Reviewed-on: https://chromium-review.googlesource.com/936689
> Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>
> Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>

Bug:  825521 
Change-Id: I72d4a66e9e60dbf6284557b42edb6491d5eac79f
Reviewed-on: https://chromium-review.googlesource.com/979712
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>

[modify] https://crrev.com/36a54493a96a3a136c7e4b59f51f49555c6748a9/go/src/infra/tricium/appengine/gerrit/gerrit_test.go
[modify] https://crrev.com/36a54493a96a3a136c7e4b59f51f49555c6748a9/go/src/infra/tricium/appengine/gerrit/gerrit.go

Status: Started (was: Assigned)
A new version was deployed that includes the above change, and now the Go app is throwing panic: runtime error: invalid memory address or nil pointer dereference on this line:
  authOK, err := auth.GetState(ctx).DB().IsMember(ctx, ident, gerritDetails.WhitelistedGroup)

Possibilities:
 - auth.GetState(ctx) might return nil
 - state.DB() might return nil
Update: I tried temporarily switching to a dirty version with https://chromium-review.googlesource.com/c/infra/infra/+/981524 and discovered that auth.GetState(ctx) is null because ctx doesn't contain a State object as expected.
Summary: Whitelisted group functionality (logged error: "Failed to create identity for , ..." ...) (was: Logged error in tricium-prod: "Failed to create identity for , ...")
Update again: ctx doesn't contain State because it's normally set up in middleware which is not currently used for internal routes in tricium such as the gerrit poller:
https://cs.chromium.org/chromium/infra/go/src/infra/tricium/appengine/common/common.go?q=common/common.go&l=59

Nodir suggested adding a auth.Authenticate middleware there (maybe setting the current user explicitly as anonymous). After setting that up, the original check for group membership might work.
Cc: no...@chromium.org
Quick note: we will indeed want to use an explicit "anonymous" authentication method, since the auth library doesn't accept an Authenticate middleware with no methods:

https://cs.chromium.org/chromium/infra/go/src/go.chromium.org/luci/server/auth/auth.go?l=147

        ...
	// We will need working DB factory below to check IP whitelist.
	cfg := getConfig(c)
	if cfg == nil || cfg.DBProvider == nil || len(a.Methods) == 0 {
		report(ErrNotConfigured, "ERROR_NOT_CONFIGURED")
		return nil, ErrNotConfigured
	}
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 28 2018

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/infradata/config/+/b8fa8174dd83287f0cbc493b15f61557c4e083ca

commit b8fa8174dd83287f0cbc493b15f61557c4e083ca
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Wed Mar 28 00:05:02 2018

Project Member

Comment 9 by bugdroid1@chromium.org, Mar 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/b7fc09a1b0d427e4a4fe14bcd1330a11a5c0650b

commit b7fc09a1b0d427e4a4fe14bcd1330a11a5c0650b
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Thu Mar 29 15:44:40 2018

For internal routes, set up auth state using anonymous identity

This is the fix proposed by Nodir for the issue that State
wasn't set in the context. The reason was because normally
that State is set up by Middleware, but the MiddlewareChain
for internal routes (including the poll cron job) didn't
include anything that sets up State.

This CL adds a Middleware auth.Authenticate that sets the
current identity as anonymous and doesn't do anything else.
This is OK for the poller since the current identity isn't
checked; it's the CL owner's identity that's checked.

As a follow-up, I'd like to:

 1. Add "anonymous" auth method in the relevant package in luci-go
    (maybe "go.chromium.org/luci/appengine/gaeauth/server"?)
 2. Change this code to use that.

I tested this by deploying a dirty version for a minute in
the evening, and it appeared to work as expected.

Bug:  825521 
Change-Id: I7f49fca10545517bbf798d75b593181688fcfbe7
Reviewed-on: https://chromium-review.googlesource.com/985392
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>

[modify] https://crrev.com/b7fc09a1b0d427e4a4fe14bcd1330a11a5c0650b/go/src/infra/tricium/appengine/common/common.go

Project Member

Comment 10 by bugdroid1@chromium.org, Mar 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/faf639998786c01cb6e99bd640f5348c92e53cba

commit faf639998786c01cb6e99bd640f5348c92e53cba
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Thu Mar 29 17:42:40 2018

Add more handling when checking whitelisted groups and expand test

Specifically:
 - log an error but then continue and don't analyze
   when whitelisted groups can't be checked.
 - Expand gerrit/poll.go unit test to cover whitelisted_groups

Bug:  825521 
Change-Id: I0736e7888d61333f9363c5343461847d6aac1e04
Reviewed-on: https://chromium-review.googlesource.com/981524
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>

[modify] https://crrev.com/faf639998786c01cb6e99bd640f5348c92e53cba/go/src/infra/tricium/appengine/gerrit/poll.go
[modify] https://crrev.com/faf639998786c01cb6e99bd640f5348c92e53cba/go/src/infra/tricium/appengine/gerrit/poll_test.go

Status: Fixed (was: Started)
Now fixed; see https://fuchsia-review.googlesource.com/c/garnet/+/135473.

So, the current state is, for now Tricium still uses the service config for whitelist groups, but we should now be able to add experimental analyzers in the project config and see them run.

Sign in to add a comment