From d53c3d5c1b6c38e826f316a71f48fbea3394e6b4 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Mon, 10 Sep 2018 16:26:55 -0700 Subject: [PATCH] plans: Retain output value changes for all outputs in memory During the plan operation we need to retain _somewhere_ the planned changes for all outputs so we can refer to them during expression evaluation. For consistency with how we handle resource instance changes, we'll keep them in the plan so we can properly retain unknown values, which cannot be written to state. As with output values in the state, only root output plans are retained in a round-trip through the on-disk plan file format, but that's okay because we can trivially re-calculate all of these during apply. We include the _root_ outputs in the plan file only because they are externally-visible side effects that ought to be included in any rendering of the plan made from the plan file for user inspection. --- plans/changes.go | 37 ++++++++++++++++++---- plans/changes_src.go | 27 +++++++++++++++- plans/changes_sync.go | 55 +++++++++++++++++++++++++++++++++ plans/planfile/planfile_test.go | 4 +-- plans/planfile/tfplan.go | 23 +++++++++++--- plans/planfile/tfplan_test.go | 11 ++++--- 6 files changed, 139 insertions(+), 18 deletions(-) diff --git a/plans/changes.go b/plans/changes.go index fac1fec74..69bfffaaa 100644 --- a/plans/changes.go +++ b/plans/changes.go @@ -12,19 +12,27 @@ import ( // A Changes object can be rendered into a visual diff (by the caller, using // code in another package) for display to the user. type Changes struct { - Resources []*ResourceInstanceChangeSrc - RootOutputs map[string]*OutputChangeSrc + // Resources tracks planned changes to resource instance objects. + Resources []*ResourceInstanceChangeSrc + + // Outputs tracks planned changes output values. + // + // Note that although an in-memory plan contains planned changes for + // outputs throughout the configuration, a plan serialized + // to disk retains only the root outputs because they are + // externally-visible, while other outputs are implementation details and + // can be easily re-calculated during the apply phase. Therefore only root + // module outputs will survive a round-trip through a plan file. + Outputs []*OutputChangeSrc } // NewChanges returns a valid Changes object that describes no changes. func NewChanges() *Changes { - return &Changes{ - RootOutputs: make(map[string]*OutputChangeSrc), - } + return &Changes{} } func (c *Changes) Empty() bool { - return (len(c.Resources) + len(c.RootOutputs)) == 0 + return (len(c.Resources) + len(c.Outputs)) == 0 } // ResourceInstance returns the planned change for the current object of the @@ -55,6 +63,19 @@ func (c *Changes) ResourceInstanceDeposed(addr addrs.AbsResourceInstance, key st return nil } +// OutputValue returns the planned change for the output value with the +// given address, if any. Returns nil if no change is planned. +func (c *Changes) OutputValue(addr addrs.AbsOutputValue) *OutputChangeSrc { + addrStr := addr.String() + for _, oc := range c.Outputs { + if oc.Addr.String() == addrStr { + return oc + } + } + + return nil +} + // SyncWrapper returns a wrapper object around the receiver that can be used // to make certain changes to the receiver in a concurrency-safe way, as long // as all callers share the same wrapper object. @@ -203,6 +224,10 @@ func (rc *ResourceInstanceChange) Simplify(destroying bool) *ResourceInstanceCha // OutputChange describes a change to an output value. type OutputChange struct { + // Addr is the absolute address of the output value that the change + // will apply to. + Addr addrs.AbsOutputValue + // Change is an embedded description of the change. // // For output value changes, the type constraint for the DynamicValue diff --git a/plans/changes_src.go b/plans/changes_src.go index 6c46f9b31..f2782ac3c 100644 --- a/plans/changes_src.go +++ b/plans/changes_src.go @@ -94,8 +94,12 @@ func (rcs *ResourceInstanceChangeSrc) DeepCopy() *ResourceInstanceChangeSrc { return &ret } -// OutputChange describes a change to an output value. +// OutputChangeSrc describes a change to an output value. type OutputChangeSrc struct { + // Addr is the absolute address of the output value that the change + // will apply to. + Addr addrs.AbsOutputValue + // ChangeSrc is an embedded description of the not-yet-decoded change. // // For output value changes, the type constraint for the DynamicValue @@ -117,11 +121,32 @@ func (ocs *OutputChangeSrc) Decode() (*OutputChange, error) { return nil, err } return &OutputChange{ + Addr: ocs.Addr, Change: *change, Sensitive: ocs.Sensitive, }, nil } +// DeepCopy creates a copy of the receiver where any pointers to nested mutable +// values are also copied, thus ensuring that future mutations of the receiver +// will not affect the copy. +// +// Some types used within a resource change are immutable by convention even +// though the Go language allows them to be mutated, such as the types from +// the addrs package. These are _not_ copied by this method, under the +// assumption that callers will behave themselves. +func (ocs *OutputChangeSrc) DeepCopy() *OutputChangeSrc { + if ocs == nil { + return nil + } + ret := *ocs + + ret.ChangeSrc.Before = ret.ChangeSrc.Before.Copy() + ret.ChangeSrc.After = ret.ChangeSrc.After.Copy() + + return &ret +} + // ChangeSrc is a not-yet-decoded Change. type ChangeSrc struct { // Action defines what kind of change is being made. diff --git a/plans/changes_sync.go b/plans/changes_sync.go index 258869986..e9305eaf9 100644 --- a/plans/changes_sync.go +++ b/plans/changes_sync.go @@ -87,3 +87,58 @@ func (cs *ChangesSync) RemoveResourceInstanceChange(addr addrs.AbsResourceInstan return } } + +// AppendOutputChange records the given output value change in the set of +// planned value changes. +// +// The caller must ensure that there are no concurrent writes to the given +// change while this method is running, but it is safe to resume mutating +// it after this method returns without affecting the saved change. +func (cs *ChangesSync) AppendOutputChange(changeSrc *OutputChangeSrc) { + if cs == nil { + panic("AppendOutputChange on nil ChangesSync") + } + cs.lock.Lock() + defer cs.lock.Unlock() + + s := changeSrc.DeepCopy() + cs.changes.Outputs = append(cs.changes.Outputs, s) +} + +// GetOutputChange searches the set of output value changes for one matching +// the given address, returning it if it exists. +// +// If no such change exists, nil is returned. +// +// The returned object is a deep copy of the change recorded in the plan, so +// callers may mutate it although it's generally better (less confusing) to +// treat planned changes as immutable after they've been initially constructed. +func (cs *ChangesSync) GetOutputChange(addr addrs.AbsOutputValue) *OutputChangeSrc { + if cs == nil { + panic("GetOutputChange on nil ChangesSync") + } + cs.lock.Lock() + defer cs.lock.Unlock() + + return cs.changes.OutputValue(addr) +} + +// RemoveOutputChange searches the set of output value changes for one matching +// the given address, and removes it from the set if it exists. +func (cs *ChangesSync) RemoveOutputChange(addr addrs.AbsOutputValue) { + if cs == nil { + panic("RemoveOutputChange on nil ChangesSync") + } + cs.lock.Lock() + defer cs.lock.Unlock() + + addrStr := addr.String() + for i, r := range cs.changes.Resources { + if r.Addr.String() != addrStr { + continue + } + copy(cs.changes.Outputs[i:], cs.changes.Outputs[i+1:]) + cs.changes.Outputs = cs.changes.Outputs[:len(cs.changes.Outputs)-1] + return + } +} diff --git a/plans/planfile/planfile_test.go b/plans/planfile/planfile_test.go index 7624b3b0e..5b44e05b2 100644 --- a/plans/planfile/planfile_test.go +++ b/plans/planfile/planfile_test.go @@ -43,8 +43,8 @@ func TestRoundtrip(t *testing.T) { // file is tested more fully in tfplan_test.go . planIn := &plans.Plan{ Changes: &plans.Changes{ - Resources: []*plans.ResourceInstanceChangeSrc{}, - RootOutputs: map[string]*plans.OutputChangeSrc{}, + Resources: []*plans.ResourceInstanceChangeSrc{}, + Outputs: []*plans.OutputChangeSrc{}, }, ProviderSHA256s: map[string][]byte{}, VariableValues: map[string]plans.DynamicValue{ diff --git a/plans/planfile/tfplan.go b/plans/planfile/tfplan.go index 88dcf50d3..787542225 100644 --- a/plans/planfile/tfplan.go +++ b/plans/planfile/tfplan.go @@ -52,8 +52,8 @@ func readTfplan(r io.Reader) (*plans.Plan, error) { plan := &plans.Plan{ VariableValues: map[string]plans.DynamicValue{}, Changes: &plans.Changes{ - RootOutputs: map[string]*plans.OutputChangeSrc{}, - Resources: []*plans.ResourceInstanceChangeSrc{}, + Outputs: []*plans.OutputChangeSrc{}, + Resources: []*plans.ResourceInstanceChangeSrc{}, }, ProviderSHA256s: map[string][]byte{}, @@ -66,10 +66,14 @@ func readTfplan(r io.Reader) (*plans.Plan, error) { return nil, fmt.Errorf("invalid plan for output %q: %s", name, err) } - plan.Changes.RootOutputs[name] = &plans.OutputChangeSrc{ + plan.Changes.Outputs = append(plan.Changes.Outputs, &plans.OutputChangeSrc{ + // All output values saved in the plan file are root module outputs, + // since we don't retain others. (They can be easily recomputed + // during apply). + Addr: addrs.OutputValue{Name: name}.Absolute(addrs.RootModuleInstance), ChangeSrc: *change, Sensitive: rawOC.Sensitive, - } + }) } for _, rawRC := range rawPlan.ResourceChanges { @@ -288,7 +292,16 @@ func writeTfplan(plan *plans.Plan, w io.Writer) error { ResourceChanges: []*planproto.ResourceInstanceChange{}, } - for name, oc := range plan.Changes.RootOutputs { + for _, oc := range plan.Changes.Outputs { + // When serializing a plan we only retain the root outputs, since + // changes to these are externally-visible side effects (e.g. via + // terraform_remote_state). + if !oc.Addr.Module.IsRoot() { + continue + } + + name := oc.Addr.OutputValue.Name + // Writing outputs as cty.DynamicPseudoType forces the stored values // to also contain dynamic type information, so we can recover the // original type when we read the values back in readTFPlan. diff --git a/plans/planfile/tfplan_test.go b/plans/planfile/tfplan_test.go index 7ab7377f0..dcb76530a 100644 --- a/plans/planfile/tfplan_test.go +++ b/plans/planfile/tfplan_test.go @@ -21,15 +21,17 @@ func TestTFPlanRoundTrip(t *testing.T) { "foo": mustNewDynamicValueStr("foo value"), }, Changes: &plans.Changes{ - RootOutputs: map[string]*plans.OutputChangeSrc{ - "bar": { + Outputs: []*plans.OutputChangeSrc{ + { + Addr: addrs.OutputValue{Name: "bar"}.Absolute(addrs.RootModuleInstance), ChangeSrc: plans.ChangeSrc{ Action: plans.Create, After: mustNewDynamicValueStr("bar value"), }, Sensitive: false, }, - "baz": { + { + Addr: addrs.OutputValue{Name: "baz"}.Absolute(addrs.RootModuleInstance), ChangeSrc: plans.ChangeSrc{ Action: plans.NoOp, Before: mustNewDynamicValueStr("baz value"), @@ -37,7 +39,8 @@ func TestTFPlanRoundTrip(t *testing.T) { }, Sensitive: false, }, - "secret": { + { + Addr: addrs.OutputValue{Name: "secret"}.Absolute(addrs.RootModuleInstance), ChangeSrc: plans.ChangeSrc{ Action: plans.Update, Before: mustNewDynamicValueStr("old secret value"),