graphqlbackend: add permissionsSyncJobs query#44387

bobheadxi · 2022-11-14T23:40:15Z

Adds a new permissionsSyncJobs that returns the most recent outcomes of permissions sync jobs, with per-provider details included. This will be used to validate permissions syncing is starting correctly and the desired providers are being involved and in a healthy state.

We don't implement pagination since the endpoint is intended as a stream of recent events and retention is low (see https://github.com/sourcegraph/sourcegraph/pull/44258), but the endpoint schema is set up so that it can be extended to support pagination in the future.

Closes https://github.com/sourcegraph/customer/issues/1534
Stacked on https://github.com/sourcegraph/sourcegraph/pull/44258

Test plan

Add a GitHub code host with "authorization": {}, and add a private repo to the list

Wait a few minutes

Remove the token:

Get by ID:

bobheadxi · 2022-11-14T23:40:19Z

Current dependencies on/for this PR:

main
- PR PermsSyncer: refactor fetchUserPermsViaExternalAccounts results #44251
  - PR PermsSyncer: report exact state of each provider in each job #44257
    - PR PermsSyncer: track recent job states in redis #44258
      - PR graphqlbackend: add permissionsSyncJobs query #44387 👈

This comment was auto-generated by Graphite.

sg-e2e-regression-test-bob · 2022-11-14T23:47:44Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
0.01% (+0.26 kb)	0.00% (+0.51 kb)	0.00% (+0.25 kb)	0.00% (0)

Look at the Statoscope report for a full comparison between the commits 08153cc and d2e4f01 or learn more.

Open explanation

Initial size is the size of the initial bundle (the one that is loaded when you open the page)
Total size is the size of the initial bundle + all the async loaded chunks
Async size is the size of all the async loaded chunks
Modules is the number of modules in the initial bundle

kopancek · 2022-11-15T17:10:19Z

cmd/frontend/graphqlbackend/authz.graphql

We already have a standard way to do pagination. Based on PageInfo type defined in graphql (IIRC). Which in turn is based on cursor pagination, not offset and limit.

I think we can reuse it instead of introducing custom code. The convention is then to ask for

first: Int
after: String
Where first is basically the limit as you use it. And after is the cursor that points to the item after which want to get the next page worth of items. You can see it in action on org members here.

Ah, I copied this from another endpoint here. That said, I currently don't intend for this endpoint to be paginated - it should only ever return the X most recent runs, older runs will expire and should not be paginated for

For now I'm opting to not implement pagination here, since the endpoint and data are not designed to return enough data to warrant pagination at the moment. I've updated various docstrings, and also made the data retention a more aggressive 5m by default (https://github.com/sourcegraph/sourcegraph/pull/44258)

I did rename count to first, however, so the parameters can one day be extended to paginate if needed!

enterprise/cmd/frontend/internal/authz/resolvers/permissions_sync_jobs.go

kopancek · 2022-11-15T17:15:20Z

enterprise/cmd/frontend/internal/authz/resolvers/permissions_sync_jobs.go

I think you can consider splitting all of these resolvers into their own file. They seem to be completely separate from responsibility point of view.

I think I will keep these in the same file for now - they are both very thin adapters for the GraphQL interfaces, and it seems the general practice is to keep them together (e.g. the bitbucket endpoint resolver)

enterprise/cmd/frontend/internal/authz/resolvers/permissions_sync_jobs.go

enterprise/cmd/frontend/internal/authz/resolvers/resolver.go

eseliger

lgtm, pending implementing the remaining comments (not sure if review was re-requested intentionally yet?)

cmd/frontend/graphqlbackend/authz.graphql

eseliger · 2022-11-16T16:55:18Z

cmd/frontend/graphqlbackend/authz.graphql

yeah I think that's ok, if we want to avoid confusion we might want to rename it to something else if we got anything better, but not strictly required.

eseliger

LGTM, pending one last thing for Node

eseliger · 2022-11-16T17:15:32Z

enterprise/cmd/frontend/internal/authz/resolvers/resolver.go

This resolver should have a method called NodeResolvers

Should be called like that: https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/cmd/frontend/graphqlbackend/graphqlbackend.go?L366 for the authz resolver in the same method
For that, add to the interface type this signature https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/cmd/frontend/graphqlbackend/batches.go?L302
Finally, the NodeResolvers method should return a mapping from Kind to a resolver func.

Thanks, added and tested! https://github.com/sourcegraph/sourcegraph/pull/44387/commits/1687dfe48c520525e2aee023fcd20f9057d87fa5

Looks good!

enterprise/cmd/frontend/internal/authz/resolvers/permissions_sync_jobs.go

Co-authored-by: Erik Seliger <erikseliger@me.com>

Changes to make query align with pagination/Node paradigms made and reviewed by Erik

keegancsmith · 2023-01-19T07:38:07Z

enterprise/internal/authz/syncjobs/records_reader.go

+type recordsReader struct {
+	// readOnlyCache is a replaceable abstraction over rcache.Cache.
+	readOnlyCache interface {
+		ListKeys(ctx context.Context) ([]string, error)


@bobheadxi sorry to revive an old PR. But I've been reviewing our use of redis and noticed this use of ListKeys. FYI this operation has the complexity of listing all keys on the redis instance, not just those associated with the named cache. It will only return the relevant keys, but redis doesn't have a way to do this otherwise.

How many keys do you expect us to get? From looking around, it seems like this would be better suited using rcache.FIFOList. WDYT?
Instead of this

The FIFOList wasn't available at the time, but I did spot it in other PRs - I can make the switch here :) Will open a PR tomorrow!

cla-bot bot added the cla-signed label Nov 14, 2022

This was referenced Nov 14, 2022

PermsSyncer: track recent job states in redis #44258

Merged

PermsSyncer: report exact state of each provider in each job #44257

Merged

PermsSyncer: refactor fetchUserPermsViaExternalAccounts results #44251

Merged

bobheadxi force-pushed the 11-14-graphqlbackend_add_permissionsSyncJobs_endpoint branch from 6b0db24 to eeb2359 Compare November 15, 2022 00:07

bobheadxi force-pushed the 11-10-PermsSyncer_track_recent_job_states_in_redis branch from ba45511 to 3cde488 Compare November 15, 2022 16:43

bobheadxi force-pushed the 11-14-graphqlbackend_add_permissionsSyncJobs_endpoint branch from eeb2359 to 013067d Compare November 15, 2022 16:43

bobheadxi force-pushed the 11-10-PermsSyncer_track_recent_job_states_in_redis branch from 3cde488 to fa813d6 Compare November 15, 2022 16:45

bobheadxi force-pushed the 11-14-graphqlbackend_add_permissionsSyncJobs_endpoint branch from 013067d to fd4c405 Compare November 15, 2022 16:45

kopancek reviewed Nov 15, 2022

View reviewed changes

bobheadxi force-pushed the 11-10-PermsSyncer_track_recent_job_states_in_redis branch from fa813d6 to f78b88d Compare November 15, 2022 17:46

bobheadxi force-pushed the 11-14-graphqlbackend_add_permissionsSyncJobs_endpoint branch 2 times, most recently from 1d82a35 to b4cba34 Compare November 15, 2022 17:48

bobheadxi force-pushed the 11-10-PermsSyncer_track_recent_job_states_in_redis branch from 5169b47 to 17edfe2 Compare November 15, 2022 18:07

bobheadxi force-pushed the 11-14-graphqlbackend_add_permissionsSyncJobs_endpoint branch 5 times, most recently from e2287c5 to f6e62b8 Compare November 15, 2022 23:40

bobheadxi force-pushed the 11-10-PermsSyncer_track_recent_job_states_in_redis branch from 57cdeed to 244d49a Compare November 15, 2022 23:42

bobheadxi force-pushed the 11-14-graphqlbackend_add_permissionsSyncJobs_endpoint branch 5 times, most recently from 1465a70 to 480e5ec Compare November 16, 2022 00:32

bobheadxi force-pushed the 11-10-PermsSyncer_track_recent_job_states_in_redis branch from 5eaae52 to edeecce Compare November 16, 2022 00:32

bobheadxi force-pushed the 11-14-graphqlbackend_add_permissionsSyncJobs_endpoint branch 2 times, most recently from 567ede0 to f030783 Compare November 16, 2022 00:42

bobheadxi force-pushed the 11-14-graphqlbackend_add_permissionsSyncJobs_endpoint branch 2 times, most recently from 3b69175 to 0e21a6c Compare November 16, 2022 16:44

bobheadxi requested a review from eseliger November 16, 2022 16:46

eseliger reviewed Nov 16, 2022

View reviewed changes

bobheadxi requested review from eseliger and mrnugget November 16, 2022 17:10

eseliger approved these changes Nov 16, 2022

View reviewed changes

bobheadxi force-pushed the 11-14-graphqlbackend_add_permissionsSyncJobs_endpoint branch from 3541b9b to a37cf74 Compare November 16, 2022 17:18

Base automatically changed from 11-10-PermsSyncer_track_recent_job_states_in_redis to main November 16, 2022 18:11

bobheadxi and others added 13 commits November 16, 2022 10:11

graphqlbackend: add permissionsSyncJobs endpoint

5fae4fe

rename count to first

c9c87d7

add changelog entry

5168b7d

add tests for ListKeys

de3e613

fixup

dd4fbce

docstrings

82c71e4

apply review comments

e949b20

expand docstrings for messages

b30edca

implement Node paradigms

21c4067

rename request to job

03fbc3c

Apply suggestions from code review

a5a7355

Co-authored-by: Erik Seliger <erikseliger@me.com>

use entity in gql ID

cbf9ef8

Co-authored-by: Erik Seliger <erikseliger@me.com>

implement resolve by ID

ba0c0db

bobheadxi force-pushed the 11-14-graphqlbackend_add_permissionsSyncJobs_endpoint branch from 1687dfe to ba0c0db Compare November 16, 2022 18:11

mention TTL configuration for sync jobs records

08153cc

bobheadxi merged commit 923a15a into main Nov 16, 2022

bobheadxi deleted the 11-14-graphqlbackend_add_permissionsSyncJobs_endpoint branch November 16, 2022 19:44

bobheadxi mentioned this pull request Nov 19, 2022

PermsSyncer: back queue with database #44633

Closed

keegancsmith reviewed Jan 19, 2023

View reviewed changes

Conversation

bobheadxi commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

bobheadxi commented Nov 14, 2022

Uh oh!

sg-e2e-regression-test-bob commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle size report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobheadxi Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eseliger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eseliger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

bobheadxi commented Nov 14, 2022 •

edited

Loading

sg-e2e-regression-test-bob commented Nov 14, 2022 •

edited

Loading

bobheadxi Nov 16, 2022 •

edited

Loading