MAC address of Values Not Yet Known not working

[ravager-dk]

Mac addresses calculated at runtime by another resource fails plan face.

The issue is caused by StateFunc throwing parsing error. This should not be used during planning.

Created pull request #233 to fix it

[ravager-dk]

Stack trace from the terraform-provider-xenorchestra_v0.24.0 plugin:

panic: Mac address 74D93920-ED26-11E3-AC10-0800200C9A66 was not parsable. This should never happened because Terraform's validation should happen before this is stored into state

goroutine 119 [running]:
github.com/ddelnano/terraform-provider-xenorchestra/xoa.resourceVmSchema.func1(0xd81f00, 0xc000281ba0, 0x15, 0x0)
github.com/ddelnano/terraform-provider-xenorchestra/xoa/resource_xenorchestra_vm.go:237 +0x145
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.schemaMap.diffString(0xc0003a24e0, 0xc0003b4840, 0x15, 0xc00027f900, 0xc0004e8f00, 0x1058a68, 0xc0006e6d80, 0x476e00, 0x0, 0x100000000000000)
github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/schema.go:1375 +0x6e3
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.schemaMap.diff(0xc0003a24e0, 0xc0003b4840, 0x15, 0xc00027f900, 0xc0004e9508, 0x1058a68, 0xc0006e6d80, 0x0, 0x0, 0x0)
github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/schema.go:957 +0x4e8
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.schemaMap.diffList(0xc0003a24e0, 0xeda417, 0x7, 0xc000404280, 0xc0004e9508, 0x1058a68, 0xc0006e6d80, 0x203000, 0x0, 0x0)
github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/schema.go:1091 +0x631
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.schemaMap.diff(0xc0003a24e0, 0xeda417, 0x7, 0xc000404280, 0xc0005176e0, 0x1058a68, 0xc0006e6d80, 0x40d900, 0x0, 0x0)
github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/schema.go:959 +0x473
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.schemaMap.Diff(0xc0003a24e0, 0x10544c8, 0xc00054d9c0, 0xc0005752d0, 0xc0001fe120, 0x0, 0xed3580, 0xc000349ea0, 0x1582700, 0xd81f00, ...)
github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/schema.go:522 +0x219
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).SimpleDiff(0xc0001629c0, 0x10544c8, 0xc00054d9c0, 0xc0005752d0, 0xc0001fe120, 0xed3580, 0xc000349ea0, 0x0, 0x0, 0x0)
github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/resource.go:446 +0x9f
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*GRPCProviderServer).PlanResourceChange(0xc0003a8048, 0x10544c8, 0xc00054d9c0, 0xc0003f5860, 0xc00054d9c0, 0xe93ce0, 0xc00035aa00)
github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/grpc_provider.go:693 +0x7c5
github.com/hashicorp/terraform-plugin-go/tfprotov5/server.(*server).PlanResourceChange(0xc0003a1780, 0x1054570, 0xc00054d9c0, 0xc000574e00, 0xc0003a1780, 0xc00035aab0, 0xc000103ba0)
github.com/hashicorp/[email protected]/tfprotov5/server/server.go:315 +0xb5
github.com/hashicorp/terraform-plugin-go/tfprotov5/internal/tfplugin5._Provider_PlanResourceChange_Handler(0xe93ce0, 0xc0003a1780, 0x1054570, 0xc00035aab0, 0xc0000b54a0, 0x0, 0x1054570, 0xc00035aab0, 0xc00020f100, 0x643)
github.com/hashicorp/[email protected]/tfprotov5/internal/tfplugin5/tfplugin5_grpc.pb.go:362 +0x214
google.golang.org/grpc.(*Server).processUnaryRPC(0xc000338000, 0x105cb38, 0xc0003b7c80, 0xc0000fee00, 0xc0000b6720, 0x1542708, 0x0, 0x0, 0x0)
google.golang.org/[email protected]/server.go:1194 +0x52b
google.golang.org/grpc.(*Server).handleStream(0xc000338000, 0x105cb38, 0xc0003b7c80, 0xc0000fee00, 0x0)
google.golang.org/[email protected]/server.go:1517 +0xd0c
google.golang.org/grpc.(*Server).serveStreams.func1.2(0xc0003ae1b0, 0xc000338000, 0x105cb38, 0xc0003b7c80, 0xc0000fee00)
google.golang.org/[email protected]/server.go:859 +0xab
created by google.golang.org/grpc.(*Server).serveStreams.func1
google.golang.org/[email protected]/server.go:857 +0x1fd

Error: The terraform-provider-xenorchestra_v0.24.0 plugin crashed!

This is always indicative of a bug within the plugin. It would be immensely
helpful if you could report the crash with the plugin's maintainers so that it
can be fixed. The output above should help diagnose the issue.

[ravager-dk]

Reproducible with the following code:
`
terraform {
required_providers {
xenorchestra = {
source = "terra-farm/xenorchestra"
version = "~>0.24.0"
}
macaddress = {
source = "ivoronin/macaddress"
version = "0.3.2"
}
}
}

provider "xenorchestra" {
insecure = true # Or set XOA_INSECURE environment variable to any value
}

provider "macaddress" {

}

data "xenorchestra_pool" "pool" {
name_label = "LabPool"
}

data "xenorchestra_host" "host" {
name_label = "xcp-ng-lab"
}

data "xenorchestra_template" "template" {
name_label = "Template"
}

data "xenorchestra_network" "NetWork" {
name_label = "NetworkName"
pool_id = data.xenorchestra_pool.pool.id
}

data "xenorchestra_sr" "local_storage" {
name_label = "Local storage"
pool_id = data.xenorchestra_pool.pool.id
}

resource "macaddress" "mac" {
count = 2
// ea:48:63
}

resource "xenorchestra_vm" "VMss" {
affinity_host = data.xenorchestra_host.host.id
count = 2
cpus = 2
disk {
attached = true
name_label = "Test-${count.index + 1}-Disk0"
size = 21474836480
sr_id = data.xenorchestra_sr.local_storage.id
}
template = data.xenorchestra_template.template.id
exp_nested_hvm = false
memory_max = 2147483648
name_description = "Test server"
name_label = "Test-${count.index + 1}"
network {
attached = true
# device = 0
network_id = data.xenorchestra_network.NetWork.id
mac_address = macaddress.mac[count.index].address
}
vga = "cirrus"
videoram = 4
}
`

[ddelnano]

At first I mistook the 74D93920-ED26-11E3-AC10-0800200C9A66 value as a malformed mac address from the ivoronin/macaddress provider. It turns out this is a sentinel value used by terraform to indicate variables that are unknown at a particular time (source). In addition to that, computed properties should not have StateFunc set (see hashicorp/terraform#30447 for more details).

I mention this because I didn't fully understand how this issue manifested and was initially hesitant to change StateFunc's behavior as proposed in #236 without understanding that context.

Given the Terraform team states that StateFunc shouldn't be used with computed properties, I'm hesitant to update its logic to handle terraform's unknown sentinel value. It seems like the more appropriate fix would be to remove the StateFunc property entirely or change mac_address to no longer be computed (if that makes sense and is possible).

Since the error is caused by terraform's unknown sentinel value, if you apply the macaddress configuration first (via terraform apply -target=macaddress.mac), it will prevent this sentinel value from existing and triggering the panic. I realize that isn't ideal, but given StateFunc is incorrect for this computed property I'd prefer to evaluate the options I mentioned above before we commit to incorrectly use StateFunc.

Will that work as a temporary workaround until this is investigated?

[ravager-dk]

@ddelnano
That works as a temporary solution, sure. As you know my initial proposal was to remove the statefunc entirely, but this breaks the support for dashed Mac addresses.
Maybe implement the PR and remove computed?

[ddelnano]

Yea, apologies for the back and forth on this.

I think maintaining backwards compatibility is important, but before the issue was fully understood I didn't see strong upside for breaking that functionality. However knowing that the mac address format functionality is built on terraform features that are deemed incompatible, I believe considering a breaking change is more reasonable now.

[ravager-dk]

I think the issue hashicorp/terraform#30447 describes is more a general issue moving forward. So statefunc could be used for now, but once the provider needs to be upgraded to the new plugin framework, there will be issues.
I suggest maybe an interim solution instead of a breaking change and then transition the provider to the new framework, at which point there are likely going to be other breaking changes anyway.

[ddelnano]

Sorry for the slow response on this, but this will be fixed in v0.26.1 of the provider.