Add retry for oauth metadata endpoint #4259

squiishyy · 2023-10-18T18:27:32Z

Tracking issue

https://unionai.atlassian.net/browse/CLOUD-175

Describe your changes

Added functionality to retry getting oauth metadata information upon 5xx error codes.
Added config values to be able to specify number of retry attempts and retry delay duration.
Added tests as well!

Check all the applicable boxes

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Screenshots

Note to reviewers

squiishyy · 2023-10-18T18:39:20Z

flyteadmin/auth/authzserver/metadata_provider.go

@@ -71,10 +79,15 @@ func (s OAuth2MetadataProvider) GetOAuth2Metadata(ctx context.Context, r *servic
 			}
 			httpClient.Transport = transport
 		}
-
-		response, err := httpClient.Get(externalMetadataURL.String())
+		logger.Printf(ctx, "retryAttempts: %v retryDuration: %v", s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelayMilliseconds)


Here for testing

codecov · 2023-10-18T19:50:39Z

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (96e3497) 59.05% compared to head (03fc38a) 59.24%.
Report is 2 commits behind head on master.

❗ Current head 03fc38a differs from pull request most recent head 3838ead. Consider uploading reports for the commit 3838ead to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4259      +/-   ##
==========================================
+ Coverage   59.05%   59.24%   +0.18%     
==========================================
  Files         621      621              
  Lines       53106    53147      +41     
==========================================
+ Hits        31360    31485     +125     
+ Misses      19246    19151      -95     
- Partials     2500     2511      +11

Flag	Coverage Δ
unittests	`59.24% <70.45%> (+0.18%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
flyteadmin/auth/config/config.go	`71.42% <ø> (ø)`
flyteadmin/auth/config/config_flags.go	`63.15% <100.00%> (+1.33%)`	⬆️
flyteadmin/pkg/async/shared.go	`87.87% <73.33%> (-12.13%)`	⬇️
flyteadmin/auth/authzserver/metadata_provider.go	`71.13% <66.66%> (-0.10%)`	⬇️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

flyteadmin/auth/authzserver/metadata_provider.go

pmahindrakar-oss · 2023-10-18T22:16:07Z

flyteadmin/auth/authzserver/metadata_provider.go

+			if response != nil {
+				logger.Errorf(ctx, "Failed to get oauth metadata. Error code: %v. Err: %v", response.StatusCode, err)
+				return nil, flyteErrors.NewFlyteAdminError(codes.Code(response.StatusCode), "Failed to get oauth metadata.")
+			}


Are we expected to see a non nil response on err, I would consider not considering this case, we have special handling around responses in error cases

Not expected! Just wanted to add an edge case here, happy to remove it though! I did not know we had special handling around responses in error cases :)

In this particular case, you are getting the statusCode from the response so we can keep it.
Also on L90 , 500 is not a valid grpc error code, I would use code.Internal there

pmahindrakar-oss · 2023-10-18T22:18:51Z

flyteadmin/pkg/async/shared.go

@@ -10,6 +11,24 @@ import (
 // RetryDelay indicates how long to wait between restarting a subscriber connection in the case of network failures.
 var RetryDelay = 30 * time.Second

+func RetryOnSpecificErrorCodes(totalAttempts int, delay time.Duration, f func() (*http.Response, error), IsErrorCodeRetryable func(*http.Response) bool) error {


Can we instead reuse this one which comes from K8s utils. We are using this in flyteadmin already https://github.com/flyteorg/flyte/blob/master/flyteadmin/scheduler/executor/executor_impl.go#L100

Definitely can, is there any specific reason as to why you're wanting to use this one? We use the other retry functions in shared.go elsewhere in FlyteAdmin as well

Oh. For consitency across our repos , would be good to use one if they provide the same functionality.
In cloud repo too we use the k8s retry util in grafana dataplane with usage of RetryOnConflict and hence was seeing if we could use the same

Ah gotcha! makes sense, happy to migrate over to that :) will work on that now

Signed-off-by: Chao-Heng Lee <[email protected]> Signed-off-by: squiishyy <[email protected]>

Signed-off-by: squiishyy <[email protected]>

…ttprequestwithretry, need to work on config mappingn still, tests in Signed-off-by: squiishyy <[email protected]>

Signed-off-by: squiishyy <[email protected]>

squiishyy · 2023-10-19T00:31:40Z

Closed in favor of: #4262

squiishyy changed the title ~~Add retry for oauth metadata~~ Add retry for oauth metadata 2 Oct 18, 2023

squiishyy mentioned this pull request Oct 18, 2023

Add retry for oauth metadata endpoint #4254

Closed

3 tasks

squiishyy changed the title ~~Add retry for oauth metadata 2~~ Add retry for oauth metadata endpoint Oct 18, 2023

squiishyy marked this pull request as ready for review October 18, 2023 18:37

squiishyy commented Oct 18, 2023

View reviewed changes

pmahindrakar-oss reviewed Oct 18, 2023

View reviewed changes

chaohengstudent and others added 11 commits October 18, 2023 16:51

[flytepropeller] Add test under flytepropeller/manager (flyteorg#4237)

7b9a9d0

Signed-off-by: Chao-Heng Lee <[email protected]> Signed-off-by: squiishyy <[email protected]>

logic and commented out tests

28f329b

Signed-off-by: squiishyy <[email protected]>

make RetryOnSpecificErrorCodes, added functionality for calling sendh…

d85ec1d

…ttprequestwithretry, need to work on config mappingn still, tests in Signed-off-by: squiishyy <[email protected]>

adding default config values

4b498ea

Signed-off-by: squiishyy <[email protected]>

imports

95a5d32

Signed-off-by: squiishyy <[email protected]>

updated tests to work

9a126b3

Signed-off-by: squiishyy <[email protected]>

added test

becb042

Signed-off-by: squiishyy <[email protected]>

spelling and info

ae7ea12

Signed-off-by: squiishyy <[email protected]>

working, removing comment

a4f2da9

Signed-off-by: squiishyy <[email protected]>

add back

fe4e8ee

Signed-off-by: squiishyy <[email protected]>

code review

3838ead

Signed-off-by: squiishyy <[email protected]>

squiishyy force-pushed the squiishyy/oauth2-retry-2 branch from 0644587 to 3838ead Compare October 18, 2023 23:52

squiishyy mentioned this pull request Oct 19, 2023

Try 2: Add retry for oauth metadata endpoint #4262

Merged

3 tasks

squiishyy closed this Oct 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add retry for oauth metadata endpoint #4259

Add retry for oauth metadata endpoint #4259

squiishyy commented Oct 18, 2023 •

edited

Loading

squiishyy Oct 18, 2023

codecov bot commented Oct 18, 2023 •

edited

Loading

pmahindrakar-oss Oct 18, 2023

squiishyy Oct 18, 2023

pmahindrakar-oss Oct 18, 2023

pmahindrakar-oss Oct 18, 2023

squiishyy Oct 18, 2023

pmahindrakar-oss Oct 18, 2023

squiishyy Oct 18, 2023

squiishyy commented Oct 19, 2023

Add retry for oauth metadata endpoint #4259

Add retry for oauth metadata endpoint #4259

Conversation

squiishyy commented Oct 18, 2023 • edited Loading

Tracking issue

Describe your changes

Check all the applicable boxes

Screenshots

Note to reviewers

Choose a reason for hiding this comment

codecov bot commented Oct 18, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squiishyy commented Oct 19, 2023

squiishyy commented Oct 18, 2023 •

edited

Loading

codecov bot commented Oct 18, 2023 •

edited

Loading