VAULT-30189: enos_remote_exec: Use non-static ID as prefix for scripts (

#21)

* VAULT-30189: enos_remote_exec: Use non-static ID as prefix for scripts

When `enos_remote_exec` executes a script on a remote machine it copies
the contents to a local place on the target disk before executing it.
Because scripts can be re-used by different resources but provided
different env vars, we used to consider the env vars as part of the SHA
but it looks like it was it was erroneously removed[0] at some point.

What we have now is a case where multiple resources can use the same
script and therefore the same SHA in parallel. That leads to a race where
multiple resources are writing, executing, and cleaning up scripts with
the same SHA.

Instead, of reverting to the prior behavior, we instead now generate a
non-static ID for each `enos_remote_exec` resource at apply time. When
we copy over script contents we prepend this ID, along with the content
SHA, to prevent any script mod races.

[0] 44fe793

* linter: fix lastest linter issues and pin workflows

Signed-off-by: Ryan Cragun <me@ryan.ec>

.github/workflows/release.yml

@@ -33,7 +33,7 @@ jobs:
         uses: ./.github/actions/build-provider
         with:
           target: ${{matrix.os}}/${{ matrix.arch }}
-      - uses: actions/upload-artifact@0b2256b8c012f0828dc542b3febcab082c67f72b # v4.3.4
+      - uses: actions/upload-artifact@834a144ee995460fba8ed112a2fc961b36a5ec5a # v4.3.6
         with:
           name: ${{ steps.build.outputs.name }}
           path: dist/${{ steps.build.outputs.name }}

.github/workflows/test.yml

@@ -32,7 +32,7 @@ jobs:
         uses: ./.github/actions/build-provider
         with:
           target: ${{matrix.os}}/${{ matrix.arch }}
-      - uses: actions/upload-artifact@0b2256b8c012f0828dc542b3febcab082c67f72b # v4.3.4
+      - uses: actions/upload-artifact@834a144ee995460fba8ed112a2fc961b36a5ec5a # v4.3.6
         with:
           name: ${{ steps.build.outputs.name }}
           path: dist/${{ steps.build.outputs.name }}

.golangci.yml

@@ -107,6 +107,7 @@ linters:
     - nestif
     - nonamedreturns
     - nosprintfhostport
+    - predeclared
     - promlinter
     - rowserrcheck
     - sqlclosecheck

VERSION

@@ -1 +1 @@
-0.5.4
+0.5.5

docs/resources/remote_exec.md

@@ -49,7 +49,7 @@ If a double quote must be included in the command it should be escaped as follow
 
 ### Read-Only
 
-- `id` (String) The resource identifier is always static
+- `id` (String) A random ID number associated with the resource. This is created a single time during the initial 'apply' phase. It is utilized as a prefix when copying file contents to the remote target
 - `stderr` (String) The aggregate STDERR of all inline commnads, scripts, or content. If nothing is output this value will be set to a blank string
 - `stdout` (String) The aggregate STDOUT of all inline commnads, scripts, or content. If nothing is output this value will be set to a blank string
 - `sum` (String) A digest of the inline commands, source files, and environment variables. If the sum changes between runs all commands will execute again

internal/flightcontrol/download/command.go

@@ -152,7 +152,7 @@ func (c *Command) Download() error {
 			}
 		}
 
-		dst, err := os.OpenFile(c.args.destination, os.O_RDWR|os.O_CREATE, fs.FileMode(c.args.mode))
+		dst, err := os.OpenFile(c.args.destination, os.O_RDWR|os.O_CREATE, fs.FileMode(c.args.mode)) //#nosec: G115
 		if err != nil {
 			return err
 		}

internal/flightcontrol/zip/upzip_command.go

@@ -114,7 +114,7 @@ func (c *UnzipCommand) Unzip() error {
 	}
 	defer archive.Close()
 
-	fileMode := fs.FileMode(c.args.mode)
+	fileMode := fs.FileMode(c.args.mode) //#nosec:G115
 
 	// Make sure we've got a destination directory
 	dstDir, err := os.Open(c.args.destination)
@@ -128,7 +128,7 @@ func (c *UnzipCommand) Unzip() error {
 			return err
 		}
 
-		err = os.MkdirAll(c.args.destination, fs.FileMode(c.args.destinationMode))
+		err = os.MkdirAll(c.args.destination, fs.FileMode(c.args.destinationMode)) //#nosec:G115
 		if err != nil {
 			return err
 		}

internal/plugin/resource_remote_exec.go

@@ -14,6 +14,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-go/tftypes"
 
 	"github.com/hashicorp-forge/terraform-provider-enos/internal/diags"
+	"github.com/hashicorp-forge/terraform-provider-enos/internal/random"
 	"github.com/hashicorp-forge/terraform-provider-enos/internal/remoteflight"
 	resource "github.com/hashicorp-forge/terraform-provider-enos/internal/server/resourcerouter"
 	"github.com/hashicorp-forge/terraform-provider-enos/internal/server/state"
@@ -196,7 +197,6 @@ func (r *remoteExec) ApplyResourceChange(ctx context.Context, req resource.Apply
 		// nothing to do on delete
 		return
 	}
-	plannedState.ID.Set("static")
 
 	transport := transportUtil.ApplyValidatePlannedAndBuildTransport(ctx, plannedState, r, res)
 	if diags.HasErrors(res.Diagnostics) {
@@ -206,6 +206,10 @@ func (r *remoteExec) ApplyResourceChange(ctx context.Context, req resource.Apply
 	// If our priorState Sum is blank then we're creating the resource. If
 	// it's not blank and doesn't match the planned state we're updating.
 	_, pok := priorState.ID.Get()
+	if !pok {
+		plannedState.ID.Set(random.ID())
+	}
+
 	priorSum, prsumok := priorState.Sum.Get()
 	plannedSum, plsumok := plannedState.Sum.Get()
 
@@ -271,7 +275,7 @@ If a double quote must be included in the command it should be escaped as follow
 					Name:        "id",
 					Type:        tftypes.String,
 					Computed:    true,
-					Description: resourceStaticIDDescription,
+					Description: "A random ID number associated with the resource. This is created a single time during the initial 'apply' phase. It is utilized as a prefix when copying file contents to the remote target",
 				},
 				{
 					Name:        "sum",
@@ -334,7 +338,6 @@ If a double quote must be included in the command it should be escaped as follow
 func (r *remoteExec) ExecuteCommands(ctx context.Context, state *remoteExecStateV1, client it.Transport) (ui.UI, error) {
 	var err error
 	ui := ui.NewBuffered()
-	env, _ := state.Env.GetStrings()
 
 	if inline, ok := state.Inline.GetStrings(); ok {
 		for _, cmd := range inline {
@@ -353,7 +356,7 @@ func (r *remoteExec) ExecuteCommands(ctx context.Context, state *remoteExecState
 				source := tfile.NewReader(cmd)
 				defer source.Close()
 
-				return r.copyAndRun(ctx, ui, client, source, "inline", env)
+				return r.copyAndRun(ctx, ui, client, source, "inline", state)
 			}
 			if err := exec(cmd); err != nil {
 				return ui, fmt.Errorf("running inline command failed, due to: %w", err)
@@ -370,7 +373,7 @@ func (r *remoteExec) ExecuteCommands(ctx context.Context, state *remoteExecState
 				}
 				defer script.Close()
 
-				return r.copyAndRun(ctx, ui, client, script, "script", env)
+				return r.copyAndRun(ctx, ui, client, script, "script", state)
 			}
 
 			if err := exec(path); err != nil {
@@ -383,7 +386,7 @@ func (r *remoteExec) ExecuteCommands(ctx context.Context, state *remoteExecState
 		content := tfile.NewReader(cont)
 		defer content.Close()
 
-		err = r.copyAndRun(ctx, ui, client, content, "content", env)
+		err = r.copyAndRun(ctx, ui, client, content, "content", state)
 		if err != nil {
 			return ui, fmt.Errorf("running command content failed, due to: %w", err)
 		}
@@ -395,7 +398,7 @@ func (r *remoteExec) ExecuteCommands(ctx context.Context, state *remoteExecState
 // copyAndRun copies the copyable source to the target using the configured transport,
 // sets the environment variables and executes the content of the source.
 // It returns STDOUT, STDERR, and any errors encountered.
-func (r *remoteExec) copyAndRun(ctx context.Context, ui ui.UI, client it.Transport, src it.Copyable, srcType string, env map[string]string) error {
+func (r *remoteExec) copyAndRun(ctx context.Context, ui ui.UI, client it.Transport, src it.Copyable, srcType string, state *remoteExecStateV1) error {
 	select {
 	case <-ctx.Done():
 		return ctx.Err()
@@ -417,9 +420,10 @@ func (r *remoteExec) copyAndRun(ctx context.Context, ui ui.UI, client it.Transpo
 	// TODO: Eventually we'll probably have to support /tmp being mounted
 	// with no exec. In those cases we'll have to make this configurable
 	// or find another strategy for executing scripts.
+	env, _ := state.Env.GetStrings()
 	res, err := remoteflight.RunScript(ctx, client, remoteflight.NewRunScriptRequest(
 		remoteflight.WithRunScriptContent(src),
-		remoteflight.WithRunScriptDestination(fmt.Sprintf("/tmp/%s.sh", sha)),
+		remoteflight.WithRunScriptDestination(fmt.Sprintf("/tmp/%s-%s.sh", state.ID.Value(), sha)),
 		remoteflight.WithRunScriptEnv(env),
 		remoteflight.WithRunScriptChmod("0777"),
 	))

internal/plugin/tftypes.go

@@ -130,7 +130,7 @@ func encodeTfObjectDynamicPseudoType(
 
 	// MarshalMsgPack is deprecated but it's by far the easiest way to inspect the serialized value
 	// of the raw attribute.
-	//nolint:staticcheck
+	//nolint:staticcheck,nolintlint
 	//lint:ignore SA1019 we have to use this internal only API to determine DynamicPseudoType types.
 	msgpackBytes, err := planVal.MarshalMsgPack(tftypes.DynamicPseudoType)
 	if err != nil {

internal/transport/test/test_suite.go

@@ -40,7 +40,6 @@ func NewTransportTestSuite(transportFn func(t *testing.T) it.Transport) *Transpo
 func (s *TransportTestSuite) TestRun() {
 	t := s.T()
 	transport := s.transportFn(t)
-	t.Parallel()
 
 	type args struct {
 		command it.Command
@@ -117,7 +116,6 @@ func (s *TransportTestSuite) TestRun() {
 func (s *TransportTestSuite) TestCopy() {
 	t := s.T()
 	transport := s.transportFn(t)
-	t.Parallel()
 
 	type args struct {
 		ctx      context.Context
@@ -194,7 +192,6 @@ func (s *TransportTestSuite) TestCopy() {
 func (s *TransportTestSuite) TestStream() {
 	t := s.T()
 	transport := s.transportFn(t)
-	t.Parallel()
 
 	type args struct {
 		ctx     context.Context