fix(compute/metadata): fix retry logic to not panic on error (#4714)

In the case of an error res will be nil. Also, we should not break on nil err
as it might be eligible for a retry from the status code.

Fixes: #4713

Author: codyoss

compute/metadata/metadata.go

@@ -308,20 +308,24 @@ func (c *Client) getETag(suffix string) (value, etag string, err error) {
 	req.Header.Set("Metadata-Flavor", "Google")
 	req.Header.Set("User-Agent", userAgent)
 	var res *http.Response
+	var reqErr error
 	retryer := newRetryer()
 	for {
-		var err error
-		res, err = c.hc.Do(req)
-		if err == nil {
-			break
+		res, reqErr = c.hc.Do(req)
+		var code int
+		if res != nil {
+			code = res.StatusCode
 		}
-		if delay, shouldRetry := retryer.Retry(res.StatusCode, err); shouldRetry {
+		if delay, shouldRetry := retryer.Retry(code, reqErr); shouldRetry {
 			if err := gax.Sleep(ctx, delay); err != nil {
 				return "", "", err
 			}
 			continue
 		}
-		return "", "", err
+		break
+	}
+	if reqErr != nil {
+		return "", "", nil
 	}
 	defer res.Body.Close()
 	if res.StatusCode == http.StatusNotFound {

compute/metadata/metadata_go113_test.go

@@ -0,0 +1,96 @@
+// Copyright 2016 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//go:build go1.13
+// +build go1.13
+
+package metadata
+
+import (
+	"io"
+	"io/ioutil"
+	"net/http"
+	"strings"
+	"testing"
+)
+
+func TestRetry(t *testing.T) {
+	tests := []struct {
+		name        string
+		timesToFail int
+		failCode    int
+		failErr     error
+		response    string
+	}{
+		{
+			name:     "no retries",
+			response: "test",
+		},
+		{
+			name:        "retry 500 once",
+			response:    "test",
+			failCode:    500,
+			timesToFail: 1,
+		},
+		{
+			name:        "retry io.ErrUnexpectedEOF once",
+			response:    "test",
+			failErr:     io.ErrUnexpectedEOF,
+			timesToFail: 1,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ft := &failingTransport{
+				timesToFail: tt.timesToFail,
+				failCode:    tt.failCode,
+				failErr:     tt.failErr,
+				response:    tt.response,
+			}
+			c := NewClient(&http.Client{Transport: ft})
+			s, err := c.Get("")
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if ft.called != ft.failedAttempts+1 {
+				t.Fatalf("failed %d times, want %d", ft.called, ft.failedAttempts+1)
+			}
+			if s != tt.response {
+				t.Fatalf("c.Get() = %q, want %q", s, tt.response)
+			}
+		})
+	}
+}
+
+type failingTransport struct {
+	timesToFail int
+	failCode    int
+	failErr     error
+	response    string
+
+	failedAttempts int
+	called         int
+}
+
+func (r *failingTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	r.called++
+	if r.failedAttempts < r.timesToFail {
+		r.failedAttempts++
+		if r.failErr != nil {
+			return nil, r.failErr
+		}
+		return &http.Response{StatusCode: r.failCode}, nil
+	}
+	return &http.Response{StatusCode: http.StatusOK, Body: ioutil.NopCloser(strings.NewReader(r.response))}, nil
+}

compute/metadata/retry.go

@@ -16,6 +16,7 @@ package metadata
 
 import (
 	"io"
+	"net/http"
 	"time"
 
 	"github.com/googleapis/gax-go/v2"
@@ -43,6 +44,9 @@ type metadataRetryer struct {
 }
 
 func (r *metadataRetryer) Retry(status int, err error) (time.Duration, bool) {
+	if status == http.StatusOK {
+		return 0, false
+	}
 	retryOk := shouldRetry(status, err)
 	if !retryOk {
 		return 0, false

compute/metadata/retry_test.go

@@ -87,6 +87,13 @@ func TestMetadataRetryer(t *testing.T) {
 			wantDelay:       0,
 			wantShouldRetry: false,
 		},
+		{
+			name:            "don't retry 200",
+			code:            200,
+			err:             nil,
+			wantDelay:       0,
+			wantShouldRetry: false,
+		},
 	}
 
 	for _, tc := range tests {