clone works

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

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
+// 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.
+// 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.
+//
+// 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)
+}

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
+// 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.
+// 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.
+//
+// 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)
+}

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},
+	}
+}

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) {
+func FillCreateBody(ctx context.Context, planValue Value, body *vms.CreateRequestBody, diags *diag.Diagnostics) {
-	if plan == nil {
+	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 {
+	// TODO: should we check for !null?
-		dns := *plan.DNS
+	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
+			var planDNS, stateDNS ModelDNS
-			stateDNS := state.DNS
+			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, " "))
-					//}
 				}
 			}
 

fwprovider/vm/cloudinit/resource_test.go

@@ -253,17 +253,175 @@ func TestResource_VM2_CloudInit_Update(t *testing.T) {
 				),
 			},
 		}},
-		//	{
+		{"delete dns block", []resource.TestStep{
-		//		// step 9: update the VM: remove the dns block
+			{
-		//		Config: te.RenderConfig(`
+				Config: te.RenderConfig(`
-		//		resource "proxmox_virtual_environment_vm2" "test_vm" {
+				resource "proxmox_virtual_environment_vm2" "test_vm" {
-		//			node_name = "{{.NodeName}}"
+					node_name = "{{.NodeName}}"
-		//			name = "test-ci"
+					id = {{.RandomVMID}}
-		//			initialization = {}
+					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 {

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"`
+	CloudInit     cloudinit.Value `tfsdk:"initialization"`
-	CPU           cpu.Value        `tfsdk:"cpu"`
+	CPU           cpu.Value       `tfsdk:"cpu"`
-	ID            types.Int64      `tfsdk:"id"`
+	ID            types.Int64     `tfsdk:"id"`
-	Name          types.String     `tfsdk:"name"`
+	Name          types.String    `tfsdk:"name"`
-	NodeName      types.String     `tfsdk:"node_name"`
+	NodeName      types.String    `tfsdk:"node_name"`
-	StopOnDestroy types.Bool       `tfsdk:"stop_on_destroy"`
+	StopOnDestroy types.Bool      `tfsdk:"stop_on_destroy"`
-	Tags          stringset.Value  `tfsdk:"tags"`
+	Tags          stringset.Value `tfsdk:"tags"`
-	Template      types.Bool       `tfsdk:"template"`
+	Template      types.Bool      `tfsdk:"template"`
-	Timeouts      timeouts.Value   `tfsdk:"timeouts"`
+	Timeouts      timeouts.Value  `tfsdk:"timeouts"`
-	VGA           vga.Value        `tfsdk:"vga"`
+	VGA           vga.Value       `tfsdk:"vga"`
 }
 
 // read retrieves the current state of the resource from the API and updates the state.

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)