From 09a71676f444b9426ac86cbe80dbb2785d038373 Mon Sep 17 00:00:00 2001 From: Steph Date: Wed, 27 Aug 2025 15:47:53 +0200 Subject: [PATCH 1/2] add validation to check if a resource identity is null after an apply or a read --- helper/schema/grpc_provider.go | 37 ++++ helper/schema/grpc_provider_test.go | 312 ++++++++++++++++++++++++++++ helper/schema/resource.go | 5 + 3 files changed, 354 insertions(+) diff --git a/helper/schema/grpc_provider.go b/helper/schema/grpc_provider.go index e6334e924e..5f6490c43e 100644 --- a/helper/schema/grpc_provider.go +++ b/helper/schema/grpc_provider.go @@ -880,6 +880,7 @@ func (s *GRPCProviderServer) ReadResource(ctx context.Context, req *tfprotov5.Re resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, err) return resp, nil } + // Step 2: Turn cty.Value into flatmap representation identityAttrs := hcl2shim.FlatmapValueFromHCL2(currentIdentityVal) // Step 3: Well, set it in the instanceState @@ -970,6 +971,24 @@ func (s *GRPCProviderServer) ReadResource(ctx context.Context, req *tfprotov5.Re return resp, nil } + if !res.ResourceBehavior.AllowNullIdentity { + isFullyNull := true + for _, v := range newIdentityVal.AsValueMap() { + if !v.IsNull() { + isFullyNull = false + break + } + } + + if isFullyNull { + resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf( + "Missing Resource Identity After Read: The Terraform provider unexpectedly returned no resource identity after having no errors in the resource read. "+ + "This is always a problem with the provider and should be reported to the provider developer", + )) + return resp, nil + } + } + newIdentityMP, err := msgpack.Marshal(newIdentityVal, identityBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, err) @@ -1544,6 +1563,24 @@ func (s *GRPCProviderServer) ApplyResourceChange(ctx context.Context, req *tfpro return resp, nil } + if !res.ResourceBehavior.AllowNullIdentity { + isFullyNull := true + for _, v := range newIdentityVal.AsValueMap() { + if !v.IsNull() { + isFullyNull = false + break + } + } + + if isFullyNull { + resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf( + "Missing Resource Identity After Create: The Terraform provider unexpectedly returned no resource identity after having no errors in the resource create. "+ + "This is always a problem with the provider and should be reported to the provider developer", + )) + return resp, nil + } + } + if !res.ResourceBehavior.MutableIdentity && !create && !plannedIdentityVal.IsNull() && !plannedIdentityVal.RawEquals(newIdentityVal) { resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf( "Unexpected Identity Change: During the update operation, the Terraform Provider unexpectedly returned a different identity than the previously stored one.\n\n"+ diff --git a/helper/schema/grpc_provider_test.go b/helper/schema/grpc_provider_test.go index ab15715a95..b40837be5b 100644 --- a/helper/schema/grpc_provider_test.go +++ b/helper/schema/grpc_provider_test.go @@ -5707,6 +5707,103 @@ func TestReadResource(t *testing.T) { }, }, }, + "prevent-null-identity": { + server: NewGRPCProviderServer(&Provider{ + ResourcesMap: map[string]*Resource{ + "test": { + SchemaVersion: 1, + Schema: map[string]*Schema{ + "id": { + Type: TypeString, + Required: true, + }, + "test": { + Type: TypeString, + }, + }, + Identity: &ResourceIdentity{ + Version: 1, + SchemaFunc: func() map[string]*Schema { + return map[string]*Schema{ + "subscription_id": { + Type: TypeString, + RequiredForImport: true, + }, + "resource_group_name": { + Type: TypeString, + RequiredForImport: true, + }, + "name": { + Type: TypeString, + RequiredForImport: true, + }, + } + }, + }, + ReadContext: func(ctx context.Context, d *ResourceData, meta interface{}) diag.Diagnostics { + err := d.Set("test", "hello") + if err != nil { + return diag.FromErr(err) + } + + return nil + }, + }, + }, + }), + req: &tfprotov5.ReadResourceRequest{ + TypeName: "test", + CurrentIdentity: &tfprotov5.ResourceIdentityData{ + IdentityData: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "subscription_id": cty.String, + "resource_group_name": cty.String, + "name": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "subscription_id": cty.NullVal(cty.String), + "resource_group_name": cty.NullVal(cty.String), + "name": cty.NullVal(cty.String), + }), + ), + }, + }, + CurrentState: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + "test": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("initial"), + "test": cty.UnknownVal(cty.String), + }), + ), + }, + }, + expected: &tfprotov5.ReadResourceResponse{ + NewState: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + "test": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("initial"), + "test": cty.StringVal("hello"), + }), + ), + }, + Diagnostics: []*tfprotov5.Diagnostic{ + { + Severity: tfprotov5.DiagnosticSeverityError, + Summary: "Missing Resource Identity After Read: The Terraform provider unexpectedly returned no resource identity after having no errors in the resource read. " + + "This is always a problem with the provider and should be reported to the provider developer", + }, + }, + }, + }, "update-resource-identity-may-not-change": { server: NewGRPCProviderServer(&Provider{ ResourcesMap: map[string]*Resource{ @@ -8233,6 +8330,221 @@ func TestApplyResourceChange(t *testing.T) { }, }, }, + "create: null identity not allowed in ApplyResourceChangeResponse": { + server: NewGRPCProviderServer(&Provider{ + ResourcesMap: map[string]*Resource{ + "test": { + SchemaVersion: 4, + CreateContext: func(_ context.Context, rd *ResourceData, _ interface{}) diag.Diagnostics { + rd.SetId("baz") + + return nil + }, + Schema: map[string]*Schema{}, + Identity: &ResourceIdentity{ + Version: 1, + SchemaFunc: func() map[string]*Schema { + return map[string]*Schema{ + "subscription_id": { + Type: TypeString, + RequiredForImport: true, + }, + "resource_group_name": { + Type: TypeString, + RequiredForImport: true, + }, + "name": { + Type: TypeString, + RequiredForImport: true, + }, + } + }, + }, + }, + }, + }), + req: &tfprotov5.ApplyResourceChangeRequest{ + TypeName: "test", + PriorState: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{}), + cty.NullVal( + cty.Object(map[string]cty.Type{}), + ), + ), + }, + PlannedState: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + }), + ), + }, + PlannedIdentity: &tfprotov5.ResourceIdentityData{ + IdentityData: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "subscription_id": cty.String, + "resource_group_name": cty.String, + "name": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "subscription_id": cty.NullVal(cty.String), + "resource_group_name": cty.NullVal(cty.String), + "name": cty.NullVal(cty.String), + }), + ), + }, + }, + Config: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.NullVal(cty.String), + }), + ), + }, + }, + expected: &tfprotov5.ApplyResourceChangeResponse{ + NewState: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("baz"), + }), + ), + }, + Private: []uint8(`{"schema_version":"4"}`), + Diagnostics: []*tfprotov5.Diagnostic{ + { + Severity: tfprotov5.DiagnosticSeverityError, + Summary: "Missing Resource Identity After Create: The Terraform provider unexpectedly returned no resource identity after having no errors in the resource create. " + + "This is always a problem with the provider and should be reported to the provider developer", + }, + }, + }, + }, + "create: null identity allowed in ApplyResourceChangeResponse": { + server: NewGRPCProviderServer(&Provider{ + ResourcesMap: map[string]*Resource{ + "test": { + ResourceBehavior: ResourceBehavior{ + AllowNullIdentity: true, + }, + SchemaVersion: 4, + CreateContext: func(_ context.Context, rd *ResourceData, _ interface{}) diag.Diagnostics { + rd.SetId("baz") + + return nil + }, + Schema: map[string]*Schema{}, + Identity: &ResourceIdentity{ + Version: 1, + SchemaFunc: func() map[string]*Schema { + return map[string]*Schema{ + "subscription_id": { + Type: TypeString, + RequiredForImport: true, + }, + "resource_group_name": { + Type: TypeString, + RequiredForImport: true, + }, + "name": { + Type: TypeString, + RequiredForImport: true, + }, + } + }, + }, + }, + }, + }), + req: &tfprotov5.ApplyResourceChangeRequest{ + TypeName: "test", + PriorState: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{}), + cty.NullVal( + cty.Object(map[string]cty.Type{}), + ), + ), + }, + PlannedState: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + }), + ), + }, + PlannedIdentity: &tfprotov5.ResourceIdentityData{ + IdentityData: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "subscription_id": cty.String, + "resource_group_name": cty.String, + "name": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "subscription_id": cty.NullVal(cty.String), + "resource_group_name": cty.NullVal(cty.String), + "name": cty.NullVal(cty.String), + }), + ), + }, + }, + Config: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.NullVal(cty.String), + }), + ), + }, + }, + expected: &tfprotov5.ApplyResourceChangeResponse{ + NewState: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("baz"), + }), + ), + }, + Private: []uint8(`{"schema_version":"4"}`), + UnsafeToUseLegacyTypeSystem: true, + NewIdentity: &tfprotov5.ResourceIdentityData{ + IdentityData: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "subscription_id": cty.String, + "resource_group_name": cty.String, + "name": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "subscription_id": cty.NullVal(cty.String), + "resource_group_name": cty.NullVal(cty.String), + "name": cty.NullVal(cty.String), + }), + ), + }, + }, + }, + }, "create-resource-identity-may-change": { server: NewGRPCProviderServer(&Provider{ ResourcesMap: map[string]*Resource{ diff --git a/helper/schema/resource.go b/helper/schema/resource.go index 8314043680..7044a3e776 100644 --- a/helper/schema/resource.go +++ b/helper/schema/resource.go @@ -679,6 +679,11 @@ type ResourceBehavior struct { // resource's lifecycle. Setting this flag to true will disable the SDK validation that ensures identity // data doesn't change during RPC calls. MutableIdentity bool + + // AllowNullIdentity toggles whether the managed resource allows identities to be null. Setting this flag to true + // disables the SDK validation that ensures the identity cannot be null at the end of ApplyResourceChange and ReadResource + // RPC calls in certain situations. + AllowNullIdentity bool } // ProviderDeferredBehavior enables provider-defined logic to be executed From 705793308f7333711a281615487f4ac37797418b Mon Sep 17 00:00:00 2001 From: Steph Date: Thu, 28 Aug 2025 11:26:45 +0200 Subject: [PATCH 2/2] review comments --- helper/schema/grpc_provider.go | 65 ++++++++-------- helper/schema/grpc_provider_test.go | 114 ---------------------------- helper/schema/resource.go | 5 -- 3 files changed, 34 insertions(+), 150 deletions(-) diff --git a/helper/schema/grpc_provider.go b/helper/schema/grpc_provider.go index 5f6490c43e..4370443ba1 100644 --- a/helper/schema/grpc_provider.go +++ b/helper/schema/grpc_provider.go @@ -8,6 +8,7 @@ import ( "encoding/json" "fmt" "strconv" + "strings" "sync" "github.com/hashicorp/go-cty/cty" @@ -962,6 +963,22 @@ func (s *GRPCProviderServer) ReadResource(ctx context.Context, req *tfprotov5.Re return resp, nil } + isFullyNull := true + for _, v := range newIdentityVal.AsValueMap() { + if !v.IsNull() { + isFullyNull = false + break + } + } + + if isFullyNull { + resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf( + "Missing Resource Identity After Read: The Terraform provider unexpectedly returned no resource identity after having no errors in the resource read. "+ + "This is always a problem with the provider and should be reported to the provider developer", + )) + return resp, nil + } + // If we're refreshing the resource state (excluding a recently imported resource), validate that the new identity isn't changing if !res.ResourceBehavior.MutableIdentity && !readFollowingImport && !currentIdentityVal.IsNull() && !currentIdentityVal.RawEquals(newIdentityVal) { resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf("Unexpected Identity Change: %s", "During the read operation, the Terraform Provider unexpectedly returned a different identity then the previously stored one.\n\n"+ @@ -971,24 +988,6 @@ func (s *GRPCProviderServer) ReadResource(ctx context.Context, req *tfprotov5.Re return resp, nil } - if !res.ResourceBehavior.AllowNullIdentity { - isFullyNull := true - for _, v := range newIdentityVal.AsValueMap() { - if !v.IsNull() { - isFullyNull = false - break - } - } - - if isFullyNull { - resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf( - "Missing Resource Identity After Read: The Terraform provider unexpectedly returned no resource identity after having no errors in the resource read. "+ - "This is always a problem with the provider and should be reported to the provider developer", - )) - return resp, nil - } - } - newIdentityMP, err := msgpack.Marshal(newIdentityVal, identityBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, err) @@ -1563,22 +1562,26 @@ func (s *GRPCProviderServer) ApplyResourceChange(ctx context.Context, req *tfpro return resp, nil } - if !res.ResourceBehavior.AllowNullIdentity { - isFullyNull := true - for _, v := range newIdentityVal.AsValueMap() { - if !v.IsNull() { - isFullyNull = false - break - } + isFullyNull := true + for _, v := range newIdentityVal.AsValueMap() { + if !v.IsNull() { + isFullyNull = false + break } + } - if isFullyNull { - resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf( - "Missing Resource Identity After Create: The Terraform provider unexpectedly returned no resource identity after having no errors in the resource create. "+ - "This is always a problem with the provider and should be reported to the provider developer", - )) - return resp, nil + if isFullyNull { + op := "Create" + if !create { + op = "Update" } + + resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf( + "Missing Resource Identity After %s: The Terraform provider unexpectedly returned no resource identity after having no errors in the resource %s. "+ + "This is always a problem with the provider and should be reported to the provider developer", op, strings.ToLower(op), + )) + + return resp, nil } if !res.ResourceBehavior.MutableIdentity && !create && !plannedIdentityVal.IsNull() && !plannedIdentityVal.RawEquals(newIdentityVal) { diff --git a/helper/schema/grpc_provider_test.go b/helper/schema/grpc_provider_test.go index b40837be5b..6109a7d430 100644 --- a/helper/schema/grpc_provider_test.go +++ b/helper/schema/grpc_provider_test.go @@ -8431,120 +8431,6 @@ func TestApplyResourceChange(t *testing.T) { }, }, }, - "create: null identity allowed in ApplyResourceChangeResponse": { - server: NewGRPCProviderServer(&Provider{ - ResourcesMap: map[string]*Resource{ - "test": { - ResourceBehavior: ResourceBehavior{ - AllowNullIdentity: true, - }, - SchemaVersion: 4, - CreateContext: func(_ context.Context, rd *ResourceData, _ interface{}) diag.Diagnostics { - rd.SetId("baz") - - return nil - }, - Schema: map[string]*Schema{}, - Identity: &ResourceIdentity{ - Version: 1, - SchemaFunc: func() map[string]*Schema { - return map[string]*Schema{ - "subscription_id": { - Type: TypeString, - RequiredForImport: true, - }, - "resource_group_name": { - Type: TypeString, - RequiredForImport: true, - }, - "name": { - Type: TypeString, - RequiredForImport: true, - }, - } - }, - }, - }, - }, - }), - req: &tfprotov5.ApplyResourceChangeRequest{ - TypeName: "test", - PriorState: &tfprotov5.DynamicValue{ - MsgPack: mustMsgpackMarshal( - cty.Object(map[string]cty.Type{}), - cty.NullVal( - cty.Object(map[string]cty.Type{}), - ), - ), - }, - PlannedState: &tfprotov5.DynamicValue{ - MsgPack: mustMsgpackMarshal( - cty.Object(map[string]cty.Type{ - "id": cty.String, - }), - cty.ObjectVal(map[string]cty.Value{ - "id": cty.UnknownVal(cty.String), - }), - ), - }, - PlannedIdentity: &tfprotov5.ResourceIdentityData{ - IdentityData: &tfprotov5.DynamicValue{ - MsgPack: mustMsgpackMarshal( - cty.Object(map[string]cty.Type{ - "subscription_id": cty.String, - "resource_group_name": cty.String, - "name": cty.String, - }), - cty.ObjectVal(map[string]cty.Value{ - "subscription_id": cty.NullVal(cty.String), - "resource_group_name": cty.NullVal(cty.String), - "name": cty.NullVal(cty.String), - }), - ), - }, - }, - Config: &tfprotov5.DynamicValue{ - MsgPack: mustMsgpackMarshal( - cty.Object(map[string]cty.Type{ - "id": cty.String, - }), - cty.ObjectVal(map[string]cty.Value{ - "id": cty.NullVal(cty.String), - }), - ), - }, - }, - expected: &tfprotov5.ApplyResourceChangeResponse{ - NewState: &tfprotov5.DynamicValue{ - MsgPack: mustMsgpackMarshal( - cty.Object(map[string]cty.Type{ - "id": cty.String, - }), - cty.ObjectVal(map[string]cty.Value{ - "id": cty.StringVal("baz"), - }), - ), - }, - Private: []uint8(`{"schema_version":"4"}`), - UnsafeToUseLegacyTypeSystem: true, - NewIdentity: &tfprotov5.ResourceIdentityData{ - IdentityData: &tfprotov5.DynamicValue{ - MsgPack: mustMsgpackMarshal( - cty.Object(map[string]cty.Type{ - "subscription_id": cty.String, - "resource_group_name": cty.String, - "name": cty.String, - }), - cty.ObjectVal(map[string]cty.Value{ - "subscription_id": cty.NullVal(cty.String), - "resource_group_name": cty.NullVal(cty.String), - "name": cty.NullVal(cty.String), - }), - ), - }, - }, - }, - }, "create-resource-identity-may-change": { server: NewGRPCProviderServer(&Provider{ ResourcesMap: map[string]*Resource{ diff --git a/helper/schema/resource.go b/helper/schema/resource.go index 7044a3e776..8314043680 100644 --- a/helper/schema/resource.go +++ b/helper/schema/resource.go @@ -679,11 +679,6 @@ type ResourceBehavior struct { // resource's lifecycle. Setting this flag to true will disable the SDK validation that ensures identity // data doesn't change during RPC calls. MutableIdentity bool - - // AllowNullIdentity toggles whether the managed resource allows identities to be null. Setting this flag to true - // disables the SDK validation that ensures the identity cannot be null at the end of ApplyResourceChange and ReadResource - // RPC calls in certain situations. - AllowNullIdentity bool } // ProviderDeferredBehavior enables provider-defined logic to be executed