Resource Identity: Add identity data to existing RPCs and add two new identity related RPCs" by ansgarm · Pull Request #1444 · hashicorp/terraform-plugin-sdk

ansgarm · 2025-03-14T16:28:20Z

This PR adds initial support for Resource Identity to the SDKv2. It's in an early stage, meaning it's only lightly tested and mostly covering the happy path.

update plugin go
initial generate of RPC + small shim test
wip - resource identity schema
Added initial testing for getresourceidentityschemas
Added initial attempt for GetResourceIdentitySchemas and UpgradeResourceIdentity.
start implementing identity in ReadResource – test doesn't work yet, but getting close
Added more for GetResourceIdentitySchemas and UpgradeResourceIdentity.
Copied CoreIdentitySchema to CoreResourceIdentitySchema and started ConfigIdentitySchemaToProto and coreConfigIdentitySchema to use in GetResourceIdentitySchemas
Continuing addition of functions for Resource Identity
Added TODO for internal validation later
Tests for GetResourceIdentity passing
Initial test written for UpgradeResourceIdentity passing
make ReadResource test pass
use the right identity upgraders
support get, set for identity data
support get, set for identity data in diffs in PlanResourceChange customize diff functions
Added to the internal validation in schema and corresponding tests, need to check if the expected behavior is correct
Added TestUpgradeResourceIdentity_removedAttr and TestUpgradeResourceIdentity_jsonStateBigInt however BigInt doesn't work just yet
safeguard empty identity sent by Terraform
implement identity in ApplyResourceChange
allow using Identity() when there wasn't one yet
fix check for previous state
Added scaffolding for internalIdentityValidate in resource.go to validate resource identities
update terraform-plugin-go to latest main
fix diff possibly being empty
update RawIdentity to RawState as the former was retired
update code comments and remove obsolete todos

austinvalle

Took a first look, good stuff so far!

austinvalle · 2025-03-14T22:11:37Z

+		if v.RequiredForImport {
+
+		} else if v.OptionalForImport {
+
+		} else if !v.Required && !v.Optional && !v.Computed {


It would be nice to validate the identity schema separate from the other schemas, it would make logic like this a little cleaner.

Would this allow you to create invalid resource schemas that use OptionalForImport or RequiredForImport, and therefore have Required, Optional, and Computed as false?

Technically, this internalValidate() should be schema type agnostic. I don't think it's even possible to pass information about the schema type to this method without some major refactoring. If we have resource identity schema-specific validation that might need to be a separate method that's called in (*provider).InternalValidate(). This is where the validation that blocks write-only attributes in non-managed resource schemas is:

terraform-plugin-sdk/helper/schema/provider.go

Lines 235 to 238 in 19e5b30

dataSourceSchema := schemaMap(r.SchemaMap())

if dataSourceSchema.hasWriteOnly() {

validationErrors = append(validationErrors, fmt.Errorf("data source %s cannot contain write-only attributes", k))

}

We'll implement validation in a follow-up PR – I'll leave this discussion open for reference.

SBGoods

Looks pretty good so far! Left a couple of comments

SBGoods · 2025-03-17T19:20:36Z

+		if v.RequiredForImport {
+
+		} else if v.OptionalForImport {
+
+		} else if !v.Required && !v.Optional && !v.Computed {


Technically, this internalValidate() should be schema type agnostic. I don't think it's even possible to pass information about the schema type to this method without some major refactoring. If we have resource identity schema-specific validation that might need to be a separate method that's called in (*provider).InternalValidate(). This is where the validation that blocks write-only attributes in non-managed resource schemas is:

terraform-plugin-sdk/helper/schema/provider.go

Lines 235 to 238 in 19e5b30

dataSourceSchema := schemaMap(r.SchemaMap())

if dataSourceSchema.hasWriteOnly() {

validationErrors = append(validationErrors, fmt.Errorf("data source %s cannot contain write-only attributes", k))

}

…rceIdentity.

…but getting close

…onfigIdentitySchemaToProto and coreConfigIdentitySchema to use in GetResourceIdentitySchemas

…tomize diff functions

…eed to check if the expected behavior is correct

…Identity_jsonStateBigInt however BigInt doesn't work just yet

…date resource identities

ansgarm · 2025-03-19T12:14:32Z

+		if v.WriteOnly && v.Default != nil {
+			return fmt.Errorf("%s: Default cannot be set with WriteOnly", k)
+		}


I think we can keep this small improvement

austinvalle

LGTM 🚀

We should follow up this PR with a bump to the terraform-plugin-go@v0.27.0-alpha.1 and maybe a pre-release changelog similar to the framework one -> https://github.com/hashicorp/terraform-plugin-framework/blob/main/.changes/1.15.0-alpha.1.md

austinvalle · 2025-03-19T12:24:56Z

+	// TODO: is there a more elegant way to do this? this requires us to look for the identity schema block again
+	if req.CurrentIdentity != nil && req.CurrentIdentity.IdentityData != nil {


I'm not 100% sure what you mean by this comment, but in general we'll always need the identity schema to decode identity data from the protocol (any DynamicValue in the protocol needs a schema to decode to avoid losing any type/value information)

Perhaps all of this logic can eventually be extracted to a function that transforms a *tfprotov5.ResourceIdentityData into a flatmap, but not sure we can avoid the usage of the schema.

Oh, that comment is mostly there to indicate that this code is repeated multiple times – so extracting it into a function might make sense, but yeah, schema would need to be passed into it to work.

austinvalle · 2025-03-19T12:27:19Z

+
+		// convert req.CurrentIdentity to flat map identity structure
+		// Step 1: Turn JSON into cty.Value based on schema
+		identityBlock := s.getResourceIdentitySchemaBlock(req.TypeName) // TODO: handle error if no schema exists (and after we add the error)


I agree, we should generally trying to return some nice error if we encounter identity data unexpectedly

austinvalle · 2025-03-19T12:31:57Z

+		// Step 2: Turn cty.Value into flatmap representation
+		identityAttrs := hcl2shim.FlatmapValueFromHCL2(identityVal)
+		// Step 3: Well, set it in the priorState
+		priorState.Identity = identityAttrs


Weird, I didn't know that planned state was also kind of side-car'd with prior state 😅

This might be copy-pasted in error – I don't recall if it was needed

austinvalle · 2025-03-19T12:44:39Z

+	// TODO: if schema defines identity, we should error if there's none written to newInstanceState
+	if res.Identity != nil {


Nothing I think needs immediate action before we merge this PR, just some thoughts

I'm actually not sure where the logic to "delete" the resource (send back a null value) happens in SDKv2, but I think we'll want to make sure we send a null identity object back if we're destroying.

For example, here it is in framework: https://github.com/hashicorp/terraform-plugin-framework/blob/f01eb331c102f536ee3b93c7a6066edbabb815fb/internal/fwserver/server_deleteresource.go#L116-L134

I also don't know if core cares about what we send back, but this might be something we run into if they do. Perhaps a follow-up! (Once we start using terraform-plugin-testing to test identity, we'll find out quick if this is a problem 😆 )

austinvalle · 2025-03-19T12:49:29Z

 				UnsafeToUseLegacyTypeSystem: true,
 			},
 		},
+		"basic-plan-with-identity": {


These are great tests to have here! ❤️

I'd say once we start getting more complex then this, we can start implementing terraform-provider-corner tests, since they'll hit the plan/apply combo

austinvalle · 2025-03-19T13:00:10Z

+			}
+
+			result.Identity = mapWIdentity.Map()
+		} // TODO: else log error?


➕ to eventually returning errors in situations like this. This would indicate that either Terraform has a bug or the SDK has a bug, so we want to explicitly call that out.

…m-plugin-sdk#1444

ansgarm · 2025-03-19T14:26:59Z

ansgarm force-pushed the feat/resource-identity branch from 5a14a0c to 1aaf288 Compare March 14, 2025 17:12

austinvalle reviewed Mar 14, 2025

View reviewed changes

ansgarm force-pushed the feat/resource-identity branch from 37bfb7d to 2fe253a Compare March 17, 2025 11:12

SBGoods reviewed Mar 17, 2025

View reviewed changes

rainkwan added this to the v2.37.0 milestone Mar 19, 2025

rainkwan mentioned this pull request Mar 19, 2025

Resource Identity: Add internal validation #1447

Merged

ansgarm marked this pull request as ready for review March 19, 2025 07:43

ansgarm requested a review from a team as a code owner March 19, 2025 07:43

austinvalle and others added 22 commits March 19, 2025 08:43

update plugin go

af3dae7

initial generate of RPC + small shim test

f9ec400

wip - resource identity schema

2139a0c

Added initial testing for getresourceidentityschemas

ad3356c

Added initial attempt for GetResourceIdentitySchemas and UpgradeResou…

87c482c

…rceIdentity.

start implementing identity in ReadResource – test doesn't work yet, …

a22ffdc

…but getting close

Added more for GetResourceIdentitySchemas and UpgradeResourceIdentity.

cdc62c9

Copied CoreIdentitySchema to CoreResourceIdentitySchema and started C…

a591385

…onfigIdentitySchemaToProto and coreConfigIdentitySchema to use in GetResourceIdentitySchemas

Continuing addition of functions for Resource Identity

52916f1

Added TODO for internal validation later

ff37a55

Tests for GetResourceIdentity passing

d1da135

Initial test written for UpgradeResourceIdentity passing

84cd708

make ReadResource test pass

8600444

use the right identity upgraders

ebe3a78

support get, set for identity data

d90920b

support get, set for identity data in diffs in PlanResourceChange cus…

c744b07

…tomize diff functions

Added to the internal validation in schema and corresponding tests, n…

4e84a3f

…eed to check if the expected behavior is correct

Added TestUpgradeResourceIdentity_removedAttr and TestUpgradeResource…

35aec49

…Identity_jsonStateBigInt however BigInt doesn't work just yet

safeguard empty identity sent by Terraform

6df14c0

implement identity in ApplyResourceChange

c502da9

allow using Identity() when there wasn't one yet

67ebf3c

fix check for previous state

ea629b4

rainkwan and others added 11 commits March 19, 2025 08:43

Added scaffolding for internalIdentityValidate in resource.go to vali…

6029321

…date resource identities

update terraform-plugin-go to latest main

c173450

fix diff possibly being empty

bdaa63f

update RawIdentity to RawState as the former was retired

3318721

update code comments and remove obsolete todos

0b9db4a

address review feedback

b7cac1e

fix lint

79430c2

add copyright headers

c82496f

fix TestUpgradeResourceIdentity_jsonStateBigInt test

0ca4455

update todo comments

c6d10c4

remove obsolete comment

379753f

ansgarm force-pushed the feat/resource-identity branch from 08cbafd to 379753f Compare March 19, 2025 07:43

ansgarm requested review from SBGoods and austinvalle March 19, 2025 08:01

remove validation for identity schemas, will be done in separate PR

fea8e20

ansgarm force-pushed the feat/resource-identity branch from 4dd72ac to fea8e20 Compare March 19, 2025 12:13

ansgarm commented Mar 19, 2025

View reviewed changes

austinvalle approved these changes Mar 19, 2025

View reviewed changes

ansgarm mentioned this pull request Mar 19, 2025

update terraform-plugin-go to v0.27.0-alpha.1 and add changie entry #1449

Merged

ansgarm added a commit to hashicorp/terraform-provider-corner that referenced this pull request Mar 19, 2025

Bump terraform-provider-sdk to latest commit on PR hashicorp/terrafor…

f1933a3

…m-plugin-sdk#1444

rainkwan merged commit f58eb00 into main Mar 19, 2025
21 checks passed

rainkwan deleted the feat/resource-identity branch March 19, 2025 14:24

github-actions Bot locked as resolved and limited conversation to collaborators Apr 18, 2025

	dataSourceSchema := schemaMap(r.SchemaMap())
	if dataSourceSchema.hasWriteOnly() {
	validationErrors = append(validationErrors, fmt.Errorf("data source %s cannot contain write-only attributes", k))
	}

		// TODO: is there a more elegant way to do this? this requires us to look for the identity schema block again
		if req.CurrentIdentity != nil && req.CurrentIdentity.IdentityData != nil {

		// TODO: if schema defines identity, we should error if there's none written to newInstanceState
		if res.Identity != nil {

Conversation

ansgarm commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

austinvalle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SBGoods left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

austinvalle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ansgarm commented Mar 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ansgarm commented Mar 14, 2025 •

edited

Loading