Fix panics due to missing graph edges on empty modules or invalid refs

[liamcervante]

This PR fixes two crashes within Terraform v1.8.0. In both cases a module expansion was being missed, and then crashing when something tried to reference the missed module.

The first occurs when referencing an output that doesn't exist within the try() function. In this case the executing node was not getting connected to the module expansion node because the output node to connect doesn't exist. Now, the transform_reference transformer will unfold module call output references into simple module call references if the target node for a module call output does not exist.

The second occurs when referencing a module block directly (ie. not a module output), on a module that has no changes during the apply. The module expansion node was being removed as it had no direct references (as no changes from within the module). Now, the module expansion node implements the referenceable interface, and we'll connect nodes that reference the module directly both to the expansion node and the closer node. This will ensure the expansion of a module happens before anything that references it is processed.

Fixes #34976

Target Release

1.8.1

Draft CHANGELOG entry

BUG FIXES

• Fix crash in terraform plan when referencing a module output that does not exist within the try(...) function.
• Fix crash in terraform apply when referencing a module with no planned changes.

[jbardin]

OK, this is starting to make more sense. The original graph shape was structured around the variables and outputs, so only referencePath was really being used to connect for_each references. There was no real reference directly to a module until expansion was introduced, and even then it relied on the output objects for coordination. I think we may have a bug open somewhere about a module with no outputs not being referencable which this might also fix.

[github-actions]

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

[favoretti]

@jbardin I think this introduced a regression. We started getting cycle errors, where 1.8.0 didn't have them in statements where we actually use try() to verify whether a particular part of module output exists or not :) Is this intentional?

[jbardin]

Thanks @favoretti, I thought there was a parallel discussion about testing this against a self-referencing module for cycles, but maybe I was mistaken. I do see now how this will create a cycle in that situation though, so I'll take a closer look.

Thanks!

[jbardin]

In case someone gets to this before I do, it's a little more subtle than just a self-referencing module. If you have a module which has outputs which can be tied back to it's own inputs it still works, unless you have a reference that resolves to the entire module somehow:

module a {
  source = "./mod"
  first = "input"
  second = local.intermediate
}

locals {
  intermediate = try(module.a.maybe, "oops")
}

I can't think of any examples where this pattern would have worked without try, a direct reference to module.a would have produced a cycle previously too, and invalid outputs would have given Error: Unsupported attribute without try to catch the error. I'm not sure yet if it's possible to resolve this unique use of try, given that previously it would have only worked out with the correct values by chance when there was no edge to resolve the evaluation order.

[jbardin]

Hi @favoretti,

Unfortunately I'm not seeing an easy way to resolve this. Would you say that my example above is an accurate representation of the situation?

If that's the case, the problem is that this worked before by chance, only because the referencing was broken. Conceptually when you write try(module.name.something, ...), the fact that something may not exist means that you need to lookup the entire module.name object. If the result of that try value is then fed back into the same module you are going to have a cycle, because you are feeding the result back into the same object you are inspecting.

We get around this for single module instances without count or for_each with some tricks to connect only the exact variables and outputs, but when you add try into the mix you are creating a dynamic situation where we can't connect the exact nodes to bypass module call.

I'm going to keep an eye on this in the background to see if some new idea for reference resolution comes to mind -- we don't like to break existing configuration even if it was something we never intended to be used, but that behavior is also classified as a bug which makes it harder to reconcile.

Thanks!

[favoretti]

@jbardin Thanks for looking into this. Let us try and untangle the loop to verify that the scenario you poste above does represent what we've been accidentally abusing and get back to you on that.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.