Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prep azcore v1.6.1 for release #20961

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions sdk/azcore/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
# Release History

## 1.6.1 (Unreleased)

### Features Added

### Breaking Changes
## 1.6.1 (2023-06-06)

### Bugs Fixed

### Other Changes
* Retry policy always clones the underlying `*http.Request` before invoking the next policy.
* Added some non-standard error codes to the list of error codes for unregistered resource providers.

## 1.6.0 (2023-05-04)

Expand Down
2 changes: 1 addition & 1 deletion sdk/azcore/arm/runtime/pipeline_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func TestDisableAutoRPRegistration(t *testing.T) {
srv, close := mock.NewServer()
defer close()
// initial response that RP is unregistered
srv.SetResponse(mock.WithStatusCode(http.StatusConflict), mock.WithBody([]byte(rpUnregisteredResp)))
srv.SetResponse(mock.WithStatusCode(http.StatusConflict), mock.WithBody([]byte(rpUnregisteredResp1)))
opts := &armpolicy.ClientOptions{DisableRPRegistration: true, ClientOptions: policy.ClientOptions{Transport: srv}}
req, err := azruntime.NewRequest(context.Background(), http.MethodGet, srv.URL())
if err != nil {
Expand Down
18 changes: 16 additions & 2 deletions sdk/azcore/arm/runtime/policy_register_rp.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ func (r *rpRegistrationPolicy) Do(req *azpolicy.Request) (*http.Response, error)
// policy is disabled
return req.Next()
}
const unregisteredRPCode = "MissingSubscriptionRegistration"
const registeredState = "Registered"
var rp string
var resp *http.Response
Expand All @@ -101,7 +100,7 @@ func (r *rpRegistrationPolicy) Do(req *azpolicy.Request) (*http.Response, error)
// to the caller so its error unmarshalling will kick in
return resp, err
}
if !strings.EqualFold(reqErr.ServiceError.Code, unregisteredRPCode) {
if !isUnregisteredRPCode(reqErr.ServiceError.Code) {
// not a 409 due to unregistered RP. just return the response
// to the caller so its error unmarshalling will kick in
return resp, err
Expand Down Expand Up @@ -173,6 +172,21 @@ func (r *rpRegistrationPolicy) Do(req *azpolicy.Request) (*http.Response, error)
return resp, fmt.Errorf("exceeded attempts to register %s", rp)
}

var unregisteredRPCodes = []string{
"MissingSubscriptionRegistration",
"MissingRegistrationForResourceProvider",
"Subscription Not Registered",
}

func isUnregisteredRPCode(errorCode string) bool {
for _, code := range unregisteredRPCodes {
if strings.EqualFold(errorCode, code) {
return true
}
}
return false
}

func getSubscription(path string) (string, error) {
parts := strings.Split(path, "/")
for i, v := range parts {
Expand Down
34 changes: 26 additions & 8 deletions sdk/azcore/arm/runtime/policy_register_rp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
"github.com/stretchr/testify/require"
)

const rpUnregisteredResp = `{
const rpUnregisteredResp1 = `{
"error":{
"code":"MissingSubscriptionRegistration",
"message":"The subscription registration is in 'Unregistered' state. The subscription must be registered to use namespace 'Microsoft.Storage'. See https://aka.ms/rps-not-found for how to register subscriptions.",
Expand All @@ -37,6 +37,19 @@ const rpUnregisteredResp = `{
}
}`

const rpUnregisteredResp2 = `{
"error":{
"code":"MissingRegistrationForResourceProvider",
"message":"The subscription registration is in 'Unregistered' state. The subscription must be registered to use namespace 'Microsoft.Storage'. See https://aka.ms/rps-not-found for how to register subscriptions.",
"details":[{
"code":"MissingRegistrationForResourceProvider",
"target":"Microsoft.Storage",
"message":"The subscription registration is in 'Unregistered' state. The subscription must be registered to use namespace 'Microsoft.Storage'. See https://aka.ms/rps-not-found for how to register subscriptions."
}
]
}
}`

// some content was omitted here as it's not relevant
const rpRegisteringResp = `{
"id": "/subscriptions/00000000-0000-0000-0000-000000000000/providers/Microsoft.Storage",
Expand Down Expand Up @@ -89,7 +102,7 @@ func TestRPRegistrationPolicySuccess(t *testing.T) {
srv, close := mock.NewServer()
defer close()
// initial response that RP is unregistered
srv.AppendResponse(mock.WithStatusCode(http.StatusConflict), mock.WithBody([]byte(rpUnregisteredResp)))
srv.AppendResponse(mock.WithStatusCode(http.StatusConflict), mock.WithBody([]byte(rpUnregisteredResp1)))
// polling responses to Register() and Get(), in progress
srv.RepeatResponse(5, mock.WithStatusCode(http.StatusOK), mock.WithBody([]byte(rpRegisteringResp)))
// polling response, successful registration
Expand Down Expand Up @@ -180,7 +193,7 @@ func TestRPRegistrationPolicyTimesOut(t *testing.T) {
srv, close := mock.NewServer()
defer close()
// initial response that RP is unregistered
srv.AppendResponse(mock.WithStatusCode(http.StatusConflict), mock.WithBody([]byte(rpUnregisteredResp)))
srv.AppendResponse(mock.WithStatusCode(http.StatusConflict), mock.WithBody([]byte(rpUnregisteredResp1)))
// polling responses to Register() and Get(), in progress but slow
// tests registration takes too long, times out
srv.RepeatResponse(10, mock.WithStatusCode(http.StatusOK), mock.WithBody([]byte(rpRegisteringResp)), mock.WithSlowResponse(400*time.Millisecond))
Expand Down Expand Up @@ -212,7 +225,7 @@ func TestRPRegistrationPolicyExceedsAttempts(t *testing.T) {
// add a cycle of unregistered->registered so that we keep retrying and hit the cap
for i := 0; i < 4; i++ {
// initial response that RP is unregistered
srv.AppendResponse(mock.WithStatusCode(http.StatusConflict), mock.WithBody([]byte(rpUnregisteredResp)))
srv.AppendResponse(mock.WithStatusCode(http.StatusConflict), mock.WithBody([]byte(rpUnregisteredResp1)))
// polling responses to Register() and Get(), in progress
srv.RepeatResponse(2, mock.WithStatusCode(http.StatusOK), mock.WithBody([]byte(rpRegisteringResp)))
// polling response, successful registration
Expand Down Expand Up @@ -246,7 +259,7 @@ func TestRPRegistrationPolicyCanCancel(t *testing.T) {
srv, close := mock.NewServer()
defer close()
// initial response that RP is unregistered
srv.AppendResponse(mock.WithStatusCode(http.StatusConflict), mock.WithBody([]byte(rpUnregisteredResp)))
srv.AppendResponse(mock.WithStatusCode(http.StatusConflict), mock.WithBody([]byte(rpUnregisteredResp2)))
// polling responses to Register() and Get(), in progress but slow so we have time to cancel
srv.RepeatResponse(10, mock.WithStatusCode(http.StatusOK), mock.WithBody([]byte(rpRegisteringResp)), mock.WithSlowResponse(300*time.Millisecond))
// log only RP registration
Expand Down Expand Up @@ -287,7 +300,7 @@ func TestRPRegistrationPolicyDisabled(t *testing.T) {
srv, close := mock.NewServer()
defer close()
// initial response that RP is unregistered
srv.AppendResponse(mock.WithStatusCode(http.StatusConflict), mock.WithBody([]byte(rpUnregisteredResp)))
srv.AppendResponse(mock.WithStatusCode(http.StatusConflict), mock.WithBody([]byte(rpUnregisteredResp2)))
ops := testRPRegistrationOptions(srv)
ops.MaxAttempts = -1
client := newFakeClient(t, srv, ops)
Expand All @@ -305,7 +318,7 @@ func TestRPRegistrationPolicyDisabled(t *testing.T) {
require.Error(t, err)
var respErr *exported.ResponseError
require.ErrorAs(t, err, &respErr)
require.EqualValues(t, "MissingSubscriptionRegistration", respErr.ErrorCode)
require.EqualValues(t, "MissingRegistrationForResourceProvider", respErr.ErrorCode)
require.Zero(t, resp)
// shouldn't be any log entries
require.Zero(t, logEntries)
Expand All @@ -315,7 +328,7 @@ func TestRPRegistrationPolicyAudience(t *testing.T) {
srv, close := mock.NewServer()
defer close()
// initial response that RP is unregistered
srv.AppendResponse(mock.WithStatusCode(http.StatusConflict), mock.WithBody([]byte(rpUnregisteredResp)))
srv.AppendResponse(mock.WithStatusCode(http.StatusConflict), mock.WithBody([]byte(rpUnregisteredResp2)))
// polling responses to Register() and Get(), in progress
srv.AppendResponse(mock.WithStatusCode(http.StatusOK), mock.WithBody([]byte(rpRegisteringResp)))
// polling response, successful registration
Expand Down Expand Up @@ -399,6 +412,11 @@ func TestRPRegistrationPolicyEnvironmentsInSubExceeded(t *testing.T) {
require.EqualValues(t, 0, logEntries)
}

func TestIsUnregisteredRPCode(t *testing.T) {
require.True(t, isUnregisteredRPCode("Subscription Not Registered"))
require.False(t, isUnregisteredRPCode("Your subscription isn't registered"))
}

type fakeClient struct {
ep string
pl runtime.Pipeline
Expand Down
3 changes: 2 additions & 1 deletion sdk/azcore/runtime/policy_retry.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ func (p *retryPolicy) Do(req *policy.Request) (resp *http.Response, err error) {
}

if options.TryTimeout == 0 {
resp, err = req.Next()
clone := req.Clone(req.Raw().Context())
resp, err = clone.Next()
} else {
// Set the per-try time for this particular retry operation and then Do the operation.
tryCtx, tryCancel := context.WithTimeout(req.Raw().Context(), options.TryTimeout)
Expand Down
33 changes: 33 additions & 0 deletions sdk/azcore/runtime/policy_retry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -810,6 +810,39 @@ func TestRetryableRequestBodyWithCloser(t *testing.T) {
require.True(t, tr.closeCalled)
}

func TestRetryPolicySuccessWithRetryPreserveHeaders(t *testing.T) {
srv, close := mock.NewServer()
defer close()
srv.AppendResponse(mock.WithStatusCode(http.StatusRequestTimeout))
srv.AppendResponse()
pl := exported.NewPipeline(srv, NewRetryPolicy(testRetryOptions()), exported.PolicyFunc(challengeLikePolicy))
req, err := NewRequest(context.Background(), http.MethodGet, srv.URL())
require.NoError(t, err)
body := newRewindTrackingBody("stuff")
require.NoError(t, req.SetBody(body, "text/plain"))
resp, err := pl.Do(req)
require.NoError(t, err)
require.EqualValues(t, http.StatusOK, resp.StatusCode)
require.EqualValues(t, 2, srv.Requests())
require.EqualValues(t, 1, body.rcount)
require.True(t, body.closed)
}

func challengeLikePolicy(req *policy.Request) (*http.Response, error) {
if req.Body() == nil {
return nil, errors.New("request body wasn't restored")
}
if req.Raw().Header.Get("content-type") != "text/plain" {
return nil, errors.New("content-type header wasn't restored")
}

// remove the body and header. the retry policy should restore them
if err := req.SetBody(nil, ""); err != nil {
return nil, err
}
return req.Next()
}

func newRewindTrackingBody(s string) *rewindTrackingBody {
// there are two rewinds that happen before rewinding for a retry
// 1. to get the body's size in SetBody()
Expand Down