From 1ae2a99e9e2cea536ca1dd8ec3a2985ca8862288 Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Wed, 1 Sep 2021 10:02:08 -0600 Subject: [PATCH 1/9] fix(compute/metadata): fix retry logic to not panic on error In the case of an error res will be nil. Fixes: #4713 --- compute/metadata/metadata.go | 9 +++++---- compute/metadata/retry.go | 3 +++ compute/metadata/retry_test.go | 7 +++++++ 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/compute/metadata/metadata.go b/compute/metadata/metadata.go index 646950811c2..ae4bd551869 100644 --- a/compute/metadata/metadata.go +++ b/compute/metadata/metadata.go @@ -312,16 +312,17 @@ func (c *Client) getETag(suffix string) (value, etag string, err error) { for { var err error res, err = c.hc.Do(req) - if err == nil { - break + var code int + if res != nil { + code = res.StatusCode } - if delay, shouldRetry := retryer.Retry(res.StatusCode, err); shouldRetry { + if delay, shouldRetry := retryer.Retry(code, err); shouldRetry { if err := gax.Sleep(ctx, delay); err != nil { return "", "", err } continue } - return "", "", err + break } defer res.Body.Close() if res.StatusCode == http.StatusNotFound { diff --git a/compute/metadata/retry.go b/compute/metadata/retry.go index e290452234e..d371789ee4d 100644 --- a/compute/metadata/retry.go +++ b/compute/metadata/retry.go @@ -43,6 +43,9 @@ type metadataRetryer struct { } func (r *metadataRetryer) Retry(status int, err error) (time.Duration, bool) { + if status == 200 { + return 0, false + } retryOk := shouldRetry(status, err) if !retryOk { return 0, false diff --git a/compute/metadata/retry_test.go b/compute/metadata/retry_test.go index 5908ea07933..c96ef054ae9 100644 --- a/compute/metadata/retry_test.go +++ b/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 { From 363dcf74ad11a3df3f15faebc385a4b2c804bf8b Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Wed, 1 Sep 2021 10:32:07 -0600 Subject: [PATCH 2/9] pr feedback and another test --- compute/metadata/metadata_test.go | 69 +++++++++++++++++++++++++++++++ compute/metadata/retry.go | 3 +- 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/compute/metadata/metadata_test.go b/compute/metadata/metadata_test.go index f779c18b719..9870e1fffcc 100644 --- a/compute/metadata/metadata_test.go +++ b/compute/metadata/metadata_test.go @@ -16,10 +16,12 @@ package metadata import ( "bytes" + "io" "io/ioutil" "log" "net/http" "os" + "strings" "sync" "testing" ) @@ -100,6 +102,54 @@ func TestGet_LeadingSlash(t *testing.T) { } } +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.failedAttempts, tt.timesToFail) + } + if s != tt.response { + t.Fatalf("c.Get() = %q, want %q", s, tt.response) + } + }) + } +} + type captureTransport struct { url string } @@ -127,3 +177,22 @@ func (r *rrt) RoundTrip(req *http.Request) (*http.Response, error) { r.gotUserAgent = req.Header.Get("User-Agent") return &http.Response{Body: ioutil.NopCloser(bytes.NewReader(nil))}, nil } + +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++ + return &http.Response{StatusCode: r.failCode}, r.failErr + } + return &http.Response{StatusCode: http.StatusOK, Body: ioutil.NopCloser(strings.NewReader(r.response))}, nil +} diff --git a/compute/metadata/retry.go b/compute/metadata/retry.go index d371789ee4d..b1d1e7981a1 100644 --- a/compute/metadata/retry.go +++ b/compute/metadata/retry.go @@ -16,6 +16,7 @@ package metadata import ( "io" + "net/http" "time" "github.com/googleapis/gax-go/v2" @@ -43,7 +44,7 @@ type metadataRetryer struct { } func (r *metadataRetryer) Retry(status int, err error) (time.Duration, bool) { - if status == 200 { + if status == http.StatusOK { return 0, false } retryOk := shouldRetry(status, err) From a1b22d0a8b78f24ff42d07e0049bf9224d6562c0 Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Wed, 1 Sep 2021 12:50:59 -0600 Subject: [PATCH 3/9] handler error post retry --- compute/metadata/metadata.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/compute/metadata/metadata.go b/compute/metadata/metadata.go index ae4bd551869..471adfdb1de 100644 --- a/compute/metadata/metadata.go +++ b/compute/metadata/metadata.go @@ -308,15 +308,15 @@ 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) + res, reqErr = c.hc.Do(req) var code int if res != nil { code = res.StatusCode } - if delay, shouldRetry := retryer.Retry(code, err); shouldRetry { + if delay, shouldRetry := retryer.Retry(code, reqErr); shouldRetry { if err := gax.Sleep(ctx, delay); err != nil { return "", "", err } @@ -324,6 +324,9 @@ func (c *Client) getETag(suffix string) (value, etag string, err error) { } break } + if reqErr != nil { + return "", "", nil + } defer res.Body.Close() if res.StatusCode == http.StatusNotFound { return "", "", NotDefinedError(suffix) From 2cb70930e0487c433cb089ffaa07249c7a709ef8 Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Wed, 1 Sep 2021 12:52:47 -0600 Subject: [PATCH 4/9] fix test --- compute/metadata/metadata_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/compute/metadata/metadata_test.go b/compute/metadata/metadata_test.go index 9870e1fffcc..bbbff00c49b 100644 --- a/compute/metadata/metadata_test.go +++ b/compute/metadata/metadata_test.go @@ -192,7 +192,10 @@ func (r *failingTransport) RoundTrip(req *http.Request) (*http.Response, error) r.called++ if r.failedAttempts < r.timesToFail { r.failedAttempts++ - return &http.Response{StatusCode: r.failCode}, r.failErr + 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 } From 031ffa9d29cf2ea992a35932f98fec688bca5edd Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Wed, 1 Sep 2021 13:42:54 -0600 Subject: [PATCH 5/9] refactor test to run with different error --- compute/metadata/metadata_linux_test.go | 96 +++++++++++++++++++++++++ compute/metadata/metadata_test.go | 72 ------------------- 2 files changed, 96 insertions(+), 72 deletions(-) create mode 100644 compute/metadata/metadata_linux_test.go diff --git a/compute/metadata/metadata_linux_test.go b/compute/metadata/metadata_linux_test.go new file mode 100644 index 00000000000..a5b78c2710e --- /dev/null +++ b/compute/metadata/metadata_linux_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 linux +// +build linux + +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 syscall.ECONNRESET once", + response: "test", + failErr: syscall.ECONNRESET, + 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 +} diff --git a/compute/metadata/metadata_test.go b/compute/metadata/metadata_test.go index bbbff00c49b..f779c18b719 100644 --- a/compute/metadata/metadata_test.go +++ b/compute/metadata/metadata_test.go @@ -16,12 +16,10 @@ package metadata import ( "bytes" - "io" "io/ioutil" "log" "net/http" "os" - "strings" "sync" "testing" ) @@ -102,54 +100,6 @@ func TestGet_LeadingSlash(t *testing.T) { } } -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.failedAttempts, tt.timesToFail) - } - if s != tt.response { - t.Fatalf("c.Get() = %q, want %q", s, tt.response) - } - }) - } -} - type captureTransport struct { url string } @@ -177,25 +127,3 @@ func (r *rrt) RoundTrip(req *http.Request) (*http.Response, error) { r.gotUserAgent = req.Header.Get("User-Agent") return &http.Response{Body: ioutil.NopCloser(bytes.NewReader(nil))}, nil } - -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 -} From c8510e10807afce589b9f8344c80113d52e4d3f9 Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Wed, 1 Sep 2021 15:22:10 -0600 Subject: [PATCH 6/9] fmt --- compute/metadata/metadata_linux_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compute/metadata/metadata_linux_test.go b/compute/metadata/metadata_linux_test.go index a5b78c2710e..20fa3e3b5e1 100644 --- a/compute/metadata/metadata_linux_test.go +++ b/compute/metadata/metadata_linux_test.go @@ -44,9 +44,9 @@ func TestRetry(t *testing.T) { timesToFail: 1, }, { - name: "retry syscall.ECONNRESET once", + name: "retry io.ErrUnexpectedEOF once", response: "test", - failErr: syscall.ECONNRESET, + failErr: io.ErrUnexpectedEOF, timesToFail: 1, }, } From d4a49f9a2e999a36d22ff52ad4909f692f6fef50 Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Wed, 1 Sep 2021 16:00:37 -0600 Subject: [PATCH 7/9] actually update test --- compute/metadata/metadata_linux_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compute/metadata/metadata_linux_test.go b/compute/metadata/metadata_linux_test.go index 20fa3e3b5e1..a5b78c2710e 100644 --- a/compute/metadata/metadata_linux_test.go +++ b/compute/metadata/metadata_linux_test.go @@ -44,9 +44,9 @@ func TestRetry(t *testing.T) { timesToFail: 1, }, { - name: "retry io.ErrUnexpectedEOF once", + name: "retry syscall.ECONNRESET once", response: "test", - failErr: io.ErrUnexpectedEOF, + failErr: syscall.ECONNRESET, timesToFail: 1, }, } From 4b1e94b0b1b7840481533fae4fd3659663aa4a97 Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Thu, 2 Sep 2021 08:36:13 -0600 Subject: [PATCH 8/9] fmt --- compute/metadata/metadata_linux_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compute/metadata/metadata_linux_test.go b/compute/metadata/metadata_linux_test.go index a5b78c2710e..c9d48c6b3f0 100644 --- a/compute/metadata/metadata_linux_test.go +++ b/compute/metadata/metadata_linux_test.go @@ -18,10 +18,10 @@ package metadata import ( - "io" "io/ioutil" "net/http" "strings" + "syscall" "testing" ) From 564f86bd0d7d40ac3af9b859b9939e234c0f6e1e Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Thu, 2 Sep 2021 09:38:58 -0600 Subject: [PATCH 9/9] fix test --- .../{metadata_linux_test.go => metadata_go113_test.go} | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) rename compute/metadata/{metadata_linux_test.go => metadata_go113_test.go} (94%) diff --git a/compute/metadata/metadata_linux_test.go b/compute/metadata/metadata_go113_test.go similarity index 94% rename from compute/metadata/metadata_linux_test.go rename to compute/metadata/metadata_go113_test.go index c9d48c6b3f0..94e2594c1c9 100644 --- a/compute/metadata/metadata_linux_test.go +++ b/compute/metadata/metadata_go113_test.go @@ -12,16 +12,16 @@ // See the License for the specific language governing permissions and // limitations under the License. -//go:build linux -// +build linux +//go:build go1.13 +// +build go1.13 package metadata import ( + "io" "io/ioutil" "net/http" "strings" - "syscall" "testing" ) @@ -44,9 +44,9 @@ func TestRetry(t *testing.T) { timesToFail: 1, }, { - name: "retry syscall.ECONNRESET once", + name: "retry io.ErrUnexpectedEOF once", response: "test", - failErr: syscall.ECONNRESET, + failErr: io.ErrUnexpectedEOF, timesToFail: 1, }, }