lxc / terraform-provider-incus

Incus provider for Terraform/OpenTofu
https://linuxcontainers.org/incus
Mozilla Public License 2.0
50 stars 11 forks source link

Don't allow `auto` as value for `ipv4.address` or `ipv6.address` #57

Open clbouvier opened 5 months ago

clbouvier commented 5 months ago
terraform {
  required_providers {
    incus = {
      source = "lxc/incus"
      version = "~> 0.1.1"
    }
  }
  required_version = ">= 1.8"
}

provider "incus" {}

resource "incus_network" "net" {
  name = "a-bridged-network"

  type = "bridge"

  config = {
    "ipv4.address" = "auto"
    "ipv4.nat" = "true"

    "ipv6.address" = "auto"
    "ipv6.nat" = "true"
  }
}

gives the next errors.

incus_network.net: Creating...
╷
│ Error: Provider produced inconsistent result after apply
│ 
│ When applying changes to incus_network.net, provider "provider[\"registry.terraform.io/lxc/incus\"]" produced an unexpected new value: .config["ipv4.address"]: was
│ cty.StringVal("auto"), but now cty.StringVal("10.58.213.1/24").
│ 
│ This is a bug in the provider, which should be reported in the provider's own issue tracker.
╵
╷
│ Error: Provider produced inconsistent result after apply
│ 
│ When applying changes to incus_network.net, provider "provider[\"registry.terraform.io/lxc/incus\"]" produced an unexpected new value: .config["ipv6.address"]: was
│ cty.StringVal("auto"), but now cty.StringVal("fd42:1c94:e5e9:4d18::1/64").
│ 
│ This is a bug in the provider, which should be reported in the provider's own issue tracker.

though it is expected on incus side because the keyes ipv{4,6}.address are generated if we don't override them (auto as initial value)

maveonair commented 4 months ago

I'm not sure if we should change the logic to allow a user to set the value auto. Because you can already achieve that by doing this instead:

resource "incus_network" "net" {
  name = "a-bridge"

  type = "bridge"

  config = {
    "ipv4.nat" = "true"
    "ipv6.nat" = "true"
  }
}

So if you don't set a value, we normally take the reasonable default values from Incus, which in this case would be auto. Also, auto is actually no longer correct after tofu apply because it now has a real value that corresponds to the automatically selected network address.

Could you perhaps explain why it is necessary to declare auto at all?

clbouvier commented 4 months ago

I am simply using auto as default value in a cidr_range like input variable in a dedicated module. If I don't set the variable explicitly, I tell incus to choose an automatic IP range.

Anyway, you are right I can simply skip the option inside config. It is not a big deal.

If we don't want to change the logic, we may add a constraint like auto is not a valid option regards to incus proposes and set an explicit error mentioning that it is more how we are using the provider than the provider itself the problem...

maveonair commented 4 months ago

I am simply using auto as default value in a cidr_range like input variable in a dedicated module. If I don't set the variable explicitly, I tell incus to choose an automatic IP range.

Anyway, you are right I can simply skip the option inside config. It is not a big deal.

If we don't want to change the logic, we may add a constraint like auto is not a valid option regards to that incus proposes and set an explicit error mentioning that it is more we are using with the provider than the provider itself the problem...

What do you think is the better experience from the user's point of view:

clbouvier commented 4 months ago

What do you think is the better experience from the user's point of view:

* set the value to `auto` and ignore the calculated values?

* receive an error message when `auto` is used as the value?

I would prefer the second option.

stgraber commented 3 months ago

I think having auto fail is the right thing to do because if Terraform begins enforcing that by actually updating the Incus side value to auto, it would cause every Terraform run to change the subnet on the Incus side :)

(That's because changing ipv4.address or ipv6.address from a valid subnet/address to auto will cause a new subnet to be selected)

It makes sense to not require a subnet to be picked, but that's different from having the user specifically set the values to auto.

JulesdeCube commented 2 months ago

Hello I would like to work on this issue if it's possible ?

stgraber commented 2 months ago

Sure. I assigned it to you!