From 1e8cfc52e9fa472a2a19f03124edf239d7c84316 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Thu, 25 May 2017 09:31:31 -0700 Subject: [PATCH] helper/schema: Updated ResourceDiff prototype This new prototype removes the dependence on a underlying ResourceData, moving several items up to ensure an approach that more matches ResourceData. Namely, we create our own MultiLevelFieldReader that also layers updated diff values on top. New values use a small re-implementation of the MapFieldWriter/Reader that allows computed values to be set. The assumption here now is that a second diff will happen after the first one is done, processing the new values set in the 2 new reader/writer levels and updating the diff as necessary. --- helper/schema/resource_diff.go | 385 +++++++++++++++++++++++---------- 1 file changed, 276 insertions(+), 109 deletions(-) diff --git a/helper/schema/resource_diff.go b/helper/schema/resource_diff.go index 66b72d906..91658d3e5 100644 --- a/helper/schema/resource_diff.go +++ b/helper/schema/resource_diff.go @@ -4,128 +4,196 @@ import ( "fmt" "reflect" "strings" + "sync" "github.com/hashicorp/terraform/terraform" ) +// newValueWriter is a minor re-implementation of MapFieldWriter to include +// keys that should be marked as computed, to represent the new part of a +// pseudo-diff. +type newValueWriter struct { + *MapFieldWriter + + // A list of keys that should be marked as computed. + computedKeys map[string]bool + + // A lock to prevent races on writes. The underlying writer will have one as + // well - this is for computed keys. + lock sync.Mutex +} + +// WriteField overrides MapValueWriter's WriteField, adding the ability to flag +// the address as computed. +func (w *newValueWriter) WriteField(address []string, value interface{}, computed bool) error { + if err := w.MapFieldWriter.WriteField(address, value); err != nil { + return err + } + + w.lock.Lock() + defer w.lock.Unlock() + if w.result == nil { + w.computedKeys = make(map[string]bool) + } + + if computed { + w.computedKeys[strings.Join(address, ".")] = true + } + return nil +} + +// ComputedKeysMap returns the underlying computed keys map. +func (w *newValueWriter) ComputedKeysMap() map[string]bool { + w.lock.Lock() + defer w.lock.Unlock() + if w.result == nil { + w.computedKeys = make(map[string]bool) + } + return w.computedKeys +} + +// newValueReader is a minor re-implementation of MapFieldReader and is the +// read counterpart to MapValueWriter, allowing the read of keys flagged as +// computed to accommodate the diff override logic in ResourceDiff. +type newValueReader struct { + *MapFieldReader + + // The list of computed keys from a newValueWriter. + computedKeys map[string]bool +} + +// ReadField reads the values from the underlying writer, returning the +// computed value if it is found as well. +func (r *newValueReader) ReadField(address []string) (FieldReadResult, error) { + v, err := r.MapFieldReader.ReadField(address) + if err != nil { + return FieldReadResult{}, err + } + if _, ok := r.computedKeys[strings.Join(address, ".")]; ok { + v.Computed = true + } + + return v, nil +} + // ResourceDiff is used to query and make custom changes to an in-flight diff. // It can be used to veto particular changes in the diff, customize the diff // that has been created, or diff values not controlled by config. // // The object functions similar to ResourceData, however most notably lacks -// Set, SetPartial, and Partial, as it should only be used to change diff -// values only. Most other frist-class ResourceData functions exist, namely -// Get, GetOk, HasChange, and GetChange exist. +// Set, SetPartial, and Partial, as it should be used to change diff values +// only. Most other frist-class ResourceData functions exist, namely Get, +// GetOk, HasChange, and GetChange exist. // // All functions in ResourceDiff, save for ForceNew, can only be used on // computed fields. type ResourceDiff struct { - // The underlying ResourceData object used as a refrence and diff storage. - data *ResourceData + // The schema for the resource being worked on. + schema map[string]*Schema - // A source "copy" of the ResourceData object, designed to preserve the - // original diff, schema, and state to allow for rollbacks. - originalData *ResourceData + // The current config for this resource. + config *terraform.ResourceConfig - // A writer that holds overridden old fields. + // The state for this resource as it exists post-refresh, after the initial + // diff. + state *terraform.InstanceState + + // The diff created by Terraform. This diff is used, along with state, + // config, and custom-set diff data, to provide a multi-level reader + // experience similar to ResourceData. + diff *terraform.InstanceDiff + + // The internal reader structure that contains the state, config, the default + // diff, and the new diff. + multiReader *MultiLevelFieldReader + + // A writer that writes overridden old fields. oldWriter *MapFieldWriter - // A reader that is tied to oldWriter's map. - oldReader *MapFieldReader - - // A writer that holds overridden new fields. - newWriter *MapFieldWriter - - // A reader that is tied to newWriter's map. - newReader *MapFieldReader - - // A map of keys that will be force-flagged as computed. - computedKeys map[string]bool - - // A catalog of top-level keys that are safely diffable. diffChange will - // panic if the key is not found here to guard against bugs and edge cases - // when processing diffs. - catalog map[string]bool + // A writer that writes overridden new fields. + newWriter *newValueWriter } // newResourceDiff creates a new ResourceDiff instance. -func newResourceDiff(diff *terraform.InstanceDiff, data *ResourceData) *ResourceDiff { - d := new(ResourceDiff) - d.originalData = &ResourceData{ - schema: data.schema, - state: data.state, - config: data.config, +func newResourceDiff(schema map[string]*Schema, config *terraform.ResourceConfig, state *terraform.InstanceState, diff *terraform.InstanceDiff) *ResourceDiff { + d := &ResourceDiff{ + config: config, + state: state, diff: diff, } - d.data = new(ResourceData) - d.data.config = data.config - d.data.state = d.originalData.state.DeepCopy() - d.data.diff = d.originalData.diff.DeepCopy() - for k, v := range d.originalData.schema { + // Duplicate the passed in schema to ensure that any changes we make with + // functions like ForceNew don't affect the referenced schema. + for k, v := range schema { newSchema := *v - d.data.schema[k] = &newSchema + d.schema[k] = &newSchema } - d.data.once.Do(d.data.init) - - d.oldWriter = &MapFieldWriter{Schema: data.schema} - d.oldReader = &MapFieldReader{ - Schema: data.schema, + d.oldWriter = &MapFieldWriter{Schema: d.schema} + d.newWriter = &newValueWriter{ + MapFieldWriter: &MapFieldWriter{Schema: d.schema}, + } + readers := make(map[string]FieldReader) + var stateAttributes map[string]string + if d.state != nil { + stateAttributes = d.state.Attributes + readers["state"] = &MapFieldReader{ + Schema: d.schema, + Map: BasicMapReader(stateAttributes), + } + } + if d.config != nil { + readers["config"] = &ConfigFieldReader{ + Schema: d.schema, + Config: d.config, + } + } + if d.diff != nil { + readers["diff"] = &DiffFieldReader{ + Schema: d.schema, + Diff: d.diff, + Source: &MultiLevelFieldReader{ + Levels: []string{"state", "config"}, + Readers: readers, + }, + } + } + readers["newDiffOld"] = &MapFieldReader{ + Schema: d.schema, Map: BasicMapReader(d.oldWriter.Map()), } + readers["newDiffNew"] = &newValueReader{ + MapFieldReader: &MapFieldReader{ + Schema: d.schema, + Map: BasicMapReader(d.newWriter.Map()), + }, + computedKeys: d.newWriter.ComputedKeysMap(), + } + d.multiReader = &MultiLevelFieldReader{ + Levels: []string{ + "state", + "config", + "diff", + "newDiffOld", + "newDiffNew", + }, - d.newWriter = &MapFieldWriter{Schema: data.schema} - d.newReader = &MapFieldReader{ - Schema: data.schema, - Map: BasicMapReader(d.newWriter.Map()), + Readers: readers, } return d } -// ClearAll re-creates the ResourceDiff instance and drops the old one on the -// floor. The new instance starts off without a diff. +// ClearAll wipes the current diff. This cannot be undone - use only if you +// need to create a whole new diff from scatch, such as when you are leaning on +// the provider completely to create the diff. func (d *ResourceDiff) ClearAll() { - nd := newResourceDiff(d.originalData.diff, d.originalData) - nd.data.diff = new(terraform.InstanceDiff) - d = nd -} - -// Reset re-creates the ResourceDiff instance, similar to ClearAll, but with -// the original diff preserved. -func (d *ResourceDiff) Reset() { - nd := newResourceDiff(d.originalData.diff, d.originalData) - d = nd -} - -// getDiff returns the current diff as it is in the underlying ResourceData -// object. -func (d *ResourceDiff) getDiff() *terraform.InstanceDiff { - return d.data.diff + d.diff = new(terraform.InstanceDiff) } // diffChange helps to implement resourceDiffer and derives its change values -// from ResourceDiff's own change data. -// -// Note that it's a currently unsupported operation to diff a field (and hence -// use this function) on a field that has not been explicitly operated on with -// SetNew, SetNewComputed, or SetDiff. The function will panic if you do. +// from ResourceDiff's own change data, in addition to existing diff, config, and state. func (d *ResourceDiff) diffChange(key string) (interface{}, interface{}, bool, bool) { - // Panic if key was never set by any of our diff functions. It's not a legit - // use case of this function to be used outside of very specific functions in - // ResourceDiff. - if _, ok := d.catalog[key]; !ok { - panic(fmt.Errorf("ResourceDiff.diffChange: %s was not found as a valid set key", key)) - } - - old, err := d.oldReader.ReadField(strings.Split(key, ".")) - if err != nil { - panic(fmt.Errorf("ResourceDiff.diffChange: Reading old value for %s failed: %s", key, err)) - } - new, err := d.newReader.ReadField(strings.Split(key, ".")) - if err != nil { - panic(fmt.Errorf("ResourceDiff.diffChange: Reading new value for %s failed: %s", key, err)) - } + old, new := d.getChange(key) if !old.Exists { old.Value = nil @@ -134,12 +202,7 @@ func (d *ResourceDiff) diffChange(key string) (interface{}, interface{}, bool, b new.Value = nil } - var computed bool - if v, ok := d.computedKeys[strings.Split(key, ".")[0]]; ok { - computed = v - } - - return old.Value, new.Value, !reflect.DeepEqual(old.Value, new.Value), computed + return old.Value, new.Value, !reflect.DeepEqual(old.Value, new.Value), new.Computed } // SetNew is used to set a new diff value for the mentioned key. The value must @@ -160,7 +223,7 @@ func (d *ResourceDiff) SetNew(key string, value interface{}) error { // // This function is only allowed on computed keys. func (d *ResourceDiff) SetNewComputed(key string) error { - return d.SetDiff(key, d.Get(key), d.data.schema[key].ZeroValue(), true) + return d.SetDiff(key, d.Get(key), d.schema[key].ZeroValue(), true) } // SetDiff allows the setting of both old and new values for the diff @@ -171,24 +234,19 @@ func (d *ResourceDiff) SetNewComputed(key string) error { // // This function is only allowed on computed keys. func (d *ResourceDiff) SetDiff(key string, old, new interface{}, computed bool) error { - if !d.data.schema[key].Computed { + if !d.schema[key].Computed { return fmt.Errorf("SetNew, SetNewComputed, and SetDiff are allowed on computed attributes only - %s is not one", key) } if err := d.oldWriter.WriteField(strings.Split(key, "."), old); err != nil { - delete(d.catalog, key) return fmt.Errorf("Cannot set old diff value for key %s: %s", key, err) } - if err := d.newWriter.WriteField(strings.Split(key, "."), new); err != nil { - delete(d.catalog, key) + if err := d.newWriter.WriteField(strings.Split(key, "."), new, computed); err != nil { return fmt.Errorf("Cannot set new diff value for key %s: %s", key, err) } - d.computedKeys[key] = computed - d.catalog[key] = true - - return schemaMap(d.data.schema).diff(key, d.data.schema[key], d.data.diff, d, false) + return nil } // ForceNew force-flags ForceNew in the schema for a specific key, and @@ -203,31 +261,140 @@ func (d *ResourceDiff) ForceNew(key string) error { } old, new := d.GetChange(key) - d.data.schema[key].ForceNew = true + d.schema[key].ForceNew = true return d.SetDiff(key, old, new, false) } // Get hands off to ResourceData.Get. func (d *ResourceDiff) Get(key string) interface{} { - return d.data.Get(key) + r, _ := d.GetOk(key) + return r } -// GetChange hands off to ResourceData.GetChange. +// GetChange gets the change between the state and diff, checking first to see +// if a overridden diff exists. +// +// This implementation differs from ResourceData's in the way that we first get +// results from the exact levels for the new diff, then from state and diff as +// per normal. func (d *ResourceDiff) GetChange(key string) (interface{}, interface{}) { - return d.data.getChange(key, getSourceState, getSourceDiff) + old, new := d.getChange(key) + return old.Value, new.Value } -// GetOk hands off to ResourceData.GetOk. +// GetOk functions the same way as ResourceData.GetOk, but it also checks the +// new diff levels to provide data consistent with the current state of the +// customized diff. func (d *ResourceDiff) GetOk(key string) (interface{}, bool) { - return d.data.GetOk(key) + r := d.get(strings.Split(key, "."), "newDiffNew") + exists := r.Exists && !r.Computed + if exists { + // If it exists, we also want to verify it is not the zero-value. + value := r.Value + zero := r.Schema.Type.Zero() + + if eq, ok := value.(Equal); ok { + exists = !eq.Equal(zero) + } else { + exists = !reflect.DeepEqual(value, zero) + } + } + + return r.Value, exists } -// HasChange hands off to ResourceData.HasChange. +// HasChange checks to see if there is a change between state and the diff, or +// in the overridden diff. func (d *ResourceDiff) HasChange(key string) bool { - return d.data.HasChange(key) + old, new := d.GetChange(key) + + // If the type implements the Equal interface, then call that + // instead of just doing a reflect.DeepEqual. An example where this is + // needed is *Set + if eq, ok := old.(Equal); ok { + return !eq.Equal(new) + } + + return !reflect.DeepEqual(old, new) } -// Id hands off to ResourceData.Id. +// Id returns the ID of this resource. +// +// Note that technically, ID does not change during diffs (it either has +// already changed in the refresh, or will change on update), hence we do not +// support updating the ID or fetching it from anything else other than state. func (d *ResourceDiff) Id() string { - return d.data.Id() + var result string + + if d.state != nil { + result = d.state.ID + } + return result +} + +// getChange gets values from two different levels, designed for use in +// diffChange, HasChange, and GetChange. +// +// This implementation differs from ResourceData's in the way that we first get +// results from the exact levels for the new diff, then from state and diff as +// per normal. +func (d *ResourceDiff) getChange(key string) (getResult, getResult) { + old := d.getExact(strings.Split(key, "."), "newDiffOld") + new := d.getExact(strings.Split(key, "."), "newDiffNew") + + if old.Exists && new.Exists { + // Both values should exist if SetDiff operated on this key. + // TODO: Maybe verify this. Zero values might be an issue here. + return old, new + } + + // If we haven't set this in the new diff, then we want to get the default + // levels as if we were using ResourceData normally. + old = d.get(strings.Split(key, "."), "state") + new = d.get(strings.Split(key, "."), "diff") + return old, new +} + +// get performs the appropriate multi-level reader logic for ResourceDiff, +// starting at source. Refer to newResourceDiff for the level order. +func (d *ResourceDiff) get(addr []string, source string) getResult { + result, err := d.multiReader.ReadFieldMerge(addr, source) + if err != nil { + panic(err) + } + + return d.finalizeResult(addr, result) +} + +// getExact gets an attribute from the exact level referenced by source. +func (d *ResourceDiff) getExact(addr []string, source string) getResult { + result, err := d.multiReader.ReadFieldExact(addr, source) + if err != nil { + panic(err) + } + + return d.finalizeResult(addr, result) +} + +// finalizeResult does some post-processing of the result produced by get and getExact. +func (d *ResourceDiff) finalizeResult(addr []string, result FieldReadResult) getResult { + // If the result doesn't exist, then we set the value to the zero value + var schema *Schema + if schemaL := addrToSchema(addr, d.schema); len(schemaL) > 0 { + schema = schemaL[len(schemaL)-1] + } + + if result.Value == nil && schema != nil { + result.Value = result.ValueOrZero(schema) + } + + // Transform the FieldReadResult into a getResult. It might be worth + // merging these two structures one day. + return getResult{ + Value: result.Value, + ValueProcessed: result.ValueProcessed, + Computed: result.Computed, + Exists: result.Exists, + Schema: schema, + } }