From 7a681536b9d0a559e88470118a08b165b6476ed4 Mon Sep 17 00:00:00 2001 From: Hajime Terasawa Date: Fri, 7 Jul 2023 19:14:44 +0900 Subject: [PATCH 1/4] fix: ensure cloning workingdir before calling plan api Signed-off-by: Hajime Terasawa --- server/controllers/api_controller.go | 33 +++++++++++++++++++++-- server/controllers/api_controller_test.go | 11 ++++++-- server/server.go | 2 ++ 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/server/controllers/api_controller.go b/server/controllers/api_controller.go index ff85371469..9b751c1ab8 100644 --- a/server/controllers/api_controller.go +++ b/server/controllers/api_controller.go @@ -33,6 +33,8 @@ type APIController struct { RepoAllowlistChecker *events.RepoAllowlistChecker Scope tally.Scope VCSClient vcs.Client + WorkingDir events.WorkingDir + WorkingDirLocker events.WorkingDirLocker } type APIRequest struct { @@ -90,12 +92,18 @@ func (a *APIController) Plan(w http.ResponseWriter, r *http.Request) { return } + err = a.apiSetup(ctx) + if err != nil { + a.apiReportError(w, http.StatusInternalServerError, err) + return + } + result, err := a.apiPlan(request, ctx) if err != nil { a.apiReportError(w, http.StatusInternalServerError, err) return } - defer a.Locker.UnlockByPull(ctx.HeadRepo.FullName, 0) // nolint: errcheck + defer a.Locker.UnlockByPull(ctx.HeadRepo.FullName, ctx.Pull.Num) // nolint: errcheck if result.HasErrors() { code = http.StatusInternalServerError } @@ -124,7 +132,7 @@ func (a *APIController) Apply(w http.ResponseWriter, r *http.Request) { a.apiReportError(w, http.StatusInternalServerError, err) return } - defer a.Locker.UnlockByPull(ctx.HeadRepo.FullName, 0) // nolint: errcheck + defer a.Locker.UnlockByPull(ctx.HeadRepo.FullName, ctx.Pull.Num) // nolint: errcheck // We can now prepare and run the apply step result, err := a.apiApply(request, ctx) @@ -144,6 +152,27 @@ func (a *APIController) Apply(w http.ResponseWriter, r *http.Request) { a.respond(w, logging.Warn, code, "%s", string(response)) } +func (a *APIController) apiSetup(ctx *command.Context) error { + pull := ctx.Pull + baseRepo := ctx.Pull.BaseRepo + headRepo := ctx.HeadRepo + + unlockFn, err := a.WorkingDirLocker.TryLock(baseRepo.FullName, pull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir) + if err != nil { + return err + } + ctx.Log.Debug("got workspace lock") + defer unlockFn() + + // ensure workingDir is present + _, _, err = a.WorkingDir.Clone(ctx.Log, headRepo, pull, events.DefaultWorkspace) + if err != nil { + return err + } + + return nil +} + func (a *APIController) apiPlan(request *APIRequest, ctx *command.Context) (*command.Result, error) { cmds, cc, err := request.getCommands(ctx, a.ProjectCommandBuilder.BuildPlanCommands) if err != nil { diff --git a/server/controllers/api_controller_test.go b/server/controllers/api_controller_test.go index 3b3aa520aa..0f7981793f 100644 --- a/server/controllers/api_controller_test.go +++ b/server/controllers/api_controller_test.go @@ -62,12 +62,17 @@ func setup(t *testing.T) (controllers.APIController, *MockProjectCommandBuilder, RegisterMockTestingT(t) locker := NewMockLocker() logger := logging.NewNoopLogger(t) - scope, _, _ := metrics.NewLoggingScope(logger, "null") parser := NewMockEventParsing() - vcsClient := NewMockClient() repoAllowlistChecker, err := events.NewRepoAllowlistChecker("*") + scope, _, _ := metrics.NewLoggingScope(logger, "null") + vcsClient := NewMockClient() + workingDir := NewMockWorkingDir() Ok(t, err) + workingDirLocker := NewMockWorkingDirLocker() + When(workingDirLocker.TryLock(Any[string](), Any[int](), Eq(events.DefaultWorkspace), Eq(events.DefaultRepoRelDir))). + ThenReturn(func() {}, nil) + projectCommandBuilder := NewMockProjectCommandBuilder() When(projectCommandBuilder.BuildPlanCommands(Any[*command.Context](), Any[*events.CommentCommand]())). ThenReturn([]command.ProjectContext{{ @@ -107,6 +112,8 @@ func setup(t *testing.T) (controllers.APIController, *MockProjectCommandBuilder, PostWorkflowHooksCommandRunner: postWorkflowHooksCommandRunner, VCSClient: vcsClient, RepoAllowlistChecker: repoAllowlistChecker, + WorkingDir: workingDir, + WorkingDirLocker: workingDirLocker, } return ac, projectCommandBuilder, projectCommandRunner } diff --git a/server/server.go b/server/server.go index 22f6db5498..3a9e76a12d 100644 --- a/server/server.go +++ b/server/server.go @@ -922,6 +922,8 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { RepoAllowlistChecker: repoAllowlist, Scope: statsScope.SubScope("api"), VCSClient: vcsClient, + WorkingDir: workingDir, + WorkingDirLocker: workingDirLocker, } eventsController := &events_controllers.VCSEventsController{ From 07d9fc450b0baae92fb181746fb9fd081fac0793 Mon Sep 17 00:00:00 2001 From: Hajime Terasawa Date: Sat, 8 Jul 2023 14:23:11 +0900 Subject: [PATCH 2/4] fix: ensure cloning workingdir before calling apply api Signed-off-by: Hajime Terasawa --- server/controllers/api_controller.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/server/controllers/api_controller.go b/server/controllers/api_controller.go index 9b751c1ab8..ebfb7aea25 100644 --- a/server/controllers/api_controller.go +++ b/server/controllers/api_controller.go @@ -126,6 +126,12 @@ func (a *APIController) Apply(w http.ResponseWriter, r *http.Request) { return } + err = a.apiSetup(ctx) + if err != nil { + a.apiReportError(w, http.StatusInternalServerError, err) + return + } + // We must first make the plan for all projects _, err = a.apiPlan(request, ctx) if err != nil { From 417e1ac5898295da5770ca556167d48337158947 Mon Sep 17 00:00:00 2001 From: Hajime Terasawa Date: Sat, 8 Jul 2023 15:10:40 +0900 Subject: [PATCH 3/4] test: add unit tests Signed-off-by: Hajime Terasawa --- server/controllers/api_controller_test.go | 194 +++++++++++++++++++--- 1 file changed, 167 insertions(+), 27 deletions(-) diff --git a/server/controllers/api_controller_test.go b/server/controllers/api_controller_test.go index 0f7981793f..c5b17ee893 100644 --- a/server/controllers/api_controller_test.go +++ b/server/controllers/api_controller_test.go @@ -25,37 +25,177 @@ const atlantisToken = "token" func TestAPIController_Plan(t *testing.T) { ac, projectCommandBuilder, projectCommandRunner := setup(t) - body, _ := json.Marshal(controllers.APIRequest{ - Repository: "Repo", - Ref: "main", - Type: "Gitlab", - Projects: []string{"default"}, - }) - req, _ := http.NewRequest("POST", "", bytes.NewBuffer(body)) - req.Header.Set(atlantisTokenHeader, atlantisToken) - w := httptest.NewRecorder() - ac.Plan(w, req) - ResponseContains(t, w, http.StatusOK, "") - projectCommandBuilder.VerifyWasCalledOnce().BuildPlanCommands(Any[*command.Context](), Any[*events.CommentCommand]()) - projectCommandRunner.VerifyWasCalledOnce().Plan(Any[command.ProjectContext]()) + + cases := []struct { + repository string + ref string + vcsType string + pr int + projects []string + paths []struct { + Directory string + Workspace string + } + }{ + { + repository: "Repo", + ref: "main", + vcsType: "Gitlab", + projects: []string{"default"}, + }, + { + repository: "Repo", + ref: "main", + vcsType: "Gitlab", + pr: 1, + }, + { + repository: "Repo", + ref: "main", + vcsType: "Gitlab", + paths: []struct { + Directory string + Workspace string + }{ + { + Directory: ".", + Workspace: "myworkspace", + }, + { + Directory: "./myworkspace2", + Workspace: "myworkspace2", + }, + }, + }, + { + repository: "Repo", + ref: "main", + vcsType: "Gitlab", + pr: 1, + projects: []string{"test"}, + paths: []struct { + Directory string + Workspace string + }{ + { + Directory: ".", + Workspace: "myworkspace", + }, + }, + }, + } + + expectedCalls := 0 + for _, c := range cases { + body, _ := json.Marshal(controllers.APIRequest{ + Repository: c.repository, + Ref: c.ref, + Type: c.vcsType, + PR: c.pr, + Projects: c.projects, + Paths: c.paths, + }) + + req, _ := http.NewRequest("POST", "", bytes.NewBuffer(body)) + req.Header.Set(atlantisTokenHeader, atlantisToken) + w := httptest.NewRecorder() + ac.Plan(w, req) + ResponseContains(t, w, http.StatusOK, "") + + expectedCalls += len(c.projects) + expectedCalls += len(c.paths) + } + + projectCommandBuilder.VerifyWasCalled(Times(expectedCalls)).BuildPlanCommands(Any[*command.Context](), Any[*events.CommentCommand]()) + projectCommandRunner.VerifyWasCalled(Times(expectedCalls)).Plan(Any[command.ProjectContext]()) } func TestAPIController_Apply(t *testing.T) { ac, projectCommandBuilder, projectCommandRunner := setup(t) - body, _ := json.Marshal(controllers.APIRequest{ - Repository: "Repo", - Ref: "main", - Type: "Gitlab", - Projects: []string{"default"}, - }) - req, _ := http.NewRequest("POST", "", bytes.NewBuffer(body)) - req.Header.Set(atlantisTokenHeader, atlantisToken) - w := httptest.NewRecorder() - ac.Apply(w, req) - ResponseContains(t, w, http.StatusOK, "") - projectCommandBuilder.VerifyWasCalledOnce().BuildApplyCommands(Any[*command.Context](), Any[*events.CommentCommand]()) - projectCommandRunner.VerifyWasCalledOnce().Plan(Any[command.ProjectContext]()) - projectCommandRunner.VerifyWasCalledOnce().Apply(Any[command.ProjectContext]()) + + cases := []struct { + repository string + ref string + vcsType string + pr int + projects []string + paths []struct { + Directory string + Workspace string + } + }{ + { + repository: "Repo", + ref: "main", + vcsType: "Gitlab", + projects: []string{"default"}, + }, + { + repository: "Repo", + ref: "main", + vcsType: "Gitlab", + pr: 1, + }, + { + repository: "Repo", + ref: "main", + vcsType: "Gitlab", + paths: []struct { + Directory string + Workspace string + }{ + { + Directory: ".", + Workspace: "myworkspace", + }, + { + Directory: "./myworkspace2", + Workspace: "myworkspace2", + }, + }, + }, + { + repository: "Repo", + ref: "main", + vcsType: "Gitlab", + pr: 1, + projects: []string{"test"}, + paths: []struct { + Directory string + Workspace string + }{ + { + Directory: ".", + Workspace: "myworkspace", + }, + }, + }, + } + + expectedCalls := 0 + for _, c := range cases { + body, _ := json.Marshal(controllers.APIRequest{ + Repository: c.repository, + Ref: c.ref, + Type: c.vcsType, + PR: c.pr, + Projects: c.projects, + Paths: c.paths, + }) + + req, _ := http.NewRequest("POST", "", bytes.NewBuffer(body)) + req.Header.Set(atlantisTokenHeader, atlantisToken) + w := httptest.NewRecorder() + ac.Apply(w, req) + ResponseContains(t, w, http.StatusOK, "") + + expectedCalls += len(c.projects) + expectedCalls += len(c.paths) + } + + projectCommandBuilder.VerifyWasCalled(Times(expectedCalls)).BuildApplyCommands(Any[*command.Context](), Any[*events.CommentCommand]()) + projectCommandRunner.VerifyWasCalled(Times(expectedCalls)).Plan(Any[command.ProjectContext]()) + projectCommandRunner.VerifyWasCalled(Times(expectedCalls)).Apply(Any[command.ProjectContext]()) } func setup(t *testing.T) (controllers.APIController, *MockProjectCommandBuilder, *MockProjectCommandRunner) { From 9496f79badf4b18c3a89a9f7aa68357c97b73123 Mon Sep 17 00:00:00 2001 From: Hajime Terasawa Date: Wed, 1 Jan 2025 08:06:17 +0900 Subject: [PATCH 4/4] chore: run fmt autofix Signed-off-by: Hajime Terasawa --- server/controllers/api_controller_test.go | 4 ++-- server/server.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/server/controllers/api_controller_test.go b/server/controllers/api_controller_test.go index c5b17ee893..b1f73893ac 100644 --- a/server/controllers/api_controller_test.go +++ b/server/controllers/api_controller_test.go @@ -252,8 +252,8 @@ func setup(t *testing.T) (controllers.APIController, *MockProjectCommandBuilder, PostWorkflowHooksCommandRunner: postWorkflowHooksCommandRunner, VCSClient: vcsClient, RepoAllowlistChecker: repoAllowlistChecker, - WorkingDir: workingDir, - WorkingDirLocker: workingDirLocker, + WorkingDir: workingDir, + WorkingDirLocker: workingDirLocker, } return ac, projectCommandBuilder, projectCommandRunner } diff --git a/server/server.go b/server/server.go index 3a9e76a12d..3db32822f8 100644 --- a/server/server.go +++ b/server/server.go @@ -922,8 +922,8 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { RepoAllowlistChecker: repoAllowlist, Scope: statsScope.SubScope("api"), VCSClient: vcsClient, - WorkingDir: workingDir, - WorkingDirLocker: workingDirLocker, + WorkingDir: workingDir, + WorkingDirLocker: workingDirLocker, } eventsController := &events_controllers.VCSEventsController{