From 0b2cc6298b8adbdaf698c8ce764a476c08bd3061 Mon Sep 17 00:00:00 2001
From: Martin Atkins <mart@degeneration.co.uk>
Date: Thu, 14 Feb 2019 09:36:20 -0800
Subject: [PATCH] plans/objchange: Fix panic in AssertObjectCompatible with set
 blocks

Our usual "ground rules" for mapping configschema to cty call for the
collection values representing nested block types to always be known and
non-null, using an empty collection to represent the absense of any blocks
of that type so that users can always safely use length(...) etc on them
without worrying about them sometimes being null.

However, due to some different behaviors in the legacy SDK we've allowed
it an exception to this rule which means that we can see unknown and null
collections in these positions in object values returned from provider
operations like PlanResourceChange and ApplyResourceChange when the legacy
SDK opt-out is activated.

As a consequence of this, we need to be mindful in our safety check
functions, like AssertObjectCompatible here, of tolerating these non-ideal
situations to allow the safety checks to complete. We run these checks
even when the provider requests an opt-out, because we want to note any
inconsistencies as WARNING level log lines to aid in debugging.
---
 plans/objchange/compatible.go      |  2 +-
 plans/objchange/compatible_test.go | 37 ++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/plans/objchange/compatible.go b/plans/objchange/compatible.go
index f19e72d77..fa1fd0328 100644
--- a/plans/objchange/compatible.go
+++ b/plans/objchange/compatible.go
@@ -157,7 +157,7 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu
 				}
 			}
 		case configschema.NestingSet:
-			if !plannedV.IsKnown() || plannedV.IsNull() || actualV.IsNull() {
+			if !plannedV.IsKnown() || !actualV.IsKnown() || plannedV.IsNull() || actualV.IsNull() {
 				continue
 			}
 
diff --git a/plans/objchange/compatible_test.go b/plans/objchange/compatible_test.go
index 01050912c..ddd8a2850 100644
--- a/plans/objchange/compatible_test.go
+++ b/plans/objchange/compatible_test.go
@@ -1073,6 +1073,43 @@ func TestAssertObjectCompatible(t *testing.T) {
 				`.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`,
 			},
 		},
+		{
+			// This one is an odd situation where the value representing the
+			// block itself is unknown. This is never supposed to be true,
+			// but in legacy SDK mode we allow such things to pass through as
+			// a warning, and so we must tolerate them for matching purposes.
+			&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.UnknownVal(cty.Set(cty.Object(map[string]cty.Type{
+					"foo": cty.String,
+				}))),
+			}),
+			nil,
+		},
 	}
 
 	for _, test := range tests {