query: propagate graph node removal to descendants

[dsa0x]

This replaces the resourceMatcher function with a transformer called towards the end of the graph transformation process.

At that stage, all resources and blocks should have been added into the graph. The transformer targets the list resource nodes in the graph, keeping all vertices along the path of each list resource nodes. This ensures that the final graph is complete in evaluating list resources.

Target Release

1.15.x

Rollback Plan

• If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

CHANGELOG entry

• This change is user-facing and I added a changelog entry.
• This change is not user-facing.

[github-actions]

The equivalence tests failed. Please investigate here.

[jbardin]

I think you want to reverse the logic here and find what you need to keep rather than remove -- you can see the current TargetsTransformer for an example. While it's unlikely to happen between resources right now with the structure of query files, this can technically remove things like named values or providers you depend on, because those can still reference non-list resources.

[dsa0x]

🤔 I intentionally let them be removed, because they are probably not able to evaluate correctly if their dependencies are missing. Since those nodes depend on a node that has been removed, their evaluation would likely result in an error. Is that what we want rather than removing them?

[jbardin]

The correct way to evaluate dependencies is to follow the dependencies from the source (usually via Ancestors). If you walk in the opposite direction, you don't know where those nodes in turn have their own dependencies. This means that a node which is a shared dependency of what you want to keep and what you want to remove could be removed. Take this minimal example

# .tf
ephemeral test_secret key {
}

provider "bufo" {
  key = ephemeral.test_secret.key
}

# .tfquery.hcl
list bufo_resource all {
}

When you reach ephemeral.test_secret_key you would walk backwards and remove the bufo provider which the list resource depends on, and actually remove the list resource too! What instead needs to happen is that you start from the list resource and keep everything that it depends on.

[dsa0x]

Thank you. Yeah the list resource itself would get removed here 🥲 . However, unlike -target, which would try to keep as may things in the graph that the target may need (here the target is the list resource), we want to still remove ephemeral.test_secret_key because we only want to keep list resource nodes, and potentially other non-resource node.

When we implement this with Ancestors like you suggested, we also would keep provider.bufo, but not ephemeral.test_secret_key. When we try to evaluate the provider node, we would probably fail because it depends on a non-list resource. That would be okay, right?

[jbardin]

In order for list resources to work, you need to be able to configure their provider. The list.bufo_resource in my example would require that the bufo provider be correctly configured to operate, and that configuration requires the ephemeral.test_secret resource. As far as I understood it, the primary reason for including the main configuration in query evaluation is because we need the providers which are configured there.

Since providers can depend on literally anything, you must take all dependencies into account. I think the only alternative would be to require static provider configuration directly in the tfquery file, then there would be no reason to evaluate the rest of the config at all.

Your graph at this point is the source of truth for the sum of all dependencies between nodes, so you have to trust that everything upstream from list resources is required for the list resources. Trying to individually pick out nodes without following all the dependencies is only going to lead to eventual errors.

[dsa0x]

I think then this brings us back to the point where terraform query may then also execute managed resources. I thought we wanted to avoid that in the first place.

Also If the attribute that the provider depends on is known after apply, the provider cannot be configured, and the list cannot be evaluated.

If we are mixing configuration like this, we either have to accept that

1. a query plan does not only operate on query files. At best, it's like a target for list blocks, and may still include resources needed for those blocks to execute.
2. we take a hard stance on preventing cross-usage, and (mis)use would likely lead to confusing errors.

[dsa0x]

I went with the second option in 6a8f3d2

in the new example test, the list resource depends on a provider which depends on a managed resource. We will turn this into plan errors like
"Reference to uninitialized resource"

[jbardin]

Yeah, technically the provider can reference managed resources too, although of course it's not recommended. If the query is being executed when that resource already exists in the state, then it will probably still work; but if the value is unknown during the plan, then there's not much we can do other than error out.

[jbardin]

There can be other types of resource nodes in the graph here, just skipping these is not a complete guarantee that you can't pickup something unusual. Note that GraphNodeConfigResource encompasses all resource modes, so you could be removing things you need as well.

In the rare case of a managed resource is captured as a dependency of a list resource (which would probably just be through a provider for now), I would say it's probably OK to just continue and see what happens. You should be at least a -refresh-only type mode, so there shouldn't be any changes to plan.

[dsa0x]

Note that GraphNodeConfigResource encompasses all resource modes, so you could be removing things you need as well.

Because the outer loop is going over the entire graph, and keeping all list resource nodes (on line 42), we would have kept it, even if it wasn't added in this internal loop.

[dsa0x]

I would say it's probably OK to just continue and see what happens. You should be at least a -refresh-only type mode, so there shouldn't be any changes to plan.

We could also do that. But if it isn't a refresh mode, e.g. the resource is not yet in the state, the operation cannot succeed. But it would not succeed anyway if the resource is unknown

[jbardin]

Exactly! It can't work either way, so we probably don't need to go out of our way to avoid it, while also risking other inconsistencies in the graph.

[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.