CBG-4687 have sgcollect_info use a token #7596

torcolvin · 2025-06-18T02:16:37Z

CBG-4687 have sgcollect_info use a token

Created a field on handlers that marks if is sgcollect that indicates whether sgcollect will be called.
Create a specific token for performing these operations
Switch sgcollect code pass auth headers rather than explicit username/password

Risk mitigation/testing strategy:

Modifying the python code comes with some inherent risk due to type safety. I set up mypy to be able to run type checking and uv to freeze the environment. I can pull these changes out of this PR.
Created two types of integration testing:
- test endpoints with specific tokens
- create an integration test that runs with rosmar/couchbase server on all platforms.
Integration test for /_sgcollect_info comes at some risk and complexity, but it was useful to find bugs:
- Found unrelated bug where we need to specify --sync-gateway-url since it will try to connect on http://127.0.0.1:4985 and then https://127.0.0.1:4985
Test seams:
- modify sgcollectPath and sgcollectPathErr for testing. I don't know why these are globals in the first place. To run the tests on windows, you need to run python tools/sgcollect_info instead of tools/sgcollect_info since executing the file works on mac/linux by reading the shebang line. This lead to the weird modification of sgcollectPath to be a []string{} to fit in for testing. In production sgcollect_info is a self extracting binary that runs python so this is not used in a non testing path.
- I wanted to test to see if all the endpoints were hit and return 200. I don't know a way to test this using stats on the admin API, so I created sgCollect.stdout for testing only! I think there's some value in looking for errors, I found an exception we should handle in python get_paths_from_expvars if there was no file named *.json in the command line.
Actually running /_sgcollect_info is really useful, but it's really slow. It's about 1 minute locally (the bulk of the time is collecting the pprofs) and 6 minutes in github actions. I think this is is also subject to failure if there isn't python on the OS, or there's odd configurations. We run sgcollect_info in prod on python 3.9 (but it runs on 3.9 -> newest). I do not like having a slow test here. I considered running it only in jenkins, but I think this is actually some code where platform matter since there are "real paths" here. I do not think this is generally useful to run for the amount of time it takes.

Possible future refactors:

Removing sgcollectPath globals, this is really weird and if you are executing a subprocess doing a path manipulation is so fast. If we really did want to optimize it, you could add a sync.Once to ServerContext.
Rewriting sgcollect in go. This sounds really big, but this is a real mess to test and we could potentially share code in a library where /_sgcollect_info endpoint and an independent binary work together.

For the reviewer:

Input on the testing strategy, and whether we should drop some of this from CI
Manual testing with real SG and /_sgcollect_info endpoint and sgcollect_info executable

TODO

fix the -race condition that occurs because I try to buffer stdout to a channel in the test instance
Related to above, I think setting cmd.Stdout to io.Pipe might cause dangling go routines

Pre-review checklist

Removed debug logging (fmt.Print, log.Print, ...)
Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Integration Tests

GSI=true,xattrs=true https://jenkins.sgwdev.com/job/SyncGateway-Integration/000/

bbrks · 2025-06-18T13:48:37Z

rest/admin_api.go

+	addr, err := h.server.getServerAddr(adminServer)
+	if err != nil {
+		return base.HTTPErrorf(http.StatusInternalServerError, "Error getting admin server address: %v", err)
+	}
+	if h.server.Config.API.HTTPS.TLSCertPath != "" {
+		addr = "https://" + addr
+	} else {
+		params.adminURL = "http://" + addr
+	}
+	params.adminURL = addr


Whilst I agree this might be useful to do to avoid the sgcollect_info script iterating to find a working localhost URL as it does today, this is not related to the token feature and we should evaluate and implement each separately.

Let's try this URL and fall back to localhost

moved this into #7601 to be able to write the tests

bbrks · 2025-06-18T13:52:29Z

rest/server_context.go

@@ -2277,3 +2280,31 @@ func (sc *ServerContext) getClusterUUID(ctx context.Context) (string, error) {
 	}
 	return base.ParseClusterUUID(output)
 }
+
+type sgcollectCookieManager struct {
+	tokens map[string]struct{}


Do we ever expect to support more than 1 valid token at any one time?

AFAIK since /_sgcollect_info is a singleton and has sync mechanisms in place to avoid multiple runs, this isn't and won't be required.

I think this is adding complexity by maintaining a map, since it's more likely that we can unintentionally build up a set of valid/usable tokens.

It might be better with a single token string that has a timestamp to also allow expiry after a period of time. This covers the case where the sgcollect info script hangs indefinitely and keeps the token alive, or any situations where we may be unintentionally not deleting the token from the map.

bbrks · 2025-06-18T14:05:49Z

.github/workflows/ci.yml

+          uv run -- pytest
+      - name: Run mypy
+        run: |
+          uv run -- mypy tools/sgcollect_info tools


I am not super keen on changing dependency management for the sake of it but if we do want to do it - again we should do it independent of feature PRs.

bbrks · 2025-06-18T14:08:41Z

rest/sgcollect.go

 	return args
 }

 // sgCollectPaths attempts to return the absolute paths to Sync Gateway and to sgcollect_info binaries.
-func sgCollectPaths() (sgBinary, sgCollectBinary string, err error) {
+func sgCollectPaths() (sgBinary string, sgCollectBinary []string, err error) {


In what circumstances does sgCollectBinary need to be a slice? I'm confused...

It is ["python", "tools/sgcollect_info"] when called in tests (only!) but always just sgcollect_info which is a self extracting binary in prod.

bbrks · 2025-06-18T15:35:24Z

rest/sgcollect.go

@@ -28,6 +28,10 @@ import (
 	"github.com/pkg/errors"
 )

+const (
+	adminAPITokenHeader = "SG_BEARER_TOKEN"


Header->EnvVar

bbrks · 2025-06-18T15:36:45Z

tools/sgcollect_info

+        return {"Authorization": f"Bearer {session_token}"}
+    elif username:
+        base64string = base64.b64encode(
+            (f"{username}: {password}").encode("utf-8")


leading space is breaking password here

bbrks · 2025-06-18T15:56:27Z

tools/sgcollect_info

+    session_token_name = "SG_BEARER_TOKEN"
+    session_token = os.environ.get(session_token_name)
+    if session_token:
+        return {"Authorization": f"Bearer {session_token}"}


Replace Bearer with SGCollectToken

bbrks · 2025-06-18T15:57:06Z

tools/sgcollect_info

+    """
+    Return the headers for authentication.
+    """
+    session_token_name = "SG_BEARER_TOKEN"


SG_BEARER_TOKEN -> SG_COLLECT_TOKEN

bbrks · 2025-06-23T11:25:17Z

@torcolvin ready for a rebase now the other changes have been split out and merged (#7597)

torcolvin · 2025-06-24T02:38:59Z

Split into:

#7597
#7601
#7604

CBG-4687 have sgcollect_info take a token rather than username/password

efa5992

bbrks requested changes Jun 18, 2025

View reviewed changes

bbrks reviewed Jun 18, 2025

View reviewed changes

bbrks assigned torcolvin Jun 23, 2025

torcolvin closed this Jun 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

CBG-4687 have sgcollect_info use a token #7596

CBG-4687 have sgcollect_info use a token #7596

Uh oh!

torcolvin commented Jun 18, 2025

Uh oh!

bbrks Jun 18, 2025

Uh oh!

bbrks Jun 18, 2025

Uh oh!

torcolvin Jun 24, 2025

Uh oh!

bbrks Jun 18, 2025

Uh oh!

bbrks Jun 18, 2025

Uh oh!

bbrks Jun 18, 2025

Uh oh!

torcolvin Jun 24, 2025

Uh oh!

bbrks Jun 18, 2025

Uh oh!

bbrks Jun 18, 2025 •

edited

Loading

Uh oh!

bbrks Jun 18, 2025

Uh oh!

bbrks Jun 18, 2025

Uh oh!

bbrks commented Jun 23, 2025

Uh oh!

torcolvin commented Jun 24, 2025

Uh oh!

Uh oh!

CBG-4687 have sgcollect_info use a token #7596

CBG-4687 have sgcollect_info use a token #7596

Uh oh!

Conversation

torcolvin commented Jun 18, 2025

TODO

Pre-review checklist

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!

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!

Choose a reason for hiding this comment

Uh oh!

bbrks Jun 18, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

bbrks commented Jun 23, 2025

Uh oh!

torcolvin commented Jun 24, 2025

Uh oh!

Uh oh!

bbrks Jun 18, 2025 •

edited

Loading