Merge pull request #20553 from hashicorp/jbardin/invalid-timeouts-in-plan

Prevent unexpected timeout blocks from being planned

helper/plugin/grpc_provider.go

@@ -1125,22 +1125,34 @@ func normalizeFlatmapContainers(prior map[string]string, attrs map[string]string
 // the Private/Meta fields. we need to copy those values into the planned state
 // so that core doesn't see a perpetual diff with the timeout block.
 func copyTimeoutValues(to cty.Value, from cty.Value) cty.Value {
-	// if `from` is null, then there are no attributes, and if `to` is null we
+	// if `to` is null we are planning to remove it altogether.
-	// are planning to remove it altogether.
+	if to.IsNull() {
-	if from.IsNull() || to.IsNull() {
 		return to
 	}
+	toAttrs := to.AsValueMap()
+	// We need to remove the key since the hcl2shims will add a non-null block
+	// because we can't determine if a single block was null from the flatmapped
+	// values. This needs to conform to the correct schema for marshaling, so
+	// change the value to null rather than deleting it from the object map.
+	timeouts, ok := toAttrs[schema.TimeoutsConfigKey]
+	if ok {
+		toAttrs[schema.TimeoutsConfigKey] = cty.NullVal(timeouts.Type())
+	}
+
+	// if from is null then there are no timeouts to copy
+	if from.IsNull() {
+		return cty.ObjectVal(toAttrs)
+	}
 
 	fromAttrs := from.AsValueMap()
-	timeouts, ok := fromAttrs[schema.TimeoutsConfigKey]
+	timeouts, ok = fromAttrs[schema.TimeoutsConfigKey]
 
-	// no timeouts to copy
+	// timeouts shouldn't be unknown, but don't copy possibly invalid values either
-	// timeouts shouldn't be unknown, but don't copy possibly invalid values
 	if !ok || timeouts.IsNull() || !timeouts.IsWhollyKnown() {
-		return to
+		// no timeouts block to copy
+		return cty.ObjectVal(toAttrs)
 	}
 
-	toAttrs := to.AsValueMap()
 	toAttrs[schema.TimeoutsConfigKey] = timeouts
 
 	return cty.ObjectVal(toAttrs)

plans/objchange/objchange.go

@@ -25,6 +25,13 @@ import (
 // produce strange results with more "extreme" cases, such as a nested set
 // block where _all_ attributes are computed.
 func ProposedNewObject(schema *configschema.Block, prior, config cty.Value) cty.Value {
+	// If the config and prior are both null, return early here before
+	// populating the prior block. The prevents non-null blocks from appearing
+	// the proposed state value.
+	if config.IsNull() && prior.IsNull() {
+		return prior
+	}
+
 	if prior.IsNull() {
 		// In this case, we will construct a synthetic prior value that is
 		// similar to the result of decoding an empty configuration block,

plans/objchange/objchange_test.go

@@ -83,6 +83,45 @@ func TestProposedNewObject(t *testing.T) {
 				}),
 			}),
 		},
+		"null block remains null": {
+			&configschema.Block{
+				Attributes: map[string]*configschema.Attribute{
+					"foo": {
+						Type:     cty.String,
+						Optional: true,
+					},
+				},
+				BlockTypes: map[string]*configschema.NestedBlock{
+					"baz": {
+						Nesting: configschema.NestingSingle,
+						Block: configschema.Block{
+							Attributes: map[string]*configschema.Attribute{
+								"boz": {
+									Type:     cty.String,
+									Optional: true,
+									Computed: true,
+								},
+							},
+						},
+					},
+				},
+			},
+			cty.NullVal(cty.DynamicPseudoType),
+			cty.ObjectVal(map[string]cty.Value{
+				"foo": cty.StringVal("bar"),
+				"baz": cty.NullVal(cty.Object(map[string]cty.Type{
+					"boz": cty.String,
+				})),
+			}),
+			// The baz block does not exist in the config, and therefore
+			// shouldn't be planned.
+			cty.ObjectVal(map[string]cty.Value{
+				"foo": cty.StringVal("bar"),
+				"baz": cty.NullVal(cty.Object(map[string]cty.Type{
+					"boz": cty.String,
+				})),
+			}),
+		},
 		"no prior with set": {
 			// This one is here because our handling of sets is more complex
 			// than others (due to the fuzzy correlation heuristic) and