-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Allow provider maintainers to define identifying attributes which should always be shown in plan diffs #30753
Comments
It's interesting to observe here that in practice the very attributes that tend to be useful for identification are also the ones that will rarely be changed in-place, because changing them could be considered as deleting an object and creating a new one, rather than an in-place edit. Although our existing heuristic of just recognizing certain common names as identifying across all providers does get us most of the way there (as you mentioned), it does seem like providers need to be the decider here in order for the result to be useful in all cases. It may well end up being a separate concern, but I think it would also be good to think about other ways Terraform could use the identifying attributes information other than just deciding which attributes to show when unchanged. For example, perhaps it would make sense to make the code that presents diffs for sets use explicitly-declared identifying attributes to infer when a particular change ought to be represented as an in-place update of an existing object, rather than removing one object and adding another in its place. I mention this here not because I want addressing that to be a blocker for resolving this issue, but just in case we might design the provider protocol features a little differently in the event we might extend it for this further improvement later. |
Closed #30811 as a dupe of this issue. The referenced issue contains a specific use case that would benefit from the feature described here. |
Off-hand question: I'm wondering if something like this could help with terraform-plugin-sdk's set handling issues like #28281, which my (potentially incorrect) understanding is likely due to its internal set value hashes changing the ordering of things. I don't think that particular problem should occur in terraform-plugin-framework though since it implements set types as slices through and through. EDIT: Okay, yeah, I'm just reiterating what Martin is mentioning above. 😄 |
I think #28281 is also running into old SDK quirks where the protocol 4 shims are lossy and e.g. cause null values to get replaced by empty strings and other such quirkiness. We currently filter out that sort of noise at the leaves of the plan renderer (individual attribute values), but the higher-level logic which decides if e.g. an entire block is equal to its predecessor still gets tripped up by it. Offering some way for a provider to hint that the plan renderer should use a particular set of attributes as a correlation key would allow Terraform to present those changes as So all of that is to say: I think there's a bit of both in #28281, but I wouldn't expect correlation hints to fully solve it. (We might at that point still choose to close #28281 anyway, since the new framework exists as our canonical full answer, albeit one that requires an incredibly heavy lift to adopt for existing providers.) |
We are running into situations over in terraform-plugin-framework where the framework logic could use this information for its underlying planning and data handling logic. hashicorp/terraform-plugin-framework#720 is a fairly trivial proposal (in terms of how its defined by provider developers anyways) that I think may be applicable for here is well. I'm wondering if there is any particular pitfalls or downsides to just naming the identifying attributes, or if additional information about these identifying attributes might be required for the Terraform side of this type of enhancement. There's still other design work required should this warrant being exposed across the protocol, but going down that road might be a welcome enhancement for practitioner experience since it could partially resolve at least some use cases in #27547 once provider developers opt into it. EDIT: Some additional context about potential protocol updates. Theoretically, the framework enhancement could be exposed to Terraform via: message Schema {
message Block {
# ... other fields ...
repeated Attribute attributes = 2;
repeated string identifying_attributes = 7;
}
message Object {
# ... other fields ...
repeated Attribute attributes = 1;
repeated string identifying_attributes = 6;
}
# ... other messages/fields ...
} Then theoretically, Terraform could interrogate this information while rendering the plan. Otherwise, |
I think the main question that was left unanswered in earlier discussion was whether we want to hang any additional semantic meaning on the idea of "identifying attributes" beyond just that they are useful to show in the plan UI even when they haven't changed. The linked issue hashicorp/terraform-plugin-framework#720 proposes that indeed it would have the additional meaning of representing a custom unique identifier for a collection of objects, which I think is a reasonable goal but raises some new design questions we'd need to answer:
I don't mean the above points as an argument against trying to solve this. Instead I'm just trying to think through what implications that change is likely to have for existing design assumptions that we might need to change to support a deeper meaning of "identifying attribute" than just the original request to show certain attributes even if they haven't changed. |
Thank you for the deeper consideration, @apparentlymart, this is exactly the type of design questions I would hope would come out of this. At the end of the day, maybe its overly optimistic that a single field can capture the nuances here. Some initial, personal thoughts below --
There are certainly (at least) two choices here. In the case that the answer is no, then I believe this information is more about providing "hints" to various logic (whether it be the framework trying to align prior state data or Terraform's plan renderer) about what attributes might be more "important" to consider. It would therefore be okay to take this information into account if it is known, but otherwise fallback to existing logic. Maybe "identifying" is the wrong terminology in this case. In the case that the answer is yes, then I believe a lot more design consideration comes into play on both sides of the protocol. The further questions are certainly applicable for this choice. On the provider side, it should enforce this rule and provide specific recommendations to developers should this rule trigger a runtime implementation/validation error.
Presumably, I think anyways, "identifying" attributes by nature would not be able to support the notion of "semantic" equality. It would be treated the same as changing the element key of a map. Some anti-patterns to this notion would then be cases where identifier-like values may take relative or absolute forms or where they may be case insensitive. Not supporting semantic equality there does solve some thorny problems, so it may be worth the explicit design choice.
I think this touches on what is trying to be solved on the provider side though, while a list may be ordered, a complex key could be used to align prior state data with its new index. It is not about changing the semantics of how a list type operates, but more about aligning computed data during planning.
This is a key (🥁 ) discussion point, going back to your first consideration question. If this information is just "hints" and may contain computed values, then the heuristic would mainly be for the purposes of plan rendering always showing certain information for objects. Otherwise, I think it must only support configurable and known planned values to avoid this situation. Terraform would have to defer validation until apply-time in certain cases to support this though. Having gone though this response, it does feel a little less than ideal to overload multiple concepts here into a single field as they may require different values/nuances. If the answer here should be that there is explicit "always show these attributes as part of a plan" information in the protocol, then I think that's a separate implementation from what we are trying to accomplish on the provider side of realigning computed data for list and set elements where they are potentially rearranged. |
Is there any workaround in the meantime? I mean, how can I expose the hidden resources? |
Wait. Are we saying that we know the unchanged attributes, but to this day it is impossible to provide this information to users. I must be confused. This cannot be |
Current Terraform Version
Use-cases
When Terraform is displaying a concise diff, unchanged attributes other than
id
,name
, andtags
are suppressed. In some cases, this can make the output difficult or impossible to understand, as the identifying data is unchanged and hidden.Two examples from the "verbose plan" enhancement request:
The
google_iam_policy
data sourcebinding
blocks suppress the uniquerole
attribute:The only unchanged element here is
role
, which is exactly what is needed to identify the meaning of this diff. In this case,binding
is a nested block backed by a set.aws_cloudfront_distribution
changes are hard to understand as identifying attributes for each rule are suppressed:In this case,
ordered_cache_behavior
is a nested block backed by a list, so Terraform is able to determine which elements have changed. It's not immediately clear which attributes ought to be displayed here, as I believe none are truly uniquely identifying, but my guess would bepath_pattern
,allowed_methods
, andcached_methods
.References
The text was updated successfully, but these errors were encountered: