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

Only show external changes which contributed to the plan #30486

Merged
merged 19 commits into from
Mar 18, 2022
Merged

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Feb 7, 2022

This is a continuation of the Contributing Resources Prototype in #28944.

Using the same mechanisms for static analysis of the configuration, we take a different approach to the displaying of contributing changes in the plan, with the end goal being to display as little extraneous information as possible to the user. The assumption made here is that extra refresh information, i.e "false positives", are a larger impediment to user's understanding of the plan than possibly hidden refresh information, which in most cases can be still inferred from the changes in the plan output. Taking this premise, we record only the specific attribute addresses that contributed to the plan, and filter the output down to those specific references.

Previously we would also show all external attribute changes for any resource instance with planned changes, because there is no way to infer any relationships between attributes made by the provider. While this may be the case technically, in practice the fact that the updated value will be shown in the diff is usually enough to deduce what is causing the inconsistency. In all cases a full refresh plan can be still run to see the drift report in its entirety.

The process of filtering these diffs is a bit of a hack, but I didn't want to risk any change to the diff rendering in this PR. What we do here is to create a fake after value for the change during rendering, starting with the before value, and only adding in the changes from the real after value which were marked as relevant. This lets us use the same diff renderer, but shows a reduced set of changes in the normal plan output.

As an example of the reduced output, take this configuration

resource "simple_resource" "ok" {
  for_each = { a = "1", b = "2" }
}

resource "simple_resource" "two" {
  value    = simple_resource.ok["a"].id
}

resource "simple_resource" "three" {
  for_each = simple_resource.ok
}

These fake resources will update their id attribute on every read, but only simple_resource.two makes use of this attribute in its configuration. Even though all resources will show external changes in a refresh-only plan, a normal plan will output:

Note: Objects have changed outside of Terraform

Terraform detected the following changes made outside of Terraform since the last "terraform
apply" which may have affected this plan:

  # simple_resource.ok["a"] has changed
  ~ resource "simple_resource" "ok" {
      ~ id = "2022-02-07 15:05:56.954005" -> "2022-02-07 15:05:59.009979"
    }


Unless you have made equivalent changes to your configuration, or ignored the relevant attributes
using ignore_changes, the following plan may include actions to undo or respond to these changes.

───────────────────────────────────────────────────────────────────────────────────────────────────

Terraform used the selected providers to generate the following execution plan. Resource actions
are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # simple_resource.two will be updated in-place
  ~ resource "simple_resource" "two" {
        id    = "2022-02-07 15:05:59.010821"
      ~ value = "2022-02-07 15:05:56.954005" -> "2022-02-07 15:05:59.009979"
    }

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

Additionally we also add the resource instances and individual attributes which may have contributed to the planned changes to the json format of the plan in a relevant_attributes field. We use the already existing path encoding for individual attributes as defined for replace_paths.

This PR also does not address any ignore_changes alike feature for external changes rather than configuration changes. A goal here is to take the fact that the configuration already contains most of the information needed to determine if a particular value is used or not. The new analysis aims to take that existing context and automatically apply it to the external changes report, negating the need for an "unused attributes" feature. We can evaluate if that feature, or any additional flags are still needed in the future.

apparentlymart and others added 11 commits March 4, 2022 15:51
We've ended up implementing something approximately like this in a few
places now, so this is a centralized version that we can consolidate on
moving forward, gradually removing that duplication.
Previously the "providers" package contained only a type for representing
the schema of a particular object within a provider, and the terraform
package had the responsibility of aggregating many of those together to
describe the entire surface area of a provider.

Here we move what was previously terraform.ProviderSchema to instead be
providers.Schemas, retaining its existing API otherwise, and leave behind
a type alias to allow us to gradually update other references over time.

We've gradually been shrinking down the responsibilities of the
"terraform" package to just representing the graph components and
behaviors anyway, but the specific motivation for doing this _now_ is to
allow for other packages to both be called by the terraform package _and_
work with provider schemas at the same time, without creating a package
dependency cycle: instead, these other packages can just import the
"providers" package and not need to import the "terraform" package at all.

For now this does still leave the responsibility for _building_ a
providers.Schemas object over in the "terraform" package, because it's
currently doing that as part of some larger work that isn't easily
separable, and so reorganizing that would be a more involved and riskier
change than just moving the existing type elsewhere.
Our existing functionality for dealing with references generally only has
to concern itself with one level of references at a time, and only within
one module, because we use it to draw a dependency graph which then ends
up reflecting the broader context.

However, there are some situations where it's handy to be able to ask
questions about the indirect contributions to a particular expression in
the configuration, particularly for additional hints in the user interface
where we're just providing some extra context rather than changing
behavior.

This new "globalref" package therefore aims to be the home for algorithms
for use-cases like this. It introduces its own special "Reference" type
that wraps addrs.Reference to annotate it also with the usually-implied
context about where the references would be evaluated.

With that building block we can therefore ask questions whose answers
might involve discussing references in multiple packages at once, such as
"which resources directly or indirectly contribute to this expression?",
including indirect hops through input variables or output values which
would therefore change the evaluation context.

The current implementations of this are around mapping references onto the
static configuration expressions that they refer to, which is a pretty
broad and conservative approach that unfortunately therefore loses
accuracy when confronted with complex expressions that might take dynamic
actions on the contents of an object. My hunch is that this'll be good
enough to get some initial small use-cases solved, though there's plenty
room for improvement in accuracy.

It's somewhat ironic that this sort of "what is this value built from?"
question is the use-case I had in mind when I designed the "marks" feature
in cty, yet we've ended up putting it to an unexpected but still valid
use in Terraform for sensitivity analysis and our currently handling of
that isn't really tight enough to permit other concurrent uses of marks
for other use-cases. I expect we can address that later and so maybe we'll
try for a more accurate version of these analyses at a later date, but my
hunch is that this'll be good enough for us to still get some good use out
of it in the near future, particular related to helping understand where
unknown values came from and in tailoring our refresh results in plan
output to deemphasize detected changes that couldn't possibly have
contributed to the proposed plan.
Only show drift changes which may have affected the plan output.
Maybe we can ensure schemas are all loaded at this point, but we can
tackle that later.
@jbardin jbardin requested a review from a team March 4, 2022 20:52
@jbardin jbardin force-pushed the jbardin/drift branch 2 times, most recently from ef51e47 to 21ebef0 Compare March 8, 2022 17:39
@jbardin jbardin marked this pull request as ready for review March 11, 2022 16:48
@jbardin jbardin changed the title Contributing Resources Only show external changes which contributed to the plan Mar 11, 2022
@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

Convert a global reference to a specific AbsResource and attribute pair.
The hcl.Traversal is converted to a cty.Path at this point because plan
rendering is based on cty values.
Storing individual contributing attributes will allow finer tuning of
the plan rendering.

add contributing to outputs
This is functionally equivalent, but will allow us to filter the change
values directly for reduced drift output.
Filter the refresh changes from the normal plan UI at the attribute
level. We do this by constructing fake plans.Change records for diff
generation, reverting all attribute changes that do not match any of the
plan's ContributingResourceReferences.
Track individual instance drift rather than whole resources which
contributed to the plan. This will allow the output to be more precise,
and we can still use NoKey instances as a proxy for containing resources
when needed.
Add the resource instances and individual attributes which may have
contributed to the planned changes to the json format of the plan. We
use the existing path encoding for individual attributes, which is
already used in the replace_paths change field.
@jbardin jbardin merged commit fef66f9 into main Mar 18, 2022
@jbardin jbardin deleted the jbardin/drift branch March 18, 2022 18:19
@github-actions
Copy link

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

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants