for expression produces unnecessary cycle dependency

[dmrzzz]

Terraform Version

$ terraform-0.14.10 version
Terraform v0.14.10
+ provider registry.terraform.io/hashicorp/null v3.1.0

Terraform Configuration Files

I have three types of resources: alpha which has no dependencies, bravo which depends on alpha, and charlie which depends on both alpha and bravo. But for conceptual modeling reasons (not shown here), it makes sense to keep bravo as a singleton while grouping alpha and charlie together in a module which gets instantiated multiple times.

m/module.tf:

resource "null_resource" "alpha" {
}

output "alpha_id" {
  value = null_resource.alpha.id
}

variable "bravo_id" {
  type = string
}

# this charlie depends on this alpha and the singleton bravo
resource "null_resource" "charlie" {
  triggers = {
    alpha_id  = null_resource.alpha.id
    bravo_id = var.bravo_id
  }
}

output "charlie_id" {
  value = null_resource.charlie.id
}

main.tf:

module "m" {
  for_each = toset(["red", "blue"])
  source   = "./m"

  bravo_id = null_resource.bravo.id
}

# the singleton bravo depends on several alphas
locals {
  # cycle error
  alpha_ids = [for k in ["red", "blue"] : module.m[k].alpha_id]

  # works fine
  #alpha_ids = [module.m["red"].alpha_id, module.m["blue"].alpha_id]
}
resource "null_resource" "bravo" {
  triggers = {
    alpha_ids = join(",", local.alpha_ids)
  }
}

Expected Behavior

Terraform creates 2 alphas, then 1 bravo, then 2 charlies.

Actual Behavior

With the configs as above, Terraform emits a cycle error.

However, if I use my alternative definition for local.alpha_ids, it works as expected.

Both definitions use exactly the same module outputs, yet the for expression incurs extra dependencies which result in a cycle.

Steps to Reproduce

1. terraform-0.14.10 init
2. apply:

$ terraform-0.14.10 apply

Error: Cycle: module.m.output.charlie_id (expand), null_resource.bravo, module.m.var.bravo_id (expand), module.m.null_resource.charlie, module.m (close), local.alpha_ids (expand)

3. Edit main.tf to comment out the first alpha_ids line and uncomment the second one.
4. apply:

$ terraform-0.14.10 apply

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # null_resource.bravo will be created
  + resource "null_resource" "bravo" {
      + id       = (known after apply)
      + triggers = (known after apply)
    }

  # module.m["blue"].null_resource.alpha will be created
  + resource "null_resource" "alpha" {
      + id = (known after apply)
    }

  # module.m["blue"].null_resource.charlie will be created
  + resource "null_resource" "charlie" {
      + id       = (known after apply)
      + triggers = (known after apply)
    }

  # module.m["red"].null_resource.alpha will be created
  + resource "null_resource" "alpha" {
      + id = (known after apply)
    }

  # module.m["red"].null_resource.charlie will be created
  + resource "null_resource" "charlie" {
      + id       = (known after apply)
      + triggers = (known after apply)
    }

Plan: 5 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

module.m["red"].null_resource.alpha: Creating...
module.m["blue"].null_resource.alpha: Creating...
module.m["red"].null_resource.alpha: Creation complete after 0s [id=5028284708804093992]
module.m["blue"].null_resource.alpha: Creation complete after 0s [id=2433911416685148906]
null_resource.bravo: Creating...
null_resource.bravo: Creation complete after 0s [id=1873439510834029155]
module.m["red"].null_resource.charlie: Creating...
module.m["blue"].null_resource.charlie: Creating...
module.m["blue"].null_resource.charlie: Creation complete after 0s [id=8038431529247947963]
module.m["red"].null_resource.charlie: Creation complete after 0s [id=7761344243634067070]

Apply complete! Resources: 5 added, 0 changed, 0 destroyed.

[dmrzzz]

I don't spend much time interpreting dependency graphs, but I suspect this might be of interest in highlighting the difference:

$ terraform-0.14.10 graph | grep local.alpha_ids
		"[root] local.alpha_ids (expand)" -> "[root] module.m (close)"
		"[root] local.alpha_ids (expand)" -> "[root] module.m.output.alpha_id (expand)"
		"[root] local.alpha_ids (expand)" -> "[root] module.m.output.charlie_id (expand)"
		"[root] meta.count-boundary (EachMode fixup)" -> "[root] local.alpha_ids (expand)"
		"[root] null_resource.bravo (expand)" -> "[root] local.alpha_ids (expand)"

vs

$ terraform-0.14.10 graph | grep local.alpha_ids
		"[root] local.alpha_ids (expand)" -> "[root] module.m.output.alpha_id (expand)"
		"[root] null_resource.bravo (expand)" -> "[root] local.alpha_ids (expand)"

[jbardin]

Hi @dmrzzz,

Thanks for filing the issue! I think the surprising part of the cycle here is actually that it is avoidable with your second case. The cycle can be sen from the configuration more simply as:

module.m -> null_resource.bravo -> local.alpha_ids -> module.m

Due to these references a cycle is expected here. The fact that removing the for expressions removes the cycle is a bit surprising, and I'm curious as to what is actually happening there; but it is not totally unexpected as we did need to allow for "circular" references from a module's outputs to its inputs for backwards compatibility. That behavior though will generally only work with single module instances, as the expansion of the module with count or for_each forces the module to be treated more like a configuration object similar to a resource.

[dmrzzz]

@jbardin what you're saying makes sense if you assume/require that a module is fundamentally atomic, i.e. first all its inputs must be available, then all resources inside the module are handled, and finally all outputs of the module become available (simultaneously).

But (AFAIK) there's no particular reason to require that. The documentation defines a module as simply "a container for multiple resources that are used together", and one of Terraform's greatest strengths is handling resource dependency graphs as flexibly as possible behind the scenes in order to make things easy on the humans. So IMHO the ideal vision is for any particular bunch of resources to have the same dependency constraints whether they are grouped into several modules or all defined in the root module.*

(* unless of course I specify depends_on for the module itself, but that's not the case here)

From a resource-oriented point of view my example contains no cycle, and Terraform is delightfully smart about this as long as I only use references with hardcoded keys like module.m["red"].alpha_id.

My thought is that if it can handle that case in the ideal way, it hopefully shouldn't be too much of a stretch to also handle module.m[expression].alpha_id where the expression is 100% known to have the value "red" (it's not computed or anything).

By the way, it turns out the dividing line today is much simpler than I originally submitted; it seems that using literally any expression at all introduces the extra dependencies. Here is my new simplest formulation:

locals {
  # works fine
  #alpha_ids = [module.m["red"].alpha_id, module.m["blue"].alpha_id]

  # cycle error
  alpha_ids = [module.m[("red")].alpha_id, module.m[("blue")].alpha_id]
}

[jbardin]

Thanks @dmrzzz. I understand what you are saying, but from the language perspective we have to initially assume that any module which contains count or for_each could have dependencies that change how the module is expanded as a whole, and also dependents that are based on how it was expanded. Basically we need to start with the premise that any module that is using count or for_each is a single ("fundamentally atomic") unit. In this particular case the static portions of the expressions make it clear that the dependency can be avoided, but that same claim cannot be made in the general case for all expressions.

Terraform doesn't actually enforce this limitation specifically, the cycle falls out of the language constructs and order of operations that Terraform requires to accomplish the task at hand. Internally there was second module expansion implementation tested which did enforce this isolation of modules, but backwards compatibility was preferred in lieu of the conceptually simpler implementation. One of the unfortunate results of the chosen implementation however, is that we don't have the ability to make clear cut determinations about what is a cycle before evaluating the configuration. We would like to make this easier for users to reason about, and have an issue open to track the idea at #26150.

What appears to be happening in your example is that the having an expression within the index means that the references can only be determined up to the point of the index; which basically means that we can tell exactly what graph node module.m["red"].alpha_id refers to, but module.m[k].alpha_id does not point to an exact instance therefor we have to refer to the entirety of module.m. The module.m[("red")].alpha_id case is interesting, because the ("red") expression is not evaluated and is treated like any other expression, but this is safe in general, since in most cases an expression is not simply a wrapper around a static literal.

There may be room for improvement of the evaluation of configuration references, so that we can find precise references in more cases, as well as making cycle debugging easier for users in general as noted above. I'm leaving this open as we can look into the improvement and refinement of the behavior, but the fact that there is a cycle from the configuration presented will still be expected even though it may not be enforced.

[dmrzzz]

@jbardin I understand now, thanks very much for the explanations (both here and in the linked issue).

I would love to see future versions able to find precise references in more cases as you mentioned; feel free to retag this as a feature request instead of a bug.

Meanwhile, I think there's a quick win to be had by mentioning this in the docs (somewhere under modules, but also in https://www.terraform.io/docs/language/meta-arguments/for_each.html). FWIW the specific insight I personally needed most was "Caution: modules with for_each behave in a fundamentally different way than modules without for_each when it comes to dependencies."

[dmrzzz]

I happened to see https://github.com/hashicorp/terraform/pull/33234 and was curious whether it might be helpful in this context (now or someday).

Could Refinements perhaps be used to let TF treat [for k in ["red", "blue"] : module.m[k].alpha_id] the same as [module.m["red"].alpha_id, module.m["blue"].alpha_id] when drawing the dependency graph?

(note: please don't feel any pressure to spend time and effort on a detailed explanation if the answer is no, I'll happily take your word for it)

In case it makes any difference, my current real-world use case for this issue is still based on completely known information but has another layer of indirection; it takes the form [for x,k in var.foo : module.m[k].alpha_id], where var.foo is a map(string) hardcoded in terraform.tfvars. My current workaround is

[for x,k in var.foo : 
  k == "red" ? module.m["red"].alpha_id :
  k == "blue" ? module.m["blue"].alpha_id :
  # ...
  k == "green" ? module.m["green"].alpha_id
]

which works in this particular case because it happens that I can very confidently guess at a superset of the values that might ever appear in var.foo

[jbardin]

Hi @dmrzzz,

Refinements serve a different purpose than references. Dependencies are handled via static references in the configuration, not the value derived from the expressions containing the references. Taking your example, [for k in ["red", "blue"] : module.m[k].alpha_id] and [module.m["red"].alpha_id, module.m["blue"].alpha_id] are already handled in the exact same way. Both expressions refer to module.m as a whole, which must be evaluated before we can determine if there are even "red" and "blue" keys to index.

[dmrzzz]

Sorry @jbardin but they are not already handled in the exact same way; as formulated in my original issue submission (and just retested in TF 1.5.3), the first one causes TF to fail with a dependency cycle:

$ terraform-1.5.3 apply
╷
│ Error: Cycle: local.alpha_ids (expand), null_resource.bravo, module.m.var.bravo_id (expand), module.m.null_resource.charlie, module.m.output.charlie_id (expand), module.m (close)

while the second one succeeds:

$ terraform-1.5.3 apply
...
Apply complete! Resources: 5 added, 0 changed, 0 destroyed.

Thanks for clarifying that Refinements are not the answer, though. I'll go back to crossing my fingers for some other future enhancement.

[jbardin]

Sorry, I was trying clarify without repeating the answer from above, yes the outcome is different, but the methodology for gathering references from an expression is the same in either case, we just happen to treat the completely static case slightly differently for backward compatibility with non-expanding modules. The fact that you can skip the cycle by making static references is more an accident in the implementation, and does not apply to the general case with non-static references. Even if partial evaluation were possible to get more precise references, I doubt that it would be allowed to change the outcome because the general case would not yield any better result and you'd still be able to create a different form of the same issue, just in a more convoluted way.