Merge pull request hashicorp#7877 from hashicorp/jbardin/races

core: Fix race conditions, mostly around diffs

Author: jbardin

command/hook_count.go

@@ -47,7 +47,7 @@ func (h *CountHook) PreApply(
 	}
 
 	action := countHookActionChange
-	if d.Destroy {
+	if d.GetDestroy() {
 		action = countHookActionRemove
 	} else if s.ID == "" {
 		action = countHookActionAdd

command/hook_ui.go

@@ -84,8 +84,10 @@ func (h *UiHook) PreApply(
 	// Get all the attributes that are changing, and sort them. Also
 	// determine the longest key so that we can align them all.
 	keyLen := 0
-	keys := make([]string, 0, len(d.Attributes))
-	for key, _ := range d.Attributes {
+
+	dAttrs := d.CopyAttributes()
+	keys := make([]string, 0, len(dAttrs))
+	for key, _ := range dAttrs {
 		// Skip the ID since we do that specially
 		if key == "id" {
 			continue
@@ -100,7 +102,7 @@ func (h *UiHook) PreApply(
 
 	// Go through and output each attribute
 	for _, attrK := range keys {
-		attrDiff := d.Attributes[attrK]
+		attrDiff, _ := d.GetAttribute(attrK)
 
 		v := attrDiff.New
 		u := attrDiff.Old

config/raw_config.go

@@ -93,6 +93,8 @@ func (r *RawConfig) Value() interface{} {
 // structure will always successfully decode into its ultimate
 // structure using something like mapstructure.
 func (r *RawConfig) Config() map[string]interface{} {
+	r.lock.Lock()
+	defer r.lock.Unlock()
 	return r.config
 }
 

helper/resource/wait.go

@@ -20,13 +20,15 @@ func Retry(timeout time.Duration, f RetryFunc) error {
 		MinTimeout: 500 * time.Millisecond,
 		Refresh: func() (interface{}, string, error) {
 			rerr := f()
+
+			resultErrMu.Lock()
+			defer resultErrMu.Unlock()
+
 			if rerr == nil {
 				resultErr = nil
 				return 42, "success", nil
 			}
 
-			resultErrMu.Lock()
-			defer resultErrMu.Unlock()
 			resultErr = rerr.Err
 
 			if rerr.Retryable {

terraform/diff.go

@@ -8,6 +8,7 @@ import (
 	"regexp"
 	"sort"
 	"strings"
+	"sync"
 )
 
 // DiffChangeType is an enum with the kind of changes a diff has planned.
@@ -216,9 +217,9 @@ func (d *ModuleDiff) String() string {
 
 		crud := "UPDATE"
 		switch {
-		case rdiff.RequiresNew() && (rdiff.Destroy || rdiff.DestroyTainted):
+		case rdiff.RequiresNew() && (rdiff.GetDestroy() || rdiff.GetDestroyTainted()):
 			crud = "DESTROY/CREATE"
-		case rdiff.Destroy:
+		case rdiff.GetDestroy():
 			crud = "DESTROY"
 		case rdiff.RequiresNew():
 			crud = "CREATE"
@@ -230,8 +231,9 @@ func (d *ModuleDiff) String() string {
 			name))
 
 		keyLen := 0
-		keys := make([]string, 0, len(rdiff.Attributes))
-		for key, _ := range rdiff.Attributes {
+		rdiffAttrs := rdiff.CopyAttributes()
+		keys := make([]string, 0, len(rdiffAttrs))
+		for key, _ := range rdiffAttrs {
 			if key == "id" {
 				continue
 			}
@@ -244,7 +246,7 @@ func (d *ModuleDiff) String() string {
 		sort.Strings(keys)
 
 		for _, attrK := range keys {
-			attrDiff := rdiff.Attributes[attrK]
+			attrDiff, _ := rdiff.GetAttribute(attrK)
 
 			v := attrDiff.New
 			u := attrDiff.Old
@@ -279,6 +281,7 @@ func (d *ModuleDiff) String() string {
 
 // InstanceDiff is the diff of a resource from some state to another.
 type InstanceDiff struct {
+	mu             sync.Mutex
 	Attributes     map[string]*ResourceAttrDiff
 	Destroy        bool
 	DestroyTainted bool
@@ -324,18 +327,22 @@ func (d *InstanceDiff) init() {
 	}
 }
 
+func NewInstanceDiff() *InstanceDiff {
+	return &InstanceDiff{Attributes: make(map[string]*ResourceAttrDiff)}
+}
+
 // ChangeType returns the DiffChangeType represented by the diff
 // for this single instance.
 func (d *InstanceDiff) ChangeType() DiffChangeType {
 	if d.Empty() {
 		return DiffNone
 	}
 
-	if d.RequiresNew() && (d.Destroy || d.DestroyTainted) {
+	if d.RequiresNew() && (d.GetDestroy() || d.GetDestroyTainted()) {
 		return DiffDestroyCreate
 	}
 
-	if d.Destroy {
+	if d.GetDestroy() {
 		return DiffDestroy
 	}
 
@@ -352,6 +359,8 @@ func (d *InstanceDiff) Empty() bool {
 		return true
 	}
 
+	d.mu.Lock()
+	defer d.mu.Unlock()
 	return !d.Destroy && len(d.Attributes) == 0
 }
 
@@ -366,6 +375,17 @@ func (d *InstanceDiff) RequiresNew() bool {
 		return false
 	}
 
+	d.mu.Lock()
+	defer d.mu.Unlock()
+
+	return d.requiresNew()
+}
+
+func (d *InstanceDiff) requiresNew() bool {
+	if d == nil {
+		return false
+	}
+
 	if d.DestroyTainted {
 		return true
 	}
@@ -379,24 +399,103 @@ func (d *InstanceDiff) RequiresNew() bool {
 	return false
 }
 
+// These methods are properly locked, for use outside other InstanceDiff
+// methods but everywhere else within in the terraform package.
+// TODO refactor the locking scheme
+func (d *InstanceDiff) SetTainted(b bool) {
+	d.mu.Lock()
+	defer d.mu.Unlock()
+
+	d.DestroyTainted = b
+}
+
+func (d *InstanceDiff) GetDestroyTainted() bool {
+	d.mu.Lock()
+	defer d.mu.Unlock()
+
+	return d.DestroyTainted
+}
+
+func (d *InstanceDiff) SetDestroy(b bool) {
+	d.mu.Lock()
+	defer d.mu.Unlock()
+
+	d.Destroy = b
+}
+
+func (d *InstanceDiff) GetDestroy() bool {
+	d.mu.Lock()
+	defer d.mu.Unlock()
+
+	return d.Destroy
+}
+
+func (d *InstanceDiff) SetAttribute(key string, attr *ResourceAttrDiff) {
+	d.mu.Lock()
+	defer d.mu.Unlock()
+
+	d.Attributes[key] = attr
+}
+
+func (d *InstanceDiff) DelAttribute(key string) {
+	d.mu.Lock()
+	defer d.mu.Unlock()
+
+	delete(d.Attributes, key)
+}
+
+func (d *InstanceDiff) GetAttribute(key string) (*ResourceAttrDiff, bool) {
+	d.mu.Lock()
+	defer d.mu.Unlock()
+
+	attr, ok := d.Attributes[key]
+	return attr, ok
+}
+func (d *InstanceDiff) GetAttributesLen() int {
+	d.mu.Lock()
+	defer d.mu.Unlock()
+
+	return len(d.Attributes)
+}
+
+// Safely copies the Attributes map
+func (d *InstanceDiff) CopyAttributes() map[string]*ResourceAttrDiff {
+	d.mu.Lock()
+	defer d.mu.Unlock()
+
+	attrs := make(map[string]*ResourceAttrDiff)
+	for k, v := range d.Attributes {
+		attrs[k] = v
+	}
+
+	return attrs
+}
+
 // Same checks whether or not two InstanceDiff's are the "same". When
 // we say "same", it is not necessarily exactly equal. Instead, it is
 // just checking that the same attributes are changing, a destroy
 // isn't suddenly happening, etc.
 func (d *InstanceDiff) Same(d2 *InstanceDiff) (bool, string) {
-	if d == nil && d2 == nil {
+	// we can safely compare the pointers without a lock
+	switch {
+	case d == nil && d2 == nil:
+		return true, ""
+	case d == nil || d2 == nil:
+		return false, "one nil"
+	case d == d2:
 		return true, ""
-	} else if d == nil || d2 == nil {
-		return false, "both nil"
 	}
 
-	if d.Destroy != d2.Destroy {
+	d.mu.Lock()
+	defer d.mu.Unlock()
+
+	if d.Destroy != d2.GetDestroy() {
 		return false, fmt.Sprintf(
-			"diff: Destroy; old: %t, new: %t", d.Destroy, d2.Destroy)
+			"diff: Destroy; old: %t, new: %t", d.Destroy, d2.GetDestroy())
 	}
-	if d.RequiresNew() != d2.RequiresNew() {
+	if d.requiresNew() != d2.RequiresNew() {
 		return false, fmt.Sprintf(
-			"diff RequiresNew; old: %t, new: %t", d.RequiresNew(), d2.RequiresNew())
+			"diff RequiresNew; old: %t, new: %t", d.requiresNew(), d2.RequiresNew())
 	}
 
 	// Go through the old diff and make sure the new diff has all the
@@ -406,7 +505,7 @@ func (d *InstanceDiff) Same(d2 *InstanceDiff) (bool, string) {
 	for k, _ := range d.Attributes {
 		checkOld[k] = struct{}{}
 	}
-	for k, _ := range d2.Attributes {
+	for k, _ := range d2.CopyAttributes() {
 		checkNew[k] = struct{}{}
 	}
 
@@ -431,7 +530,7 @@ func (d *InstanceDiff) Same(d2 *InstanceDiff) (bool, string) {
 		delete(checkOld, k)
 		delete(checkNew, k)
 
-		_, ok := d2.Attributes[k]
+		_, ok := d2.GetAttribute(k)
 		if !ok {
 			// If there's no new attribute, and the old diff expected the attribute
 			// to be removed, that's just fine.
@@ -483,7 +582,7 @@ func (d *InstanceDiff) Same(d2 *InstanceDiff) (bool, string) {
 			// Similarly, in a RequiresNew scenario, a list that shows up in the plan
 			// diff can disappear from the apply diff, which is calculated from an
 			// empty state.
-			if d.RequiresNew() && (strings.HasSuffix(k, ".#") || strings.HasSuffix(k, ".%")) {
+			if d.requiresNew() && (strings.HasSuffix(k, ".#") || strings.HasSuffix(k, ".%")) {
 				ok = true
 			}
 

terraform/eval_apply.go

@@ -35,9 +35,9 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) {
 	}
 
 	// Remove any output values from the diff
-	for k, ad := range diff.Attributes {
+	for k, ad := range diff.CopyAttributes() {
 		if ad.Type == DiffAttrOutput {
-			delete(diff.Attributes, k)
+			diff.DelAttribute(k)
 		}
 	}
 
@@ -49,7 +49,7 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) {
 
 	// Flag if we're creating a new instance
 	if n.CreateNew != nil {
-		*n.CreateNew = state.ID == "" && !diff.Destroy || diff.RequiresNew()
+		*n.CreateNew = state.ID == "" && !diff.GetDestroy() || diff.RequiresNew()
 	}
 
 	{

terraform/eval_check_prevent_destroy.go

@@ -22,7 +22,7 @@ func (n *EvalCheckPreventDestroy) Eval(ctx EvalContext) (interface{}, error) {
 	diff := *n.Diff
 	preventDestroy := n.Resource.Lifecycle.PreventDestroy
 
-	if diff.Destroy && preventDestroy {
+	if diff.GetDestroy() && preventDestroy {
 		return nil, fmt.Errorf(preventDestroyErrStr, n.Resource.Id())
 	}