Validate identity to match identity schema (#36904)

* Validate identity to match identity schema

* validate on new create too  and add tests

* validate schema nesting type

* validate required attrs

* remove need for extra fn

---------

Co-authored-by: Samsondeen Dare <[email protected]>

Author: dbanck

internal/terraform/context_apply2_test.go

@@ -2224,6 +2224,7 @@ import {
 						Required: true,
 					},
 				},
+				Nesting: configschema.NestingSingle,
 			},
 		},
 	})

internal/terraform/context_apply_identity_test.go

@@ -22,8 +22,10 @@ func TestContext2Apply_identity(t *testing.T) {
 		prevRunState    *states.State
 		requiresReplace []cty.Path
 		plannedIdentity cty.Value
+		appliedIdentity cty.Value
 
-		expectedIdentity cty.Value
+		expectedIdentity  cty.Value
+		expectDiagnostics tfdiags.Diagnostics
 	}{
 		"create": {
 			plannedIdentity: cty.ObjectVal(map[string]cty.Value{
@@ -33,6 +35,17 @@ func TestContext2Apply_identity(t *testing.T) {
 				"id": cty.StringVal("foo"),
 			}),
 		},
+		"create - invalid applied identity schema": {
+			plannedIdentity: cty.ObjectVal(map[string]cty.Value{
+				"id": cty.StringVal("foo"),
+			}),
+			appliedIdentity: cty.ObjectVal(map[string]cty.Value{
+				"id": cty.BoolVal(false),
+			}),
+			expectDiagnostics: tfdiags.Diagnostics{
+				tfdiags.Sourceless(tfdiags.Error, "Provider produced an identity that doesn't match the schema", "Provider \"registry.terraform.io/hashicorp/test\" returned an identity for test_resource.test that doesn't match the identity schema: .id: string required, but received bool. \n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker."),
+			},
+		},
 
 		"update": {
 			prevRunState: states.BuildState(func(s *states.SyncState) {
@@ -165,10 +178,23 @@ func TestContext2Apply_identity(t *testing.T) {
 				}
 			}
 
+			if !tc.appliedIdentity.IsNull() {
+				p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse {
+					resp := providers.ApplyResourceChangeResponse{}
+					resp.NewState = req.PlannedState
+					resp.NewIdentity = tc.appliedIdentity
+					return resp
+				}
+			}
+
 			plan, diags := ctx.Plan(m, tc.prevRunState, &PlanOpts{Mode: tc.mode})
 			tfdiags.AssertNoDiagnostics(t, diags)
 
 			state, diags := ctx.Apply(plan, m, nil)
+			if tc.expectDiagnostics.HasErrors() {
+				tfdiags.AssertDiagnosticsMatch(t, diags, tc.expectDiagnostics)
+				return
+			}
 			tfdiags.AssertNoDiagnostics(t, diags)
 
 			if !tc.expectedIdentity.IsNull() {

internal/terraform/context_plan_identity_test.go

@@ -437,6 +437,22 @@ func TestContext2Plan_resource_identity_plan(t *testing.T) {
 				"id": cty.StringVal("foo"),
 			}),
 		},
+		"create - invalid planned identity schema": {
+			plannedIdentity: cty.ObjectVal(map[string]cty.Value{
+				"id": cty.BoolVal(false),
+			}),
+			expectDiagnostics: tfdiags.Diagnostics{
+				tfdiags.Sourceless(tfdiags.Error, "Provider produced an identity that doesn't match the schema", "Provider \"registry.terraform.io/hashicorp/test\" returned an identity for test_resource.test that doesn't match the identity schema: .id: string required, but received bool. \n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker."),
+			},
+		},
+		"create - null planned identity schema": {
+			plannedIdentity: cty.ObjectVal(map[string]cty.Value{
+				"id": cty.NullVal(cty.String),
+			}),
+			expectDiagnostics: tfdiags.Diagnostics{
+				tfdiags.Sourceless(tfdiags.Error, "Provider produced an identity that doesn't match the schema", "Provider \"registry.terraform.io/hashicorp/test\" returned an identity for test_resource.test that doesn't match the identity schema: attributes \"id\" are required and must not be null. \n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker."),
+			},
+		},
 		"update": {
 			prevRunState: states.BuildState(func(s *states.SyncState) {
 				s.SetResourceInstanceCurrent(

internal/terraform/node_resource_abstract_instance.go

@@ -396,6 +396,17 @@ func (n *NodeAbstractResourceInstance) planDestroy(ctx EvalContext, currentState
 		return noop, deferred, nil
 	}
 
+	_, providerSchema, err := getProvider(ctx, n.ResolvedProvider)
+	if err != nil {
+		return plan, deferred, diags.Append(err)
+	}
+	schema := providerSchema.SchemaForResourceAddr(n.Addr.Resource.Resource)
+	if schema.Body == nil {
+		// Should be caught during validation, so we don't bother with a pretty error here
+		diags = diags.Append(fmt.Errorf("provider does not support resource type %q", n.Addr.Resource.Resource.Type))
+		return nil, deferred, diags
+	}
+
 	// If we are in a context where we forget instead of destroying, we can
 	// just return the forget change without consulting the provider.
 	if ctx.Forget() {
@@ -453,7 +464,7 @@ func (n *NodeAbstractResourceInstance) planDestroy(ctx EvalContext, currentState
 		if !resp.PlannedIdentity.IsNull() {
 			// Destroying is an operation where we allow identity changes.
 			diags = diags.Append(n.validateIdentityKnown(resp.PlannedIdentity))
-			diags = diags.Append(n.validateIdentity(resp.PlannedIdentity))
+			diags = diags.Append(n.validateIdentity(resp.PlannedIdentity, providerSchema.Provider.Identity))
 		}
 
 		// We may not have a config for all destroys, but we want to reference
@@ -659,7 +670,7 @@ func (n *NodeAbstractResourceInstance) refresh(ctx EvalContext, deposedKey state
 
 		if !resp.Identity.IsNull() {
 			diags = diags.Append(n.validateIdentityKnown(resp.Identity))
-			diags = diags.Append(n.validateIdentity(resp.Identity))
+			diags = diags.Append(n.validateIdentity(resp.Identity, schema.Identity))
 			diags = diags.Append(n.validateIdentityDidNotChange(state, resp.Identity))
 		}
 		if resp.Deferred != nil {
@@ -1117,7 +1128,7 @@ func (n *NodeAbstractResourceInstance) plan(
 			}
 		}
 
-		diags = diags.Append(n.validateIdentity(plannedIdentity))
+		diags = diags.Append(n.validateIdentity(plannedIdentity, schema.Identity))
 	}
 	if diags.HasErrors() {
 		return nil, nil, deferred, keyData, diags
@@ -1179,7 +1190,7 @@ func (n *NodeAbstractResourceInstance) plan(
 
 			if !resp.PlannedIdentity.IsNull() {
 				// On replace the identity is allowed to change and be unknown.
-				diags = diags.Append(n.validateIdentity(resp.PlannedIdentity))
+				diags = diags.Append(n.validateIdentity(resp.PlannedIdentity, schema.Identity))
 			}
 		}
 		// We need to tread carefully here, since if there are any warnings
@@ -2636,7 +2647,7 @@ func (n *NodeAbstractResourceInstance) apply(
 
 		if !resp.NewIdentity.IsNull() {
 			diags = diags.Append(n.validateIdentityKnown(resp.NewIdentity))
-			diags = diags.Append(n.validateIdentity(resp.NewIdentity))
+			diags = diags.Append(n.validateIdentity(resp.NewIdentity, schema.Identity))
 			if !change.Action.IsReplace() {
 				diags = diags.Append(n.validateIdentityDidNotChange(state, resp.NewIdentity))
 			}
@@ -2920,7 +2931,7 @@ func (n *NodeAbstractResourceInstance) validateIdentityDidNotChange(state *state
 	return diags
 }
 
-func (n *NodeAbstractResourceInstance) validateIdentity(newIdentity cty.Value) (diags tfdiags.Diagnostics) {
+func (n *NodeAbstractResourceInstance) validateIdentity(newIdentity cty.Value, identitySchema *configschema.Object) (diags tfdiags.Diagnostics) {
 	if _, marks := newIdentity.UnmarkDeep(); len(marks) > 0 {
 		diags = diags.Append(tfdiags.Sourceless(
 			tfdiags.Error,
@@ -2932,6 +2943,54 @@ func (n *NodeAbstractResourceInstance) validateIdentity(newIdentity cty.Value) (
 		))
 	}
 
+	// The identity schema is always a single object, so we can check the
+	// nesting type here.
+	if identitySchema.Nesting != configschema.NestingSingle {
+		diags = diags.Append(tfdiags.Sourceless(
+			tfdiags.Error,
+			"Provider produced invalid identity",
+			fmt.Sprintf(
+				"Provider %q returned an identity with a nesting type of %s for %s. \n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker.",
+				n.ResolvedProvider.Provider, identitySchema.Nesting, n.Addr,
+			),
+		))
+	}
+
+	newType := newIdentity.Type()
+	currentType := identitySchema.ImpliedType()
+	if errs := newType.TestConformance(currentType); len(errs) > 0 {
+		for _, err := range errs {
+			diags = diags.Append(tfdiags.Sourceless(
+				tfdiags.Error,
+				"Provider produced an identity that doesn't match the schema",
+				fmt.Sprintf(
+					"Provider %q returned an identity for %s that doesn't match the identity schema: %s. \n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker.",
+					n.ResolvedProvider.Provider, n.Addr, tfdiags.FormatError(err),
+				),
+			))
+		}
+		return diags
+	}
+
+	// Check for required attributes
+	names := make([]string, 0, len(identitySchema.Attributes))
+	for name, attrS := range identitySchema.Attributes {
+		if attrS.Required && newIdentity.GetAttr(name).IsNull() {
+			names = append(names, name)
+		}
+	}
+	if len(names) > 0 {
+		diags = diags.Append(tfdiags.Sourceless(
+			tfdiags.Error,
+			"Provider produced an identity that doesn't match the schema",
+			fmt.Sprintf(
+				"Provider %q returned an identity for %s that doesn't match the identity schema: attributes %q are required and must not be null. \n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker.",
+				n.ResolvedProvider.Provider, n.Addr,
+				strings.Join(names, ", "),
+			),
+		))
+	}
+
 	return diags
 }