Merge branch 'master' of github.com:hashicorp/terraform

* 'master' of github.com:hashicorp/terraform:
  Docs typo - s/instaces/instances
  Update CHANGELOG.md
  terraform: shadow graph uses GraphWalkerPanicwrap to catch errors
  terraform: GraphWalkerPanicwrap catches panics during graph walks

Author: catsby

CHANGELOG.md

@@ -5,6 +5,7 @@ FEATURES:
  * **New Resource:** `postgresql_extension` [GH-9210]
 
 IMPROVEMENTS:
+ * core: Improve shadow graph robustness by catching panics during graph evaluation. [GH-9852]
  * provider/aws: Provide the option to skip_destroy on aws_volume_attachment [GH-9792]
  * provider/aws: Allows aws_alb security_groups to be updated [GH-9804]
  * provider/aws: Add the enable_sni attribute for Route53 health checks. [GH-9822]

terraform/context.go

@@ -736,11 +736,14 @@ func (c *Context) walk(
 
 	// If we have a shadow graph, wait for that to complete.
 	if shadowCloser != nil {
-		// Build the graph walker for the shadow.
-		shadowWalker := &ContextGraphWalker{
+		// Build the graph walker for the shadow. We also wrap this in
+		// a panicwrap so that panics are captured. For the shadow graph,
+		// we just want panics to be normal errors rather than to crash
+		// Terraform.
+		shadowWalker := GraphWalkerPanicwrap(&ContextGraphWalker{
 			Context:   shadowCtx,
 			Operation: operation,
-		}
+		})
 
 		// Kick off the shadow walk. This will block on any operations
 		// on the real walk so it is fine to start first.

terraform/graph.go

@@ -3,6 +3,7 @@ package terraform
 import (
 	"fmt"
 	"log"
+	"runtime/debug"
 	"strings"
 	"sync"
 
@@ -217,11 +218,40 @@ func (g *Graph) walk(walker GraphWalker) error {
 	// Get the path for logs
 	path := strings.Join(ctx.Path(), ".")
 
+	// Determine if our walker is a panic wrapper
+	panicwrap, ok := walker.(GraphWalkerPanicwrapper)
+	if !ok {
+		panicwrap = nil // just to be sure
+	}
+
 	// Walk the graph.
 	var walkFn dag.WalkFunc
 	walkFn = func(v dag.Vertex) (rerr error) {
 		log.Printf("[DEBUG] vertex '%s.%s': walking", path, dag.VertexName(v))
 
+		// If we have a panic wrap GraphWalker and a panic occurs, recover
+		// and call that. We ensure the return value is an error, however,
+		// so that future nodes are not called.
+		defer func() {
+			// If no panicwrap, do nothing
+			if panicwrap == nil {
+				return
+			}
+
+			// If no panic, do nothing
+			err := recover()
+			if err == nil {
+				return
+			}
+
+			// Modify the return value to show the error
+			rerr = fmt.Errorf("vertex %q captured panic: %s\n\n%s",
+				dag.VertexName(v), err, debug.Stack())
+
+			// Call the panic wrapper
+			panicwrap.Panic(v, err)
+		}()
+
 		walker.EnterVertex(v)
 		defer func() { walker.ExitVertex(v, rerr) }()
 

terraform/graph_test.go

@@ -69,6 +69,29 @@ func TestGraphReplace_DependableWithNonDependable(t *testing.T) {
 	}
 }
 
+func TestGraphWalk_panicWrap(t *testing.T) {
+	var g Graph
+
+	// Add our crasher
+	v := &testGraphSubPath{
+		PathFn: func() []string {
+			panic("yo")
+		},
+	}
+	g.Add(v)
+
+	err := g.Walk(GraphWalkerPanicwrap(new(NullGraphWalker)))
+	if err == nil {
+		t.Fatal("should error")
+	}
+}
+
+type testGraphSubPath struct {
+	PathFn func() []string
+}
+
+func (v *testGraphSubPath) Path() []string { return v.PathFn() }
+
 type testGraphDependable struct {
 	VertexName      string
 	DependentOnMock []string

terraform/graph_walk.go

@@ -15,12 +15,42 @@ type GraphWalker interface {
 	ExitEvalTree(dag.Vertex, interface{}, error) error
 }
 
+// GrpahWalkerPanicwrapper can be optionally implemented to catch panics
+// that occur while walking the graph. This is not generally recommended
+// since panics should crash Terraform and result in a bug report. However,
+// this is particularly useful for situations like the shadow graph where
+// you don't ever want to cause a panic.
+type GraphWalkerPanicwrapper interface {
+	GraphWalker
+
+	// Panic is called when a panic occurs. This will halt the panic from
+	// propogating so if the walker wants it to crash still it should panic
+	// again. This is called from within a defer so runtime/debug.Stack can
+	// be used to get the stack trace of the panic.
+	Panic(dag.Vertex, interface{})
+}
+
+// GraphWalkerPanicwrap wraps an existing Graphwalker to wrap and swallow
+// the panics. This doesn't lose the panics since the panics are still
+// returned as errors as part of a graph walk.
+func GraphWalkerPanicwrap(w GraphWalker) GraphWalkerPanicwrapper {
+	return &graphWalkerPanicwrapper{
+		GraphWalker: w,
+	}
+}
+
+type graphWalkerPanicwrapper struct {
+	GraphWalker
+}
+
+func (graphWalkerPanicwrapper) Panic(dag.Vertex, interface{}) {}
+
 // NullGraphWalker is a GraphWalker implementation that does nothing.
 // This can be embedded within other GraphWalker implementations for easily
 // implementing all the required functions.
 type NullGraphWalker struct{}
 
-func (NullGraphWalker) EnterPath([]string) EvalContext                  { return nil }
+func (NullGraphWalker) EnterPath([]string) EvalContext                  { return new(MockEvalContext) }
 func (NullGraphWalker) ExitPath([]string)                               {}
 func (NullGraphWalker) EnterVertex(dag.Vertex)                          {}
 func (NullGraphWalker) ExitVertex(dag.Vertex, error)                    {}

website/source/docs/providers/aws/r/elb.html.markdown

@@ -14,7 +14,7 @@ Provides an Elastic Load Balancer resource.
 provides both a standalone [ELB Attachment resource](elb_attachment.html)
 (describing an instance attached to an ELB), and an ELB resource with
 `instances` defined in-line. At this time you cannot use an ELB with in-line
-instaces in conjunction with a ELB Attachment resources. Doing so will cause a
+instances in conjunction with a ELB Attachment resources. Doing so will cause a
 conflict and will overwrite attachments.
 ## Example Usage
 

website/source/docs/providers/aws/r/elb_attachment.html.markdown

@@ -14,7 +14,7 @@ Provides an Elastic Load Balancer Attachment resource.
 both a standalone ELB Attachment resource (describing an instance attached to
 an ELB), and an [Elastic Load Balancer resource](elb.html) with
 `instances` defined in-line. At this time you cannot use an ELB with in-line
-instaces in conjunction with an ELB Attachment resource. Doing so will cause a
+instances in conjunction with an ELB Attachment resource. Doing so will cause a
 conflict and will overwrite attachments.
 ## Example Usage