Always evaluate resources in their entirety

[jbardin]

The primary goal of this PR is to always return entire resources to be evaluated, rather than individual instances. This allows us to push the evaluation of resource indexes down into expression evaluation.

Invalid indexed references to other resources were silently returning unknown values when the instance didn't exist. These were only caught by extra validation in data sources when the config still contained unknown values during ReadDataSource. This now will catch invalid indexes, and adds an IsWhollyKnown validation to the config during apply, preventing config values from being silently dropped.

Only data sources caught this previously, but returned a rather unhelpful error of

configuration for data.null_data_source.foo[0].id still contains unknown values during apply (this is a bug in Terraform; please report it!)

Now that indexes are handled during expression evaluation, we can get a full diagnostic output:

Error: Invalid index

  on main.tf line 7, in resource "null_resource" "b":
   7:     a = null_resource.foo[0].id
    |----------------
    | null_resource.foo is empty tuple

The given key does not identify an element in this collection value.

[apparentlymart]

I left some questions/feedback inline, but this broadly makes sense to me and I expect my questions may be arising from not having the context fully loaded.

[apparentlymart]

This was originally designed to be generic so that we could in principle support self in other contexts in future, since there are several issues open around that sort of thing. Fully generalizing this isn't necessary right now but since this limitation is not obvious in the Scope API, could we handle this by panicking a more specific error instead?

Relatedly, it seems like the condition right below this can no longer be true because if selfAddr were addrs.Self this type assertion would've panicked already. That branch is also just trying to present a more helpful panic message when the caller uses this API improperly, so perhaps we should invert this to let that one "win" for the more specific case.

[apparentlymart]

Rather than handling this as a totally special case up here, could we instead get the same result by setting rawSubj to subj.ContainingResource and then just letting this fall out into the switch statement below? If I'm reading this right, it seems like we'd then branch to the case addrs.Resource label and get the same result.

(Alternatively, maybe we could just skip populating managedResources altogether here; populating self should be sufficient I think, because if there were any references to the resource itself as well then we'll catch them on a different iteration of the loop. That would also remove the risk that some future enhancement allows self to refer to data resources and we end up incorrectly putting them in the managedResources map below, just thinking defensively about what ways outside code might violate these assumptions in future.)

[jbardin]

I think your latter suggestion is correct, and I just followed existing code+tests which ended up populating the managed resources.

We need to assign an instance to self in the final context, so I think this section is still required to extract the "self" value from the resource as a whole, whereas the switch below is adding resources.

[apparentlymart]

Could we remove this case by putting something like the following fixup before the switch here:

if addr, ok := rawSubj.(addrs.ResourceInstance); ok {
    rawSubj = addr.ContainingResource()
}

I think that wouldn't have been possible with how self was handled before, where each of these cases was responsible for detecting if it was self and setting it, but now that self is being handled as special anyway it seems like that removes the remaining need for this clause. I'm probably missing something though, since there seem to be a bunch of different codepaths through there and I'm not sure I'm catching all of the interactions.

[jbardin]

Good call. I think I meant to try and coalesce these cases and forgot

[jbardin]

Updated with the minor fixes from review.

[Ninir]

@jbardin It seems this PR is breaking the following use-case:

module "mymodule" {
  source           = "something/something"
  name             = "myname"
  create_resources = var.create_resources

  security_groups = [
    # ...
    aws_security_group.mysg[0].id,
  ]
}

resource "aws_security_group" "mysg" {
  count = var.create_resources ? 1 : 0
  name = "myname"
  # ...
}

Because we can't have count.index on the module (as suggested by #22480 (comment)), the only way I see to fix it is to use the coaslesce function, or add the check manually, as in:

module "mymodule" {
  source           = "something/something"
  name             = "myname"
  create_resources = var.create_resources

  security_groups = [
    # ...
    var.create_resources? aws_security_group.mysg[0].id : null,
  ]
}

# ...

We have a bunch of other places like that in our codebase, and thus I'm wondering if it should really be fixed that way or not.

Let me know if that makes sense!

[jbardin]

Hi @Ninir,

Yes, if there are no aws_security_group.mysg instances, the expression aws_security_group.mysg[0] should always fail since the index is invalid.

I would use which ever solution leads to the most readable result. Taking a guess from the snippet above, a splat expression should give you want you want

security_groups = aws_security_group.mysg[*].id

Which is equivalent to a list comprehension, which can add additional filtering as an if clause:

[for sg in aws_security_group.mysg : sg.id if ...]

If you don't want to operate on the instances as a complete group however, guarding single references with a conditional would be required.

[Ninir]

@jbardin ok, so it is up to people to fix their cases got it. Thank you!

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.