Plan correct optional and computed attributes in nested objects and sets #32536
Conversation
When structural attributes were added, optional+computed were not correctly handled when containing nested values which could themselves be computed. This would cause terraform to ignore previously computed values from state when generating the proposed plan. The special case for optional+computed was incorrect, but isn't needed in the context of planning new values anyway. Attributes are either computed, or not computed. When optional+computed is set and there is no configuration, the attribute is treated as computed. It is up to the provider to determine how and when to deal with any changes to that computed value.
jbardin
force-pushed
the
jbardin/optional-computed-nested-objects
branch
from
0038b64
to
51713f9
Compare
Combine and simplify the set comparison functions for NestingSet blocks and attribute types. The set handling for structural attributes was not recursing into nested values. Once a simplified method for comparing set elements was devised for nested types, it turns out the same method could be applied to nested set blocks as well.
Add a number of test cases which fail without the prior changes.
jbardin
force-pushed
the
jbardin/optional-computed-nested-objects
branch
from
51713f9
to
470ed22
Compare
Simplify the proposedNewAttributes cases, and add another test for coverage.
jbardin
force-pushed
the
jbardin/optional-computed-nested-objects
branch
from
e34a360
to
7ca9abe
Compare
| // optional or computed is see if it is set in the config, however | ||
| // for set elements we don't yet know which element value | ||
| // corresponds to the configuration. | ||
| return cty.NullVal(v.Type()), nil |
There was a problem hiding this comment.
The code just returns null for any computed value with disregard whether it's specified in config or not (in case it's both computed and optional). That means that the comparison with config returns false and the proposed new state is set to config instead of the prior state (cmp[0])
There was a problem hiding this comment.
That is intended. This transformation is used to create value to attempt and correlate config with prior state within a set, it is not directly used as a plan value. Because we don't know what the original config value was, we cannot tell if the prior value was computed or not, so we must assume it was computed.
There was a problem hiding this comment.
It leads to dirty plans for unchanged configurations with sets that have underlying computed and optional attributes, so sets cannot be used in these cases. At least with the current framework implementation.
There was a problem hiding this comment.
That is correct. If there are no stable attributes which can correlate configuration to prior state, then you are not going to get a very useful proposed plan. Set elements are identified by their value, so if a set element could have all computed attributes there's no way to determine if it could have originated from the given configuration or not (the simpler example is where an optional+computed value is removed from the configuration and Terraform cannot tell if it was originally computed or not, but made worse by being included within a set).
Using computed values at all within a set would be unusual unless you have very specific needs and fully understand all the implications when trying to plan changes to the values.
There was a problem hiding this comment.
Sorry, extra changes are expected, but this should not cause "dirty plans" when there are no changes. I thought I confirmed tests for the no-changes case, but let me double check on that,
There was a problem hiding this comment.
The test fails if we make the optional attribute to be both optional and computed - "optional": {Type: cty.String, Optional: true, Computed: true},. The check for equality fails in that case and the config is used while it should be the prior state.
There was a problem hiding this comment.
@dimuon, yes, that's what I meant above by there being no non-computed attributes which can be used to correlate config and state values. This is just inherent in trying to use optional+computed values within a set, because we cannot always determine whether a value was generated from config or not. There is no correct value to send in this case, and either will result in false positives, but we happen to send the config when it exists (in most cases, I have some null edge cases to consider in a followup PR). That does not mean it can't be fixed up by the provider during the plan, but in practice you should carefully consider whether optional+computed is really what you want to use in the first place.
The specific behavior BTW is captured in this test.
There was a problem hiding this comment.
@jbardin , I see your point. IMO the current design means that set works as expected only if it contains no more than one computed and optional underlying attributes. Once set contains 2 or more such attributes, it can lead to "dirty plan" for configurations with no change (e.g. when configuration defines one of these optional and computed fields and doesn't set any value for the second and state contains a computed value the second attribute). Please correct that if my conclusion is wrong.
Regarding fixing the behavior in the provider. It looks like the default way to do this is plan modifiers. But if the resource is big, the provider has to provide plan modifies for every computed field (or a plan modifier for the whole resource) but writing a plan modifier can be tricky for complex resources because plan modifier should understand relationship between different fields to make a correct decision (e.g. whether it should use the current state of fallback to Unknown).
I'm not sure whether the implementation can be improved though. Can we skip nullifying computed attributes before comparing them to config but not take computed attribute into comparison if state has some value for it and config doesn't?
Anyway, @jbardin , thank you for the interesting discussion.
There was a problem hiding this comment.
@dimuon, IMHO optional+computed should be used sparingly, and in hindsight probably should not have been allowed within set types, but that was decided long ago ;)
The problem with your suggestion lies in the fact that sets are unordered data types indexed only by the element value itself, so when the value in config and the value in state are not exactly equal, we cannot tell "if state has some value for it and config doesn't", because we don't know if those were logically the same value in the first place.
I do have another idea for a comparison method I'm going test out later (this PR was mainly aimed at fixing errors in the current behavior, not changing the behavior). While it's not a solvable problem, we may be able to do the comparison using the lifecycle rules that a provider must abide by to try and determine which values may share a common lineage. I don't know yet if that will result in plans looking more closely like the common cases providers expect, or if we're going to generate different changes breaking existing providers.
There was a problem hiding this comment.
FYI most of this PR will be superseded by a later PR that is going to test a new set comparison method which can check for most of these edge cases.
| // configV will always be null in this case, by definition. | ||
| // priorV may also be null, but that's okay. | ||
| newV = priorV | ||
| case attr.NestedType != nil: | ||
| // For non-computed NestedType attributes, we need to descend |
There was a problem hiding this comment.
This refers to "non-computed NestedType attributes", but the case statement doesn't test for attr.Computed. Am I misunderstanding or should this read case !attr.Computed && attr.NestedType != nil?
There was a problem hiding this comment.
It's effectively non-computed because the config must be non-null, which is check in the case above. If it were optional+computed being treated as computed it's still caught in the first case.
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
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. |
When structural attributes were added, optional+computed attributes were not correctly handled when they contain nested values which could themselves be computed. This would cause terraform to ignore previously computed values from state when generating the proposed plan. The special case for optional+computed was incorrect, but isn't needed in the context of planning new values anyway -- attributes are either computed, or not computed depending on the existence of a configuration value. It is up to the provider to determine how and when to deal with any changes to that computed value.
We can also combine and simplify the set comparison functions for NestingSet blocks and attribute types. The set handling was not recursing into nested values, and once a simplified method for comparing set elements was devised for nested attributes, it turns out the same method could be applied to nested set blocks as well.
While it may be safer to leave the questionable set block behavior as-is to avoid possible regressions with legacy providers, the framework has added full support for nested blocks, which means that new providers now have access to the complete values and are no longer limited by the behaviors of the legacy SDK. A number of tests cases for nested set blocks were added here, which do fail without the accompanying changes.
Fixes #32522