From d3556005bee162041840c107de09f1047b781da4 Mon Sep 17 00:00:00 2001 From: Joel Hendrix Date: Thu, 6 Feb 2020 21:17:36 -0800 Subject: [PATCH] v13.3.3 (#504) * Deserialize additionalInfo in ARM error * Allow a new authorizer to be created from a configuration file by specifying a resource instead of a base url. This enables resource like KeyVault and Container Registry to use an authorizer configured from a configuration file. * [WIP] Using the Context from the timeout if provided (#315) * Using the timeout from the context if available - Makes PollingDuration optional * Renaming the registration start time * Making PollingDuration not a pointer * fixing a broken reference * Add NewAuthorizerFromCli method which uses Azure CLI to obtain a token for the currently logged in user, for local development scenarios. (#316) * Adding User assigned identity support for the MSIConfig authorizor (#332) * Adding ByteSlicePtr (#399) * Adding a new `WithXML` method (#402) * Add HTTP status code response helpers (#403) Added IsHTTPStatus() and HasHTTPStatus() methods to autorest.Response * adding a new preparer for `MERGE` used in the Storage API's (#406) * New Preparer/Responder for `Unmarshalling Bytes` (#407) * New Preparer: WithBytes * New Responder: `ByUnmarshallingBytes` * Reusing the bytes, rather than copying them * Fixing the broken test / switching to read the bytes directly * Support HTTP-Date in Retry-After header (#410) RFC specifies Retry-After header can be integer value expressing seconds or an HTTP-Date indicating when to try again. Removed superfluous check for HTTP status code. * Add support for multi-tenant authentication (#412) * Add support for multi-tenant authentication Support for multi-tenant via x-ms-authorization-auxiliary header has been added for client credentials with secret scenario; this basically bundles multiple OAuthConfig and ServicePrincipalToken types into corresponding MultiTenant* types along with a new authorizer that adds the primary and auxiliary token headers to the reqest. The authenticaion helpers have been updated to support this scenario; if environment var AZURE_AUXILIARY_TENANT_IDS is set with a semicolon delimited list of tenants the multi-tenant codepath will kick in to create the appropriate authorizer. * feedback * rename Options to OAuthOptions (#415) * Support custom SendDecorator chains via context (#417) * Support custom SendDecorator chains via context Added `autorest.WithSendDecorators` and `autorest.GetSendDecorators` for adding and retrieving a custom chain of SendDecorators to the provided context. Added `autorest.DoRetryForStatusCodesWithCap` and `autorest.DelayForBackoffWithCap` to enforce an upper bound on the duration between retries. Fixed up some code comments. * small refactor based on PR feedback * remove some changes for dev branch * merge master into dev (#427) * v12.3.0 (#418) * Deserialize additionalInfo in ARM error * Allow a new authorizer to be created from a configuration file by specifying a resource instead of a base url. This enables resource like KeyVault and Container Registry to use an authorizer configured from a configuration file. * [WIP] Using the Context from the timeout if provided (#315) * Using the timeout from the context if available - Makes PollingDuration optional * Renaming the registration start time * Making PollingDuration not a pointer * fixing a broken reference * Add NewAuthorizerFromCli method which uses Azure CLI to obtain a token for the currently logged in user, for local development scenarios. (#316) * Adding User assigned identity support for the MSIConfig authorizor (#332) * Adding ByteSlicePtr (#399) * Adding a new `WithXML` method (#402) * Add HTTP status code response helpers (#403) Added IsHTTPStatus() and HasHTTPStatus() methods to autorest.Response * adding a new preparer for `MERGE` used in the Storage API's (#406) * New Preparer/Responder for `Unmarshalling Bytes` (#407) * New Preparer: WithBytes * New Responder: `ByUnmarshallingBytes` * Reusing the bytes, rather than copying them * Fixing the broken test / switching to read the bytes directly * Support HTTP-Date in Retry-After header (#410) RFC specifies Retry-After header can be integer value expressing seconds or an HTTP-Date indicating when to try again. Removed superfluous check for HTTP status code. * Add support for multi-tenant authentication (#412) * Add support for multi-tenant authentication Support for multi-tenant via x-ms-authorization-auxiliary header has been added for client credentials with secret scenario; this basically bundles multiple OAuthConfig and ServicePrincipalToken types into corresponding MultiTenant* types along with a new authorizer that adds the primary and auxiliary token headers to the reqest. The authenticaion helpers have been updated to support this scenario; if environment var AZURE_AUXILIARY_TENANT_IDS is set with a semicolon delimited list of tenants the multi-tenant codepath will kick in to create the appropriate authorizer. * feedback * rename Options to OAuthOptions (#415) * Support custom SendDecorator chains via context (#417) * Support custom SendDecorator chains via context Added `autorest.WithSendDecorators` and `autorest.GetSendDecorators` for adding and retrieving a custom chain of SendDecorators to the provided context. Added `autorest.DoRetryForStatusCodesWithCap` and `autorest.DelayForBackoffWithCap` to enforce an upper bound on the duration between retries. Fixed up some code comments. * small refactor based on PR feedback * remove some changes for dev branch * v12.3.0 * add yaml file for azure devops CI (#419) * add status badge for azure devops CI (#420) * enable build and test on linux (#421) * enable build and test on linux * fail on first error and use portable std* * update test to run on devops * Refactor azure devops pipeline (#422) Break monolithic script into separate scripts with useful names. Moved formatting checks to the end with succeededOrFailed conditions. * remove travis artifacts (#423) * remove unnecessary trigger section from devops (#424) * Use accessTokens.json from AZURE_CONFIG_DIR if AZURE_ACCESS_TOKEN_FILE is not set before falling back on ~/.azure/ (#471) * support for parsing error messages from xml responses (#465) * support for parsing error messages from xml responses * fixing the linting * removed some duplicate code * fix bug introduced in refactoring * added XML test and fixed bug it uncovered * fix godoc comment for methods that are safe for concurrent use (#475) * New Authorizers for Azure Storage (#416) * Authorizers for Blob, File, Queue and Table Storage * Adding a SharedKey authorizer * refactor based on existing storage implementation * add missing storage emulator account name * replace hard-coded strings with constants * changed to by-ref * Adding a new Authorizer for SAS Token Authentication (#478) * Adding a new Authorizer for SAS Token Authentication This commit introduces a new Authorizer for authenticating with Blob Storage using a SAS Token ``` $ go test -v ./autorest/ -run="TestSas" === RUN TestSasNewSasAuthorizerEmptyToken --- PASS: TestSasNewSasAuthorizerEmptyToken (0.00s) === RUN TestSasNewSasAuthorizerEmptyTokenWithWhitespace --- PASS: TestSasNewSasAuthorizerEmptyTokenWithWhitespace (0.00s) === RUN TestSasNewSasAuthorizerValidToken --- PASS: TestSasNewSasAuthorizerValidToken (0.00s) === RUN TestSasAuthorizerRequest --- PASS: TestSasAuthorizerRequest (0.00s) authorization_sas_test.go:76: [DEBUG] Testing Case "empty querystring without a prefix".. authorization_sas_test.go:76: [DEBUG] Testing Case "empty querystring with a prefix".. authorization_sas_test.go:76: [DEBUG] Testing Case "existing querystring without a prefix".. authorization_sas_test.go:76: [DEBUG] Testing Case "existing querystring with a prefix".. PASS ok github.com/Azure/go-autorest/autorest 0.011s ``` * minor clean-up * token: support for a custom refresh func (#476) * token: support for a custom refresh func * pass closures by value * minor clean-up * Fix Dropped Errors (#480) * autorest: fix dropped errror * autorest/adal: fix dropped test error * Duration order consistency when multiplying number by time unit (#499) * Drain response bodies (#432) The retry helpers and a few other methods weren't reading and closing response bodies leading to connection leaks. * Enable exponential back-off when retrying on 429 (#503) * Enable exponential back-off when retrying on 429 * enforce a 2-minute cap on delays if there isn't one * updated comment * fix type-o * update version and CHANGELOG Co-authored-by: Nick Co-authored-by: Tom Harvey Co-authored-by: Sam Kreter Co-authored-by: Delyan Raychev Co-authored-by: Patrick Decat Co-authored-by: Tony Abboud Co-authored-by: Lars Lehtonen Co-authored-by: Maxim Fominykh --- CHANGELOG.md | 8 +++++++ autorest/adal/token.go | 7 +++++- autorest/adal/token_test.go | 3 +++ autorest/authorization.go | 31 ++++++++++++------------ autorest/authorization_storage.go | 3 +++ autorest/azure/async.go | 12 +++++++++- autorest/sender.go | 16 +++++++++++-- autorest/utility.go | 11 +++++++++ autorest/utility_test.go | 39 +++++++++++++++++++++++++++++++ autorest/version.go | 2 +- 10 files changed, 112 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e6430bfb9..aca6c6d6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ # CHANGELOG +## v13.3.3 + +### Bug Fixes + +- Fixed connection leak when retrying requests. +- Enabled exponential back-off with a 2-minute cap when retrying on 429. +- Fixed some cases where errors were inadvertently dropped. + ## v13.3.2 ### Bug Fixes diff --git a/autorest/adal/token.go b/autorest/adal/token.go index 33bbd6ea1..b65b2c8b2 100644 --- a/autorest/adal/token.go +++ b/autorest/adal/token.go @@ -24,6 +24,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "io/ioutil" "math" "net/http" @@ -248,7 +249,7 @@ func (secret *ServicePrincipalCertificateSecret) SignJwt(spt *ServicePrincipalTo "sub": spt.inner.ClientID, "jti": base64.URLEncoding.EncodeToString(jti), "nbf": time.Now().Unix(), - "exp": time.Now().Add(time.Hour * 24).Unix(), + "exp": time.Now().Add(24 * time.Hour).Unix(), } signedString, err := token.SignedString(secret.PrivateKey) @@ -972,6 +973,10 @@ func retryForIMDS(sender Sender, req *http.Request, maxAttempts int) (resp *http delay := time.Duration(0) for attempt < maxAttempts { + if resp != nil && resp.Body != nil { + io.Copy(ioutil.Discard, resp.Body) + resp.Body.Close() + } resp, err = sender.Do(req) // we want to retry if err is not nil or the status code is in the list of retry codes if err == nil && !responseHasStatusCode(resp, retries...) { diff --git a/autorest/adal/token_test.go b/autorest/adal/token_test.go index d20a475f4..8f4ec25c4 100644 --- a/autorest/adal/token_test.go +++ b/autorest/adal/token_test.go @@ -978,6 +978,9 @@ func TestNewMultiTenantServicePrincipalToken(t *testing.T) { t.Fatalf("autorest/adal: unexpected error while creating multitenant config: %v", err) } mt, err := NewMultiTenantServicePrincipalToken(cfg, "clientID", "superSecret", "resource") + if err != nil { + t.Fatalf("autorest/adal: unexpected error while creating multitenant service principal token: %v", err) + } if !strings.Contains(mt.PrimaryToken.inner.OauthConfig.AuthorizeEndpoint.String(), TestTenantID) { t.Fatal("didn't find primary tenant ID in primary SPT") } diff --git a/autorest/authorization.go b/autorest/authorization.go index 54e87b5b6..f43e1a6ed 100644 --- a/autorest/authorization.go +++ b/autorest/authorization.go @@ -171,20 +171,21 @@ func (bacb *BearerAuthorizerCallback) WithAuthorization() PrepareDecorator { removeRequestBody(&rCopy) resp, err := bacb.sender.Do(&rCopy) - if err == nil && resp.StatusCode == 401 { - defer resp.Body.Close() - if hasBearerChallenge(resp) { - bc, err := newBearerChallenge(resp) + if err != nil { + return r, err + } + DrainResponseBody(resp) + if resp.StatusCode == 401 && hasBearerChallenge(resp.Header) { + bc, err := newBearerChallenge(resp.Header) + if err != nil { + return r, err + } + if bacb.callback != nil { + ba, err := bacb.callback(bc.values[tenantID], bc.values["resource"]) if err != nil { return r, err } - if bacb.callback != nil { - ba, err := bacb.callback(bc.values[tenantID], bc.values["resource"]) - if err != nil { - return r, err - } - return Prepare(r, ba.WithAuthorization()) - } + return Prepare(r, ba.WithAuthorization()) } } } @@ -194,8 +195,8 @@ func (bacb *BearerAuthorizerCallback) WithAuthorization() PrepareDecorator { } // returns true if the HTTP response contains a bearer challenge -func hasBearerChallenge(resp *http.Response) bool { - authHeader := resp.Header.Get(bearerChallengeHeader) +func hasBearerChallenge(header http.Header) bool { + authHeader := header.Get(bearerChallengeHeader) if len(authHeader) == 0 || strings.Index(authHeader, bearer) < 0 { return false } @@ -206,8 +207,8 @@ type bearerChallenge struct { values map[string]string } -func newBearerChallenge(resp *http.Response) (bc bearerChallenge, err error) { - challenge := strings.TrimSpace(resp.Header.Get(bearerChallengeHeader)) +func newBearerChallenge(header http.Header) (bc bearerChallenge, err error) { + challenge := strings.TrimSpace(header.Get(bearerChallengeHeader)) trimmedChallenge := challenge[len(bearer)+1:] // challenge is a set of key=value pairs that are comma delimited diff --git a/autorest/authorization_storage.go b/autorest/authorization_storage.go index 33e5f1270..b844a3df4 100644 --- a/autorest/authorization_storage.go +++ b/autorest/authorization_storage.go @@ -104,6 +104,9 @@ func (sk *SharedKeyAuthorizer) WithAuthorization() PrepareDecorator { } sk, err := buildSharedKey(sk.accountName, sk.accountKey, r, sk.keyType) + if err != nil { + return r, err + } return Prepare(r, WithHeader(headerAuthorization, sk)) }) } diff --git a/autorest/azure/async.go b/autorest/azure/async.go index 1cb41cbeb..c5fc511f6 100644 --- a/autorest/azure/async.go +++ b/autorest/azure/async.go @@ -258,7 +258,17 @@ func (f Future) GetResult(sender autorest.Sender) (*http.Response, error) { if err != nil { return nil, err } - return sender.Do(req) + resp, err := sender.Do(req) + if err == nil && resp.Body != nil { + // copy the body and close it so callers don't have to + defer resp.Body.Close() + b, err := ioutil.ReadAll(resp.Body) + if err != nil { + return resp, err + } + resp.Body = ioutil.NopCloser(bytes.NewReader(b)) + } + return resp, err } type pollingTracker interface { diff --git a/autorest/sender.go b/autorest/sender.go index 5e595d7b1..a07670b8c 100644 --- a/autorest/sender.go +++ b/autorest/sender.go @@ -243,6 +243,7 @@ func DoRetryForAttempts(attempts int, backoff time.Duration) SendDecorator { if err != nil { return resp, err } + DrainResponseBody(resp) resp, err = s.Do(rr.Request()) if err == nil { return resp, err @@ -283,11 +284,12 @@ func DoRetryForStatusCodesWithCap(attempts int, backoff, cap time.Duration, code func doRetryForStatusCodesImpl(s Sender, r *http.Request, count429 bool, attempts int, backoff, cap time.Duration, codes ...int) (resp *http.Response, err error) { rr := NewRetriableRequest(r) // Increment to add the first call (attempts denotes number of retries) - for attempt := 0; attempt < attempts+1; { + for attempt, delayCount := 0, 0; attempt < attempts+1; { err = rr.Prepare() if err != nil { return } + DrainResponseBody(resp) resp, err = s.Do(rr.Request()) // we want to retry if err is not nil (e.g. transient network failure). note that for failed authentication // resp and err will both have a value, so in this case we don't want to retry as it will never succeed. @@ -295,7 +297,13 @@ func doRetryForStatusCodesImpl(s Sender, r *http.Request, count429 bool, attempt return resp, err } delayed := DelayWithRetryAfter(resp, r.Context().Done()) - if !delayed && !DelayForBackoffWithCap(backoff, cap, attempt, r.Context().Done()) { + // enforce a 2 minute cap between requests when 429 status codes are + // not going to be counted as an attempt and when the cap is 0. + // this should only happen in the absence of a retry-after header. + if !count429 && cap == 0 { + cap = 2 * time.Minute + } + if !delayed && !DelayForBackoffWithCap(backoff, cap, delayCount, r.Context().Done()) { return resp, r.Context().Err() } // when count429 == false don't count a 429 against the number @@ -303,6 +311,9 @@ func doRetryForStatusCodesImpl(s Sender, r *http.Request, count429 bool, attempt if count429 || (resp == nil || resp.StatusCode != http.StatusTooManyRequests) { attempt++ } + // delay count is tracked separately from attempts to + // ensure that 429 participates in exponential back-off + delayCount++ } return resp, err } @@ -347,6 +358,7 @@ func DoRetryForDuration(d time.Duration, backoff time.Duration) SendDecorator { if err != nil { return resp, err } + DrainResponseBody(resp) resp, err = s.Do(rr.Request()) if err == nil { return resp, err diff --git a/autorest/utility.go b/autorest/utility.go index 27f824f54..67baab2ce 100644 --- a/autorest/utility.go +++ b/autorest/utility.go @@ -20,6 +20,7 @@ import ( "encoding/xml" "fmt" "io" + "io/ioutil" "net" "net/http" "net/url" @@ -226,3 +227,13 @@ func IsTemporaryNetworkError(err error) bool { } return false } + +// DrainResponseBody reads the response body then closes it. +func DrainResponseBody(resp *http.Response) error { + if resp != nil && resp.Body != nil { + _, err := io.Copy(ioutil.Discard, resp.Body) + resp.Body.Close() + return err + } + return nil +} diff --git a/autorest/utility_test.go b/autorest/utility_test.go index 2c6ed95c1..ab85c784c 100644 --- a/autorest/utility_test.go +++ b/autorest/utility_test.go @@ -20,6 +20,7 @@ import ( "encoding/xml" "errors" "fmt" + "io" "net/http" "net/url" "reflect" @@ -469,3 +470,41 @@ func withErrorRespondDecorator(e *error) RespondDecorator { }) } } + +type mockDrain struct { + read bool + closed bool +} + +func (md *mockDrain) Read(b []byte) (int, error) { + md.read = true + b = append(b, 0xff) + return 1, io.EOF +} + +func (md *mockDrain) Close() error { + md.closed = true + return nil +} + +func TestDrainResponseBody(t *testing.T) { + err := DrainResponseBody(nil) + if err != nil { + t.Fatalf("expected nil error, got %v", err) + } + err = DrainResponseBody(&http.Response{}) + if err != nil { + t.Fatalf("expected nil error, got %v", err) + } + md := &mockDrain{} + err = DrainResponseBody(&http.Response{Body: md}) + if err != nil { + t.Fatalf("expected nil error, got %v", err) + } + if !md.closed { + t.Fatal("mockDrain wasn't closed") + } + if !md.read { + t.Fatal("mockDrain wasn't read") + } +} diff --git a/autorest/version.go b/autorest/version.go index 6c1d86566..bdaff1d08 100644 --- a/autorest/version.go +++ b/autorest/version.go @@ -19,7 +19,7 @@ import ( "runtime" ) -const number = "v13.3.2" +const number = "v13.3.3" var ( userAgent = fmt.Sprintf("Go/%s (%s-%s) go-autorest/%s",