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

State file "check_results" section not cleaned up after destroy, if variable validation is used #35383

Closed
bczoma opened this issue Jun 25, 2024 · 7 comments
Labels
bug new new issue not yet triaged working as designed confirmed as reported and closed because the behavior is intended

Comments

@bczoma
Copy link

bczoma commented Jun 25, 2024

Terraform Version

$ terraform version
Terraform v1.8.5
on linux_amd64

Terraform Configuration Files

variable "pw_length" {
  type = number
  default = 16
  validation {
    condition = var.pw_length > 0
    error_message = "Password length must be greater than 0"
  }
}

terraform {
  required_providers {
    random = {
      source = "hashicorp/random"
      version = "3.6.2"
    }
  }
}

resource "random_password" "pw" {
  length = var.pw_length
  special = true  
}

Debug Output

https://gist.github.com/bczoma/89172cd0afb5f0381e1deeeab889ad82#file-terraform-log

Expected Behavior

State file contains empty "check_results" block.

Actual Behavior

State file "check_results" block is not cleaned up.

{
  "version": 4,
  "terraform_version": "1.8.5",
  "serial": 4,
  "lineage": "fdb7b943-0b55-26b4-96ad-5b6631e592e9",
  "outputs": {},
  "resources": [],
  "check_results": [
    {
      "object_kind": "var",
      "config_addr": "module.random_password.var.pw_length",
      "status": "unknown",
      "objects": null
    }
  ]
}

Steps to Reproduce

Use the Terraform config file:

  1. terraform init
  2. terraform apply
  3. terraform destroy

Additional Context

Wanted to raise to get confirmation this is a benign behavior. I couldn't find any related information from my search.

References

No response

@bczoma bczoma added bug new new issue not yet triaged labels Jun 25, 2024
@bczoma bczoma changed the title State file "check_results" section not cleaned up after destroy State file "check_results" section not cleaned up after destroy, if variable validation is used Jun 25, 2024
@bczoma
Copy link
Author

bczoma commented Jun 25, 2024

I have also tested this without the validation in the variable and the behavior is as expected: "check_results" is empty.

@apparentlymart
Copy link
Member

Hi @bczoma! Thanks for raising this.

This is an interesting question! Technically variables are never "destroyed" in the sense that resource instances can be, their conditions are checked even during a destroy plan, and thus currently they do cause check results to be generated when planning and applying in destroy mode.

However, I do appreciate the argument that it can be unsatisfying when a full destroy doesn't result in a truly "empty" state, and if any of the input variable checks had failed you likely wouldn't be able to reach this end state anyway. It's also a little weird that the status is given as "unknown" rather than as "pass"; that does suggest that something unusual is happening here, and perhaps we could decide that the appropriate fix here is to make sure that status gets populated correctly in this case rather than omitting the check entirely.

It sounds like you were mainly interested in clarifying whether this result was a cause for concern. Is that right? If so, then I can confirm that Terraform isn't intended to consider checks when it decides whether a particular state snapshot is "empty". The definition of "empty" is focused on whether there are any resource instances and therefore the potential of any remote objects that would be "forgotten" by Terraform if the state snapshot were just discarded.

@bczoma
Copy link
Author

bczoma commented Jun 25, 2024

@apparentlymart , appreciate the quick response and the explanation. Yes indeed I wanted to find out if there was anything that could/should be done at the user level. I've got my answer thank you and I'm ready to close this but will leave it to you in case this should be left open for any eventual next step.

@apparentlymart
Copy link
Member

apparentlymart commented Jun 25, 2024

Thanks for confirming, @bczoma.

Since you've drawn attention to the fact that the check status is getting set to "unknown" in this case, and I don't believe that's intentional, it does seem like we should probably at least understand why it behaves that way, and hopefully fix it if it's feasible to do so.

I wonder if perhaps the graph pruning we do during the apply step is eliminating the variable due to it not appearing to be needed for the actions in the plan, and therefore Terraform never visits the input variable and so doesn't get a chance to decide its final check result. But I've not actually done anything to check that theory yet.

If this quirk only affects the destroy case then we might decide that it's not significant enough to be worth extra complexity to resolve, but my main concern for now is whether this is indicating that something is missing more broadly, in which case fixing it might also benefit other situations beyond a full destroy.

@liamcervante liamcervante added the confirmed a Terraform Core team member has reproduced this issue label Jun 26, 2024
@liamcervante
Copy link
Member

I think what's happening here is that the checks are not executed at all during a destroy apply operation. So the set of expected check results are loaded from the plan as unknown Terraform is expecting them to execute during the operation, then they don't execute so everything get's tracked as unknown. This issue affects all kinds of checks, as far as I can tell, not just variable validations.

This leads to another problem / issue: if resource targeting is used, any checks that are not needed by the -target operation are also skipped, and hence written into the state as unknown since the initialisation of the state check results always reads the entire configuration regardless of targeting.

I'm not sure what the expected behaviour should be in the event of a partial operation (ie. is it correct that checks should be set to unknown in the state if they didn't execute in the most recent operation, or should they keep the status from the last time they were executed). I guess an argument could be made for both.

Either way, the root cause is the same in both issues. We register all the checks in the configuration up front to be unknown before the operation begins, and then if those checks don't operate within the operation their statuses stay as unknown.

@apparentlymart
Copy link
Member

FWIW when I was originally designing the checks concept I intentionally made it report anything that was excluded using -target appear as "unknown", because my mental model is that the configuration is the authority for which checks are expected, and if we skipped running a specific check for any reason (including -target) then it's better to be explicit about that so that someone referring to the checks can see clearly that they are incomplete.

At the time I was doing that we only had resource and output preconditions and postconditions where "destroy" effectively removes the object that the check applies to, and so reporting it as "unknown" is arguably reasonable as a way to say that the check is declared in configuration but is not applicable to the current state.

Input variables are an interesting oddity because they are not something that persists between rounds, and so it's not really meaningful to discuss "destroying" them. However, if them ending up in this "unknown" status is just a logical consequence of them following the same rules as destroyed (or un-targeted) resource instances then I'd say that's probably fine, in my opinion. I was raising it only out of concern that this might be reflecting something more sinister going on, but it sounds like it's not actually a big cause for concern.

@liamcervante
Copy link
Member

Given Martin's comment I will close this as working-as-designed. Given the behaviour of the other checks is behaving as originally intended, I also think the input variables being consistent with this is acceptable.

@liamcervante liamcervante closed this as not planned Won't fix, can't repro, duplicate, stale Jun 27, 2024
@liamcervante liamcervante added working as designed confirmed as reported and closed because the behavior is intended and removed confirmed a Terraform Core team member has reproduced this issue labels Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug new new issue not yet triaged working as designed confirmed as reported and closed because the behavior is intended
Projects
None yet
Development

No branches or pull requests

3 participants