Disallow null resource identities #1193
Conversation
| @@ -198,6 +198,17 @@ func (s *Server) ReadResource(ctx context.Context, req *ReadResourceRequest, res | |||
| } | |||
| } | |||
|
|
|||
| if req.IdentitySchema != nil { | |||
| if resp.NewIdentity.Raw.IsFullyNull() { | |||
There was a problem hiding this comment.
💭 Thinking about whether or not we need to make an exception for readFollowingImport here. I think we still expect the provider to return a proper resource identity in order for the resource to be valid 🧐.
There was a problem hiding this comment.
I think you're right. To repeat it back, I'd say what we're enforcing here is that if the provider indicates it supports identity (by providing a schema), it must be able to return said identity from a read, even if it was recently imported. Which sounds correct to me
readFollowingImport was introduced specifically to avoid validating the mutation of an identity due to the Terraform import methods using ReadResource with a stub state/identity. After the read is complete, the entire identity should be present or a diagnostic should be returned 👍🏻 . The same reason why we don't need an "updateFollowingImport", since the identity should be fully populated prior to that
There was a problem hiding this comment.
Code LGTM, as you mentioned, should probably point to the GH ref of terraform-plugin-go after that is merged.
Also looks like one of the corner resource's needs to be updated to actually set identity, a good catch! 👍🏻 (Also looks like there is an extra log statement in there 😆 )
| @@ -198,6 +198,17 @@ func (s *Server) ReadResource(ctx context.Context, req *ReadResourceRequest, res | |||
| } | |||
| } | |||
|
|
|||
| if req.IdentitySchema != nil { | |||
| if resp.NewIdentity.Raw.IsFullyNull() { | |||
There was a problem hiding this comment.
I think you're right. To repeat it back, I'd say what we're enforcing here is that if the provider indicates it supports identity (by providing a schema), it must be able to return said identity from a read, even if it was recently imported. Which sounds correct to me
readFollowingImport was introduced specifically to avoid validating the mutation of an identity due to the Terraform import methods using ReadResource with a stub state/identity. After the read is complete, the entire identity should be present or a diagnostic should be returned 👍🏻 . The same reason why we don't need an "updateFollowingImport", since the identity should be fully populated prior to that
Related Issue
Fixes #1184. Depends on hashicorp/terraform-plugin-go#541.
Description
This pull request updates the
ReadResourceandApplyResourceChangeRPCs to return an error diagnostic on the Create, Read, and Update paths when a null resource identity would be returned for a resource that has an identity schema.Rollback Plan
Changes to Security Controls
None.