From 8feca3b25d9fc6bf30b384c2e760ae5e6b0d8109 Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Tue, 22 Apr 2025 15:28:55 -0400 Subject: [PATCH 1/2] chore: Fix identity TODOs and add documentation for interfaces --- internal/fromproto5/resource_identity.go | 4 +--- internal/fromproto6/resource_identity.go | 4 +--- internal/fwserver/server_createresource.go | 1 - internal/fwserver/server_deleteresource.go | 2 -- .../fwserver/server_importresourcestate.go | 1 - .../fwserver/server_planresourcechange.go | 2 -- internal/fwserver/server_readresource.go | 1 - internal/fwserver/server_updateresource.go | 1 - resource/resource.go | 19 ++++++++++++++++++- 9 files changed, 20 insertions(+), 15 deletions(-) diff --git a/internal/fromproto5/resource_identity.go b/internal/fromproto5/resource_identity.go index c6e1a8014..c78f77866 100644 --- a/internal/fromproto5/resource_identity.go +++ b/internal/fromproto5/resource_identity.go @@ -13,9 +13,7 @@ import ( "github.com/hashicorp/terraform-plugin-go/tfprotov5" ) -// TODO:ResourceIdentity: Should we create a wrapping struct to contain the identity data? To match the protocol (in-case we want to introduce other identity things) -// - Need to think more on this (like what if we want to introduce display-only attributes) -// - If we introduce one, add a test as well. +// ResourceIdentity returns the *tfsdk.ResourceIdentity for a *tfprotov5.ResourceIdentityData and fwschema.Schema. func ResourceIdentity(ctx context.Context, in *tfprotov5.ResourceIdentityData, schema fwschema.Schema) (*tfsdk.ResourceIdentity, diag.Diagnostics) { if in == nil { return nil, nil diff --git a/internal/fromproto6/resource_identity.go b/internal/fromproto6/resource_identity.go index d9fc1b4e5..5dc262332 100644 --- a/internal/fromproto6/resource_identity.go +++ b/internal/fromproto6/resource_identity.go @@ -13,9 +13,7 @@ import ( "github.com/hashicorp/terraform-plugin-go/tfprotov6" ) -// TODO:ResourceIdentity: Should we create a wrapping struct to contain the identity data? To match the protocol (in-case we want to introduce other identity things) -// - Need to think more on this (like what if we want to introduce display-only attributes) -// - If we introduce one, add a test as well. +// ResourceIdentity returns the *tfsdk.ResourceIdentity for a *tfprotov6.ResourceIdentityData and fwschema.Schema. func ResourceIdentity(ctx context.Context, in *tfprotov6.ResourceIdentityData, schema fwschema.Schema) (*tfsdk.ResourceIdentity, diag.Diagnostics) { if in == nil { return nil, nil diff --git a/internal/fwserver/server_createresource.go b/internal/fwserver/server_createresource.go index 0e5b873ac..00a13d68e 100644 --- a/internal/fwserver/server_createresource.go +++ b/internal/fwserver/server_createresource.go @@ -101,7 +101,6 @@ func (s *Server) CreateResource(ctx context.Context, req *CreateResourceRequest, } // If the resource supports identity and there is no planned identity data, pre-populate with a null value. - // TODO:ResourceIdentity: This logic is likely useless since plan should already handle this, probably should remove. if req.PlannedIdentity == nil && req.IdentitySchema != nil { nullIdentityTfValue := tftypes.NewValue(req.IdentitySchema.Type().TerraformType(ctx), nil) diff --git a/internal/fwserver/server_deleteresource.go b/internal/fwserver/server_deleteresource.go index d556badfd..c7d04380b 100644 --- a/internal/fwserver/server_deleteresource.go +++ b/internal/fwserver/server_deleteresource.go @@ -98,8 +98,6 @@ func (s *Server) DeleteResource(ctx context.Context, req *DeleteResourceRequest, resp.Private = req.PlannedPrivate } - // If the resource supports identity pre-populate a null value. - // TODO:ResourceIdentity: This should probably be prior identity, but we don't currently have that in the protocol. if req.IdentitySchema != nil { nullIdentityTfValue := tftypes.NewValue(req.IdentitySchema.Type().TerraformType(ctx), nil) diff --git a/internal/fwserver/server_importresourcestate.go b/internal/fwserver/server_importresourcestate.go index 23961aacf..000a0fad6 100644 --- a/internal/fwserver/server_importresourcestate.go +++ b/internal/fwserver/server_importresourcestate.go @@ -149,7 +149,6 @@ func (s *Server) ImportResourceState(ctx context.Context, req *ImportResourceSta } // If the resource supports identity and we are not importing by identity, pre-populate with a null value. - // TODO:ResourceIdentity: Is there any reason a provider WOULD NOT want to populate an identity when it supports one? if req.Identity == nil && req.IdentitySchema != nil { nullTfValue := tftypes.NewValue(req.IdentitySchema.Type().TerraformType(ctx), nil) diff --git a/internal/fwserver/server_planresourcechange.go b/internal/fwserver/server_planresourcechange.go index 7910bac84..fb6c161d8 100644 --- a/internal/fwserver/server_planresourcechange.go +++ b/internal/fwserver/server_planresourcechange.go @@ -119,8 +119,6 @@ func (s *Server) PlanResourceChange(ctx context.Context, req *PlanResourceChange } // If the resource supports identity and there is no prior identity data, pre-populate with a null value. - // TODO:ResourceIdentity: Is there any reason a provider WOULD NOT want to populate an identity when it supports one? - // TODO:ResourceIdentity: Should this be set to all unknowns? if req.PriorIdentity == nil && req.IdentitySchema != nil { nullIdentityTfValue := tftypes.NewValue(req.IdentitySchema.Type().TerraformType(ctx), nil) diff --git a/internal/fwserver/server_readresource.go b/internal/fwserver/server_readresource.go index 1c56d44c5..557b1a714 100644 --- a/internal/fwserver/server_readresource.go +++ b/internal/fwserver/server_readresource.go @@ -120,7 +120,6 @@ func (s *Server) ReadResource(ctx context.Context, req *ReadResourceRequest, res } // If the resource supports identity and there is no current identity data, pre-populate with a null value. - // TODO:ResourceIdentity: Is there any reason a provider WOULD NOT want to populate an identity when it supports one? if req.CurrentIdentity == nil && req.IdentitySchema != nil { nullTfValue := tftypes.NewValue(req.IdentitySchema.Type().TerraformType(ctx), nil) diff --git a/internal/fwserver/server_updateresource.go b/internal/fwserver/server_updateresource.go index 19827b7e3..041dee187 100644 --- a/internal/fwserver/server_updateresource.go +++ b/internal/fwserver/server_updateresource.go @@ -122,7 +122,6 @@ func (s *Server) UpdateResource(ctx context.Context, req *UpdateResourceRequest, } // If the resource supports identity and there is no planned identity data, pre-populate with a null value. - // TODO:ResourceIdentity: This logic is likely useless since plan should already handle this, probably should remove. if req.PlannedIdentity == nil && req.IdentitySchema != nil { nullIdentityTfValue := tftypes.NewValue(req.IdentitySchema.Type().TerraformType(ctx), nil) diff --git a/resource/resource.go b/resource/resource.go index f99824023..f6813282f 100644 --- a/resource/resource.go +++ b/resource/resource.go @@ -19,6 +19,9 @@ import ( // - Plan Modification: Schema-based or entire plan // via ResourceWithModifyPlan. // - State Upgrades: ResourceWithUpgradeState +// - Identity: Define an identity schema with ResourceWithIdentity to enable +// storing identity data in state during CRUD operations. Identity data is +// used by Terraform to uniquely identify a managed resource. // // Although not required, it is conventional for resources to implement the // ResourceWithImportState interface. @@ -199,7 +202,21 @@ type ResourceWithValidateConfig interface { // ResourceWithIdentity is an interface type that extends Resource to implement managed resource identity. // -// TODO:ResourceIdentity: Add more documentation here to describe what identity is used for. +// Managed resources can optionally define an identity schema, which is a separate object stored in state +// alongside the resource instance data. This identity data is used by Terraform to uniquely identify +// managed resources and has additional restrictions that allow external programs to determine equality +// between two identities. +// +// Resource identity schemas can only contain primitive (bool, string, number) attributes and lists that +// contain primitive elements. Additionally, a resource identity should have the following properties: +// - The resource identity should correspond to at most one remote object per provider, across all +// instances of that provider. +// - Given a resource identity, the provider should be able to determine whether the corresponding remote +// object exists, and if so, return the resource state. Resources that support identity can be imported +// by the identity object via the ResourceWithImportState interface. +// - The resource identity should not change during the lifecycle of the remote object. That is, from the +// creation of the remote object in the remote system until its destruction. An exception to this rule +// is an upgrade of the identity data after a schema change, via the ResourceWithUpgradeIdentity interface. type ResourceWithIdentity interface { Resource From 5cfb471201ad237eface6d6ea5b79b8960457ffb Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Tue, 22 Apr 2025 15:41:08 -0400 Subject: [PATCH 2/2] Update resource/resource.go --- resource/resource.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resource/resource.go b/resource/resource.go index f6813282f..2cf334a77 100644 --- a/resource/resource.go +++ b/resource/resource.go @@ -202,7 +202,7 @@ type ResourceWithValidateConfig interface { // ResourceWithIdentity is an interface type that extends Resource to implement managed resource identity. // -// Managed resources can optionally define an identity schema, which is a separate object stored in state +// Managed resources can optionally define an identity schema, which represents a separate object stored in state // alongside the resource instance data. This identity data is used by Terraform to uniquely identify // managed resources and has additional restrictions that allow external programs to determine equality // between two identities.