Whitelisted group functionality (logged error: "Failed to create identity for , ..." ...) |
|||||
Issue descriptionThis 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
,
Mar 25 2018
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.
,
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
,
Mar 26 2018
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
,
Mar 27 2018
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.
,
Mar 27 2018
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.
,
Mar 27 2018
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 }
,
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
,
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
,
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
,
Mar 30 2018
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 |
|||||
Comment 1 by qyears...@chromium.org
, Mar 24 2018