Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
clbouvier opened this issue May 5, 2024 · 7 comments
Open

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

clbouvier opened this issue May 5, 2024 · 7 comments
Assignees
Labels
Bug Confirmed to be a bug Easy Good for new contributors

Comments

@clbouvier
Copy link

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
Copy link
Member

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?

@maveonair maveonair added the Incomplete Waiting on more information from reporter label May 27, 2024
@clbouvier
Copy link
Author

clbouvier commented May 27, 2024

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
Copy link
Member

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:

  • set the value to auto and ignore the calculated values?
  • receive an error message when auto is used as the value?

@clbouvier
Copy link
Author

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.

@maveonair maveonair added Maybe Undecided whether in scope for the project and removed Incomplete Waiting on more information from reporter labels May 27, 2024
@stgraber
Copy link
Member

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.

@stgraber stgraber added Bug Confirmed to be a bug Easy Good for new contributors and removed Maybe Undecided whether in scope for the project labels Jun 13, 2024
@stgraber stgraber changed the title incus_network ipv{4,6}.address = "auto" produces inconsistent result Don't allow auto as value for ipv4.address or ipv6.address Jun 13, 2024
@JulesdeCube
Copy link

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

@stgraber
Copy link
Member

Sure. I assigned it to you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed to be a bug Easy Good for new contributors
Development

No branches or pull requests

4 participants