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

Coalescelist broken in 0.12 #25390

Open
schollii opened this issue Jun 25, 2020 · 4 comments
Open

Coalescelist broken in 0.12 #25390

schollii opened this issue Jun 25, 2020 · 4 comments
Labels
config enhancement v0.12 Issues (primarily bugs) reported against v0.12 releases

Comments

@schollii
Copy link

schollii commented Jun 25, 2020

Terraform Version

0.12.24

Terraform Configuration Files

... =coalescelist(A, B) 

With A and B both empty

Debug Output

Crash Output

Expected Behavior

Returns empty list

Actual Behavior

Tf aborts
Code that worked in 0.11 no longer works in 0.12.

Steps to Reproduce

Additional Context

References

@schollii
Copy link
Author

schollii commented Jun 25, 2020

The real problem here is deeper. I noticed the above issue during a terraform destroy, where an outputs.tf file had the following code:

output "dynamodb_table_name" {
  value = element(
    coalescelist(
      aws_dynamodb_table.with_server_side_encryption.*.name,
      aws_dynamodb_table.without_server_side_encryption.*.name
    ),
    0
  )
  description = "DynamoDB table name"
}

This works in terraform 0.11 but not in 0.12, where as a result of the destroy, both lists are empty, so terraform 0.12 aborts because there coalescelist aborts when all args empty lists. And I can see how semantically, this is the right thing to do for coalescelist: coalescelist is designed to return the first of its arguments that is not an empty list, so if they are all empty, it is probably reasonable that it should raise an exception.

So I'm tempted to say that the real issue is 2 things:

  • terraform destroy tries to render outputs; see below
  • coalescelist needs to be re-designed or deprecated and replaced by something that is more forgiving

Outputs on destroy:

Terraform destroy tries to render outputs, for a destroy this does not really make sense. Note this is different from removal of code from .tf files because although the net effect might be the same, with code removal you can reasonably expect the corresponding outputs code will be removed too.

Perhaps the use case of only some resources destroyed, via targetting. In that case, the outputs would be in .tf still. I think then the proper solution would be to have terraform ignore errors from outputs that are related to destroyed resources, and it should print a warning like "Warning: Output ABC could not be resolved as it depends on resources that have been destroyed or don't yet exist".

I suppose the same situation could exist when you use terraform apply with target, when there has not been a full apply done at least once; in that case, some of the outputs may not resolve, yet there is nothing really wrong with doing that.

So I think it should be possible for terraform to catch these 2 situations of outputs that cannot be resolved, and just warn, and continue instead of aborting.

Problematic coalescelist:

Real problem with coalescelist is that its behavior was changed drastically from 0.11 to 0.12. Instead, a new function say coalescelist2() should have been created. This would have allowed existing code to work, with the 0.12 documentation pointing out clearly the weaknesss of the coalescelist() function and the recommendation to move code to use coalescelist2() or be rewritten using different functions.

On a purely semantic level, it could be useful to have a None type that could be returned in some cases. Eg coalescelist2() could return None instead of raising an exception, which makes sense semantically: return first non-empty list, or None if all lists empty.

However None would probably need to be very forgiving of operations on it, eg so you could write coalescelist2()[0] and this would also return None. So basically most operations on None would work and just return None. Another way of writing this could be coalescelist2().first() and this would also work if the first() is a function on list that returns first element.

@apparentlymart
Copy link
Member

Hi @schollii! Thanks for reporting this.

As I think you suspected, the change to the coalescelist behavior in 0.12 was intentional so that this function can give better feedback when it's given nonsensical arguments.

However, the interaction with the destroy phase (where a previously-sensible set of values suddenly become invalid) was of course not intentional, and I expect our efforts to address this will be focused on either not evaluating that output at all or evaluating it in a way that allows it to succeed during the destroy phase.

The behavior you are seeing here reminds me of another issue #21096, but I can't be sure if these are related because your issue didn't include a trace log to indicate what phase of the destroy operation detected the error. If these issues both share a root cause then running terraform destroy -refresh=false should serve as a workaround until we address the underlying problem in a future release.

If these two do share a root cause then that's good news, because we're currently looking into potential solutions to #21096 as a possible part of the next major release (still to be determined, based on the outcome of that research effort), and so that could potentially address both issues at once. Even if they don't share a root cause, they seem to have related symptoms -- Terraform evaluating something at an unsuitable time -- and so we may be able to address them both with a similar solution anyway.

@thpang
Copy link

thpang commented Aug 12, 2020

So @apparentlymart given your comment above and several issues my team has seen with tf destroy and output issue with version 0.12, what issue is tracking this work? Lots of issues on our side when we run destroy and the output command complaining about items not being there, which was never an issue before. So something with the functionality has changed and not in a good way.

@iancward
Copy link

iancward commented Sep 3, 2021

It's worth comparing the behavior of coalesce() with coalescelist() here, as coalesce() has no issues returning an empty string if all strings provided are empty.

The pattern was usually:

locals {
  myvar = coalesce("", othervar_one, othervar_two)
}

And if othervar_one and othervar_two were both null or empty string, coalesce would happily return the empty string first argument. However, coalescelist() throws an error and the plan or apply aborts if all arguments are empty lists.

It would be great if coalescelist() was returned to the previous functionality and works similarly to coalesce() again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config enhancement v0.12 Issues (primarily bugs) reported against v0.12 releases
Projects
None yet
Development

No branches or pull requests

5 participants