feat(file): add support to set the file mode (#1478)

* feat(file): Add support to set the file mode

GH-733 [1] implemented basic support for hook scripts, but the authors
"did not manage to find time to work on" [2] also including support to
set the file mode. This small but important feature makes the use of the
`proxmox_virtual_environment_container.hook_script_file_id` [3] and
`virtual_environment_vm.hook_script_file_id` [34] attributes basically
useless when not combined with the manual step of making the uploaded
file executable (manually running `chmod +x /path/to/script` or using
other methods, based on the storage backend). Using the
`hook_script_file_id` on its own also causes all planned and applies
changes in the same execution to not be saved in the state because the
Proxmox VE API responses with a HTTP `500` because the uploaded and
assigned file is not executable.

This pull request implements the missing feature to set the file mode
by adding a new `file_mode` attribute of type `string` where an
octal-formatted value can be passed, e.g. `0700` or only `600`.
Note that the support for the octal prefixes `0o` and `0x` are not
supported to reduced the complexity, even though Go of course support
it, including the used `os.FileMode` type [5].
Changing the file mode also causes the file to be replaced, which is
true for almost any attribute in the `proxmox_virtual_environment_file`
resource, to ensure that the file mode can also be changed after the
initial creation.

[1]: https://github.com/bpg/terraform-provider-proxmox/pull/733
[2]: https://github.com/bpg/terraform-provider-proxmox/pull/733#issuecomment-2096716738
[3]: https://registry.terraform.io/providers/bpg/proxmox/latest/docs/resources/virtual_environment_container#hook_script_file_id
[4]: https://registry.terraform.io/providers/bpg/proxmox/latest/docs/resources/virtual_environment_vm#hook_script_file_id
[5]: https://pkg.go.dev/os#FileMode

Related to GH-570
Related to GH-733

Signed-off-by: Sven Greb <development@svengreb.de>


---------

Signed-off-by: Sven Greb <development@svengreb.de>

docs/resources/virtual_environment_container.md

@@ -233,7 +233,7 @@ output "ubuntu_container_public_key" {
     - `keyctl` - (Optional) Whether the container supports `keyctl()` system
       call (defaults to `false`)
     - `mount` - (Optional) List of allowed mount types (`cifs` or `nfs`)
-- `hook_script_file_id` - (Optional) The identifier for a file containing a hook script (needs to be executable).
+- `hook_script_file_id` - (Optional) The identifier for a file containing a hook script (needs to be executable, e.g. by using the `proxmox_virtual_environment_file.file_mode` attribute).
 
 ## Attribute Reference
 

docs/resources/virtual_environment_file.md

@@ -80,6 +80,27 @@ resource "proxmox_virtual_environment_file" "cloud_config" {
 }
 ```
 
+The `file_mode` attribute can be used to make a script file executable, e.g. when referencing the file in the `hook_script_file_id` attribute of [a container](https://registry.terraform.io/providers/bpg/proxmox/latest/docs/resources/virtual_environment_container#hook_script_file_id) or [a VM](https://registry.terraform.io/providers/bpg/proxmox/latest/docs/resources/virtual_environment_vm#hook_script_file_id) resource which is a requirement enforced by the Proxmox VE API.
+
+```hcl
+resource "proxmox_virtual_environment_file" "hook_script" {
+  content_type = "snippets"
+  datastore_id = "local"
+  node_name    = "pve"
+  # Hook scripts must be executable, otherwise the Proxmox VE API will reject the configuration for the VM/CT.
+  file_mode    = "0700"
+
+  source_raw {
+    data      = <<-EOF
+      #!/usr/bin/env bash
+
+      echo "Running hook script"
+      EOF
+    file_name = "prepare-hook.sh"
+  }
+}
+```
+
 ### Container Template (`vztmpl`)
 
 -> Consider using `proxmox_virtual_environment_download_file` resource instead. Using this resource for container images is less efficient (requires to transfer uploaded image to node) though still supported.
@@ -105,6 +126,7 @@ resource "proxmox_virtual_environment_file" "ubuntu_container_template" {
     - `snippets` (allowed extensions: any)
     - `vztmpl` (allowed extensions: `.tar.gz`, `.tar.xz`, `tar.zst`)
 - `datastore_id` - (Required) The datastore id.
+- `file_mode` - The file mode in octal format, e.g. `0700` or `600`. Note that the prefixes `0o` and `0x` is not supported! Setting this attribute is also only allowed for `root@pam` authenticated user.
 - `node_name` - (Required) The node name.
 - `overwrite` - (Optional) Whether to overwrite an existing file (defaults to
     `true`).

docs/resources/virtual_environment_vm.md

@@ -545,7 +545,7 @@ output "ubuntu_vm_public_key" {
         - `vmware` - VMware Compatible.
     - `clipboard` - (Optional) Enable VNC clipboard by setting to `vnc`. See the [Proxmox documentation](https://pve.proxmox.com/pve-docs/pve-admin-guide.html#qm_virtual_machines_settings) section 10.2.8 for more information.
 - `vm_id` - (Optional) The VM identifier.
-- `hook_script_file_id` - (Optional) The identifier for a file containing a hook script (needs to be executable).
+- `hook_script_file_id` - (Optional) The identifier for a file containing a hook script (needs to be executable, e.g. by using the `proxmox_virtual_environment_file.file_mode` attribute). 
 
 ## Attribute Reference
 

proxmox/api/client_types.go

@@ -29,4 +29,9 @@ type FileUploadRequest struct {
 	ContentType string
 	FileName    string
 	File        *os.File
+	// Will be handled as unsigned 32-bit integer since the underlying type of os.FileMode is the same, but must be parsed
+	// as string due to the conversion of the octal format.
+	// References:
+	//   1. https://en.wikipedia.org/wiki/Chmod#Special_modes
+	Mode string
 }

proxmox/ssh/client.go

@@ -16,6 +16,7 @@ import (
 	"path"
 	"path/filepath"
 	"runtime"
+	"strconv"
 	"strings"
 
 	"github.com/hashicorp/terraform-plugin-log/tflog"
@@ -332,6 +333,17 @@ func (c *client) NodeStreamUpload(
 		return err
 	}
 
+	if d.Mode != "" {
+		parsedFileMode, parseErr := strconv.ParseUint(d.Mode, 8, 12)
+		if parseErr != nil {
+			return fmt.Errorf("failed to parse file mode %q: %w", d.Mode, err)
+		}
+
+		if err = c.changeModeUploadedFile(ctx, sshClient, remoteFilePath, os.FileMode(uint32(parsedFileMode))); err != nil {
+			return err
+		}
+	}
+
 	tflog.Debug(ctx, "uploaded file to datastore", map[string]interface{}{
 		"remote_file_path": remoteFilePath,
 	})
@@ -410,6 +422,49 @@ func (c *client) checkUploadedFile(
 	return nil
 }
 
+func (c *client) changeModeUploadedFile(
+	ctx context.Context,
+	sshClient *ssh.Client,
+	remoteFilePath string,
+	fileMode os.FileMode,
+) error {
+	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.Warn(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)
+	}
+
+	if err = sftpClient.Chmod(remoteFilePath, fileMode); err != nil {
+		return fmt.Errorf("failed to change file mode of remote file from %#o (%s) to %#o (%s): %w",
+			remoteStat.Mode().Perm(), remoteStat.Mode(), fileMode.Perm(), fileMode, err)
+	}
+
+	tflog.Debug(ctx, "changed mode of uploaded file", map[string]interface{}{
+		"before": fmt.Sprintf("%#o (%s)", remoteStat.Mode().Perm(), remoteStat.Mode()),
+		"after":  fmt.Sprintf("%#o (%s)", fileMode.Perm(), fileMode),
+	})
+
+	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

@@ -48,6 +48,7 @@ const (
 	mkResourceVirtualEnvironmentFileDatastoreID          = "datastore_id"
 	mkResourceVirtualEnvironmentFileFileModificationDate = "file_modification_date"
 	mkResourceVirtualEnvironmentFileFileName             = "file_name"
+	mkResourceVirtualEnvironmentFileFileMode             = "file_mode"
 	mkResourceVirtualEnvironmentFileFileSize             = "file_size"
 	mkResourceVirtualEnvironmentFileFileTag              = "file_tag"
 	mkResourceVirtualEnvironmentFileNodeName             = "node_name"
@@ -95,6 +96,15 @@ func File() *schema.Resource {
 				Description: "The file name",
 				Computed:    true,
 			},
+			mkResourceVirtualEnvironmentFileFileMode: {
+				Type: schema.TypeString,
+				Description: `The file mode in octal format, e.g. "0700" or "600".` +
+					`Note that the prefixes "0o" and "0x" are not supported!` +
+					`Setting this attribute is also only allowed for "root@pam" authenticated user.`,
+				Optional:         true,
+				ValidateDiagFunc: validators.FileMode(),
+				ForceNew:         true,
+			},
 			mkResourceVirtualEnvironmentFileFileSize: {
 				Type:        schema.TypeInt,
 				Description: "The file size in bytes",
@@ -308,6 +318,7 @@ func fileParseImportID(id string) (string, fileVolumeID, error) {
 
 func fileCreate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics {
 	uploadTimeout := d.Get(mkResourceVirtualEnvironmentFileTimeoutUpload).(int)
+	fileMode := d.Get(mkResourceVirtualEnvironmentFileFileMode).(string)
 
 	ctx, cancel := context.WithTimeout(ctx, time.Duration(uploadTimeout)*time.Second)
 	defer cancel()
@@ -538,6 +549,7 @@ func fileCreate(ctx context.Context, d *schema.ResourceData, m interface{}) diag
 		ContentType: *contentType,
 		FileName:    *fileName,
 		File:        file,
+		Mode:        fileMode,
 	}
 
 	switch *contentType {

proxmoxtf/resource/file_test.go

@@ -39,6 +39,7 @@ func TestFileSchema(t *testing.T) {
 	test.AssertOptionalArguments(t, s, []string{
 		mkResourceVirtualEnvironmentFileContentType,
 		mkResourceVirtualEnvironmentFileSourceFile,
+		mkResourceVirtualEnvironmentFileFileMode,
 		mkResourceVirtualEnvironmentFileSourceRaw,
 		mkResourceVirtualEnvironmentFileTimeoutUpload,
 	})
@@ -55,6 +56,7 @@ func TestFileSchema(t *testing.T) {
 		mkResourceVirtualEnvironmentFileDatastoreID:          schema.TypeString,
 		mkResourceVirtualEnvironmentFileFileModificationDate: schema.TypeString,
 		mkResourceVirtualEnvironmentFileFileName:             schema.TypeString,
+		mkResourceVirtualEnvironmentFileFileMode:             schema.TypeString,
 		mkResourceVirtualEnvironmentFileFileSize:             schema.TypeInt,
 		mkResourceVirtualEnvironmentFileFileTag:              schema.TypeString,
 		mkResourceVirtualEnvironmentFileNodeName:             schema.TypeString,

proxmoxtf/resource/validators/file.go

@@ -9,6 +9,7 @@ package validators
 import (
 	"fmt"
 	"regexp"
+	"strconv"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
@@ -66,6 +67,35 @@ func FileID() schema.SchemaValidateDiagFunc {
 	})
 }
 
+// FileMode is a schema validation function for file mode.
+func FileMode() schema.SchemaValidateDiagFunc {
+	return validation.ToDiagFunc(
+		func(i interface{}, k string) ([]string, []error) {
+			var errs []error
+
+			v, ok := i.(string)
+			if !ok {
+				errs = append(errs, fmt.Errorf(
+					`expected string in octal format (e.g. "0o700" or "0700"") for %q, but got %v of type %T`, k, v, i))
+				return nil, errs
+			}
+
+			mode, err := strconv.ParseInt(v, 10, 64)
+			if err != nil {
+				errs = append(errs, fmt.Errorf("failed to parse file mode %q: %w", v, err))
+				return nil, errs
+			}
+
+			if mode < 1 || mode > int64(^uint32(0)) {
+				errs = append(errs, fmt.Errorf("%q must be in the range (%d - %d), got %d", v, 1, ^uint32(0), mode))
+				return nil, errs
+			}
+
+			return []string{}, errs
+		},
+	)
+}
+
 // FileSize is a schema validation function for file size.
 func FileSize() schema.SchemaValidateDiagFunc {
 	return validation.ToDiagFunc(func(i interface{}, k string) ([]string, []error) {

proxmoxtf/resource/validators/file_test.go

@@ -41,3 +41,39 @@ func TestFileID(t *testing.T) {
 		})
 	}
 }
+
+func TestFileMode(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name  string
+		value string
+		valid bool
+	}{
+		{"valid", "0700", true},
+		{"invalid", "invalid", false},
+		// Even though Go supports octal prefixes, we should not allow them in the string value to reduce the complexity.
+		{"invalid", "0o700", false},
+		{"invalid", "0x700", false},
+		// Maximum value for uint32, incremented by one.
+		{"too large", "4294967296", false},
+		{"too small", "0", false},
+		{"negative", "-1", false},
+		{"empty", "", false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			f := FileMode()
+			res := f(tt.value, nil)
+
+			if tt.valid {
+				require.Empty(t, res, "validate: '%s'", tt.value)
+			} else {
+				require.NotEmpty(t, res, "validate: '%s'", tt.value)
+			}
+		})
+	}
+}