From 5c2ef13b15f4c7bb6b5d3d5f40af6e5cb2cf7c81 Mon Sep 17 00:00:00 2001 From: squiishyy Date: Fri, 13 Oct 2023 14:02:19 -0700 Subject: [PATCH 01/17] logic and commented out tests Signed-off-by: squiishyy --- .../auth/authzserver/metadata_provider.go | 37 ++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/flyteadmin/auth/authzserver/metadata_provider.go b/flyteadmin/auth/authzserver/metadata_provider.go index b1887ef4c2..14af8a970c 100644 --- a/flyteadmin/auth/authzserver/metadata_provider.go +++ b/flyteadmin/auth/authzserver/metadata_provider.go @@ -2,16 +2,28 @@ package authzserver import ( "context" + "errors" + "fmt" "io/ioutil" "net/http" "net/url" "strings" + "time" + + errrs "github.com/pkg/errors" + "google.golang.org/api/googleapi" "github.com/flyteorg/flyte/flyteadmin/auth" authConfig "github.com/flyteorg/flyte/flyteadmin/auth/config" + "github.com/flyteorg/flyte/flyteadmin/pkg/async" "github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/service" ) +var ( + retryAttempts = 5 + retryDelay = 1 * time.Second +) + type OAuth2MetadataProvider struct { cfg *authConfig.Config } @@ -72,7 +84,7 @@ func (s OAuth2MetadataProvider) GetOAuth2Metadata(ctx context.Context, r *servic httpClient.Transport = transport } - response, err := httpClient.Get(externalMetadataURL.String()) + response, err := sendAndRetryHttpRequest(httpClient, externalMetadataURL.String()) if err != nil { return nil, err } @@ -107,3 +119,26 @@ func NewService(config *authConfig.Config) OAuth2MetadataProvider { cfg: config, } } + +func sendAndRetryHttpRequest(client *http.Client, url string) (*http.Response, error) { + var response *http.Response + var err error + err = async.RetryOnSpecificErrors(retryAttempts, retryDelay, func() error { + response, err = client.Get(url) + return err + }, isTransientError) + + if err != nil { + e, _ := errrs.Cause(err).(*googleapi.Error) + return nil, errors.New(fmt.Sprintf("Failed to get oauth metadata after %v attempts. Error code: %v Err: %v", retryAttempts, e.Code, e.Error())) + } + + return response, nil +} + +func isTransientError(err error) bool { + if e, ok := errrs.Cause(err).(*googleapi.Error); ok && e.Code >= 500 && e.Code <= 599 { + return true + } + return false +} From b48be8a2da3806fc90d7639e94c9eb3d4c378986 Mon Sep 17 00:00:00 2001 From: squiishyy Date: Tue, 17 Oct 2023 16:41:57 -0700 Subject: [PATCH 02/17] make RetryOnSpecificErrorCodes, added functionality for calling sendhttprequestwithretry, need to work on config mappingn still, tests in Signed-off-by: squiishyy --- .../auth/authzserver/metadata_provider.go | 38 ++++++++++++------- .../authzserver/metadata_provider_test.go | 28 ++++++++++++++ flyteadmin/auth/config/config.go | 3 +- flyteadmin/pkg/async/shared.go | 19 ++++++++++ 4 files changed, 73 insertions(+), 15 deletions(-) diff --git a/flyteadmin/auth/authzserver/metadata_provider.go b/flyteadmin/auth/authzserver/metadata_provider.go index 14af8a970c..2b7e4f5cca 100644 --- a/flyteadmin/auth/authzserver/metadata_provider.go +++ b/flyteadmin/auth/authzserver/metadata_provider.go @@ -10,18 +10,20 @@ import ( "strings" "time" - errrs "github.com/pkg/errors" - "google.golang.org/api/googleapi" + "google.golang.org/grpc/codes" "github.com/flyteorg/flyte/flyteadmin/auth" + flyteErrors "github.com/flyteorg/flyte/flyteadmin/pkg/errors" + "github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/service" + "github.com/flyteorg/flyte/flytestdlib/logger" + authConfig "github.com/flyteorg/flyte/flyteadmin/auth/config" "github.com/flyteorg/flyte/flyteadmin/pkg/async" - "github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/service" ) var ( - retryAttempts = 5 - retryDelay = 1 * time.Second + defaultRetryAttempts = 5 + defaultRetryDelay = 1 * time.Second ) type OAuth2MetadataProvider struct { @@ -84,8 +86,13 @@ func (s OAuth2MetadataProvider) GetOAuth2Metadata(ctx context.Context, r *servic httpClient.Transport = transport } - response, err := sendAndRetryHttpRequest(httpClient, externalMetadataURL.String()) + response, err := sendAndRetryHttpRequest(httpClient, externalMetadataURL.String(), defaultRetryAttempts, defaultRetryDelay) if err != nil { + 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.") + } + logger.Errorf(ctx, "Failed to get oauth metadata. Err: %v", response.StatusCode, err) return nil, err } @@ -120,24 +127,27 @@ func NewService(config *authConfig.Config) OAuth2MetadataProvider { } } -func sendAndRetryHttpRequest(client *http.Client, url string) (*http.Response, error) { +func sendAndRetryHttpRequest(client *http.Client, url string, retryAttempts int, retryDelay time.Duration) (*http.Response, error) { var response *http.Response var err error - err = async.RetryOnSpecificErrors(retryAttempts, retryDelay, func() error { + err = async.RetryOnSpecificErrorCodes(retryAttempts, retryDelay, func() (*http.Response, error) { response, err = client.Get(url) - return err - }, isTransientError) + return response, err + }, isTransientErrorCode) if err != nil { - e, _ := errrs.Cause(err).(*googleapi.Error) - return nil, errors.New(fmt.Sprintf("Failed to get oauth metadata after %v attempts. Error code: %v Err: %v", retryAttempts, e.Code, e.Error())) + return nil, err + } + + if response.StatusCode != http.StatusOK { + return response, errors.New(fmt.Sprint("Failed to get oauth metadata")) } return response, nil } -func isTransientError(err error) bool { - if e, ok := errrs.Cause(err).(*googleapi.Error); ok && e.Code >= 500 && e.Code <= 599 { +func isTransientErrorCode(resp *http.Response) bool { + if resp.StatusCode >= 500 && resp.StatusCode <= 599 { return true } return false diff --git a/flyteadmin/auth/authzserver/metadata_provider_test.go b/flyteadmin/auth/authzserver/metadata_provider_test.go index f1b244012e..1ed8b0b56a 100644 --- a/flyteadmin/auth/authzserver/metadata_provider_test.go +++ b/flyteadmin/auth/authzserver/metadata_provider_test.go @@ -16,6 +16,8 @@ import ( config2 "github.com/flyteorg/flyte/flytestdlib/config" ) +var oauthMetadataFailureErrorMessage = "Failed to get oauth metadata" + func TestOAuth2MetadataProvider_FlyteClient(t *testing.T) { provider := NewService(&authConfig.Config{ AppAuth: authConfig.OAuth2Options{ @@ -111,3 +113,29 @@ func TestOAuth2MetadataProvider_OAuth2Metadata(t *testing.T) { assert.Equal(t, "https://dev-14186422.okta.com", resp.Issuer) }) } + +func TestSendAndRetryHttpRequest(t *testing.T) { + hf := func(w http.ResponseWriter, r *http.Request) { + switch strings.TrimSpace(r.URL.Path) { + case "/": + mockExternalMetadataEndpointTransientFailure(w, r) + default: + http.NotFoundHandler().ServeHTTP(w, r) + } + + } + + server := httptest.NewServer(http.HandlerFunc(hf)) + defer server.Close() + http.DefaultClient = server.Client() + + resp, err := sendAndRetryHttpRequest(server.Client(), server.URL, 5, 0) + assert.Error(t, err) + assert.Equal(t, oauthMetadataFailureErrorMessage, err.Error()) + assert.NotNil(t, resp) + assert.Equal(t, 500, resp.StatusCode) +} + +func mockExternalMetadataEndpointTransientFailure(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(500) +} diff --git a/flyteadmin/auth/config/config.go b/flyteadmin/auth/config/config.go index 217983683e..a192da04da 100644 --- a/flyteadmin/auth/config/config.go +++ b/flyteadmin/auth/config/config.go @@ -191,7 +191,8 @@ type ExternalAuthorizationServer struct { AllowedAudience []string `json:"allowedAudience" pflag:",Optional: A list of allowed audiences. If not provided, the audience is expected to be the public Uri of the service."` MetadataEndpointURL config.URL `json:"metadataUrl" pflag:",Optional: If the server doesn't support /.well-known/oauth-authorization-server, you can set a custom metadata url here.'"` // HTTPProxyURL allows operators to access external OAuth2 servers using an external HTTP Proxy - HTTPProxyURL config.URL `json:"httpProxyURL" pflag:",OPTIONAL: HTTP Proxy to be used for OAuth requests."` + HTTPProxyURL config.URL `json:"httpProxyURL" pflag:",OPTIONAL: HTTP Proxy to be used for OAuth requests."` + RetryAttempts int `json:"retryAttempts" default:"5" pflag:", Optional: The number of attempted retries on a transient failure to get the OAuth metadata"` } // OAuth2Options defines settings for app auth. diff --git a/flyteadmin/pkg/async/shared.go b/flyteadmin/pkg/async/shared.go index 9fafb50479..26b8268478 100644 --- a/flyteadmin/pkg/async/shared.go +++ b/flyteadmin/pkg/async/shared.go @@ -2,6 +2,7 @@ package async import ( "context" + "net/http" "time" "github.com/flyteorg/flyte/flytestdlib/logger" @@ -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(attempts int, delay time.Duration, f func() (*http.Response, error), IsErrorCodeRetryable func(*http.Response) bool) error { + var err error + var resp *http.Response + for attempt := 0; attempt <= attempts; attempt++ { + resp, err = f() + if err != nil { + return err + } + if !IsErrorCodeRetryable(resp) { + return err + } + logger.Warningf(context.Background(), + "Failed status code %v on attempt %d of %d", resp.StatusCode, attempt, attempts) + time.Sleep(delay) + } + return err +} + func RetryOnSpecificErrors(attempts int, delay time.Duration, f func() error, IsErrorRetryable func(error) bool) error { var err error for attempt := 0; attempt <= attempts; attempt++ { From 18e54024197c125856e68a7c52aaa30bc19d0a77 Mon Sep 17 00:00:00 2001 From: squiishyy Date: Wed, 18 Oct 2023 11:10:33 -0700 Subject: [PATCH 03/17] adding default config values Signed-off-by: squiishyy --- .../auth/authzserver/metadata_provider.go | 13 +++------ flyteadmin/auth/config/config.go | 9 ++++-- flyteadmin/auth/config/config_flags.go | 2 ++ flyteadmin/auth/config/config_flags_test.go | 28 +++++++++++++++++++ 4 files changed, 41 insertions(+), 11 deletions(-) diff --git a/flyteadmin/auth/authzserver/metadata_provider.go b/flyteadmin/auth/authzserver/metadata_provider.go index 2b7e4f5cca..6dd30e5cf7 100644 --- a/flyteadmin/auth/authzserver/metadata_provider.go +++ b/flyteadmin/auth/authzserver/metadata_provider.go @@ -21,11 +21,6 @@ import ( "github.com/flyteorg/flyte/flyteadmin/pkg/async" ) -var ( - defaultRetryAttempts = 5 - defaultRetryDelay = 1 * time.Second -) - type OAuth2MetadataProvider struct { cfg *authConfig.Config } @@ -85,15 +80,15 @@ func (s OAuth2MetadataProvider) GetOAuth2Metadata(ctx context.Context, r *servic } httpClient.Transport = transport } - - response, err := sendAndRetryHttpRequest(httpClient, externalMetadataURL.String(), defaultRetryAttempts, defaultRetryDelay) + logger.Printf(ctx, "retryAttempts: %v retryDuration: %v", s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelayMilliseconds) + response, err := sendAndRetryHttpRequest(httpClient, externalMetadataURL.String(), s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelayMilliseconds.Duration) if err != nil { 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.") } - logger.Errorf(ctx, "Failed to get oauth metadata. Err: %v", response.StatusCode, err) - return nil, err + logger.Errorf(ctx, "Failed to get oauth metadata. Err: %v", err) + return nil, flyteErrors.NewFlyteAdminError(codes.Code(500), "Failed to get oauth metadata.") } raw, err := ioutil.ReadAll(response.Body) diff --git a/flyteadmin/auth/config/config.go b/flyteadmin/auth/config/config.go index a192da04da..8ba4f7aca6 100644 --- a/flyteadmin/auth/config/config.go +++ b/flyteadmin/auth/config/config.go @@ -79,6 +79,10 @@ var ( }, }, AppAuth: OAuth2Options{ + ExternalAuthServer: ExternalAuthorizationServer{ + RetryAttempts: 5, + RetryDelayMilliseconds: config.Duration{Duration: 1000 * time.Millisecond}, + }, AuthServerType: AuthorizationServerTypeSelf, ThirdParty: ThirdPartyConfigOptions{ FlyteClientConfig: FlyteClientConfig{ @@ -191,8 +195,9 @@ type ExternalAuthorizationServer struct { AllowedAudience []string `json:"allowedAudience" pflag:",Optional: A list of allowed audiences. If not provided, the audience is expected to be the public Uri of the service."` MetadataEndpointURL config.URL `json:"metadataUrl" pflag:",Optional: If the server doesn't support /.well-known/oauth-authorization-server, you can set a custom metadata url here.'"` // HTTPProxyURL allows operators to access external OAuth2 servers using an external HTTP Proxy - HTTPProxyURL config.URL `json:"httpProxyURL" pflag:",OPTIONAL: HTTP Proxy to be used for OAuth requests."` - RetryAttempts int `json:"retryAttempts" default:"5" pflag:", Optional: The number of attempted retries on a transient failure to get the OAuth metadata"` + HTTPProxyURL config.URL `json:"httpProxyURL" pflag:",OPTIONAL: HTTP Proxy to be used for OAuth requests."` + RetryAttempts int `json:"retryAttempts" pflag:", Optional: The number of attempted retries on a transient failure to get the OAuth metadata"` + RetryDelayMilliseconds config.Duration `json:"retryDelayMilliseconds" pflag:", Optional, Duration in milliseconds to wait between retries"` } // OAuth2Options defines settings for app auth. diff --git a/flyteadmin/auth/config/config_flags.go b/flyteadmin/auth/config/config_flags.go index 225e8a5c9d..a1cb45b501 100755 --- a/flyteadmin/auth/config/config_flags.go +++ b/flyteadmin/auth/config/config_flags.go @@ -77,6 +77,8 @@ func (cfg Config) GetPFlagSet(prefix string) *pflag.FlagSet { cmdFlags.StringSlice(fmt.Sprintf("%v%v", prefix, "appAuth.externalAuthServer.allowedAudience"), DefaultConfig.AppAuth.ExternalAuthServer.AllowedAudience, "Optional: A list of allowed audiences. If not provided, the audience is expected to be the public Uri of the service.") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "appAuth.externalAuthServer.metadataUrl"), DefaultConfig.AppAuth.ExternalAuthServer.MetadataEndpointURL.String(), "Optional: If the server doesn't support /.well-known/oauth-authorization-server, you can set a custom metadata url here.'") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "appAuth.externalAuthServer.httpProxyURL"), DefaultConfig.AppAuth.ExternalAuthServer.HTTPProxyURL.String(), "OPTIONAL: HTTP Proxy to be used for OAuth requests.") + cmdFlags.Int(fmt.Sprintf("%v%v", prefix, "appAuth.externalAuthServer.retryAttempts"), DefaultConfig.AppAuth.ExternalAuthServer.RetryAttempts, " Optional: The number of attempted retries on a transient failure to get the OAuth metadata") + cmdFlags.String(fmt.Sprintf("%v%v", prefix, "appAuth.externalAuthServer.retryDelayMilliseconds"), DefaultConfig.AppAuth.ExternalAuthServer.RetryDelayMilliseconds.String(), " Optional, Duration in milliseconds to wait between retries") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "appAuth.thirdPartyConfig.flyteClient.clientId"), DefaultConfig.AppAuth.ThirdParty.FlyteClientConfig.ClientID, "public identifier for the app which handles authorization for a Flyte deployment") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "appAuth.thirdPartyConfig.flyteClient.redirectUri"), DefaultConfig.AppAuth.ThirdParty.FlyteClientConfig.RedirectURI, "This is the callback uri registered with the app which handles authorization for a Flyte deployment") cmdFlags.StringSlice(fmt.Sprintf("%v%v", prefix, "appAuth.thirdPartyConfig.flyteClient.scopes"), DefaultConfig.AppAuth.ThirdParty.FlyteClientConfig.Scopes, "Recommended scopes for the client to request.") diff --git a/flyteadmin/auth/config/config_flags_test.go b/flyteadmin/auth/config/config_flags_test.go index 28efafc380..3145c826f2 100755 --- a/flyteadmin/auth/config/config_flags_test.go +++ b/flyteadmin/auth/config/config_flags_test.go @@ -477,6 +477,34 @@ func TestConfig_SetFlags(t *testing.T) { } }) }) + t.Run("Test_appAuth.externalAuthServer.retryAttempts", func(t *testing.T) { + + t.Run("Override", func(t *testing.T) { + testValue := "1" + + cmdFlags.Set("appAuth.externalAuthServer.retryAttempts", testValue) + if vInt, err := cmdFlags.GetInt("appAuth.externalAuthServer.retryAttempts"); err == nil { + testDecodeJson_Config(t, fmt.Sprintf("%v", vInt), &actual.AppAuth.ExternalAuthServer.RetryAttempts) + + } else { + assert.FailNow(t, err.Error()) + } + }) + }) + t.Run("Test_appAuth.externalAuthServer.retryDelayMilliseconds", func(t *testing.T) { + + t.Run("Override", func(t *testing.T) { + testValue := DefaultConfig.AppAuth.ExternalAuthServer.RetryDelayMilliseconds.String() + + cmdFlags.Set("appAuth.externalAuthServer.retryDelayMilliseconds", testValue) + if vString, err := cmdFlags.GetString("appAuth.externalAuthServer.retryDelayMilliseconds"); err == nil { + testDecodeJson_Config(t, fmt.Sprintf("%v", vString), &actual.AppAuth.ExternalAuthServer.RetryDelayMilliseconds) + + } else { + assert.FailNow(t, err.Error()) + } + }) + }) t.Run("Test_appAuth.thirdPartyConfig.flyteClient.clientId", func(t *testing.T) { t.Run("Override", func(t *testing.T) { From 0a7c7f9810fb866f452c9c0f8f03e2a55167670b Mon Sep 17 00:00:00 2001 From: squiishyy Date: Wed, 18 Oct 2023 11:33:28 -0700 Subject: [PATCH 04/17] imports Signed-off-by: squiishyy --- flyteadmin/auth/authzserver/metadata_provider.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/flyteadmin/auth/authzserver/metadata_provider.go b/flyteadmin/auth/authzserver/metadata_provider.go index 6dd30e5cf7..61802964c6 100644 --- a/flyteadmin/auth/authzserver/metadata_provider.go +++ b/flyteadmin/auth/authzserver/metadata_provider.go @@ -13,12 +13,11 @@ import ( "google.golang.org/grpc/codes" "github.com/flyteorg/flyte/flyteadmin/auth" + authConfig "github.com/flyteorg/flyte/flyteadmin/auth/config" + "github.com/flyteorg/flyte/flyteadmin/pkg/async" flyteErrors "github.com/flyteorg/flyte/flyteadmin/pkg/errors" "github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/service" "github.com/flyteorg/flyte/flytestdlib/logger" - - authConfig "github.com/flyteorg/flyte/flyteadmin/auth/config" - "github.com/flyteorg/flyte/flyteadmin/pkg/async" ) type OAuth2MetadataProvider struct { From 3406652f1cb15fa9d475d78c271991cd2f78e81f Mon Sep 17 00:00:00 2001 From: squiishyy Date: Wed, 18 Oct 2023 11:53:18 -0700 Subject: [PATCH 05/17] updated tests to work Signed-off-by: squiishyy --- flyteadmin/auth/authzserver/metadata_provider_test.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/flyteadmin/auth/authzserver/metadata_provider_test.go b/flyteadmin/auth/authzserver/metadata_provider_test.go index 1ed8b0b56a..ec82c87a0f 100644 --- a/flyteadmin/auth/authzserver/metadata_provider_test.go +++ b/flyteadmin/auth/authzserver/metadata_provider_test.go @@ -115,25 +115,31 @@ func TestOAuth2MetadataProvider_OAuth2Metadata(t *testing.T) { } func TestSendAndRetryHttpRequest(t *testing.T) { + numRetries := 0 hf := func(w http.ResponseWriter, r *http.Request) { switch strings.TrimSpace(r.URL.Path) { case "/": mockExternalMetadataEndpointTransientFailure(w, r) + numRetries++ default: http.NotFoundHandler().ServeHTTP(w, r) } - } server := httptest.NewServer(http.HandlerFunc(hf)) defer server.Close() http.DefaultClient = server.Client() + retryAttempts := 5 - resp, err := sendAndRetryHttpRequest(server.Client(), server.URL, 5, 0) + resp, err := sendAndRetryHttpRequest(server.Client(), server.URL, retryAttempts, 0 /* for testing */) assert.Error(t, err) assert.Equal(t, oauthMetadataFailureErrorMessage, err.Error()) assert.NotNil(t, resp) assert.Equal(t, 500, resp.StatusCode) + + // Will always be 1 more than expected because of needing to run on 0 if + // specifying no retries in config so we will send the request at least 1 time + assert.Equal(t, retryAttempts+1, numRetries) } func mockExternalMetadataEndpointTransientFailure(w http.ResponseWriter, r *http.Request) { From a616eb5606853839e8ad4d5ea23bb52ed8c9a91c Mon Sep 17 00:00:00 2001 From: squiishyy Date: Wed, 18 Oct 2023 13:20:08 -0700 Subject: [PATCH 06/17] added test Signed-off-by: squiishyy --- .../auth/authzserver/metadata_provider.go | 2 +- .../authzserver/metadata_provider_test.go | 10 ++++----- flyteadmin/pkg/async/shared.go | 6 +++--- flyteadmin/pkg/async/shared_test.go | 21 +++++++++++++++++++ 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/flyteadmin/auth/authzserver/metadata_provider.go b/flyteadmin/auth/authzserver/metadata_provider.go index 61802964c6..7faeac4f2c 100644 --- a/flyteadmin/auth/authzserver/metadata_provider.go +++ b/flyteadmin/auth/authzserver/metadata_provider.go @@ -124,7 +124,7 @@ func NewService(config *authConfig.Config) OAuth2MetadataProvider { func sendAndRetryHttpRequest(client *http.Client, url string, retryAttempts int, retryDelay time.Duration) (*http.Response, error) { var response *http.Response var err error - err = async.RetryOnSpecificErrorCodes(retryAttempts, retryDelay, func() (*http.Response, error) { + err = async.RetryOnSpecificErrorCodes(retryAttempts+1, retryDelay, func() (*http.Response, error) { response, err = client.Get(url) return response, err }, isTransientErrorCode) diff --git a/flyteadmin/auth/authzserver/metadata_provider_test.go b/flyteadmin/auth/authzserver/metadata_provider_test.go index ec82c87a0f..47c0a2fea4 100644 --- a/flyteadmin/auth/authzserver/metadata_provider_test.go +++ b/flyteadmin/auth/authzserver/metadata_provider_test.go @@ -115,12 +115,12 @@ func TestOAuth2MetadataProvider_OAuth2Metadata(t *testing.T) { } func TestSendAndRetryHttpRequest(t *testing.T) { - numRetries := 0 + requestAttempts := 0 hf := func(w http.ResponseWriter, r *http.Request) { switch strings.TrimSpace(r.URL.Path) { case "/": mockExternalMetadataEndpointTransientFailure(w, r) - numRetries++ + requestAttempts++ default: http.NotFoundHandler().ServeHTTP(w, r) } @@ -130,16 +130,14 @@ func TestSendAndRetryHttpRequest(t *testing.T) { defer server.Close() http.DefaultClient = server.Client() retryAttempts := 5 + totalAttempts := retryAttempts + 1 // 1 for the inital try resp, err := sendAndRetryHttpRequest(server.Client(), server.URL, retryAttempts, 0 /* for testing */) assert.Error(t, err) assert.Equal(t, oauthMetadataFailureErrorMessage, err.Error()) assert.NotNil(t, resp) assert.Equal(t, 500, resp.StatusCode) - - // Will always be 1 more than expected because of needing to run on 0 if - // specifying no retries in config so we will send the request at least 1 time - assert.Equal(t, retryAttempts+1, numRetries) + assert.Equal(t, totalAttempts, requestAttempts) } func mockExternalMetadataEndpointTransientFailure(w http.ResponseWriter, r *http.Request) { diff --git a/flyteadmin/pkg/async/shared.go b/flyteadmin/pkg/async/shared.go index 26b8268478..96b6ff7b52 100644 --- a/flyteadmin/pkg/async/shared.go +++ b/flyteadmin/pkg/async/shared.go @@ -11,10 +11,10 @@ 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(attempts int, delay time.Duration, f func() (*http.Response, error), IsErrorCodeRetryable func(*http.Response) bool) error { +func RetryOnSpecificErrorCodes(totalAttempts int, delay time.Duration, f func() (*http.Response, error), IsErrorCodeRetryable func(*http.Response) bool) error { var err error var resp *http.Response - for attempt := 0; attempt <= attempts; attempt++ { + for attempt := 1; attempt <= totalAttempts; attempt++ { resp, err = f() if err != nil { return err @@ -23,7 +23,7 @@ func RetryOnSpecificErrorCodes(attempts int, delay time.Duration, f func() (*htt return err } logger.Warningf(context.Background(), - "Failed status code %v on attempt %d of %d", resp.StatusCode, attempt, attempts) + "Failed status code %v on attempt %d of %d", resp.StatusCode, attempt, totalAttempts) time.Sleep(delay) } return err diff --git a/flyteadmin/pkg/async/shared_test.go b/flyteadmin/pkg/async/shared_test.go index 68ca57bc0d..387352336e 100644 --- a/flyteadmin/pkg/async/shared_test.go +++ b/flyteadmin/pkg/async/shared_test.go @@ -2,6 +2,7 @@ package async import ( "errors" + "net/http" "testing" "time" @@ -46,3 +47,23 @@ func TestRetryOnlyOnRetryableExceptions(t *testing.T) { assert.EqualValues(t, attemptsRecorded, 2) assert.EqualError(t, err, "not-foo") } + +func TestRetryOnlyOnRetryableErrorCodes(t *testing.T) { + attemptsRecorded := 0 + var response *http.Response + err := RetryOnSpecificErrorCodes(3, time.Millisecond, func() (*http.Response, error) { + attemptsRecorded++ + if attemptsRecorded <= 1 { + retryResp := http.Response{StatusCode: 500} + return &retryResp, nil + } + okResp := http.Response{StatusCode: http.StatusOK} + response = &okResp + return &okResp, errors.New("not an error") + }, func(resp *http.Response) bool { + return resp.StatusCode == 500 + }) + assert.Equal(t, 2, attemptsRecorded) + assert.EqualError(t, err, "not an error") + assert.Equal(t, http.StatusOK, response.StatusCode) +} From 211d62910ae48fffe43b15fc74c8dcff9ad9b3bf Mon Sep 17 00:00:00 2001 From: squiishyy Date: Wed, 18 Oct 2023 14:51:49 -0700 Subject: [PATCH 07/17] spelling and info Signed-off-by: squiishyy --- flyteadmin/auth/authzserver/metadata_provider.go | 2 +- flyteadmin/auth/authzserver/metadata_provider_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/flyteadmin/auth/authzserver/metadata_provider.go b/flyteadmin/auth/authzserver/metadata_provider.go index 7faeac4f2c..fe3f69f467 100644 --- a/flyteadmin/auth/authzserver/metadata_provider.go +++ b/flyteadmin/auth/authzserver/metadata_provider.go @@ -79,7 +79,7 @@ func (s OAuth2MetadataProvider) GetOAuth2Metadata(ctx context.Context, r *servic } httpClient.Transport = transport } - logger.Printf(ctx, "retryAttempts: %v retryDuration: %v", s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelayMilliseconds) + logger.Info(ctx, "retryAttempts: %v retryDuration: %v", s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelayMilliseconds) response, err := sendAndRetryHttpRequest(httpClient, externalMetadataURL.String(), s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelayMilliseconds.Duration) if err != nil { if response != nil { diff --git a/flyteadmin/auth/authzserver/metadata_provider_test.go b/flyteadmin/auth/authzserver/metadata_provider_test.go index 47c0a2fea4..b9ac6c1ab2 100644 --- a/flyteadmin/auth/authzserver/metadata_provider_test.go +++ b/flyteadmin/auth/authzserver/metadata_provider_test.go @@ -130,7 +130,7 @@ func TestSendAndRetryHttpRequest(t *testing.T) { defer server.Close() http.DefaultClient = server.Client() retryAttempts := 5 - totalAttempts := retryAttempts + 1 // 1 for the inital try + totalAttempts := retryAttempts + 1 // 1 for the initial try resp, err := sendAndRetryHttpRequest(server.Client(), server.URL, retryAttempts, 0 /* for testing */) assert.Error(t, err) From 25831a7722da441cf2b642ecd45ce9467abc32bf Mon Sep 17 00:00:00 2001 From: squiishyy Date: Wed, 18 Oct 2023 15:03:11 -0700 Subject: [PATCH 08/17] working, removing comment Signed-off-by: squiishyy --- flyteadmin/auth/authzserver/metadata_provider.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flyteadmin/auth/authzserver/metadata_provider.go b/flyteadmin/auth/authzserver/metadata_provider.go index fe3f69f467..29f2e05a4a 100644 --- a/flyteadmin/auth/authzserver/metadata_provider.go +++ b/flyteadmin/auth/authzserver/metadata_provider.go @@ -79,7 +79,7 @@ func (s OAuth2MetadataProvider) GetOAuth2Metadata(ctx context.Context, r *servic } httpClient.Transport = transport } - logger.Info(ctx, "retryAttempts: %v retryDuration: %v", s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelayMilliseconds) + response, err := sendAndRetryHttpRequest(httpClient, externalMetadataURL.String(), s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelayMilliseconds.Duration) if err != nil { if response != nil { From e8bade817f12fdce89b592b0d4cb3af765235ec5 Mon Sep 17 00:00:00 2001 From: squiishyy Date: Wed, 18 Oct 2023 15:55:41 -0700 Subject: [PATCH 09/17] add back Signed-off-by: squiishyy --- flyteadmin/auth/authzserver/metadata_provider.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flyteadmin/auth/authzserver/metadata_provider.go b/flyteadmin/auth/authzserver/metadata_provider.go index 29f2e05a4a..fe3f69f467 100644 --- a/flyteadmin/auth/authzserver/metadata_provider.go +++ b/flyteadmin/auth/authzserver/metadata_provider.go @@ -79,7 +79,7 @@ func (s OAuth2MetadataProvider) GetOAuth2Metadata(ctx context.Context, r *servic } httpClient.Transport = transport } - + logger.Info(ctx, "retryAttempts: %v retryDuration: %v", s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelayMilliseconds) response, err := sendAndRetryHttpRequest(httpClient, externalMetadataURL.String(), s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelayMilliseconds.Duration) if err != nil { if response != nil { From 275a689780f3aa4ac7a99f8cf0ebf0e67c468acd Mon Sep 17 00:00:00 2001 From: squiishyy Date: Wed, 18 Oct 2023 16:44:53 -0700 Subject: [PATCH 10/17] code review Signed-off-by: squiishyy --- flyteadmin/auth/authzserver/metadata_provider.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/flyteadmin/auth/authzserver/metadata_provider.go b/flyteadmin/auth/authzserver/metadata_provider.go index fe3f69f467..d02616c4be 100644 --- a/flyteadmin/auth/authzserver/metadata_provider.go +++ b/flyteadmin/auth/authzserver/metadata_provider.go @@ -82,12 +82,8 @@ func (s OAuth2MetadataProvider) GetOAuth2Metadata(ctx context.Context, r *servic logger.Info(ctx, "retryAttempts: %v retryDuration: %v", s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelayMilliseconds) response, err := sendAndRetryHttpRequest(httpClient, externalMetadataURL.String(), s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelayMilliseconds.Duration) if err != nil { - 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.") - } - logger.Errorf(ctx, "Failed to get oauth metadata. Err: %v", err) - return nil, flyteErrors.NewFlyteAdminError(codes.Code(500), "Failed to get oauth metadata.") + logger.Errorf(ctx, "Failed to get oauth metadata. Error code: %v. Err: %v", response.StatusCode, err) + return nil, flyteErrors.NewFlyteAdminErrorf(codes.Code(response.StatusCode), "Failed to get oauth metadata. Err: %v", err) } raw, err := ioutil.ReadAll(response.Body) From dc74462389e8c19e2b6d8cb7bbf7e455044b2aab Mon Sep 17 00:00:00 2001 From: squiishyy Date: Wed, 18 Oct 2023 18:11:18 -0700 Subject: [PATCH 11/17] code review, refactoring to use retry.onerror Signed-off-by: squiishyy --- .../auth/authzserver/metadata_provider.go | 58 ++++++---- .../authzserver/metadata_provider_test.go | 101 +++++++++++++----- flyteadmin/auth/config/config.go | 10 +- 3 files changed, 121 insertions(+), 48 deletions(-) diff --git a/flyteadmin/auth/authzserver/metadata_provider.go b/flyteadmin/auth/authzserver/metadata_provider.go index d02616c4be..66be1eee25 100644 --- a/flyteadmin/auth/authzserver/metadata_provider.go +++ b/flyteadmin/auth/authzserver/metadata_provider.go @@ -11,10 +11,12 @@ import ( "time" "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/util/retry" "github.com/flyteorg/flyte/flyteadmin/auth" authConfig "github.com/flyteorg/flyte/flyteadmin/auth/config" - "github.com/flyteorg/flyte/flyteadmin/pkg/async" flyteErrors "github.com/flyteorg/flyte/flyteadmin/pkg/errors" "github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/service" "github.com/flyteorg/flyte/flytestdlib/logger" @@ -79,11 +81,15 @@ func (s OAuth2MetadataProvider) GetOAuth2Metadata(ctx context.Context, r *servic } httpClient.Transport = transport } - logger.Info(ctx, "retryAttempts: %v retryDuration: %v", s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelayMilliseconds) - response, err := sendAndRetryHttpRequest(httpClient, externalMetadataURL.String(), s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelayMilliseconds.Duration) + logger.Info(ctx, "retryAttempts: %v retryDuration: %v", s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelay) + response, err := sendAndRetryHttpRequest(ctx, httpClient, externalMetadataURL.String(), s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelay.Duration) if err != nil { - logger.Errorf(ctx, "Failed to get oauth metadata. Error code: %v. Err: %v", response.StatusCode, err) - return nil, flyteErrors.NewFlyteAdminErrorf(codes.Code(response.StatusCode), "Failed to get oauth metadata. Err: %v", err) + if response != nil { + logger.Errorf(ctx, "Failed to get oauth metadata. Error code: %v. Err: %v", response.StatusCode, err) + return nil, flyteErrors.NewFlyteAdminErrorf(codes.Code(response.StatusCode), "Failed to get oauth metadata. Err: %v", err) + } + logger.Errorf(ctx, "Failed to get oauth metadata with no response. Err: %v", err) + return nil, flyteErrors.NewFlyteAdminErrorf(codes.Internal, "Failed to get oauth metadata. Err: %v", err) } raw, err := ioutil.ReadAll(response.Body) @@ -117,28 +123,44 @@ func NewService(config *authConfig.Config) OAuth2MetadataProvider { } } -func sendAndRetryHttpRequest(client *http.Client, url string, retryAttempts int, retryDelay time.Duration) (*http.Response, error) { +func sendAndRetryHttpRequest(ctx context.Context, client *http.Client, url string, retryAttempts int, retryDelay time.Duration) (*http.Response, error) { var response *http.Response var err error - err = async.RetryOnSpecificErrorCodes(retryAttempts+1, retryDelay, func() (*http.Response, error) { - response, err = client.Get(url) - return response, err - }, isTransientErrorCode) + totalAttempts := retryAttempts + 1 // Add 1 for the case where 0 retryAttempts are specified + + backoff := wait.Backoff{ + Duration: retryDelay, + Steps: totalAttempts, + Cap: 0, + } + + // first func is to determine retriability of error + // second is to run logic + err = retry.OnError(backoff, + func(err error) bool { // + if grpcErr := status.Code(err); grpcErr == codes.Internal { + logger.Debugf(ctx, "Failed to get oauth metadata, going to retry. Err: %v", err) + return true + } + return false + }, func() error { // Send HTTP request + response, err = client.Get(url) + if err != nil { + return err + } + if response != nil && response.StatusCode >= http.StatusInternalServerError && response.StatusCode <= http.StatusNetworkAuthenticationRequired { + return flyteErrors.NewFlyteAdminError(codes.Internal, "Failed to get oauth metadata.") + } + return nil + }) if err != nil { return nil, err } if response.StatusCode != http.StatusOK { - return response, errors.New(fmt.Sprint("Failed to get oauth metadata")) + return response, errors.New(fmt.Sprintf("Failed to get oauth metadata with status code %v", response.StatusCode)) } return response, nil } - -func isTransientErrorCode(resp *http.Response) bool { - if resp.StatusCode >= 500 && resp.StatusCode <= 599 { - return true - } - return false -} diff --git a/flyteadmin/auth/authzserver/metadata_provider_test.go b/flyteadmin/auth/authzserver/metadata_provider_test.go index b9ac6c1ab2..cb8a2b5f97 100644 --- a/flyteadmin/auth/authzserver/metadata_provider_test.go +++ b/flyteadmin/auth/authzserver/metadata_provider_test.go @@ -16,7 +16,7 @@ import ( config2 "github.com/flyteorg/flyte/flytestdlib/config" ) -var oauthMetadataFailureErrorMessage = "Failed to get oauth metadata" +var oauthMetadataFailureErrorMessage = "Failed to get oauth metadata." func TestOAuth2MetadataProvider_FlyteClient(t *testing.T) { provider := NewService(&authConfig.Config{ @@ -115,31 +115,82 @@ func TestOAuth2MetadataProvider_OAuth2Metadata(t *testing.T) { } func TestSendAndRetryHttpRequest(t *testing.T) { - requestAttempts := 0 - hf := func(w http.ResponseWriter, r *http.Request) { - switch strings.TrimSpace(r.URL.Path) { - case "/": - mockExternalMetadataEndpointTransientFailure(w, r) - requestAttempts++ - default: - http.NotFoundHandler().ServeHTTP(w, r) + t.Run("Retry into failure", func(t *testing.T) { + requestAttempts := 0 + hf := func(w http.ResponseWriter, r *http.Request) { + switch strings.TrimSpace(r.URL.Path) { + case "/": + w.WriteHeader(500) + requestAttempts++ + default: + http.NotFoundHandler().ServeHTTP(w, r) + } } - } - server := httptest.NewServer(http.HandlerFunc(hf)) - defer server.Close() - http.DefaultClient = server.Client() - retryAttempts := 5 - totalAttempts := retryAttempts + 1 // 1 for the initial try - - resp, err := sendAndRetryHttpRequest(server.Client(), server.URL, retryAttempts, 0 /* for testing */) - assert.Error(t, err) - assert.Equal(t, oauthMetadataFailureErrorMessage, err.Error()) - assert.NotNil(t, resp) - assert.Equal(t, 500, resp.StatusCode) - assert.Equal(t, totalAttempts, requestAttempts) -} + server := httptest.NewServer(http.HandlerFunc(hf)) + defer server.Close() + http.DefaultClient = server.Client() + retryAttempts := 5 + totalAttempts := retryAttempts + 1 // 1 for the initial try + + resp, err := sendAndRetryHttpRequest(context.Background(), server.Client(), server.URL, retryAttempts, 0 /* for testing */) + assert.Error(t, err) + assert.Equal(t, oauthMetadataFailureErrorMessage, err.Error()) + assert.Nil(t, resp) + assert.Equal(t, totalAttempts, requestAttempts) + }) + + t.Run("Retry into success", func(t *testing.T) { + requestAttempts := 0 + hf := func(w http.ResponseWriter, r *http.Request) { + switch strings.TrimSpace(r.URL.Path) { + case "/": + if requestAttempts > 2 { + w.WriteHeader(200) + } else { + requestAttempts++ + w.WriteHeader(500) + } + default: + http.NotFoundHandler().ServeHTTP(w, r) + } + } + + server := httptest.NewServer(http.HandlerFunc(hf)) + defer server.Close() + http.DefaultClient = server.Client() + retryAttempts := 5 + expectedRequestAttempts := 3 + + resp, err := sendAndRetryHttpRequest(context.Background(), server.Client(), server.URL, retryAttempts, 0 /* for testing */) + assert.NoError(t, err) + assert.NotNil(t, resp) + assert.Equal(t, 200, resp.StatusCode) + assert.Equal(t, expectedRequestAttempts, requestAttempts) + }) + + t.Run("Success", func(t *testing.T) { + requestAttempts := 0 + hf := func(w http.ResponseWriter, r *http.Request) { + switch strings.TrimSpace(r.URL.Path) { + case "/": + w.WriteHeader(200) + default: + http.NotFoundHandler().ServeHTTP(w, r) + } + } + + server := httptest.NewServer(http.HandlerFunc(hf)) + defer server.Close() + http.DefaultClient = server.Client() + retryAttempts := 5 + expectedRequestAttempts := 0 + + resp, err := sendAndRetryHttpRequest(context.Background(), server.Client(), server.URL, retryAttempts, 0 /* for testing */) + assert.NoError(t, err) + assert.NotNil(t, resp) + assert.Equal(t, 200, resp.StatusCode) + assert.Equal(t, expectedRequestAttempts, requestAttempts) + }) -func mockExternalMetadataEndpointTransientFailure(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(500) } diff --git a/flyteadmin/auth/config/config.go b/flyteadmin/auth/config/config.go index 8ba4f7aca6..f8c30745bb 100644 --- a/flyteadmin/auth/config/config.go +++ b/flyteadmin/auth/config/config.go @@ -80,8 +80,8 @@ var ( }, AppAuth: OAuth2Options{ ExternalAuthServer: ExternalAuthorizationServer{ - RetryAttempts: 5, - RetryDelayMilliseconds: config.Duration{Duration: 1000 * time.Millisecond}, + RetryAttempts: 5, + RetryDelay: config.Duration{Duration: 1 * time.Second}, }, AuthServerType: AuthorizationServerTypeSelf, ThirdParty: ThirdPartyConfigOptions{ @@ -195,9 +195,9 @@ type ExternalAuthorizationServer struct { AllowedAudience []string `json:"allowedAudience" pflag:",Optional: A list of allowed audiences. If not provided, the audience is expected to be the public Uri of the service."` MetadataEndpointURL config.URL `json:"metadataUrl" pflag:",Optional: If the server doesn't support /.well-known/oauth-authorization-server, you can set a custom metadata url here.'"` // HTTPProxyURL allows operators to access external OAuth2 servers using an external HTTP Proxy - HTTPProxyURL config.URL `json:"httpProxyURL" pflag:",OPTIONAL: HTTP Proxy to be used for OAuth requests."` - RetryAttempts int `json:"retryAttempts" pflag:", Optional: The number of attempted retries on a transient failure to get the OAuth metadata"` - RetryDelayMilliseconds config.Duration `json:"retryDelayMilliseconds" pflag:", Optional, Duration in milliseconds to wait between retries"` + HTTPProxyURL config.URL `json:"httpProxyURL" pflag:",OPTIONAL: HTTP Proxy to be used for OAuth requests."` + RetryAttempts int `json:"retryAttempts" pflag:", Optional: The number of attempted retries on a transient failure to get the OAuth metadata"` + RetryDelay config.Duration `json:"retryDelay" pflag:", Optional, Duration to wait between retries"` } // OAuth2Options defines settings for app auth. From 29accb55a12ca4fb91970541e24ffdce6075cd62 Mon Sep 17 00:00:00 2001 From: squiishyy Date: Thu, 19 Oct 2023 09:40:30 -0700 Subject: [PATCH 12/17] few code review comments, removing unused logic Signed-off-by: squiishyy --- .../auth/authzserver/metadata_provider.go | 11 +++++----- flyteadmin/auth/config/config_flags.go | 2 +- flyteadmin/auth/config/config_flags_test.go | 10 ++++----- flyteadmin/pkg/async/shared.go | 19 ----------------- flyteadmin/pkg/async/shared_test.go | 21 ------------------- 5 files changed, 11 insertions(+), 52 deletions(-) diff --git a/flyteadmin/auth/authzserver/metadata_provider.go b/flyteadmin/auth/authzserver/metadata_provider.go index 66be1eee25..01144acb6a 100644 --- a/flyteadmin/auth/authzserver/metadata_provider.go +++ b/flyteadmin/auth/authzserver/metadata_provider.go @@ -11,7 +11,6 @@ import ( "time" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/util/retry" @@ -134,11 +133,11 @@ func sendAndRetryHttpRequest(ctx context.Context, client *http.Client, url strin Cap: 0, } - // first func is to determine retriability of error - // second is to run logic + // First func is to determine if err is a retryable err. Second is executable logic + retryableOauthMetadataError := flyteErrors.NewFlyteAdminError(codes.Internal, "Failed to get oauth metadata.") err = retry.OnError(backoff, func(err error) bool { // - if grpcErr := status.Code(err); grpcErr == codes.Internal { + if err == retryableOauthMetadataError { logger.Debugf(ctx, "Failed to get oauth metadata, going to retry. Err: %v", err) return true } @@ -149,7 +148,7 @@ func sendAndRetryHttpRequest(ctx context.Context, client *http.Client, url strin return err } if response != nil && response.StatusCode >= http.StatusInternalServerError && response.StatusCode <= http.StatusNetworkAuthenticationRequired { - return flyteErrors.NewFlyteAdminError(codes.Internal, "Failed to get oauth metadata.") + return retryableOauthMetadataError } return nil }) @@ -158,7 +157,7 @@ func sendAndRetryHttpRequest(ctx context.Context, client *http.Client, url strin return nil, err } - if response.StatusCode != http.StatusOK { + if response != nil && response.StatusCode != http.StatusOK { return response, errors.New(fmt.Sprintf("Failed to get oauth metadata with status code %v", response.StatusCode)) } diff --git a/flyteadmin/auth/config/config_flags.go b/flyteadmin/auth/config/config_flags.go index a1cb45b501..4012f98f5d 100755 --- a/flyteadmin/auth/config/config_flags.go +++ b/flyteadmin/auth/config/config_flags.go @@ -78,7 +78,7 @@ func (cfg Config) GetPFlagSet(prefix string) *pflag.FlagSet { cmdFlags.String(fmt.Sprintf("%v%v", prefix, "appAuth.externalAuthServer.metadataUrl"), DefaultConfig.AppAuth.ExternalAuthServer.MetadataEndpointURL.String(), "Optional: If the server doesn't support /.well-known/oauth-authorization-server, you can set a custom metadata url here.'") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "appAuth.externalAuthServer.httpProxyURL"), DefaultConfig.AppAuth.ExternalAuthServer.HTTPProxyURL.String(), "OPTIONAL: HTTP Proxy to be used for OAuth requests.") cmdFlags.Int(fmt.Sprintf("%v%v", prefix, "appAuth.externalAuthServer.retryAttempts"), DefaultConfig.AppAuth.ExternalAuthServer.RetryAttempts, " Optional: The number of attempted retries on a transient failure to get the OAuth metadata") - cmdFlags.String(fmt.Sprintf("%v%v", prefix, "appAuth.externalAuthServer.retryDelayMilliseconds"), DefaultConfig.AppAuth.ExternalAuthServer.RetryDelayMilliseconds.String(), " Optional, Duration in milliseconds to wait between retries") + cmdFlags.String(fmt.Sprintf("%v%v", prefix, "appAuth.externalAuthServer.retryDelay"), DefaultConfig.AppAuth.ExternalAuthServer.RetryDelay.String(), " Optional, Duration to wait between retries") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "appAuth.thirdPartyConfig.flyteClient.clientId"), DefaultConfig.AppAuth.ThirdParty.FlyteClientConfig.ClientID, "public identifier for the app which handles authorization for a Flyte deployment") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "appAuth.thirdPartyConfig.flyteClient.redirectUri"), DefaultConfig.AppAuth.ThirdParty.FlyteClientConfig.RedirectURI, "This is the callback uri registered with the app which handles authorization for a Flyte deployment") cmdFlags.StringSlice(fmt.Sprintf("%v%v", prefix, "appAuth.thirdPartyConfig.flyteClient.scopes"), DefaultConfig.AppAuth.ThirdParty.FlyteClientConfig.Scopes, "Recommended scopes for the client to request.") diff --git a/flyteadmin/auth/config/config_flags_test.go b/flyteadmin/auth/config/config_flags_test.go index 3145c826f2..26fe17dd0e 100755 --- a/flyteadmin/auth/config/config_flags_test.go +++ b/flyteadmin/auth/config/config_flags_test.go @@ -491,14 +491,14 @@ func TestConfig_SetFlags(t *testing.T) { } }) }) - t.Run("Test_appAuth.externalAuthServer.retryDelayMilliseconds", func(t *testing.T) { + t.Run("Test_appAuth.externalAuthServer.retryDelay", func(t *testing.T) { t.Run("Override", func(t *testing.T) { - testValue := DefaultConfig.AppAuth.ExternalAuthServer.RetryDelayMilliseconds.String() + testValue := DefaultConfig.AppAuth.ExternalAuthServer.RetryDelay.String() - cmdFlags.Set("appAuth.externalAuthServer.retryDelayMilliseconds", testValue) - if vString, err := cmdFlags.GetString("appAuth.externalAuthServer.retryDelayMilliseconds"); err == nil { - testDecodeJson_Config(t, fmt.Sprintf("%v", vString), &actual.AppAuth.ExternalAuthServer.RetryDelayMilliseconds) + cmdFlags.Set("appAuth.externalAuthServer.retryDelay", testValue) + if vString, err := cmdFlags.GetString("appAuth.externalAuthServer.retryDelay"); err == nil { + testDecodeJson_Config(t, fmt.Sprintf("%v", vString), &actual.AppAuth.ExternalAuthServer.RetryDelay) } else { assert.FailNow(t, err.Error()) diff --git a/flyteadmin/pkg/async/shared.go b/flyteadmin/pkg/async/shared.go index 96b6ff7b52..9fafb50479 100644 --- a/flyteadmin/pkg/async/shared.go +++ b/flyteadmin/pkg/async/shared.go @@ -2,7 +2,6 @@ package async import ( "context" - "net/http" "time" "github.com/flyteorg/flyte/flytestdlib/logger" @@ -11,24 +10,6 @@ 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 { - var err error - var resp *http.Response - for attempt := 1; attempt <= totalAttempts; attempt++ { - resp, err = f() - if err != nil { - return err - } - if !IsErrorCodeRetryable(resp) { - return err - } - logger.Warningf(context.Background(), - "Failed status code %v on attempt %d of %d", resp.StatusCode, attempt, totalAttempts) - time.Sleep(delay) - } - return err -} - func RetryOnSpecificErrors(attempts int, delay time.Duration, f func() error, IsErrorRetryable func(error) bool) error { var err error for attempt := 0; attempt <= attempts; attempt++ { diff --git a/flyteadmin/pkg/async/shared_test.go b/flyteadmin/pkg/async/shared_test.go index 387352336e..68ca57bc0d 100644 --- a/flyteadmin/pkg/async/shared_test.go +++ b/flyteadmin/pkg/async/shared_test.go @@ -2,7 +2,6 @@ package async import ( "errors" - "net/http" "testing" "time" @@ -47,23 +46,3 @@ func TestRetryOnlyOnRetryableExceptions(t *testing.T) { assert.EqualValues(t, attemptsRecorded, 2) assert.EqualError(t, err, "not-foo") } - -func TestRetryOnlyOnRetryableErrorCodes(t *testing.T) { - attemptsRecorded := 0 - var response *http.Response - err := RetryOnSpecificErrorCodes(3, time.Millisecond, func() (*http.Response, error) { - attemptsRecorded++ - if attemptsRecorded <= 1 { - retryResp := http.Response{StatusCode: 500} - return &retryResp, nil - } - okResp := http.Response{StatusCode: http.StatusOK} - response = &okResp - return &okResp, errors.New("not an error") - }, func(resp *http.Response) bool { - return resp.StatusCode == 500 - }) - assert.Equal(t, 2, attemptsRecorded) - assert.EqualError(t, err, "not an error") - assert.Equal(t, http.StatusOK, response.StatusCode) -} From 2f309789efc8efe41295f9ec7cb6d181cbea89aa Mon Sep 17 00:00:00 2001 From: squiishyy Date: Thu, 19 Oct 2023 10:23:00 -0700 Subject: [PATCH 13/17] retry on a wider range Signed-off-by: squiishyy --- flyteadmin/auth/authzserver/metadata_provider.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/flyteadmin/auth/authzserver/metadata_provider.go b/flyteadmin/auth/authzserver/metadata_provider.go index 01144acb6a..d4501b5dee 100644 --- a/flyteadmin/auth/authzserver/metadata_provider.go +++ b/flyteadmin/auth/authzserver/metadata_provider.go @@ -130,24 +130,23 @@ func sendAndRetryHttpRequest(ctx context.Context, client *http.Client, url strin backoff := wait.Backoff{ Duration: retryDelay, Steps: totalAttempts, - Cap: 0, } - // First func is to determine if err is a retryable err. Second is executable logic retryableOauthMetadataError := flyteErrors.NewFlyteAdminError(codes.Internal, "Failed to get oauth metadata.") err = retry.OnError(backoff, - func(err error) bool { // + func(err error) bool { // Determine if error is retryable if err == retryableOauthMetadataError { - logger.Debugf(ctx, "Failed to get oauth metadata, going to retry. Err: %v", err) return true } return false }, func() error { // Send HTTP request response, err = client.Get(url) if err != nil { + logger.Error(ctx, "Failed to send oauth metadata HTTP request. Err: %v", err) return err } - if response != nil && response.StatusCode >= http.StatusInternalServerError && response.StatusCode <= http.StatusNetworkAuthenticationRequired { + if response != nil && response.StatusCode >= http.StatusUnauthorized && response.StatusCode <= http.StatusNetworkAuthenticationRequired { + logger.Errorf(ctx, "Failed to get oauth metadata, going to retry. StatusCode: %v Err: %v", response.StatusCode, err) return retryableOauthMetadataError } return nil From 8ab79187bc4daff478d23a4cd5b1dce34fc86a99 Mon Sep 17 00:00:00 2001 From: squiishyy Date: Thu, 19 Oct 2023 10:37:31 -0700 Subject: [PATCH 14/17] comment Signed-off-by: squiishyy --- flyteadmin/auth/authzserver/metadata_provider.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flyteadmin/auth/authzserver/metadata_provider.go b/flyteadmin/auth/authzserver/metadata_provider.go index d4501b5dee..ae7f037062 100644 --- a/flyteadmin/auth/authzserver/metadata_provider.go +++ b/flyteadmin/auth/authzserver/metadata_provider.go @@ -125,7 +125,7 @@ func NewService(config *authConfig.Config) OAuth2MetadataProvider { func sendAndRetryHttpRequest(ctx context.Context, client *http.Client, url string, retryAttempts int, retryDelay time.Duration) (*http.Response, error) { var response *http.Response var err error - totalAttempts := retryAttempts + 1 // Add 1 for the case where 0 retryAttempts are specified + totalAttempts := retryAttempts + 1 // Add one for initial http request attempt backoff := wait.Backoff{ Duration: retryDelay, From e8b96daca21cbdf5f7ec2677d94e9858a38e3e9b Mon Sep 17 00:00:00 2001 From: squiishyy Date: Thu, 19 Oct 2023 12:30:54 -0700 Subject: [PATCH 15/17] updated logs Signed-off-by: squiishyy --- flyteadmin/auth/authzserver/metadata_provider.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/flyteadmin/auth/authzserver/metadata_provider.go b/flyteadmin/auth/authzserver/metadata_provider.go index ae7f037062..e9731e7c2e 100644 --- a/flyteadmin/auth/authzserver/metadata_provider.go +++ b/flyteadmin/auth/authzserver/metadata_provider.go @@ -80,15 +80,10 @@ func (s OAuth2MetadataProvider) GetOAuth2Metadata(ctx context.Context, r *servic } httpClient.Transport = transport } - logger.Info(ctx, "retryAttempts: %v retryDuration: %v", s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelay) + response, err := sendAndRetryHttpRequest(ctx, httpClient, externalMetadataURL.String(), s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelay.Duration) if err != nil { - if response != nil { - logger.Errorf(ctx, "Failed to get oauth metadata. Error code: %v. Err: %v", response.StatusCode, err) - return nil, flyteErrors.NewFlyteAdminErrorf(codes.Code(response.StatusCode), "Failed to get oauth metadata. Err: %v", err) - } - logger.Errorf(ctx, "Failed to get oauth metadata with no response. Err: %v", err) - return nil, flyteErrors.NewFlyteAdminErrorf(codes.Internal, "Failed to get oauth metadata. Err: %v", err) + return nil, err } raw, err := ioutil.ReadAll(response.Body) @@ -142,7 +137,7 @@ func sendAndRetryHttpRequest(ctx context.Context, client *http.Client, url strin }, func() error { // Send HTTP request response, err = client.Get(url) if err != nil { - logger.Error(ctx, "Failed to send oauth metadata HTTP request. Err: %v", err) + logger.Errorf(ctx, "Failed to send oauth metadata HTTP request. Err: %v", err) return err } if response != nil && response.StatusCode >= http.StatusUnauthorized && response.StatusCode <= http.StatusNetworkAuthenticationRequired { From 4772c0cae7f3a878035810342308e5b87a9ec64c Mon Sep 17 00:00:00 2001 From: squiishyy Date: Thu, 19 Oct 2023 12:52:54 -0700 Subject: [PATCH 16/17] lint Signed-off-by: squiishyy --- flyteadmin/auth/authzserver/metadata_provider.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/flyteadmin/auth/authzserver/metadata_provider.go b/flyteadmin/auth/authzserver/metadata_provider.go index e9731e7c2e..b0f8b750bf 100644 --- a/flyteadmin/auth/authzserver/metadata_provider.go +++ b/flyteadmin/auth/authzserver/metadata_provider.go @@ -2,7 +2,6 @@ package authzserver import ( "context" - "errors" "fmt" "io/ioutil" "net/http" @@ -130,10 +129,7 @@ func sendAndRetryHttpRequest(ctx context.Context, client *http.Client, url strin retryableOauthMetadataError := flyteErrors.NewFlyteAdminError(codes.Internal, "Failed to get oauth metadata.") err = retry.OnError(backoff, func(err error) bool { // Determine if error is retryable - if err == retryableOauthMetadataError { - return true - } - return false + return err == retryableOauthMetadataError }, func() error { // Send HTTP request response, err = client.Get(url) if err != nil { @@ -152,7 +148,7 @@ func sendAndRetryHttpRequest(ctx context.Context, client *http.Client, url strin } if response != nil && response.StatusCode != http.StatusOK { - return response, errors.New(fmt.Sprintf("Failed to get oauth metadata with status code %v", response.StatusCode)) + return response, fmt.Errorf("failed to get oauth metadata with status code %v", response.StatusCode) } return response, nil From 59cc2ff1058b3f79f778810dc3ee4b4488432d06 Mon Sep 17 00:00:00 2001 From: squiishyy Date: Thu, 19 Oct 2023 13:16:48 -0700 Subject: [PATCH 17/17] linting 2 Signed-off-by: squiishyy --- flyteadmin/auth/authzserver/metadata_provider.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/flyteadmin/auth/authzserver/metadata_provider.go b/flyteadmin/auth/authzserver/metadata_provider.go index b0f8b750bf..8a0acd906c 100644 --- a/flyteadmin/auth/authzserver/metadata_provider.go +++ b/flyteadmin/auth/authzserver/metadata_provider.go @@ -80,7 +80,7 @@ func (s OAuth2MetadataProvider) GetOAuth2Metadata(ctx context.Context, r *servic httpClient.Transport = transport } - response, err := sendAndRetryHttpRequest(ctx, httpClient, externalMetadataURL.String(), s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelay.Duration) + response, err := sendAndRetryHTTPRequest(ctx, httpClient, externalMetadataURL.String(), s.cfg.AppAuth.ExternalAuthServer.RetryAttempts, s.cfg.AppAuth.ExternalAuthServer.RetryDelay.Duration) if err != nil { return nil, err } @@ -116,7 +116,7 @@ func NewService(config *authConfig.Config) OAuth2MetadataProvider { } } -func sendAndRetryHttpRequest(ctx context.Context, client *http.Client, url string, retryAttempts int, retryDelay time.Duration) (*http.Response, error) { +func sendAndRetryHTTPRequest(ctx context.Context, client *http.Client, url string, retryAttempts int, retryDelay time.Duration) (*http.Response, error) { var response *http.Response var err error totalAttempts := retryAttempts + 1 // Add one for initial http request attempt