Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure cloning workingdir before calling plan api #3584

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: ensure cloning workingdir before calling plan api #3584

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7a68153
fix: ensure cloning workingdir before calling plan api
terakoya76 Jul 7, 2023
07d9fc4
fix: ensure cloning workingdir before calling apply api
terakoya76 Jul 8, 2023
417e1ac
test: add unit tests
terakoya76 Jul 8, 2023
9496f79
chore: run fmt autofix
terakoya76 Dec 31, 2024
8049046
Merge branch 'main' into fix_issue_2949
jamengual Dec 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 37 additions & 2 deletions server/controllers/api_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2465 introduced a lock for each PR, but the release of the lock appears to have been omitted.

if result.HasErrors() {
code = http.StatusInternalServerError
}
Expand All @@ -118,13 +126,19 @@ 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 {
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)
Expand All @@ -144,6 +158,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 {
Expand Down
205 changes: 176 additions & 29 deletions server/controllers/api_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,49 +25,194 @@ 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) {
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{{
Expand Down Expand Up @@ -107,6 +252,8 @@ func setup(t *testing.T) (controllers.APIController, *MockProjectCommandBuilder,
PostWorkflowHooksCommandRunner: postWorkflowHooksCommandRunner,
VCSClient: vcsClient,
RepoAllowlistChecker: repoAllowlistChecker,
WorkingDir: workingDir,
WorkingDirLocker: workingDirLocker,
}
return ac, projectCommandBuilder, projectCommandRunner
}
2 changes: 2 additions & 0 deletions server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
Loading