Skip to content

identity: Update change validation to ensure empty identities (all null attributes) are not validated #1229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 22, 2025
Merged

identity: Update change validation to ensure empty identities (all null attributes) are not validated #1229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
02a37e3
add tests first (failing ATM)
austinvalle Sep 22, 2025
5e6c94c
commit the actual fix to pass the failing tests
austinvalle Sep 22, 2025
89dbff0
add changelog
austinvalle Sep 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changes/unreleased/BUG FIXES-20250922-175218.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
kind: BUG FIXES
body: 'all: Prevent identity change validation from raising an error when prior identity is empty (all attributes are null)'
time: 2025-09-22T17:52:18.613034-04:00
custom:
Issue: "1229"
88 changes: 88 additions & 0 deletions internal/fwserver/server_applyresourcechange_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,24 @@ func TestServerApplyResourceChange(t *testing.T) {
},
}

type testMultiIdentitySchemaData struct {
TestAttrA types.String `tfsdk:"test_attr_a"`
TestAttrB types.Int64 `tfsdk:"test_attr_b"`
}

testMultiAttrIdentitySchema := identityschema.Schema{
Attributes: map[string]identityschema.Attribute{
"test_attr_a": identityschema.StringAttribute{
RequiredForImport: true,
},
"test_attr_b": identityschema.Int64Attribute{
OptionalForImport: true,
},
},
}

testMultiAttrIdentityType := testMultiAttrIdentitySchema.Type().TerraformType(context.Background())

testEmptyPlan := &tfsdk.Plan{
Raw: tftypes.NewValue(testSchemaType, nil),
Schema: testSchema,
Expand Down Expand Up @@ -1719,6 +1737,76 @@ func TestServerApplyResourceChange(t *testing.T) {
Private: testEmptyPrivate,
},
},
"update-response-newidentity-empty-plannedidentity": {
server: &fwserver.Server{
Provider: &testprovider.Provider{},
},
request: &fwserver.ApplyResourceChangeRequest{
Config: &tfsdk.Config{
Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{
"test_computed": tftypes.NewValue(tftypes.String, nil),
"test_required": tftypes.NewValue(tftypes.String, "test-new-value"),
}),
Schema: testSchema,
},
PlannedState: &tfsdk.Plan{
Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{
"test_computed": tftypes.NewValue(tftypes.String, "test-plannedstate-value"),
"test_required": tftypes.NewValue(tftypes.String, "test-new-value"),
}),
Schema: testSchema,
},
PriorState: &tfsdk.State{
Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{
"test_computed": tftypes.NewValue(tftypes.String, nil),
"test_required": tftypes.NewValue(tftypes.String, "test-old-value"),
}),
Schema: testSchema,
},
PlannedIdentity: &tfsdk.ResourceIdentity{
Raw: tftypes.NewValue(testMultiAttrIdentityType, map[string]tftypes.Value{
"test_attr_a": tftypes.NewValue(tftypes.String, nil),
"test_attr_b": tftypes.NewValue(tftypes.Number, nil),
}),
Schema: testMultiAttrIdentitySchema,
},
IdentitySchema: testMultiAttrIdentitySchema,
ResourceSchema: testSchema,
Resource: &testprovider.ResourceWithIdentity{
Resource: &testprovider.Resource{
CreateMethod: func(_ context.Context, _ resource.CreateRequest, resp *resource.CreateResponse) {
resp.Diagnostics.AddError("Unexpected Method Call", "Expected: Update, Got: Create")
},
DeleteMethod: func(_ context.Context, _ resource.DeleteRequest, resp *resource.DeleteResponse) {
resp.Diagnostics.AddError("Unexpected Method Call", "Expected: Update, Got: Delete")
},
UpdateMethod: func(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) {
resp.Diagnostics.Append(resp.Identity.Set(ctx, testMultiIdentitySchemaData{
TestAttrA: types.StringValue("new value"),
TestAttrB: types.Int64Value(20),
})...)
},
},
},
},
expectedResponse: &fwserver.ApplyResourceChangeResponse{
NewIdentity: &tfsdk.ResourceIdentity{
Raw: tftypes.NewValue(testMultiAttrIdentityType, map[string]tftypes.Value{
"test_attr_a": tftypes.NewValue(tftypes.String, "new value"),
"test_attr_b": tftypes.NewValue(tftypes.Number, 20),
}),
Schema: testMultiAttrIdentitySchema,
},
NewState: &tfsdk.State{
Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{
"test_computed": tftypes.NewValue(tftypes.String, nil),
"test_required": tftypes.NewValue(tftypes.String, "test-old-value"),
}),
Schema: testSchema,
},
Private: testEmptyPrivate,
},
},
"update-response-newidentity-with-plannedidentity": {
server: &fwserver.Server{
Provider: &testprovider.Provider{},
Expand Down
2 changes: 1 addition & 1 deletion internal/fwserver/server_planresourcechange.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ func (s *Server) PlanResourceChange(ctx context.Context, req *PlanResourceChange
}

// If we're updating or deleting and we already have an identity stored, validate that the planned identity isn't changing
if !req.ResourceBehavior.MutableIdentity && !req.PriorState.Raw.IsNull() && !req.PriorIdentity.Raw.IsNull() && !req.PriorIdentity.Raw.Equal(resp.PlannedIdentity.Raw) {
if !req.ResourceBehavior.MutableIdentity && !req.PriorState.Raw.IsNull() && !req.PriorIdentity.Raw.IsFullyNull() && !req.PriorIdentity.Raw.Equal(resp.PlannedIdentity.Raw) {
resp.Diagnostics.AddError(
"Unexpected Identity Change",
"During the planning operation, the Terraform Provider unexpectedly returned a different identity than the previously stored one.\n\n"+
Expand Down
89 changes: 89 additions & 0 deletions internal/fwserver/server_planresourcechange_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,24 @@ func TestServerPlanResourceChange(t *testing.T) {
},
}

type testMultiIdentitySchemaData struct {
TestAttrA types.String `tfsdk:"test_attr_a"`
TestAttrB types.Int64 `tfsdk:"test_attr_b"`
}

testMultiAttrIdentitySchema := identityschema.Schema{
Attributes: map[string]identityschema.Attribute{
"test_attr_a": identityschema.StringAttribute{
RequiredForImport: true,
},
"test_attr_b": identityschema.Int64Attribute{
OptionalForImport: true,
},
},
}

testMultiAttrIdentityType := testMultiAttrIdentitySchema.Type().TerraformType(context.Background())

testSchemaWriteOnly := schema.Schema{
Attributes: map[string]schema.Attribute{
"test_computed": schema.StringAttribute{
Expand Down Expand Up @@ -6722,6 +6740,77 @@ func TestServerPlanResourceChange(t *testing.T) {
PlannedPrivate: testEmptyPrivate,
},
},
"update-resourcewithmodifyplan-empty-prioridentity-plannedidentity-changed": {
server: &fwserver.Server{
Provider: &testprovider.Provider{},
},
request: &fwserver.PlanResourceChangeRequest{
Config: &tfsdk.Config{
Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{
"test_computed": tftypes.NewValue(tftypes.String, nil),
"test_required": tftypes.NewValue(tftypes.String, "test-new-value"),
}),
Schema: testSchema,
},
ProposedNewState: &tfsdk.Plan{
Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{
"test_computed": tftypes.NewValue(tftypes.String, nil),
"test_required": tftypes.NewValue(tftypes.String, "test-new-value"),
}),
Schema: testSchema,
},
PriorState: &tfsdk.State{
Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{
"test_computed": tftypes.NewValue(tftypes.String, nil),
"test_required": tftypes.NewValue(tftypes.String, "test-old-value"),
}),
Schema: testSchema,
},
PriorIdentity: &tfsdk.ResourceIdentity{
Raw: tftypes.NewValue(testMultiAttrIdentityType, map[string]tftypes.Value{
"test_attr_a": tftypes.NewValue(tftypes.String, nil),
"test_attr_b": tftypes.NewValue(tftypes.Number, nil),
}),
Schema: testMultiAttrIdentitySchema,
},
IdentitySchema: testMultiAttrIdentitySchema,
ResourceSchema: testSchema,
Resource: &testprovider.ResourceWithIdentityAndModifyPlan{
ModifyPlanMethod: func(ctx context.Context, req resource.ModifyPlanRequest, resp *resource.ModifyPlanResponse) {
var data testSchemaData
resp.Diagnostics.Append(req.Plan.Get(ctx, &data)...)
data.TestComputed = types.StringValue("test-plannedstate-value")
resp.Diagnostics.Append(resp.Plan.Set(ctx, &data)...)

var identityData testMultiIdentitySchemaData
resp.Diagnostics.Append(req.Identity.Get(ctx, &identityData)...)
identityData.TestAttrA = types.StringValue("new value")
identityData.TestAttrB = types.Int64Value(20)
resp.Diagnostics.Append(resp.Identity.Set(ctx, &identityData)...)
},
IdentitySchemaMethod: func(ctx context.Context, req resource.IdentitySchemaRequest, resp *resource.IdentitySchemaResponse) {
resp.IdentitySchema = testMultiAttrIdentitySchema
},
},
},
expectedResponse: &fwserver.PlanResourceChangeResponse{
PlannedIdentity: &tfsdk.ResourceIdentity{
Raw: tftypes.NewValue(testMultiAttrIdentityType, map[string]tftypes.Value{
"test_attr_a": tftypes.NewValue(tftypes.String, "new value"),
"test_attr_b": tftypes.NewValue(tftypes.Number, 20),
}),
Schema: testMultiAttrIdentitySchema,
},
PlannedState: &tfsdk.State{
Raw: tftypes.NewValue(testSchemaType, map[string]tftypes.Value{
"test_computed": tftypes.NewValue(tftypes.String, "test-plannedstate-value"),
"test_required": tftypes.NewValue(tftypes.String, "test-new-value"),
}),
Schema: testSchema,
},
PlannedPrivate: testEmptyPrivate,
},
},
"update-resourcewithmodifyplan-invalid-response-plannedidentity-changed": {
server: &fwserver.Server{
Provider: &testprovider.Provider{},
Expand Down
2 changes: 1 addition & 1 deletion internal/fwserver/server_readresource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ func (s *Server) ReadResource(ctx context.Context, req *ReadResourceRequest, res
}

// If we're refreshing the resource state (excluding a recently imported resource), validate that the new identity isn't changing
if !req.ResourceBehavior.MutableIdentity && !readFollowingImport && !req.CurrentIdentity.Raw.IsNull() && !req.CurrentIdentity.Raw.Equal(resp.NewIdentity.Raw) {
if !req.ResourceBehavior.MutableIdentity && !readFollowingImport && !req.CurrentIdentity.Raw.IsFullyNull() && !req.CurrentIdentity.Raw.Equal(resp.NewIdentity.Raw) {
Copy link
Contributor

@rainkwan rainkwan Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FullyNull is an interesting way to say it

resp.Diagnostics.AddError(
"Unexpected Identity Change",
"During the read operation, the Terraform Provider unexpectedly returned a different identity than the previously stored one.\n\n"+
Expand Down
55 changes: 55 additions & 0 deletions internal/fwserver/server_readresource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,19 @@ func TestServerReadResource(t *testing.T) {
},
}

testMultiAttrIdentitySchema := identityschema.Schema{
Attributes: map[string]identityschema.Attribute{
"test_attr_a": identityschema.StringAttribute{
RequiredForImport: true,
},
"test_attr_b": identityschema.Int64Attribute{
OptionalForImport: true,
},
},
}

testMultiAttrIdentityType := testMultiAttrIdentitySchema.Type().TerraformType(context.Background())

testSchemaWriteOnly := schema.Schema{
Attributes: map[string]schema.Attribute{
"test_write_only": schema.StringAttribute{
Expand Down Expand Up @@ -663,6 +676,48 @@ func TestServerReadResource(t *testing.T) {
Private: testEmptyPrivate,
},
},
"response-identity-valid-update-empty-currentidentity": {
server: &fwserver.Server{
Provider: &testprovider.Provider{},
},
request: &fwserver.ReadResourceRequest{
CurrentState: testCurrentState,
CurrentIdentity: &tfsdk.ResourceIdentity{
Raw: tftypes.NewValue(testMultiAttrIdentityType, map[string]tftypes.Value{
"test_attr_a": tftypes.NewValue(tftypes.String, nil),
"test_attr_b": tftypes.NewValue(tftypes.Number, nil),
}),
Schema: testMultiAttrIdentitySchema,
},
IdentitySchema: testMultiAttrIdentitySchema,
Resource: &testprovider.ResourceWithIdentity{
Resource: &testprovider.Resource{
ReadMethod: func(ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse) {
identityData := struct {
TestAttrA types.String `tfsdk:"test_attr_a"`
TestAttrB types.Int64 `tfsdk:"test_attr_b"`
}{
TestAttrA: types.StringValue("new value"),
TestAttrB: types.Int64Value(20),
}

resp.Diagnostics.Append(resp.Identity.Set(ctx, &identityData)...)
},
},
},
},
expectedResponse: &fwserver.ReadResourceResponse{
NewState: testCurrentState,
NewIdentity: &tfsdk.ResourceIdentity{
Raw: tftypes.NewValue(testMultiAttrIdentityType, map[string]tftypes.Value{
"test_attr_a": tftypes.NewValue(tftypes.String, "new value"),
"test_attr_b": tftypes.NewValue(tftypes.Number, 20),
}),
Schema: testMultiAttrIdentitySchema,
},
Private: testEmptyPrivate,
},
},
"response-identity-valid-update-after-import": {
server: &fwserver.Server{
Provider: &testprovider.Provider{},
Expand Down
2 changes: 1 addition & 1 deletion internal/fwserver/server_updateresource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ func (s *Server) UpdateResource(ctx context.Context, req *UpdateResourceRequest,
}

// If we already have an identity stored, validate that the new identity hasn't changing
if !req.ResourceBehavior.MutableIdentity && !req.PlannedIdentity.Raw.IsNull() && !req.PlannedIdentity.Raw.Equal(resp.NewIdentity.Raw) {
if !req.ResourceBehavior.MutableIdentity && !req.PlannedIdentity.Raw.IsFullyNull() && !req.PlannedIdentity.Raw.Equal(resp.NewIdentity.Raw) {
resp.Diagnostics.AddError(
"Unexpected Identity Change",
"During the update operation, the Terraform Provider unexpectedly returned a different identity than the previously stored one.\n\n"+
Expand Down