From 7ca74cd3e07ff9eec6eda610b646a7eda2dc8723 Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Thu, 19 Aug 2021 11:11:42 -0600 Subject: [PATCH 1/4] feat(compute/metadata): retry error when talking to metadata service This was reported internally to be causing issues. This package is used by some of our auth flows so it should be good to make this package more resilient to transient failures. Implmentation inspired by what we do for some of our http based services. Because this package is currently not context aware I needed to add attempts so retrying does not happen forever. Five attempts was arbitrarily chosen. Fixes: #4642 --- compute/metadata/metadata.go | 20 ++++- compute/metadata/retry.go | 73 ++++++++++++++++++ compute/metadata/retry_linux.go | 25 +++++++ compute/metadata/retry_linux_test.go | 39 ++++++++++ compute/metadata/retry_test.go | 108 +++++++++++++++++++++++++++ 5 files changed, 262 insertions(+), 3 deletions(-) create mode 100644 compute/metadata/retry.go create mode 100644 compute/metadata/retry_linux.go create mode 100644 compute/metadata/retry_linux_test.go create mode 100644 compute/metadata/retry_test.go diff --git a/compute/metadata/metadata.go b/compute/metadata/metadata.go index 545bd9d379ce..db775def65c7 100644 --- a/compute/metadata/metadata.go +++ b/compute/metadata/metadata.go @@ -32,6 +32,8 @@ import ( "strings" "sync" "time" + + "github.com/googleapis/gax-go/v2" ) const ( @@ -304,9 +306,21 @@ func (c *Client) getETag(suffix string) (value, etag string, err error) { } req.Header.Set("Metadata-Flavor", "Google") req.Header.Set("User-Agent", userAgent) - res, err := c.hc.Do(req) - if err != nil { - return "", "", err + var res *http.Response + retryer := newRetryer() + for { + var err error + res, err = c.hc.Do(req) + if err != nil { + if delay, shouldRetry :=retryer.Retry(res.StatusCode, err); shouldRetry { + if err := gax.Sleep(context.Background(), 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 new file mode 100644 index 000000000000..d7374f7a4a6b --- /dev/null +++ b/compute/metadata/retry.go @@ -0,0 +1,73 @@ +// Copyright 2021 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. + +package metadata + +import ( + "io" + "time" + + "github.com/googleapis/gax-go/v2" +) + +var ( + syscallRetryable = func(err error) bool { return false } +) + +func newRetryer() *metadataRetryer { + return &metadataRetryer{bo: &gax.Backoff{Initial: 100 * time.Millisecond}} +} + +type backoff interface { + Pause() time.Duration +} + +type metadataRetryer struct { + bo backoff + attempts int +} + +func (r *metadataRetryer) Retry(status int, err error) (time.Duration, bool) { + retryOk := shouldRetry(status, err) + if !retryOk { + return 0, false + } + r.attempts++ + if r.attempts == 6 { + return 0, false + } + return r.bo.Pause(), true +} + +func shouldRetry(status int, err error) bool { + if 500 <= status && status <= 599 { + return true + } + if err == io.ErrUnexpectedEOF { + return true + } + // Transient network errors should be retried. + if syscallRetryable(err) { + return true + } + if err, ok := err.(interface{ Temporary() bool }); ok { + if err.Temporary() { + return true + } + } + if err, ok := err.(interface{ Unwrap() error }); ok { + return shouldRetry(status, err.Unwrap()) + } + return false +} diff --git a/compute/metadata/retry_linux.go b/compute/metadata/retry_linux.go new file mode 100644 index 000000000000..6e90cb85ba75 --- /dev/null +++ b/compute/metadata/retry_linux.go @@ -0,0 +1,25 @@ +// Copyright 2021 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. + +// +build linux + +package metadata + +import "syscall" + +func init() { + // Initialize syscallRetryable to return true on transient socket-level + // errors. These errors are specific to Linux. + syscallRetryable = func(err error) bool { return err == syscall.ECONNRESET || err == syscall.ECONNREFUSED } +} diff --git a/compute/metadata/retry_linux_test.go b/compute/metadata/retry_linux_test.go new file mode 100644 index 000000000000..a2b42a675512 --- /dev/null +++ b/compute/metadata/retry_linux_test.go @@ -0,0 +1,39 @@ +// Copyright 2021 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. + +// +build linux + +package metadata + +import ( + "syscall" + "testing" +) + +func TestMetadataRetryerLinux(t *testing.T) { + retryer := metadataRetryer{bo: constantBackoff{}} + + t.Run("retry on syscall.ECONNRESET", func(t *testing.T) { + delay, shouldRetry := retryer.Retry(400, syscall.ECONNRESET) + if !shouldRetry { + t.Fatal("retryer.Retry(500, nil) = false, want true") + } + }) + t.Run("retry on syscall.ECONNREFUSED", func(t *testing.T) { + delay, shouldRetry := retryer.Retry(400, syscall.ECONNREFUSED) + if !shouldRetry { + t.Fatal("retryer.Retry(500, nil) = false, want true") + } + }) +} diff --git a/compute/metadata/retry_test.go b/compute/metadata/retry_test.go new file mode 100644 index 000000000000..5d0a2f2405f0 --- /dev/null +++ b/compute/metadata/retry_test.go @@ -0,0 +1,108 @@ +// Copyright 2021 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. + +package metadata + +import ( + "io" + "testing" + "time" +) + +type constantBackoff struct{} + +func (b constantBackoff) Pause() time.Duration { return 100 } + +type errTemp struct{} + +func (e errTemp) Error() string { return "temporary error" } + +func (e errTemp) Temporary() bool { return true } + +type errWrapped struct { + e error +} + +func (e errWrapped) Error() string { return "unwrap me to get more context" } + +func (e errWrapped) Unwrap() error { return e.e } + +func TestMetadataRetryer(t *testing.T) { + t.Run("retry on 500", func(t *testing.T) { + retryer := metadataRetryer{bo: constantBackoff{}} + delay, shouldRetry := retryer.Retry(500, nil) + if !shouldRetry { + t.Fatal("retryer.Retry(500, nil) = false, want true") + } + if delay != 100 { + t.Fatalf("retryer.Retry(500, nil) = %d, want 100", delay) + } + }) + t.Run("don't retry 400", func(t *testing.T) { + retryer := metadataRetryer{bo: constantBackoff{}} + delay, shouldRetry := retryer.Retry(400, io.EOF) + if shouldRetry { + t.Fatal("retryer.Retry(400, io.EOF) = true, want false") + } + if delay != 0 { + t.Fatalf("retryer.Retry(400, io.EOF) = %d, want 0", delay) + } + }) + t.Run("retry on io.ErrUnexpectedEOF", func(t *testing.T) { + retryer := metadataRetryer{bo: constantBackoff{}} + _, shouldRetry := retryer.Retry(400, io.ErrUnexpectedEOF) + if !shouldRetry { + t.Fatal("retryer.Retry(400, io.ErrUnexpectedEOF) = false, want true") + } + }) + t.Run("retry on temporary error", func(t *testing.T) { + retryer := metadataRetryer{bo: constantBackoff{}} + err := errTemp{} + _, shouldRetry := retryer.Retry(400, err) + if !shouldRetry { + t.Fatal("retryer.Retry(400, err) = false, want true") + } + }) + t.Run("retry on wrapped temporary error", func(t *testing.T) { + retryer := metadataRetryer{bo: constantBackoff{}} + err := errWrapped{errTemp{}} + _, shouldRetry := retryer.Retry(400, err) + if !shouldRetry { + t.Fatal("retryer.Retry(400, err) = false, want true") + } + }) + t.Run("don't retry on wrapped io.EOF", func(t *testing.T) { + retryer := metadataRetryer{bo: constantBackoff{}} + err := errWrapped{io.EOF} + _, shouldRetry := retryer.Retry(400, err) + if shouldRetry { + t.Fatal("retryer.Retry(400, err) = true, want false") + } + }) + t.Run("stop retry after 5 attempts", func(t *testing.T) { + retryer := metadataRetryer{bo: constantBackoff{}} + for i := 1; i <= 6; i++ { + _, shouldRetry := retryer.Retry(500, nil) + if i == 6 { + if shouldRetry { + t.Fatal("an error should only be retried 5 times") + } + break + } + if !shouldRetry { + t.Fatalf("retryer.Retry(500, nil) = false, want true") + } + } + }) +} From 913c786a058e5ead4dfcc4e915d06245689b9a52 Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Thu, 19 Aug 2021 12:36:58 -0600 Subject: [PATCH 2/4] review feedback --- compute/metadata/metadata.go | 16 ++++++++-------- compute/metadata/retry.go | 8 ++++++-- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/compute/metadata/metadata.go b/compute/metadata/metadata.go index db775def65c7..94cb12b82769 100644 --- a/compute/metadata/metadata.go +++ b/compute/metadata/metadata.go @@ -311,16 +311,16 @@ func (c *Client) getETag(suffix string) (value, etag string, err error) { for { var err error res, err = c.hc.Do(req) - if err != nil { - if delay, shouldRetry :=retryer.Retry(res.StatusCode, err); shouldRetry { - if err := gax.Sleep(context.Background(), delay); err != nil { - return "", "", err - } - continue + if err == nil { + break + } + if delay, shouldRetry := retryer.Retry(res.StatusCode, err); shouldRetry { + if err := gax.Sleep(context.Background(), delay); err != nil { + return "", "", err } - return "", "", err + continue } - break + return "", "", err } defer res.Body.Close() if res.StatusCode == http.StatusNotFound { diff --git a/compute/metadata/retry.go b/compute/metadata/retry.go index d7374f7a4a6b..e290452234e2 100644 --- a/compute/metadata/retry.go +++ b/compute/metadata/retry.go @@ -21,6 +21,10 @@ import ( "github.com/googleapis/gax-go/v2" ) +const ( + maxRetryAttempts = 5 +) + var ( syscallRetryable = func(err error) bool { return false } ) @@ -43,10 +47,10 @@ func (r *metadataRetryer) Retry(status int, err error) (time.Duration, bool) { if !retryOk { return 0, false } - r.attempts++ - if r.attempts == 6 { + if r.attempts == maxRetryAttempts { return 0, false } + r.attempts++ return r.bo.Pause(), true } From 5344d1c66f8036ddffd078f6a674013571931f08 Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Thu, 19 Aug 2021 13:21:26 -0600 Subject: [PATCH 3/4] remove unused var --- compute/metadata/retry_linux_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compute/metadata/retry_linux_test.go b/compute/metadata/retry_linux_test.go index a2b42a675512..8db8e215d304 100644 --- a/compute/metadata/retry_linux_test.go +++ b/compute/metadata/retry_linux_test.go @@ -25,13 +25,13 @@ func TestMetadataRetryerLinux(t *testing.T) { retryer := metadataRetryer{bo: constantBackoff{}} t.Run("retry on syscall.ECONNRESET", func(t *testing.T) { - delay, shouldRetry := retryer.Retry(400, syscall.ECONNRESET) + _, shouldRetry := retryer.Retry(400, syscall.ECONNRESET) if !shouldRetry { t.Fatal("retryer.Retry(500, nil) = false, want true") } }) t.Run("retry on syscall.ECONNREFUSED", func(t *testing.T) { - delay, shouldRetry := retryer.Retry(400, syscall.ECONNREFUSED) + _, shouldRetry := retryer.Retry(400, syscall.ECONNREFUSED) if !shouldRetry { t.Fatal("retryer.Retry(500, nil) = false, want true") } From 20d38dc244496e058d16edefe76832f5fa6fb67c Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Thu, 19 Aug 2021 14:38:10 -0600 Subject: [PATCH 4/4] add feedback --- compute/metadata/metadata.go | 3 +- compute/metadata/retry_linux_test.go | 4 +- compute/metadata/retry_test.go | 138 +++++++++++++++------------ 3 files changed, 79 insertions(+), 66 deletions(-) diff --git a/compute/metadata/metadata.go b/compute/metadata/metadata.go index 94cb12b82769..646950811c27 100644 --- a/compute/metadata/metadata.go +++ b/compute/metadata/metadata.go @@ -284,6 +284,7 @@ func NewClient(c *http.Client) *Client { // getETag returns a value from the metadata service as well as the associated ETag. // This func is otherwise equivalent to Get. func (c *Client) getETag(suffix string) (value, etag string, err error) { + ctx := context.TODO() // Using a fixed IP makes it very difficult to spoof the metadata service in // a container, which is an important use-case for local testing of cloud // deployments. To enable spoofing of the metadata service, the environment @@ -315,7 +316,7 @@ func (c *Client) getETag(suffix string) (value, etag string, err error) { break } if delay, shouldRetry := retryer.Retry(res.StatusCode, err); shouldRetry { - if err := gax.Sleep(context.Background(), delay); err != nil { + if err := gax.Sleep(ctx, delay); err != nil { return "", "", err } continue diff --git a/compute/metadata/retry_linux_test.go b/compute/metadata/retry_linux_test.go index 8db8e215d304..a046c6973573 100644 --- a/compute/metadata/retry_linux_test.go +++ b/compute/metadata/retry_linux_test.go @@ -27,13 +27,13 @@ func TestMetadataRetryerLinux(t *testing.T) { t.Run("retry on syscall.ECONNRESET", func(t *testing.T) { _, shouldRetry := retryer.Retry(400, syscall.ECONNRESET) if !shouldRetry { - t.Fatal("retryer.Retry(500, nil) = false, want true") + t.Fatal("retryer.Retry(400, syscall.ECONNRESET) = false, want true") } }) t.Run("retry on syscall.ECONNREFUSED", func(t *testing.T) { _, shouldRetry := retryer.Retry(400, syscall.ECONNREFUSED) if !shouldRetry { - t.Fatal("retryer.Retry(500, nil) = false, want true") + t.Fatal("retryer.Retry(400, syscall.ECONNREFUSED) = false, want true") } }) } diff --git a/compute/metadata/retry_test.go b/compute/metadata/retry_test.go index 5d0a2f2405f0..5908ea079333 100644 --- a/compute/metadata/retry_test.go +++ b/compute/metadata/retry_test.go @@ -39,70 +39,82 @@ func (e errWrapped) Error() string { return "unwrap me to get more context" } func (e errWrapped) Unwrap() error { return e.e } func TestMetadataRetryer(t *testing.T) { - t.Run("retry on 500", func(t *testing.T) { - retryer := metadataRetryer{bo: constantBackoff{}} - delay, shouldRetry := retryer.Retry(500, nil) - if !shouldRetry { - t.Fatal("retryer.Retry(500, nil) = false, want true") - } - if delay != 100 { - t.Fatalf("retryer.Retry(500, nil) = %d, want 100", delay) - } - }) - t.Run("don't retry 400", func(t *testing.T) { - retryer := metadataRetryer{bo: constantBackoff{}} - delay, shouldRetry := retryer.Retry(400, io.EOF) - if shouldRetry { - t.Fatal("retryer.Retry(400, io.EOF) = true, want false") - } - if delay != 0 { - t.Fatalf("retryer.Retry(400, io.EOF) = %d, want 0", delay) - } - }) - t.Run("retry on io.ErrUnexpectedEOF", func(t *testing.T) { - retryer := metadataRetryer{bo: constantBackoff{}} - _, shouldRetry := retryer.Retry(400, io.ErrUnexpectedEOF) - if !shouldRetry { - t.Fatal("retryer.Retry(400, io.ErrUnexpectedEOF) = false, want true") - } - }) - t.Run("retry on temporary error", func(t *testing.T) { - retryer := metadataRetryer{bo: constantBackoff{}} - err := errTemp{} - _, shouldRetry := retryer.Retry(400, err) - if !shouldRetry { - t.Fatal("retryer.Retry(400, err) = false, want true") - } - }) - t.Run("retry on wrapped temporary error", func(t *testing.T) { - retryer := metadataRetryer{bo: constantBackoff{}} - err := errWrapped{errTemp{}} - _, shouldRetry := retryer.Retry(400, err) - if !shouldRetry { - t.Fatal("retryer.Retry(400, err) = false, want true") - } - }) - t.Run("don't retry on wrapped io.EOF", func(t *testing.T) { - retryer := metadataRetryer{bo: constantBackoff{}} - err := errWrapped{io.EOF} - _, shouldRetry := retryer.Retry(400, err) - if shouldRetry { - t.Fatal("retryer.Retry(400, err) = true, want false") - } - }) - t.Run("stop retry after 5 attempts", func(t *testing.T) { - retryer := metadataRetryer{bo: constantBackoff{}} - for i := 1; i <= 6; i++ { - _, shouldRetry := retryer.Retry(500, nil) - if i == 6 { - if shouldRetry { - t.Fatal("an error should only be retried 5 times") - } - break + tests := []struct { + name string + code int + err error + wantDelay time.Duration + wantShouldRetry bool + }{ + { + name: "retry on 500", + code: 500, + wantDelay: 100, + wantShouldRetry: true, + }, + { + name: "don't retry on 400", + code: 400, + err: io.EOF, + wantDelay: 0, + wantShouldRetry: false, + }, + { + name: "retry on io.ErrUnexpectedEOF", + code: 400, + err: io.ErrUnexpectedEOF, + wantDelay: 100, + wantShouldRetry: true, + }, + { + name: "retry on temporary error", + code: 400, + err: errTemp{}, + wantDelay: 100, + wantShouldRetry: true, + }, + { + name: "retry on wrapped temporary error", + code: 400, + err: errWrapped{errTemp{}}, + wantDelay: 100, + wantShouldRetry: true, + }, + { + name: "don't retry on wrapped io.EOF", + code: 400, + err: errWrapped{io.EOF}, + wantDelay: 0, + wantShouldRetry: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + retryer := metadataRetryer{bo: constantBackoff{}} + delay, shouldRetry := retryer.Retry(tc.code, tc.err) + if delay != tc.wantDelay { + t.Fatalf("retryer.Retry(%v, %v) = %v, want %v", tc.code, tc.err, delay, tc.wantDelay) } - if !shouldRetry { - t.Fatalf("retryer.Retry(500, nil) = false, want true") + if shouldRetry != tc.wantShouldRetry { + t.Fatalf("retryer.Retry(%v, %v) = %v, want %v", tc.code, tc.err, shouldRetry, tc.wantShouldRetry) } + }) + } +} + +func TestMetadataRetryerAttempts(t *testing.T) { + retryer := metadataRetryer{bo: constantBackoff{}} + for i := 1; i <= 6; i++ { + _, shouldRetry := retryer.Retry(500, nil) + if i == 6 { + if shouldRetry { + t.Fatal("an error should only be retried 5 times") + } + break + } + if !shouldRetry { + t.Fatalf("retryer.Retry(500, nil) = false, want true") } - }) + } }