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.

Sign up for GitHub

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

Plan correct optional and computed attributes in nested objects and sets #32536

Merged
merged 8 commits into from
Jan 25, 2023
Merged

Plan correct optional and computed attributes in nested objects and sets #32536

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
93f739e
optional+computed can contain nested objects
jbardin Jan 17, 2023
e661e91
failing test for nested set types
jbardin Jan 17, 2023
986127e
remove unused isConfig arg
jbardin Jan 18, 2023
ac4f5fe
reformat
jbardin Jan 18, 2023
048ee34
better handling of nested sets in objchange
jbardin Jan 18, 2023
470ed22
failing tests for nested sets in objchange
jbardin Jan 18, 2023
7ca9abe
optional+computed
jbardin Jan 19, 2023
e16b848
expand test case
jbardin Jan 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
244 changes: 58 additions & 186 deletions internal/plans/objchange/objchange.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,6 @@ func proposedNewNestedBlock(schema *configschema.NestedBlock, prior, config cty.
var newV cty.Value

switch schema.Nesting {

case configschema.NestingSingle:
// A NestingSingle configuration block value can be null, and since it
// cannot be computed we can always take the configuration value.
Expand Down Expand Up @@ -234,7 +233,7 @@ func proposedNewNestedBlock(schema *configschema.NestedBlock, prior, config cty.
// if the non-computed attribute values are identical.
var cmpVals [][2]cty.Value
if prior.IsKnown() && !prior.IsNull() {
cmpVals = setElementCompareValues(&schema.Block, prior, false)
cmpVals = setElementCompareValues(&schema.Block, prior)
}
configVLen := 0
if config.IsKnown() && !config.IsNull() {
Expand All @@ -250,6 +249,7 @@ func proposedNewNestedBlock(schema *configschema.NestedBlock, prior, config cty.
if used[i] {
continue
}

if cmp[1].RawEquals(configEV) {
priorEV = cmp[0]
used[i] = true // we can't use this value on a future iteration
Expand Down Expand Up @@ -286,64 +286,58 @@ func proposedNewAttributes(attrs map[string]*configschema.Attribute, prior, conf
}

configV := config.GetAttr(name)

var newV cty.Value
switch {
case attr.Computed && attr.Optional:
// This is the trickiest scenario: we want to keep the prior value
// if the config isn't overriding it. Note that due to some
// ambiguity here, setting an optional+computed attribute from
// config and then later switching the config to null in a
// subsequent change causes the initial config value to be "sticky"
// unless the provider specifically overrides it during its own
// plan customization step.
if configV.IsNull() {
newV = priorV
} else {
newV = configV
}
case attr.Computed:
// required isn't considered when constructing the plan, so attributes
// are essentially either computed or not computed. In the case of
// optional+computed, they are only computed when there is no
// configuration.
case attr.Computed && configV.IsNull():
// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// into the individual nested attributes to build the final
// value, unless the entire nested attribute is unknown.
newV = proposedNewNestedType(attr.NestedType, priorV, configV)
default:
if attr.NestedType != nil {
// For non-computed NestedType attributes, we need to descend
// into the individual nested attributes to build the final
// value, unless the entire nested attribute is unknown.
if !configV.IsKnown() {
newV = configV
} else {
newV = proposedNewNestedType(attr.NestedType, priorV, configV)
}
} else {
// For non-computed attributes, we always take the config value,
// even if it is null. If it's _required_ then null values
// should've been caught during an earlier validation step, and
// so we don't really care about that here.
newV = configV
}
// For non-computed attributes, we always take the config value,
// even if it is null. If it's _required_ then null values
// should've been caught during an earlier validation step, and
// so we don't really care about that here.
newV = configV
}
newAttrs[name] = newV
}
return newAttrs
}

func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value) cty.Value {
// If the config is null or empty, we will be using this default value.
// if the config isn't known at all, then we must use that value
if !config.IsNull() && !config.IsKnown() {
return config
}

// Even if the config is null or empty, we will be using this default value.
newV := config

switch schema.Nesting {
case configschema.NestingSingle:
if !config.IsNull() {
newV = cty.ObjectVal(proposedNewAttributes(schema.Attributes, prior, config))
} else {
newV = cty.NullVal(config.Type())
// If the config is null, we already have our value. If the attribute
// is optional+computed, we won't reach this branch with a null value
// since the computed case would have been taken.
if config.IsNull() {
break
}

newV = cty.ObjectVal(proposedNewAttributes(schema.Attributes, prior, config))

case configschema.NestingList:
// Nested blocks are correlated by index.
configVLen := 0
if config.IsKnown() && !config.IsNull() {
if !config.IsNull() {
configVLen = config.LengthInt()
}

Expand Down Expand Up @@ -434,7 +428,7 @@ func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value)
// if the non-computed attribute values are identical.
var cmpVals [][2]cty.Value
if prior.IsKnown() && !prior.IsNull() {
cmpVals = setElementCompareValuesFromObject(schema, prior)
cmpVals = setElementCompareValues(schema, prior)
}
configVLen := 0
if config.IsKnown() && !config.IsNull() {
Expand Down Expand Up @@ -484,165 +478,43 @@ func proposedNewNestedType(schema *configschema.Object, prior, config cty.Value)
// in proposedNewBlock. The result is a heuristic rather than an exact science,
// since e.g. two separate elements may reduce to the same value through this
// process. The caller must therefore be ready to deal with duplicates.
func setElementCompareValues(schema *configschema.Block, set cty.Value, isConfig bool) [][2]cty.Value {
func setElementCompareValues(schema attrPath, set cty.Value) [][2]cty.Value {
ret := make([][2]cty.Value, 0, set.LengthInt())
for it := set.ElementIterator(); it.Next(); {
_, ev := it.Element()
ret = append(ret, [2]cty.Value{ev, setElementCompareValue(schema, ev, isConfig)})
ret = append(ret, [2]cty.Value{ev, setElementComputedAsNull(schema, ev)})
}
return ret
}

// setElementCompareValue creates a new value that has all of the same
// non-computed attribute values as the one given but has all computed
// attribute values forced to null.
//
// If isConfig is true then non-null Optional+Computed attribute values will
// be preserved. Otherwise, they will also be set to null.
//
// The input value must conform to the schema's implied type, and the return
// value is guaranteed to conform to it.
func setElementCompareValue(schema *configschema.Block, v cty.Value, isConfig bool) cty.Value {
if v.IsNull() || !v.IsKnown() {
return v
}
type attrPath interface {
AttributeByPath(cty.Path) *configschema.Attribute
}

attrs := map[string]cty.Value{}
for name, attr := range schema.Attributes {
switch {
case attr.Computed && attr.Optional:
if isConfig {
attrs[name] = v.GetAttr(name)
} else {
attrs[name] = cty.NullVal(attr.ImpliedType())
}
case attr.Computed:
attrs[name] = cty.NullVal(attr.ImpliedType())
default:
attrs[name] = v.GetAttr(name)
func setElementComputedAsNull(schema attrPath, elem cty.Value) cty.Value {
elem, _ = cty.Transform(elem, func(path cty.Path, v cty.Value) (cty.Value, error) {
if v.IsNull() || !v.IsKnown() {
return v, nil
}
}

for name, blockType := range schema.BlockTypes {
elementType := blockType.Block.ImpliedType()

switch blockType.Nesting {
case configschema.NestingSingle, configschema.NestingGroup:
attrs[name] = setElementCompareValue(&blockType.Block, v.GetAttr(name), isConfig)

case configschema.NestingList, configschema.NestingSet:
cv := v.GetAttr(name)
if cv.IsNull() || !cv.IsKnown() {
attrs[name] = cv
continue
}

if l := cv.LengthInt(); l > 0 {
elems := make([]cty.Value, 0, l)
for it := cv.ElementIterator(); it.Next(); {
_, ev := it.Element()
elems = append(elems, setElementCompareValue(&blockType.Block, ev, isConfig))
}

switch {
case blockType.Nesting == configschema.NestingSet:
// SetValEmpty would panic if given elements that are not
// all of the same type, but that's guaranteed not to
// happen here because our input value was _already_ a
// set and we've not changed the types of any elements here.
attrs[name] = cty.SetVal(elems)

// NestingList cases
case elementType.HasDynamicTypes():
attrs[name] = cty.TupleVal(elems)
default:
attrs[name] = cty.ListVal(elems)
}
} else {
switch {
case blockType.Nesting == configschema.NestingSet:
attrs[name] = cty.SetValEmpty(elementType)

// NestingList cases
case elementType.HasDynamicTypes():
attrs[name] = cty.EmptyTupleVal
default:
attrs[name] = cty.ListValEmpty(elementType)
}
}

case configschema.NestingMap:
cv := v.GetAttr(name)
if cv.IsNull() || !cv.IsKnown() || cv.LengthInt() == 0 {
attrs[name] = cv
continue
}
elems := make(map[string]cty.Value)
for it := cv.ElementIterator(); it.Next(); {
kv, ev := it.Element()
elems[kv.AsString()] = setElementCompareValue(&blockType.Block, ev, isConfig)
}

switch {
case elementType.HasDynamicTypes():
attrs[name] = cty.ObjectVal(elems)
default:
attrs[name] = cty.MapVal(elems)
}

default:
// Should never happen, since the above cases are comprehensive.
panic(fmt.Sprintf("unsupported block nesting mode %s", blockType.Nesting))
attr := schema.AttributeByPath(path)
if attr == nil {
// we must be at a nested block rather than an attribute, continue
return v, nil
}
}

return cty.ObjectVal(attrs)
}

// setElementCompareValues takes a known, non-null value of a cty.Set type and
// returns a table -- constructed of two-element arrays -- that maps original
// set element values to corresponding values that have all of the computed
// values removed, making them suitable for comparison with values obtained
// from configuration. The element type of the set must conform to the implied
// type of the given schema, or this function will panic.
//
// In the resulting slice, the zeroth element of each array is the original
// value and the one-indexed element is the corresponding "compare value".
//
// This is intended to help correlate prior elements with configured elements
// in proposedNewBlock. The result is a heuristic rather than an exact science,
// since e.g. two separate elements may reduce to the same value through this
// process. The caller must therefore be ready to deal with duplicates.
func setElementCompareValuesFromObject(schema *configschema.Object, set cty.Value) [][2]cty.Value {
ret := make([][2]cty.Value, 0, set.LengthInt())
for it := set.ElementIterator(); it.Next(); {
_, ev := it.Element()
ret = append(ret, [2]cty.Value{ev, setElementCompareValueFromObject(schema, ev)})
}
return ret
}

// setElementCompareValue creates a new value that has all of the same
// non-computed attribute values as the one given but has all computed
// attribute values forced to null.
//
// The input value must conform to the schema's implied type, and the return
// value is guaranteed to conform to it.
func setElementCompareValueFromObject(schema *configschema.Object, v cty.Value) cty.Value {
if v.IsNull() || !v.IsKnown() {
return v
}
attrs := map[string]cty.Value{}

for name, attr := range schema.Attributes {
attrV := v.GetAttr(name)
switch {
case attr.Computed:
attrs[name] = cty.NullVal(attr.Type)
default:
attrs[name] = attrV
if attr.Computed {
// If this could have been computed, we always return null in order
// to compare config and state values.
//
// The only way to determine if an optional+computed value is
// 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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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])

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@jbardin jbardin Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jbardin jbardin Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
return v, nil
})

return cty.ObjectVal(attrs)
return elem
}
Loading