From d68e8e697dcd36f8c1fd9ec9d8a894903c4f2827 Mon Sep 17 00:00:00 2001 From: Don Browne Date: Thu, 14 Mar 2024 09:12:05 +0000 Subject: [PATCH] Move repository create to RepositoryService (#2632) This completes the current refactoring of repository logic. One small behaviour change is introduced: when the create logic is unable to insert the new repo into the DB, it will attempts to delete the webhook which it created. The previous logic would just leave the webhook in the customer's github repo. --- .../controlplane/handlers_githubwebhooks.go | 86 ----- .../controlplane/handlers_repositories.go | 111 ++----- .../handlers_repositories_test.go | 300 +++++++----------- internal/controlplane/server.go | 4 - internal/repositories/github/mock/service.go | 16 + internal/repositories/github/service.go | 151 +++++++++ internal/repositories/github/service_test.go | 187 ++++++++++- 7 files changed, 483 insertions(+), 372 deletions(-) diff --git a/internal/controlplane/handlers_githubwebhooks.go b/internal/controlplane/handlers_githubwebhooks.go index 7659f19f6b..359add114c 100644 --- a/internal/controlplane/handlers_githubwebhooks.go +++ b/internal/controlplane/handlers_githubwebhooks.go @@ -213,92 +213,6 @@ func handleParseError(typ string, parseErr error) *metrics.WebhookEventState { return state } -// registerWebhookForRepository registers a set repository and sets up the webhook for each of them -// and returns the registration result for each repository. -// If an error occurs, the registration is aborted and the error is returned. -// https://docs.github.com/en/rest/reference/repos#create-a-repository-webhook - -// The actual logic for webhook creation lives in the WebhookManager interface -// TODO: the remaining logic should be refactored into a repository -// registration interface -func (s *Server) registerWebhookForRepository( - ctx context.Context, - pbuild *providers.ProviderBuilder, - projectID uuid.UUID, - repo *pb.UpstreamRepositoryRef, -) (*pb.RegisterRepoResult, error) { - logger := zerolog.Ctx(ctx).With(). - Str("repoName", repo.Name). - Str("repoOwner", repo.Owner). - Logger() - ctx = logger.WithContext(ctx) - - if !pbuild.Implements(db.ProviderTypeGithub) { - return nil, fmt.Errorf("provider %s is not supported for github webhook", pbuild.GetName()) - } - - client, err := pbuild.GetGitHub() - if err != nil { - return nil, fmt.Errorf("error creating github provider: %w", err) - } - - regResult := &pb.RegisterRepoResult{ - // We will overwrite this later when we've looked it up from the provider, - // but existing clients expect a message here, so let's add one. - Repository: &pb.Repository{ - Name: repo.Name, // Not normalized, from client - Owner: repo.Owner, // Not normalized, from client - }, - Status: &pb.RegisterRepoResult_Status{ - Success: false, - }, - } - - // let's verify that the repository actually exists. - repoGet, err := client.GetRepository(ctx, repo.Owner, repo.Name) - if err != nil { - errorStr := err.Error() - regResult.Status.Error = &errorStr - return regResult, nil - } - - // skip if we try to register a private repository - if repoGet.GetPrivate() && !features.ProjectAllowsPrivateRepos(ctx, s.store, projectID) { - errorStr := "repository is private" - regResult.Status.Error = &errorStr - return regResult, nil - } - - hookUUID, githubHook, err := s.webhookManager.CreateWebhook(ctx, client, repo.Owner, repo.Name) - if err != nil { - logger.Error().Msgf("error while creating webhook: %v", err) - errorStr := err.Error() - regResult.Status.Error = &errorStr - return regResult, nil - } - - regResult.Status.Success = true - - regResult.Repository = &pb.Repository{ - Name: repoGet.GetName(), - Owner: repoGet.GetOwner().GetLogin(), - RepoId: repoGet.GetID(), - HookId: githubHook.GetID(), - HookUrl: githubHook.GetURL(), - DeployUrl: repoGet.GetDeploymentsURL(), - CloneUrl: repoGet.GetCloneURL(), - HookType: githubHook.GetType(), - HookName: githubHook.GetName(), - HookUuid: hookUUID, - IsPrivate: repoGet.GetPrivate(), - IsFork: repoGet.GetFork(), - DefaultBranch: repoGet.GetDefaultBranch(), - License: repoGet.GetLicense().GetSPDXID(), - } - - return regResult, nil -} - func (s *Server) parseGithubEventForProcessing( rawWHPayload []byte, msg *message.Message, diff --git a/internal/controlplane/handlers_repositories.go b/internal/controlplane/handlers_repositories.go index 75845813e4..103fc3558f 100644 --- a/internal/controlplane/handlers_repositories.go +++ b/internal/controlplane/handlers_repositories.go @@ -32,9 +32,9 @@ import ( "github.com/stacklok/minder/internal/projects/features" "github.com/stacklok/minder/internal/providers" github "github.com/stacklok/minder/internal/providers/github" - "github.com/stacklok/minder/internal/reconcilers" "github.com/stacklok/minder/internal/util" cursorutil "github.com/stacklok/minder/internal/util/cursor" + "github.com/stacklok/minder/internal/util/ptr" pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" v1 "github.com/stacklok/minder/pkg/providers/v1" ) @@ -46,106 +46,41 @@ const maxFetchLimit = 100 // Once a user had enrolled in a project (they have a valid token), they can register // repositories to be monitored by the minder by provisioning a webhook on the // repository(ies). -func (s *Server) RegisterRepository(ctx context.Context, - in *pb.RegisterRepositoryRequest) (*pb.RegisterRepositoryResponse, error) { - entityCtx := engine.EntityFromContext(ctx) - projectID := entityCtx.Project.ID - - provider, err := getProviderFromRequestOrDefault(ctx, s.store, in, projectID) - if err != nil { - return nil, providerError(err) - } - - pbOpts := []providers.ProviderBuilderOption{ - providers.WithProviderMetrics(s.provMt), - providers.WithRestClientCache(s.restClientCache), - } - p, err := providers.GetProviderBuilder(ctx, provider, s.store, s.cryptoEngine, pbOpts...) +func (s *Server) RegisterRepository( + ctx context.Context, + in *pb.RegisterRepositoryRequest, +) (*pb.RegisterRepositoryResponse, error) { + projectID, client, err := s.getProjectIDAndClient(ctx, in) if err != nil { - return nil, status.Errorf(codes.Internal, "cannot get provider builder: %v", err) - } - - // Unmarshal the in.GetRepositories() into a struct Repository - if in.GetRepository() == nil || in.GetRepository().Name == "" { - return nil, util.UserVisibleError(codes.InvalidArgument, "no repository provided") + return nil, err } - repo := in.GetRepository() - - result, err := s.registerWebhookForRepository(ctx, p, projectID, repo) - if err != nil { - return nil, util.UserVisibleError(codes.Internal, "cannot register webhook: %v", err) + // Validate that the Repository struct in the request + repoReference := in.GetRepository() + if repoReference == nil || repoReference.Name == "" || repoReference.Owner == "" { + return nil, util.UserVisibleError(codes.InvalidArgument, "missing repository owner and/or name") } - r := result.Repository - response := &pb.RegisterRepositoryResponse{ - Result: result, - } - - // Convert each result to a pb.Repository object - if result.Status.Error != nil { - return response, nil - } - - // update the database - dbRepo, err := s.store.CreateRepository(ctx, db.CreateRepositoryParams{ - Provider: provider.Name, - ProviderID: provider.ID, - ProjectID: projectID, - RepoOwner: r.Owner, - RepoName: r.Name, - RepoID: r.RepoId, - IsPrivate: r.IsPrivate, - IsFork: r.IsFork, - WebhookID: sql.NullInt64{ - Int64: r.HookId, - Valid: true, - }, - CloneUrl: r.CloneUrl, - WebhookUrl: r.HookUrl, - DeployUrl: r.DeployUrl, - DefaultBranch: sql.NullString{ - String: r.DefaultBranch, - Valid: true, - }, - License: sql.NullString{ - String: r.License, - Valid: true, + Result: &pb.RegisterRepoResult{ + Status: &pb.RegisterRepoResult_Status{ + Success: false, + }, }, - }) - // even if we set the webhook, if we couldn't create it in the database, we'll return an error - if err != nil { - log.Printf("error creating repository '%s/%s' in database: %v", r.Owner, r.Name, err) - - result.Status.Success = false - errorStr := "error creating repository in database" - result.Status.Error = &errorStr - return response, nil } - repoDBID := dbRepo.ID.String() - r.Id = &repoDBID - - // publish a reconciling event for the registered repositories - log.Printf("publishing register event for repository: %s/%s", r.Owner, r.Name) - - msg, err := reconcilers.NewRepoReconcilerMessage(provider.Name, r.RepoId, projectID) + // To be backwards compatible with the existing implementation, we return + // an 200-type response with any errors inside the response body once we + // validate the response body + newRepo, err := s.repos.CreateRepository(ctx, client, projectID, repoReference) if err != nil { - log.Printf("error creating reconciler event: %v", err) + log.Printf("error while registering repository: %v", err) + response.Result.Status.Error = ptr.Ptr(err.Error()) return response, nil } - // This is a non-fatal error, so we'll just log it and continue with the next ones - if err := s.evt.Publish(reconcilers.InternalReconcilerEventTopic, msg); err != nil { - log.Printf("error publishing reconciler event: %v", err) - } - - // Telemetry logging - logger.BusinessRecord(ctx).Provider = provider.Name - logger.BusinessRecord(ctx).Project = projectID - logger.BusinessRecord(ctx).Repository = dbRepo.ID - + response.Result.Status.Success = true + response.Result.Repository = newRepo return response, nil } diff --git a/internal/controlplane/handlers_repositories_test.go b/internal/controlplane/handlers_repositories_test.go index a066bbbd75..1c76edf9d0 100644 --- a/internal/controlplane/handlers_repositories_test.go +++ b/internal/controlplane/handlers_repositories_test.go @@ -17,23 +17,18 @@ package controlplane import ( "context" "database/sql" - "encoding/json" "errors" "net/http" - "reflect" "testing" - "github.com/ThreeDotsLabs/watermill/message" "github.com/go-git/go-git/v5" "github.com/google/go-github/v56/github" "github.com/google/uuid" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" "golang.org/x/oauth2" - "google.golang.org/protobuf/proto" mockdb "github.com/stacklok/minder/database/mock" - "github.com/stacklok/minder/internal/config/server" mockcrypto "github.com/stacklok/minder/internal/crypto/mock" "github.com/stacklok/minder/internal/db" "github.com/stacklok/minder/internal/engine" @@ -41,8 +36,6 @@ import ( "github.com/stacklok/minder/internal/providers/ratecache" ghrepo "github.com/stacklok/minder/internal/repositories/github" mockghrepo "github.com/stacklok/minder/internal/repositories/github/mock" - mockghhook "github.com/stacklok/minder/internal/repositories/github/webhooks/mock" - "github.com/stacklok/minder/internal/util/ptr" pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" provinfv1 "github.com/stacklok/minder/pkg/providers/v1" ) @@ -50,190 +43,106 @@ import ( func TestServer_RegisterRepository(t *testing.T) { t.Parallel() - accessToken := "AUTH" - - tests := []struct { - name string - req *pb.RegisterRepositoryRequest - repo github.Repository - repoErr error - want *pb.RegisterRepositoryResponse - events []message.Message - wantErr bool - }{{ - name: "register repository, invalid upstream ID", - req: &pb.RegisterRepositoryRequest{ - Provider: "github", - Repository: &pb.UpstreamRepositoryRef{ - Owner: "test", - Name: "a-test", - RepoId: 31337, - }, - Context: &pb.Context{ - Provider: proto.String("github"), - Project: proto.String(uuid.NewString()), - }, + scenarios := []struct { + Name string + RepoOwner string + RepoName string + RepoServiceSetup repoMockBuilder + ProviderFails bool + ExpectedError string + }{ + { + Name: "Repo creation fails when provider cannot be found", + RepoOwner: repoOwner, + RepoName: repoName, + ProviderFails: true, + ExpectedError: "cannot retrieve providers", }, - repo: github.Repository{ - Owner: &github.User{ - Login: github.String("test"), - }, - Name: github.String("a-test"), - ID: github.Int64(1234), // NOTE: does not match RPC! + { + Name: "Repo creation fails when repo name is missing", + RepoOwner: repoOwner, + RepoName: "", + ExpectedError: "missing repository owner and/or name", }, - want: &pb.RegisterRepositoryResponse{ - Result: &pb.RegisterRepoResult{ - Repository: &pb.Repository{ - Id: proto.String(uuid.NewString()), - Owner: "test", - Name: "a-test", - RepoId: 1234, - HookId: 1, - HookUuid: uuid.New().String(), - }, - Status: &pb.RegisterRepoResult_Status{ - Success: true, - }, - }, + { + Name: "Repo creation fails when repo owner is missing", + RepoOwner: "", + RepoName: repoName, + ExpectedError: "missing repository owner and/or name", }, - events: []message.Message{{ - Metadata: map[string]string{ - "provider": "github", - }, - Payload: []byte(`{"repository":1234}`), - }}, - }, { - name: "repo not found", - req: &pb.RegisterRepositoryRequest{ - Provider: "github", - Repository: &pb.UpstreamRepositoryRef{ - Owner: "test", - Name: "b-test", - }, - Context: &pb.Context{ - Provider: proto.String("github"), - Project: proto.String(uuid.NewString()), - }, - }, - repoErr: errors.New("Repo not found"), - want: &pb.RegisterRepositoryResponse{ - Result: &pb.RegisterRepoResult{ - // NOTE: the client as of v0.0.31 expects that the Repository - // field is always non-null, even if the repo doesn't exist. - Repository: &pb.Repository{ - Owner: "test", - Name: "b-test", - }, - Status: &pb.RegisterRepoResult_Status{ - Error: proto.String("Repo not found"), - }, - }, - }, - }} - for _, tc := range tests { - tt := tc - t.Run(tt.name, func(t *testing.T) { + Name: "Repo creation fails when repo does not exist in Github", + RepoOwner: repoOwner, + RepoName: repoName, + RepoServiceSetup: newRepoService(withFailedCreate(ghprovider.ErrNotFound)), + ExpectedError: ghprovider.ErrNotFound.Error(), + }, + { + Name: "Repo creation fails repo is private, and private repos are not allowed", + RepoOwner: repoOwner, + RepoName: repoName, + RepoServiceSetup: newRepoService(withFailedCreate(ghrepo.ErrPrivateRepoForbidden)), + ExpectedError: "private repos cannot be registered in this project", + }, + { + Name: "Repo creation on unexpected error", + RepoOwner: repoOwner, + RepoName: repoName, + RepoServiceSetup: newRepoService(withFailedCreate(errDefault)), + ExpectedError: errDefault.Error(), + }, + { + Name: "Repo creation is successful", + RepoOwner: repoOwner, + RepoName: repoName, + RepoServiceSetup: newRepoService(withSuccessfulCreate), + }, + } + + for i := range scenarios { + scenario := scenarios[i] + t.Run(scenario.Name, func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) defer ctrl.Finish() - mockStore := mockdb.NewMockStore(ctrl) - - projectUUID := uuid.MustParse(tt.req.GetContext().GetProject()) - - stubClient := StubGitHub{ - ExpectedOwner: tt.req.Repository.GetOwner(), - ExpectedRepo: tt.req.Repository.GetName(), - T: t, - Repo: &tt.repo, - RepoErr: tt.repoErr, - } - - mockStore.EXPECT(). - GetParentProjects(gomock.Any(), projectUUID). - Return([]uuid.UUID{projectUUID}, nil) - mockStore.EXPECT(). - ListProvidersByProjectID(gomock.Any(), []uuid.UUID{projectUUID}). - Return([]db.Provider{{ - ID: uuid.New(), - Name: "github", - Implements: []db.ProviderType{db.ProviderTypeGithub}, - Version: provinfv1.V1, - }}, nil) - mockStore.EXPECT(). - GetAccessTokenByProjectID(gomock.Any(), gomock.Any()). - Return(db.ProviderAccessToken{ - EncryptedToken: "encryptedToken", - }, nil) - if tt.repoErr == nil { - mockStore.EXPECT(). - CreateRepository(gomock.Any(), gomock.Any()). - Return(db.Repository{ - ID: uuid.MustParse(tt.want.Result.Repository.GetId()), - }, nil) - } - - cancelable, cancel := context.WithCancel(context.Background()) - clientCache := ratecache.NewRestClientCache(cancelable) - defer cancel() - clientCache.Set("", accessToken, db.ProviderTypeGithub, &stubClient) - - stubEventer := StubEventer{} - stubWebhookManager := mockghhook.NewMockWebhookManager(ctrl) - if tt.repoErr == nil { - stubbedHook := &github.Hook{ - ID: ptr.Ptr[int64](1), - } - stubWebhookManager.EXPECT(). - CreateWebhook(gomock.Any(), gomock.Any(), gomock.Eq(tt.req.Repository.Owner), gomock.Eq(tt.req.Repository.Name)). - Return(tt.want.Result.Repository.HookUuid, stubbedHook, nil) - } - - mockCryptoEngine := mockcrypto.NewMockEngine(ctrl) - mockCryptoEngine.EXPECT().DecryptOAuthToken(gomock.Any()).Return(oauth2.Token{AccessToken: accessToken}, nil).AnyTimes() - - s := &Server{ - store: mockStore, - cryptoEngine: mockCryptoEngine, - restClientCache: clientCache, - cfg: &server.Config{}, - evt: &stubEventer, - webhookManager: stubWebhookManager, - } ctx := engine.WithEntityContext(context.Background(), &engine.EntityContext{ - Provider: engine.Provider{Name: tt.req.Context.GetProvider()}, - Project: engine.Project{ID: projectUUID}, + Provider: engine.Provider{Name: ghprovider.Github}, + Project: engine.Project{ID: projectID}, }) - got, err := s.RegisterRepository(ctx, tt.req) - if (err != nil) != tt.wantErr { - t.Errorf("Server.RegisterRepository() error = %v, wantErr %v", err, tt.wantErr) - return - } + server := createServer(ctrl, scenario.RepoServiceSetup, scenario.ProviderFails) - if len(tt.events) != len(stubEventer.Sent) { - t.Fatalf("expected %d events, got %d", len(tt.events), len(stubEventer.Sent)) + req := &pb.RegisterRepositoryRequest{ + Repository: &pb.UpstreamRepositoryRef{ + Owner: scenario.RepoOwner, + Name: scenario.RepoName, + }, } - for i := range tt.events { - got := stubEventer.Sent[i] - want := &tt.events[i] - if !reflect.DeepEqual(got.Metadata, want.Metadata) { - t.Errorf("event %d.Metadata = %+v, want %+v", i, got.Metadata, want.Metadata) - } - var gotPayload, wantPayload EventPayload - if err := json.Unmarshal(got.Payload, &gotPayload); err != nil { - t.Fatalf("failed to unmarshal event %d.Payload: %v", i, err) - } - if err := json.Unmarshal(want.Payload, &wantPayload); err != nil { - t.Fatalf("failed to unmarshal event %d.Payload: %v", i, err) + res, err := server.RegisterRepository(ctx, req) + if scenario.ExpectedError == "" { + expectation := &pb.RegisterRepositoryResponse{ + Result: &pb.RegisterRepoResult{ + Repository: creationResult, + Status: &pb.RegisterRepoResult_Status{ + Success: true, + }, + }, } - if !reflect.DeepEqual(gotPayload.Repository, wantPayload.Repository) { - t.Errorf("event %d.Payload = %q, want %q", i, string(got.Payload), string(want.Payload)) + require.NoError(t, err) + require.Equal(t, res, expectation) + } else { + // due to the mix of error handling styles in this endpoint, we + // need to do some hackery here + var errMsg string + if err != nil { + errMsg = err.Error() + } else if err == nil && res.Result.Status.Success == false && res.Result.Status.Error != nil { + errMsg = *res.Result.Status.Error + } else { + t.Fatal("expected error, but no error was found") } - } - - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("Server.RegisterRepository() = %v, want %v", got, tt.want) + require.True(t, res == nil || res.Result.Status.Success == false) + require.Contains(t, errMsg, scenario.ExpectedError) } }) } @@ -352,14 +261,20 @@ type ( ) const ( + repoOwner = "acme-corp" + repoName = "api-gateway" repoOwnerAndName = "acme-corp/api-gateway" repoID = "3eb6d254-4163-460f-89f7-44e2ae916e71" accessToken = "TOKEN" ) var ( - projectID = uuid.New() - errDefault = errors.New("oh no") + projectID = uuid.New() + errDefault = errors.New("oh no") + creationResult = &pb.Repository{ + Owner: repoOwner, + Name: repoName, + } ) func newRepoService(opts ...func(repoServiceMock)) repoMockBuilder { @@ -372,6 +287,20 @@ func newRepoService(opts ...func(repoServiceMock)) repoMockBuilder { } } +func withSuccessfulCreate(mock repoServiceMock) { + mock.EXPECT(). + CreateRepository(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(creationResult, nil) +} + +func withFailedCreate(err error) func(repoServiceMock) { + return func(mock repoServiceMock) { + mock.EXPECT(). + CreateRepository(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, err) + } +} + func withSuccessfulDeleteByName(mock repoServiceMock) { withFailedDeleteByName(nil)(mock) } @@ -565,17 +494,8 @@ func (*StubGitHub) UpdateReview(context.Context, string, string, int, int64, str } // GetRepository implements v1.GitHub. -func (s *StubGitHub) GetRepository(_ context.Context, owner string, repo string) (*github.Repository, error) { - if owner != s.ExpectedOwner { - s.T.Errorf("expected owner %q, got %q", s.ExpectedOwner, owner) - } - if repo != s.ExpectedRepo { - s.T.Errorf("expected repo %q, got %q", s.ExpectedRepo, repo) - } - if s.RepoErr != nil { - return nil, s.RepoErr - } - return s.Repo, nil +func (*StubGitHub) GetRepository(_ context.Context, _ string, _ string) (*github.Repository, error) { + panic("unimplemented") } // GetCredential implements v1.GitHub. diff --git a/internal/controlplane/server.go b/internal/controlplane/server.go index 1c11bde5a3..879c6cca79 100644 --- a/internal/controlplane/server.go +++ b/internal/controlplane/server.go @@ -92,9 +92,6 @@ type Server struct { profileValidator *profiles.Validator ruleTypes ruletypes.RuleTypeService repos github.RepositoryService - // TODO: this will be removed from server when the create repo - // flow is refactored - webhookManager webhooks.WebhookManager // Implementations for service registration pb.UnimplementedHealthServiceServer @@ -165,7 +162,6 @@ func NewServer( profileValidator: profiles.NewValidator(store), ruleTypes: ruletypes.NewRuleTypeService(store), repos: github.NewRepositoryService(whManager, store, evt), - webhookManager: whManager, // TODO: this currently always returns authorized as a transitionary measure. // When OpenFGA is fully rolled out, we may want to make this a hard error or set to false. authzClient: &mock.NoopClient{Authorized: true}, diff --git a/internal/repositories/github/mock/service.go b/internal/repositories/github/mock/service.go index eaae5a55a3..b262264b0d 100644 --- a/internal/repositories/github/mock/service.go +++ b/internal/repositories/github/mock/service.go @@ -15,6 +15,7 @@ import ( uuid "github.com/google/uuid" clients "github.com/stacklok/minder/internal/repositories/github/clients" + v1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" gomock "go.uber.org/mock/gomock" ) @@ -41,6 +42,21 @@ func (m *MockRepositoryService) EXPECT() *MockRepositoryServiceMockRecorder { return m.recorder } +// CreateRepository mocks base method. +func (m *MockRepositoryService) CreateRepository(arg0 context.Context, arg1 clients.GitHubRepoClient, arg2 uuid.UUID, arg3 *v1.UpstreamRepositoryRef) (*v1.Repository, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CreateRepository", arg0, arg1, arg2, arg3) + ret0, _ := ret[0].(*v1.Repository) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// CreateRepository indicates an expected call of CreateRepository. +func (mr *MockRepositoryServiceMockRecorder) CreateRepository(arg0, arg1, arg2, arg3 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateRepository", reflect.TypeOf((*MockRepositoryService)(nil).CreateRepository), arg0, arg1, arg2, arg3) +} + // DeleteRepositoryByID mocks base method. func (m *MockRepositoryService) DeleteRepositoryByID(arg0 context.Context, arg1 clients.GitHubRepoClient, arg2, arg3 uuid.UUID) error { m.ctrl.T.Helper() diff --git a/internal/repositories/github/service.go b/internal/repositories/github/service.go index 2a19c6d015..433ca08118 100644 --- a/internal/repositories/github/service.go +++ b/internal/repositories/github/service.go @@ -17,21 +17,36 @@ package github import ( "context" + "database/sql" + "errors" "fmt" + "github.com/google/go-github/v56/github" "github.com/google/uuid" "github.com/rs/zerolog/log" "github.com/stacklok/minder/internal/db" "github.com/stacklok/minder/internal/events" "github.com/stacklok/minder/internal/logger" + "github.com/stacklok/minder/internal/projects/features" ghprovider "github.com/stacklok/minder/internal/providers/github" + "github.com/stacklok/minder/internal/reconcilers" ghclient "github.com/stacklok/minder/internal/repositories/github/clients" "github.com/stacklok/minder/internal/repositories/github/webhooks" + "github.com/stacklok/minder/internal/util/ptr" + pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" ) // RepositoryService encapsulates logic related to registering and deleting repos type RepositoryService interface { + // CreateRepository registers a GitHub repository, including creating + // a webhook in the repo in GitHub. + CreateRepository( + ctx context.Context, + client ghclient.GitHubRepoClient, + projectID uuid.UUID, + repo *pb.UpstreamRepositoryRef, + ) (*pb.Repository, error) // DeleteRepositoryByName removes the webhook and deletes the repo from the // database. The repo is identified by its name and project. DeleteRepositoryByName( @@ -51,6 +66,13 @@ type RepositoryService interface { ) error } +var ( + // ErrPrivateRepoForbidden is returned when creation fails due to an + // attempt to register a private repo in a project which does not allow + // private repos + ErrPrivateRepoForbidden = errors.New("private repos cannot be registered in this project") +) + type repositoryService struct { webhookManager webhooks.WebhookManager store db.Store @@ -70,6 +92,63 @@ func NewRepositoryService( } } +func (r *repositoryService) CreateRepository( + ctx context.Context, + client ghclient.GitHubRepoClient, + projectID uuid.UUID, + repo *pb.UpstreamRepositoryRef, +) (*pb.Repository, error) { + // get information about the repo from GitHub, and ensure it exists + githubRepo, err := client.GetRepository(ctx, repo.Owner, repo.Name) + if err != nil { + return nil, fmt.Errorf("error retrieving repo from github: %w", err) + } + + // skip if this is a private repo, and private repos are not enabled + if githubRepo.GetPrivate() && !features.ProjectAllowsPrivateRepos(ctx, r.store, projectID) { + return nil, ErrPrivateRepoForbidden + } + + // create a webhook to capture events from the repository + hookUUID, githubHook, err := r.webhookManager.CreateWebhook(ctx, client, repo.Owner, repo.Name) + if err != nil { + return nil, fmt.Errorf("error creating webhook in repo: %w", err) + } + + // insert the repository into the DB + dbID, pbRepo, err := r.persistRepository( + ctx, + githubRepo, + githubHook, + hookUUID, + projectID, + ) + if err != nil { + log.Printf("error creating repository '%s/%s' in database: %v", repo.Owner, repo.Name, err) + // Attempt to clean up the webhook we created earlier. This is a + // best-effort attempt: If it fails, the customer either has to delete + // the hook manually, or it will be deleted the next time the customer + // attempts to register a repo. + cleanupErr := r.webhookManager.DeleteWebhook(ctx, client, repo.Owner, repo.Name, *githubHook.ID) + if cleanupErr != nil { + log.Printf("error deleting new webhook: %v", cleanupErr) + } + return nil, fmt.Errorf("error creating repository in database: %w", err) + } + + // publish a reconciling event for the registered repositories + if err = r.pushReconcilerEvent(pbRepo, projectID); err != nil { + return nil, err + } + + // Telemetry logging + logger.BusinessRecord(ctx).Provider = ghprovider.Github + logger.BusinessRecord(ctx).Project = projectID + logger.BusinessRecord(ctx).Repository = dbID + + return pbRepo, nil +} + func (r *repositoryService) DeleteRepositoryByName( ctx context.Context, client ghclient.GitHubRepoClient, @@ -134,3 +213,75 @@ func (r *repositoryService) deleteRepository(ctx context.Context, client ghclien return nil } + +func (r *repositoryService) pushReconcilerEvent(pbRepo *pb.Repository, projectID uuid.UUID) error { + log.Printf("publishing register event for repository: %s/%s", pbRepo.Owner, pbRepo.Name) + + msg, err := reconcilers.NewRepoReconcilerMessage(ghprovider.Github, pbRepo.RepoId, projectID) + if err != nil { + return fmt.Errorf("error creating reconciler event: %v", err) + } + + // This is a non-fatal error, so we'll just log it and continue with the next ones + if err = r.eventProducer.Publish(reconcilers.InternalReconcilerEventTopic, msg); err != nil { + log.Printf("error publishing reconciler event: %v", err) + } + + return nil +} + +// returns DB PK along with protobuf representation of a repo +func (r *repositoryService) persistRepository( + ctx context.Context, + githubRepo *github.Repository, + githubHook *github.Hook, + hookUUID string, + projectID uuid.UUID, +) (uuid.UUID, *pb.Repository, error) { + // instantiate the response object + pbRepo := &pb.Repository{ + Name: githubRepo.GetName(), + Owner: githubRepo.GetOwner().GetLogin(), + RepoId: githubRepo.GetID(), + HookId: githubHook.GetID(), + HookUrl: githubHook.GetURL(), + DeployUrl: githubRepo.GetDeploymentsURL(), + CloneUrl: githubRepo.GetCloneURL(), + HookType: githubHook.GetType(), + HookName: githubHook.GetName(), + HookUuid: hookUUID, + IsPrivate: githubRepo.GetPrivate(), + IsFork: githubRepo.GetFork(), + DefaultBranch: githubRepo.GetDefaultBranch(), + } + + // update the database + dbRepo, err := r.store.CreateRepository(ctx, db.CreateRepositoryParams{ + // assumption: provider name is always `github.Github` when enrolling + // GitHub repos + Provider: ghprovider.Github, + ProjectID: projectID, + RepoOwner: pbRepo.Owner, + RepoName: pbRepo.Name, + RepoID: pbRepo.RepoId, + IsPrivate: pbRepo.IsPrivate, + IsFork: pbRepo.IsFork, + WebhookID: sql.NullInt64{ + Int64: pbRepo.HookId, + Valid: true, + }, + CloneUrl: pbRepo.CloneUrl, + WebhookUrl: pbRepo.HookUrl, + DeployUrl: pbRepo.DeployUrl, + DefaultBranch: sql.NullString{ + String: pbRepo.DefaultBranch, + Valid: true, + }, + }) + if err != nil { + return uuid.Nil, nil, err + } + + pbRepo.Id = ptr.Ptr(dbRepo.ID.String()) + return dbRepo.ID, pbRepo, nil +} diff --git a/internal/repositories/github/service_test.go b/internal/repositories/github/service_test.go index a1b0af6e83..98be3b6d09 100644 --- a/internal/repositories/github/service_test.go +++ b/internal/repositories/github/service_test.go @@ -17,9 +17,11 @@ package github_test import ( "context" "database/sql" + "encoding/json" "errors" "testing" + gh "github.com/google/go-github/v56/github" "github.com/google/uuid" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" @@ -32,8 +34,102 @@ import ( cf "github.com/stacklok/minder/internal/repositories/github/fixtures" "github.com/stacklok/minder/internal/repositories/github/webhooks" mockghhook "github.com/stacklok/minder/internal/repositories/github/webhooks/mock" + "github.com/stacklok/minder/internal/util/ptr" + pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" ) +func TestRepositoryService_CreateRepository(t *testing.T) { + t.Parallel() + + scenarios := []struct { + Name string + ClientSetup cf.ClientMockBuilder + DBSetup dbMockBuilder + WebhookSetup whMockBuilder + EventsSetup eventMockBuilder + EventSendFails bool + ExpectedError string + }{ + { + Name: "CreateRepository fails when repo cannot be found in GitHub", + ClientSetup: cf.NewClientMock(cf.WithFailedGet), + ExpectedError: "error retrieving repo from github", + }, + { + Name: "CreateRepository fails for private repo in project which disallows private repos", + ClientSetup: cf.NewClientMock(cf.WithSuccessfulGet(privateRepo)), + DBSetup: newDBMock(withPrivateReposDisabled), + ExpectedError: "private repos cannot be registered in this project", + }, + { + Name: "CreateRepository fails when webhook creation fails", + ClientSetup: cf.NewClientMock(cf.WithSuccessfulGet(publicRepo)), + WebhookSetup: newWebhookMock(withFailedWebhookCreate), + ExpectedError: "error creating webhook in repo", + }, + { + Name: "CreateRepository fails when repo cannot be inserted into database", + ClientSetup: cf.NewClientMock(cf.WithSuccessfulGet(publicRepo)), + DBSetup: newDBMock(withFailedCreate), + WebhookSetup: newWebhookMock(withSuccessfulWebhookCreate, withSuccessfulWebhookDelete), + ExpectedError: "error creating repository", + }, + { + Name: "CreateRepository fails when repo cannot be inserted into database (cleanup fails)", + ClientSetup: cf.NewClientMock(cf.WithSuccessfulGet(publicRepo)), + DBSetup: newDBMock(withFailedCreate), + WebhookSetup: newWebhookMock(withSuccessfulWebhookCreate, withFailedWebhookDelete), + ExpectedError: "error creating repository", + }, + { + Name: "CreateRepository succeeds", + ClientSetup: cf.NewClientMock(cf.WithSuccessfulGet(publicRepo)), + DBSetup: newDBMock(withSuccessfulCreate), + WebhookSetup: newWebhookMock(withSuccessfulWebhookCreate), + }, + { + Name: "CreateRepository succeeds (private repos enabled)", + ClientSetup: cf.NewClientMock(cf.WithSuccessfulGet(privateRepo)), + DBSetup: newDBMock(withPrivateReposEnabled, withSuccessfulCreate), + WebhookSetup: newWebhookMock(withSuccessfulWebhookCreate), + }, + { + Name: "CreateRepository succeeds (skips failed event send)", + ClientSetup: cf.NewClientMock(cf.WithSuccessfulGet(publicRepo)), + DBSetup: newDBMock(withSuccessfulCreate), + WebhookSetup: newWebhookMock(withSuccessfulWebhookCreate), + EventSendFails: true, + }, + } + + for i := range scenarios { + scenario := scenarios[i] + t.Run(scenario.Name, func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + ctx := context.Background() + + var ghClient clients.GitHubRepoClient + if scenario.ClientSetup != nil { + ghClient = scenario.ClientSetup(ctrl) + } + + svc := createService(ctrl, scenario.WebhookSetup, scenario.DBSetup, scenario.EventSendFails) + res, err := svc.CreateRepository(ctx, ghClient, projectID, upstreamRepo) + if scenario.ExpectedError == "" { + require.NoError(t, err) + // cheat here a little... + expectation := newExpectation(res.IsPrivate) + require.Equal(t, expectation, res) + } else { + require.Nil(t, res) + require.ErrorContains(t, err, scenario.ExpectedError) + } + }) + } +} + func TestRepositoryService_DeleteRepository(t *testing.T) { t.Parallel() @@ -167,7 +263,9 @@ const ( ) var ( + hookUUID = uuid.New().String() repoID = uuid.New() + ghRepoID = ptr.Ptr[int64](0xE1E10) projectID = uuid.New() errDefault = errors.New("uh oh") dbRepo = db.Repository{ @@ -180,13 +278,24 @@ var ( Int64: cf.HookID, }, } + upstreamRepo = ptr.Ptr(pb.UpstreamRepositoryRef{ + Owner: repoOwner, + Name: repoName, + }) + webhook = &gh.Hook{ + ID: ptr.Ptr[int64](cf.HookID), + } + publicRepo = newGithubRepo(false) + privateRepo = newGithubRepo(true) ) type ( - dbMock = *mockdb.MockStore - dbMockBuilder = func(controller *gomock.Controller) dbMock - whMock = *mockghhook.MockWebhookManager - whMockBuilder = func(controller *gomock.Controller) whMock + dbMock = *mockdb.MockStore + dbMockBuilder = func(controller *gomock.Controller) dbMock + whMock = *mockghhook.MockWebhookManager + whMockBuilder = func(controller *gomock.Controller) whMock + eventMock = *mockevents.MockInterface + eventMockBuilder = func(controller *gomock.Controller) eventMock ) func newDBMock(opts ...func(dbMock)) dbMockBuilder { @@ -209,6 +318,18 @@ func newWebhookMock(opts ...func(mock whMock)) whMockBuilder { } } +func withSuccessfulWebhookCreate(mock whMock) { + mock.EXPECT(). + CreateWebhook(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(hookUUID, webhook, nil) +} + +func withFailedWebhookCreate(mock whMock) { + mock.EXPECT(). + CreateWebhook(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return("", nil, errDefault) +} + func withSuccessfulWebhookDelete(mock whMock) { mock.EXPECT(). DeleteWebhook(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). @@ -257,3 +378,61 @@ func withSuccessfulDelete(mock dbMock) { DeleteRepository(gomock.Any(), gomock.Eq(repoID)). Return(nil) } + +func withFailedCreate(mock dbMock) { + mock.EXPECT(). + CreateRepository(gomock.Any(), gomock.Any()). + Return(db.Repository{}, errDefault) +} + +func withSuccessfulCreate(mock dbMock) { + mock.EXPECT(). + CreateRepository(gomock.Any(), gomock.Any()). + Return(dbRepo, nil) +} + +func withPrivateReposEnabled(mock dbMock) { + mock.EXPECT(). + GetFeatureInProject(gomock.Any(), gomock.Any()). + Return(json.RawMessage{}, nil) +} + +func withPrivateReposDisabled(mock dbMock) { + mock.EXPECT(). + GetFeatureInProject(gomock.Any(), gomock.Any()). + Return(json.RawMessage{}, sql.ErrNoRows) +} + +func newGithubRepo(isPrivate bool) *gh.Repository { + return &gh.Repository{ + ID: ghRepoID, + Name: ptr.Ptr(repoName), + Owner: &gh.User{ + Login: ptr.Ptr(repoOwner), + }, + Private: ptr.Ptr(isPrivate), + DeploymentsURL: ptr.Ptr("https://foo.com"), + CloneURL: ptr.Ptr("http://cloneurl.com"), + Fork: ptr.Ptr(false), + DefaultBranch: ptr.Ptr("main"), + } +} + +func newExpectation(isPrivate bool) *pb.Repository { + return &pb.Repository{ + Id: ptr.Ptr(dbRepo.ID.String()), + Name: publicRepo.GetName(), + Owner: publicRepo.GetOwner().GetLogin(), + RepoId: publicRepo.GetID(), + HookId: webhook.GetID(), + HookUrl: webhook.GetURL(), + DeployUrl: publicRepo.GetDeploymentsURL(), + CloneUrl: publicRepo.GetCloneURL(), + HookType: webhook.GetType(), + HookName: webhook.GetName(), + HookUuid: hookUUID, + IsPrivate: isPrivate, + IsFork: publicRepo.GetFork(), + DefaultBranch: publicRepo.GetDefaultBranch(), + } +}