From 1c1df6dc5095e86917833fa7db8d0b2a28a2649a Mon Sep 17 00:00:00 2001
From: Alisdair McDiarmid <alisdair@users.noreply.github.com>
Date: Thu, 19 Mar 2020 10:20:10 -0400
Subject: [PATCH] registry: Fix panic when server is unreachable

Non-HTTP errors previously resulted in a panic due to dereferencing the
resp pointer while it was nil, as part of rendering the error message.
This commit changes the error message formatting to cope with a nil
response, and extends test coverage.

Fixes #24384
---
 registry/client.go             | 20 +++++++++----
 registry/client_test.go        | 55 ++++++++++++++++++++++++++++++++++
 registry/test/mock_registry.go |  2 +-
 3 files changed, 70 insertions(+), 7 deletions(-)

diff --git a/registry/client.go b/registry/client.go
index 5da890c4c2..c518543250 100644
--- a/registry/client.go
+++ b/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 {
-		errMsg = fmt.Sprintf(" %s", err)
+	if resp != nil {
+		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",
-			numTries, resp.StatusCode, errMsg)
+		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: %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
diff --git a/registry/client_test.go b/registry/client_test.go
index cf48fdb9ef..3647c2ccb5 100644
--- a/registry/client_test.go
+++ b/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()
diff --git a/registry/test/mock_registry.go b/registry/test/mock_registry.go
index 924365f2b4..68af193d79 100644
--- a/registry/test/mock_registry.go
+++ b/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"