feat(file)!: snippets upload using SSH input stream (#1085)

* feat(file)!: safer snippets upload using SSH input stream
* fixes for acceptance tests on windows
* enable other OS-es for acceptance tests
* update example templates to use api token auth

---------

Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com>

.github/workflows/test.yml

@@ -8,6 +8,7 @@ on:
 
 jobs:
   build:
+    name: Build
     runs-on: ubuntu-latest
     timeout-minutes: 5
     steps:
@@ -40,6 +41,7 @@ jobs:
         run: go vet . && go build -v .
 
   test:
+    name: Unit Tests
     needs: build
     runs-on: ubuntu-latest
     steps:
@@ -75,12 +77,14 @@ jobs:
         run: make docs && git diff --exit-code
 
   testacc:
+    if: "!contains(github.head_ref, 'renovate/') && !contains(github.head_ref, 'release-please')"
+    name: Dispatch Acceptance Tests
     needs: build
     runs-on: ubuntu-latest
     steps:
       - name: Invoke acceptance tests workflow
         uses: benc-uk/workflow-dispatch@v1
         with:
           workflow: testacc.yml
-          ref: "main"
+          ref: ${{ github.event.pull_request.head.ref }}
           inputs: '{"ref": "${{ github.head_ref }}" }'

.github/workflows/testacc.yml

@@ -10,11 +10,10 @@ on:
 
 jobs:
   acceptance:
-    #refs/heads/renovate/tools
     strategy:
       max-parallel: 1
       matrix:
-        os: [ ubuntu-latest ]
+        os: [ ubuntu-latest, windows-latest, macos-latest ]
         terraform: [ 1.6 ]
     runs-on: ${{ matrix.os }}
     environment: pve-acc

docs/index.md

@@ -200,7 +200,7 @@ You can configure the `sudo` privilege for the user via the command line on the
     ```sh
     terraform ALL=(root) NOPASSWD: /sbin/pvesm
     terraform ALL=(root) NOPASSWD: /sbin/qm
-    terraform ALL=(root) NOPASSWD: /usr/bin/mv /tmp/tfpve/* /var/lib/vz/*
+    terraform ALL=(root) NOPASSWD: /usr/bin/tee /var/lib/vz/*
     ```
 
   Save the file and exit.

example/main.tf

@@ -1,9 +1,9 @@
 provider "proxmox" {
   endpoint = var.virtual_environment_endpoint
-  username = var.virtual_environment_username
-  password = var.virtual_environment_password
+  api_token = var.virtual_environment_api_token
   insecure = true
   ssh {
     agent = true
+    username = var.virtual_environment_ssh_username
   }
 }

example/variables.tf

@@ -3,12 +3,12 @@ variable "virtual_environment_endpoint" {
   description = "The endpoint for the Proxmox Virtual Environment API (example: https://host:port)"
 }
 
-variable "virtual_environment_password" {
+variable "virtual_environment_api_token" {
   type        = string
-  description = "The password for the Proxmox Virtual Environment API"
+  description = "The API token for the Proxmox Virtual Environment API"
 }
 
-variable "virtual_environment_username" {
+variable "virtual_environment_ssh_username" {
   type        = string
-  description = "The username and realm for the Proxmox Virtual Environment API (example: root@pam)"
-}
+  description = "The username for the Proxmox Virtual Environment API"
+}

fwprovider/tests/resource_file_test.go

@@ -125,11 +125,12 @@ func uploadSnippetFile(t *testing.T, file *os.File) {
 	u, err := url.ParseRequestURI(endpoint)
 	require.NoError(t, err)
 
+	sshAgent := utils.GetAnyBoolEnv("PROXMOX_VE_SSH_AGENT")
 	sshUsername := utils.GetAnyStringEnv("PROXMOX_VE_SSH_USERNAME")
-	sshAgentSocket := utils.GetAnyStringEnv("SSH_AUTH_SOCK", "PROXMOX_VE_SSH_AUTH_SOCK", "PM_VE_SSH_AUTH_SOCK")
+	sshAgentSocket := utils.GetAnyStringEnv("SSH_AUTH_SOCK", "PROXMOX_VE_SSH_AUTH_SOCK")
 	sshPrivateKey := utils.GetAnyStringEnv("PROXMOX_VE_SSH_PRIVATE_KEY")
 	sshClient, err := ssh.NewClient(
-		sshUsername, "", true, sshAgentSocket, sshPrivateKey,
+		sshUsername, "", sshAgent, sshAgentSocket, sshPrivateKey,
 		"", "", "",
 		&nodeResolver{
 			node: ssh.ProxmoxNode{
@@ -146,21 +147,13 @@ func uploadSnippetFile(t *testing.T, file *os.File) {
 	defer f.Close()
 
 	fname := filepath.Base(file.Name())
-	err = sshClient.NodeUpload(context.Background(), "pve", "/tmp/tfpve/testacc",
+	err = sshClient.NodeStreamUpload(context.Background(), "pve", "/var/lib/vz/",
 		&api.FileUploadRequest{
 			ContentType: "snippets",
 			FileName:    fname,
 			File:        f,
 		})
 	require.NoError(t, err)
-
-	_, err = sshClient.ExecuteNodeCommands(context.Background(), "pve", []string{
-		fmt.Sprintf(`%s; try_sudo "mv /tmp/tfpve/testacc/snippets/%s /var/lib/vz/snippets/%s" && rm -rf /tmp/tfpve/testacc/`,
-			ssh.TrySudo,
-			fname, fname,
-		),
-	})
-	require.NoError(t, err)
 }
 
 func createFile(t *testing.T, namePattern string, content string) *os.File {
@@ -218,7 +211,7 @@ resource "proxmox_virtual_environment_file" "test" {
   }
   %s
 }
-	`, getProviderConfig(t), accTestNodeName, fname, strings.Join(extra, "\n"))
+	`, getProviderConfig(t), accTestNodeName, strings.ReplaceAll(fname, `\`, `/`), strings.Join(extra, "\n"))
 }
 
 func testAccResourceFileTwoSourcesCreatedConfig(t *testing.T) string {
@@ -281,7 +274,7 @@ resource "proxmox_virtual_environment_file" "test" {
     path = "%s"
   }
 }
-	`, getProviderConfig(t), accTestNodeName, fname)
+	`, getProviderConfig(t), accTestNodeName, strings.ReplaceAll(fname, `\`, `/`))
 }
 
 func testAccResourceFileSnippetUpdatedCheck(fname string) resource.TestCheckFunc {

proxmox/ssh/client.go

@@ -51,6 +51,10 @@ type Client interface {
 	// NodeUpload uploads a file to a node.
 	NodeUpload(ctx context.Context, nodeName string,
 		remoteFileDir string, fileUploadRequest *api.FileUploadRequest) error
+
+	// NodeStreamUpload uploads a file to a node by streaming its content over SSH.
+	NodeStreamUpload(ctx context.Context, nodeName string,
+		remoteFileDir string, fileUploadRequest *api.FileUploadRequest) error
 }
 
 type client struct {
@@ -247,6 +251,112 @@ func (c *client) NodeUpload(
 	return nil
 }
 
+func (c *client) NodeStreamUpload(
+	ctx context.Context,
+	nodeName string,
+	remoteFileDir string,
+	d *api.FileUploadRequest,
+) error {
+	ip, err := c.nodeResolver.Resolve(ctx, nodeName)
+	if err != nil {
+		return fmt.Errorf("failed to find node endpoint: %w", err)
+	}
+
+	tflog.Debug(ctx, "uploading file to the node datastore via SSH input stream ", map[string]interface{}{
+		"node_address": ip,
+		"remote_dir":   remoteFileDir,
+		"file_name":    d.FileName,
+		"content_type": d.ContentType,
+	})
+
+	fileInfo, err := d.File.Stat()
+	if err != nil {
+		return fmt.Errorf("failed to get file info: %w", err)
+	}
+
+	fileSize := fileInfo.Size()
+
+	sshClient, err := c.openNodeShell(ctx, ip)
+	if err != nil {
+		return fmt.Errorf("failed to open SSH client: %w", err)
+	}
+
+	defer func(sshClient *ssh.Client) {
+		e := sshClient.Close()
+		if e != nil {
+			tflog.Error(ctx, "failed to close SSH client", map[string]interface{}{
+				"error": e,
+			})
+		}
+	}(sshClient)
+
+	if d.ContentType != "" {
+		remoteFileDir = filepath.Join(remoteFileDir, d.ContentType)
+	}
+
+	remoteFilePath := strings.ReplaceAll(filepath.Join(remoteFileDir, d.FileName), `\`, "/")
+
+	sshSession, err := sshClient.NewSession()
+	if err != nil {
+		return fmt.Errorf("failed to create SSH session: %w", err)
+	}
+
+	defer func(session *ssh.Session) {
+		e := session.Close()
+		if e != nil {
+			tflog.Error(ctx, "failed to close SSH session", map[string]interface{}{
+				"error": e,
+			})
+		}
+	}(sshSession)
+
+	sshSession.Stdin = d.File
+
+	output, err := sshSession.CombinedOutput(
+		fmt.Sprintf(`%s; try_sudo "/usr/bin/tee %s"`, TrySudo, remoteFilePath),
+	)
+	if err != nil {
+		return fmt.Errorf("error transferring file: %s", string(output))
+	}
+
+	sftpClient, err := sftp.NewClient(sshClient)
+	if err != nil {
+		return fmt.Errorf("failed to create SFTP client: %w", err)
+	}
+
+	defer func(sftpClient *sftp.Client) {
+		e := sftpClient.Close()
+		if e != nil {
+			tflog.Error(ctx, "failed to close SFTP client", map[string]interface{}{
+				"error": e,
+			})
+		}
+	}(sftpClient)
+
+	remoteFile, err := sftpClient.Open(remoteFilePath)
+	if err != nil {
+		return fmt.Errorf("failed to open remote file %s: %w", remoteFilePath, err)
+	}
+
+	remoteStat, err := remoteFile.Stat()
+	if err != nil {
+		return fmt.Errorf("failed to read remote file %s: %w", remoteFilePath, err)
+	}
+
+	bytesUploaded := remoteStat.Size()
+	if bytesUploaded != fileSize {
+		return fmt.Errorf("failed to upload file %s: uploaded %d bytes, expected %d bytes",
+			remoteFilePath, bytesUploaded, fileSize)
+	}
+
+	tflog.Debug(ctx, "uploaded file to datastore", map[string]interface{}{
+		"remote_file_path": remoteFilePath,
+		"size":             bytesUploaded,
+	})
+
+	return nil
+}
+
 // openNodeShell establishes a new SSH connection to a node.
 func (c *client) openNodeShell(ctx context.Context, node ProxmoxNode) (*ssh.Client, error) {
 	homeDir, err := os.UserHomeDir()

proxmoxtf/resource/file.go

@@ -22,7 +22,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/google/uuid"
 	"github.com/hashicorp/go-cty/cty"
 	"github.com/hashicorp/terraform-plugin-log/tflog"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
@@ -573,35 +572,12 @@ func fileCreate(ctx context.Context, d *schema.ResourceData, m interface{}) diag
 			}...)
 		}
 
-		// the temp directory is used to store the file on the node before moving it to the datastore
-		// will be created if it does not exist
-		tempFileDir := fmt.Sprintf("/tmp/tfpve/%s", uuid.NewString())
-
-		err = capi.SSH().NodeUpload(ctx, nodeName, tempFileDir, request)
+		err = capi.SSH().NodeStreamUpload(ctx, nodeName, *datastore.Path, request)
 		if err != nil {
 			diags = append(diags, diag.FromErr(err)...)
 			return diags
 		}
 
-		// handle the case where the file is uploaded to a subdirectory of the datastore
-		srcDir := tempFileDir
-		dstDir := *datastore.Path
-
-		if request.ContentType != "" {
-			srcDir = tempFileDir + "/" + request.ContentType
-			dstDir = *datastore.Path + "/" + request.ContentType
-		}
-
-		_, err := capi.SSH().ExecuteNodeCommands(ctx, nodeName, []string{
-			// the `mv` command should be scoped to the specific directories in sudoers!
-			fmt.Sprintf(`%s; try_sudo "mv %s/%s %s/%s" && rmdir %s && rmdir %s || echo`,
-				ssh.TrySudo,
-				srcDir, *fileName,
-				dstDir, *fileName,
-				srcDir,
-				tempFileDir,
-			),
-		})
 		if err != nil {
 			if matches, e := regexp.MatchString(`cannot move .* Permission denied`, err.Error()); e == nil && matches {
 				return diag.FromErr(ssh.NewErrUserHasNoPermission(capi.SSH().Username()))