registry+getproviders: Registry client policy centralized in main

The primary reason for this change is that registry.NewClient was
originally imposing its own decision about service discovery request
policy on every other user of the shared disco.Disco object by modifying
it directly.

We have been moving towards using a dependency inversion style where
package main is responsible for deciding how everything should be
configured based on global CLI arguments, environment variables, and the
CLI configuration, and so this commit moves to using that model for the
HTTP clients used by the module and provider registry client code.

This also makes explicit what was previously hidden away: that all service
discovery requests are made using the same HTTP client policy as for
requests to module registries, even if the service being discovered is not
a registry. This doesn't seem to have been the intention of the code as
previously written, but was still its ultimate effect: there is only one
disco.Disco object shared across all discovery callers and so changing its
configuration in any way changes it for everyone.

This initial rework is certainly not perfect: these components were not
originally designed to work in this way and there are lots of existing
test cases relying on them working the old way, and so this is a compromise
to get the behavior we now need (using consistent HTTP client settings
across all callers) without disrupting too much existing code.

Signed-off-by: Martin Atkins <mart@degeneration.co.uk>

cmd/tofu/commands.go

@@ -11,6 +11,7 @@ import (
 	"os/signal"
 
 	"github.com/hashicorp/go-plugin"
+	"github.com/hashicorp/go-retryablehttp"
 	svchost "github.com/hashicorp/terraform-svchost"
 	"github.com/hashicorp/terraform-svchost/auth"
 	"github.com/hashicorp/terraform-svchost/disco"
@@ -109,6 +110,12 @@ func initCommands(
 		ShutdownCh:    makeShutdownCh(),
 		CallerContext: ctx,
 
+		MakeRegistryHTTPClient: func() *retryablehttp.Client {
+			// This ctx is used only to choose global configuration settings
+			// for the client, and is not retained as part of the result for
+			// making individual HTTP requests.
+			return newRegistryHTTPClient(ctx)
+		},
 		ModulePackageFetcher: modulePkgFetcher,
 		ProviderSource:       providerSrc,
 		ProviderDevOverrides: providerDevOverrides,

cmd/tofu/main.go

@@ -18,7 +18,6 @@ import (
 	"time"
 
 	"github.com/hashicorp/go-plugin"
-	"github.com/hashicorp/terraform-svchost/disco"
 	"github.com/mattn/go-shellwords"
 	"github.com/mitchellh/cli"
 	"github.com/mitchellh/colorstring"
@@ -27,7 +26,6 @@ import (
 	"github.com/opentofu/opentofu/internal/command/cliconfig"
 	"github.com/opentofu/opentofu/internal/command/format"
 	"github.com/opentofu/opentofu/internal/didyoumean"
-	"github.com/opentofu/opentofu/internal/httpclient"
 	"github.com/opentofu/opentofu/internal/logging"
 	"github.com/opentofu/opentofu/internal/terminal"
 	"github.com/opentofu/opentofu/internal/tracing"
@@ -161,25 +159,17 @@ func realMain() int {
 	}
 
 	// Get any configured credentials from the config and initialize
-	// a service discovery object. The slightly awkward predeclaration of
-	// disco is required to allow us to pass untyped nil as the creds source
-	// when creating the source fails. Otherwise we pass a typed nil which
-	// breaks the nil checks in the disco object
-	var services *disco.Disco
+	// a service discovery object.
 	credsSrc, err := credentialsSource(config)
-	if err == nil {
-		services = disco.NewWithCredentialsSource(credsSrc)
-	} else {
+	if err != nil {
 		// Most commands don't actually need credentials, and most situations
 		// that would get us here would already have been reported by the config
 		// loading above, so we'll just log this one as an aid to debugging
 		// in the unlikely event that it _does_ arise.
 		log.Printf("[WARN] Cannot initialize remote host credentials manager: %s", err)
-		// passing (untyped) nil as the creds source is okay because the disco
-		// object checks that and just acts as though no credentials are present.
-		services = disco.NewWithCredentialsSource(nil)
+		credsSrc = nil // must be an untyped nil for newServiceDiscovery to understand "no credentials available"
 	}
-	services.SetUserAgent(httpclient.OpenTofuUserAgent(version.String()))
+	services := newServiceDiscovery(ctx, credsSrc)
 
 	modulePkgFetcher := remoteModulePackageFetcher(ctx, config.OCICredentialsPolicy)
 

cmd/tofu/provider_source.go

@@ -185,7 +185,7 @@ func implicitProviderSource(services *disco.Disco) getproviders.Source {
 	// local copy will take precedence.
 	searchRules = append(searchRules, getproviders.MultiSourceSelector{
 		Source: getproviders.NewMemoizeSource(
-			getproviders.NewRegistrySource(services),
+			getproviders.NewRegistrySource(services, newRegistryHTTPClient(context.TODO())),
 		),
 		Exclude: directExcluded,
 	})
@@ -196,7 +196,7 @@ func implicitProviderSource(services *disco.Disco) getproviders.Source {
 func providerSourceForCLIConfigLocation(loc cliconfig.ProviderInstallationLocation, services *disco.Disco, makeOCICredsPolicy ociCredsPolicyBuilder) (getproviders.Source, tfdiags.Diagnostics) {
 	if loc == cliconfig.ProviderInstallationDirect {
 		return getproviders.NewMemoizeSource(
-			getproviders.NewRegistrySource(services),
+			getproviders.NewRegistrySource(services, newRegistryHTTPClient(context.TODO())),
 		), nil
 	}
 
@@ -225,7 +225,12 @@ func providerSourceForCLIConfigLocation(loc cliconfig.ProviderInstallationLocati
 			))
 			return nil, diags
 		}
-		return getproviders.NewHTTPMirrorSource(url, services.CredentialsSource()), nil
+		// For historical reasons, we use the registry client timeout for this
+		// even though this isn't actually a registry. The other behavior of
+		// this client is not suitable for the HTTP mirror source, so we
+		// don't use this client directly.
+		httpTimeout := newRegistryHTTPClient(context.TODO()).HTTPClient.Timeout
+		return getproviders.NewHTTPMirrorSource(url, services.CredentialsSource(), httpTimeout), nil
 
 	case cliconfig.ProviderInstallationOCIMirror:
 		mappingFunc := loc.RepositoryMapping

cmd/tofu/registries_disco.go

@@ -0,0 +1,89 @@
+// Copyright (c) The OpenTofu Authors
+// SPDX-License-Identifier: MPL-2.0
+// Copyright (c) 2023 HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package main
+
+import (
+	"context"
+	"log"
+	"os"
+	"strconv"
+	"time"
+
+	"github.com/hashicorp/go-retryablehttp"
+	"github.com/hashicorp/terraform-svchost/auth"
+	"github.com/hashicorp/terraform-svchost/disco"
+	"github.com/opentofu/opentofu/internal/httpclient"
+	"github.com/opentofu/opentofu/internal/logging"
+	"github.com/opentofu/opentofu/version"
+)
+
+const (
+	// registryDiscoveryRetryEnvName is the name of the environment variable that
+	// can be configured to customize number of retries for module and provider
+	// discovery requests with the remote registry.
+	registryDiscoveryRetryEnvName      = "TF_REGISTRY_DISCOVERY_RETRY"
+	registryDiscoveryDefaultRetryCount = 1
+
+	// registryClientTimeoutEnvName is the name of the environment variable that
+	// can be configured to customize the timeout duration (seconds) for module
+	// and provider discovery with a remote registry. For historical reasons
+	// this also applies to all service discovery requests regardless of whether
+	// they are registry-related.
+	registryClientTimeoutEnvName        = "TF_REGISTRY_CLIENT_TIMEOUT"
+	registryClientDefaultRequestTimeout = 10 * time.Second
+)
+
+// newServiceDiscovery returns a newly-created [disco.Disco] object that is
+// configured appropriately for use elsewhere in OpenTofu.
+//
+// The credSrc argument represents the policy for how the service discovery
+// object should obtain authentication credentials for service discovery
+// requests. Passing a nil credSrc is acceptable and means that all discovery
+// requests are to be made anonymously.
+func newServiceDiscovery(ctx context.Context, credSrc auth.CredentialsSource) *disco.Disco {
+	services := disco.NewWithCredentialsSource(credSrc)
+	services.SetUserAgent(httpclient.OpenTofuUserAgent(version.String()))
+
+	// For historical reasons, the registry request retry policy also applies
+	// to all service discovery requests, which we implement by using transport
+	// from a HTTP client that is configured for registry client use.
+	client := newRegistryHTTPClient(ctx)
+	services.Transport = client.HTTPClient.Transport
+
+	return services
+}
+
+// newRegistryHTTPClient returns a new HTTP client configured to respect the
+// automatic retry behavior expected for registry requests and service discovery
+// requests.
+func newRegistryHTTPClient(ctx context.Context) *retryablehttp.Client {
+	// The retry count is configurable by environment variable.
+	retryCount := registryDiscoveryDefaultRetryCount
+	if v := os.Getenv(registryDiscoveryRetryEnvName); v != "" {
+		override, err := strconv.Atoi(v)
+		if err == nil && override > 0 {
+			retryCount = override
+		}
+	}
+
+	// The timeout is also configurable by environment variable.
+	timeout := registryClientDefaultRequestTimeout
+	if v := os.Getenv(registryClientTimeoutEnvName); v != "" {
+		override, err := strconv.Atoi(v)
+		if err == nil && timeout > 0 {
+			timeout = time.Duration(override) * time.Second
+		}
+	}
+
+	client := httpclient.NewForRegistryRequests(ctx, retryCount, timeout)
+
+	// Per historical tradition our registry client also generates log messages
+	// describing the requests that it makes.
+	logOutput := logging.LogOutput()
+	client.Logger = log.New(logOutput, "", log.Flags())
+
+	return client
+}

internal/command/command_test.go

@@ -985,7 +985,7 @@ func testServices(t *testing.T) (services *disco.Disco, cleanup func()) {
 // of your test in order to shut down the test server.
 func testRegistrySource(t *testing.T) (source *getproviders.RegistrySource, cleanup func()) {
 	services, close := testServices(t)
-	source = getproviders.NewRegistrySource(services)
+	source = getproviders.NewRegistrySource(services, nil)
 	return source, close
 }
 

internal/command/meta.go

@@ -20,6 +20,7 @@ import (
 	"time"
 
 	"github.com/hashicorp/go-plugin"
+	"github.com/hashicorp/go-retryablehttp"
 	"github.com/hashicorp/terraform-svchost/disco"
 	"github.com/mitchellh/cli"
 	"github.com/mitchellh/colorstring"
@@ -139,6 +140,15 @@ type Meta struct {
 	// unit testing.
 	ModulePackageFetcher *getmodules.PackageFetcher
 
+	// MakeRegistryHTTPClient is a function called each time a command needs
+	// an HTTP client that will be used to make requests to a module or
+	// provider registry.
+	//
+	// This is used by package main to deal with some operator-configurable
+	// settings for retries and timeouts. If this isn't set then a new client
+	// with reasonable defaults for tests will be used instead.
+	MakeRegistryHTTPClient func() *retryablehttp.Client
+
 	// BrowserLauncher is used by commands that need to open a URL in a
 	// web browser.
 	BrowserLauncher webbrowser.Launcher

internal/command/meta_config.go

@@ -12,7 +12,9 @@ import (
 	"os"
 	"path/filepath"
 	"sort"
+	"time"
 
+	"github.com/hashicorp/go-retryablehttp"
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/hcl/v2/hclsyntax"
 	"github.com/zclconf/go-cty/cty"
@@ -22,6 +24,7 @@ import (
 	"github.com/opentofu/opentofu/internal/configs"
 	"github.com/opentofu/opentofu/internal/configs/configload"
 	"github.com/opentofu/opentofu/internal/configs/configschema"
+	"github.com/opentofu/opentofu/internal/httpclient"
 	"github.com/opentofu/opentofu/internal/initwd"
 	"github.com/opentofu/opentofu/internal/registry"
 	"github.com/opentofu/opentofu/internal/tfdiags"
@@ -456,7 +459,23 @@ func (m *Meta) initConfigLoader() (*configload.Loader, error) {
 
 // registryClient instantiates and returns a new Registry client.
 func (m *Meta) registryClient(ctx context.Context) *registry.Client {
-	return registry.NewClient(ctx, m.Services, nil)
+	httpClient := m.registryHTTPClient(ctx)
+	return registry.NewClient(ctx, m.Services, httpClient)
+}
+
+// registryHTTPClient returns a [retryablehttp.Client] intended for use in
+// any interactions with module or provider registries.
+//
+// This calls [Meta.MakeRegistryHTTPClient] if set, but provides a plausible
+// default client to use when that isn't set, since that's very common in
+// our test cases in this package.
+func (m *Meta) registryHTTPClient(ctx context.Context) *retryablehttp.Client {
+	if m.MakeRegistryHTTPClient != nil {
+		return m.MakeRegistryHTTPClient()
+	} else {
+		// Some reasonable default settings for most tests to use.
+		return httpclient.NewForRegistryRequests(ctx, 1, 10*time.Second)
+	}
 }
 
 // configValueFromCLI parses a configuration value that was provided in a

internal/command/providers_lock.go

@@ -134,12 +134,17 @@ func (c *ProvidersLockCommand) Run(args []string) int {
 			c.showDiagnostics(diags)
 			return 1
 		}
-		source = getproviders.NewHTTPMirrorSource(u, c.Services.CredentialsSource())
+		// For historical reasons, we use the registry client timeout for this
+		// even though this isn't actually a registry. The other behavior of
+		// this client is not suitable for the HTTP mirror source, so we
+		// don't use this client directly.
+		httpTimeout := c.registryHTTPClient(ctx).HTTPClient.Timeout
+		source = getproviders.NewHTTPMirrorSource(u, c.Services.CredentialsSource(), httpTimeout)
 	default:
 		// With no special options we consult upstream registries directly,
 		// because that gives us the most information to produce as complete
 		// and portable as possible a lock entry.
-		source = getproviders.NewRegistrySource(c.Services)
+		source = getproviders.NewRegistrySource(c.Services, c.registryHTTPClient(ctx))
 	}
 
 	config, confDiags := c.loadConfig(ctx, ".")

internal/command/providers_mirror.go

@@ -112,7 +112,7 @@ func (c *ProvidersMirrorCommand) Run(args []string) int {
 	// directory without needing to first disable that local mirror
 	// in the CLI configuration.
 	source := getproviders.NewMemoizeSource(
-		getproviders.NewRegistrySource(c.Services),
+		getproviders.NewRegistrySource(c.Services, c.registryHTTPClient(ctx)),
 	)
 
 	// Providers from registries always use HTTP, so we don't need the full

internal/getproviders/http_mirror_source.go

@@ -16,6 +16,7 @@ import (
 	"net/url"
 	"path"
 	"strings"
+	"time"
 
 	"github.com/hashicorp/go-retryablehttp"
 	svchost "github.com/hashicorp/terraform-svchost"
@@ -24,7 +25,6 @@ import (
 
 	"github.com/opentofu/opentofu/internal/addrs"
 	"github.com/opentofu/opentofu/internal/httpclient"
-	"github.com/opentofu/opentofu/internal/logging"
 	"github.com/opentofu/opentofu/version"
 )
 
@@ -46,10 +46,9 @@ var _ Source = (*HTTPMirrorSource)(nil)
 // (When the URL comes from user input, such as in the CLI config, it's the
 // UI/config layer's responsibility to validate this and return a suitable
 // error message for the end-user audience.)
-func NewHTTPMirrorSource(baseURL *url.URL, creds svcauth.CredentialsSource) *HTTPMirrorSource {
-	httpClient := httpclient.New(context.TODO())
-	httpClient.Timeout = requestTimeout
-	httpClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
+func NewHTTPMirrorSource(baseURL *url.URL, creds svcauth.CredentialsSource, requestTimeout time.Duration) *HTTPMirrorSource {
+	httpClient := httpclient.NewForRegistryRequests(context.TODO(), 0, requestTimeout)
+	httpClient.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
 		// If we get redirected more than five times we'll assume we're
 		// in a redirect loop and bail out, rather than hanging forever.
 		if len(via) > 5 {
@@ -60,25 +59,14 @@ func NewHTTPMirrorSource(baseURL *url.URL, creds svcauth.CredentialsSource) *HTT
 	return newHTTPMirrorSourceWithHTTPClient(baseURL, creds, httpClient)
 }
 
-func newHTTPMirrorSourceWithHTTPClient(baseURL *url.URL, creds svcauth.CredentialsSource, httpClient *http.Client) *HTTPMirrorSource {
+func newHTTPMirrorSourceWithHTTPClient(baseURL *url.URL, creds svcauth.CredentialsSource, httpClient *retryablehttp.Client) *HTTPMirrorSource {
 	if baseURL.Scheme != "https" {
 		panic("non-https URL for HTTP mirror")
 	}
-
-	// We borrow the retry settings and behaviors from the registry client,
-	// because our needs here are very similar to those of the registry client.
-	retryableClient := retryablehttp.NewClient()
-	retryableClient.HTTPClient = httpClient
-	retryableClient.RetryMax = discoveryRetry
-	retryableClient.RequestLogHook = requestLogHook
-	retryableClient.ErrorHandler = maxRetryErrorHandler
-
-	retryableClient.Logger = log.New(logging.LogOutput(), "", log.Flags())
-
 	return &HTTPMirrorSource{
 		baseURL:    baseURL,
 		creds:      creds,
-		httpClient: retryableClient,
+		httpClient: httpClient,
 	}
 }
 

internal/getproviders/http_mirror_source_test.go

@@ -14,6 +14,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/hashicorp/go-retryablehttp"
 	svchost "github.com/hashicorp/terraform-svchost"
 	svcauth "github.com/hashicorp/terraform-svchost/auth"
 
@@ -37,7 +38,9 @@ func TestHTTPMirrorSource(t *testing.T) {
 			"token": "placeholder-token",
 		},
 	})
-	source := newHTTPMirrorSourceWithHTTPClient(baseURL, creds, httpClient)
+	retryHTTPClient := retryablehttp.NewClient()
+	retryHTTPClient.HTTPClient = httpClient
+	source := newHTTPMirrorSourceWithHTTPClient(baseURL, creds, retryHTTPClient)
 
 	existingProvider := addrs.MustParseProviderSourceString("terraform.io/test/exists")
 	missingProvider := addrs.MustParseProviderSourceString("terraform.io/test/missing")
@@ -72,7 +75,7 @@ func TestHTTPMirrorSource(t *testing.T) {
 		}
 	})
 	t.Run("AvailableVersions without required credentials", func(t *testing.T) {
-		unauthSource := newHTTPMirrorSourceWithHTTPClient(baseURL, nil, httpClient)
+		unauthSource := newHTTPMirrorSourceWithHTTPClient(baseURL, nil, retryHTTPClient)
 		_, _, err := unauthSource.AvailableVersions(context.Background(), existingProvider)
 		switch err := err.(type) {
 		case ErrUnauthorized:

internal/getproviders/registry_client.go

@@ -13,13 +13,9 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"log"
 	"net/http"
 	"net/url"
-	"os"
 	"path"
-	"strconv"
-	"time"
 
 	"github.com/hashicorp/go-retryablehttp"
 	svchost "github.com/hashicorp/terraform-svchost"
@@ -29,8 +25,6 @@ import (
 	"go.opentelemetry.io/otel/trace"
 
 	"github.com/opentofu/opentofu/internal/addrs"
-	"github.com/opentofu/opentofu/internal/httpclient"
-	"github.com/opentofu/opentofu/internal/logging"
 	"github.com/opentofu/opentofu/internal/tracing"
 	"github.com/opentofu/opentofu/internal/tracing/traceattrs"
 	"github.com/opentofu/opentofu/version"
@@ -38,33 +32,8 @@ import (
 
 const (
 	terraformVersionHeader = "X-Terraform-Version"
-
-	// registryDiscoveryRetryEnvName is the name of the environment variable that
-	// can be configured to customize number of retries for module and provider
-	// discovery requests with the remote registry.
-	registryDiscoveryRetryEnvName = "TF_REGISTRY_DISCOVERY_RETRY"
-	registryClientDefaultRetry    = 1
-
-	// registryClientTimeoutEnvName is the name of the environment variable that
-	// can be configured to customize the timeout duration (seconds) for module
-	// and provider discovery with the remote registry.
-	registryClientTimeoutEnvName = "TF_REGISTRY_CLIENT_TIMEOUT"
-
-	// defaultRequestTimeout is the default timeout duration for requests to the
-	// remote registry.
-	defaultRequestTimeout = 10 * time.Second
 )
 
-var (
-	discoveryRetry int
-	requestTimeout time.Duration
-)
-
-func init() {
-	configureDiscoveryRetry()
-	configureRequestTimeout()
-}
-
 var SupportedPluginProtocols = MustParseVersionConstraints(">= 5, <7")
 
 // registryClient is a client for the provider registry protocol that is
@@ -77,22 +46,11 @@ type registryClient struct {
 	httpClient *retryablehttp.Client
 }
 
-func newRegistryClient(ctx context.Context, baseURL *url.URL, creds svcauth.HostCredentials) *registryClient {
-	httpClient := httpclient.New(ctx)
-	httpClient.Timeout = requestTimeout
-
-	retryableClient := retryablehttp.NewClient()
-	retryableClient.HTTPClient = httpClient
-	retryableClient.RetryMax = discoveryRetry
-	retryableClient.RequestLogHook = requestLogHook
-	retryableClient.ErrorHandler = maxRetryErrorHandler
-
-	retryableClient.Logger = log.New(logging.LogOutput(), "", log.Flags())
-
+func newRegistryClient(ctx context.Context, baseURL *url.URL, creds svcauth.HostCredentials, httpClient *retryablehttp.Client) *registryClient {
 	return &registryClient{
 		baseURL:    baseURL,
 		creds:      creds,
-		httpClient: retryableClient,
+		httpClient: httpClient,
 	}
 }
 
@@ -494,50 +452,6 @@ func (c *registryClient) getFile(ctx context.Context, url *url.URL) ([]byte, err
 	return data, nil
 }
 
-// configureDiscoveryRetry configures the number of retries the registry client
-// will attempt for requests with retryable errors, like 502 status codes
-func configureDiscoveryRetry() {
-	discoveryRetry = registryClientDefaultRetry
-
-	if v := os.Getenv(registryDiscoveryRetryEnvName); v != "" {
-		retry, err := strconv.Atoi(v)
-		if err == nil && retry > 0 {
-			discoveryRetry = retry
-		}
-	}
-}
-
-func requestLogHook(logger retryablehttp.Logger, req *http.Request, i int) {
-	if i > 0 {
-		logger.Printf("[INFO] Previous request to the remote registry failed, attempting retry.")
-	}
-}
-
-func maxRetryErrorHandler(resp *http.Response, err error, numTries int) (*http.Response, error) {
-	// Close the body per library instructions
-	if resp != nil {
-		resp.Body.Close()
-	}
-
-	// Additional error detail: if we have a response, use the status code;
-	// if we have an error, use that; otherwise nothing. We will never have
-	// both response and error.
-	var errMsg string
-	if resp != nil {
-		errMsg = fmt.Sprintf(": %s returned from %s", resp.Status, HostFromRequest(resp.Request))
-	} else if err != nil {
-		errMsg = fmt.Sprintf(": %s", err)
-	}
-
-	// This function is always called with numTries=RetryMax+1. If we made any
-	// retry attempts, include that in the error message.
-	if numTries > 1 {
-		return resp, fmt.Errorf("the request failed after %d attempts, please try again later%s",
-			numTries, errMsg)
-	}
-	return resp, fmt.Errorf("the request failed, please try again later%s", errMsg)
-}
-
 // HostFromRequest extracts host the same way net/http Request.Write would,
 // accounting for empty Request.Host
 func HostFromRequest(req *http.Request) string {
@@ -553,16 +467,3 @@ func HostFromRequest(req *http.Request) string {
 	// https://cs.opensource.google/go/go/+/refs/tags/go1.18.4:src/net/http/request.go;l=574
 	return ""
 }
-
-// configureRequestTimeout configures the registry client request timeout from
-// environment variables
-func configureRequestTimeout() {
-	requestTimeout = defaultRequestTimeout
-
-	if v := os.Getenv(registryClientTimeoutEnvName); v != "" {
-		timeout, err := strconv.Atoi(v)
-		if err == nil && timeout > 0 {
-			requestTimeout = time.Duration(timeout) * time.Second
-		}
-	}
-}

internal/getproviders/registry_client_test.go

@@ -13,7 +13,6 @@ import (
 	"net/http/httptest"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/apparentlymart/go-versions/versions"
 	"github.com/google/go-cmp/cmp"
@@ -22,75 +21,6 @@ import (
 	"github.com/opentofu/opentofu/internal/addrs"
 )
 
-func TestConfigureDiscoveryRetry(t *testing.T) {
-	t.Run("default retry", func(t *testing.T) {
-		if discoveryRetry != registryClientDefaultRetry {
-			t.Fatalf("expected retry %q, got %q", registryClientDefaultRetry, discoveryRetry)
-		}
-
-		rc := newRegistryClient(t.Context(), nil, nil)
-		if rc.httpClient.RetryMax != registryClientDefaultRetry {
-			t.Fatalf("expected client retry %q, got %q",
-				registryClientDefaultRetry, rc.httpClient.RetryMax)
-		}
-	})
-
-	t.Run("configured retry", func(t *testing.T) {
-		defer func() {
-			discoveryRetry = registryClientDefaultRetry
-		}()
-		t.Setenv(registryDiscoveryRetryEnvName, "2")
-
-		configureDiscoveryRetry()
-		expected := 2
-		if discoveryRetry != expected {
-			t.Fatalf("expected retry %q, got %q",
-				expected, discoveryRetry)
-		}
-
-		rc := newRegistryClient(t.Context(), nil, nil)
-		if rc.httpClient.RetryMax != expected {
-			t.Fatalf("expected client retry %q, got %q",
-				expected, rc.httpClient.RetryMax)
-		}
-	})
-}
-
-func TestConfigureRegistryClientTimeout(t *testing.T) {
-	t.Run("default timeout", func(t *testing.T) {
-		if requestTimeout != defaultRequestTimeout {
-			t.Fatalf("expected timeout %q, got %q",
-				defaultRequestTimeout.String(), requestTimeout.String())
-		}
-
-		rc := newRegistryClient(t.Context(), nil, nil)
-		if rc.httpClient.HTTPClient.Timeout != defaultRequestTimeout {
-			t.Fatalf("expected client timeout %q, got %q",
-				defaultRequestTimeout.String(), rc.httpClient.HTTPClient.Timeout.String())
-		}
-	})
-
-	t.Run("configured timeout", func(t *testing.T) {
-		defer func() {
-			requestTimeout = defaultRequestTimeout
-		}()
-		t.Setenv(registryClientTimeoutEnvName, "20")
-
-		configureRequestTimeout()
-		expected := 20 * time.Second
-		if requestTimeout != expected {
-			t.Fatalf("expected timeout %q, got %q",
-				expected, requestTimeout.String())
-		}
-
-		rc := newRegistryClient(t.Context(), nil, nil)
-		if rc.httpClient.HTTPClient.Timeout != expected {
-			t.Fatalf("expected client timeout %q, got %q",
-				expected, rc.httpClient.HTTPClient.Timeout.String())
-		}
-	})
-}
-
 // testRegistryServices starts up a local HTTP server running a fake provider registry
 // service and returns a service discovery object pre-configured to consider
 // the host "example.com" to be served by the fake registry service.
@@ -144,7 +74,7 @@ func testRegistryServices(t *testing.T) (services *disco.Disco, baseURL string,
 // of your test in order to shut down the test server.
 func testRegistrySource(t *testing.T) (source *RegistrySource, baseURL string, cleanup func()) {
 	services, baseURL, close := testRegistryServices(t)
-	source = NewRegistrySource(services)
+	source = NewRegistrySource(services, nil)
 	return source, baseURL, close
 }
 

internal/getproviders/registry_source.go

@@ -8,26 +8,38 @@ package getproviders
 import (
 	"context"
 	"fmt"
+	"time"
 
+	"github.com/hashicorp/go-retryablehttp"
 	svchost "github.com/hashicorp/terraform-svchost"
 	disco "github.com/hashicorp/terraform-svchost/disco"
 
 	"github.com/opentofu/opentofu/internal/addrs"
+	"github.com/opentofu/opentofu/internal/httpclient"
 )
 
 // RegistrySource is a Source that knows how to find and install providers from
 // their originating provider registries.
 type RegistrySource struct {
 	services   *disco.Disco
+	httpClient *retryablehttp.Client
 }
 
 var _ Source = (*RegistrySource)(nil)
 
 // NewRegistrySource creates and returns a new source that will install
 // providers from their originating provider registries.
-func NewRegistrySource(services *disco.Disco) *RegistrySource {
+func NewRegistrySource(services *disco.Disco, httpClient *retryablehttp.Client) *RegistrySource {
+	if httpClient == nil {
+		// As an aid to our tests that don't really care that much about
+		// the HTTP client configuration, we'll provide some reasonable
+		// defaults if no custom client is provided.
+		httpClient = httpclient.NewForRegistryRequests(context.Background(), 1, 10*time.Second)
+	}
+
 	return &RegistrySource{
 		services:   services,
+		httpClient: httpClient,
 	}
 }
 
@@ -147,7 +159,7 @@ func (s *RegistrySource) registryClient(ctx context.Context, hostname svchost.Ho
 		return nil, fmt.Errorf("failed to retrieve credentials for %s: %w", hostname, err)
 	}
 
-	return newRegistryClient(ctx, url, creds), nil
+	return newRegistryClient(ctx, url, creds, s.httpClient), nil
 }
 
 func (s *RegistrySource) ForDisplay(provider addrs.Provider) string {

internal/getproviders/registry_source_test.go

@@ -60,7 +60,7 @@ func TestSourceAvailableVersions(t *testing.T) {
 		{
 			"fails.example.com/foo/bar",
 			nil,
-			`could not query provider registry for fails.example.com/foo/bar: the request failed after 2 attempts, please try again later: Get "` + baseURL + `/fails-immediately/foo/bar/versions": EOF`,
+			`could not query provider registry for fails.example.com/foo/bar: request failed after 2 attempts: Get "` + baseURL + `/fails-immediately/foo/bar/versions": EOF`,
 		},
 	}
 
@@ -188,7 +188,7 @@ func TestSourcePackageMeta(t *testing.T) {
 			"1.2.0",
 			"linux", "amd64",
 			PackageMeta{},
-			`could not query provider registry for fails.example.com/awesomesauce/happycloud: the request failed after 2 attempts, please try again later: Get "http://placeholder-origin/fails-immediately/awesomesauce/happycloud/1.2.0/download/linux/amd64": EOF`,
+			`could not query provider registry for fails.example.com/awesomesauce/happycloud: request failed after 2 attempts: Get "http://placeholder-origin/fails-immediately/awesomesauce/happycloud/1.2.0/download/linux/amd64": EOF`,
 		},
 	}
 

internal/httpclient/registry_client.go

@@ -0,0 +1,85 @@
+// Copyright (c) The OpenTofu Authors
+// SPDX-License-Identifier: MPL-2.0
+// Copyright (c) 2023 HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package httpclient
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+	"time"
+
+	"github.com/hashicorp/go-retryablehttp"
+)
+
+// NewForRegistryRequests is a variant of [New] that deals with some additional
+// policy concerns related to "registry requests".
+//
+// Exactly what constitutes a "registry request" is actually more about
+// historical technical debt than intentional design, since these concerns
+// were originally supposed to be handled internally within the module and
+// provider registry clients but the implementation of that unfortunately caused
+// the effects to "leak out" into other parts of the system, which we now
+// preserve for backward compatibility here.
+//
+// Therefore "registry requests" includes the following:
+//   - All requests from the client of our network service discovery protocol,
+//     even though not all discoverable services are actually "registries".
+//   - Requests to module registries during module installation.
+//   - Requests to provider registries during provider installation.
+//
+// The retryCount argument specifies how many times requests from the resulting
+// client should be automatically retried when certain transient errors occur.
+//
+// The timeout argument specifies a deadline for the completion of each
+// request made using the client.
+func NewForRegistryRequests(ctx context.Context, retryCount int, timeout time.Duration) *retryablehttp.Client {
+	// We'll start with the result of New, so that what we return still
+	// honors our general policy for HTTP client behavior.
+	baseClient := New(ctx)
+	baseClient.Timeout = timeout
+
+	// Registry requests historically offered automatic retry on certain
+	// transient errors implemented using the retryablehttp library, so
+	// we'll now deal with that here.
+	retryableClient := retryablehttp.NewClient()
+	retryableClient.HTTPClient = baseClient
+	retryableClient.RetryMax = retryCount
+	retryableClient.RequestLogHook = registryRequestLogHook
+	retryableClient.ErrorHandler = registryMaxRetryErrorHandler
+
+	return retryableClient
+}
+
+func registryRequestLogHook(logger retryablehttp.Logger, req *http.Request, i int) {
+	if i > 0 {
+		logger.Printf("[INFO] Failed request to %s; retrying", req.URL.String())
+	}
+}
+
+func registryMaxRetryErrorHandler(resp *http.Response, err error, numTries int) (*http.Response, error) {
+	// Close the body per library instructions
+	if resp != nil {
+		resp.Body.Close()
+	}
+
+	// Additional error detail: if we have a response, use the status code;
+	// if we have an error, use that; otherwise nothing. We will never have
+	// both response and error.
+	var errMsg string
+	if resp != nil {
+		errMsg = fmt.Sprintf(": %s returned from %s", resp.Status, resp.Request.URL)
+	} else if err != nil {
+		errMsg = fmt.Sprintf(": %s", err)
+	}
+
+	// This function is always called with numTries=RetryMax+1. If we made any
+	// retry attempts, include that in the error message.
+	if numTries > 1 {
+		return resp, fmt.Errorf("request failed after %d attempts%s",
+			numTries, errMsg)
+	}
+	return resp, fmt.Errorf("request failed%s", errMsg)
+}

internal/providercache/installer_test.go

@@ -2740,7 +2740,7 @@ func testServices(t *testing.T) (services *disco.Disco, baseURL string, cleanup
 // of your test in order to shut down the test server.
 func testRegistrySource(t *testing.T) (source *getproviders.RegistrySource, baseURL string, cleanup func()) {
 	services, baseURL, close := testServices(t)
-	source = getproviders.NewRegistrySource(services)
+	source = getproviders.NewRegistrySource(services, nil)
 	return source, baseURL, close
 }
 

internal/registry/client.go

@@ -13,9 +13,7 @@ import (
 	"log"
 	"net/http"
 	"net/url"
-	"os"
 	"path"
-	"strconv"
 	"strings"
 	"time"
 
@@ -26,7 +24,6 @@ import (
 	"go.opentelemetry.io/otel/trace"
 
 	"github.com/opentofu/opentofu/internal/httpclient"
-	"github.com/opentofu/opentofu/internal/logging"
 	"github.com/opentofu/opentofu/internal/registry/regsrc"
 	"github.com/opentofu/opentofu/internal/registry/response"
 	"github.com/opentofu/opentofu/internal/tracing"
@@ -39,35 +36,12 @@ const (
 	xTerraformVersion  = "X-Terraform-Version"
 	modulesServiceID   = "modules.v1"
 	providersServiceID = "providers.v1"
-
-	// registryDiscoveryRetryEnvName is the name of the environment variable that
-	// can be configured to customize number of retries for module and provider
-	// discovery requests with the remote registry.
-	registryDiscoveryRetryEnvName = "TF_REGISTRY_DISCOVERY_RETRY"
-	defaultRetry                  = 1
-
-	// registryClientTimeoutEnvName is the name of the environment variable that
-	// can be configured to customize the timeout duration (seconds) for module
-	// and provider discovery with the remote registry.
-	registryClientTimeoutEnvName = "TF_REGISTRY_CLIENT_TIMEOUT"
-
-	// defaultRequestTimeout is the default timeout duration for requests to the
-	// remote registry.
-	defaultRequestTimeout = 10 * time.Second
 )
 
 var (
 	tfVersion = version.String()
-
-	discoveryRetry int
-	requestTimeout time.Duration
 )
 
-func init() {
-	configureDiscoveryRetry()
-	configureRequestTimeout()
-}
-
 // Client provides methods to query OpenTofu Registries.
 type Client struct {
 	// this is the client to be used for all requests.
@@ -79,30 +53,19 @@ type Client struct {
 }
 
 // NewClient returns a new initialized registry client.
-func NewClient(ctx context.Context, services *disco.Disco, client *http.Client) *Client {
+func NewClient(ctx context.Context, services *disco.Disco, client *retryablehttp.Client) *Client {
 	if services == nil {
 		services = disco.New()
 	}
 
 	if client == nil {
-		client = httpclient.New(ctx)
-		client.Timeout = requestTimeout
+		// The following is a fallback client configuration intended primarily
+		// for our test cases that directly call this function.
+		client = httpclient.NewForRegistryRequests(ctx, 1, 10*time.Second)
 	}
-	retryableClient := retryablehttp.NewClient()
-	retryableClient.HTTPClient = client
-	retryableClient.RetryMax = discoveryRetry
-	retryableClient.RequestLogHook = requestLogHook
-	retryableClient.ErrorHandler = maxRetryErrorHandler
-
-	logOutput := logging.LogOutput()
-	retryableClient.Logger = log.New(logOutput, "", log.Flags())
-
-	services.Transport = retryableClient.HTTPClient.Transport
-
-	services.SetUserAgent(httpclient.OpenTofuUserAgent(version.String()))
 
 	return &Client{
-		client:   retryableClient,
+		client:   client,
 		services: services,
 	}
 }
@@ -310,60 +273,3 @@ func (c *Client) ModuleLocation(ctx context.Context, module *regsrc.Module, vers
 
 	return location, nil
 }
-
-// configureDiscoveryRetry configures the number of retries the registry client
-// will attempt for requests with retryable errors, like 502 status codes
-func configureDiscoveryRetry() {
-	discoveryRetry = defaultRetry
-
-	if v := os.Getenv(registryDiscoveryRetryEnvName); v != "" {
-		retry, err := strconv.Atoi(v)
-		if err == nil && retry > 0 {
-			discoveryRetry = retry
-		}
-	}
-}
-
-func requestLogHook(logger retryablehttp.Logger, req *http.Request, i int) {
-	if i > 0 {
-		logger.Printf("[INFO] Previous request to the remote registry failed, attempting retry.")
-	}
-}
-
-func maxRetryErrorHandler(resp *http.Response, err error, numTries int) (*http.Response, error) {
-	// Close the body per library instructions
-	if resp != nil {
-		resp.Body.Close()
-	}
-
-	// Additional error detail: if we have a response, use the status code;
-	// if we have an error, use that; otherwise nothing. We will never have
-	// both response and error.
-	var errMsg string
-	if resp != nil {
-		errMsg = fmt.Sprintf(": %s returned from %s", resp.Status, resp.Request.URL)
-	} else if err != nil {
-		errMsg = fmt.Sprintf(": %s", err)
-	}
-
-	// This function is always called with numTries=RetryMax+1. If we made any
-	// retry attempts, include that in the error message.
-	if numTries > 1 {
-		return resp, fmt.Errorf("the request failed after %d attempts, please try again later%s",
-			numTries, errMsg)
-	}
-	return resp, fmt.Errorf("the request failed, please try again later%s", errMsg)
-}
-
-// configureRequestTimeout configures the registry client request timeout from
-// environment variables
-func configureRequestTimeout() {
-	requestTimeout = defaultRequestTimeout
-
-	if v := os.Getenv(registryClientTimeoutEnvName); v != "" {
-		timeout, err := strconv.Atoi(v)
-		if err == nil && timeout > 0 {
-			requestTimeout = time.Duration(timeout) * time.Second
-		}
-	}
-}

internal/registry/client_test.go

@@ -16,6 +16,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/hashicorp/go-retryablehttp"
 	version "github.com/hashicorp/go-version"
 	"github.com/hashicorp/terraform-svchost/disco"
 
@@ -26,75 +27,6 @@ import (
 	tfversion "github.com/opentofu/opentofu/version"
 )
 
-func TestConfigureDiscoveryRetry(t *testing.T) {
-	t.Run("default retry", func(t *testing.T) {
-		if discoveryRetry != defaultRetry {
-			t.Fatalf("expected retry %q, got %q", defaultRetry, discoveryRetry)
-		}
-
-		rc := NewClient(t.Context(), nil, nil)
-		if rc.client.RetryMax != defaultRetry {
-			t.Fatalf("expected client retry %q, got %q",
-				defaultRetry, rc.client.RetryMax)
-		}
-	})
-
-	t.Run("configured retry", func(t *testing.T) {
-		defer func() {
-			discoveryRetry = defaultRetry
-		}()
-		t.Setenv(registryDiscoveryRetryEnvName, "2")
-
-		configureDiscoveryRetry()
-		expected := 2
-		if discoveryRetry != expected {
-			t.Fatalf("expected retry %q, got %q",
-				expected, discoveryRetry)
-		}
-
-		rc := NewClient(t.Context(), nil, nil)
-		if rc.client.RetryMax != expected {
-			t.Fatalf("expected client retry %q, got %q",
-				expected, rc.client.RetryMax)
-		}
-	})
-}
-
-func TestConfigureRegistryClientTimeout(t *testing.T) {
-	t.Run("default timeout", func(t *testing.T) {
-		if requestTimeout != defaultRequestTimeout {
-			t.Fatalf("expected timeout %q, got %q",
-				defaultRequestTimeout.String(), requestTimeout.String())
-		}
-
-		rc := NewClient(t.Context(), nil, nil)
-		if rc.client.HTTPClient.Timeout != defaultRequestTimeout {
-			t.Fatalf("expected client timeout %q, got %q",
-				defaultRequestTimeout.String(), rc.client.HTTPClient.Timeout.String())
-		}
-	})
-
-	t.Run("configured timeout", func(t *testing.T) {
-		defer func() {
-			requestTimeout = defaultRequestTimeout
-		}()
-		t.Setenv(registryClientTimeoutEnvName, "20")
-
-		configureRequestTimeout()
-		expected := 20 * time.Second
-		if requestTimeout != expected {
-			t.Fatalf("expected timeout %q, got %q",
-				expected, requestTimeout.String())
-		}
-
-		rc := NewClient(t.Context(), nil, nil)
-		if rc.client.HTTPClient.Timeout != expected {
-			t.Fatalf("expected client timeout %q, got %q",
-				expected, rc.client.HTTPClient.Timeout.String())
-		}
-	})
-}
-
 func TestLookupModuleVersions(t *testing.T) {
 	server := test.Registry()
 	defer server.Close()
@@ -316,20 +248,20 @@ func TestLookupModuleRetryError(t *testing.T) {
 	}
 
 	// verify maxRetryErrorHandler handler returned the error
-	if !strings.Contains(err.Error(), "the request failed after 2 attempts, please try again later") {
+	if !strings.Contains(err.Error(), "request failed after 2 attempts") {
 		t.Fatal("unexpected error, got:", err)
 	}
 }
 
 func TestLookupModuleNoRetryError(t *testing.T) {
-	// Disable retries
-	discoveryRetry = 0
-	defer configureDiscoveryRetry()
-
 	server := test.RegistryRetryableErrorsServer()
 	defer server.Close()
 
-	client := NewClient(t.Context(), test.Disco(server), nil)
+	client := NewClient(
+		t.Context(), test.Disco(server),
+		// Retries are disabled by the second argument to this function
+		httpclient.NewForRegistryRequests(t.Context(), 0, 10*time.Second),
+	)
 
 	src := "example.com/test-versions/name/provider"
 	modsrc, err := regsrc.ParseModuleSource(src)
@@ -345,7 +277,7 @@ func TestLookupModuleNoRetryError(t *testing.T) {
 	}
 
 	// verify maxRetryErrorHandler handler returned the error
-	if !strings.Contains(err.Error(), "the request failed, please try again later") {
+	if !strings.Contains(err.Error(), "request failed:") {
 		t.Fatal("unexpected error, got:", err)
 	}
 }
@@ -371,7 +303,7 @@ func TestLookupModuleNetworkError(t *testing.T) {
 	}
 
 	// verify maxRetryErrorHandler handler returned the correct error
-	if !strings.Contains(err.Error(), "the request failed after 2 attempts, please try again later") {
+	if !strings.Contains(err.Error(), "request failed after 2 attempts") {
 		t.Fatal("unexpected error, got:", err)
 	}
 }
@@ -481,9 +413,9 @@ func TestModuleLocation_readRegistryResponse(t *testing.T) {
 			transport := &testTransport{
 				mockURL: mockServer.URL,
 			}
-			client := NewClient(t.Context(), test.Disco(registryServer), &http.Client{
-				Transport: transport,
-			})
+			httpClient := retryablehttp.NewClient()
+			httpClient.HTTPClient.Transport = transport
+			client := NewClient(t.Context(), test.Disco(registryServer), httpClient)
 
 			mod, err := regsrc.ParseModuleSource(tc.src)
 			if err != nil {