From 7a30830dc2bdd1e00da7630a9297bc668d6e86f3 Mon Sep 17 00:00:00 2001 From: Erik Seliger Date: Thu, 2 May 2024 18:30:44 +0200 Subject: [PATCH] httpcli: Move init function from conf package into httpcli (#62318) There was a weird dependency between internal/conf and internal/httpcli because of a cyclic import. This is resolved by using conftypes instead, so we can properly split those concerns. Test plan: Existing tests don't break. --- cmd/frontend/shared/BUILD.bazel | 1 + cmd/frontend/shared/shared.go | 3 +- internal/conf/BUILD.bazel | 1 - internal/conf/init.go | 41 --------------- internal/httpcli/BUILD.bazel | 2 + internal/httpcli/external.go | 52 +++++++++---------- internal/httpcli/init.go | 36 +++++++++++++ internal/httpcli/redis_logger_middleware.go | 9 ++-- .../httpcli/redis_logger_middleware_test.go | 12 ++--- internal/service/svcmain/BUILD.bazel | 1 + internal/service/svcmain/svcmain.go | 2 + 11 files changed, 81 insertions(+), 79 deletions(-) create mode 100644 internal/httpcli/init.go diff --git a/cmd/frontend/shared/BUILD.bazel b/cmd/frontend/shared/BUILD.bazel index 7f083dbf549..8a67ee3a37e 100644 --- a/cmd/frontend/shared/BUILD.bazel +++ b/cmd/frontend/shared/BUILD.bazel @@ -47,6 +47,7 @@ go_library( "//internal/database/connections/live", "//internal/debugserver", "//internal/env", + "//internal/httpcli", "//internal/observation", "//internal/oobmigration/migrations/register", "//internal/service", diff --git a/cmd/frontend/shared/shared.go b/cmd/frontend/shared/shared.go index d5efc210230..44960fb7c8b 100644 --- a/cmd/frontend/shared/shared.go +++ b/cmd/frontend/shared/shared.go @@ -45,6 +45,7 @@ import ( "github.com/sourcegraph/sourcegraph/internal/conf/conftypes" "github.com/sourcegraph/sourcegraph/internal/database" connections "github.com/sourcegraph/sourcegraph/internal/database/connections/live" + "github.com/sourcegraph/sourcegraph/internal/httpcli" "github.com/sourcegraph/sourcegraph/internal/observation" "github.com/sourcegraph/sourcegraph/schema" ) @@ -138,7 +139,7 @@ func SwitchableSiteConfig() conftypes.WatchableSiteConfig { go func() { <-AutoUpgradeDone - conf.EnsureHTTPClientIsConfigured() + httpcli.Configure(confClient) switchable.WatchableSiteConfig = confClient for _, watcher := range switchable.watchers { confClient.Watch(watcher) diff --git a/internal/conf/BUILD.bazel b/internal/conf/BUILD.bazel index bd4fbc40daf..827399ddfc4 100644 --- a/internal/conf/BUILD.bazel +++ b/internal/conf/BUILD.bazel @@ -33,7 +33,6 @@ go_library( "//internal/dotcom", "//internal/env", "//internal/hashutil", - "//internal/httpcli", "//internal/jsonc", "//internal/license", "//internal/src-cli", diff --git a/internal/conf/init.go b/internal/conf/init.go index d7441f0d1b6..18861f4ad15 100644 --- a/internal/conf/init.go +++ b/internal/conf/init.go @@ -1,12 +1,5 @@ package conf -import ( - "reflect" - "sync" - - "github.com/sourcegraph/sourcegraph/internal/httpcli" -) - // Init function completes the initialization process of the conf package, starting the configuration continuous changes polling // if in client mode. The conf.Watch function can safely be called before calling Init to register callbacks reacting to the changes. // @@ -18,38 +11,4 @@ func Init() { go DefaultClient().continuouslyUpdate(nil) close(configurationServerFrontendOnlyInitialized) } - - EnsureHTTPClientIsConfigured() -} - -var ensureHTTPClientIsConfiguredOnce sync.Once - -// EnsureHTTPClientIsConfigured configures the httpcli package settings. We have to do this -// in this package as conf itself uses httpcli's internal client. -func EnsureHTTPClientIsConfigured() { - ensureHTTPClientIsConfiguredOnce.Do(func() { - go Watch(func() { - // TLS external config - tlsBefore := httpcli.TLSExternalConfig() - tlsAfter := Get().ExperimentalFeatures.TlsExternal - if !reflect.DeepEqual(tlsBefore, tlsAfter) { - httpcli.SetTLSExternalConfig(tlsAfter) - } - - // Outbound request log limit and redact headers - outboundRequestLogLimitBefore := httpcli.OutboundRequestLogLimit() - outboundRequestLogLimitAfter := int32(Get().OutboundRequestLogLimit) - if outboundRequestLogLimitBefore != outboundRequestLogLimitAfter { - httpcli.SetOutboundRequestLogLimit(outboundRequestLogLimitAfter) - } - redactOutboundRequestHeadersBefore := httpcli.RedactOutboundRequestHeaders() - redactOutboundRequestHeadersAfter := true - if Get().RedactOutboundRequestHeaders != nil { - redactOutboundRequestHeadersAfter = *Get().RedactOutboundRequestHeaders - } - if redactOutboundRequestHeadersBefore != redactOutboundRequestHeadersAfter { - httpcli.SetRedactOutboundRequestHeaders(redactOutboundRequestHeadersAfter) - } - }) - }) } diff --git a/internal/httpcli/BUILD.bazel b/internal/httpcli/BUILD.bazel index 2b288850c22..09c1eaf22fb 100644 --- a/internal/httpcli/BUILD.bazel +++ b/internal/httpcli/BUILD.bazel @@ -7,6 +7,7 @@ go_library( "client.go", "doc.go", "external.go", + "init.go", "noop_response_cache.go", "redis_logger_middleware.go", "transport.go", @@ -15,6 +16,7 @@ go_library( visibility = ["//:__subpackages__"], deps = [ "//internal/actor", + "//internal/conf/conftypes", "//internal/conf/deploy", "//internal/env", "//internal/hostmatcher", diff --git a/internal/httpcli/external.go b/internal/httpcli/external.go index baaa002318b..bb2e5064711 100644 --- a/internal/httpcli/external.go +++ b/internal/httpcli/external.go @@ -22,49 +22,49 @@ type externalTransport struct { effective *http.Transport } -var tlsExternalConfig struct { +var tlsExternalConfigStore struct { sync.RWMutex *schema.TlsExternal } -var outboundRequestLogLimit atomic.Int32 -var redactOutboundRequestHeaders atomic.Bool +var outboundRequestLogLimitStore atomic.Int32 +var redactOutboundRequestHeadersStore atomic.Bool -// SetTLSExternalConfig is called by the conf package whenever TLSExternalConfig changes. +// setTLSExternalConfig is called by the conf package whenever TLSExternalConfig changes. // This is needed to avoid circular imports. -func SetTLSExternalConfig(c *schema.TlsExternal) { - tlsExternalConfig.Lock() - tlsExternalConfig.TlsExternal = c - tlsExternalConfig.Unlock() +func setTLSExternalConfig(c *schema.TlsExternal) { + tlsExternalConfigStore.Lock() + tlsExternalConfigStore.TlsExternal = c + tlsExternalConfigStore.Unlock() } -// TLSExternalConfig returns the current value of the global TLS external config. -func TLSExternalConfig() *schema.TlsExternal { - tlsExternalConfig.RLock() - defer tlsExternalConfig.RUnlock() - return tlsExternalConfig.TlsExternal +// tlsExternalConfig returns the current value of the global TLS external config. +func tlsExternalConfig() *schema.TlsExternal { + tlsExternalConfigStore.RLock() + defer tlsExternalConfigStore.RUnlock() + return tlsExternalConfigStore.TlsExternal } -// SetOutboundRequestLogLimit is called by the conf package whenever OutboundRequestLogLimit changes. +// setOutboundRequestLogLimit is called by the conf package whenever OutboundRequestLogLimit changes. // This is needed to avoid circular imports. -func SetOutboundRequestLogLimit(i int32) { - outboundRequestLogLimit.Store(i) +func setOutboundRequestLogLimit(i int32) { + outboundRequestLogLimitStore.Store(i) } -// OutboundRequestLogLimit returns the current value of the global OutboundRequestLogLimit value. -func OutboundRequestLogLimit() int32 { - return outboundRequestLogLimit.Load() +// outboundRequestLogLimit returns the current value of the global OutboundRequestLogLimit value. +func outboundRequestLogLimit() int32 { + return outboundRequestLogLimitStore.Load() } -// SetRedactOutboundRequestHeaders is called by the conf package whenever the RedactOutboundRequestHeaders setting changes. +// setRedactOutboundRequestHeaders is called by the conf package whenever the RedactOutboundRequestHeaders setting changes. // This is needed to avoid circular imports. -func SetRedactOutboundRequestHeaders(b bool) { - redactOutboundRequestHeaders.Store(b) +func setRedactOutboundRequestHeaders(b bool) { + redactOutboundRequestHeadersStore.Store(b) } -// RedactOutboundRequestHeaders returns the current value of the global RedactOutboundRequestHeaders setting. -func RedactOutboundRequestHeaders() bool { - return redactOutboundRequestHeaders.Load() +// redactOutboundRequestHeaders returns the current value of the global redactOutboundRequestHeaders setting. +func redactOutboundRequestHeaders() bool { + return redactOutboundRequestHeadersStore.Load() } func (t *externalTransport) RoundTrip(r *http.Request) (*http.Response, error) { @@ -72,7 +72,7 @@ func (t *externalTransport) RoundTrip(r *http.Request) (*http.Response, error) { config, effective := t.config, t.effective t.mu.RUnlock() - if current := TLSExternalConfig(); current == nil { + if current := tlsExternalConfig(); current == nil { return t.base.RoundTrip(r) } else if !reflect.DeepEqual(config, current) { effective = t.update(r.Context(), current) diff --git a/internal/httpcli/init.go b/internal/httpcli/init.go new file mode 100644 index 00000000000..0c674efff22 --- /dev/null +++ b/internal/httpcli/init.go @@ -0,0 +1,36 @@ +package httpcli + +import ( + "reflect" + + "github.com/sourcegraph/sourcegraph/internal/conf/conftypes" +) + +// Configure configures the httpcli package settings with TLS and outbound request +// logging settings from the site configuration. +func Configure(cfg conftypes.WatchableSiteConfig) { + cfg.Watch(func() { + siteConfig := cfg.SiteConfig() + // TLS external config + tlsBefore := tlsExternalConfig() + tlsAfter := siteConfig.ExperimentalFeatures.TlsExternal + if !reflect.DeepEqual(tlsBefore, tlsAfter) { + setTLSExternalConfig(tlsAfter) + } + + // Outbound request log limit and redact headers + outboundRequestLogLimitBefore := outboundRequestLogLimit() + outboundRequestLogLimitAfter := int32(siteConfig.OutboundRequestLogLimit) + if outboundRequestLogLimitBefore != outboundRequestLogLimitAfter { + setOutboundRequestLogLimit(outboundRequestLogLimitAfter) + } + redactOutboundRequestHeadersBefore := redactOutboundRequestHeaders() + redactOutboundRequestHeadersAfter := true + if siteConfig.RedactOutboundRequestHeaders != nil { + redactOutboundRequestHeadersAfter = *siteConfig.RedactOutboundRequestHeaders + } + if redactOutboundRequestHeadersBefore != redactOutboundRequestHeadersAfter { + setRedactOutboundRequestHeaders(redactOutboundRequestHeadersAfter) + } + }) +} diff --git a/internal/httpcli/redis_logger_middleware.go b/internal/httpcli/redis_logger_middleware.go index 90ea6522a5e..4df37db8c3f 100644 --- a/internal/httpcli/redis_logger_middleware.go +++ b/internal/httpcli/redis_logger_middleware.go @@ -13,6 +13,7 @@ import ( "time" "github.com/sourcegraph/log" + "github.com/sourcegraph/sourcegraph/internal/conf/deploy" "github.com/sourcegraph/sourcegraph/internal/rcache" "github.com/sourcegraph/sourcegraph/internal/types" @@ -21,7 +22,7 @@ import ( // outboundRequestsRedisFIFOList is a FIFO redis cache to store the requests. var outboundRequestsRedisFIFOList = rcache.NewFIFOListDynamic("outbound-requests", func() int { - return int(OutboundRequestLogLimit()) + return int(outboundRequestLogLimit()) }) const sourcegraphPrefix = "github.com/sourcegraph/sourcegraph/" @@ -34,8 +35,8 @@ func redisLoggerMiddleware() Middleware { resp, err := cli.Do(req) duration := time.Since(start) - limit := OutboundRequestLogLimit() - shouldRedactSensitiveHeaders := !deploy.IsDev(deploy.Type()) || RedactOutboundRequestHeaders() + limit := outboundRequestLogLimit() + shouldRedactSensitiveHeaders := !deploy.IsDev(deploy.Type()) || redactOutboundRequestHeaders() // Feature is turned off, do not log if limit == 0 { @@ -126,7 +127,7 @@ func redisLoggerMiddleware() Middleware { // GetOutboundRequestLogItems returns all outbound request log items after the given key, // in ascending order, trimmed to maximum {limit} items. Example for `after`: "2021-01-01T00_00_00.000000". func GetOutboundRequestLogItems(ctx context.Context, after string) ([]*types.OutboundRequestLogItem, error) { - var limit = int(OutboundRequestLogLimit()) + var limit = int(outboundRequestLogLimit()) if limit == 0 { return []*types.OutboundRequestLogItem{}, nil diff --git a/internal/httpcli/redis_logger_middleware_test.go b/internal/httpcli/redis_logger_middleware_test.go index 5e165c4262d..560e30e52ac 100644 --- a/internal/httpcli/redis_logger_middleware_test.go +++ b/internal/httpcli/redis_logger_middleware_test.go @@ -88,7 +88,7 @@ func TestRedisLoggerMiddleware(t *testing.T) { } // Enable feature - setOutboundRequestLogLimit(t, 1) + mockOutboundRequestLogLimit(t, 1) for _, tc := range testCases { tc := tc @@ -138,7 +138,7 @@ func TestRedisLoggerMiddleware_multiple(t *testing.T) { rcache.SetupForTest(t) // Enable the feature - setOutboundRequestLogLimit(t, int32(limit)) + mockOutboundRequestLogLimit(t, int32(limit)) // Build client with middleware cli := redisLoggerMiddleware()(newFakeClient(http.StatusOK, []byte(`{"responseBody":true}`), nil)) @@ -284,8 +284,8 @@ func TestRedisLoggerMiddleware_formatStackFrame(t *testing.T) { } } -func setOutboundRequestLogLimit(t *testing.T, limit int32) { - old := OutboundRequestLogLimit() - SetOutboundRequestLogLimit(limit) - t.Cleanup(func() { SetOutboundRequestLogLimit(old) }) +func mockOutboundRequestLogLimit(t *testing.T, limit int32) { + old := outboundRequestLogLimit() + setOutboundRequestLogLimit(limit) + t.Cleanup(func() { setOutboundRequestLogLimit(old) }) } diff --git a/internal/service/svcmain/BUILD.bazel b/internal/service/svcmain/BUILD.bazel index 6037b0d522d..9ecd268a4d1 100644 --- a/internal/service/svcmain/BUILD.bazel +++ b/internal/service/svcmain/BUILD.bazel @@ -10,6 +10,7 @@ go_library( "//internal/debugserver", "//internal/env", "//internal/hostname", + "//internal/httpcli", "//internal/logging", "//internal/observation", "//internal/profiler", diff --git a/internal/service/svcmain/svcmain.go b/internal/service/svcmain/svcmain.go index 5430901175d..0ca474b2aee 100644 --- a/internal/service/svcmain/svcmain.go +++ b/internal/service/svcmain/svcmain.go @@ -12,6 +12,7 @@ import ( "github.com/sourcegraph/sourcegraph/internal/debugserver" "github.com/sourcegraph/sourcegraph/internal/env" "github.com/sourcegraph/sourcegraph/internal/hostname" + "github.com/sourcegraph/sourcegraph/internal/httpcli" "github.com/sourcegraph/sourcegraph/internal/logging" "github.com/sourcegraph/sourcegraph/internal/observation" "github.com/sourcegraph/sourcegraph/internal/profiler" @@ -95,6 +96,7 @@ func run( Logging: conf.NewLogsSinksSource(conf.DefaultClient()), Tracing: tracer.ConfConfigurationSource{WatchableSiteConfig: conf.DefaultClient()}, } + httpcli.Configure(conf.DefaultClient()) } if oobConfig.Logging != nil {