Skip to content

Commit

Permalink
helper/schema: Add Resource type flags for disabling legacy type sy…
Browse files Browse the repository at this point in the history
…stem protocol flags

Reference: #1227

Introduces new `Resource` type `EnableLegacyTypeSystemApplyErrors` and `EnableLegacyTypeSystemPlanErrors` fields that prevent the `PlanResourceChange` and `ApplyResourceChange` protocol operation responses from setting the `UnsafeLegacyTypeSystem` flag. When Terraform encounters the `UnsafeLegacyTypeSystem` flag, it will demote certain data consistency issues from error diagnostics that would immediately prevent Terraform workflows from completing to warning logs, which can be difficult for provider developers to discover. Provider developers are not  generally expected to try enabling these settings unless discovering these data consistency issues upfront is desired before migrating resources to terraform-plugin-framework.

This setting is on the individual resource level because these errors can be quite prevalent in existing terraform-plugin-sdk resources and therefore it would be problematic to not give provider developers more control over the behavior.

The settings are split between plan and apply because the protocol flag is on two separate protocol operations and because certain errors for one operation may be unavoidable, so it leaves provider developers the option to still raise errors for the other operation.

This change set includes new website documentation to discuss these data consistency issues and how to potentially resolve them more in-depth. It is expected that the framework migration guide will be updated to recommend enabling these settings before migrating to help provider developers understand the issues before migrating. The error resolution section has one particular error to start and it is expected that this section will grow over time as provider developers report the various data consistency errors that Terraform can raise.
  • Loading branch information
bflad committed Aug 8, 2023
1 parent bb15948 commit 2bbfe8e
Show file tree
Hide file tree
Showing 9 changed files with 345 additions and 84 deletions.
8 changes: 8 additions & 0 deletions .changes/unreleased/ENHANCEMENTS-20230808-111940.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
kind: ENHANCEMENTS
body: 'helper/schema: Added `Resource` type `EnableLegacyTypeSystemApplyErrors`
field, which will prevent Terraform from demoting data consistency errors
to warning logs during `ApplyResourceChange` (`Create`, `Update`, and `Delete`)
operations with the resource'
time: 2023-08-08T11:19:40.137598-04:00
custom:
Issue: "1227"
7 changes: 7 additions & 0 deletions .changes/unreleased/ENHANCEMENTS-20230808-111941.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
kind: ENHANCEMENTS
body: 'helper/schema: Added `Resource` type `EnableLegacyTypeSystemPlanErrors`
field, which can be used to prevent Terraform from demoting data consistency
errors to warning logs during `PlanResourceChange` operations with the resource'
time: 2023-08-08T11:19:41.137598-04:00
custom:
Issue: "1227"
12 changes: 12 additions & 0 deletions .changes/unreleased/NOTES-20230808-103724.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
kind: NOTES
body: 'helper/schema: The `Resource` type `EnableApplyLegacyTypeSystemErrors`
and `EnablePlanLegacyTypeSystemErrors` fields can be enabled to more easily
discover resource data consistency errors which Terraform would normally
demote to warning logs. Before enabling the flag in a production release for
a resource, the resource should be exhaustively acceptance tested as there may
be unrecoverable error situations for practitioners. It is recommended to
first enable and test in environments where it is easy to clean up resources,
potentially outside of Terraform.'
time: 2023-08-08T10:37:24.017324-04:00
custom:
Issue: "1227"
22 changes: 14 additions & 8 deletions helper/schema/grpc_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -662,20 +662,23 @@ func (s *GRPCProviderServer) PlanResourceChange(ctx context.Context, req *tfprot
ctx = logging.InitContext(ctx)
resp := &tfprotov5.PlanResourceChangeResponse{}

res, ok := s.provider.ResourcesMap[req.TypeName]
if !ok {
resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf("unknown resource type: %s", req.TypeName))
return resp, nil
}
schemaBlock := s.getResourceSchemaBlock(req.TypeName)

// This is a signal to Terraform Core that we're doing the best we can to
// shim the legacy type system of the SDK onto the Terraform type system
// but we need it to cut us some slack. This setting should not be taken
// forward to any new SDK implementations, since setting it prevents us
// from catching certain classes of provider bug that can lead to
// confusing downstream errors.
resp.UnsafeToUseLegacyTypeSystem = true //nolint:staticcheck

res, ok := s.provider.ResourcesMap[req.TypeName]
if !ok {
resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf("unknown resource type: %s", req.TypeName))
return resp, nil
if !res.EnableLegacyTypeSystemPlanErrors {
//nolint:staticcheck // explicitly for this SDK
resp.UnsafeToUseLegacyTypeSystem = true
}
schemaBlock := s.getResourceSchemaBlock(req.TypeName)

priorStateVal, err := msgpack.Unmarshal(req.PriorState.MsgPack, schemaBlock.ImpliedType())
if err != nil {
Expand Down Expand Up @@ -1075,7 +1078,10 @@ func (s *GRPCProviderServer) ApplyResourceChange(ctx context.Context, req *tfpro
// forward to any new SDK implementations, since setting it prevents us
// from catching certain classes of provider bug that can lead to
// confusing downstream errors.
resp.UnsafeToUseLegacyTypeSystem = true //nolint:staticcheck
if !res.EnableLegacyTypeSystemApplyErrors {
//nolint:staticcheck // explicitly for this SDK
resp.UnsafeToUseLegacyTypeSystem = true
}

return resp, nil
}
Expand Down
215 changes: 139 additions & 76 deletions helper/schema/grpc_provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -575,77 +575,113 @@ func TestUpgradeState_flatmapStateMissingMigrateState(t *testing.T) {
}

func TestPlanResourceChange(t *testing.T) {
r := &Resource{
SchemaVersion: 4,
Schema: map[string]*Schema{
"foo": {
Type: TypeInt,
Optional: true,
t.Parallel()

testCases := map[string]struct {
TestResource *Resource
ExpectedUnsafeLegacyTypeSystem bool
}{
"basic": {
TestResource: &Resource{
SchemaVersion: 4,
Schema: map[string]*Schema{
"foo": {
Type: TypeInt,
Optional: true,
},
},
},
ExpectedUnsafeLegacyTypeSystem: true,
},
"EnableLegacyTypeSystemPlanErrors": {
TestResource: &Resource{
EnableLegacyTypeSystemPlanErrors: true,
Schema: map[string]*Schema{
"foo": {
Type: TypeInt,
Optional: true,
},
},
},
ExpectedUnsafeLegacyTypeSystem: false,
},
}

server := NewGRPCProviderServer(&Provider{
ResourcesMap: map[string]*Resource{
"test": r,
},
})
for name, testCase := range testCases {
name, testCase := name, testCase

schema := r.CoreConfigSchema()
priorState, err := msgpack.Marshal(cty.NullVal(schema.ImpliedType()), schema.ImpliedType())
if err != nil {
t.Fatal(err)
}
t.Run(name, func(t *testing.T) {
t.Parallel()

// A propsed state with only the ID unknown will produce a nil diff, and
// should return the propsed state value.
proposedVal, err := schema.CoerceValue(cty.ObjectVal(map[string]cty.Value{
"id": cty.UnknownVal(cty.String),
}))
if err != nil {
t.Fatal(err)
}
proposedState, err := msgpack.Marshal(proposedVal, schema.ImpliedType())
if err != nil {
t.Fatal(err)
}
server := NewGRPCProviderServer(&Provider{
ResourcesMap: map[string]*Resource{
"test": testCase.TestResource,
},
})

config, err := schema.CoerceValue(cty.ObjectVal(map[string]cty.Value{
"id": cty.NullVal(cty.String),
}))
if err != nil {
t.Fatal(err)
}
configBytes, err := msgpack.Marshal(config, schema.ImpliedType())
if err != nil {
t.Fatal(err)
}
schema := testCase.TestResource.CoreConfigSchema()
priorState, err := msgpack.Marshal(cty.NullVal(schema.ImpliedType()), schema.ImpliedType())
if err != nil {
t.Fatal(err)
}

testReq := &tfprotov5.PlanResourceChangeRequest{
TypeName: "test",
PriorState: &tfprotov5.DynamicValue{
MsgPack: priorState,
},
ProposedNewState: &tfprotov5.DynamicValue{
MsgPack: proposedState,
},
Config: &tfprotov5.DynamicValue{
MsgPack: configBytes,
},
}
// A propsed state with only the ID unknown will produce a nil diff, and
// should return the propsed state value.
proposedVal, err := schema.CoerceValue(cty.ObjectVal(map[string]cty.Value{
"id": cty.UnknownVal(cty.String),
}))
if err != nil {
t.Fatal(err)
}
proposedState, err := msgpack.Marshal(proposedVal, schema.ImpliedType())
if err != nil {
t.Fatal(err)
}

resp, err := server.PlanResourceChange(context.Background(), testReq)
if err != nil {
t.Fatal(err)
}
config, err := schema.CoerceValue(cty.ObjectVal(map[string]cty.Value{
"id": cty.NullVal(cty.String),
}))
if err != nil {
t.Fatal(err)
}
configBytes, err := msgpack.Marshal(config, schema.ImpliedType())
if err != nil {
t.Fatal(err)
}

plannedStateVal, err := msgpack.Unmarshal(resp.PlannedState.MsgPack, schema.ImpliedType())
if err != nil {
t.Fatal(err)
}
testReq := &tfprotov5.PlanResourceChangeRequest{
TypeName: "test",
PriorState: &tfprotov5.DynamicValue{
MsgPack: priorState,
},
ProposedNewState: &tfprotov5.DynamicValue{
MsgPack: proposedState,
},
Config: &tfprotov5.DynamicValue{
MsgPack: configBytes,
},
}

if !cmp.Equal(proposedVal, plannedStateVal, valueComparer) {
t.Fatal(cmp.Diff(proposedVal, plannedStateVal, valueComparer))
resp, err := server.PlanResourceChange(context.Background(), testReq)
if err != nil {
t.Fatal(err)
}

plannedStateVal, err := msgpack.Unmarshal(resp.PlannedState.MsgPack, schema.ImpliedType())
if err != nil {
t.Fatal(err)
}

if !cmp.Equal(proposedVal, plannedStateVal, valueComparer) {
t.Fatal(cmp.Diff(proposedVal, plannedStateVal, valueComparer))
}

//nolint:staticcheck // explicitly for this SDK
if testCase.ExpectedUnsafeLegacyTypeSystem != resp.UnsafeToUseLegacyTypeSystem {
//nolint:staticcheck // explicitly for this SDK
t.Fatalf("expected UnsafeLegacyTypeSystem %t, got: %t", testCase.ExpectedUnsafeLegacyTypeSystem, resp.UnsafeToUseLegacyTypeSystem)
}
})
}
}

Expand Down Expand Up @@ -730,12 +766,13 @@ func TestPlanResourceChange_bigint(t *testing.T) {
}

func TestApplyResourceChange(t *testing.T) {
testCases := []struct {
Description string
TestResource *Resource
t.Parallel()

testCases := map[string]struct {
TestResource *Resource
ExpectedUnsafeLegacyTypeSystem bool
}{
{
Description: "Create",
"Create": {
TestResource: &Resource{
SchemaVersion: 4,
Schema: map[string]*Schema{
Expand All @@ -749,9 +786,9 @@ func TestApplyResourceChange(t *testing.T) {
return nil
},
},
ExpectedUnsafeLegacyTypeSystem: true,
},
{
Description: "CreateContext",
"CreateContext": {
TestResource: &Resource{
SchemaVersion: 4,
Schema: map[string]*Schema{
Expand All @@ -765,9 +802,9 @@ func TestApplyResourceChange(t *testing.T) {
return nil
},
},
ExpectedUnsafeLegacyTypeSystem: true,
},
{
Description: "CreateWithoutTimeout",
"CreateWithoutTimeout": {
TestResource: &Resource{
SchemaVersion: 4,
Schema: map[string]*Schema{
Expand All @@ -781,9 +818,9 @@ func TestApplyResourceChange(t *testing.T) {
return nil
},
},
ExpectedUnsafeLegacyTypeSystem: true,
},
{
Description: "Create_cty",
"Create_cty": {
TestResource: &Resource{
SchemaVersion: 4,
Schema: map[string]*Schema{
Expand All @@ -806,9 +843,9 @@ func TestApplyResourceChange(t *testing.T) {
return nil
},
},
ExpectedUnsafeLegacyTypeSystem: true,
},
{
Description: "CreateContext_SchemaFunc",
"CreateContext_SchemaFunc": {
TestResource: &Resource{
SchemaFunc: func() map[string]*Schema {
return map[string]*Schema{
Expand All @@ -823,12 +860,32 @@ func TestApplyResourceChange(t *testing.T) {
return nil
},
},
ExpectedUnsafeLegacyTypeSystem: true,
},
"EnableLegacyTypeSystemApplyErrors": {
TestResource: &Resource{
EnableLegacyTypeSystemApplyErrors: true,
Schema: map[string]*Schema{
"foo": {
Type: TypeInt,
Optional: true,
},
},
CreateContext: func(_ context.Context, rd *ResourceData, _ interface{}) diag.Diagnostics {
rd.SetId("bar")
return nil
},
},
ExpectedUnsafeLegacyTypeSystem: false,
},
}

for _, testCase := range testCases {
testCase := testCase
t.Run(testCase.Description, func(t *testing.T) {
for name, testCase := range testCases {
name, testCase := name, testCase

t.Run(name, func(t *testing.T) {
t.Parallel()

server := NewGRPCProviderServer(&Provider{
ResourcesMap: map[string]*Resource{
"test": testCase.TestResource,
Expand Down Expand Up @@ -892,6 +949,12 @@ func TestApplyResourceChange(t *testing.T) {
if id != "bar" {
t.Fatalf("incorrect final state: %#v\n", newStateVal)
}

//nolint:staticcheck // explicitly for this SDK
if testCase.ExpectedUnsafeLegacyTypeSystem != resp.UnsafeToUseLegacyTypeSystem {
//nolint:staticcheck // explicitly for this SDK
t.Fatalf("expected UnsafeLegacyTypeSystem %t, got: %t", testCase.ExpectedUnsafeLegacyTypeSystem, resp.UnsafeToUseLegacyTypeSystem)
}
})
}
}
Expand Down
Loading

0 comments on commit 2bbfe8e

Please sign in to comment.