Merge pull request #16204 from hashicorp/jbardin/outputs

make outputs error

terraform/context.go

@@ -539,6 +539,7 @@ func (c *Context) Plan() (*Plan, error) {
 	var operation walkOperation
 	if c.destroy {
 		operation = walkPlanDestroy
+		p.Destroy = true
 	} else {
 		// Set our state to be something temporary. We do this so that
 		// the plan can update a fake state so that variables work, then

terraform/context_apply_test.go

@@ -8928,8 +8928,7 @@ func TestContext2Apply_providerWithLocals(t *testing.T) {
 	// Destroy won't work because the local value is removed before the
 	// provider.  Once this is fixed this test will start to fail, and we
 	// can remove the invalid interpolation string;
-	// if providerRegion != "bar" {
-	if providerRegion != "${local.foo}" {
+	if providerRegion != "bar" {
 		t.Fatalf("expected region %q, got: %q", "bar", providerRegion)
 	}
 }

terraform/context_plan_test.go

@@ -3614,3 +3614,106 @@ aws_instance.foo.1:
 		t.Fatalf("bad:\n%s\n\nexpected\n\n%s", actual, expected)
 	}
 }
+
+func TestContext2Plan_invalidOutput(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+data "aws_instance" "name" {}
+
+output "out" {
+  value = "${data.aws_instance.name.missing}"
+}`,
+	})
+
+	p := testProvider("aws")
+	ctx := testContext2(t, &ContextOpts{
+		Module: m,
+		ProviderResolver: ResourceProviderResolverFixed(
+			map[string]ResourceProviderFactory{
+				"aws": testProviderFuncFixed(p),
+			},
+		),
+	})
+
+	// if this ever fails to pass validate, add a resource to reference in the config
+	w, e := ctx.Validate()
+	if len(w) > 0 {
+		t.Fatalf("warnings generated on validate: %#v", w)
+	}
+	if len(e) > 0 {
+		t.Fatalf("errors generated on validate: %v", e)
+	}
+
+	_, err := ctx.Refresh()
+	if err != nil {
+		t.Fatalf("refresh err: %s", err)
+	}
+
+	_, err = ctx.Plan()
+	switch {
+	case featureOutputErrors:
+		if err == nil {
+			t.Fatal("expected error")
+		}
+	default:
+		if err != nil {
+			t.Fatalf("plan err: %s", err)
+		}
+	}
+}
+
+func TestContext2Plan_invalidModuleOutput(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"child/main.tf": `
+data "aws_instance" "name" {}
+
+output "out" {
+  value = "${data.aws_instance.name.missing}"
+}`,
+		"main.tf": `
+module "child" {
+  source = "./child"
+}
+
+resource "aws_instance" "foo" {
+  foo = "${module.child.out}"
+}`,
+	})
+
+	p := testProvider("aws")
+	ctx := testContext2(t, &ContextOpts{
+		Module: m,
+		ProviderResolver: ResourceProviderResolverFixed(
+			map[string]ResourceProviderFactory{
+				"aws": testProviderFuncFixed(p),
+			},
+		),
+	})
+
+	// if this ever fails to pass validate, add a resource to reference in the config
+	w, e := ctx.Validate()
+	if len(w) > 0 {
+		t.Fatalf("warnings generated on validate: %#v", w)
+	}
+	if len(e) > 0 {
+		t.Fatalf("errors generated on validate: %v", e)
+	}
+
+	_, err := ctx.Refresh()
+	if err != nil {
+		t.Fatalf("refresh err: %s", err)
+	}
+
+	_, err = ctx.Plan()
+	switch {
+	case featureOutputErrors:
+		if err == nil {
+			t.Fatal("expected error")
+		}
+	default:
+		if err != nil {
+			t.Fatalf("plan err: %s", err)
+		}
+	}
+
+}

terraform/eval_interpolate.go

@@ -9,14 +9,19 @@ import (
 // EvalInterpolate is an EvalNode implementation that takes a raw
 // configuration and interpolates it.
 type EvalInterpolate struct {
-	Config   *config.RawConfig
-	Resource *Resource
-	Output   **ResourceConfig
+	Config        *config.RawConfig
+	Resource      *Resource
+	Output        **ResourceConfig
+	ContinueOnErr bool
 }
 
 func (n *EvalInterpolate) Eval(ctx EvalContext) (interface{}, error) {
 	rc, err := ctx.Interpolate(n.Config, n.Resource)
 	if err != nil {
+		if n.ContinueOnErr {
+			log.Printf("[WARN] Interpolation %q failed: %s", n.Config.Key, err)
+			return nil, EvalEarlyExitError{}
+		}
 		return nil, err
 	}
 
@@ -26,28 +31,3 @@ func (n *EvalInterpolate) Eval(ctx EvalContext) (interface{}, error) {
 
 	return nil, nil
 }
-
-// EvalTryInterpolate is an EvalNode implementation that takes a raw
-// configuration and interpolates it, but only logs a warning on an
-// interpolation error, and stops further Eval steps.
-// This is used during Input where a value may not be known before Refresh, but
-// we don't want to block Input.
-type EvalTryInterpolate struct {
-	Config   *config.RawConfig
-	Resource *Resource
-	Output   **ResourceConfig
-}
-
-func (n *EvalTryInterpolate) Eval(ctx EvalContext) (interface{}, error) {
-	rc, err := ctx.Interpolate(n.Config, n.Resource)
-	if err != nil {
-		log.Printf("[WARN] Interpolation %q failed: %s", n.Config.Key, err)
-		return nil, EvalEarlyExitError{}
-	}
-
-	if n.Output != nil {
-		*n.Output = rc
-	}
-
-	return nil, nil
-}

terraform/eval_output.go

@@ -41,15 +41,16 @@ type EvalWriteOutput struct {
 	Name      string
 	Sensitive bool
 	Value     *config.RawConfig
+	// ContinueOnErr allows interpolation to fail during Input
+	ContinueOnErr bool
 }
 
 // TODO: test
 func (n *EvalWriteOutput) Eval(ctx EvalContext) (interface{}, error) {
+	// This has to run before we have a state lock, since interpolation also
+	// reads the state
 	cfg, err := ctx.Interpolate(n.Value, nil)
-	if err != nil {
-		// Log error but continue anyway
-		log.Printf("[WARN] Output interpolation %q failed: %s", n.Name, err)
-	}
+	// handle the error after we have the module from the state
 
 	state, lock := ctx.State()
 	if state == nil {
@@ -59,13 +60,32 @@ func (n *EvalWriteOutput) Eval(ctx EvalContext) (interface{}, error) {
 	// Get a write lock so we can access this instance
 	lock.Lock()
 	defer lock.Unlock()
-
 	// Look for the module state. If we don't have one, create it.
 	mod := state.ModuleByPath(ctx.Path())
 	if mod == nil {
 		mod = state.AddModule(ctx.Path())
 	}
 
+	// handling the interpolation error
+	if err != nil {
+		switch {
+		case featureOutputErrors:
+			if n.ContinueOnErr {
+				log.Printf("[ERROR] Output interpolation %q failed: %s", n.Name, err)
+				// if we're continueing, make sure the output is included, and
+				// marked as unknown
+				mod.Outputs[n.Name] = &OutputState{
+					Type:  "string",
+					Value: config.UnknownVariableValue,
+				}
+				return nil, EvalEarlyExitError{}
+			}
+			return nil, err
+		default:
+			log.Printf("[WARN] Output interpolation %q failed: %s", n.Name, err)
+		}
+	}
+
 	// Get the value from the config
 	var valueRaw interface{} = config.UnknownVariableValue
 	if cfg != nil {

terraform/features.go

@@ -0,0 +1,9 @@
+package terraform
+
+import (
+	"os"
+)
+
+// This file holds feature flags for the next release
+
+var featureOutputErrors = os.Getenv("TF_OUTPUT_ERRORS") != ""

terraform/graph_builder_apply.go

@@ -120,6 +120,13 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer {
 		// Connect references so ordering is correct
 		&ReferenceTransformer{},
 
+		// Reverse the edges to outputs and locals, so that
+		// interpolations don't fail during destroy.
+		GraphTransformIf(
+			func() bool { return b.Destroy },
+			&DestroyValueReferenceTransformer{},
+		),
+
 		// Add the node to fix the state count boundaries
 		&CountBoundaryTransformer{},
 

terraform/graph_builder_input.go

@@ -10,9 +10,6 @@ import (
 // and is based on the PlanGraphBuilder. The PlanGraphBuilder passed in will be
 // modified and should not be used for any other operations.
 func InputGraphBuilder(p *PlanGraphBuilder) GraphBuilder {
-	// convert this to an InputPlan
-	p.Input = true
-
 	// We're going to customize the concrete functions
 	p.CustomConcrete = true
 

terraform/graph_builder_plan.go

@@ -40,9 +40,6 @@ type PlanGraphBuilder struct {
 	// Validate will do structural validation of the graph.
 	Validate bool
 
-	// Input represents that this builder is for an Input operation.
-	Input bool
-
 	// CustomConcrete can be set to customize the node types created
 	// for various parts of the plan. This is useful in order to customize
 	// the plan behavior.
@@ -115,7 +112,6 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer {
 		// Add module variables
 		&ModuleVariableTransformer{
 			Module: b.Module,
-			Input:  b.Input,
 		},
 
 		// Connect so that the references are ready for targeting. We'll

terraform/node_module_destroy.go

@@ -1,29 +0,0 @@
-package terraform
-
-import (
-	"fmt"
-)
-
-// NodeDestroyableModule represents a module destruction.
-type NodeDestroyableModuleVariable struct {
-	PathValue []string
-}
-
-func (n *NodeDestroyableModuleVariable) Name() string {
-	result := "plan-destroy"
-	if len(n.PathValue) > 1 {
-		result = fmt.Sprintf("%s.%s", modulePrefixStr(n.PathValue), result)
-	}
-
-	return result
-}
-
-// GraphNodeSubPath
-func (n *NodeDestroyableModuleVariable) Path() []string {
-	return n.PathValue
-}
-
-// GraphNodeEvalable
-func (n *NodeDestroyableModuleVariable) EvalTree() EvalNode {
-	return &EvalDiffDestroyModule{Path: n.PathValue}
-}

terraform/node_module_variable.go

@@ -15,9 +15,6 @@ type NodeApplyableModuleVariable struct {
 	Value     *config.RawConfig // Value is the value that is set
 
 	Module *module.Tree // Antiquated, want to remove
-
-	// Input is set if this graph was created for the Input operation.
-	Input bool
 }
 
 func (n *NodeApplyableModuleVariable) Name() string {
@@ -96,23 +93,24 @@ func (n *NodeApplyableModuleVariable) EvalTree() EvalNode {
 	var config *ResourceConfig
 	variables := make(map[string]interface{})
 
-	var interpolate EvalNode
-
-	if n.Input {
-		interpolate = &EvalTryInterpolate{
-			Config: n.Value,
-			Output: &config,
-		}
-	} else {
-		interpolate = &EvalInterpolate{
-			Config: n.Value,
-			Output: &config,
-		}
-	}
-
 	return &EvalSequence{
 		Nodes: []EvalNode{
-			interpolate,
+			&EvalOpFilter{
+				Ops: []walkOperation{walkInput},
+				Node: &EvalInterpolate{
+					Config:        n.Value,
+					Output:        &config,
+					ContinueOnErr: true,
+				},
+			},
+			&EvalOpFilter{
+				Ops: []walkOperation{walkRefresh, walkPlan, walkApply,
+					walkDestroy, walkValidate},
+				Node: &EvalInterpolate{
+					Config: n.Value,
+					Output: &config,
+				},
+			},
 
 			&EvalVariableBlock{
 				Config:         &config,

terraform/node_output.go

@@ -69,17 +69,33 @@ func (n *NodeApplyableOutput) References() []string {
 
 // GraphNodeEvalable
 func (n *NodeApplyableOutput) EvalTree() EvalNode {
-	return &EvalOpFilter{
-		Ops: []walkOperation{walkRefresh, walkPlan, walkApply,
-			walkDestroy, walkInput, walkValidate},
-		Node: &EvalSequence{
-			Nodes: []EvalNode{
-				&EvalWriteOutput{
+	return &EvalSequence{
+		Nodes: []EvalNode{
+			&EvalOpFilter{
+				// Don't let interpolation errors stop Input, since it happens
+				// before Refresh.
+				Ops: []walkOperation{walkInput},
+				Node: &EvalWriteOutput{
+					Name:          n.Config.Name,
+					Sensitive:     n.Config.Sensitive,
+					Value:         n.Config.RawConfig,
+					ContinueOnErr: true,
+				},
+			},
+			&EvalOpFilter{
+				Ops: []walkOperation{walkRefresh, walkPlan, walkApply, walkValidate},
+				Node: &EvalWriteOutput{
 					Name:      n.Config.Name,
 					Sensitive: n.Config.Sensitive,
 					Value:     n.Config.RawConfig,
 				},
 			},
+			&EvalOpFilter{
+				Ops: []walkOperation{walkDestroy, walkPlanDestroy},
+				Node: &EvalDeleteOutput{
+					Name: n.Config.Name,
+				},
+			},
 		},
 	}
 }

terraform/plan.go

@@ -71,6 +71,9 @@ type Plan struct {
 	// Backend is the backend that this plan should use and store data with.
 	Backend *BackendState
 
+	// Destroy indicates that this plan was created for a full destroy operation
+	Destroy bool
+
 	once sync.Once
 }
 
@@ -100,6 +103,7 @@ func (p *Plan) contextOpts(base *ContextOpts) (*ContextOpts, error) {
 	opts.Module = p.Module
 	opts.Targets = p.Targets
 	opts.ProviderSHA256s = p.ProviderSHA256s
+	opts.Destroy = p.Destroy
 
 	if opts.State == nil {
 		opts.State = p.State

terraform/transform_destroy_edge.go

@@ -119,6 +119,9 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 		return &NodeApplyableProvider{NodeAbstractProvider: a}
 	}
 	steps := []GraphTransformer{
+		// Add the local values
+		&LocalTransformer{Module: t.Module},
+
 		// Add outputs and metadata
 		&OutputTransformer{Module: t.Module},
 		&AttachResourceConfigTransformer{Module: t.Module},

terraform/transform_module_variable.go

@@ -17,7 +17,6 @@ type ModuleVariableTransformer struct {
 	Module *module.Tree
 
 	DisablePrune bool // True if pruning unreferenced should be disabled
-	Input        bool // True if this is from an Input operation.
 }
 
 func (t *ModuleVariableTransformer) Transform(g *Graph) error {
@@ -100,7 +99,6 @@ func (t *ModuleVariableTransformer) transformSingle(g *Graph, parent, m *module.
 			Config:    v,
 			Value:     value,
 			Module:    t.Module,
-			Input:     t.Input,
 		}
 
 		if !t.DisablePrune {

terraform/transform_reference.go

@@ -76,6 +76,37 @@ func (t *ReferenceTransformer) Transform(g *Graph) error {
 	return nil
 }
 
+// DestroyReferenceTransformer is a GraphTransformer that reverses the edges
+// for nodes that depend on an Output or Local value. Output and local nodes are
+// removed during destroy, so anything which depends on them must be evaluated
+// first. These can't be interpolated during destroy, so the stored value must
+// be used anyway hence they don't need to be re-evaluated.
+type DestroyValueReferenceTransformer struct{}
+
+func (t *DestroyValueReferenceTransformer) Transform(g *Graph) error {
+	vs := g.Vertices()
+
+	for _, v := range vs {
+		switch v.(type) {
+		case *NodeApplyableOutput, *NodeLocal:
+			// OK
+		default:
+			continue
+		}
+
+		// reverse any incoming edges so that the value is removed last
+		for _, e := range g.EdgesTo(v) {
+			source := e.Source()
+			log.Printf("[TRACE] output dep: %s", dag.VertexName(source))
+
+			g.RemoveEdge(e)
+			g.Connect(&DestroyEdge{S: v, T: source})
+		}
+	}
+
+	return nil
+}
+
 // ReferenceMap is a structure that can be used to efficiently check
 // for references on a graph.
 type ReferenceMap struct {

terraform/transform_targets.go

@@ -73,9 +73,11 @@ func (t *TargetsTransformer) Transform(g *Graph) error {
 			if _, ok := v.(GraphNodeResource); ok {
 				removable = true
 			}
+
 			if vr, ok := v.(RemovableIfNotTargeted); ok {
 				removable = vr.RemoveIfNotTargeted()
 			}
+
 			if removable && !targetedNodes.Include(v) {
 				log.Printf("[DEBUG] Removing %q, filtered by targeting.", dag.VertexName(v))
 				g.Remove(v)
@@ -135,7 +137,10 @@ func (t *TargetsTransformer) selectTargetedNodes(
 			}
 		}
 	}
+	return t.addDependencies(targetedNodes, g)
+}
 
+func (t *TargetsTransformer) addDependencies(targetedNodes *dag.Set, g *Graph) (*dag.Set, error) {
 	// Handle nodes that need to be included if their dependencies are included.
 	// This requires multiple passes since we need to catch transitive
 	// dependencies if and only if they are via other nodes that also
@@ -157,11 +162,6 @@ func (t *TargetsTransformer) selectTargetedNodes(
 			}
 
 			dependers = dependers.Filter(func(dv interface{}) bool {
-				// Can ignore nodes that are already targeted
-				/*if targetedNodes.Include(dv) {
-					return false
-				}*/
-
 				_, ok := dv.(GraphNodeTargetDownstream)
 				return ok
 			})
@@ -180,6 +180,7 @@ func (t *TargetsTransformer) selectTargetedNodes(
 				// depending on in case that informs its decision about whether
 				// it is safe to be targeted.
 				deps := g.DownEdges(v)
+
 				depsTargeted := deps.Intersection(targetedNodes)
 				depsUntargeted := deps.Difference(depsTargeted)
 
@@ -193,7 +194,44 @@ func (t *TargetsTransformer) selectTargetedNodes(
 		}
 	}
 
-	return targetedNodes, nil
+	return targetedNodes.Filter(func(dv interface{}) bool {
+		return filterPartialOutputs(dv, targetedNodes, g)
+	}), nil
+}
+
+// Outputs may have been included transitively, but if any of their
+// dependencies have been pruned they won't be resolvable.
+// If nothing depends on the output, and the output is missing any
+// dependencies, remove it from the graph.
+// This essentially maintains the previous behavior where interpolation in
+// outputs would fail silently, but can now surface errors where the output
+// is required.
+func filterPartialOutputs(v interface{}, targetedNodes *dag.Set, g *Graph) bool {
+	// should this just be done with TargetDownstream?
+	if _, ok := v.(*NodeApplyableOutput); !ok {
+		return true
+	}
+
+	dependers := g.UpEdges(v)
+	for _, d := range dependers.List() {
+		if _, ok := d.(*NodeCountBoundary); ok {
+			continue
+		}
+		// as soon as we see a real dependency, we mark this as
+		// non-removable
+		return true
+	}
+
+	depends := g.DownEdges(v)
+
+	for _, d := range depends.List() {
+		if !targetedNodes.Include(d) {
+			log.Printf("[WARN] %s missing targeted dependency %s, removing from the graph",
+				dag.VertexName(v), dag.VertexName(d))
+			return false
+		}
+	}
+	return true
 }
 
 func (t *TargetsTransformer) nodeIsTarget(