From 33dd675a56169f70acef2a3a3c7b5cf2663d4b7b Mon Sep 17 00:00:00 2001 From: Katrina Rogan Date: Tue, 12 Apr 2022 14:37:51 -0700 Subject: [PATCH] Revert "Always register the metadata endpoint (#392)" (#403) This reverts commit f04414e2849f9ac499c309561c643f13c283cd08. --- auth/authzserver/metadata_provider.go | 13 ++++-------- auth/authzserver/metadata_provider_test.go | 23 +++++++++++----------- auth/handlers.go | 5 ++--- go.mod | 2 +- go.sum | 4 ++-- pkg/config/config.go | 6 ------ pkg/config/serverconfig_flags.go | 1 - pkg/config/serverconfig_flags_test.go | 14 ------------- pkg/server/service.go | 12 +++++------ 9 files changed, 26 insertions(+), 54 deletions(-) diff --git a/auth/authzserver/metadata_provider.go b/auth/authzserver/metadata_provider.go index 36f79aa672..6e6ed79a7e 100644 --- a/auth/authzserver/metadata_provider.go +++ b/auth/authzserver/metadata_provider.go @@ -7,8 +7,6 @@ import ( "net/url" "strings" - "github.com/flyteorg/flyteadmin/pkg/config" - "github.com/flyteorg/flyteadmin/auth" authConfig "github.com/flyteorg/flyteadmin/auth/config" @@ -17,11 +15,10 @@ import ( ) type OAuth2MetadataProvider struct { - cfg *authConfig.Config - serverCfg *config.ServerConfig + cfg *authConfig.Config } -// AuthFuncOverride overrides auth func to enforce anonymous access on the implemented APIs +// Override auth func to enforce anonymous access on the implemented APIs // Ref: https://github.com/grpc-ecosystem/go-grpc-middleware/blob/master/auth/auth.go#L31 func (s OAuth2MetadataProvider) AuthFuncOverride(ctx context.Context, fullMethodName string) (context.Context, error) { return ctx, nil @@ -93,13 +90,11 @@ func (s OAuth2MetadataProvider) GetPublicClientConfig(context.Context, *service. RedirectUri: s.cfg.AppAuth.ThirdParty.FlyteClientConfig.RedirectURI, Scopes: s.cfg.AppAuth.ThirdParty.FlyteClientConfig.Scopes, AuthorizationMetadataKey: s.cfg.GrpcAuthorizationHeader, - ServiceHttpEndpoint: s.serverCfg.ServiceHTTPEndpoint.String(), }, nil } -func NewService(serverCfg *config.ServerConfig, config *authConfig.Config) OAuth2MetadataProvider { +func NewService(config *authConfig.Config) OAuth2MetadataProvider { return OAuth2MetadataProvider{ - cfg: config, - serverCfg: serverCfg, + cfg: config, } } diff --git a/auth/authzserver/metadata_provider_test.go b/auth/authzserver/metadata_provider_test.go index b397d2f5a0..527091c71b 100644 --- a/auth/authzserver/metadata_provider_test.go +++ b/auth/authzserver/metadata_provider_test.go @@ -8,17 +8,16 @@ import ( "strings" "testing" - "github.com/flyteorg/flyteadmin/pkg/config" - - stdConfig "github.com/flyteorg/flytestdlib/config" + config2 "github.com/flyteorg/flytestdlib/config" + "github.com/flyteorg/flyteadmin/auth/config" authConfig "github.com/flyteorg/flyteadmin/auth/config" "github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/service" "github.com/stretchr/testify/assert" ) func TestOAuth2MetadataProvider_FlyteClient(t *testing.T) { - provider := NewService(&config.ServerConfig{}, &authConfig.Config{ + provider := NewService(&authConfig.Config{ AppAuth: authConfig.OAuth2Options{ ThirdParty: authConfig.ThirdPartyConfigOptions{ FlyteClientConfig: authConfig.FlyteClientConfig{ @@ -40,8 +39,8 @@ func TestOAuth2MetadataProvider_FlyteClient(t *testing.T) { func TestOAuth2MetadataProvider_OAuth2Metadata(t *testing.T) { t.Run("Self AuthServer", func(t *testing.T) { - provider := NewService(&config.ServerConfig{}, &authConfig.Config{ - AuthorizedURIs: []stdConfig.URL{{URL: *authConfig.MustParseURL("https://issuer/")}}, + provider := NewService(&authConfig.Config{ + AuthorizedURIs: []config2.URL{{URL: *config.MustParseURL("https://issuer/")}}, }) ctx := context.Background() @@ -75,12 +74,12 @@ func TestOAuth2MetadataProvider_OAuth2Metadata(t *testing.T) { http.DefaultClient = s.Client() t.Run("External AuthServer", func(t *testing.T) { - provider := NewService(&config.ServerConfig{}, &authConfig.Config{ - AuthorizedURIs: []stdConfig.URL{{URL: *authConfig.MustParseURL("https://issuer/")}}, + provider := NewService(&authConfig.Config{ + AuthorizedURIs: []config2.URL{{URL: *config.MustParseURL("https://issuer/")}}, AppAuth: authConfig.OAuth2Options{ AuthServerType: authConfig.AuthorizationServerTypeExternal, ExternalAuthServer: authConfig.ExternalAuthorizationServer{ - BaseURL: stdConfig.URL{URL: *authConfig.MustParseURL(s.URL)}, + BaseURL: config2.URL{URL: *config.MustParseURL(s.URL)}, }, }, }) @@ -92,14 +91,14 @@ func TestOAuth2MetadataProvider_OAuth2Metadata(t *testing.T) { }) t.Run("External AuthServer fallback url", func(t *testing.T) { - provider := NewService(&config.ServerConfig{}, &authConfig.Config{ - AuthorizedURIs: []stdConfig.URL{{URL: *authConfig.MustParseURL("https://issuer/")}}, + provider := NewService(&authConfig.Config{ + AuthorizedURIs: []config2.URL{{URL: *config.MustParseURL("https://issuer/")}}, AppAuth: authConfig.OAuth2Options{ AuthServerType: authConfig.AuthorizationServerTypeExternal, }, UserAuth: authConfig.UserAuthConfig{ OpenID: authConfig.OpenIDOptions{ - BaseURL: stdConfig.URL{URL: *authConfig.MustParseURL(s.URL)}, + BaseURL: config2.URL{URL: *config.MustParseURL(s.URL)}, }, }, }) diff --git a/auth/handlers.go b/auth/handlers.go index 5d2a451793..26c9469c01 100644 --- a/auth/handlers.go +++ b/auth/handlers.go @@ -54,9 +54,8 @@ func RegisterHandlers(ctx context.Context, handler interfaces.HandlerRegisterer, handler.HandleFunc("/logout", GetLogoutEndpointHandler(ctx, authCtx)) } -// RefreshTokensIfExists looks for access token and refresh token, if both are present and the access token is expired, -// then attempt to refresh. Otherwise do nothing and proceed to the next handler. If successfully refreshed, proceed to -// the landing page. +// Look for access token and refresh token, if both are present and the access token is expired, then attempt to +// refresh. Otherwise do nothing and proceed to the next handler. If successfully refreshed, proceed to the landing page. func RefreshTokensIfExists(ctx context.Context, authCtx interfaces.AuthenticationContext, authHandler http.HandlerFunc) http.HandlerFunc { return func(writer http.ResponseWriter, request *http.Request) { diff --git a/go.mod b/go.mod index 6b9840d28e..1d6186bf40 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( github.com/cloudevents/sdk-go/v2 v2.8.0 github.com/coreos/go-oidc v2.2.1+incompatible github.com/evanphx/json-patch v4.9.0+incompatible - github.com/flyteorg/flyteidl v0.24.18 + github.com/flyteorg/flyteidl v0.24.17 github.com/flyteorg/flyteplugins v0.10.16 github.com/flyteorg/flytepropeller v0.16.36 github.com/flyteorg/flytestdlib v0.4.23 diff --git a/go.sum b/go.sum index c496e1be1a..b1018df1d3 100644 --- a/go.sum +++ b/go.sum @@ -369,8 +369,8 @@ github.com/felixge/httpsnoop v1.0.1 h1:lvB5Jl89CsZtGIWuTcDM1E/vkVs49/Ml7JJe07l8S github.com/felixge/httpsnoop v1.0.1/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U= github.com/flyteorg/flyteidl v0.23.0/go.mod h1:576W2ViEyjTpT+kEVHAGbrTP3HARNUZ/eCwrNPmdx9U= github.com/flyteorg/flyteidl v0.24.0/go.mod h1:576W2ViEyjTpT+kEVHAGbrTP3HARNUZ/eCwrNPmdx9U= -github.com/flyteorg/flyteidl v0.24.18 h1:5HMlc11OcSzRHR5VRRAac2gH6yO2XFcQ3fWlcl9tj+k= -github.com/flyteorg/flyteidl v0.24.18/go.mod h1:vHSugApgS3hRITIafzQDU8DZD/W8wFRfFcgaFU35Dww= +github.com/flyteorg/flyteidl v0.24.17 h1:Xx70bJbuQGyvS8uAyU4AN74rot6KnzJ9r/L9gcCdEsU= +github.com/flyteorg/flyteidl v0.24.17/go.mod h1:vHSugApgS3hRITIafzQDU8DZD/W8wFRfFcgaFU35Dww= github.com/flyteorg/flyteplugins v0.10.16 h1:rwNI2MACPbcST2O6CEUsNW6bccz7ZLni0GiY3orevfw= github.com/flyteorg/flyteplugins v0.10.16/go.mod h1:YBWV8QnFakDJfLyua8pYddiWqszAqseBKIJPNMERlos= github.com/flyteorg/flytepropeller v0.16.36 h1:5uE8JsutrPVyLVrRJ8BgvhZUOmTBFkEkn5xmIOo21nU= diff --git a/pkg/config/config.go b/pkg/config/config.go index 761d936126..95d094a8bd 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -26,12 +26,6 @@ type ServerConfig struct { DeprecatedThirdPartyConfig authConfig.ThirdPartyConfigOptions `json:"thirdPartyConfig" pflag:",Deprecated please use auth.appAuth.thirdPartyConfig instead."` DataProxy DataProxyConfig `json:"dataProxy" pflag:",Defines data proxy configuration."` - - // ServiceHTTPEndpoint allows specifying the http endpoint this admin instance is accessible on. This is useful - // for when there is no ingress setup or when the service is serving http and grpc over two different ports. - // Setting it here allows gRPC clients to retrieve the http endpoint to be able to present it to end-users (e.g. - // open up the browser to the workflow overview page) - ServiceHTTPEndpoint config.URL `json:"serviceHttpEndpoint" pflag:",Defines the http endpoint the service is accessible at."` } type DataProxyConfig struct { diff --git a/pkg/config/serverconfig_flags.go b/pkg/config/serverconfig_flags.go index d1f36a5d9a..5bb8a48a94 100755 --- a/pkg/config/serverconfig_flags.go +++ b/pkg/config/serverconfig_flags.go @@ -73,6 +73,5 @@ func (cfg ServerConfig) GetPFlagSet(prefix string) *pflag.FlagSet { cmdFlags.String(fmt.Sprintf("%v%v", prefix, "dataProxy.upload.maxExpiresIn"), defaultServerConfig.DataProxy.Upload.MaxExpiresIn.String(), "Maximum allowed expiration duration.") cmdFlags.Int(fmt.Sprintf("%v%v", prefix, "dataProxy.upload.defaultFileNameLength"), defaultServerConfig.DataProxy.Upload.DefaultFileNameLength, "Default length for the generated file name if not provided in the request.") cmdFlags.String(fmt.Sprintf("%v%v", prefix, "dataProxy.upload.storagePrefix"), defaultServerConfig.DataProxy.Upload.StoragePrefix, "Storage prefix to use for all upload requests.") - cmdFlags.String(fmt.Sprintf("%v%v", prefix, "serviceHttpEndpoint"), defaultServerConfig.ServiceHTTPEndpoint.String(), "Defines the http endpoint the service is accessible at.") return cmdFlags } diff --git a/pkg/config/serverconfig_flags_test.go b/pkg/config/serverconfig_flags_test.go index 6d26839d5f..699a656c53 100755 --- a/pkg/config/serverconfig_flags_test.go +++ b/pkg/config/serverconfig_flags_test.go @@ -421,18 +421,4 @@ func TestServerConfig_SetFlags(t *testing.T) { } }) }) - t.Run("Test_serviceHttpEndpoint", func(t *testing.T) { - - t.Run("Override", func(t *testing.T) { - testValue := defaultServerConfig.ServiceHTTPEndpoint.String() - - cmdFlags.Set("serviceHttpEndpoint", testValue) - if vString, err := cmdFlags.GetString("serviceHttpEndpoint"); err == nil { - testDecodeJson_ServerConfig(t, fmt.Sprintf("%v", vString), &actual.ServiceHTTPEndpoint) - - } else { - assert.FailNow(t, err.Error()) - } - }) - }) } diff --git a/pkg/server/service.go b/pkg/server/service.go index b32baee87f..77dc460cf3 100644 --- a/pkg/server/service.go +++ b/pkg/server/service.go @@ -72,7 +72,7 @@ func blanketAuthorization(ctx context.Context, req interface{}, _ *grpc.UnarySer // Creates a new gRPC Server with all the configuration func newGRPCServer(ctx context.Context, pluginRegistry *plugins.Registry, cfg *config.ServerConfig, - storageCfg *storage.Config, authCfg *authConfig.Config, authCtx interfaces.AuthenticationContext, + storageCfg *storage.Config, authCtx interfaces.AuthenticationContext, scope promutils.Scope, opts ...grpc.ServerOption) (*grpc.Server, error) { // Not yet implemented for streaming var chainedUnaryInterceptors grpc.UnaryServerInterceptor @@ -107,8 +107,8 @@ func newGRPCServer(ctx context.Context, pluginRegistry *plugins.Registry, cfg *c configuration := runtime2.NewConfigurationProvider() service.RegisterAdminServiceServer(grpcServer, adminservice.NewAdminServer(ctx, pluginRegistry, configuration, cfg.KubeConfig, cfg.Master, dataStorageClient, scope.NewSubScope("admin"))) - service.RegisterAuthMetadataServiceServer(grpcServer, authzserver.NewService(cfg, authCfg)) if cfg.Security.UseAuth { + service.RegisterAuthMetadataServiceServer(grpcServer, authCtx.AuthMetadataService()) service.RegisterIdentityServiceServer(grpcServer, authCtx.IdentityService()) } @@ -243,7 +243,7 @@ func serveGatewayInsecure(ctx context.Context, pluginRegistry *plugins.Registry, } } - oauth2MetadataProvider := authzserver.NewService(cfg, authCfg) + oauth2MetadataProvider := authzserver.NewService(authCfg) oidcUserInfoProvider := auth.NewUserInfoProvider() authCtx, err = auth.NewAuthenticationContext(ctx, sm, oauth2Provider, oauth2ResourceServer, oauth2MetadataProvider, oidcUserInfoProvider, authCfg) @@ -253,7 +253,7 @@ func serveGatewayInsecure(ctx context.Context, pluginRegistry *plugins.Registry, } } - grpcServer, err := newGRPCServer(ctx, pluginRegistry, cfg, storageConfig, authCfg, authCtx, scope) + grpcServer, err := newGRPCServer(ctx, pluginRegistry, cfg, storageConfig, authCtx, scope) if err != nil { return fmt.Errorf("failed to create a newGRPCServer. Error: %w", err) } @@ -346,7 +346,7 @@ func serveGatewaySecure(ctx context.Context, pluginRegistry *plugins.Registry, c } } - oauth2MetadataProvider := authzserver.NewService(cfg, authCfg) + oauth2MetadataProvider := authzserver.NewService(authCfg) oidcUserInfoProvider := auth.NewUserInfoProvider() authCtx, err = auth.NewAuthenticationContext(ctx, sm, oauth2Provider, oauth2ResourceServer, oauth2MetadataProvider, oidcUserInfoProvider, authCfg) @@ -356,7 +356,7 @@ func serveGatewaySecure(ctx context.Context, pluginRegistry *plugins.Registry, c } } - grpcServer, err := newGRPCServer(ctx, pluginRegistry, cfg, storageCfg, authCfg, authCtx, scope, grpc.Creds(credentials.NewServerTLSFromCert(cert))) + grpcServer, err := newGRPCServer(ctx, pluginRegistry, cfg, storageCfg, authCtx, scope, grpc.Creds(credentials.NewServerTLSFromCert(cert))) if err != nil { return fmt.Errorf("failed to create a newGRPCServer. Error: %w", err) }