fix(rest): apply server config to HTTP session#1440
Conversation
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Applying server config to the session is the right fix. One non-blocking resource-cleanup nit (inline).
| } | ||
|
|
||
| func (r *Catalog) fetchConfig(ctx context.Context, opts *options) (*http.Client, *options, error) { | ||
| func (r *Catalog) fetchConfig(ctx context.Context, opts *options) (*options, error) { |
There was a problem hiding this comment.
Non-blocking: the bootstrap session created solely to fetch /v1/config is discarded without closing idle connections, so initializing many catalogs can leave idle sockets/goroutines around until transport timeouts (more noticeable with bootstrap OAuth). Consider calling CloseIdleConnections() on internally-owned bootstrap transports after the config request, or returning a cleanup handle so user-provided transports aren't closed.
…-overrides # Conflicts: # catalog/rest/rest.go
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Thanks! One resource-leak issue before merge:
Medium — OAuth bootstrap transport leaked (catalog/rest/rest.go:980)
Bootstrap cleanup only closes the catalog session transport. When WithOAuthTLSConfig + credentials are used, setupOAuthManager creates a separate internal OAuth HTTP transport for the bootstrap token request, but that transport is discarded without CloseIdleConnections, leaving idle OAuth connections/goroutines behind for each catalog initialization.
Fix: track and close internally-created OAuth bootstrap transports as part of temporary session cleanup, while still not closing user-provided transports.
2 participants
REST configuration was fetched with a session constructed from the initial client options, and that same session remained active after server defaults and overrides were merged. Transport-level configuration therefore appeared in catalog properties without affecting later requests.
Initialization now uses the initial session only to fetch
/v1/config, then creates the catalog session from the merged options. Session construction no longer stores an automatically created OAuth manager in the options, preventing bootstrap authentication state from carrying into the final session. The final client is created before applying the catalog prefix so OAuth endpoints remain rooted at/v1.The regression test supplies a required header through config overrides, performs a catalog operation, and verifies that only the post-config request receives it.
Tests:
go test ./...golangci-lint run --timeout=10m