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", 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/context_apply_test.go b/terraform/context_apply_test.go index 73614c59b..e36f48e65 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..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 } @@ -91,7 +88,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 @@ -150,17 +147,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_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(), 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/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 439c2a9cf..00f39c80e 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. +// 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 - Config *configs.Output + Addr addrs.OutputValue + Module addrs.Module + Config *configs.Output + Changes []*plans.OutputChangeSrc + Destroy bool } var ( @@ -34,17 +37,37 @@ 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) { - var g Graph + if n.Destroy { + // 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) + } + expander := ctx.InstanceExpander() + + var g Graph 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) @@ -52,6 +75,27 @@ func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) { return &g, nil } +// 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 + } + + var g Graph + 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 +} + func (n *nodeExpandOutput) Name() string { path := n.Module.String() addr := n.Addr.String() + " (expand)" @@ -100,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) } @@ -108,6 +154,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 +247,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 +255,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 +323,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 +355,44 @@ func (n *NodeDestroyableOutput) Execute(ctx EvalContext, op walkOperation) error if state == nil { return 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 { + sensitiveBefore = o.Sensitive + before = o.Value + break + } + } + } + + changes := ctx.Changes() + if changes != nil { + change := &plans.OutputChange{ + Addr: n.Addr, + Sensitive: sensitiveBefore, + Change: plans.Change{ + Action: plans.Delete, + Before: before, + 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] 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 + } + state.RemoveOutputValue(n.Addr) return nil } @@ -309,6 +409,75 @@ 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 + 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() && + 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: sensitiveChange, + 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 @@ -322,50 +491,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/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 d1b130c63..1e40f9edd 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 { @@ -37,65 +43,61 @@ 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. + + 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} + + var rootChange *plans.OutputChangeSrc + for _, c := range changes { + if c.Addr.Module.IsRoot() && c.Addr.OutputValue.Name == o.Name { + rootChange = c + } } + + destroy := t.Destroy + if rootChange != nil { + 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: + node = &NodeDestroyableOutput{ + Addr: addr.Absolute(addrs.RootModuleInstance), + Config: o, + } + + case c.Path.IsRoot(): + 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 {