From e379a4a6cfc8cb6cbb889e9a8a32872ca9bcb228 Mon Sep 17 00:00:00 2001 From: Don Browne Date: Wed, 8 May 2024 16:05:41 +0100 Subject: [PATCH] Remove ProviderBuilder (#3282) Fixes #2845 Remove final usages of provider builder in CLI and delete code. --- cmd/dev/app/container/cmd_verify.go | 34 +--- cmd/server/app/webhook_update.go | 133 +++++++++----- internal/providers/providers.go | 275 ---------------------------- 3 files changed, 100 insertions(+), 342 deletions(-) diff --git a/cmd/dev/app/container/cmd_verify.go b/cmd/dev/app/container/cmd_verify.go index 6c43bd59ee..d174c813c9 100644 --- a/cmd/dev/app/container/cmd_verify.go +++ b/cmd/dev/app/container/cmd_verify.go @@ -17,8 +17,6 @@ package container import ( "context" - "database/sql" - "encoding/json" "fmt" "os" "strings" @@ -26,14 +24,15 @@ import ( "github.com/spf13/cobra" "github.com/spf13/viper" - serverconfig "github.com/stacklok/minder/internal/config/server" - "github.com/stacklok/minder/internal/db" - "github.com/stacklok/minder/internal/providers" "github.com/stacklok/minder/internal/providers/credentials" + "github.com/stacklok/minder/internal/providers/github/clients" + "github.com/stacklok/minder/internal/providers/ratecache" + "github.com/stacklok/minder/internal/providers/telemetry" "github.com/stacklok/minder/internal/verifier" "github.com/stacklok/minder/internal/verifier/sigstore" "github.com/stacklok/minder/internal/verifier/sigstore/container" "github.com/stacklok/minder/internal/verifier/verifyif" + minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" provifv1 "github.com/stacklok/minder/pkg/providers/v1" ) @@ -110,26 +109,11 @@ func runCmdVerify(cmd *cobra.Command, _ []string) error { } func buildGitHubClient(token string) (provifv1.GitHub, error) { - pbuild := providers.NewProviderBuilder( - &db.Provider{ - Name: "test", - Version: "v1", - Implements: []db.ProviderType{ - "rest", - "git", - "github", - }, - Definition: json.RawMessage(`{ - "rest": {}, - "github": {} - }`), - }, - sql.NullString{}, - false, + return clients.NewRestClient( + &minderv1.GitHubProviderConfig{}, + &ratecache.NoopRestClientCache{}, credentials.NewGitHubTokenCredential(token), - &serverconfig.ProviderConfig{}, - nil, // this is unused here + clients.NewGitHubClientFactory(telemetry.NewNoopMetrics()), + "", ) - - return pbuild.GetGitHub() } diff --git a/cmd/server/app/webhook_update.go b/cmd/server/app/webhook_update.go index 8269d5a185..bf1129f10f 100644 --- a/cmd/server/app/webhook_update.go +++ b/cmd/server/app/webhook_update.go @@ -35,6 +35,11 @@ import ( "github.com/stacklok/minder/internal/logger" "github.com/stacklok/minder/internal/providers" ghprovider "github.com/stacklok/minder/internal/providers/github" + "github.com/stacklok/minder/internal/providers/github/clients" + ghmanager "github.com/stacklok/minder/internal/providers/github/manager" + "github.com/stacklok/minder/internal/providers/manager" + "github.com/stacklok/minder/internal/providers/ratecache" + "github.com/stacklok/minder/internal/providers/telemetry" provifv1 "github.com/stacklok/minder/pkg/providers/v1" ) @@ -69,23 +74,11 @@ func runCmdWebhookUpdate(cmd *cobra.Command, _ []string) error { providerName := cmd.Flag("provider").Value.String() - cryptoEng, err := crypto.EngineFromAuthConfig(&cfg.Auth) + store, err := wireUpDB(ctx, cfg) if err != nil { - return fmt.Errorf("failed to create crypto engine: %w", err) + return err } - dbConn, _, err := cfg.Database.GetDBConnection(ctx) - if err != nil { - return fmt.Errorf("unable to connect to database: %w", err) - } - defer func(dbConn *sql.DB) { - err := dbConn.Close() - if err != nil { - zerolog.Ctx(ctx).Error().Err(err).Msg("error closing database connection") - } - }(dbConn) - - store := db.NewStore(dbConn) allProviders, err := store.GlobalListProviders(ctx) if err != nil { return fmt.Errorf("unable to list providers: %w", err) @@ -96,16 +89,24 @@ func runCmdWebhookUpdate(cmd *cobra.Command, _ []string) error { return fmt.Errorf("unable to parse webhook url: %w", err) } - fallbackTokenClient := ghprovider.NewFallbackTokenClient(cfg.Provider) + whSecret, err := getWebhookSecret(cfg) + if err != nil { + return err + } + + providerManager, err := wireUpProviderManager(cfg, store) + if err != nil { + return fmt.Errorf("failed to instantiate provider manager: %w", err) + } for _, provider := range allProviders { - zerolog.Ctx(ctx).Info().Str("name", provider.Name).Str("uuid", provider.ID.String()).Msg("provider") - pb, err := providers.GetProviderBuilder(ctx, provider, store, cryptoEng, &cfg.Provider, fallbackTokenClient) - if err != nil { - return fmt.Errorf("unable to get provider builder: %w", err) + if providerName != "" && providerName != provider.Name { + continue } - if !pb.Implements(db.ProviderType(providerName)) { + if !provider.CanImplement(db.ProviderTypeGithub) { + // currently we can only operate on GitHub + // revisit this once we add more providers with webhooks zerolog.Ctx(ctx).Info(). Str("name", provider.Name). Str("uuid", provider.ID.String()). @@ -113,30 +114,29 @@ func runCmdWebhookUpdate(cmd *cobra.Command, _ []string) error { continue } - var updateErr error - - if db.ProviderType(providerName) == db.ProviderTypeGithub { - ghCli, err := pb.GetGitHub() - if err != nil { - zerolog.Ctx(ctx).Err(err).Msg("cannot get github client") - continue - } - - whSecret, err := cfg.WebhookConfig.GetWebhookSecret() - if err != nil { - zerolog.Ctx(ctx).Err(err).Msg("cannot get webhook secret") - continue - } - - if whSecret == "" { - zerolog.Ctx(ctx).Error().Msg("webhook secret is empty") - continue - } - updateErr = updateGithubWebhooks(ctx, ghCli, store, provider, webhookUrl.Host, whSecret) - } else { - updateErr = fmt.Errorf("provider type %s not supported", providerName) + zerolog.Ctx(ctx).Info(). + Str("name", provider.Name). + Str("uuid", provider.ID.String()). + Msg("provider") + + // We end up querying each provider db record twice - once to build + // the slice which this loop iterates over, and a second time to + // instantiate the provider. Taking this approach since we plan on + // changing webhook handling in minder, so I do not want to create any + // throwaway code. + providerInstance, err := providerManager.InstantiateFromID(ctx, provider.ID) + if err != nil { + zerolog.Ctx(ctx).Err(err).Msg("cannot instantiate provider") + continue + } + + ghCli, err := provifv1.As[provifv1.GitHub](providerInstance) + if err != nil { + zerolog.Ctx(ctx).Err(err).Msg("cannot convert to github provider") + continue } + updateErr := updateGithubWebhooks(ctx, ghCli, store, provider, webhookUrl.Host, whSecret) if updateErr != nil { zerolog.Ctx(ctx).Err(updateErr).Msg("unable to update webhooks") } @@ -217,3 +217,52 @@ func updateGithubRepoHooks( return nil } + +func wireUpProviderManager(cfg *serverconfig.Config, store db.Store) (manager.ProviderManager, error) { + cryptoEng, err := crypto.EngineFromAuthConfig(&cfg.Auth) + if err != nil { + return nil, fmt.Errorf("failed to create crypto engine: %w", err) + } + fallbackTokenClient := ghprovider.NewFallbackTokenClient(cfg.Provider) + providerStore := providers.NewProviderStore(store) + githubProviderManager := ghmanager.NewGitHubProviderClassManager( + &ratecache.NoopRestClientCache{}, + clients.NewGitHubClientFactory(telemetry.NewNoopMetrics()), + &cfg.Provider, + fallbackTokenClient, + cryptoEng, + nil, // whManager not needed here (only when creating/delete webhooks) + store, + nil, // ghProviderService not needed here + ) + + return manager.NewProviderManager(providerStore, githubProviderManager) +} + +func wireUpDB(ctx context.Context, cfg *serverconfig.Config) (db.Store, error) { + dbConn, _, err := cfg.Database.GetDBConnection(ctx) + if err != nil { + return nil, fmt.Errorf("unable to connect to database: %w", err) + } + defer func(dbConn *sql.DB) { + err := dbConn.Close() + if err != nil { + zerolog.Ctx(ctx).Error().Err(err).Msg("error closing database connection") + } + }(dbConn) + + return db.NewStore(dbConn), nil +} + +func getWebhookSecret(cfg *serverconfig.Config) (string, error) { + secret, err := cfg.WebhookConfig.GetWebhookSecret() + if err != nil { + return "", fmt.Errorf("cannot read secret from config: %w", err) + } + + if secret == "" { + return "", fmt.Errorf("webhook secret is empty in config: %w", err) + } + + return secret, nil +} diff --git a/internal/providers/providers.go b/internal/providers/providers.go index 4fb80f63f1..150b3e05f6 100644 --- a/internal/providers/providers.go +++ b/internal/providers/providers.go @@ -22,7 +22,6 @@ import ( "errors" "fmt" - gogithub "github.com/google/go-github/v61/github" "github.com/google/uuid" "github.com/rs/zerolog" "golang.org/x/exp/slices" @@ -31,216 +30,11 @@ import ( "github.com/stacklok/minder/internal/crypto" "github.com/stacklok/minder/internal/db" "github.com/stacklok/minder/internal/providers/credentials" - gitclient "github.com/stacklok/minder/internal/providers/git" "github.com/stacklok/minder/internal/providers/github/clients" - httpclient "github.com/stacklok/minder/internal/providers/http" - "github.com/stacklok/minder/internal/providers/ratecache" - "github.com/stacklok/minder/internal/providers/telemetry" minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" provinfv1 "github.com/stacklok/minder/pkg/providers/v1" ) -// ErrInvalidCredential is returned when the credential is not of the required type -var ErrInvalidCredential = errors.New("invalid credential type") - -// GetProviderBuilder is a utility function which allows for the creation of -// a provider manager. -func GetProviderBuilder( - ctx context.Context, - prov db.Provider, - store db.Store, - crypteng crypto.Engine, - provCfg *serverconfig.ProviderConfig, - fallbackTokenClient *gogithub.Client, - opts ...ProviderBuilderOption, -) (*ProviderBuilder, error) { - encToken, err := store.GetAccessTokenByProjectID(ctx, - db.GetAccessTokenByProjectIDParams{Provider: prov.Name, ProjectID: prov.ProjectID}) - if errors.Is(err, sql.ErrNoRows) { - zerolog.Ctx(ctx).Debug().Msg("no access token found for provider") - - // If we don't have an access token, check if we have an installation ID - return createProviderWithInstallationToken(ctx, store, prov, provCfg, fallbackTokenClient, opts...) - } else if err != nil { - return nil, fmt.Errorf("error getting credential: %w", err) - } - - return createProviderWithAccessToken(ctx, encToken, prov, crypteng, provCfg, fallbackTokenClient, opts...) -} - -// ProviderBuilder is a utility struct which allows for the creation of -// provider clients. -type ProviderBuilder struct { - p *db.Provider - ownerFilter sql.NullString // NOTE: we don't seem to actually use the null-ness anywhere. - isOrg bool - restClientCache ratecache.RestClientCache - credential provinfv1.Credential - metrics telemetry.ProviderMetrics - cfg *serverconfig.ProviderConfig - fallbackTokenClient *gogithub.Client -} - -// ProviderBuilderOption is a function which can be used to set options on the ProviderBuilder. -type ProviderBuilderOption func(*ProviderBuilder) - -// WithProviderMetrics sets the metrics for the ProviderBuilder -func WithProviderMetrics(metrics telemetry.ProviderMetrics) ProviderBuilderOption { - return func(pb *ProviderBuilder) { - pb.metrics = metrics - } -} - -// WithRestClientCache sets the rest client cache for the ProviderBuilder -func WithRestClientCache(cache ratecache.RestClientCache) ProviderBuilderOption { - return func(pb *ProviderBuilder) { - pb.restClientCache = cache - } -} - -// NewProviderBuilder creates a new provider builder. -func NewProviderBuilder( - p *db.Provider, - ownerFilter sql.NullString, - isOrg bool, - credential provinfv1.Credential, - cfg *serverconfig.ProviderConfig, - fallbackTokenClient *gogithub.Client, - opts ...ProviderBuilderOption, -) *ProviderBuilder { - pb := &ProviderBuilder{ - p: p, - cfg: cfg, - ownerFilter: ownerFilter, - isOrg: isOrg, - credential: credential, - metrics: telemetry.NewNoopMetrics(), - fallbackTokenClient: fallbackTokenClient, - } - - for _, opt := range opts { - opt(pb) - } - - return pb -} - -// Implements returns true if the provider implements the given type. -func (pb *ProviderBuilder) Implements(impl db.ProviderType) bool { - return pb.p.CanImplement(impl) -} - -// GetName returns the name of the provider instance as defined in the -// database. -func (pb *ProviderBuilder) GetName() string { - return pb.p.Name -} - -// GetGit returns a git client for the provider. -func (pb *ProviderBuilder) GetGit() (provinfv1.Git, error) { - if !pb.Implements(db.ProviderTypeGit) { - return nil, fmt.Errorf("provider does not implement git") - } - - if pb.Implements(db.ProviderTypeGithub) { - return pb.GetGitHub() - } - - gitCredential, ok := pb.credential.(provinfv1.GitCredential) - if !ok { - return nil, ErrInvalidCredential - } - - return gitclient.NewGit(gitCredential), nil -} - -// GetHTTP returns a github client for the provider. -func (pb *ProviderBuilder) GetHTTP() (provinfv1.REST, error) { - if !pb.Implements(db.ProviderTypeRest) { - return nil, fmt.Errorf("provider does not implement rest") - } - - // We can re-use the GitHub provider in case it also implements GitHub. - // The client gives us the ability to handle rate limiting and other - // things. - if pb.Implements(db.ProviderTypeGithub) { - return pb.GetGitHub() - } - - if pb.p.Version != provinfv1.V1 { - return nil, fmt.Errorf("provider version not supported") - } - - // TODO: Parsing will change based on version - cfg, err := httpclient.ParseV1Config(pb.p.Definition) - if err != nil { - return nil, fmt.Errorf("error parsing http config: %w", err) - } - - restCredential, ok := pb.credential.(provinfv1.RestCredential) - if !ok { - return nil, ErrInvalidCredential - } - - return httpclient.NewREST(cfg, pb.metrics, restCredential) -} - -// GetGitHub returns a github client for the provider. -func (pb *ProviderBuilder) GetGitHub() (provinfv1.GitHub, error) { - if !pb.Implements(db.ProviderTypeGithub) { - return nil, fmt.Errorf("provider does not implement github") - } - - if pb.p.Version != provinfv1.V1 { - return nil, fmt.Errorf("provider version not supported") - } - - gitHubCredential, ok := pb.credential.(provinfv1.GitHubCredential) - if !ok { - return nil, ErrInvalidCredential - } - - if pb.restClientCache != nil { - client, ok := pb.restClientCache.Get(pb.ownerFilter.String, gitHubCredential.GetCacheKey(), db.ProviderTypeGithub) - if ok { - return client.(provinfv1.GitHub), nil - } - } - - // TODO: use provider class once it's available - if pb.p.Name == clients.Github { - // TODO: Parsing will change based on version - cfg, err := clients.ParseV1OAuthConfig(pb.p.Definition) - if err != nil { - return nil, fmt.Errorf("error parsing github config: %w", err) - } - - // This should be passed in from the outside, but since I intend to - // get rid of ProviderBuilder, I am instantiating a new copy here - ghClientFactory := clients.NewGitHubClientFactory(pb.metrics) - cli, err := clients.NewRestClient(cfg, pb.restClientCache, gitHubCredential, ghClientFactory, pb.ownerFilter.String) - if err != nil { - return nil, fmt.Errorf("error creating github client: %w", err) - } - return cli, nil - } - - cfg, err := clients.ParseV1AppConfig(pb.p.Definition) - if err != nil { - return nil, fmt.Errorf("error parsing github app config: %w", err) - } - // This should be passed in from the outside, but since I intend to - // get rid of ProviderBuilder, I am instantiating a new copy here - ghClientFactory := clients.NewGitHubClientFactory(pb.metrics) - - cli, err := clients.NewGitHubAppProvider(cfg, pb.cfg.GitHubApp, pb.restClientCache, gitHubCredential, - pb.fallbackTokenClient, ghClientFactory, pb.isOrg) - if err != nil { - return nil, fmt.Errorf("error creating github app client: %w", err) - } - return cli, nil -} - // DBToPBType converts a database provider type to a protobuf provider type. func DBToPBType(t db.ProviderType) (minderv1.ProviderType, bool) { switch t { @@ -363,72 +157,3 @@ func getInstallationTokenCredential( return credentials.NewGitHubInstallationTokenCredential(ctx, provCfg.GitHubApp.AppID, privateKey, cfg.Endpoint, installation.AppInstallationID), nil } - -// createProviderWithAccessToken creates a provider with an access token. -func createProviderWithAccessToken( - ctx context.Context, - encToken db.ProviderAccessToken, - prov db.Provider, - crypteng crypto.Engine, - provCfg *serverconfig.ProviderConfig, - fallbackTokenClient *gogithub.Client, - opts ...ProviderBuilderOption, -) (*ProviderBuilder, error) { - decryptedToken, err := crypteng.DecryptOAuthToken(encToken.EncryptedToken) - if err != nil { - return nil, fmt.Errorf("error decrypting access token: %w", err) - } - zerolog.Ctx(ctx).Debug().Msg("access token found for provider") - - credential := credentials.NewGitHubTokenCredential(decryptedToken.AccessToken) - ownerFilter := encToken.OwnerFilter - isOrg := ownerFilter != sql.NullString{} && ownerFilter.String != "" - - return NewProviderBuilder(&prov, ownerFilter, isOrg, credential, provCfg, fallbackTokenClient, opts...), nil -} - -// createProviderWithAccessToken creates a provider with an installation token. -func createProviderWithInstallationToken( - ctx context.Context, - store db.Store, - prov db.Provider, - provCfg *serverconfig.ProviderConfig, - fallbackTokenClient *gogithub.Client, - opts ...ProviderBuilderOption, -) (*ProviderBuilder, error) { - installation, err := store.GetInstallationIDByProviderID(ctx, uuid.NullUUID{ - UUID: prov.ID, - Valid: true, - }) - - ownerFilter := sql.NullString{} - if errors.Is(err, sql.ErrNoRows) { - // If the provider doesn't have a known credential set the credential to empty - return NewProviderBuilder(&prov, ownerFilter, false, credentials.NewEmptyCredential(), provCfg, - fallbackTokenClient, opts...), nil - } else if err != nil { - return nil, fmt.Errorf("error getting installation ID: %w", err) - } - - cfg, err := clients.ParseV1AppConfig(prov.Definition) - if err != nil { - return nil, fmt.Errorf("error parsing github app config: %w", err) - } - - privateKey, err := provCfg.GitHubApp.GetPrivateKey() - if err != nil { - return nil, fmt.Errorf("error reading private key: %w", err) - } - - credential := credentials.NewGitHubInstallationTokenCredential(ctx, provCfg.GitHubApp.AppID, privateKey, cfg.Endpoint, - installation.AppInstallationID) - - zerolog.Ctx(ctx). - Debug(). - Str("github-app-name", provCfg.GitHubApp.AppName). - Int64("github-app-id", provCfg.GitHubApp.AppID). - Int64("github-app-installation-id", installation.AppInstallationID). - Msg("created provider with installation token") - - return NewProviderBuilder(&prov, ownerFilter, installation.IsOrg, credential, provCfg, fallbackTokenClient, opts...), nil -}