diff --git a/fwprovider/attribute/planmodifiers/use_unknown_for_null_config_list.go b/fwprovider/attribute/planmodifiers/use_unknown_for_null_config_list.go index 0911bfdc..904c57f3 100644 --- a/fwprovider/attribute/planmodifiers/use_unknown_for_null_config_list.go +++ b/fwprovider/attribute/planmodifiers/use_unknown_for_null_config_list.go @@ -10,13 +10,18 @@ import ( "context" "github.com/hashicorp/terraform-plugin-framework/attr" + "github.com/hashicorp/terraform-plugin-framework/path" "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" "github.com/hashicorp/terraform-plugin-framework/types" + + "github.com/bpg/terraform-provider-proxmox/fwprovider/attribute" ) -// UseUnknownForNullConfigList returns a plan modifier sets the value of an attribute -// to Unknown if the attribute is missing from the plan and the config is null. -// Use this for optional computed attributes that can be reset / removed by the user. +// UseUnknownForNullConfigList returns a plan modifier that sets the value of an attribute +// to Unknown if the attribute is missing from the plan and the config is null AND the resource is not a clone. +// +// Use this for optional computed attributes that can be reset / removed by the user. If the resource is a clone, +// the value will be copied from the prior state (e.g. the clone source). // // The behavior for Terraform for Optional + Computed attributes is to copy the prior state // if there is no configuration for it. This plan modifier will instead set the value to Unknown, @@ -43,11 +48,26 @@ func (m useUnknownForNullConfigList) MarkdownDescription(_ context.Context) stri // PlanModifyList implements the plan modification logic. func (m useUnknownForNullConfigList) PlanModifyList( - _ context.Context, + ctx context.Context, req planmodifier.ListRequest, resp *planmodifier.ListResponse, ) { - if !req.PlanValue.IsNull() && req.ConfigValue.IsNull() { + if !m.isClone(ctx, req) { + if req.PlanValue.IsNull() { + return + } + + if !req.ConfigValue.IsNull() { + return + } + resp.PlanValue = types.ListUnknown(m.elementType) } } + +func (m useUnknownForNullConfigList) isClone(ctx context.Context, req planmodifier.ListRequest) bool { + var cloneID types.Int64 + _ = req.Plan.GetAttribute(ctx, path.Root("clone").AtName("id"), &cloneID) + + return attribute.IsDefined(cloneID) +} diff --git a/fwprovider/attribute/planmodifiers/use_unknown_for_null_config_string.go b/fwprovider/attribute/planmodifiers/use_unknown_for_null_config_string.go index 23888aba..2200ead5 100644 --- a/fwprovider/attribute/planmodifiers/use_unknown_for_null_config_string.go +++ b/fwprovider/attribute/planmodifiers/use_unknown_for_null_config_string.go @@ -9,13 +9,18 @@ package planmodifiers import ( "context" + "github.com/hashicorp/terraform-plugin-framework/path" "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" "github.com/hashicorp/terraform-plugin-framework/types" + + "github.com/bpg/terraform-provider-proxmox/fwprovider/attribute" ) -// UseUnknownForNullConfigString returns a plan modifier sets the value of an attribute -// to Unknown if the attribute is missing from the plan and the config is null. -// Use this for optional computed attributes that can be reset / removed by the user. +// UseUnknownForNullConfigString returns a plan modifier that sets the value of an attribute +// to Unknown if the attribute is missing from the plan and the config is null AND the resource is not a clone. +// +// Use this for optional computed attributes that can be reset / removed by the user. If the resource is a clone, +// the value will be copied from the prior state (e.g. the clone source). // // The behavior for Terraform for Optional + Computed attributes is to copy the prior state // if there is no configuration for it. This plan modifier will instead set the value to Unknown, @@ -40,11 +45,26 @@ func (m useUnknownForNullConfigString) MarkdownDescription(_ context.Context) st // PlanModifyString implements the plan modification logic. func (m useUnknownForNullConfigString) PlanModifyString( - _ context.Context, + ctx context.Context, req planmodifier.StringRequest, resp *planmodifier.StringResponse, ) { - if !req.PlanValue.IsNull() && req.ConfigValue.IsNull() { + if !m.isClone(ctx, req) { + if req.PlanValue.IsNull() { + return + } + + if !req.ConfigValue.IsNull() { + return + } + resp.PlanValue = types.StringUnknown() } } + +func (m useUnknownForNullConfigString) isClone(ctx context.Context, req planmodifier.StringRequest) bool { + var cloneID types.Int64 + _ = req.Plan.GetAttribute(ctx, path.Root("clone").AtName("id"), &cloneID) + + return attribute.IsDefined(cloneID) +} diff --git a/fwprovider/vm/cloudinit/model.go b/fwprovider/vm/cloudinit/model.go index e16901bd..4713c494 100644 --- a/fwprovider/vm/cloudinit/model.go +++ b/fwprovider/vm/cloudinit/model.go @@ -7,17 +7,33 @@ package cloudinit import ( + "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/types" ) -// Model represents the cloud-init model. +// Model represents the Cloud-Init model. type Model struct { DatastoreID types.String `tfsdk:"datastore_id"` Interface types.String `tfsdk:"interface"` - DNS *ModelDNS `tfsdk:"dns"` + DNS DNSValue `tfsdk:"dns"` } type ModelDNS struct { Domain types.String `tfsdk:"domain"` Servers types.List `tfsdk:"servers"` } + +func attributeTypes() map[string]attr.Type { + return map[string]attr.Type{ + "datastore_id": types.StringType, + "interface": types.StringType, + "dns": types.ObjectType{}.WithAttributeTypes(attributeTypesDNS()), + } +} + +func attributeTypesDNS() map[string]attr.Type { + return map[string]attr.Type{ + "domain": types.StringType, + "servers": types.ListType{ElemType: types.StringType}, + } +} diff --git a/fwprovider/vm/cloudinit/resource.go b/fwprovider/vm/cloudinit/resource.go index 02cd1b92..b0186718 100644 --- a/fwprovider/vm/cloudinit/resource.go +++ b/fwprovider/vm/cloudinit/resource.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/diag" "github.com/hashicorp/terraform-plugin-framework/types" + "github.com/hashicorp/terraform-plugin-framework/types/basetypes" "github.com/bpg/terraform-provider-proxmox/fwprovider/attribute" customtypes "github.com/bpg/terraform-provider-proxmox/fwprovider/types" @@ -21,8 +22,13 @@ import ( "github.com/bpg/terraform-provider-proxmox/proxmox/nodes/vms" ) +// Value represents the type for CPU settings. +type Value = types.Object + +type DNSValue = types.Object + // NewValue returns a new Value with the given CPU settings from the PVE API. -func NewValue(ctx context.Context, config *vms.GetResponseData, vmID int, diags *diag.Diagnostics) *Model { +func NewValue(ctx context.Context, config *vms.GetResponseData, vmID int, diags *diag.Diagnostics) Value { ci := Model{} devices := config.CustomStorageDevices.Filter(func(device *vms.CustomStorageDevice) bool { @@ -30,7 +36,7 @@ func NewValue(ctx context.Context, config *vms.GetResponseData, vmID int, diags }) if len(devices) != 1 { - return nil + types.ObjectNull(attributeTypes()) } for iface, device := range devices { @@ -51,25 +57,43 @@ func NewValue(ctx context.Context, config *vms.GetResponseData, vmID int, diags } if !reflect.DeepEqual(dns, ModelDNS{}) { - ci.DNS = &dns + dnsObj, d := types.ObjectValueFrom(ctx, attributeTypesDNS(), dns) + diags.Append(d...) + + ci.DNS = dnsObj } - return &ci + obj, d := types.ObjectValueFrom(ctx, attributeTypes(), ci) + diags.Append(d...) + + return obj } - return nil + return types.ObjectNull(attributeTypes()) } // FillCreateBody fills the CreateRequestBody with the Cloud-Init settings from the Value. -func FillCreateBody(ctx context.Context, plan *Model, body *vms.CreateRequestBody) { - if plan == nil { +func FillCreateBody(ctx context.Context, planValue Value, body *vms.CreateRequestBody, diags *diag.Diagnostics) { + var plan Model + + if planValue.IsNull() || planValue.IsUnknown() { + return + } + + d := planValue.As(ctx, &plan, basetypes.ObjectAsOptions{}) + diags.Append(d...) + + if d.HasError() { return } ci := vms.CustomCloudInitConfig{} - if plan.DNS != nil { - dns := *plan.DNS + // TODO: should we check for !null? + if !plan.DNS.IsUnknown() { + var dns ModelDNS + + plan.DNS.As(ctx, &dns, basetypes.ObjectAsOptions{}) if !dns.Domain.IsUnknown() { ci.SearchDomain = dns.Domain.ValueStringPointer() @@ -98,12 +122,23 @@ func FillCreateBody(ctx context.Context, plan *Model, body *vms.CreateRequestBod // FillUpdateBody fills the UpdateRequestBody with the Cloud-Init settings from the Value. func FillUpdateBody( ctx context.Context, - plan, state *Model, + planValue, stateValue Value, updateBody *vms.UpdateRequestBody, isClone bool, diags *diag.Diagnostics, ) { - if plan == nil || reflect.DeepEqual(plan, state) { + var plan, state Model + + if planValue.IsNull() || planValue.IsUnknown() || planValue.Equal(stateValue) { + return + } + + d := planValue.As(ctx, &plan, basetypes.ObjectAsOptions{}) + diags.Append(d...) + d = stateValue.As(ctx, &state, basetypes.ObjectAsOptions{}) + diags.Append(d...) + + if diags.HasError() { return } @@ -114,13 +149,20 @@ func FillUpdateBody( // TODO: migrate cloud init to another datastore if !reflect.DeepEqual(plan.DNS, state.DNS) { - if plan.DNS == nil && state.DNS != nil && !isClone { + if attribute.ShouldBeRemoved(plan.DNS, state.DNS, isClone) { del("searchdomain", "nameserver") - } else if plan.DNS != nil { + } else if attribute.IsDefined(plan.DNS) { ci := vms.CustomCloudInitConfig{} - planDNS := plan.DNS - stateDNS := state.DNS + var planDNS, stateDNS ModelDNS + d = plan.DNS.As(ctx, &planDNS, basetypes.ObjectAsOptions{}) + diags.Append(d...) + d = state.DNS.As(ctx, &stateDNS, basetypes.ObjectAsOptions{}) + diags.Append(d...) + + if diags.HasError() { + return + } if !planDNS.Domain.Equal(stateDNS.Domain) { if attribute.ShouldBeRemoved(planDNS.Domain, stateDNS.Domain, isClone) { @@ -134,17 +176,11 @@ func FillUpdateBody( if attribute.ShouldBeRemoved(planDNS.Servers, stateDNS.Servers, isClone) { del("nameserver") } else if attribute.IsDefined(planDNS.Servers) { - // TODO: duplicates code from FillCreateBody var servers []string planDNS.Servers.ElementsAs(ctx, &servers, false) - //// special case for the servers list, if we want to remove them during update - //if len(servers) == 0 { - // del("nameserver") - //} else { ci.Nameserver = ptr.Ptr(strings.Join(servers, " ")) - //} } } diff --git a/fwprovider/vm/cloudinit/resource_test.go b/fwprovider/vm/cloudinit/resource_test.go index d18b899b..d28fc93c 100644 --- a/fwprovider/vm/cloudinit/resource_test.go +++ b/fwprovider/vm/cloudinit/resource_test.go @@ -253,17 +253,175 @@ func TestResource_VM2_CloudInit_Update(t *testing.T) { ), }, }}, - // { - // // step 9: update the VM: remove the dns block - // Config: te.RenderConfig(` - // resource "proxmox_virtual_environment_vm2" "test_vm" { - // node_name = "{{.NodeName}}" - // name = "test-ci" - // initialization = {} - // }`), - // }, - //}}, - + {"delete dns block", []resource.TestStep{ + { + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm2" "test_vm" { + node_name = "{{.NodeName}}" + id = {{.RandomVMID}} + name = "test-ci" + initialization = { + dns = { + domain = "another.domain.com" + servers = [ + "1.1.1.1" + ] + } + } + }`), + }, + { + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm2" "test_vm" { + node_name = "{{.NodeName}}" + name = "test-ci" + initialization = {} + }`), + Check: resource.ComposeTestCheckFunc( + test.NoResourceAttributesSet("proxmox_virtual_environment_vm2.test_vm", []string{ + "initialization.dns.domain", + }), + resource.TestCheckResourceAttr("proxmox_virtual_environment_vm2.test_vm", "initialization.dns.servers.#", "0"), + ), + }, + }}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resource.ParallelTest(t, resource.TestCase{ + ProtoV6ProviderFactories: te.AccProviders, + Steps: tt.steps, + }) + }) + } +} + +func TestResource_VM2_CloudInit_Clone(t *testing.T) { + t.Parallel() + + te := test.InitEnvironment(t) + + tests := []struct { + name string + steps []resource.TestStep + }{ + {"clone dns block in full", []resource.TestStep{ + { + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm2" "test_template" { + node_name = "{{.NodeName}}" + id = {{.RandomVMID1}} + name = "test-ci-template" + initialization = { + dns = { + domain = "example.com" + servers = [ + "1.1.1.1", + "8.8.8.8" + ] + } + } + } + resource "proxmox_virtual_environment_vm2" "test_vm" { + node_name = "{{.NodeName}}" + id = {{.RandomVMID2}} + name = "test-ci" + clone = { + id = proxmox_virtual_environment_vm2.test_template.id + } + }`), + Check: test.ResourceAttributes("proxmox_virtual_environment_vm2.test_vm", map[string]string{ + "initialization.datastore_id": te.DatastoreID, + "initialization.interface": "ide2", + "initialization.dns.domain": "example.com", + "initialization.dns.servers.#": "2", + "initialization.dns.servers.0": "1.1.1.1", + "initialization.dns.servers.1": "8.8.8.8", + }), + }, + }}, + {"clone dns block overwriting domain", []resource.TestStep{ + { + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm2" "test_template" { + node_name = "{{.NodeName}}" + id = {{.RandomVMID1}} + name = "test-ci-template" + initialization = { + dns = { + domain = "example.com" + servers = [ + "1.1.1.1", + "8.8.8.8" + ] + } + } + } + resource "proxmox_virtual_environment_vm2" "test_vm" { + node_name = "{{.NodeName}}" + id = {{.RandomVMID2}} + name = "test-ci" + clone = { + id = proxmox_virtual_environment_vm2.test_template.id + } + initialization = { + dns = { + domain = "another.domain.com" + } + } + }`), + Check: test.ResourceAttributes("proxmox_virtual_environment_vm2.test_vm", map[string]string{ + "initialization.datastore_id": te.DatastoreID, + "initialization.interface": "ide2", + "initialization.dns.domain": "another.domain.com", + "initialization.dns.servers.#": "2", + "initialization.dns.servers.0": "1.1.1.1", + "initialization.dns.servers.1": "8.8.8.8", + }), + }, + }}, + {"clone dns block overwriting servers", []resource.TestStep{ + { + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm2" "test_template" { + node_name = "{{.NodeName}}" + id = {{.RandomVMID1}} + name = "test-ci-template" + initialization = { + dns = { + domain = "example.com" + servers = [ + "1.1.1.1", + "8.8.8.8" + ] + } + } + } + resource "proxmox_virtual_environment_vm2" "test_vm" { + node_name = "{{.NodeName}}" + id = {{.RandomVMID2}} + name = "test-ci" + clone = { + id = proxmox_virtual_environment_vm2.test_template.id + } + initialization = { + dns = { + servers = [ + "4.4.4.4" + ] + } + } + }`), + Check: test.ResourceAttributes("proxmox_virtual_environment_vm2.test_vm", map[string]string{ + "initialization.datastore_id": te.DatastoreID, + "initialization.interface": "ide2", + "initialization.dns.domain": "example.com", + "initialization.dns.servers.#": "1", + "initialization.dns.servers.0": "4.4.4.4", + }), + }, + }}, } for _, tt := range tests { diff --git a/fwprovider/vm/model.go b/fwprovider/vm/model.go index 5398000d..5a7712df 100644 --- a/fwprovider/vm/model.go +++ b/fwprovider/vm/model.go @@ -35,16 +35,16 @@ type Model struct { ID types.Int64 `tfsdk:"id"` Retries types.Int64 `tfsdk:"retries"` } `tfsdk:"clone"` - CloudInit *cloudinit.Model `tfsdk:"initialization"` - CPU cpu.Value `tfsdk:"cpu"` - ID types.Int64 `tfsdk:"id"` - Name types.String `tfsdk:"name"` - NodeName types.String `tfsdk:"node_name"` - StopOnDestroy types.Bool `tfsdk:"stop_on_destroy"` - Tags stringset.Value `tfsdk:"tags"` - Template types.Bool `tfsdk:"template"` - Timeouts timeouts.Value `tfsdk:"timeouts"` - VGA vga.Value `tfsdk:"vga"` + CloudInit cloudinit.Value `tfsdk:"initialization"` + CPU cpu.Value `tfsdk:"cpu"` + ID types.Int64 `tfsdk:"id"` + Name types.String `tfsdk:"name"` + NodeName types.String `tfsdk:"node_name"` + StopOnDestroy types.Bool `tfsdk:"stop_on_destroy"` + Tags stringset.Value `tfsdk:"tags"` + Template types.Bool `tfsdk:"template"` + Timeouts timeouts.Value `tfsdk:"timeouts"` + VGA vga.Value `tfsdk:"vga"` } // read retrieves the current state of the resource from the API and updates the state. diff --git a/fwprovider/vm/resource.go b/fwprovider/vm/resource.go index 33795640..05fc1f7c 100644 --- a/fwprovider/vm/resource.go +++ b/fwprovider/vm/resource.go @@ -155,7 +155,7 @@ func (r *Resource) create(ctx context.Context, plan Model, diags *diag.Diagnosti // fill out create body fields with values from other resource blocks cdrom.FillCreateBody(ctx, plan.CDROM, createBody, diags) - cloudinit.FillCreateBody(ctx, plan.CloudInit, createBody) + cloudinit.FillCreateBody(ctx, plan.CloudInit, createBody, diags) cpu.FillCreateBody(ctx, plan.CPU, createBody, diags) vga.FillCreateBody(ctx, plan.VGA, createBody, diags)