From 797873b257614246fbadf167e7649cc5ed8e17e8 Mon Sep 17 00:00:00 2001 From: Pavel Boldyrev <627562+bpg@users.noreply.github.com> Date: Thu, 15 Feb 2024 19:33:23 -0500 Subject: [PATCH] fix(vm): multi-line description field is always marked as changed (#1030) Also, fix acceptance tests Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com> --- .vscode/settings.json | 1 + fwprovider/tests/resource_container_test.go | 9 ++- fwprovider/tests/resource_file_test.go | 14 +++- proxmox/api/client.go | 7 ++ proxmox/ssh/client.go | 12 +++ proxmoxtf/resource/container.go | 5 +- proxmoxtf/resource/file.go | 82 +++++++++++---------- proxmoxtf/resource/sudo.go | 15 ---- proxmoxtf/resource/vm.go | 15 +++- 9 files changed, 101 insertions(+), 59 deletions(-) delete mode 100644 proxmoxtf/resource/sudo.go diff --git a/.vscode/settings.json b/.vscode/settings.json index 1b4a084d..aaf27a40 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -10,6 +10,7 @@ "qcow", "rootfs", "signoff", + "stretchr", "tflog", "unmanaged", "virtio", diff --git a/fwprovider/tests/resource_container_test.go b/fwprovider/tests/resource_container_test.go index 9fd25b68..1b5181ea 100644 --- a/fwprovider/tests/resource_container_test.go +++ b/fwprovider/tests/resource_container_test.go @@ -43,6 +43,12 @@ func TestAccResourceContainer(t *testing.T) { func testAccResourceContainerCreateConfig(isTemplate bool) string { return fmt.Sprintf(` +resource "proxmox_virtual_environment_download_file" "ubuntu_container_template" { + content_type = "vztmpl" + datastore_id = "local" + node_name = "pve" + url = "http://download.proxmox.com/images/system/ubuntu-23.04-standard_23.04-1_amd64.tar.zst" +} resource "proxmox_virtual_environment_container" "test_container" { node_name = "%s" vm_id = 1100 @@ -74,8 +80,7 @@ resource "proxmox_virtual_environment_container" "test_container" { } operating_system { - # TODO: this file needs to be upload to PVE first - template_file_id = "local:vztmpl/ubuntu-23.04-standard_23.04-1_amd64.tar.zst" + template_file_id = proxmox_virtual_environment_download_file.ubuntu_container_template.id type = "ubuntu" } } diff --git a/fwprovider/tests/resource_file_test.go b/fwprovider/tests/resource_file_test.go index 466b201b..1f336aee 100644 --- a/fwprovider/tests/resource_file_test.go +++ b/fwprovider/tests/resource_file_test.go @@ -23,6 +23,7 @@ import ( "github.com/bpg/terraform-provider-proxmox/proxmox/api" "github.com/bpg/terraform-provider-proxmox/proxmox/ssh" + "github.com/bpg/terraform-provider-proxmox/utils" ) @@ -144,13 +145,22 @@ func uploadSnippetFile(t *testing.T, file *os.File) { defer f.Close() - err = sshClient.NodeUpload(context.Background(), "pve", "/var/lib/vz", + fname := filepath.Base(file.Name()) + err = sshClient.NodeUpload(context.Background(), "pve", "/tmp/tfpve/testacc", &api.FileUploadRequest{ ContentType: "snippets", - FileName: filepath.Base(file.Name()), + 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 { diff --git a/proxmox/api/client.go b/proxmox/api/client.go index 976cbad8..4c28e5c4 100644 --- a/proxmox/api/client.go +++ b/proxmox/api/client.go @@ -52,6 +52,9 @@ type Client interface { // IsRootTicket returns true if the authenticator is configured to use the root directly using a login ticket. // (root using token is weaker, cannot change VM arch) IsRootTicket() bool + + // HTTP returns a lower-level HTTP client. + HTTP() *http.Client } // Connection represents a connection to the Proxmox Virtual Environment API. @@ -298,6 +301,10 @@ func (c *client) IsRootTicket() bool { return c.auth.IsRootTicket() } +func (c *client) HTTP() *http.Client { + return c.conn.httpClient +} + // validateResponseCode ensures that a response is valid. func validateResponseCode(res *http.Response) error { if res.StatusCode < 200 || res.StatusCode >= 300 { diff --git a/proxmox/ssh/client.go b/proxmox/ssh/client.go index 353c4da9..16c545a6 100644 --- a/proxmox/ssh/client.go +++ b/proxmox/ssh/client.go @@ -28,6 +28,18 @@ import ( "github.com/bpg/terraform-provider-proxmox/utils" ) +const ( + // TrySudo is a shell function that tries to execute a command with sudo if the user has sudo permissions. + TrySudo = `try_sudo(){ if [ $(sudo -n pvesm apiinfo 2>&1 | grep "APIVER" | wc -l) -gt 0 ]; then sudo $1; else $1; fi }` +) + +// NewErrUserHasNoPermission creates a new error indicating that the SSH user does not have required permissions. +func NewErrUserHasNoPermission(username string) error { + return fmt.Errorf("the SSH user '%s' does not have required permissions. "+ + "Make sure 'sudo' is installed and the user is configured in sudoers file. "+ + "Refer to the documentation for more details", username) +} + // Client is an interface for performing SSH requests against the Proxmox Nodes. type Client interface { // Username returns the SSH username. diff --git a/proxmoxtf/resource/container.go b/proxmoxtf/resource/container.go index d73af221..d71bc6b1 100644 --- a/proxmoxtf/resource/container.go +++ b/proxmoxtf/resource/container.go @@ -283,7 +283,10 @@ func Container() *schema.Resource { StateFunc: func(i interface{}) string { // PVE always adds a newline to the description, so we have to do the same, // also taking in account the CLRF case (Windows) - return strings.ReplaceAll(strings.TrimSpace(i.(string)), "\r\n", "\n") + "\n" + if i.(string) != "" { + return strings.ReplaceAll(strings.TrimSpace(i.(string)), "\r\n", "\n") + "\n" + } + return "" }, }, mkResourceVirtualEnvironmentContainerDisk: { diff --git a/proxmoxtf/resource/file.go b/proxmoxtf/resource/file.go index c71dbda8..71028dc5 100644 --- a/proxmoxtf/resource/file.go +++ b/proxmoxtf/resource/file.go @@ -30,6 +30,7 @@ import ( "golang.org/x/exp/slices" "github.com/bpg/terraform-provider-proxmox/proxmox/api" + "github.com/bpg/terraform-provider-proxmox/proxmox/ssh" "github.com/bpg/terraform-provider-proxmox/proxmoxtf" "github.com/bpg/terraform-provider-proxmox/proxmoxtf/resource/validator" "github.com/bpg/terraform-provider-proxmox/utils" @@ -594,7 +595,7 @@ func fileCreate(ctx context.Context, d *schema.ResourceData, m interface{}) diag _, 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`, - trySudo, + ssh.TrySudo, srcDir, *fileName, dstDir, *fileName, srcDir, @@ -603,7 +604,7 @@ func fileCreate(ctx context.Context, d *schema.ResourceData, m interface{}) diag }) if err != nil { if matches, e := regexp.MatchString(`cannot move .* Permission denied`, err.Error()); e == nil && matches { - return diag.FromErr(newErrSSHUserNoPermission(capi.SSH().Username())) + return diag.FromErr(ssh.NewErrUserHasNoPermission(capi.SSH().Username())) } diags = append(diags, diag.Errorf("error moving file: %s", err.Error())...) @@ -776,7 +777,7 @@ func fileRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag.D readFileAttrs := readFile if fileIsURL(d) { - readFileAttrs = readURL + readFileAttrs = readURL(capi.API().HTTP()) } var diags diag.Diagnostics @@ -873,50 +874,57 @@ func readFile( return fileModificationDate, fileSize, fileTag, nil } -//nolint:nonamedreturns func readURL( + httClient *http.Client, +) func( ctx context.Context, sourceFilePath string, ) (fileModificationDate string, fileSize int64, fileTag string, err error) { - res, err := http.Head(sourceFilePath) - if err != nil { - return - } - - defer utils.CloseOrLogError(ctx)(res.Body) - - fileSize = res.ContentLength - httpLastModified := res.Header.Get("Last-Modified") - - if httpLastModified != "" { - var timeParsed time.Time - timeParsed, err = time.Parse(time.RFC1123, httpLastModified) - + return func( + ctx context.Context, + sourceFilePath string, + ) (string, int64, string, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodHead, sourceFilePath, nil) if err != nil { - timeParsed, err = time.Parse(time.RFC1123Z, httpLastModified) + return "", 0, "", fmt.Errorf("failed to create a new request: %w", err) + } + + res, err := httClient.Do(req) //nolint:bodyclose + if err != nil { + return "", 0, "", fmt.Errorf("failed to HEAD the URL: %w", err) + } + + defer utils.CloseOrLogError(ctx)(res.Body) + + fileModificationDate := "" + fileSize := res.ContentLength + fileTag := "" + + httpLastModified := res.Header.Get("Last-Modified") + if httpLastModified != "" { + var timeParsed time.Time + timeParsed, err = time.Parse(time.RFC1123, httpLastModified) + if err != nil { - return + timeParsed, err = time.Parse(time.RFC1123Z, httpLastModified) + if err != nil { + return fileModificationDate, fileSize, fileTag, fmt.Errorf("failed to parse Last-Modified header: %w", err) + } + } + + fileModificationDate = timeParsed.UTC().Format(time.RFC3339) + } + + httpTag := res.Header.Get("ETag") + if httpTag != "" { + httpTagParts := strings.Split(httpTag, "\"") + if len(httpTagParts) > 1 { + fileTag = httpTagParts[1] } } - fileModificationDate = timeParsed.UTC().Format(time.RFC3339) + return fileModificationDate, fileSize, fileTag, nil } - - httpTag := res.Header.Get("ETag") - - if httpTag != "" { - httpTagParts := strings.Split(httpTag, "\"") - - if len(httpTagParts) > 1 { - fileTag = httpTagParts[1] - } else { - fileTag = "" - } - } else { - fileTag = "" - } - - return } func fileDelete(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { diff --git a/proxmoxtf/resource/sudo.go b/proxmoxtf/resource/sudo.go deleted file mode 100644 index d9598a14..00000000 --- a/proxmoxtf/resource/sudo.go +++ /dev/null @@ -1,15 +0,0 @@ -package resource - -import ( - "fmt" -) - -const ( - trySudo = `try_sudo(){ if [ $(sudo -n pvesm apiinfo 2>&1 | grep "APIVER" | wc -l) -gt 0 ]; then sudo $1; else $1; fi }` -) - -func newErrSSHUserNoPermission(username string) error { - return fmt.Errorf("the SSH user '%s' does not have required permissions. "+ - "Make sure 'sudo' is installed and the user is configured in sudoers file. "+ - "Refer to the documentation for more details", username) -} diff --git a/proxmoxtf/resource/vm.go b/proxmoxtf/resource/vm.go index b2df642c..041a4fe5 100644 --- a/proxmoxtf/resource/vm.go +++ b/proxmoxtf/resource/vm.go @@ -18,6 +18,8 @@ import ( "time" "unicode" + "github.com/bpg/terraform-provider-proxmox/proxmox/ssh" + "github.com/google/go-cmp/cmp" "github.com/google/uuid" "github.com/hashicorp/terraform-plugin-log/tflog" @@ -619,6 +621,15 @@ func VM() *schema.Resource { Description: "The description", Optional: true, Default: dvResourceVirtualEnvironmentVMDescription, + StateFunc: func(i interface{}) string { + // PVE always adds a newline to the description, so we have to do the same, + // also taking in account the CLRF case (Windows) + if i.(string) != "" { + return strings.ReplaceAll(strings.TrimSpace(i.(string)), "\r\n", "\n") + "\n" + } + + return "" + }, }, mkResourceVirtualEnvironmentVMDisk: { Type: schema.TypeList, @@ -2975,7 +2986,7 @@ func vmCreateCustomDisks(ctx context.Context, d *schema.ResourceData, m interfac commands = append( commands, `set -e`, - trySudo, + ssh.TrySudo, fmt.Sprintf(`file_id="%s"`, fileID), fmt.Sprintf(`file_format="%s"`, fileFormat), fmt.Sprintf(`datastore_id_target="%s"`, datastoreID), @@ -3009,7 +3020,7 @@ func vmCreateCustomDisks(ctx context.Context, d *schema.ResourceData, m interfac out, err := api.SSH().ExecuteNodeCommands(ctx, nodeName, commands) if err != nil { if matches, e := regexp.Match(`pvesm: .* not found`, out); e == nil && matches { - return diag.FromErr(newErrSSHUserNoPermission(api.SSH().Username())) + return diag.FromErr(ssh.NewErrUserHasNoPermission(api.SSH().Username())) } return diag.FromErr(err)