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.

Sign up for GitHub

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

[Core feature] Allow flyteadmin to start even if OIDC is unavailable (Improve flyteadmin startup resiliency) #5702

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+58 −33
Open

[Core feature] Allow flyteadmin to start even if OIDC is unavailable (Improve flyteadmin startup resiliency) #5702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
85 changes: 53 additions & 32 deletions flyteadmin/auth/auth_context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,11 @@
httpClient *http.Client
}

func (c Context) OAuth2Provider() interfaces.OAuth2Provider {
func (c *Context) OAuth2Provider() interfaces.OAuth2Provider {

Check warning on line 53 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L53 

Added line #L53 was not covered by tests
return c.oauth2Provider
}

func (c Context) OAuth2ClientConfig(requestURL *url.URL) *oauth2.Config {
func (c *Context) OAuth2ClientConfig(requestURL *url.URL) *oauth2.Config {
if requestURL == nil || strings.HasPrefix(c.oauth2Client.RedirectURL, requestURL.ResolveReference(rootRelativeURL).String()) {
return c.oauth2Client
}
Expand All @@ -68,64 +68,86 @@
}
}

func (c Context) OidcProvider() *oidc.Provider {
func (c *Context) OidcProvider() *oidc.Provider {
if c.oidcProvider == nil {
logger.Warn(context.Background(), "OIDC provider was not initialized. Attempting to re-initialize OIDC provider...")
err := c.InitOIDC()
if err != nil {
logger.Warn(context.Background(), err)

Check warning on line 76 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L71-L76 

Added lines #L71 - L76 were not covered by tests
}
}
return c.oidcProvider
}

func (c Context) CookieManager() interfaces.CookieHandler {
func (c *Context) InitOIDC() error {
var err error

Check warning on line 83 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L82-L83 

Added lines #L82 - L83 were not covered by tests
// Construct an oidc Provider, which needs its own http Client.
oidcCtx := oidc.ClientContext(context.Background(), c.httpClient)
baseURL := c.options.UserAuth.OpenID.BaseURL.String()

Check warning on line 86 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L85-L86 

Added lines #L85 - L86 were not covered by tests

c.oidcProvider, err = oidc.NewProvider(oidcCtx, baseURL)
if err != nil {
return errors.Wrapf(ErrauthCtx, err, "Error creating oidc provider w/ issuer [%v]", baseURL)

Check warning on line 90 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L88-L90 

Added lines #L88 - L90 were not covered by tests
}

c.oauth2Client.Endpoint = c.oidcProvider.Endpoint()
return nil

Check warning on line 94 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L93-L94 

Added lines #L93 - L94 were not covered by tests
}

func (c *Context) CookieManager() interfaces.CookieHandler {

Check warning on line 97 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L97 

Added line #L97 was not covered by tests
return c.cookieManager
}

func (c Context) Options() *config.Config {
func (c *Context) Options() *config.Config {

Check warning on line 101 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L101 

Added line #L101 was not covered by tests
return c.options
}

func (c Context) GetUserInfoURL() *url.URL {
func (c *Context) GetUserInfoURL() *url.URL {

Check warning on line 105 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L105 

Added line #L105 was not covered by tests
return c.userInfoURL
}

func (c Context) GetHTTPClient() *http.Client {
func (c *Context) GetHTTPClient() *http.Client {

Check warning on line 109 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L109 

Added line #L109 was not covered by tests
return c.httpClient
}

func (c Context) GetOAuth2MetadataURL() *url.URL {
func (c *Context) GetOAuth2MetadataURL() *url.URL {

Check warning on line 113 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L113 

Added line #L113 was not covered by tests
return c.oauth2MetadataURL
}

func (c Context) GetOIdCMetadataURL() *url.URL {
func (c *Context) GetOIdCMetadataURL() *url.URL {

Check warning on line 117 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L117 

Added line #L117 was not covered by tests
return c.oidcMetadataURL
}

func (c Context) AuthMetadataService() service.AuthMetadataServiceServer {
func (c *Context) AuthMetadataService() service.AuthMetadataServiceServer {

Check warning on line 121 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L121 

Added line #L121 was not covered by tests
return c.authServiceImpl
}

func (c Context) IdentityService() service.IdentityServiceServer {
func (c *Context) IdentityService() service.IdentityServiceServer {

Check warning on line 125 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L125 

Added line #L125 was not covered by tests
return c.identityServiceIml
}

func (c Context) OAuth2ResourceServer() interfaces.OAuth2ResourceServer {
func (c *Context) OAuth2ResourceServer() interfaces.OAuth2ResourceServer {

Check warning on line 129 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L129 

Added line #L129 was not covered by tests
return c.oauth2ResourceServer
}
func NewAuthenticationContext(ctx context.Context, sm core.SecretManager, oauth2Provider interfaces.OAuth2Provider,
oauth2ResourceServer interfaces.OAuth2ResourceServer, authMetadataService service.AuthMetadataServiceServer,
identityService service.IdentityServiceServer, options *config.Config) (Context, error) {
identityService service.IdentityServiceServer, options *config.Config) (*Context, error) {

Check warning on line 134 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L134 

Added line #L134 was not covered by tests

// Construct the cookie manager object.
hashKeyBase64, err := sm.Get(ctx, options.UserAuth.CookieHashKeySecretName)
if err != nil {
return Context{}, errors.Wrapf(ErrConfigFileRead, err, "Could not read hash key file")
return &Context{}, errors.Wrapf(ErrConfigFileRead, err, "Could not read hash key file")

Check warning on line 139 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L139 

Added line #L139 was not covered by tests
}

blockKeyBase64, err := sm.Get(ctx, options.UserAuth.CookieBlockKeySecretName)
if err != nil {
return Context{}, errors.Wrapf(ErrConfigFileRead, err, "Could not read hash key file")
return &Context{}, errors.Wrapf(ErrConfigFileRead, err, "Could not read hash key file")

Check warning on line 144 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L144 

Added line #L144 was not covered by tests
}

cookieManager, err := NewCookieManager(ctx, hashKeyBase64, blockKeyBase64, options.UserAuth.CookieSetting)
if err != nil {
logger.Errorf(ctx, "Error creating cookie manager %s", err)
return Context{}, errors.Wrapf(ErrauthCtx, err, "Error creating cookie manager")
return &Context{}, errors.Wrapf(ErrauthCtx, err, "Error creating cookie manager")

Check warning on line 150 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L150 

Added line #L150 was not covered by tests
}

// Construct an http client for interacting with the IDP if necessary.
Expand All @@ -138,34 +160,26 @@
httpClient.Transport = &http.Transport{Proxy: http.ProxyURL(&options.UserAuth.HTTPProxyURL.URL)}
}

// Construct an oidc Provider, which needs its own http Client.
oidcCtx := oidc.ClientContext(ctx, httpClient)
baseURL := options.UserAuth.OpenID.BaseURL.String()
provider, err := oidc.NewProvider(oidcCtx, baseURL)
if err != nil {
return Context{}, errors.Wrapf(ErrauthCtx, err, "Error creating oidc provider w/ issuer [%v]", baseURL)
}

// Construct the golang OAuth2 library's own internal configuration object from this package's config
oauth2Config, err := GetOAuth2ClientConfig(ctx, options.UserAuth.OpenID, provider.Endpoint(), sm)
oauth2Config, err := GetOAuth2ClientConfig(ctx, options.UserAuth.OpenID, sm)

Check warning on line 164 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L164 

Added line #L164 was not covered by tests
if err != nil {
return Context{}, errors.Wrapf(ErrauthCtx, err, "Error creating OAuth2 library configuration")
return &Context{}, errors.Wrapf(ErrauthCtx, err, "Error creating OAuth2 library configuration")

Check warning on line 166 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L166 

Added line #L166 was not covered by tests
}

logger.Infof(ctx, "Base IDP URL is %s", options.UserAuth.OpenID.BaseURL)

oauth2MetadataURL, err := url.Parse(OAuth2MetadataEndpoint)
if err != nil {
logger.Errorf(ctx, "Error parsing oauth2 metadata URL %s", err)
return Context{}, errors.Wrapf(ErrauthCtx, err, "Error parsing metadata URL")
return &Context{}, errors.Wrapf(ErrauthCtx, err, "Error parsing metadata URL")

Check warning on line 174 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L174 

Added line #L174 was not covered by tests
}

logger.Infof(ctx, "Metadata endpoint is %s", oauth2MetadataURL)

oidcMetadataURL, err := url.Parse(OIdCMetadataEndpoint)
if err != nil {
logger.Errorf(ctx, "Error parsing oidc metadata URL %s", err)
return Context{}, errors.Wrapf(ErrauthCtx, err, "Error parsing metadata URL")
return &Context{}, errors.Wrapf(ErrauthCtx, err, "Error parsing metadata URL")

Check warning on line 182 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L182 

Added line #L182 was not covered by tests
}

logger.Infof(ctx, "Metadata endpoint is %s", oidcMetadataURL)
Expand All @@ -175,7 +189,6 @@
oidcMetadataURL: oidcMetadataURL,
oauth2MetadataURL: oauth2MetadataURL,
oauth2Client: &oauth2Config,
oidcProvider: provider,
httpClient: httpClient,
cookieManager: cookieManager,
oauth2Provider: oauth2Provider,
Expand All @@ -185,11 +198,20 @@
authCtx.authServiceImpl = authMetadataService
authCtx.identityServiceIml = identityService

return authCtx, nil
err = authCtx.InitOIDC()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

authContext logic which depend on OIDC (i.e. oidcProvider, oidcProvider.Endpoint) is extracted out into an initOIDC helper.

if err != nil {
if authCtx.options.UserAuth.OpenID.OnlyStartIfOIDCIsAvailable {
return &Context{}, errors.Wrapf(ErrauthCtx, err, "Error initializing OIDC")
} else {
logger.Warn(ctx, err)

Check warning on line 206 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L201-L206 

Added lines #L201 - L206 were not covered by tests
}
}

return &authCtx, nil

Check warning on line 210 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L210 

Added line #L210 was not covered by tests
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NewAuthenticationContext now returns a Context pointer, this way we can add a reusable oidcProvider to the context, after transient OIDC errors are resolved.

}

// This creates a oauth2 library config object, with values from the Flyte Admin config
func GetOAuth2ClientConfig(ctx context.Context, options config.OpenIDOptions, providerEndpoints oauth2.Endpoint, sm core.SecretManager) (cfg oauth2.Config, err error) {
func GetOAuth2ClientConfig(ctx context.Context, options config.OpenIDOptions, sm core.SecretManager) (cfg oauth2.Config, err error) {

Check warning on line 214 in flyteadmin/auth/auth_context.go

View check run for this annotation

Codecov / codecov/patch

flyteadmin/auth/auth_context.go#L214 

Added line #L214 was not covered by tests
var secret string
if len(options.DeprecatedClientSecretFile) > 0 {
secretBytes, err := ioutil.ReadFile(options.DeprecatedClientSecretFile)
Expand All @@ -212,6 +234,5 @@
ClientID: options.ClientID,
ClientSecret: secret,
Scopes: options.Scopes,
Endpoint: providerEndpoints,
}, nil
}
2 changes: 1 addition & 1 deletion flyteadmin/auth/authzserver/initialize_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
func TestRegisterHandlers(t *testing.T) {
t.Run("No OAuth2 Provider, no registration required", func(t *testing.T) {
registerer := &mocks.HandlerRegisterer{}
RegisterHandlers(registerer, auth.Context{})
RegisterHandlers(registerer, &auth.Context{})
})

t.Run("Register 4 endpoints", func(t *testing.T) {
Expand Down
4 changes: 4 additions & 0 deletions flyteadmin/auth/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ var (
"openid",
"profile",
},
OnlyStartIfOIDCIsAvailable: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Configurable onlyStartIfOIDCIsAvailable is true by default. This matches the behavior today, where if OIDC is unavailable due to e.g. DNS issues, flyteadmin will not start.

For reference, another project which takes a similar approach to OIDC issues/service startup resiliency is https://github.com/juanfont/headscale/blob/main/hscontrol/app.go#L157-L166

},
CookieSetting: CookieSettings{
Domain: "",
Expand Down Expand Up @@ -271,6 +272,9 @@ type OpenIDOptions struct {
// be supported by any OIdC server. Refer to https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims for
// a complete list. Other providers might support additional scopes that you can define in a config.
Scopes []string `json:"scopes"`

// Blocks flyte from starting up until the OIDC provider is healthy and available
OnlyStartIfOIDCIsAvailable bool `json:"onlyStartIfOIDCIsAvailable"`
}

func GetConfig() *Config {
Expand Down
Loading