From d8e6d663621ce7891a2f5ffe7ab32b82dce658f3 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 9 Oct 2020 11:09:36 -0400 Subject: [PATCH 1/6] use recorded changes for outputs We record output changes in the plan, but don't currently use them for anything other than display. If we have a wholly known output value stored in the plan, we should prefer that for apply in order to ensure consistency with the planned values. This also avoids cases where evaluation during apply cannot happen correctly, like when all resources are being removed or we are executing a destroy. We also need to record output Delete changes when the plan is for destroy operation. Otherwise without a change, the apply step will attempt to evaluate the outputs, causing errors, or leaving them in the state with stale values. --- terraform/context_apply_test.go | 5 +- terraform/graph_builder_apply.go | 2 +- terraform/graph_builder_destroy_plan.go | 5 + terraform/node_output.go | 143 +++++++++++++++++++----- terraform/transform_output.go | 22 +++- 5 files changed, 145 insertions(+), 32 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 625838b6b..28c8898cc 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -9142,10 +9142,13 @@ func TestContext2Apply_plannedDestroyInterpolatedCount(t *testing.T) { } // Applying the plan should now succeed - _, diags = ctx.Apply() + state, diags = ctx.Apply() if diags.HasErrors() { t.Fatalf("apply failed: %s", diags.Err()) } + if !state.Empty() { + t.Fatalf("state not empty: %s\n", state) + } } func TestContext2Apply_scaleInMultivarRef(t *testing.T) { diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 0ca7cf543..29f462548 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -91,7 +91,7 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { &RootVariableTransformer{Config: b.Config}, &ModuleVariableTransformer{Config: b.Config}, &LocalTransformer{Config: b.Config}, - &OutputTransformer{Config: b.Config}, + &OutputTransformer{Config: b.Config, Changes: b.Changes}, // Creates all the resource instances represented in the diff, along // with dependency edges against the whole-resource nodes added by diff --git a/terraform/graph_builder_destroy_plan.go b/terraform/graph_builder_destroy_plan.go index cbd0c12fc..d82162282 100644 --- a/terraform/graph_builder_destroy_plan.go +++ b/terraform/graph_builder_destroy_plan.go @@ -72,6 +72,11 @@ func (b *DestroyPlanGraphBuilder) Steps() []GraphTransformer { State: b.State, }, + &OutputTransformer{ + Config: b.Config, + Destroy: true, + }, + // Attach the state &AttachStateTransformer{State: b.State}, diff --git a/terraform/node_output.go b/terraform/node_output.go index 439c2a9cf..07d8ff335 100644 --- a/terraform/node_output.go +++ b/terraform/node_output.go @@ -11,15 +11,18 @@ import ( "github.com/hashicorp/terraform/lang" "github.com/hashicorp/terraform/plans" "github.com/hashicorp/terraform/states" + "github.com/hashicorp/terraform/tfdiags" "github.com/zclconf/go-cty/cty" ) // nodeExpandOutput is the placeholder for an output that has not yet had // its module path expanded. type nodeExpandOutput struct { - Addr addrs.OutputValue - Module addrs.Module - Config *configs.Output + Addr addrs.OutputValue + Module addrs.Module + Config *configs.Output + Changes []*plans.OutputChangeSrc + Destroy bool } var ( @@ -39,16 +42,62 @@ func (n *nodeExpandOutput) temporaryValue() bool { } func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) { + if n.Destroy { + return n.planDestroyOutputs(ctx) + } + var g Graph expander := ctx.InstanceExpander() for _, module := range expander.ExpandModule(n.Module) { + absAddr := n.Addr.Absolute(module) + + // Find any recorded change for this output + var change *plans.OutputChangeSrc + for _, c := range n.Changes { + if c.Addr.String() == absAddr.String() { + change = c + break + } + } + o := &NodeApplyableOutput{ - Addr: n.Addr.Absolute(module), + Addr: absAddr, + Config: n.Config, + Change: change, + } + log.Printf("[TRACE] Expanding output: adding %s as %T", o.Addr.String(), o) + g.Add(o) + } + return &g, nil +} + +// if we're planing a destroy operation, add destroy nodes for all root outputs +// in the state. +func (n *nodeExpandOutput) planDestroyOutputs(ctx EvalContext) (*Graph, error) { + // we only need to plan destroying root outputs + // Other module outputs may be used during destruction by providers that + // need to interpolate values. + if !n.Module.IsRoot() { + return nil, nil + } + + state := ctx.State() + if state == nil { + return nil, nil + } + + ms := state.Module(addrs.RootModuleInstance) + + var g Graph + for _, output := range ms.OutputValues { + o := &NodeDestroyableOutput{ + Addr: output.Addr, Config: n.Config, } log.Printf("[TRACE] Expanding output: adding %s as %T", o.Addr.String(), o) g.Add(o) } + return &g, nil } @@ -108,6 +157,8 @@ func (n *nodeExpandOutput) References() []*addrs.Reference { type NodeApplyableOutput struct { Addr addrs.AbsOutputValue Config *configs.Output // Config is the output in the config + // If this is being evaluated during apply, we may have a change recorded already + Change *plans.OutputChangeSrc } var ( @@ -199,27 +250,7 @@ func (n *NodeApplyableOutput) References() []*addrs.Reference { // GraphNodeExecutable func (n *NodeApplyableOutput) Execute(ctx EvalContext, op walkOperation) error { - // This has to run before we have a state lock, since evaluation also - // reads the state - val, diags := ctx.EvaluateExpr(n.Config.Expr, cty.DynamicPseudoType, nil) - // We'll handle errors below, after we have loaded the module. - - // Outputs don't have a separate mode for validation, so validate - // depends_on expressions here too - diags = diags.Append(validateDependsOn(ctx, n.Config.DependsOn)) - - // Ensure that non-sensitive outputs don't include sensitive values - _, marks := val.UnmarkDeep() - _, hasSensitive := marks["sensitive"] - if !n.Config.Sensitive && hasSensitive { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Output refers to sensitive values", - Detail: "Expressions used in outputs can only refer to sensitive values if the sensitive attribute is true.", - Subject: n.Config.DeclRange.Ptr(), - }) - } - + var diags tfdiags.Diagnostics state := ctx.State() if state == nil { return nil @@ -227,6 +258,40 @@ func (n *NodeApplyableOutput) Execute(ctx EvalContext, op walkOperation) error { changes := ctx.Changes() // may be nil, if we're not working on a changeset + val := cty.UnknownVal(cty.DynamicPseudoType) + changeRecorded := n.Change != nil + // we we have a change recorded, we don't need to re-evaluate if the value + // was known + if changeRecorded { + var err error + val, err = n.Change.After.Decode(cty.DynamicPseudoType) + diags = diags.Append(err) + } + + // If there was no change recorded, or the recorded change was not wholly + // known, then we need to re-evaluate the output + if !changeRecorded || !val.IsWhollyKnown() { + // This has to run before we have a state lock, since evaluation also + // reads the state + val, diags = ctx.EvaluateExpr(n.Config.Expr, cty.DynamicPseudoType, nil) + // We'll handle errors below, after we have loaded the module. + // Outputs don't have a separate mode for validation, so validate + // depends_on expressions here too + diags = diags.Append(validateDependsOn(ctx, n.Config.DependsOn)) + + // Ensure that non-sensitive outputs don't include sensitive values + _, marks := val.UnmarkDeep() + _, hasSensitive := marks["sensitive"] + if !n.Config.Sensitive && hasSensitive { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Output refers to sensitive values", + Detail: "Expressions used in outputs can only refer to sensitive values if the sensitive attribute is true.", + Subject: n.Config.DeclRange.Ptr(), + }) + } + } + // handling the interpolation error if diags.HasErrors() { if flagWarnOutputErrors { @@ -261,7 +326,7 @@ func (n *NodeApplyableOutput) DotNode(name string, opts *dag.DotOpts) *dag.DotNo } } -// NodeDestroyableOutput represents an output that is "destroybale": +// NodeDestroyableOutput represents an output that is "destroyable": // its application will remove the output from the state. type NodeDestroyableOutput struct { Addr addrs.AbsOutputValue @@ -293,6 +358,32 @@ func (n *NodeDestroyableOutput) Execute(ctx EvalContext, op walkOperation) error if state == nil { return nil } + + changes := ctx.Changes() + if changes != nil { + change := &plans.OutputChange{ + Addr: n.Addr, + Change: plans.Change{ + // This is just a weird placeholder delete action since + // we don't have an actual prior value to indicate. + // FIXME: Generate real planned changes for output values + // that include the old values. + Action: plans.Delete, + Before: cty.NullVal(cty.DynamicPseudoType), + After: cty.NullVal(cty.DynamicPseudoType), + }, + } + + cs, err := change.Encode() + if err != nil { + // Should never happen, since we just constructed this right above + panic(fmt.Sprintf("planned change for %s could not be encoded: %s", n.Addr, err)) + } + log.Printf("[TRACE] planDestroyOutput: Saving %s change for %s in changeset", change.Action, n.Addr) + changes.RemoveOutputChange(n.Addr) // remove any existing planned change, if present + changes.AppendOutputChange(cs) // add the new planned change + } + state.RemoveOutputValue(n.Addr) return nil } diff --git a/terraform/transform_output.go b/terraform/transform_output.go index d1b130c63..f0f33aa2c 100644 --- a/terraform/transform_output.go +++ b/terraform/transform_output.go @@ -6,6 +6,7 @@ import ( "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/dag" + "github.com/hashicorp/terraform/plans" ) // OutputTransformer is a GraphTransformer that adds all the outputs @@ -15,7 +16,12 @@ import ( // aren't changing since there is no downside: the state will be available // even if the dependent items aren't changing. type OutputTransformer struct { - Config *configs.Config + Config *configs.Config + Changes *plans.Changes + + // if this is a planed destroy, root outputs are still in the configuration + // so we need to record that we wish to remove them + Destroy bool } func (t *OutputTransformer) Transform(g *Graph) error { @@ -40,11 +46,19 @@ func (t *OutputTransformer) transform(g *Graph, c *configs.Config) error { // Add plannable outputs to the graph, which will be dynamically expanded // into NodeApplyableOutputs to reflect possible expansion // through the presence of "count" or "for_each" on the modules. + + var changes []*plans.OutputChangeSrc + if t.Changes != nil { + changes = t.Changes.Outputs + } + for _, o := range c.Module.Outputs { node := &nodeExpandOutput{ - Addr: addrs.OutputValue{Name: o.Name}, - Module: c.Path, - Config: o, + Addr: addrs.OutputValue{Name: o.Name}, + Module: c.Path, + Config: o, + Changes: changes, + Destroy: t.Destroy, } log.Printf("[TRACE] OutputTransformer: adding %s as %T", o.Name, node) g.Add(node) From ff21cc3c8d4d90defdb7b475eb9bfcf28e9ce707 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 9 Oct 2020 17:24:10 -0400 Subject: [PATCH 2/6] remove the need for destroyRootOutputTransformer Since root outputs can now use the planned changes, we can directly insert the correct applyable or destroyable node into the graph during plan and apply, and it will remove the outputs if they are being destroyed. --- terraform/graph_builder_apply.go | 11 --- terraform/graph_builder_plan_test.go | 4 +- terraform/node_output.go | 43 +++++------ terraform/node_resource_apply.go | 3 +- terraform/transform_output.go | 102 +++++++++++++-------------- terraform/transform_targets_test.go | 4 +- 6 files changed, 73 insertions(+), 94 deletions(-) diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 29f462548..5fc263516 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -150,17 +150,6 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { Schemas: b.Schemas, }, - // Create a destroy node for root outputs to remove them from the - // state. This does nothing unless invoked via the destroy command - // directly. A destroy is identical to a normal apply, except for the - // fact that we also have configuration to evaluate. While the rest of - // the unused nodes can be programmatically pruned (via - // pruneUnusedNodesTransformer), root module outputs always have an - // implied dependency on remote state. This means that if they exist in - // the configuration, the only signal to remove them is via the destroy - // command itself. - &destroyRootOutputTransformer{Destroy: b.Destroy}, - // We need to remove configuration nodes that are not used at all, as // they may not be able to evaluate, especially during destroy. // These include variables, locals, and instance expanders. diff --git a/terraform/graph_builder_plan_test.go b/terraform/graph_builder_plan_test.go index 956b147ea..84fec8740 100644 --- a/terraform/graph_builder_plan_test.go +++ b/terraform/graph_builder_plan_test.go @@ -288,10 +288,10 @@ local.instance_id (expand) aws_instance.web (expand) meta.count-boundary (EachMode fixup) aws_load_balancer.weblb (expand) - output.instance_id (expand) + output.instance_id openstack_floating_ip.random (expand) provider["registry.terraform.io/hashicorp/openstack"] -output.instance_id (expand) +output.instance_id local.instance_id (expand) provider["registry.terraform.io/hashicorp/aws"] openstack_floating_ip.random (expand) diff --git a/terraform/node_output.go b/terraform/node_output.go index 07d8ff335..a9a667bd7 100644 --- a/terraform/node_output.go +++ b/terraform/node_output.go @@ -15,8 +15,8 @@ import ( "github.com/zclconf/go-cty/cty" ) -// nodeExpandOutput is the placeholder for an output that has not yet had -// its module path expanded. +// nodeExpandOutput is the placeholder for a non-root module output that has +// not yet had its module path expanded. type nodeExpandOutput struct { Addr addrs.OutputValue Module addrs.Module @@ -37,17 +37,21 @@ var ( func (n *nodeExpandOutput) expandsInstances() {} func (n *nodeExpandOutput) temporaryValue() bool { - // this must always be evaluated if it is a root module output + // non root outputs are temporary return !n.Module.IsRoot() } func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) { if n.Destroy { - return n.planDestroyOutputs(ctx) + // if we're planning a destroy, we only need to handle the root outputs. + // The destroy plan doesn't evaluate any other config, so we can skip + // the rest of the outputs. + return n.planDestroyRootOutput(ctx) } - var g Graph expander := ctx.InstanceExpander() + + var g Graph for _, module := range expander.ExpandModule(n.Module) { absAddr := n.Addr.Absolute(module) @@ -71,32 +75,23 @@ func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) { return &g, nil } -// if we're planing a destroy operation, add destroy nodes for all root outputs -// in the state. -func (n *nodeExpandOutput) planDestroyOutputs(ctx EvalContext) (*Graph, error) { - // we only need to plan destroying root outputs - // Other module outputs may be used during destruction by providers that - // need to interpolate values. +// if we're planing a destroy operation, add a destroy node for any root output +func (n *nodeExpandOutput) planDestroyRootOutput(ctx EvalContext) (*Graph, error) { if !n.Module.IsRoot() { return nil, nil } - state := ctx.State() if state == nil { return nil, nil } - ms := state.Module(addrs.RootModuleInstance) - var g Graph - for _, output := range ms.OutputValues { - o := &NodeDestroyableOutput{ - Addr: output.Addr, - Config: n.Config, - } - log.Printf("[TRACE] Expanding output: adding %s as %T", o.Addr.String(), o) - g.Add(o) + o := &NodeDestroyableOutput{ + Addr: n.Addr.Absolute(addrs.RootModuleInstance), + Config: n.Config, } + log.Printf("[TRACE] Expanding output: adding %s as %T", o.Addr.String(), o) + g.Add(o) return &g, nil } @@ -149,6 +144,8 @@ func (n *nodeExpandOutput) ReferenceOutside() (selfPath, referencePath addrs.Mod // GraphNodeReferencer func (n *nodeExpandOutput) References() []*addrs.Reference { + // root outputs might be destroyable, and may not reference anything in + // that case return referencesForOutput(n.Config) } @@ -364,8 +361,6 @@ func (n *NodeDestroyableOutput) Execute(ctx EvalContext, op walkOperation) error change := &plans.OutputChange{ Addr: n.Addr, Change: plans.Change{ - // This is just a weird placeholder delete action since - // we don't have an actual prior value to indicate. // FIXME: Generate real planned changes for output values // that include the old values. Action: plans.Delete, @@ -379,7 +374,7 @@ func (n *NodeDestroyableOutput) Execute(ctx EvalContext, op walkOperation) error // Should never happen, since we just constructed this right above panic(fmt.Sprintf("planned change for %s could not be encoded: %s", n.Addr, err)) } - log.Printf("[TRACE] planDestroyOutput: Saving %s change for %s in changeset", change.Action, n.Addr) + log.Printf("[TRACE] NodeDestroyableOutput: Saving %s change for %s in changeset", change.Action, n.Addr) changes.RemoveOutputChange(n.Addr) // remove any existing planned change, if present changes.AppendOutputChange(cs) // add the new planned change } diff --git a/terraform/node_resource_apply.go b/terraform/node_resource_apply.go index 1994563fa..ff09ba614 100644 --- a/terraform/node_resource_apply.go +++ b/terraform/node_resource_apply.go @@ -26,7 +26,8 @@ var ( _ GraphNodeTargetable = (*nodeExpandApplyableResource)(nil) ) -func (n *nodeExpandApplyableResource) expandsInstances() {} +func (n *nodeExpandApplyableResource) expandsInstances() { +} func (n *nodeExpandApplyableResource) References() []*addrs.Reference { return (&NodeApplyableResource{NodeAbstractResource: n.NodeAbstractResource}).References() diff --git a/terraform/transform_output.go b/terraform/transform_output.go index f0f33aa2c..7685666ce 100644 --- a/terraform/transform_output.go +++ b/terraform/transform_output.go @@ -43,73 +43,67 @@ func (t *OutputTransformer) transform(g *Graph, c *configs.Config) error { } } - // Add plannable outputs to the graph, which will be dynamically expanded + // Add outputs to the graph, which will be dynamically expanded // into NodeApplyableOutputs to reflect possible expansion // through the presence of "count" or "for_each" on the modules. + // if this is a root output, we add the apply or destroy node directly, as + // the root modules does not expand var changes []*plans.OutputChangeSrc if t.Changes != nil { changes = t.Changes.Outputs } for _, o := range c.Module.Outputs { - node := &nodeExpandOutput{ - Addr: addrs.OutputValue{Name: o.Name}, - Module: c.Path, - Config: o, - Changes: changes, - Destroy: t.Destroy, + addr := addrs.OutputValue{Name: o.Name} + + var rootChange *plans.OutputChangeSrc + for _, c := range changes { + if c.Addr.Module.IsRoot() && c.Addr.OutputValue.Name == o.Name { + rootChange = c + } } + + var node dag.Vertex + switch { + case c.Path.IsRoot() && t.Destroy: + node = &NodeDestroyableOutput{ + Addr: addr.Absolute(addrs.RootModuleInstance), + Config: o, + } + case c.Path.IsRoot(): + destroy := t.Destroy + if rootChange != nil { + destroy = rootChange.Action == plans.Delete + } + + switch { + case destroy: + node = &NodeDestroyableOutput{ + Addr: addr.Absolute(addrs.RootModuleInstance), + Config: o, + } + default: + node = &NodeApplyableOutput{ + Addr: addr.Absolute(addrs.RootModuleInstance), + Config: o, + Change: rootChange, + } + } + + default: + node = &nodeExpandOutput{ + Addr: addr, + Module: c.Path, + Config: o, + Changes: changes, + Destroy: t.Destroy, + } + } + log.Printf("[TRACE] OutputTransformer: adding %s as %T", o.Name, node) g.Add(node) } return nil } - -// destroyRootOutputTransformer is a GraphTransformer that adds nodes to delete -// outputs during destroy. We need to do this to ensure that no stale outputs -// are ever left in the state. -type destroyRootOutputTransformer struct { - Destroy bool -} - -func (t *destroyRootOutputTransformer) Transform(g *Graph) error { - // Only clean root outputs on a full destroy - if !t.Destroy { - return nil - } - - for _, v := range g.Vertices() { - output, ok := v.(*nodeExpandOutput) - if !ok { - continue - } - - // We only destroy root outputs - if !output.Module.Equal(addrs.RootModule) { - continue - } - - // create the destroy node for this output - node := &NodeDestroyableOutput{ - Addr: output.Addr.Absolute(addrs.RootModuleInstance), - Config: output.Config, - } - - log.Printf("[TRACE] creating %s", node.Name()) - g.Add(node) - - deps := g.UpEdges(v) - - for _, d := range deps { - log.Printf("[TRACE] %s depends on %s", node.Name(), dag.VertexName(d)) - g.Connect(dag.BasicEdge(node, d)) - } - - // We no longer need the expand node, since we intend to remove this - // output from the state. - g.Remove(v) - } - return nil -} diff --git a/terraform/transform_targets_test.go b/terraform/transform_targets_test.go index b38be835c..9339b913e 100644 --- a/terraform/transform_targets_test.go +++ b/terraform/transform_targets_test.go @@ -122,7 +122,7 @@ module.child.module.grandchild.output.id (expand) module.child.module.grandchild.aws_instance.foo module.child.output.grandchild_id (expand) module.child.module.grandchild.output.id (expand) -output.grandchild_id (expand) +output.grandchild_id module.child.output.grandchild_id (expand) `) if actual != expected { @@ -193,7 +193,7 @@ module.child.module.grandchild.output.id (expand) module.child.module.grandchild.aws_instance.foo module.child.output.grandchild_id (expand) module.child.module.grandchild.output.id (expand) -output.grandchild_id (expand) +output.grandchild_id module.child.output.grandchild_id (expand) `) if actual != expected { From 0f5bf21983878fd15797ee29de50ab3a178756e6 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 9 Oct 2020 18:14:07 -0400 Subject: [PATCH 3/6] remove last use of the apply graph Destroy flag! The apply graph builder no longer uses the destroy flag, which is not always known since the destroy flag is not stored in the plan file. --- terraform/context.go | 1 - terraform/graph_builder_apply.go | 3 --- terraform/graph_builder_apply_test.go | 1 - 3 files changed, 5 deletions(-) diff --git a/terraform/context.go b/terraform/context.go index e3f26ef59..8db2e58ba 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -277,7 +277,6 @@ func (c *Context) Graph(typ GraphType, opts *ContextGraphOpts) (*Graph, tfdiags. Components: c.components, Schemas: c.schemas, Targets: c.targets, - Destroy: c.destroy, Validate: opts.Validate, }).Build(addrs.RootModuleInstance) diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 5fc263516..a0a5210af 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -40,9 +40,6 @@ type ApplyGraphBuilder struct { // outputs should go into the diff so that this is unnecessary. Targets []addrs.Targetable - // Destroy, if true, represents a pure destroy operation - Destroy bool - // Validate will do structural validation of the graph. Validate bool } diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index 9f49ff743..3e1f6e29c 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -468,7 +468,6 @@ func TestApplyGraphBuilder_provisionerDestroy(t *testing.T) { } b := &ApplyGraphBuilder{ - Destroy: true, Config: testModule(t, "graph-builder-apply-provisioner"), Changes: changes, Components: simpleMockComponentFactory(), From d82778f4fcbae074b6d6596559b433ab2b4563b4 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 9 Oct 2020 18:58:43 -0400 Subject: [PATCH 4/6] insert before values into the output changes Lookup before values for output changes. Use Update action when output has a non-null before value. --- terraform/node_output.go | 113 +++++++++++++++++++--------------- terraform/transform_output.go | 29 ++++----- 2 files changed, 75 insertions(+), 67 deletions(-) diff --git a/terraform/node_output.go b/terraform/node_output.go index a9a667bd7..84fb67408 100644 --- a/terraform/node_output.go +++ b/terraform/node_output.go @@ -356,15 +356,26 @@ func (n *NodeDestroyableOutput) Execute(ctx EvalContext, op walkOperation) error return nil } + // if this is a root module, try to get a before value from the state for + // the diff + before := cty.NullVal(cty.DynamicPseudoType) + mod := state.Module(n.Addr.Module) + if n.Addr.Module.IsRoot() && mod != nil { + for name, o := range mod.OutputValues { + if name == n.Addr.OutputValue.Name { + before = o.Value + break + } + } + } + changes := ctx.Changes() if changes != nil { change := &plans.OutputChange{ Addr: n.Addr, Change: plans.Change{ - // FIXME: Generate real planned changes for output values - // that include the old values. Action: plans.Delete, - Before: cty.NullVal(cty.DynamicPseudoType), + Before: before, After: cty.NullVal(cty.DynamicPseudoType), }, } @@ -395,6 +406,56 @@ func (n *NodeDestroyableOutput) DotNode(name string, opts *dag.DotOpts) *dag.Dot } func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.ChangesSync, val cty.Value) { + // If we have an active changeset then we'll first replicate the value in + // there and lookup the prior value in the state. This is used in + // preference to the state where present, since it *is* able to represent + // unknowns, while the state cannot. + if changes != nil { + // if this is a root module, try to get a before value from the state for + // the diff + before := cty.NullVal(cty.DynamicPseudoType) + mod := state.Module(n.Addr.Module) + if n.Addr.Module.IsRoot() && mod != nil { + for name, o := range mod.OutputValues { + if name == n.Addr.OutputValue.Name { + before = o.Value + break + } + } + } + + var action plans.Action + switch { + case val.IsNull(): + action = plans.Delete + case before.IsNull(): + action = plans.Create + case val.IsWhollyKnown() && val.Equals(before).True(): + action = plans.NoOp + default: + action = plans.Update + } + + change := &plans.OutputChange{ + Addr: n.Addr, + Sensitive: n.Config.Sensitive, + Change: plans.Change{ + Action: action, + Before: before, + After: val, + }, + } + + cs, err := change.Encode() + if err != nil { + // Should never happen, since we just constructed this right above + panic(fmt.Sprintf("planned change for %s could not be encoded: %s", n.Addr, err)) + } + log.Printf("[TRACE] ExecuteWriteOutput: Saving %s change for %s in changeset", change.Action, n.Addr) + changes.RemoveOutputChange(n.Addr) // remove any existing planned change, if present + changes.AppendOutputChange(cs) // add the new planned change + } + if val.IsKnown() && !val.IsNull() { // The state itself doesn't represent unknown values, so we null them // out here and then we'll save the real unknown value in the planned @@ -408,50 +469,4 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C state.RemoveOutputValue(n.Addr) } - // If we also have an active changeset then we'll replicate the value in - // there. This is used in preference to the state where present, since it - // *is* able to represent unknowns, while the state cannot. - if changes != nil { - // For the moment we are not properly tracking changes to output - // values, and just marking them always as "Create" or "Destroy" - // actions. A future release will rework the output lifecycle so we - // can track their changes properly, in a similar way to how we work - // with resource instances. - - var change *plans.OutputChange - if !val.IsNull() { - change = &plans.OutputChange{ - Addr: n.Addr, - Sensitive: n.Config.Sensitive, - Change: plans.Change{ - Action: plans.Create, - Before: cty.NullVal(cty.DynamicPseudoType), - After: val, - }, - } - } else { - change = &plans.OutputChange{ - Addr: n.Addr, - Sensitive: n.Config.Sensitive, - Change: plans.Change{ - // This is just a weird placeholder delete action since - // we don't have an actual prior value to indicate. - // FIXME: Generate real planned changes for output values - // that include the old values. - Action: plans.Delete, - Before: cty.NullVal(cty.DynamicPseudoType), - After: cty.NullVal(cty.DynamicPseudoType), - }, - } - } - - cs, err := change.Encode() - if err != nil { - // Should never happen, since we just constructed this right above - panic(fmt.Sprintf("planned change for %s could not be encoded: %s", n.Addr, err)) - } - log.Printf("[TRACE] ExecuteWriteOutput: Saving %s change for %s in changeset", change.Action, n.Addr) - changes.RemoveOutputChange(n.Addr) // remove any existing planned change, if present - changes.AppendOutputChange(cs) // add the new planned change - } } diff --git a/terraform/transform_output.go b/terraform/transform_output.go index 7685666ce..964a3a898 100644 --- a/terraform/transform_output.go +++ b/terraform/transform_output.go @@ -64,31 +64,24 @@ func (t *OutputTransformer) transform(g *Graph, c *configs.Config) error { } } + destroy := t.Destroy + if rootChange != nil { + destroy = rootChange.Action == plans.Delete + } + var node dag.Vertex switch { - case c.Path.IsRoot() && t.Destroy: + case c.Path.IsRoot() && destroy: node = &NodeDestroyableOutput{ Addr: addr.Absolute(addrs.RootModuleInstance), Config: o, } - case c.Path.IsRoot(): - destroy := t.Destroy - if rootChange != nil { - destroy = rootChange.Action == plans.Delete - } - switch { - case destroy: - node = &NodeDestroyableOutput{ - Addr: addr.Absolute(addrs.RootModuleInstance), - Config: o, - } - default: - node = &NodeApplyableOutput{ - Addr: addr.Absolute(addrs.RootModuleInstance), - Config: o, - Change: rootChange, - } + case c.Path.IsRoot(): + node = &NodeApplyableOutput{ + Addr: addr.Absolute(addrs.RootModuleInstance), + Config: o, + Change: rootChange, } default: From d2514a9abdf62344ea4136f63705225a70260a95 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 9 Oct 2020 19:45:31 -0400 Subject: [PATCH 5/6] update new outputs plan json --- command/testdata/show-json/basic-update/output.json | 4 ++-- command/testdata/show-json/basic-update/terraform.tfstate | 7 ++++++- .../testdata/show-json/multi-resource-update/output.json | 4 ++-- .../show-json/multi-resource-update/terraform.tfstate | 7 ++++++- 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/command/testdata/show-json/basic-update/output.json b/command/testdata/show-json/basic-update/output.json index 120491d45..5486bece2 100644 --- a/command/testdata/show-json/basic-update/output.json +++ b/command/testdata/show-json/basic-update/output.json @@ -55,9 +55,9 @@ "output_changes": { "test": { "actions": [ - "create" + "no-op" ], - "before": null, + "before": "bar", "after": "bar", "after_unknown": false } diff --git a/command/testdata/show-json/basic-update/terraform.tfstate b/command/testdata/show-json/basic-update/terraform.tfstate index b57f60f84..bc691aee0 100644 --- a/command/testdata/show-json/basic-update/terraform.tfstate +++ b/command/testdata/show-json/basic-update/terraform.tfstate @@ -3,7 +3,12 @@ "terraform_version": "0.12.0", "serial": 7, "lineage": "configuredUnchanged", - "outputs": {}, + "outputs": { + "test": { + "value": "bar", + "type": "string" + } + }, "resources": [ { "mode": "managed", diff --git a/command/testdata/show-json/multi-resource-update/output.json b/command/testdata/show-json/multi-resource-update/output.json index cc8f6d1ed..6725c2546 100644 --- a/command/testdata/show-json/multi-resource-update/output.json +++ b/command/testdata/show-json/multi-resource-update/output.json @@ -90,9 +90,9 @@ "output_changes": { "test": { "actions": [ - "create" + "no-op" ], - "before": null, + "before": "bar", "after": "bar", "after_unknown": false } diff --git a/command/testdata/show-json/multi-resource-update/terraform.tfstate b/command/testdata/show-json/multi-resource-update/terraform.tfstate index b57f60f84..bc691aee0 100644 --- a/command/testdata/show-json/multi-resource-update/terraform.tfstate +++ b/command/testdata/show-json/multi-resource-update/terraform.tfstate @@ -3,7 +3,12 @@ "terraform_version": "0.12.0", "serial": 7, "lineage": "configuredUnchanged", - "outputs": {}, + "outputs": { + "test": { + "value": "bar", + "type": "string" + } + }, "resources": [ { "mode": "managed", From 28e42816744219420671c960c013bbdacd187449 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 12 Oct 2020 12:10:01 -0400 Subject: [PATCH 6/6] handle sensitivity in the OutputChange The state is not loaded here with any marks, so we cannot rely on marks alone for equality comparison. Compare both the state and the configuration sensitivity before creating the OutputChange. --- terraform/node_output.go | 28 +++++++++++++++++++++++++--- terraform/transform_output.go | 5 +++-- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/terraform/node_output.go b/terraform/node_output.go index 84fb67408..00f39c80e 100644 --- a/terraform/node_output.go +++ b/terraform/node_output.go @@ -358,11 +358,13 @@ func (n *NodeDestroyableOutput) Execute(ctx EvalContext, op walkOperation) error // if this is a root module, try to get a before value from the state for // the diff + sensitiveBefore := false before := cty.NullVal(cty.DynamicPseudoType) mod := state.Module(n.Addr.Module) if n.Addr.Module.IsRoot() && mod != nil { for name, o := range mod.OutputValues { if name == n.Addr.OutputValue.Name { + sensitiveBefore = o.Sensitive before = o.Value break } @@ -372,7 +374,8 @@ func (n *NodeDestroyableOutput) Execute(ctx EvalContext, op walkOperation) error changes := ctx.Changes() if changes != nil { change := &plans.OutputChange{ - Addr: n.Addr, + Addr: n.Addr, + Sensitive: sensitiveBefore, Change: plans.Change{ Action: plans.Delete, Before: before, @@ -413,32 +416,51 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C if changes != nil { // if this is a root module, try to get a before value from the state for // the diff + sensitiveBefore := false before := cty.NullVal(cty.DynamicPseudoType) mod := state.Module(n.Addr.Module) if n.Addr.Module.IsRoot() && mod != nil { for name, o := range mod.OutputValues { if name == n.Addr.OutputValue.Name { before = o.Value + sensitiveBefore = o.Sensitive break } } } + // We will not show the value is either the before or after are marked + // as sensitivity. We can show the value again once sensitivity is + // removed from both the config and the state. + sensitiveChange := sensitiveBefore || n.Config.Sensitive + + // strip any marks here just to be sure we don't panic on the True comparison + val, _ = val.UnmarkDeep() + var action plans.Action switch { case val.IsNull(): action = plans.Delete + case before.IsNull(): action = plans.Create - case val.IsWhollyKnown() && val.Equals(before).True(): + + case val.IsWhollyKnown() && + val.Equals(before).True() && + n.Config.Sensitive == sensitiveBefore: + // Sensitivity must also match to be a NoOp. + // Theoretically marks may not match here, but sensitivity is the + // only one we can act on, and the state will have been loaded + // without any marks to consider. action = plans.NoOp + default: action = plans.Update } change := &plans.OutputChange{ Addr: n.Addr, - Sensitive: n.Config.Sensitive, + Sensitive: sensitiveChange, Change: plans.Change{ Action: action, Before: before, diff --git a/terraform/transform_output.go b/terraform/transform_output.go index 964a3a898..1e40f9edd 100644 --- a/terraform/transform_output.go +++ b/terraform/transform_output.go @@ -47,8 +47,6 @@ func (t *OutputTransformer) transform(g *Graph, c *configs.Config) error { // into NodeApplyableOutputs to reflect possible expansion // through the presence of "count" or "for_each" on the modules. - // if this is a root output, we add the apply or destroy node directly, as - // the root modules does not expand var changes []*plans.OutputChangeSrc if t.Changes != nil { changes = t.Changes.Outputs @@ -69,6 +67,9 @@ func (t *OutputTransformer) transform(g *Graph, c *configs.Config) error { destroy = rootChange.Action == plans.Delete } + // If this is a root output, we add the apply or destroy node directly, + // as the root modules does not expand. + var node dag.Vertex switch { case c.Path.IsRoot() && destroy: