Getting "Only image or source_instance can be set" even if source_instance defaults to null

[Iduoad]

Hi,

The following code cannot plan and keeps failing with Only image or source_instance can be set even if source_instance variable default to null.

terraform {
  required_providers {
    incus = {
      source = "lxc/incus"
      version = "0.2.0"
    }
  }
}

provider "incus" {
}

variable "source_instance" {
  description = "Source instance from which the instance is created"
  type = object({
    project  = string
    name     = string
    snapshot = optional(string)
  })
  default = null
}

resource "incus_instance" "instance1" {
  name  = "instance1"
  image = "images:ubuntu/22.04"
  
  source_instance = var.source_instance
}

when I try the following it works

resource "incus_instance" "instance1" {
  name  = "instance1"
  image = "images:ubuntu/22.04"
  
  source_instance = null
}

Went through a bit of the source code but couldn't figure out what was the issue

Thank you for the great work

[stgraber]

Definitely a bit of an odd one.

@maveonair any thoughts?

[Iduoad]

I built the provider on my machine and added a println and here is what I got

    fmt.Println(config.SourceInstance)
    fmt.Println(config.Image)

/*  | <unknown>
    | "images:ubuntu/22.04" */

[Iduoad]

I think I understand what's going on

The mutual exclusivity check is done too early here. While Image and SourceInstance can still be UNKOWN if they are fed into the resource from variables.

if !config.Image.IsNull() && !config.SourceInstance.IsNull() {

	resp.Diagnostics.AddError(
	        "Invalid Configuration",
		"Only image or source_instance can be set.",
	)
	return
}

I don't know where exactly is the best place to add the actual check but I suspect it would be Create function where the values are completely set.

I am new to Terraform provider developement so I am not sure this is exactly the case.

[maveonair]

I think I understand what's going on

The mutual exclusivity check is done too early here. While Image and SourceInstance can still be UNKOWN if they are fed into the resource from variables.

if !config.Image.IsNull() && !config.SourceInstance.IsNull() {

	resp.Diagnostics.AddError(
	        "Invalid Configuration",
		"Only image or source_instance can be set.",
	)
	return
}

I don't know where exactly is the best place to add the actual check but I suspect it would be Create function where the values are completely set.

I am new to Terraform provider developement so I am not sure this is exactly the case.

Actually, this is exactly what should happen. You can either create an instance from an image or
from an existing instance, but not both.

We can try to also check for the unknown state of the values to see if it catches null too. But we need to do it there as we also need this checks for updates.

[Iduoad]

This will be true for source_file recently added by @breml. It won't be possible to pass source_file as a null variable.

Since this condition will always (at least most of the times) evaluate to True.

c237450?diff=unified&w=0#diff-cabb6428d26399ff7392dd667882c83dc34a5aa49de3974642049448f0f5dfaeR394-R407

[breml]

If we consider the unknown state as well and let it basically overrule the null check, we would need to check again in the actual methods, that perform the operations (create, update), because only then we get the fully resolved resources and only then we can decide, if we have the case, where multiple of the mutual exclusive options (image, source_file and source_instance) are provided.

This will be true for source_file recently added by @breml. It won't be possible to pass source_file as a null variable.
Since this condition will always (at least most of the times) evaluate to True.

The code you are referring to is validating, if the given value for source_file is valid. If it is null, then there is no validation done. This could lead to a problem, if a variable is provided, which is not yet fully evaluated and then during the create call would cause an issue, since an invalid set of properties is provided.

[Iduoad]

I created a PR to make use of the ConflictsWith an AtLeastOneOf Validator from the plugin framework. I tested the changes quickly and they work fine with the previous setup and solve the issue mentioned above.

Sorry if this is incomplete, I just worked to start a POC and get your opinion on it, then start finishing up the work on the PR and start the review.

[Iduoad]

I did more testing and the new behavior is a little bit more interesting

For this example:

variable "source_instance" {
  description = "Source instance from which the instance is created"
  type = object({
    project  = string
    name     = string
    snapshot = optional(string)
  })
  default = {
    project = "value"
    name = "name"
  }
}

variable "image" {
  type = string
  default = "myimage"
  
}

resource "incus_instance" "instance1" {
  name  = "instance1"

  image = var.image

  source_instance = var.source_instance
}

Using variables with default values, everything works as expected

var.image	var.source_instance	output
set	set	Attribute "image" cannot be specified when "source_instance" is specified
set	null	Successful Plan
null	set	Successful Plan
null	null	At least one attribute out of [image,source_instance,source_file] must be specified

But when setting the attributes to literals

image	source_instance	output
var.image=null	literal	Attribute "source_instance" cannot be specified when "image" is specified
literal	var.source_instance=null	Attribute "image" cannot be specified when "source_instance" is specified

So what happens is when one of the attribute is set in the resource itself, the other ones should be omitted or set to null explicitly (!IsUnkown).

[breml]

@Iduoad In general I like the code you have in PR #181, since it replaces the existing logic with two simple Validators and with this does make it more clear and easier to reason about. There is an issue though, since your validators do not exactly the same as the existing logic:
The current logic accepts the case, where none of image, source_file and source_instance is set (atMostOneOf) while your code enforces AtLeastOneOf. I tested this with the terraform provider and if the instance has running = false, then it is fine (and accepted by incusd), if non of these attributes is provided.

Additionally, while your change does make the code better, it does not solve the issue with IsUnknow, infact I think it has the same issue as the current implementation, since this can not be solved during ValidateConfig, since at this point, the final values are not yet completely known (variables might not yet be evaluated).

[breml]

@Iduoad after thinking again, I wonder if the ConflictsWith validator in our case would be already enough, since it ensures, that non of the conflicting settings can be used in combination while at the same time does allow, if none of them is set.