Skip to content

Commit 945e8ed

Browse files
authored
Various bug fixes for azcore and internal (Azure#18905)
* Various bug fixes for azcore and internal Adjusted the initial retry delay to align with the design guidelines. Fixed an issue with modifying the original *http.Request that breaks retries during recording playback. * update test * fix tests
1 parent f069342 commit 945e8ed

File tree

8 files changed

+48
-30
lines changed

8 files changed

+48
-30
lines changed

sdk/azcore/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
### Breaking Changes
88

99
### Bugs Fixed
10+
* Adjusted the initial retry delay to 800ms per the Azure SDK guidelines.
1011

1112
### Other Changes
1213

sdk/azcore/runtime/policy_retry.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ func setDefaults(o *policy.RetryOptions) {
3838
o.MaxRetryDelay = math.MaxInt64
3939
}
4040
if o.RetryDelay == 0 {
41-
o.RetryDelay = 4 * time.Second
41+
o.RetryDelay = 800 * time.Millisecond
4242
} else if o.RetryDelay < 0 {
4343
o.RetryDelay = 0
4444
}

sdk/azcore/runtime/policy_retry_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@ func TestRetryPolicyRequestTimedOut(t *testing.T) {
333333
defer close()
334334
srv.SetError(errors.New("bogus error"))
335335
pl := exported.NewPipeline(srv, NewRetryPolicy(nil))
336-
ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
336+
ctx, cancel := context.WithTimeout(context.Background(), 400*time.Millisecond)
337337
defer cancel()
338338
req, err := NewRequest(ctx, http.MethodPost, srv.URL())
339339
if err != nil {

sdk/internal/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
### Breaking Changes
88

99
### Bugs Fixed
10+
* Don't modify the original *http.Request during recording/perf as it causes failures during retries.
1011

1112
### Other Changes
1213

sdk/internal/perf/recording.go

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -131,8 +131,8 @@ func NewProxyTransport(options *TransportOptions) *RecordingHTTPClient {
131131

132132
func (c RecordingHTTPClient) Do(req *http.Request) (*http.Response, error) {
133133
if c.mode != liveMode {
134-
err := c.replaceAuthority(req)
135-
if err != nil {
134+
var err error
135+
if req, err = c.replaceAuthority(req); err != nil {
136136
return nil, err
137137
}
138138
}
@@ -144,22 +144,29 @@ func (c *RecordingHTTPClient) SetMode(mode string) {
144144
c.mode = mode
145145
}
146146

147-
func (c *RecordingHTTPClient) replaceAuthority(rawReq *http.Request) error {
147+
func (c *RecordingHTTPClient) replaceAuthority(rawReq *http.Request) (*http.Request, error) {
148148
parsedProxyURL, err := url.Parse(c.options.proxyURL)
149149
if err != nil {
150-
return fmt.Errorf("there was an error parsing url '%s': %s", c.options.proxyURL, err.Error())
150+
return nil, fmt.Errorf("there was an error parsing url '%s': %s", c.options.proxyURL, err.Error())
151151
}
152152
originalURLHost := rawReq.URL.Host
153153
originalURLScheme := rawReq.URL.Scheme
154-
rawReq.URL.Scheme = parsedProxyURL.Scheme
155-
rawReq.URL.Host = parsedProxyURL.Host
156-
rawReq.Host = parsedProxyURL.Host
157-
158-
rawReq.Header.Set(upstreamURIHeader, fmt.Sprintf("%v://%v", originalURLScheme, originalURLHost))
159-
rawReq.Header.Set(modeHeader, c.mode)
160-
rawReq.Header.Set(idHeader, c.recID)
161-
rawReq.Header.Set("x-recording-remove", "false")
162-
return nil
154+
155+
// don't modify the original request
156+
cp := *rawReq
157+
cpURL := *cp.URL
158+
cp.URL = &cpURL
159+
cp.Header = rawReq.Header.Clone()
160+
161+
cp.URL.Scheme = parsedProxyURL.Scheme
162+
cp.URL.Host = parsedProxyURL.Host
163+
cp.Host = parsedProxyURL.Host
164+
165+
cp.Header.Set(upstreamURIHeader, fmt.Sprintf("%v://%v", originalURLScheme, originalURLHost))
166+
cp.Header.Set(modeHeader, c.mode)
167+
cp.Header.Set(idHeader, c.recID)
168+
cp.Header.Set("x-recording-remove", "false")
169+
return &cp, nil
163170
}
164171

165172
// start tells the test proxy to begin accepting requests for a given test

sdk/internal/perf/recording_test.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,17 +33,17 @@ func TestRecordingHTTPClient_Do(t *testing.T) {
3333
resp, err = client.Do(req)
3434
require.NoError(t, err)
3535
require.Equal(t, "https://localhost:5001", resp.Request.URL.String())
36-
require.Contains(t, req.Header.Get(upstreamURIHeader), "https://www.bing.com")
37-
require.Equal(t, req.Header.Get(modeHeader), "record")
38-
require.Equal(t, req.Header.Get(idHeader), client.recID)
36+
require.Contains(t, resp.Request.Header.Get(upstreamURIHeader), "https://www.bing.com")
37+
require.Equal(t, resp.Request.Header.Get(modeHeader), "record")
38+
require.Equal(t, resp.Request.Header.Get(idHeader), client.recID)
3939

4040
proxyTransportsSuite[t.Name()].SetMode("playback")
4141
req, err = http.NewRequest("POST", "https://www.bing.com", nil)
4242
require.NoError(t, err)
4343
resp, err = client.Do(req)
4444
require.NoError(t, err)
4545
require.Equal(t, "https://localhost:5001", resp.Request.URL.String())
46-
require.Contains(t, req.Header.Get(upstreamURIHeader), "https://www.bing.com")
47-
require.Equal(t, req.Header.Get(modeHeader), "playback")
48-
require.Equal(t, req.Header.Get(idHeader), client.recID)
46+
require.Contains(t, resp.Request.Header.Get(upstreamURIHeader), "https://www.bing.com")
47+
require.Equal(t, resp.Request.Header.Get(modeHeader), "playback")
48+
require.Equal(t, resp.Request.Header.Get(idHeader), client.recID)
4949
}

sdk/internal/recording/recording.go

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -533,17 +533,26 @@ func defaultOptions() *RecordingOptions {
533533
}
534534
}
535535

536-
func (r RecordingOptions) ReplaceAuthority(t *testing.T, rawReq *http.Request) {
536+
func (r RecordingOptions) ReplaceAuthority(t *testing.T, rawReq *http.Request) *http.Request {
537537
if GetRecordMode() != LiveMode && !IsLiveOnly(t) {
538538
originalURLHost := rawReq.URL.Host
539-
rawReq.URL.Scheme = r.scheme()
540-
rawReq.URL.Host = r.host()
541-
rawReq.Host = r.host()
542539

543-
rawReq.Header.Set(UpstreamURIHeader, fmt.Sprintf("%v://%v", r.scheme(), originalURLHost))
544-
rawReq.Header.Set(ModeHeader, GetRecordMode())
545-
rawReq.Header.Set(IDHeader, GetRecordingId(t))
540+
// don't modify the original request
541+
cp := *rawReq
542+
cpURL := *cp.URL
543+
cp.URL = &cpURL
544+
cp.Header = rawReq.Header.Clone()
545+
546+
cp.URL.Scheme = r.scheme()
547+
cp.URL.Host = r.host()
548+
cp.Host = r.host()
549+
550+
cp.Header.Set(UpstreamURIHeader, fmt.Sprintf("%v://%v", r.scheme(), originalURLHost))
551+
cp.Header.Set(ModeHeader, GetRecordMode())
552+
cp.Header.Set(IDHeader, GetRecordingId(t))
553+
rawReq = &cp
546554
}
555+
return rawReq
547556
}
548557

549558
func (r RecordingOptions) host() string {
@@ -756,7 +765,7 @@ type RecordingHTTPClient struct {
756765
}
757766

758767
func (c RecordingHTTPClient) Do(req *http.Request) (*http.Response, error) {
759-
c.options.ReplaceAuthority(c.t, req)
768+
req = c.options.ReplaceAuthority(c.t, req)
760769
return c.defaultClient.Do(req)
761770
}
762771

sdk/internal/recording/recording_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -473,7 +473,7 @@ func TestStartStopRecordingClient(t *testing.T) {
473473
err = json.Unmarshal(byteValue, &data)
474474
require.NoError(t, err)
475475
require.Equal(t, "https://azsdkengsys.azurecr.io/acr/v1/some_registry/_tags", data.Entries[0].RequestURI)
476-
require.Equal(t, req.URL.String(), "https://localhost:5001/acr/v1/some_registry/_tags")
476+
require.Equal(t, resp.Request.URL.String(), "https://localhost:5001/acr/v1/some_registry/_tags")
477477
}
478478

479479
func TestStopRecordingNoStart(t *testing.T) {

0 commit comments

Comments
 (0)