diff --git a/.changelog/3299.txt b/.changelog/3299.txt new file mode 100644 index 0000000000..c0e2d5195f --- /dev/null +++ b/.changelog/3299.txt @@ -0,0 +1,3 @@ +```release-note:bug +provider: Fixed an error with resources failing to upload large files (e.g. with `google_storage_bucket_object`) during retried requests +``` diff --git a/google-beta/retry_transport.go b/google-beta/retry_transport.go index c1019c8aa5..ed05c1479e 100644 --- a/google-beta/retry_transport.go +++ b/google-beta/retry_transport.go @@ -34,7 +34,6 @@ import ( "context" "errors" "fmt" - "github.com/hashicorp/errwrap" "github.com/hashicorp/terraform-plugin-sdk/helper/resource" "google.golang.org/api/googleapi" "io/ioutil" @@ -99,19 +98,29 @@ func (t *retryTransport) RoundTrip(req *http.Request) (resp *http.Response, resp backoff := time.Millisecond * 500 nextBackoff := time.Millisecond * 500 + // VCR depends on the original request body being consumed, so + // consume here. Since this won't affect the request itself, + // we do this before the actual Retry loop so we can consume the request Body as needed + // e.g. if the request couldn't be retried, we use the original request + if _, err := httputil.DumpRequestOut(req, true); err != nil { + log.Printf("[WARN] Retry Transport: Consuming original request body failed: %v", err) + } + log.Printf("[DEBUG] Retry Transport: starting RoundTrip retry loop") Retry: for { - log.Printf("[DEBUG] Retry Transport: request attempt %d", attempts) - - // Copy the request - we dont want to use the original request as - // RoundTrip contract says request body can/will be consumed + // RoundTrip contract says request body can/will be consumed, so we need to + // copy the request body for each attempt. + // If we can't copy the request, we run as a single request. newRequest, copyErr := copyHttpRequest(req) if copyErr != nil { - respErr = errwrap.Wrapf("unable to copy invalid http.Request for retry: {{err}}", copyErr) + log.Printf("[WARN] Retry Transport: Unable to copy request body: %v.", copyErr) + log.Printf("[WARN] Retry Transport: Running request as non-retryable") + resp, respErr = t.internal.RoundTrip(req) break Retry } + log.Printf("[DEBUG] Retry Transport: request attempt %d", attempts) // Do the wrapped Roundtrip. This is one request in the retry loop. resp, respErr = t.internal.RoundTrip(newRequest) attempts++ @@ -141,13 +150,6 @@ Retry: continue } } - - // VCR depends on the original request body being consumed, so consume it here - _, err := httputil.DumpRequestOut(req, true) - if err != nil { - log.Printf("[DEBUG] Retry Transport: Reading request failed: %v", err) - } - log.Printf("[DEBUG] Retry Transport: Returning after %d attempts", attempts) return resp, respErr } @@ -157,15 +159,13 @@ Retry: // so it can be consumed. func copyHttpRequest(req *http.Request) (*http.Request, error) { newRequest := *req - if req.Body == nil || req.Body == http.NoBody { return &newRequest, nil } - // Helpers like http.NewRequest add a GetBody for copying. // If not given, we should reject the request. if req.GetBody == nil { - return nil, errors.New("invalid HTTP request for transport, expected request.GetBody for non-empty Body") + return nil, errors.New("request.GetBody is not defined for non-empty Body") } bd, err := req.GetBody() diff --git a/google-beta/retry_transport_test.go b/google-beta/retry_transport_test.go index 617528ca5f..4c8ba023ec 100644 --- a/google-beta/retry_transport_test.go +++ b/google-beta/retry_transport_test.go @@ -123,6 +123,51 @@ func TestRetryTransport_SuccessWithBody(t *testing.T) { testRetryTransport_checkBody(t, resp, body) } +// Check for no and no retries if the request has no getBody (it should only run once) +func TestRetryTransport_DoesNotRetryEmptyGetBody(t *testing.T) { + msg := "non empty body" + attempted := false + + ts, client := setUpRetryTransportServerClient(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + // Check for request body + dump, err := ioutil.ReadAll(r.Body) + if err != nil { + w.WriteHeader(testRetryTransportCodeFailure) + if _, werr := w.Write([]byte(fmt.Sprintf("got error: %v", err))); werr != nil { + t.Errorf("[ERROR] unable to write to response writer: %v", err) + } + } + dumpS := string(dump) + if dumpS != msg { + w.WriteHeader(testRetryTransportCodeFailure) + if _, werr := w.Write([]byte(fmt.Sprintf("got unexpected body: %s", dumpS))); werr != nil { + t.Errorf("[ERROR] unable to write to response writer: %v", err) + } + } + if attempted { + w.WriteHeader(testRetryTransportCodeFailure) + if _, werr := w.Write([]byte("expected only one try")); werr != nil { + t.Errorf("[ERROR] unable to write to response writer: %v", err) + } + } + attempted = true + w.WriteHeader(testRetryTransportCodeRetry) + })) + defer ts.Close() + + // Create request + req, err := http.NewRequest("GET", ts.URL, strings.NewReader(msg)) + if err != nil { + t.Errorf("[ERROR] unable to make test request: %v", err) + } + // remove GetBody + req.GetBody = nil + + resp, err := client.Do(req) + testRetryTransport_checkFailedWhileRetrying(t, resp, err) +} + // handlers func testRetryTransportHandler_noRetries(t *testing.T, code int) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {