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(), + } +}