-
Notifications
You must be signed in to change notification settings - Fork 1.3k
event_logs: instrument SOAP-related event logs #44325
Conversation
49b4aee to
b6e04dc
Compare
2d75fe1 to
5fa9d6d
Compare
d8dcd53 to
9ce8705
Compare
5fa9d6d to
915bc72
Compare
ef0da48 to
257d500
Compare
257d500 to
2a5b04e
Compare
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff e8a9c9c...2a5b04e.
|
|
@sourcegraph/iam Please take this as a soft review request, because I don't know who else to tag 😁 |
michaellzc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
followed the test plan and it worked for me
code changes LGTM
| // FIXME: Can we find a way to do this only for SOAP users? | ||
| soapCount, err := db.UserExternalAccounts().Count( | ||
| r.Context(), | ||
| database.ExternalAccountsListOptions{ | ||
| UserID: subjectUserID, | ||
| ServiceType: auth.SourcegraphOperatorProviderType, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On dotcom with 34000+ rows in the user_external_accounts, this query takes ~10ms (with no result, since SOAP users are very rare), just as a data point. I think it is acceptable for now.
If this becomes a problem, we could store whether a user is created by SOAP in Redis to reduce the number of direct DB queries. Or very dumb in-memory cache within the frontend.
|
Merging as-is, post-merge comments are welcome (as always) and will address them once I'm back from PTO (Nov 28th)! |
This PR adds instruments for SOAP-related event logs, including:
"sourcegraph_operator": truein thepublic_argumentandargumentcolumns respectively.Below are the captures of event logs that we want in both
event_logsandsecurity_event_logstables:Test plan
site-config.json) with the following content, the credentials can be obtained from the "Okta test instance admin" in 1Password and the "OpenID Connect (sourcegraph.test:3443)" application:{ "authProviders": { "sourcegraphOperator": { "issuer": "https://dev-433675.oktapreview.com", "clientID": "<REDACTED>", "clientSecret": "<REDACTED>", "lifecycleDuration": 60 } } }singer.key), and use it to sign the site config:SRC_CLOUD_SITE_CONFIG"licenseKey"from thedev-private/enterprise/dev/site-config.jsonevent_logsandsecurity_event_logsare attributed with"sourcegraph_operator": trueinpublic_argumentandargumentcolumns respectively.Stacked on https://github.com/sourcegraph/sourcegraph/pull/44200,closes https://github.com/sourcegraph/customer/issues/1428