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 <muller_nicky@hotmail.com> Co-authored-by: Tom Harvey <tombuildsstuff@users.noreply.github.com> Co-authored-by: Sam Kreter <samkreter@gmail.com> Co-authored-by: Delyan Raychev <delyan.raychev@microsoft.com> Co-authored-by: Patrick Decat <pdecat@gmail.com> Co-authored-by: Tony Abboud <tdabboud@hotmail.com> Co-authored-by: Lars Lehtonen <lars.lehtonen@gmail.com> Co-authored-by: Maxim Fominykh <vominyh@yandex.ru>
Azure · Feb 7, 2020 · d355600 · d355600
1 parent 9b80a09
commit d355600
Show file tree

Hide file tree

Showing 10 changed files with 112 additions and 20 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,26 +284,36 @@ 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.
 		if err == nil && !ResponseHasStatusCode(resp, codes...) || IsTokenRefreshError(err) {
 			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
 		// of attempts so that we continue to retry until it succeeds
 		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
@@ -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
@@ -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
@@ -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",