From c03bff875567c6431a4a6f1c1ffbe37af56575fd Mon Sep 17 00:00:00 2001 From: Abhishek Pandey Date: Fri, 9 Feb 2024 20:00:24 -0800 Subject: [PATCH] Use filters.In instead of contains. Add tests --- src/pkg/services/m365/api/graph/service.go | 12 +- .../services/m365/api/graph/service_test.go | 121 ++++++++++++------ 2 files changed, 94 insertions(+), 39 deletions(-) diff --git a/src/pkg/services/m365/api/graph/service.go b/src/pkg/services/m365/api/graph/service.go index c490fc5f1..4f324924d 100644 --- a/src/pkg/services/m365/api/graph/service.go +++ b/src/pkg/services/m365/api/graph/service.go @@ -174,9 +174,15 @@ func KiotaHTTPClient( const ( defaultDelay = 3 * time.Second defaultHTTPClientTimeout = 1 * time.Hour + + // Default retry count for retry middlewares + defaultMaxRetries = 3 + // Retry count for graph adapter + // // Bumping retries to 6 since we have noticed that auth token expiry errors // may continue to persist even after 3 retries. - defaultMaxRetries = 6 + adapterMaxRetries = 6 + // FIXME: This should ideally be 0, but if we set to 0, graph // client with automatically set the context timeout to 0 as // well which will make the client unusable. @@ -203,7 +209,7 @@ type Option func(*clientConfig) // populate constructs a clientConfig according to the provided options. func populateConfig(opts ...Option) *clientConfig { cc := clientConfig{ - maxConnectionRetries: defaultMaxRetries, + maxConnectionRetries: adapterMaxRetries, maxRetries: defaultMaxRetries, minDelay: defaultDelay, timeout: defaultHTTPClientTimeout, @@ -376,7 +382,7 @@ func wrapAdapter(gra *msgraphsdkgo.GraphRequestAdapter, cc *clientConfig) *adapt } } -var connectionEnded = filters.Contains([]string{ +var connectionEnded = filters.In([]string{ "connection reset by peer", "client connection force closed", "read: connection timed out", diff --git a/src/pkg/services/m365/api/graph/service_test.go b/src/pkg/services/m365/api/graph/service_test.go index 56cab3890..7181fff32 100644 --- a/src/pkg/services/m365/api/graph/service_test.go +++ b/src/pkg/services/m365/api/graph/service_test.go @@ -93,7 +93,7 @@ func (suite *GraphIntgSuite) TestHTTPClient() { checkConfig: func(t *testing.T, c *clientConfig) { assert.Equal(t, defaultDelay, c.minDelay, "default delay") assert.Equal(t, defaultMaxRetries, c.maxRetries, "max retries") - assert.Equal(t, defaultMaxRetries, c.maxConnectionRetries, "max connection retries") + assert.Equal(t, adapterMaxRetries, c.maxConnectionRetries, "max connection retries") }, }, { @@ -221,43 +221,92 @@ func (suite *GraphIntgSuite) TestAdapterWrap_catchesPanic() { require.Contains(t, err.Error(), "panic", clues.ToCore(err)) } -func (suite *GraphIntgSuite) TestAdapterWrap_retriesConnectionClose() { - t := suite.T() - - ctx, flush := tester.NewContext(t) - defer flush() - - retryInc := 0 - - // the panics should get caught and returned as errors - alwaysECONNRESET := mwForceResp{ - err: syscall.ECONNRESET, - alternate: func(req *http.Request) (bool, *http.Response, error) { - retryInc++ - return false, nil, nil +func (suite *GraphIntgSuite) TestAdapterWrap_retriesConnectionInterruptions() { + table := []struct { + name string + providedErr error + expectRetryCount int + expectErr assert.ErrorAssertionFunc + }{ + { + name: "ECONNRESET", + providedErr: syscall.ECONNRESET, + expectRetryCount: 7, + expectErr: assert.Error, + }, + { + name: "connection reset by peer", + providedErr: clues.New("connection reset by peer"), + expectRetryCount: 7, + expectErr: assert.Error, + }, + { + name: "read: connection timed out", + providedErr: clues.New("read: connection timed out"), + expectRetryCount: 7, + expectErr: assert.Error, + }, + { + name: "unexpected EOF", + providedErr: io.ErrUnexpectedEOF, + expectRetryCount: 7, + expectErr: assert.Error, + }, + { + name: "nil error", + providedErr: nil, + expectRetryCount: 1, + expectErr: assert.NoError, + }, + { + name: "non retriable error", + providedErr: clues.New("non retriable error"), + expectRetryCount: 1, + expectErr: assert.Error, }, } - adpt, err := CreateAdapter( - suite.credentials.AzureTenantID, - suite.credentials.AzureClientID, - suite.credentials.AzureClientSecret, - count.New(), - appendMiddleware(&alwaysECONNRESET), - // Configure retry middlewares so that they don't retry on connection reset. - // Those middlewares have their own tests to verify retries. - MaxRetries(-1)) - require.NoError(t, err, clues.ToCore(err)) + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() - // Retry delay doesn't really matter here since all requests will be intercepted - // by the test middleware. Set it to 0 to reduce test runtime. - aw := adpt.(*adapterWrap) - aw.retryDelay = 0 + ctx, flush := tester.NewContext(t) + defer flush() - // the query doesn't matter - _, err = NewService(adpt).Client().Users().Get(ctx, nil) - require.ErrorIs(t, err, syscall.ECONNRESET, clues.ToCore(err)) - require.Equal(t, 4, retryInc, "number of retries") + retryInc := 0 + forceErrMW := mwForceResp{ + err: test.providedErr, + // send some dummy response, because kiota retry handler panics + // if err is nil and resp is nil. + resp: &http.Response{}, + alternate: func(req *http.Request) (bool, *http.Response, error) { + retryInc++ + return false, nil, nil + }, + } + + adpt, err := CreateAdapter( + suite.credentials.AzureTenantID, + suite.credentials.AzureClientID, + suite.credentials.AzureClientSecret, + count.New(), + appendMiddleware(&forceErrMW), + // Configure retry middlewares so that they don't retry on connection reset. + // Those middlewares have their own tests to verify retries. + MaxRetries(-1)) + require.NoError(t, err, clues.ToCore(err)) + + // Retry delay doesn't really matter here since all requests will be intercepted + // by the test middleware. Set it to 0 to reduce test runtime. + aw := adpt.(*adapterWrap) + aw.retryDelay = 0 + + // the query doesn't matter + _, err = NewService(adpt).Client().Users().Get(ctx, nil) + test.expectErr(t, err, clues.ToCore(err)) + require.Equal(t, test.expectRetryCount, retryInc, "number of retries") + }) + } } func (suite *GraphIntgSuite) TestAdapterWrap_retriesBadJWTToken() { @@ -315,14 +364,14 @@ func (suite *GraphIntgSuite) TestAdapterWrap_retriesBadJWTToken() { NewItemCalendarsItemEventsDeltaRequestBuilder("https://graph.microsoft.com/fnords/beaux/regard", adpt). Get(ctx, nil) assert.ErrorIs(t, err, core.ErrAuthTokenExpired, clues.ToCore(err)) - assert.Equal(t, 4, retryInc, "number of retries") + assert.Equal(t, adapterMaxRetries+1, retryInc, "number of retries") retryInc = 0 // the query doesn't matter _, err = NewService(adpt).Client().Users().Get(ctx, nil) assert.ErrorIs(t, err, core.ErrAuthTokenExpired, clues.ToCore(err)) - assert.Equal(t, 4, retryInc, "number of retries") + assert.Equal(t, adapterMaxRetries+1, retryInc, "number of retries") } // TestAdapterWrap_retriesInvalidRequest tests adapter retries for graph 400 @@ -398,7 +447,7 @@ func (suite *GraphIntgSuite) TestAdapterWrap_retriesInvalidRequest() { _, err = NewService(adpt).Client().Users().Get(ctx, nil) return err }, - expectedRetries: 4, + expectedRetries: adapterMaxRetries + 1, }, { name: "POST request, no retry",