Repository: runatlantis/atlantis (language: go)

Issue:
(pre/post)workflow command runner can re-clone directory on policy_check, approve_policy, apply steps
<!--- Please keep this note for the community --->

### Community Note

* Please vote on this issue by adding a 👍 [reaction](https://blog.github.com/2016-03-10-add-reactions-to-pull-requests-issues-and-comments/) to the original issue to help the community and maintainers prioritize this request. Searching for pre-existing feature requests helps us consolidate datapoints for identical requirements into a single place, thank you!
* Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.
* If you are interested in working on this issue or have submitted a pull request, please leave a comment.

<!--- Thank you for keeping this note for the community --->

---

<!---
When filing a bug, please include the following headings if possible.
Any example text in this template can be deleted.
--->

### Overview of the Issue

pre and post workflow hooks can wipe out a planfile, via re-clone, for a working dir for commands which do not run clone. i.e. policy_check, approve_policies, and apply.

https://github.com/runatlantis/atlantis/blob/main/server/events/pre_workflow_hooks_command_runner.go#L67-L70
https://github.com/runatlantis/atlantis/blob/main/server/events/post_workflow_hooks_command_runner.go#L69-L72

This can be problematic, especially when coupled with #3487.

It seems that clone is specifically avoided for policy_check step:
- https://github.com/runatlantis/atlantis/blob/main/server/events/project_command_runner.go#L440-L441

likewise, it's not done for approve policies or apply, for similar reasons.

Shouldn't the pre and post workflow runner also take in to account the command operation which is running and not clone when they are run alongside the above operations?

### Reproduction Steps

<!--- 
In order to effectively and quickly resolve the issue, please provide exact steps that allow us the reproduce the problem. If no steps are provided, then it will likely take longer to get the issue resolved.
--->


### Logs

<!---
Provide log files from Atlantis server

logs can be retrieved from the deployment or from atlantis comments by adding `--debug` such as `atlantis plan --debug`

<details>
  <summary>Logs</summary>

```
log output
```

</details>
--->


### Environment details

<!---
If not already included, please provide the following:

- Atlantis version:
- Deployment method: ecs/eks/helm/tf module
- If not running the latest Atlantis version have you tried to reproduce this issue on the latest version: 
- Atlantis flags:

Atlantis server-side config file:

```yaml
# config file
```

Repo `atlantis.yaml` file:

```yaml
# config file
```

Any other information you can provide about the environment/deployment (efs/nfs, aws/gcp, k8s/fargate, etc)
--->


### Additional Context

<!---
Additional context on the problem. Docs, links to blogs, or other material that lead you to discover this issue or were helpful in troubleshooting the issue. 

Use a bulleted list to link to tickets
--->




Discussion / hints:

Fix patch:
diff --git a/server/events/mocks/mock_comment_building.go b/server/events/mocks/mock_comment_building.go
index 371daab715..fd53f43904 100644
--- a/server/events/mocks/mock_comment_building.go
+++ b/server/events/mocks/mock_comment_building.go
@@ -145,6 +145,41 @@ func (c *MockCommentBuilder_BuildApplyComment_OngoingVerification) GetAllCapture
 	return
 }
 
+func (verifier *VerifierMockCommentBuilder) BuildApprovePoliciesComment(_param0 string, _param1 string, _param2 string) *MockCommentBuilder_BuildApprovePoliciesComment_OngoingVerification {
+	params := []pegomock.Param{_param0, _param1, _param2}
+	methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "BuildApprovePoliciesComment", params, verifier.timeout)
+	return &MockCommentBuilder_BuildApprovePoliciesComment_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations}
+}
+
+type MockCommentBuilder_BuildApprovePoliciesComment_OngoingVerification struct {
+	mock              *MockCommentBuilder
+	methodInvocations []pegomock.MethodInvocation
+}
+
+func (c *MockCommentBuilder_BuildApprovePoliciesComment_OngoingVerification) GetCapturedArguments() (string, string, string) {
+	_param0, _param1, _param2 := c.GetAllCapturedArguments()
+	return _param0[len(_param0)-1], _param1[len(_param1)-1], _param2[len(_param2)-1]
+}
+
+func (c *MockCommentBuilder_BuildApprovePoliciesComment_OngoingVerification) GetAllCapturedArguments() (_param0 []string, _param1 []string, _param2 []string) {
+	params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations)
+	if len(params) > 0 {
+		_param0 = make([]string, len(c.methodInvocations))
+		for u, param := range params[0] {
+			_param0[u] = param.(string)
+		}
+		_param1 = make([]string, len(c.methodInvocations))
+		for u, param := range params[1] {
+			_param1[u] = param.(string)
+		}
+		_param2 = make([]string, len(c.methodInvocations))
+		for u, param := range params[2] {
+			_param2[u] = param.(string)
+		}
+	}
+	return
+}
+
 func (verifier *VerifierMockCommentBuilder) BuildPlanComment(_param0 string, _param1 string, _param2 string, _param3 []string) *MockCommentBuilder_BuildPlanComment_OngoingVerification {
 	params := []pegomock.Param{_param0, _param1, _param2, _param3}
 	methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "BuildPlanComment", params, verifier.timeout)
diff --git a/server/events/mocks/mock_working_dir.go b/server/events/mocks/mock_working_dir.go
index b22447042c..362b43f7f9 100644
--- a/server/events/mocks/mock_working_dir.go
+++ b/server/events/mocks/mock_working_dir.go
@@ -132,6 +132,14 @@ func (mock *MockWorkingDir) HasDiverged(_param0 logging.SimpleLogging, _param1 s
 	return ret0
 }
 
+func (mock *MockWorkingDir) SetSafeToReClone() {
+	if mock == nil {
+		panic("mock must not be nil. Use myMock := NewMockWorkingDir().")
+	}
+	params := []pegomock.Param{}
+	pegomock.GetGenericMockFrom(mock).Invoke("SetSafeToReClone", params, []reflect.Type{})
+}
+
 func (mock *MockWorkingDir) VerifyWasCalledOnce() *VerifierMockWorkingDir {
 	return &VerifierMockWorkingDir{
 		mock:                   mock,
@@ -370,3 +378,20 @@ func (c *MockWorkingDir_HasDiverged_OngoingVerification) GetAllCapturedArguments
 	}
 	return
 }
+
+func (verifier *VerifierMockWorkingDir) SetSafeToReClone() *MockWorkingDir_SetSafeToReClone_OngoingVerification {
+	params := []pegomock.Param{}
+	methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "SetSafeToReClone", params, verifier.timeout)
+	return &MockWorkingDir_SetSafeToReClone_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations}
+}
+
+type MockWorkingDir_SetSafeToReClone_OngoingVerification struct {
+	mock              *MockWorkingDir
+	methodInvocations []pegomock.MethodInvocation
+}
+
+func (c *MockWorkingDir_SetSafeToReClone_OngoingVerification) GetCapturedArguments() {
+}
+
+func (c *MockWorkingDir_SetSafeToReClone_OngoingVerification) GetAllCapturedArguments() {
+}
diff --git a/server/events/project_command_runner.go b/server/events/project_command_runner.go
index c0e618b4eb..8c9a7592f0 100644
--- a/server/events/project_command_runner.go
+++ b/server/events/project_command_runner.go
@@ -528,6 +528,7 @@ func (p *DefaultProjectCommandRunner) doPlan(ctx command.ProjectContext) (*model
 	}
 	defer unlockFn()
 
+	p.WorkingDir.SetSafeToReClone()
 	// Clone is idempotent so okay to run even if the repo was already cloned.
 	repoDir, hasDiverged, cloneErr := p.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, ctx.Workspace)
 	if cloneErr != nil {
diff --git a/server/events/working_dir.go b/server/events/working_dir.go
index fa4087d6ad..5c57bd6a21 100644
--- a/server/events/working_dir.go
+++ b/server/events/working_dir.go
@@ -49,6 +49,10 @@ type WorkingDir interface {
 	// Delete deletes the workspace for this repo and pull.
 	Delete(r models.Repo, p models.PullRequest) error
 	DeleteForWorkspace(r models.Repo, p models.PullRequest, workspace string) error
+	// Set a flag in the workingdir so Clone() can know that it is safe to re-clone the workingdir if
+	// the upstream branch has been modified. This is only safe after grabbing the project lock
+	// and before running any plans
+	SetSafeToReClone()
 }
 
 // FileWorkspace implements WorkingDir with the file system.
@@ -75,6 +79,8 @@ type FileWorkspace struct {
 	GithubAppEnabled bool
 	// use the global setting without overriding
 	GpgNoSigningEnabled bool
+	// flag indicating if a re-clone will be safe (project lock held, about to run plan)
+	SafeToReClone bool
 }
 
 // Clone git clones headRepo, checks out the branch and then returns the absolute
@@ -90,6 +96,7 @@ func (w *FileWorkspace) Clone(
 	workspace string) (string, bool, error) {
 	cloneDir := w.cloneDir(p.BaseRepo, p, workspace)
 	hasDiverged := false
+	defer func() { w.SafeToReClone = false }()
 
 	// If the directory already exists, check if it's at the right commit.
 	// If so, then we do nothing.
@@ -117,7 +124,7 @@ func (w *FileWorkspace) Clone(
 		// We're prefix matching here because BitBucket doesn't give us the full
 		// commit, only a 12 character prefix.
 		if strings.HasPrefix(currCommit, p.HeadCommit) {
-			if w.CheckoutMerge && w.recheckDiverged(log, p, headRepo, cloneDir) {
+			if w.SafeToReClone && w.CheckoutMerge && w.recheckDiverged(log, p, headRepo, cloneDir) {
 				log.Info("base branch has been updated, using merge strategy and will clone again")
 				hasDiverged = true
 			} else {
@@ -260,17 +267,17 @@ func (w *FileWorkspace) forceClone(log logging.SimpleLogging,
 	if !w.CheckoutMerge {
 		return runGit("clone", "--depth=1", "--branch", p.HeadBranch, "--single-branch", headCloneURL, cloneDir)
 	}
-	
+
 	// if merge strategy...
 
 	// if no checkout depth, omit depth arg
 	if w.CheckoutDepth == 0 {
 		if err := runGit("clone", "--branch", p.BaseBranch, "--single-branch", baseCloneURL, cloneDir); err != nil {
-			 return err
+			return err
 		}
 	} else {
-	 	if err := runGit("clone", "--depth", fmt.Sprint(w.CheckoutDepth), "--branch", p.BaseBranch, "--single-branch", baseCloneURL, cloneDir); err != nil {
-			 return err
+		if err := runGit("clone", "--depth", fmt.Sprint(w.CheckoutDepth), "--branch", p.BaseBranch, "--single-branch", baseCloneURL, cloneDir); err != nil {
+			return err
 		}
 	}
 
@@ -285,16 +292,16 @@ func (w *FileWorkspace) forceClone(log logging.SimpleLogging,
 		fetchRemote = "origin"
 	}
 
-        // if no checkout depth, omit depth arg
-        if w.CheckoutDepth == 0 {
-                if err := runGit("fetch", fetchRemote, fetchRef); err != nil {
-                         return err
-                }
-        } else {
-                if err := runGit("fetch", "--depth", fmt.Sprint(w.CheckoutDepth), fetchRemote, fetchRef); err != nil {
-                         return err
-                }
-        }
+	// if no checkout depth, omit depth arg
+	if w.CheckoutDepth == 0 {
+		if err := runGit("fetch", fetchRemote, fetchRef); err != nil {
+			return err
+		}
+	} else {
+		if err := runGit("fetch", "--depth", fmt.Sprint(w.CheckoutDepth), fetchRemote, fetchRef); err != nil {
+			return err
+		}
+	}
 
 	if w.GpgNoSigningEnabled {
 		if err := runGit("config", "--local", "commit.gpgsign", "false"); err != nil {
@@ -361,3 +368,8 @@ func (w *FileWorkspace) sanitizeGitCredentials(s string, base models.Repo, head
 	baseReplaced := strings.Replace(s, base.CloneURL, base.SanitizedCloneURL, -1)
 	return strings.Replace(baseReplaced, head.CloneURL, head.SanitizedCloneURL, -1)
 }
+
+// Set the flag that indicates it is safe to re-clone if necessary
+func (w *FileWorkspace) SetSafeToReClone() {
+	w.SafeToReClone = true
+}


Test patch:
diff --git a/server/events/mock_workingdir_test.go b/server/events/mock_workingdir_test.go
index 0b12302382..b5a9b7335d 100644
--- a/server/events/mock_workingdir_test.go
+++ b/server/events/mock_workingdir_test.go
@@ -132,6 +132,14 @@ func (mock *MockWorkingDir) HasDiverged(_param0 logging.SimpleLogging, _param1 s
 	return ret0
 }
 
+func (mock *MockWorkingDir) SetSafeToReClone() {
+	if mock == nil {
+		panic("mock must not be nil. Use myMock := NewMockWorkingDir().")
+	}
+	params := []pegomock.Param{}
+	pegomock.GetGenericMockFrom(mock).Invoke("SetSafeToReClone", params, []reflect.Type{})
+}
+
 func (mock *MockWorkingDir) VerifyWasCalledOnce() *VerifierMockWorkingDir {
 	return &VerifierMockWorkingDir{
 		mock:                   mock,
@@ -370,3 +378,20 @@ func (c *MockWorkingDir_HasDiverged_OngoingVerification) GetAllCapturedArguments
 	}
 	return
 }
+
+func (verifier *VerifierMockWorkingDir) SetSafeToReClone() *MockWorkingDir_SetSafeToReClone_OngoingVerification {
+	params := []pegomock.Param{}
+	methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "SetSafeToReClone", params, verifier.timeout)
+	return &MockWorkingDir_SetSafeToReClone_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations}
+}
+
+type MockWorkingDir_SetSafeToReClone_OngoingVerification struct {
+	mock              *MockWorkingDir
+	methodInvocations []pegomock.MethodInvocation
+}
+
+func (c *MockWorkingDir_SetSafeToReClone_OngoingVerification) GetCapturedArguments() {
+}
+
+func (c *MockWorkingDir_SetSafeToReClone_OngoingVerification) GetAllCapturedArguments() {
+}
diff --git a/server/events/working_dir_test.go b/server/events/working_dir_test.go
index 2622c67f9a..5bd8ff7f4c 100644
--- a/server/events/working_dir_test.go
+++ b/server/events/working_dir_test.go
@@ -480,6 +480,7 @@ func TestClone_MasterHasDiverged(t *testing.T) {
 	Assert(t, hasDiverged == false, "Clone with CheckoutMerge=false should not merge")
 
 	wd.CheckoutMerge = true
+	wd.SetSafeToReClone()
 	// Run the clone twice with the merge strategy, the first run should
 	// return true for hasDiverged, subsequent runs should
 	// return false since the first call is supposed to merge.
@@ -491,6 +492,7 @@ func TestClone_MasterHasDiverged(t *testing.T) {
 	Ok(t, err)
 	Assert(t, hasDiverged == true, "First clone with CheckoutMerge=true with diverged base should have merged")
 
+	wd.SetSafeToReClone()
 	_, hasDiverged, err = wd.Clone(logging.NewNoopLogger(t), models.Repo{CloneURL: repoDir}, models.PullRequest{
 		BaseRepo:   models.Repo{CloneURL: repoDir},
 		HeadBranch: "second-pr",