From 6f7e411ed12640f561450fb3c2dfa9f6ade91603 Mon Sep 17 00:00:00 2001
From: Milan Plzik <milan.plzik@grafana.com>
Date: Thu, 6 Jul 2023 11:24:20 +0200
Subject: [PATCH] Unpack TF plugins in a more atomic way.

The original implementation of plugin cache handling unpacks plugin
archives in a way that can result in race conditions when multiple
terraform processes are accessing the same plugin cache. Since the
archives are being decompressed in chunks and output files are written
directly to the cache, we observed following manifestations of the issue:

- `text file busy` errors if other terraform processes are already
  running the plugin and another process tries to replace it.
- various plugin checksum errors triggered likely by simultaneous checksumming
  and rewriting the file.

This PR changes the zip archives with plugins are handled:

1. Instead of writing directly to the target directory,
   `installFromLocalArchive` is now writing to a temporary staging
   directory prefixed with`.temp` that resides in the plugin cache dir
   to ensure this is on the same filesystem.
2. After unpacking, the directory structure of the staging directory is
   replicated in the `targetDir`. The directories are created as needed
   and any files are moved using `os.Rename`. After this, the staging
   directory is removed.
3. Since the temporary staging directories can be picked up by
   `SearchLocalDirectory` and make it fail in the situation when they're
   removed during directory traversal, the function has been modified to
   skip any entry that starts with dot.

Signed-off-by: Milan Plzik <milan.plzik@grafana.com>
---
 internal/getproviders/filesystem_search.go |  7 ++++
 internal/providercache/installer_test.go   |  2 +-
 internal/providercache/package_install.go  | 45 +++++++++++++++++++++-
 3 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/internal/getproviders/filesystem_search.go b/internal/getproviders/filesystem_search.go
index c50a1486a4fb..847ed10888c3 100644
--- a/internal/getproviders/filesystem_search.go
+++ b/internal/getproviders/filesystem_search.go
@@ -47,6 +47,13 @@ func SearchLocalDirectory(baseDir string) (map[addrs.Provider]PackageMetaList, e
 
 	err := filepath.Walk(baseDir, func(fullPath string, info os.FileInfo, err error) error {
 		if err != nil {
+			if filepath.Base(fullPath)[0] == '.' {
+				// Dot-files should not occur in the provider cache at all.
+				// Dot-directories are used by e.g. temporary directories to
+				// unpack the packed providers. These might disappear at any
+				// moment, making the traversal fairly brittle. Just skip them.
+				return filepath.SkipDir
+			}
 			return fmt.Errorf("cannot search %s: %s", fullPath, err)
 		}
 
diff --git a/internal/providercache/installer_test.go b/internal/providercache/installer_test.go
index 6115593ae382..1c30ed41855d 100644
--- a/internal/providercache/installer_test.go
+++ b/internal/providercache/installer_test.go
@@ -2299,7 +2299,7 @@ func TestEnsureProviderVersions_local_source(t *testing.T) {
 			provider: "null",
 			version:  "2.1.0",
 			wantHash: getproviders.NilHash,
-			err:      "zip: not a valid zip file",
+			err:      "failed to decompress: zip: not a valid zip file",
 		},
 		"version-constraint-unmet": {
 			provider: "null",
diff --git a/internal/providercache/package_install.go b/internal/providercache/package_install.go
index 6838307dd83b..5424131cb206 100644
--- a/internal/providercache/package_install.go
+++ b/internal/providercache/package_install.go
@@ -6,9 +6,11 @@ package providercache
 import (
 	"context"
 	"fmt"
+	"io/fs"
 	"io/ioutil"
 	"net/http"
 	"os"
+	"path"
 	"path/filepath"
 
 	getter "github.com/hashicorp/go-getter"
@@ -136,7 +138,48 @@ func installFromLocalArchive(ctx context.Context, meta getproviders.PackageMeta,
 	// match the allowed hashes and so our caller should catch that after
 	// we return if so.
 
-	err := unzip.Decompress(targetDir, filename, true, 0000)
+	err := os.MkdirAll(targetDir, 0777)
+	if err != nil {
+		return authResult, fmt.Errorf("failed to create new directory: %w", err)
+	}
+
+	stagingDir, err := os.MkdirTemp(path.Dir(targetDir), ".temp")
+	if err != nil {
+		return authResult, fmt.Errorf("failed to create a temp dir: %w", err)
+	}
+	defer os.RemoveAll(stagingDir)
+
+	err = unzip.Decompress(stagingDir, filename, true, 0000)
+	if err != nil {
+		return authResult, fmt.Errorf("failed to decompress: %w", err)
+	}
+
+	// Try to atomically move the files from stagingDir to targetDir.
+	err = filepath.Walk(stagingDir, func(path string, info fs.FileInfo, err error) error {
+		if err != nil {
+			return fmt.Errorf("failed to copy path %s to target directory: %w", path, err)
+		}
+		relPath, err := filepath.Rel(stagingDir, path)
+		if err != nil {
+			return fmt.Errorf("failed to calculate relative path: %w", err)
+		}
+
+		if info.IsDir() {
+			// Create the directory
+			err := os.MkdirAll(filepath.Join(targetDir, relPath), info.Mode().Perm())
+			if err != nil {
+				return fmt.Errorf("failed to create path: %w", err)
+			}
+		} else {
+			// On supported platforms, this should perform atomic replacement of the file.
+			err := os.Rename(path, filepath.Join(targetDir, relPath))
+			if err != nil {
+				return fmt.Errorf("failed to move '%s': %w", path, err)
+			}
+		}
+		return nil
+	})
+
 	if err != nil {
 		return authResult, err
 	}