0
0
mirror of https://github.com/bpg/terraform-provider-proxmox.git synced 2025-06-29 18:21:10 +00:00

fix(vm): improve cpu.architecture handling (#1683)

* chore(tests): add option to select auth type (root user / token) for tests
* fix(vm): throw an error when `cpu.architecture` can't be set

---------

Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com>
This commit is contained in:
Pavel Boldyrev 2024-12-17 22:16:45 -05:00 committed by GitHub
parent 9d2118d762
commit be6f220779
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
13 changed files with 302 additions and 126 deletions

View File

@ -257,7 +257,7 @@ func (p *proxmoxProvider) Configure(
req provider.ConfigureRequest,
resp *provider.ConfigureResponse,
) {
tflog.Info(ctx, "Configuring the Proxmox provider...")
tflog.Info(ctx, "Configuring the Framework Proxmox provider...")
// Retrieve provider data from configuration
var cfg proxmoxProviderModel

View File

@ -1,3 +1,9 @@
/*
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
*/
//go:build acceptance || all
/*
@ -9,10 +15,13 @@
package test
import (
"math/rand"
"regexp"
"testing"
"github.com/hashicorp/terraform-plugin-testing/helper/resource"
"github.com/bpg/terraform-provider-proxmox/utils"
)
func TestAccResourceVM(t *testing.T) {
@ -142,6 +151,30 @@ func TestAccResourceVM(t *testing.T) {
),
}},
},
{
"set cpu.architecture as non root is not supported", []resource.TestStep{{
Config: te.RenderConfig(`
resource "proxmox_virtual_environment_vm" "test_cpu_arch" {
node_name = "{{.NodeName}}"
started = false
cpu {
architecture = "x86_64"
}
}`, WithAPIToken()),
ExpectError: regexp.MustCompile(`the CPU architecture can only be set by the root account`),
},
{
Config: te.RenderConfig(`
resource "proxmox_virtual_environment_vm" "template" {
node_name = "{{.NodeName}}"
started = false
cpu {
architecture = "x86_64"
}
}`, WithRootUser()),
Destroy: false,
}},
},
{
"update memory block", []resource.TestStep{{
Config: te.RenderConfig(`
@ -518,3 +551,49 @@ func TestAccResourceVMNetwork(t *testing.T) {
})
}
}
func TestAccResourceVMClone(t *testing.T) {
if utils.GetAnyStringEnv("TF_ACC") == "" {
t.Skip("Acceptance tests are disabled")
}
te := InitEnvironment(t)
te.AddTemplateVars(map[string]interface{}{
"TemplateVMID": 100000 + rand.Intn(99999),
})
tests := []struct {
name string
step []resource.TestStep
}{
{"clone cpu.architecture as root", []resource.TestStep{{
Config: te.RenderConfig(`
resource "proxmox_virtual_environment_vm" "template" {
node_name = "{{.NodeName}}"
vm_id = {{.TemplateVMID}}
started = false
cpu {
architecture = "x86_64"
}
}
resource "proxmox_virtual_environment_vm" "clone" {
node_name = "{{.NodeName}}"
started = false
clone {
vm_id = proxmox_virtual_environment_vm.template.vm_id
}
}`, WithRootUser()),
}}},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
resource.Test(t, resource.TestCase{
ProtoV6ProviderFactories: te.AccProviders,
Steps: tt.step,
})
})
}
}

View File

@ -25,12 +25,11 @@ import (
"github.com/bpg/terraform-provider-proxmox/fwprovider"
"github.com/bpg/terraform-provider-proxmox/proxmox/access"
"github.com/bpg/terraform-provider-proxmox/proxmox/cluster"
sdkV2provider "github.com/bpg/terraform-provider-proxmox/proxmoxtf/provider"
"github.com/bpg/terraform-provider-proxmox/proxmox/api"
"github.com/bpg/terraform-provider-proxmox/proxmox/cluster"
"github.com/bpg/terraform-provider-proxmox/proxmox/nodes"
"github.com/bpg/terraform-provider-proxmox/proxmox/nodes/storage"
sdkV2provider "github.com/bpg/terraform-provider-proxmox/proxmoxtf/provider"
"github.com/bpg/terraform-provider-proxmox/utils"
)
@ -38,7 +37,6 @@ import (
type Environment struct {
t *testing.T
templateVars map[string]any
providerConfig string
NodeName string
DatastoreID string
@ -49,10 +47,17 @@ type Environment struct {
ContainerImagesServer string
}
// InitEnvironment initializes a new test environment for acceptance tests.
func InitEnvironment(t *testing.T) *Environment {
t.Helper()
// RenderConfigOption is a configuration option for rendering the provider configuration.
type RenderConfigOption interface {
apply(rc *renderConfig) error
}
type renderConfig struct {
providerConfig string
}
// returns the ssh configuration section of the provider config.
func (r *renderConfig) ssh() string {
nodeName := utils.GetAnyStringEnv("PROXMOX_VE_ACC_NODE_NAME")
if nodeName == "" {
nodeName = "pve"
@ -61,8 +66,11 @@ func InitEnvironment(t *testing.T) *Environment {
nodeAddress := utils.GetAnyStringEnv("PROXMOX_VE_ACC_NODE_SSH_ADDRESS")
if nodeAddress == "" {
endpoint := utils.GetAnyStringEnv("PROXMOX_VE_ENDPOINT")
u, err := url.Parse(endpoint)
require.NoError(t, err)
if err != nil {
panic(err)
}
nodeAddress = u.Hostname()
}
@ -72,18 +80,67 @@ func InitEnvironment(t *testing.T) *Environment {
nodePort = "22"
}
pc := fmt.Sprintf(`
provider "proxmox" {
// one indent level
return fmt.Sprintf(`
ssh {
node {
name = "%s"
address = "%s"
port = %s
}
}
//random_vm_ids = true
}`, nodeName, nodeAddress, nodePort)
}
`, nodeName, nodeAddress, nodePort)
// WithRootUser returns a configuration option that sets the root user in the provider configuration.
func WithRootUser() RenderConfigOption {
return &rootUserConfigOption{}
}
type rootUserConfigOption struct{}
func (o *rootUserConfigOption) apply(rc *renderConfig) error {
if utils.GetAnyStringEnv("PROXMOX_VE_USERNAME") == "" || utils.GetAnyStringEnv("PROXMOX_VE_PASSWORD") == "" {
return fmt.Errorf("PROXMOX_VE_USERNAME and PROXMOX_VE_PASSWORD must be set")
}
rootUser := fmt.Sprintf("\tusername = \"%s\"\n\tpassword = \"%s\"\n\tapi_token = \"\"",
utils.GetAnyStringEnv("PROXMOX_VE_USERNAME"),
utils.GetAnyStringEnv("PROXMOX_VE_PASSWORD"),
)
rc.providerConfig = fmt.Sprintf("provider \"proxmox\" {\n%s\n%s\n}", rootUser, rc.ssh())
return nil
}
// WithAPIToken returns a configuration option that sets the API token in the provider configuration.
func WithAPIToken() RenderConfigOption {
return &apiTokenConfigOption{}
}
type apiTokenConfigOption struct{}
func (o *apiTokenConfigOption) apply(rc *renderConfig) error {
if utils.GetAnyStringEnv("PROXMOX_VE_API_TOKEN") == "" {
return fmt.Errorf("PROXMOX_VE_API_TOKEN must be set")
}
apiToken := fmt.Sprintf("\tapi_token = \"%s\"\n\tusername = \"\"\n\tpassword = \"\"",
utils.GetAnyStringEnv("PROXMOX_VE_API_TOKEN"))
rc.providerConfig = fmt.Sprintf("provider \"proxmox\" {\n%s\n%s\n}", apiToken, rc.ssh())
return nil
}
// InitEnvironment initializes a new test environment for acceptance tests.
func InitEnvironment(t *testing.T) *Environment {
t.Helper()
nodeName := utils.GetAnyStringEnv("PROXMOX_VE_ACC_NODE_NAME")
if nodeName == "" {
nodeName = "pve"
}
const datastoreID = "local"
@ -100,18 +157,17 @@ provider "proxmox" {
return &Environment{
t: t,
templateVars: map[string]any{
"ProviderConfig": pc,
"NodeName": nodeName,
"DatastoreID": datastoreID,
"CloudImagesServer": cloudImagesServer,
"ContainerImagesServer": containerImagesServer,
},
providerConfig: pc,
NodeName: nodeName,
DatastoreID: datastoreID,
AccProviders: muxProviders(t),
CloudImagesServer: cloudImagesServer,
ContainerImagesServer: containerImagesServer,
AccProviders: muxProviders(t),
}
}
@ -125,18 +181,32 @@ func (e *Environment) AddTemplateVars(vars map[string]any) {
}
// RenderConfig renders the given configuration with for the current test environment using template engine.
func (e *Environment) RenderConfig(cfg string) string {
tmpl, err := template.New("config").Parse("{{.ProviderConfig}}" + cfg)
func (e *Environment) RenderConfig(cfg string, opt ...RenderConfigOption) string {
if len(opt) == 0 {
opt = append(opt, WithAPIToken())
}
rc := &renderConfig{}
for _, o := range opt {
err := o.apply(rc)
require.NoError(e.t, err, "configuration error")
}
tmpl, err := template.New("config").Parse(cfg)
require.NoError(e.t, err)
var buf bytes.Buffer
err = tmpl.Execute(&buf, e.templateVars)
require.NoError(e.t, err)
return buf.String()
return rc.providerConfig + "\n" + buf.String()
}
// Client returns a new API client for the test environment.
// The client will be using the credentials from the environment variables, in precedence order:
// 1. API token
// 2. Ticket
// 3. User credentials.
func (e *Environment) Client() api.Client {
if e.c == nil {
e.once.Do(
@ -188,15 +258,18 @@ func (e *Environment) ClusterClient() *cluster.Client {
return &cluster.Client{Client: e.Client()}
}
// testAccMuxProviders returns a map of mux servers for the acceptance tests.
// muxProviders returns a map of mux servers for the acceptance tests.
func muxProviders(t *testing.T) map[string]func() (tfprotov6.ProviderServer, error) {
t.Helper()
ctx := context.Background()
// Init sdkV2 provider
// Init mux servers
return map[string]func() (tfprotov6.ProviderServer, error){
"proxmox": func() (tfprotov6.ProviderServer, error) {
return tf6muxserver.NewMuxServer(context.Background(),
providerserver.NewProtocol6(fwprovider.New("test")()),
func() tfprotov6.ProviderServer {
sdkV2Provider, err := tf5to6server.UpgradeServer(
ctx,
context.Background(),
func() tfprotov5.ProviderServer {
return schema.NewGRPCProviderServer(
sdkV2provider.ProxmoxVirtualEnvironment(),
@ -205,26 +278,9 @@ func muxProviders(t *testing.T) map[string]func() (tfprotov6.ProviderServer, err
)
require.NoError(t, err)
// Init framework provider
frameworkProvider := fwprovider.New("test")()
providers := []func() tfprotov6.ProviderServer{
providerserver.NewProtocol6(frameworkProvider),
func() tfprotov6.ProviderServer {
return sdkV2Provider
},
}
// Init mux servers
muxServers := map[string]func() (tfprotov6.ProviderServer, error){
"proxmox": func() (tfprotov6.ProviderServer, error) {
muxServer, e := tf6muxserver.NewMuxServer(ctx, providers...)
if e != nil {
return nil, fmt.Errorf("failed to create mux server: %w", e)
}
return muxServer, nil
)
},
}
return muxServers
}

View File

@ -16,11 +16,11 @@ import (
// is configured to use the root user.
type Authenticator interface {
// IsRoot returns true if the authenticator is configured to use the root
IsRoot() bool
IsRoot(ctx context.Context) bool
// 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
IsRootTicket(ctx context.Context) bool
// AuthenticateRequest adds authentication data to a new request.
AuthenticateRequest(ctx context.Context, req *http.Request) error

View File

@ -48,11 +48,11 @@ type Client interface {
ExpandPath(path string) string
// IsRoot returns true if the client is configured with the root user.
IsRoot() bool
IsRoot(ctx context.Context) bool
// 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
IsRootTicket(ctx context.Context) bool
// HTTP returns a lower-level HTTP client.
HTTP() *http.Client
@ -313,12 +313,12 @@ func (c *client) ExpandPath(path string) string {
return path
}
func (c *client) IsRoot() bool {
return c.auth.IsRoot()
func (c *client) IsRoot(ctx context.Context) bool {
return c.auth.IsRoot(ctx)
}
func (c *client) IsRootTicket() bool {
return c.auth.IsRootTicket()
func (c *client) IsRootTicket(ctx context.Context) bool {
return c.auth.IsRootTicket(ctx)
}
func (c *client) HTTP() *http.Client {

View File

@ -1,3 +1,9 @@
/*
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
*/
package api
import (
@ -29,11 +35,11 @@ func newTestClient(fn RoundTripFunc) *http.Client {
type dummyAuthenticator struct{}
func (dummyAuthenticator) IsRoot() bool {
func (dummyAuthenticator) IsRoot(_ context.Context) bool {
return false
}
func (dummyAuthenticator) IsRootTicket() bool {
func (dummyAuthenticator) IsRootTicket(context.Context) bool {
return false
}

View File

@ -40,12 +40,12 @@ func NewTicketAuthenticator(creds TicketCredentials) (Authenticator, error) {
}, nil
}
func (t *ticketAuthenticator) IsRoot() bool {
func (t *ticketAuthenticator) IsRoot(_ context.Context) bool {
return t.authData != nil && t.authData.Username == rootUsername
}
func (t *ticketAuthenticator) IsRootTicket() bool {
return t.IsRoot()
func (t *ticketAuthenticator) IsRootTicket(ctx context.Context) bool {
return t.IsRoot(ctx)
}
// AuthenticateRequest adds authentication data to a new request.

View File

@ -26,11 +26,11 @@ func NewTokenAuthenticator(toc TokenCredentials) (Authenticator, error) {
}, nil
}
func (t *tokenAuthenticator) IsRoot() bool {
func (t *tokenAuthenticator) IsRoot(_ context.Context) bool {
return t.username == rootUsername
}
func (t *tokenAuthenticator) IsRootTicket() bool {
func (t *tokenAuthenticator) IsRootTicket(_ context.Context) bool {
// Logged using a token, therefore not a ticket login
return false
}

View File

@ -121,12 +121,22 @@ func (t *userAuthenticator) authenticate(ctx context.Context) (*AuthenticationRe
return resBody.Data, nil
}
func (t *userAuthenticator) IsRoot() bool {
func (t *userAuthenticator) IsRoot(ctx context.Context) bool {
if t.authData == nil {
if _, err := t.authenticate(ctx); err != nil {
tflog.Warn(ctx, "Failed to authenticate while checking root status", map[string]interface{}{
"error": err.Error(),
})
return false
}
}
return t.authData != nil && t.authData.Username == rootUsername
}
func (t *userAuthenticator) IsRootTicket() bool {
return t.IsRoot()
func (t *userAuthenticator) IsRootTicket(ctx context.Context) bool {
return t.IsRoot(ctx)
}
// AuthenticateRequest adds authentication data to a new request.

View File

@ -34,7 +34,7 @@ func ProxmoxVirtualEnvironment() *schema.Provider {
}
}
func providerConfigure(_ context.Context, d *schema.ResourceData) (interface{}, diag.Diagnostics) {
func providerConfigure(ctx context.Context, d *schema.ResourceData) (interface{}, diag.Diagnostics) {
var err error
var diags diag.Diagnostics
@ -47,6 +47,8 @@ func providerConfigure(_ context.Context, d *schema.ResourceData) (interface{},
var conn *api.Connection
tflog.Info(ctx, "Configuring the SDK Proxmox provider...")
// Check environment variables
endpoint := utils.GetAnyStringEnv("PROXMOX_VE_ENDPOINT", "PM_VE_ENDPOINT")
insecure := utils.GetAnyBoolEnv("PROXMOX_VE_INSECURE", "PM_VE_INSECURE")
@ -78,7 +80,8 @@ func providerConfigure(_ context.Context, d *schema.ResourceData) (interface{},
csrfPreventionToken = v.(string)
}
if v, ok := d.GetOk(mkProviderAPIToken); ok {
//nolint:staticcheck //using GetOkExists to check if the value is set, as it can be an empty string in tests
if v, ok := d.GetOkExists(mkProviderAPIToken); ok {
apiToken = v.(string)
}
@ -86,11 +89,13 @@ func providerConfigure(_ context.Context, d *schema.ResourceData) (interface{},
otp = v.(string)
}
if v, ok := d.GetOk(mkProviderUsername); ok {
///nolint:staticcheck //using GetOkExists to check if the value is set, as it can be an empty string in tests
if v, ok := d.GetOkExists(mkProviderUsername); ok {
username = v.(string)
}
if v, ok := d.GetOk(mkProviderPassword); ok {
//nolint:staticcheck //using GetOkExists to check if the value is set, as it can be an empty string in tests
if v, ok := d.GetOkExists(mkProviderPassword); ok {
password = v.(string)
}

View File

@ -81,7 +81,7 @@ func createSchema() map[string]*schema.Schema {
Optional: true,
Sensitive: true,
Description: "The API token for the Proxmox VE API.",
ValidateFunc: validation.StringIsNotEmpty,
// note: we allow empty string as a valid value, as it is used to unset the token in tests
},
mkProviderOTP: {
Type: schema.TypeString,
@ -94,14 +94,14 @@ func createSchema() map[string]*schema.Schema {
Type: schema.TypeString,
Optional: true,
Description: "The username for the Proxmox VE API.",
ValidateFunc: validation.StringIsNotEmpty,
// note: we allow empty string as a valid value, as it is used to unset the username in tests
},
mkProviderPassword: {
Type: schema.TypeString,
Optional: true,
Sensitive: true,
Description: "The password for the Proxmox VE API.",
ValidateFunc: validation.StringIsNotEmpty,
// note: we allow empty string as a valid value, as it is used to unset the password in tests
},
mkProviderSSH: {
Type: schema.TypeList,

View File

@ -25,6 +25,7 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
"github.com/bpg/terraform-provider-proxmox/proxmox"
"github.com/bpg/terraform-provider-proxmox/proxmox/api"
"github.com/bpg/terraform-provider-proxmox/proxmox/cluster"
"github.com/bpg/terraform-provider-proxmox/proxmox/helpers/ptr"
@ -1531,6 +1532,10 @@ func VM() *schema.Resource {
customdiff.ForceNewIf(
mkVMID,
func(_ context.Context, d *schema.ResourceDiff, _ interface{}) bool {
if !d.HasChange(mkVMID) {
return false
}
newValue := d.Get(mkVMID)
// 'vm_id' is ForceNew, except when changing 'vm_id' to existing correct id
@ -1541,6 +1546,10 @@ func VM() *schema.Resource {
customdiff.ForceNewIf(
mkNodeName,
func(_ context.Context, d *schema.ResourceDiff, _ interface{}) bool {
if !d.HasChange(mkNodeName) {
return false
}
return !d.Get(mkMigrate).(bool)
},
),
@ -1945,9 +1954,8 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d
cpuFlagsConverted[fi] = flag.(string)
}
// Only the root account is allowed to change the CPU architecture, which makes this check necessary.
if client.API().IsRootTicket() && cpuArchitecture != "" {
updateBody.CPUArchitecture = &cpuArchitecture
if err := setCPUArchitecture(ctx, cpuArchitecture, client, updateBody); err != nil {
return diag.FromErr(err)
}
updateBody.CPUCores = ptr.Ptr(int64(cpuCores))
@ -2295,6 +2303,25 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d
return vmCreateStart(ctx, d, m)
}
func setCPUArchitecture(
ctx context.Context,
cpuArchitecture string,
client proxmox.Client,
updateBody *vms.UpdateRequestBody,
) error {
// Only the root account is allowed to change the CPU architecture.
if cpuArchitecture != "" {
if client.API().IsRootTicket(ctx) {
updateBody.CPUArchitecture = &cpuArchitecture
} else {
return errors.New("the `cpu.architecture` can only be set by the root account. " +
"Please switch to the root account or remove the `cpu.architecture` from the VM configuration")
}
}
return nil
}
func vmCreateCustom(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics {
createTimeoutSec := d.Get(mkTimeoutCreate).(int)
@ -2672,9 +2699,8 @@ func vmCreateCustom(ctx context.Context, d *schema.ResourceData, m interface{})
CustomStorageDevices: diskDeviceObjects,
}
// Only the root account is allowed to change the CPU architecture, which makes this check necessary.
if client.API().IsRootTicket() && cpuArchitecture != "" {
createBody.CPUArchitecture = &cpuArchitecture
if err = setCPUArchitecture(ctx, cpuArchitecture, client, createBody); err != nil {
return diag.FromErr(err)
}
if cpuHotplugged > 0 {
@ -3570,13 +3596,8 @@ func vmReadCustom(
cpu[mkCPUArchitecture] = *vmConfig.CPUArchitecture
} else {
// Default value of "arch" is "" according to the API documentation.
// However, assume the provider's default value as a workaround when the root account is not being used.
if !client.API().IsRootTicket() {
cpu[mkCPUArchitecture] = dvCPUArchitecture
} else {
cpu[mkCPUArchitecture] = ""
}
}
if vmConfig.CPUCores != nil {
cpu[mkCPUCores] = int(*vmConfig.CPUCores)
@ -4987,9 +5008,8 @@ func vmUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.D
cpuUnits := cpuBlock[mkCPUUnits].(int)
cpuAffinity := cpuBlock[mkCPUAffinity].(string)
// Only the root account is allowed to change the CPU architecture, which makes this check necessary.
if client.API().IsRootTicket() && cpuArchitecture != "" {
updateBody.CPUArchitecture = &cpuArchitecture
if err = setCPUArchitecture(ctx, cpuArchitecture, client, updateBody); err != nil {
return diag.FromErr(err)
}
updateBody.CPUCores = ptr.Ptr(int64(cpuCores))

View File

@ -7,4 +7,4 @@
#
# shellcheck disable=SC2046
TF_ACC=1 env $(xargs < testacc.env) go test -v -count 1 --tags=acceptance -timeout 360s -run "$1" github.com/bpg/terraform-provider-proxmox/fwprovider/... $2
TF_ACC=1 env $(xargs < testacc.env) go test -count 1 --tags=acceptance -timeout 360s -run "$1" github.com/bpg/terraform-provider-proxmox/fwprovider/... $2