CBG-4687 have sgcollect_info use a token #7604
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
rest/handler.go
Outdated
| if h.sgcollect && h.server.sgcollect.hasValidToken(h.rq.Header) { | ||
| return false |
There was a problem hiding this comment.
Is it worth detecting when a handler is called with an sgcollect token and the handler doesn't have h.sgcollect set?
It indicates we have added a new endpoint to sgcollect but forgotten to set the handler option which seems like an easy thing to forget.
AssertfCtx should let us catch this at dev time via the existing sgcollect tests we have.
There was a problem hiding this comment.
Added this with tests at some complexity of the code.
rest/sgcollect.go
Outdated
| if !strings.HasPrefix(auth, authPrefix) { | ||
| return false | ||
| } | ||
| if time.Since(sg.tokenAge) > 30*time.Minute { |
There was a problem hiding this comment.
Move this token timeout into a named const for clarity.
rest/sgcollect.go
Outdated
| if !strings.HasPrefix(auth, authPrefix) { | ||
| return false | ||
| } | ||
| if time.Since(sg.tokenAge) > 30*time.Minute { |
There was a problem hiding this comment.
I am wondering whether 30 minutes is long enough, or whether we'll actually get cases where sgcollect_info takes longer to run if there's a really big set of logs to process?
Do we have any information about how long we expect sgcollect_info to run for? Even locally it takes ~2 minutes to run.
There is probably data we could gather from previous collections to be sure this is long enough not to cause problems, but the easier alternative is to give us much more headroom to eliminate the chance of exceeding during a valid run, yet still have an upper-limit token lifespan (e.g. 6 hours) to avoid indefinite token lifespan in the case where we were unable to clear the token manually.
There was a problem hiding this comment.
I would potentially like a third opinion on this - the token expiry was a suggestion which I think is a useful defensive measure in case we or sgcollect screws up and keeps a token active for a long period, but I'm not quite sure what the expiry value should be.
/cc @adamcfraser
There was a problem hiding this comment.
We decided on 12 hours offline.
There was a problem hiding this comment.
Pull Request Overview
This PR implements token‐based authentication for sgcollect_info by replacing explicit username/password usage with auth headers throughout the code and tests, and by adding token generation, validation, and clearance in sgcollect_info.
- Replace basic auth functions with token-based auth and update function signatures accordingly.
- Update various task creation, HTTP client, and handler functions to use auth_headers.
- Update tests and routing to reflect these changes.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/tasks.py & tools/sgcollect.py | Update functions to accept auth_headers instead of user/pass. |
| tools-tests/* | Modify test mocks and calls to support auth_headers usage. |
| rest/sgcollect.go | Introduce token management for sgcollect_info and update env. |
| rest/routing.go & rest/handler.go | Update handler creation to pass sgcollect option and auth info. |
| base/* | Minor updates to test harness assertions and logging detail. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
rest/handler_test.go
Outdated
| } | ||
| // with valid sgcollect token, but sgcollect on the handler is disabled | ||
| require.NoError(t, sc.sgcollect.createNewToken()) | ||
| adminHandler = newHandler(sc, adminPrivs, adminServer, httptest.NewRecorder(), &http.Request{Header: http.Header{"Authorization": []string{"SGCollect invalid"}}}, handlerOptions{sgcollect: false}) |
There was a problem hiding this comment.
this should ideally be a valid token but the current implementation for the assertion doesn't care whether the token was valid or not, so it's fine
CBG-4687 have sgcollect_info use a token
For the reviewer:
This was done on top of #7601. Will turn into a non draft PR when that is merged.
Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiDependencies (if applicable)
Integration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGateway-Integration/3181/