plans/objchange: Improve precision of AssertObjectCompatible with sets

Previously we were just asserting that the number of elements didn't grow
between planned and actual. We still can't precisely correlate elements in
sets with unknown values, but here we adapt some logic we added earlier
to config/hcl2shim to ensure that we can find a plausible correlation for
each element in each set to at least one element in the other set, and
thus catch more cases where set elements might vanish or appear between
plan and apply, for improved safety.

This will still generate false negatives in some cases where unknown
values are present due to having to assume correlation is intended
wherever it is possible, but we'll catch situations where the actual value
is obviously contrary to what was planned.

plans/objchange/compatible.go

@@ -149,13 +149,19 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu
 				}
 			}
 		case configschema.NestingSet:
-			// We can't do any reasonable matching of set elements since their
-			// content is also their key, and so we have no way to correlate
-			// them. Because of this, we simply verify that we still have the
-			// same number of elements.
 			if !plannedV.IsKnown() || plannedV.IsNull() || actualV.IsNull() {
 				continue
 			}
+
+			setErrs := assertSetValuesCompatible(plannedV, actualV, path, func(plannedEV, actualEV cty.Value) bool {
+				errs := assertObjectCompatible(&blockS.Block, plannedEV, actualEV, append(path, cty.IndexStep{Key: actualEV}))
+				return len(errs) == 0
+			})
+			errs = append(errs, setErrs...)
+
+			// There can be fewer elements in a set after its elements are all
+			// known (values that turn out to be equal will coalesce) but the
+			// number of elements must never get larger.
 			plannedL := plannedV.LengthInt()
 			actualL := actualV.LengthInt()
 			if plannedL < actualL {
@@ -254,6 +260,17 @@ func assertValueCompatible(planned, actual cty.Value, path cty.Path) []error {
 		// to ensure that the number of elements is consistent, along with
 		// the general type-match checks we ran earlier in this function.
 		if planned.IsKnown() && !planned.IsNull() && !actual.IsNull() {
+
+			setErrs := assertSetValuesCompatible(planned, actual, path, func(plannedV, actualV cty.Value) bool {
+				errs := assertValueCompatible(plannedV, actualV, append(path, cty.IndexStep{Key: actualV}))
+				return len(errs) == 0
+			})
+			errs = append(errs, setErrs...)
+
+			// There can be fewer elements in a set after its elements are all
+			// known (values that turn out to be equal will coalesce) but the
+			// number of elements must never get larger.
+
 			plannedL := planned.LengthInt()
 			actualL := actual.LengthInt()
 			if plannedL < actualL {
@@ -290,3 +307,62 @@ func allLeafValuesUnknown(v cty.Value) bool {
 	})
 	return !seenKnownValue
 }
+
+// assertSetValuesCompatible checks that each of the elements in a can
+// be correlated with at least one equivalent element in b and vice-versa,
+// using the given correlation function.
+//
+// This allows the number of elements in the sets to change as long as all
+// elements in both sets can be correlated, making this function safe to use
+// with sets that may contain unknown values as long as the unknown case is
+// addressed in some reasonable way in the callback function.
+//
+// The callback always recieves values from set a as its first argument and
+// values from set b in its second argument, so it is safe to use with
+// non-commutative functions.
+//
+// As with assertValueCompatible, we assume that the target audience of error
+// messages here is a provider developer (via a bug report from a user) and so
+// we intentionally violate our usual rule of keeping cty implementation
+// details out of error messages.
+func assertSetValuesCompatible(planned, actual cty.Value, path cty.Path, f func(aVal, bVal cty.Value) bool) []error {
+	a := planned
+	b := actual
+
+	// Our methodology here is a little tricky, to deal with the fact that
+	// it's impossible to directly correlate two non-equal set elements because
+	// they don't have identities separate from their values.
+	// The approach is to count the number of equivalent elements each element
+	// of a has in b and vice-versa, and then return true only if each element
+	// in both sets has at least one equivalent.
+	as := a.AsValueSlice()
+	bs := b.AsValueSlice()
+	aeqs := make([]bool, len(as))
+	beqs := make([]bool, len(bs))
+	for ai, av := range as {
+		for bi, bv := range bs {
+			if f(av, bv) {
+				aeqs[ai] = true
+				beqs[bi] = true
+			}
+		}
+	}
+
+	var errs []error
+	for i, eq := range aeqs {
+		if !eq {
+			errs = append(errs, path.NewErrorf("planned set element %#v does not correlate with any element in actual", as[i]))
+		}
+	}
+	if len(errs) > 0 {
+		// Exit early since otherwise we're likely to generate duplicate
+		// error messages from the other perspective in the subsequent loop.
+		return errs
+	}
+	for i, eq := range beqs {
+		if !eq {
+			errs = append(errs, path.NewErrorf("actual set element %#v does not correlate with any element in plan", bs[i]))
+		}
+	}
+	return errs
+}

plans/objchange/compatible_test.go

@@ -439,6 +439,7 @@ func TestAssertObjectCompatible(t *testing.T) {
 				}),
 			}),
 			[]string{
+				`.zones: actual set element cty.StringVal("wotsit") does not correlate with any element in plan`,
 				`.zones: length changed from 1 to 2`,
 			},
 		},
@@ -847,6 +848,205 @@ func TestAssertObjectCompatible(t *testing.T) {
 				`.key: block count changed from 0 to 2`,
 			},
 		},
+
+		// NestingSet blocks
+		{
+			&configschema.Block{
+				BlockTypes: map[string]*configschema.NestedBlock{
+					"block": {
+						Nesting: configschema.NestingSet,
+						Block: configschema.Block{
+							Attributes: map[string]*configschema.Attribute{
+								"foo": {
+									Type:     cty.String,
+									Optional: true,
+								},
+							},
+						},
+					},
+				},
+			},
+			cty.ObjectVal(map[string]cty.Value{
+				"block": cty.SetVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.StringVal("hello"),
+					}),
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.StringVal("world"),
+					}),
+				}),
+			}),
+			cty.ObjectVal(map[string]cty.Value{
+				"block": cty.SetVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.StringVal("hello"),
+					}),
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.StringVal("world"),
+					}),
+				}),
+			}),
+			nil,
+		},
+		{
+			&configschema.Block{
+				BlockTypes: map[string]*configschema.NestedBlock{
+					"block": {
+						Nesting: configschema.NestingSet,
+						Block: configschema.Block{
+							Attributes: map[string]*configschema.Attribute{
+								"foo": {
+									Type:     cty.String,
+									Optional: true,
+								},
+							},
+						},
+					},
+				},
+			},
+			cty.ObjectVal(map[string]cty.Value{
+				"block": cty.SetVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.UnknownVal(cty.String),
+					}),
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.UnknownVal(cty.String),
+					}),
+				}),
+			}),
+			cty.ObjectVal(map[string]cty.Value{
+				"block": cty.SetVal([]cty.Value{
+					// This is testing the scenario where the two unknown values
+					// turned out to be equal after we learned their values,
+					// and so they coalesced together into a single element.
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.StringVal("hello"),
+					}),
+				}),
+			}),
+			nil,
+		},
+		{
+			&configschema.Block{
+				BlockTypes: map[string]*configschema.NestedBlock{
+					"block": {
+						Nesting: configschema.NestingSet,
+						Block: configschema.Block{
+							Attributes: map[string]*configschema.Attribute{
+								"foo": {
+									Type:     cty.String,
+									Optional: true,
+								},
+							},
+						},
+					},
+				},
+			},
+			cty.ObjectVal(map[string]cty.Value{
+				"block": cty.SetVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.UnknownVal(cty.String),
+					}),
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.UnknownVal(cty.String),
+					}),
+				}),
+			}),
+			cty.ObjectVal(map[string]cty.Value{
+				"block": cty.SetVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.StringVal("hello"),
+					}),
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.StringVal("world"),
+					}),
+				}),
+			}),
+			nil,
+		},
+		{
+			&configschema.Block{
+				BlockTypes: map[string]*configschema.NestedBlock{
+					"block": {
+						Nesting: configschema.NestingSet,
+						Block: configschema.Block{
+							Attributes: map[string]*configschema.Attribute{
+								"foo": {
+									Type:     cty.String,
+									Optional: true,
+								},
+							},
+						},
+					},
+				},
+			},
+			cty.ObjectVal(map[string]cty.Value{
+				"block": cty.SetVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.UnknownVal(cty.String),
+					}),
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.UnknownVal(cty.String),
+					}),
+				}),
+			}),
+			cty.ObjectVal(map[string]cty.Value{
+				"block": cty.SetVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.StringVal("hello"),
+					}),
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.StringVal("world"),
+					}),
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.StringVal("nope"),
+					}),
+				}),
+			}),
+			[]string{
+				`.block: block set length changed from 2 to 3`,
+			},
+		},
+		{
+			&configschema.Block{
+				BlockTypes: map[string]*configschema.NestedBlock{
+					"block": {
+						Nesting: configschema.NestingSet,
+						Block: configschema.Block{
+							Attributes: map[string]*configschema.Attribute{
+								"foo": {
+									Type:     cty.String,
+									Optional: true,
+								},
+							},
+						},
+					},
+				},
+			},
+			cty.ObjectVal(map[string]cty.Value{
+				"block": cty.SetVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.StringVal("hello"),
+					}),
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.StringVal("world"),
+					}),
+				}),
+			}),
+			cty.ObjectVal(map[string]cty.Value{
+				"block": cty.SetVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.StringVal("howdy"),
+					}),
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.StringVal("world"),
+					}),
+				}),
+			}),
+			[]string{
+				`.block: planned set element cty.Value{ty: cty.Object(map[string]cty.Type{"foo":cty.String}), v: map[string]interface {}{"foo":"hello"}} does not correlate with any element in actual`,
+			},
+		},
 	}
 
 	for _, test := range tests {