Add download_retry_count for direct and network_mirror in CLI configuration (#3368)

Signed-off-by: Andrei Ciobanu <andrei.ciobanu@opentofu.org>

cmd/tofu/commands.go

@@ -123,7 +123,10 @@ func initCommands(
 
 		AllowExperimentalFeatures: experimentsAreAllowed(),
 
-		ProviderSourceLocationConfig: providerSourceLocationConfig(),
+		// ProviderSourceLocationConfig is used for some commands that do not make
+		// use of the OpenTofu configuration files. Therefore, there is no way to configure
+		// the retries from other places than env vars.
+		ProviderSourceLocationConfig: providerSourceLocationConfigFromEnv(),
 	}
 
 	// The command list is included in the tofu -help

cmd/tofu/provider_source.go

@@ -12,7 +12,6 @@ import (
 	"net/url"
 	"os"
 	"path/filepath"
-	"strconv"
 
 	"github.com/apparentlymart/go-userdirs/userdirs"
 	"github.com/opentofu/svchost/disco"
@@ -61,7 +60,7 @@ func explicitProviderSource(
 
 	log.Printf("[DEBUG] Explicit provider installation configuration is set")
 	for _, methodConfig := range config.Methods {
-		source, moreDiags := providerSourceForCLIConfigLocation(ctx, methodConfig.Location, registryClientConfig, services, getOCICredsPolicy)
+		source, moreDiags := providerSourceForCLIConfigLocation(ctx, methodConfig.Location, methodConfig.Retries, registryClientConfig, services, getOCICredsPolicy)
 		diags = diags.Append(moreDiags)
 		if moreDiags.HasErrors() {
 			continue
@@ -208,7 +207,7 @@ func implicitProviderSource(
 	// local copy will take precedence.
 	searchRules = append(searchRules, getproviders.MultiSourceSelector{
 		Source: getproviders.NewMemoizeSource(
-			getproviders.NewRegistrySource(ctx, services, newRegistryHTTPClient(ctx, registryClientConfig), providerSourceLocationConfig()),
+			getproviders.NewRegistrySource(ctx, services, newRegistryHTTPClient(ctx, registryClientConfig), providerSourceLocationConfigFromEnv()),
 		),
 		Exclude: directExcluded,
 	})
@@ -219,13 +218,14 @@ func implicitProviderSource(
 func providerSourceForCLIConfigLocation(
 	ctx context.Context,
 	loc cliconfig.ProviderInstallationLocation,
+	locationRetries cliconfig.ProviderInstallationMethodRetries,
 	registryClientConfig *cliconfig.RegistryProtocolsConfig,
 	services *disco.Disco,
 	makeOCICredsPolicy ociCredsPolicyBuilder,
 ) (getproviders.Source, tfdiags.Diagnostics) {
 	if loc == cliconfig.ProviderInstallationDirect {
 		return getproviders.NewMemoizeSource(
-			getproviders.NewRegistrySource(ctx, services, newRegistryHTTPClient(ctx, registryClientConfig), providerSourceLocationConfig()),
+			getproviders.NewRegistrySource(ctx, services, newRegistryHTTPClient(ctx, registryClientConfig), providerSourceLocationConfig(locationRetries)),
 		), nil
 	}
 
@@ -259,7 +259,7 @@ func providerSourceForCLIConfigLocation(
 		// this client is not suitable for the HTTP mirror source, so we
 		// don't use this client directly.
 		httpTimeout := newRegistryHTTPClient(ctx, registryClientConfig).HTTPClient.Timeout
-		return getproviders.NewHTTPMirrorSource(ctx, url, services.CredentialsSource(), httpTimeout, providerSourceLocationConfig()), nil
+		return getproviders.NewHTTPMirrorSource(ctx, url, services.CredentialsSource(), httpTimeout, providerSourceLocationConfig(locationRetries)), nil
 
 	case cliconfig.ProviderInstallationOCIMirror:
 		mappingFunc := loc.RepositoryMapping
@@ -300,33 +300,28 @@ func providerDevOverrides(configs []*cliconfig.ProviderInstallation) map[addrs.P
 	return configs[0].DevOverrides
 }
 
-const (
-	// providerDownloadRetryCountEnvName is the environment variable name used to customize
-	// the HTTP retry count for module downloads.
-	providerDownloadRetryCountEnvName = "TF_PROVIDER_DOWNLOAD_RETRY"
-
-	providerDownloadDefaultRetry = 2
-)
-
-// providerDownloadRetry will attempt for requests with retryable errors, like 502 status codes
-func providerDownloadRetry() int {
-	res := providerDownloadDefaultRetry
-	if v := os.Getenv(providerDownloadRetryCountEnvName); v != "" {
-		retry, err := strconv.Atoi(v)
-		if err == nil && retry > 0 {
-			res = retry
-		}
-	}
-	return res
-}
-
 // providerSourceLocationConfig is meant to build a global configuration for the
 // remote locations to download a provider from. This is built out of the
 // TF_PROVIDER_DOWNLOAD_RETRY env variable and is meant to be passed through
 // [getproviders.Source] all the way down to the [getproviders.PackageLocation]
 // to be able to tweak the configurations of the http clients used there.
-func providerSourceLocationConfig() getproviders.LocationConfig {
+func providerSourceLocationConfig(locationRetries cliconfig.ProviderInstallationMethodRetries) getproviders.LocationConfig {
+	// If there is no configuration for the retries in .tofurc, get the one from env variable
+	retries, configured := locationRetries()
+	if !configured {
+		retries = cliconfig.ProviderDownloadRetries()
+	}
 	return getproviders.LocationConfig{
-		ProviderDownloadRetries: providerDownloadRetry(),
+		ProviderDownloadRetries: retries,
+	}
+}
+
+// providerSourceLocationConfigFromEnv is similar to providerSourceLocationConfig but does not
+// take into account the information from the configuration. This is like so because for some
+// commands, there is no specific tofurc configuration for the retry, so we want to use the
+// env variable if defined and if not, its default.
+func providerSourceLocationConfigFromEnv() getproviders.LocationConfig {
+	return getproviders.LocationConfig{
+		ProviderDownloadRetries: cliconfig.ProviderDownloadRetries(),
 	}
 }

cmd/tofu/provider_source_test.go

@@ -7,8 +7,11 @@ package main
 
 import (
 	"context"
+	"net/http"
+	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 	"time"
 
@@ -160,6 +163,121 @@ func TestProviderSource(t *testing.T) {
 	}
 }
 
+// TestInitProviderSourceForCLIConfigLocationWithRetries tests how the retries are handled and passed over to the
+// [getproviders.PackageLocation].
+// Normally, this should have been an e2e test, but there is no easy way to mock a `direct` registry
+// to be able to test that the retries actually work properly.
+//
+// This test contains no tests for [cliconfig.ProviderInstallationNetworkMirror] on purpose since
+// there is no way to create [getproviders.HTTPMirrorSource]
+// with an on-the-fly generated TLS certificate since it's required by the
+// [getproviders.HTTPMirrorSource] inner working.
+// Instead, the retries functionality for `network_mirror` is tested into an e2e called
+// "TestProviderNetworkMirrorRetries" since that way we were able to provide the TLS certificate
+// into the child process instead.
+func TestInitProviderSourceForCLIConfigLocationWithRetries(t *testing.T) {
+	registryHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/providers/v1/test/exists/0.0.0/download/foo_os/bar_arch":
+			_, _ = w.Write([]byte(`{"os":"foo_os","arch":"bar_arch","download_url":"/providers/v1/test/exists_0.0.0.zip","shasum":"4cbc33c22abdebe3a3679666d4052ec95c40bd8904a9458f90cf934363a14cc7"}`))
+		case "/providers/v1/test/exists_0.0.0.zip":
+			w.WriteHeader(http.StatusInternalServerError)
+			return
+		default:
+			w.WriteHeader(http.StatusNotFound)
+		}
+	})
+	cases := map[string]struct {
+		methodType     cliconfig.ProviderInstallationLocation
+		tofurcRetries  cliconfig.ProviderInstallationMethodRetries
+		envVars        map[string]string
+		expectedErrMsg string
+	}{
+		"no tofurc.direct.download_retry_count configured, no TF_PROVIDER_DOWNLOAD_RETRY, default TF_PROVIDER_DOWNLOAD_RETRY used": {
+			methodType: cliconfig.ProviderInstallationDirect,
+			tofurcRetries: func() (int, bool) {
+				return 0, false
+			},
+			envVars:        nil,
+			expectedErrMsg: "/providers/v1/test/exists_0.0.0.zip giving up after 3 attempt(s)",
+		},
+		"no tofurc.direct.download_retry_count, TF_PROVIDER_DOWNLOAD_RETRY defined, TF_PROVIDER_DOWNLOAD_RETRY used": {
+			methodType: cliconfig.ProviderInstallationDirect,
+			tofurcRetries: func() (int, bool) {
+				return 0, false
+			},
+			envVars: map[string]string{
+				"TF_PROVIDER_DOWNLOAD_RETRY": "1",
+			},
+			expectedErrMsg: "/providers/v1/test/exists_0.0.0.zip giving up after 2 attempt(s)",
+		},
+		"defined tofurc.direct.download_retry_count as 0, no TF_PROVIDER_DOWNLOAD_RETRY, tofurc used": {
+			methodType: cliconfig.ProviderInstallationDirect,
+			tofurcRetries: func() (int, bool) {
+				return 0, true
+			},
+			envVars:        nil,
+			expectedErrMsg: "/providers/v1/test/exists_0.0.0.zip giving up after 1 attempt(s)",
+		},
+		"defined tofurc.direct.download_retry_count as 1, no TF_PROVIDER_DOWNLOAD_RETRY, tofurc used": {
+			methodType: cliconfig.ProviderInstallationDirect,
+			tofurcRetries: func() (int, bool) {
+				return 1, true
+			},
+			envVars:        nil,
+			expectedErrMsg: "/providers/v1/test/exists_0.0.0.zip giving up after 2 attempt(s)",
+		},
+		"defined tofurc.direct.download_retry_count as 1, TF_PROVIDER_DOWNLOAD_RETRY defined as 2, tofurc used": {
+			methodType: cliconfig.ProviderInstallationDirect,
+			tofurcRetries: func() (int, bool) {
+				return 1, true
+			},
+			envVars: map[string]string{
+				"TF_PROVIDER_DOWNLOAD_RETRY": "2",
+			},
+			expectedErrMsg: "/providers/v1/test/exists_0.0.0.zip giving up after 2 attempt(s)",
+		},
+	}
+	for name, tt := range cases {
+		t.Run(name, func(t *testing.T) {
+			methodType := tt.methodType
+			retries := tt.tofurcRetries // This is what simulates the configuration of .tofurc
+			for k, v := range tt.envVars {
+				t.Setenv(k, v)
+			}
+
+			server := httptest.NewServer(registryHandler)
+			defer server.Close()
+			disco := disco.New(disco.WithHTTPClient(server.Client()))
+			disco.ForceHostServices("local.testing", map[string]any{
+				"providers.v1": server.URL + "/providers/v1/",
+			})
+
+			providerSrc, diags := providerSourceForCLIConfigLocation(t.Context(), methodType, retries, &cliconfig.RegistryProtocolsConfig{}, disco, nil)
+			if diags.HasErrors() {
+				t.Fatalf("unexpected error creating the provider source: %s", diags)
+			}
+			provider := addrs.MustParseProviderSourceString("local.testing/test/exists")
+			platform := getproviders.Platform{OS: "foo_os", Arch: "bar_arch"}
+			packageMeta, err := providerSrc.PackageMeta(t.Context(), provider, getproviders.UnspecifiedVersion, platform)
+			if err != nil {
+				t.Fatalf("unexpected error getting the package meta information: %s", err)
+			}
+			// We nullify the authentication since the meaning of this test is not to
+			packageMeta.Authentication = nil
+
+			instDir := t.TempDir()
+			_, err = packageMeta.Location.InstallProviderPackage(t.Context(), packageMeta, instDir, nil)
+			if err == nil {
+				t.Fatalf("expected to get an error from the installation of the provider but got nothing")
+			}
+			if contains := tt.expectedErrMsg; !strings.Contains(err.Error(), contains) {
+				t.Fatalf("expected the error from the installation to contain %q but it didn't: %s", contains, err)
+			}
+		})
+	}
+}
+
 func TestConfigureProviderDownloadRetry(t *testing.T) {
 	tests := []struct {
 		name    string
@@ -170,7 +288,7 @@ func TestConfigureProviderDownloadRetry(t *testing.T) {
 		{
 			name: "when no TF_PROVIDER_DOWNLOAD_RETRY env var, default retry attempts used for provider download",
 			expectedConfig: getproviders.LocationConfig{
-				ProviderDownloadRetries: providerDownloadDefaultRetry,
+				ProviderDownloadRetries: 2,
 			},
 		},
 		{
@@ -191,7 +309,9 @@ func TestConfigureProviderDownloadRetry(t *testing.T) {
 			}
 
 			// Call the function under test
-			got := providerSourceLocationConfig()
+			got := providerSourceLocationConfig(func() (int, bool) {
+				return 0, false
+			})
 
 			if diff := cmp.Diff(tt.expectedConfig, got); diff != "" {
 				t.Fatalf("expected no diff. got:\n%s", diff)