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

provider::assert::null() not behaving as expected #71

Closed
dustindortch opened this issue Oct 23, 2024 · 4 comments · Fixed by #83
Closed

provider::assert::null() not behaving as expected #71

dustindortch opened this issue Oct 23, 2024 · 4 comments · Fixed by #83
Labels
bug Something isn't working

Comments

@dustindortch
Copy link

I am doing some compound validation and I will use alltrue(), anytrue(), or sum() in my validation for AND, OR, and XOR operations, respectively.

The following fails validation:

variable "cidr_block" {
  default     = null
  description = "CIDR block for the VPC."
  type        = string
  # ...

  validation {
    condition = sum([
      provider::assert::null(var.cidr_block) ? 0 : 1,
      provider::assert::null(var.ipv4_ipam_pool_id) ? 0 : 1
    ]) == 1 # xor
    error_message = "Exactly one of cidr_block or ipv4_ipam_pool_id must be provided."
  }
}

Results:

terraform plan -out=tfplan

│ Error: Invalid value for variable
│ 
│   on main.tf line 17, in module "vpc":
│   17:   cidr_block = "10.0.42.0/24"
│     ├────────────────
│     │ var.cidr_block is "10.0.42.0/24"
│     │ var.ipv4_ipam_pool_id is a string
│ 
│ Exactly one of cidr_block or ipv4_ipam_pool_id must be provided.

The following validation works as expected:

variable "cidr_block" {
  default     = null
  description = "CIDR block for the VPC."
  type        = string
  # ...

  validation {
    condition = sum([
      var.cidr_block != null ? 1 : 0,
      var.ipv4_ipam_pool_id != null ? 1 : 0
    ]) == 1 # xor
    error_message = "Exactly one of cidr_block or ipv4_ipam_pool_id must be provided."
  }
}

Are these doing some sort of blocking failures? This seems bad if the context would be either inside a check block, which should be non-blocking, or inside of terraform test with an expected failure. It really limits the application.

@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer label Oct 23, 2024
@dustindortch
Copy link
Author

Some clarification after investigating further. It only seems like an issue when doing cross-object references in variable validation.

The following fails like before:

variable "cidr_block" {
  default     = null
  description = "CIDR block for the VPC."
  type        = string

  validation {
    condition = anytrue([
      provider::assert::null(var.cidr_block),
      provider::assert::cidr(var.cidr_block)
    ])
    error_message = "CIDR block must be a valid CIDR range."
  }

  validation {
    condition = anytrue([
      alltrue([
        provider::assert::not_null(var.cidr_block),
        provider::assert::null(var.ipv4_ipam_pool_id)
      ]),
      alltrue([
        provider::assert::null(var.cidr_block),
        provider::assert::not_null(var.ipv4_ipam_pool_id)
      ])
    ]) # XOR
    error_message = "Exactly one of cidr_block or ipv4_ipam_pool_id must be provided."
  }
}

However, this small change fixes it:

variable "cidr_block" {
  default     = null
  description = "CIDR block for the VPC."
  type        = string

  validation {
    condition = anytrue([
      provider::assert::null(var.cidr_block),
      provider::assert::cidr(var.cidr_block)
    ])
    error_message = "CIDR block must be a valid CIDR range."
  }

  validation {
    condition = anytrue([
      alltrue([
        provider::assert::not_null(var.cidr_block),
        var.ipv4_ipam_pool_id == null #.................. this is the change
      ]),
      alltrue([
        provider::assert::null(var.cidr_block),
        provider::assert::not_null(var.ipv4_ipam_pool_id)
      ])
    ]) # XOR
    error_message = "Exactly one of cidr_block or ipv4_ipam_pool_id must be provided."
  }
}

So, my thought was that the cross-object reference is receiving the "zero value" from the nulled variable. To validate this, I ran the following which failed, as I was thinking it would, verifying this:

variable "cidr_block" {
  default     = null
  description = "CIDR block for the VPC."
  type        = string

  validation {
    condition = anytrue([
      provider::assert::null(var.cidr_block),
      provider::assert::cidr(var.cidr_block)
    ])
    error_message = "CIDR block must be a valid CIDR range."
  }

  validation {
    condition = alltrue([
      anytrue([
        alltrue([
          provider::assert::not_null(var.cidr_block),
          var.ipv4_ipam_pool_id == null
        ]),
        alltrue([
          provider::assert::null(var.cidr_block),
          provider::assert::not_null(var.ipv4_ipam_pool_id)
        ])
      ]),
      !alltrue([
        provider::assert::not_null(var.cidr_block),
        provider::assert::not_null(var.ipv4_ipam_pool_id)
      ])
    ]) # XOR
    error_message = "Exactly one of cidr_block or ipv4_ipam_pool_id must be provided."
  }
}

So, it appears to be more of a problem with cross-object reference behavior than the provider.

Although, while investigating the code... I am curious why null and not_null aren't more similar, just getting right of the "!" in the like resp.Error = function.ConcatFuncErrors(resp.Result.Set(ctx, !argument.IsNull())) for null.

@bschaatsbergen bschaatsbergen added bug Something isn't working and removed needs-triage Waiting for first response or review from a maintainer labels Jan 10, 2025
@bschaatsbergen
Copy link
Member

bschaatsbergen commented Jan 10, 2025

Hi @dustindortch,

Thank you for reporting this issue. In v0.15.0, we merged a fix to evaluate the underlying value as null, which is necessary for cross-object references. Previously, the dynamic value of an argument was evaluated as either null or not. Additionally, we’ve disabled the allowance of unknown values being passed into not_null, which helps protect users from flaky assertions on argument references that may be unknown but could potentially be null or not.

It would be greatly appreciated if you could test this out and let us know if it resolves your issue. I’ve also added a test to the null function that includes compound validation, which is a simplified version of yours. Thanks again!

@dustindortch
Copy link
Author

@bschaatsbergen taking a look now, thanks!

@dustindortch
Copy link
Author

@bschaatsbergen it works! Fantastic! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants