Merge pull request #24411 from hashicorp/alisdair/registry-retry-panic

registry: Fix panic when server is unreachable

registry/client.go

@@ -14,7 +14,7 @@ import (
 	"time"
 
 	"github.com/hashicorp/go-retryablehttp"
-	"github.com/hashicorp/terraform-svchost"
+	svchost "github.com/hashicorp/terraform-svchost"
 	"github.com/hashicorp/terraform-svchost/disco"
 	"github.com/hashicorp/terraform/helper/logging"
 	"github.com/hashicorp/terraform/httpclient"
@@ -413,15 +413,23 @@ func maxRetryErrorHandler(resp *http.Response, err error, numTries int) (*http.R
 		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 err != nil {
+	if resp != nil {
-		errMsg = fmt.Sprintf(" %s", err)
+		errMsg = fmt.Sprintf(": %d", resp.StatusCode)
+	} 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: %d%s",
+		return resp, fmt.Errorf("the request failed after %d attempts, please try again later%s",
-			numTries, resp.StatusCode, errMsg)
+			numTries, errMsg)
 	}
-	return resp, fmt.Errorf("the request failed, please try again later: %d%s", resp.StatusCode, errMsg)
+	return resp, fmt.Errorf("the request failed, please try again later%s", errMsg)
 }
 
 // configureRequestTimeout configures the registry client request timeout from

registry/client_test.go

@@ -314,6 +314,61 @@ func TestLookupModuleRetryError(t *testing.T) {
 	}
 }
 
+func TestLookupModuleNoRetryError(t *testing.T) {
+	// Disable retries
+	discoveryRetry = 0
+	defer configureDiscoveryRetry()
+
+	server := test.RegistryRetryableErrorsServer()
+	defer server.Close()
+
+	client := NewClient(test.Disco(server), nil)
+
+	src := "example.com/test-versions/name/provider"
+	modsrc, err := regsrc.ParseModuleSource(src)
+	if err != nil {
+		t.Fatal(err)
+	}
+	resp, err := client.ModuleVersions(modsrc)
+	if err == nil {
+		t.Fatal("expected request to fail", err)
+	}
+	if resp != nil {
+		t.Fatal("unexpected response", *resp)
+	}
+
+	// verify maxRetryErrorHandler handler returned the error
+	if !strings.Contains(err.Error(), "the request failed, please try again later") {
+		t.Fatal("unexpected error, got:", err)
+	}
+}
+
+func TestLookupModuleNetworkError(t *testing.T) {
+	server := test.RegistryRetryableErrorsServer()
+	client := NewClient(test.Disco(server), nil)
+
+	// Shut down the server to simulate network failure
+	server.Close()
+
+	src := "example.com/test-versions/name/provider"
+	modsrc, err := regsrc.ParseModuleSource(src)
+	if err != nil {
+		t.Fatal(err)
+	}
+	resp, err := client.ModuleVersions(modsrc)
+	if err == nil {
+		t.Fatal("expected request to fail", err)
+	}
+	if resp != nil {
+		t.Fatal("unexpected response", *resp)
+	}
+
+	// verify maxRetryErrorHandler handler returned the correct error
+	if !strings.Contains(err.Error(), "the request failed after 2 attempts, please try again later") {
+		t.Fatal("unexpected error, got:", err)
+	}
+}
+
 func TestLookupProviderVersions(t *testing.T) {
 	server := test.Registry()
 	defer server.Close()

registry/test/mock_registry.go

@@ -12,7 +12,7 @@ import (
 	"strings"
 
 	version "github.com/hashicorp/go-version"
-	"github.com/hashicorp/terraform-svchost"
+	svchost "github.com/hashicorp/terraform-svchost"
 	"github.com/hashicorp/terraform-svchost/auth"
 	"github.com/hashicorp/terraform-svchost/disco"
 	"github.com/hashicorp/terraform/httpclient"