From 8d3feef31460375de93993ee66ca0ba7eee8e531 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Thu, 17 Feb 2022 12:13:58 -0500 Subject: [PATCH 1/3] tfsdk: Support protocol version 5 and verify valid resource type in UpgradeResourceState RPC Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/42 Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/262 Reference: https://github.com/hashicorp/terraform-provider-corner/pull/44 Terraform CLI version 0.12 through 0.14 support protocol version 5 and require the `UpgradeResourceStateResponse` type `UpgradedState` type `Msgpack` field. To support downgraded framework providers via terraform-plugin-mux `tf6to5server`, the `RawState` type `JSON` field cannot simply be passed through like the previous implementation. This change will parse the `RawState` and output it via the `tfprotov6.NewDynamicValue()` function, which will always set the `Msgpack` field. Similar logic would have already been required as part of the larger effort to support resource versioning and upgrading resource state over time. Passthrough of the same state of an invalid resource type should not have been previously allowed, since the provider did not implement the resource type. Regardless, Terraform CLI should not have reached this point since `GetProviderSchema` would not have returned the resource type anyways for other resource operations. Verified via integration testing of terraform-plugin-mux `tf6to5server`, which previously returned a non-descript `EOF` error. --- .changelog/pending.txt | 3 + tfsdk/serve.go | 100 ++++++++++- tfsdk/serve_provider_test.go | 6 + tfsdk/serve_resource_upgrade_state_test.go | 106 +++++++++++ tfsdk/serve_test.go | 197 +++++++++++++++++++++ 5 files changed, 404 insertions(+), 8 deletions(-) create mode 100644 .changelog/pending.txt create mode 100644 tfsdk/serve_resource_upgrade_state_test.go diff --git a/.changelog/pending.txt b/.changelog/pending.txt new file mode 100644 index 000000000..e3e8f1f9a --- /dev/null +++ b/.changelog/pending.txt @@ -0,0 +1,3 @@ +```release-note:bug +tfsdk: Support protocol version 5 and verify valid resource type in `UpgradeResourceState` RPC +``` diff --git a/tfsdk/serve.go b/tfsdk/serve.go index f3fae52b5..b7547b953 100644 --- a/tfsdk/serve.go +++ b/tfsdk/serve.go @@ -485,16 +485,100 @@ func (s *server) validateResourceConfig(ctx context.Context, req *tfprotov6.Vali resp.Diagnostics = validateSchemaResp.Diagnostics } -func (s *server) UpgradeResourceState(ctx context.Context, req *tfprotov6.UpgradeResourceStateRequest) (*tfprotov6.UpgradeResourceStateResponse, error) { - // uncomment when we implement this function - //ctx = s.registerContext(ctx) +// upgradeResourceStateResponse is a thin abstraction to allow native +// Diagnostics usage. +type upgradeResourceStateResponse struct { + Diagnostics diag.Diagnostics + UpgradedState *tfprotov6.DynamicValue +} - // TODO: support state upgrades +func (r upgradeResourceStateResponse) toTfprotov6() *tfprotov6.UpgradeResourceStateResponse { return &tfprotov6.UpgradeResourceStateResponse{ - UpgradedState: &tfprotov6.DynamicValue{ - JSON: req.RawState.JSON, - }, - }, nil + Diagnostics: r.Diagnostics.ToTfprotov6Diagnostics(), + UpgradedState: r.UpgradedState, + } +} + +func (s *server) UpgradeResourceState(ctx context.Context, req *tfprotov6.UpgradeResourceStateRequest) (*tfprotov6.UpgradeResourceStateResponse, error) { + ctx = s.registerContext(ctx) + resp := &upgradeResourceStateResponse{} + + s.upgradeResourceState(ctx, req, resp) + + return resp.toTfprotov6(), nil +} + +func (s *server) upgradeResourceState(ctx context.Context, req *tfprotov6.UpgradeResourceStateRequest, resp *upgradeResourceStateResponse) { + if req == nil { + return + } + + resourceType, diags := s.getResourceType(ctx, req.TypeName) + + resp.Diagnostics.Append(diags...) + + if resp.Diagnostics.HasError() { + return + } + + // No UpgradedState to return. This could return an error diagnostic about + // the odd scenario, but seems best to allow Terraform CLI to handle the + // situation itself in case it might be expected behavior. + if req.RawState == nil { + return + } + + // This implementation assumes the current schema is the only valid schema + // for the given resource and will return an error if any mismatched prior + // state is given. This matches prior behavior of the framework, but is now + // more explicit in error handling, rather than just passing through any + // potentially errant prior state, which should have resulted in a similar + // error further in the resource lifecycle. + // + // TODO: Implement resource state upgrades, rather than just using the + // current resource schema. + // Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/42 + resourceSchema, diags := resourceType.GetSchema(ctx) + + resp.Diagnostics.Append(diags...) + + if resp.Diagnostics.HasError() { + return + } + + resourceSchemaType := resourceSchema.TerraformType(ctx) + + rawStateValue, err := req.RawState.Unmarshal(resourceSchemaType) + + if err != nil { + resp.Diagnostics.AddError( + "Unable to Read Previously Saved State for UpgradeResourceState", + "There was an error reading the saved resource state using the current resource schema. "+ + "This resource was implemented in a Terraform Provider SDK that does not support upgrading resource state yet.\n\n"+ + "If the resource previously implemented different resource state versions, the provider developers will need to revert back to the previous implementation. "+ + "If this resource state was last refreshed with Terraform CLI 0.11 and earlier, it must be refreshed or applied with an older provider version first. "+ + "If you manually modified the resource state, you will need to manually modify it to match the current resource schema. "+ + "Otherwise, please report this to the provider developer:\n\n"+err.Error(), + ) + return + } + + // NewDynamicValue will ensure the Msgpack field is set for Terraform CLI + // 0.12 through 0.14 compatibility when using terraform-plugin-mux tf6to5server. + // Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/262 + upgradedStateValue, err := tfprotov6.NewDynamicValue(resourceSchemaType, rawStateValue) + + if err != nil { + resp.Diagnostics.AddError( + "Unable to Convert Previously Saved State for UpgradeResourceState", + "There was an error converting the saved resource state using the current resource schema. "+ + "This is always an issue in the Terraform Provider SDK used to implement the resource and should be reported to the provider developers.\n\n"+ + "Please report this to the provider developer:\n\n"+err.Error(), + ) + return + } + + resp.UpgradedState = &upgradedStateValue } // readResourceResponse is a thin abstraction to allow native Diagnostics usage diff --git a/tfsdk/serve_provider_test.go b/tfsdk/serve_provider_test.go index 44f809fb4..403c7cbcc 100644 --- a/tfsdk/serve_provider_test.go +++ b/tfsdk/serve_provider_test.go @@ -23,6 +23,11 @@ type testServeProvider struct { validateResourceConfigCalledResourceType string validateResourceConfigImpl func(context.Context, ValidateResourceConfigRequest, *ValidateResourceConfigResponse) + // upgrade resource state + // TODO: Implement with UpgradeResourceState support + // Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/42 + // upgradeResourceStateCalledResourceType string + // read resource request readResourceCurrentStateValue tftypes.Value readResourceCurrentStateSchema Schema @@ -636,6 +641,7 @@ func (t *testServeProvider) GetResources(_ context.Context) (map[string]Resource "test_attribute_plan_modifiers": testServeResourceTypeAttributePlanModifiers{}, "test_config_validators": testServeResourceTypeConfigValidators{}, "test_import_state": testServeResourceTypeImportState{}, + "test_upgrade_state": testServeResourceTypeUpgradeState{}, "test_validate_config": testServeResourceTypeValidateConfig{}, }, nil } diff --git a/tfsdk/serve_resource_upgrade_state_test.go b/tfsdk/serve_resource_upgrade_state_test.go new file mode 100644 index 000000000..0851024b1 --- /dev/null +++ b/tfsdk/serve_resource_upgrade_state_test.go @@ -0,0 +1,106 @@ +package tfsdk + +import ( + "context" + "fmt" + + "github.com/hashicorp/terraform-plugin-framework/diag" + "github.com/hashicorp/terraform-plugin-framework/types" + "github.com/hashicorp/terraform-plugin-go/tfprotov6" + "github.com/hashicorp/terraform-plugin-go/tftypes" +) + +// This resource is a placeholder for UpgradeResourceState testing, +// so it is decoupled from other test resources. +// TODO: Implement UpgradeResourceState support, when added +// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/42 +type testServeResourceTypeUpgradeState struct{} + +func (t testServeResourceTypeUpgradeState) GetSchema(_ context.Context) (Schema, diag.Diagnostics) { + return Schema{ + Attributes: map[string]Attribute{ + "id": { + Type: types.StringType, + Computed: true, + }, + "optional_string": { + Type: types.StringType, + Optional: true, + }, + "required_string": { + Type: types.StringType, + Required: true, + }, + }, + }, nil +} + +func (t testServeResourceTypeUpgradeState) NewResource(_ context.Context, p Provider) (Resource, diag.Diagnostics) { + provider, ok := p.(*testServeProvider) + if !ok { + prov, ok := p.(*testServeProviderWithMetaSchema) + if !ok { + panic(fmt.Sprintf("unexpected provider type %T", p)) + } + provider = prov.testServeProvider + } + return testServeResourceUpgradeState{ + provider: provider, + }, nil +} + +var testServeResourceTypeUpgradeStateSchema = &tfprotov6.Schema{ + Block: &tfprotov6.SchemaBlock{ + Attributes: []*tfprotov6.SchemaAttribute{ + { + Name: "id", + Computed: true, + Type: tftypes.String, + }, + { + Name: "optional_string", + Optional: true, + Type: tftypes.String, + }, + { + Name: "required_string", + Required: true, + Type: tftypes.String, + }, + }, + }, +} + +var testServeResourceTypeUpgradeStateTftype = tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "id": tftypes.String, + "optional_string": tftypes.String, + "required_string": tftypes.String, + }, +} + +type testServeResourceUpgradeStateData struct { + Id string `tfsdk:"id"` + OptionalString *string `tfsdk:"optional_string"` + RequiredString string `tfsdk:"required_string"` +} + +type testServeResourceUpgradeState struct { + provider *testServeProvider +} + +func (r testServeResourceUpgradeState) Create(ctx context.Context, req CreateResourceRequest, resp *CreateResourceResponse) { + // Intentionally blank. Not expected to be called during testing. +} +func (r testServeResourceUpgradeState) Read(ctx context.Context, req ReadResourceRequest, resp *ReadResourceResponse) { + // Intentionally blank. Not expected to be called during testing. +} +func (r testServeResourceUpgradeState) Update(ctx context.Context, req UpdateResourceRequest, resp *UpdateResourceResponse) { + // Intentionally blank. Not expected to be called during testing. +} +func (r testServeResourceUpgradeState) Delete(ctx context.Context, req DeleteResourceRequest, resp *DeleteResourceResponse) { + // Intentionally blank. Not expected to be called during testing. +} +func (r testServeResourceUpgradeState) ImportState(ctx context.Context, req ImportResourceStateRequest, resp *ImportResourceStateResponse) { + ResourceImportStateNotImplemented(ctx, "intentionally not implemented", resp) +} diff --git a/tfsdk/serve_test.go b/tfsdk/serve_test.go index 5fa35ff64..5368586a4 100644 --- a/tfsdk/serve_test.go +++ b/tfsdk/serve_test.go @@ -2,6 +2,8 @@ package tfsdk import ( "context" + "encoding/json" + "strings" "sync" "testing" "time" @@ -279,6 +281,7 @@ func TestServerGetProviderSchema(t *testing.T) { "test_attribute_plan_modifiers": testServeResourceTypeAttributePlanModifiersSchema, "test_config_validators": testServeResourceTypeConfigValidatorsSchema, "test_import_state": testServeResourceTypeImportStateSchema, + "test_upgrade_state": testServeResourceTypeUpgradeStateSchema, "test_validate_config": testServeResourceTypeValidateConfigSchema, }, DataSourceSchemas: map[string]*tfprotov6.Schema{ @@ -314,6 +317,7 @@ func TestServerGetProviderSchemaWithProviderMeta(t *testing.T) { "test_attribute_plan_modifiers": testServeResourceTypeAttributePlanModifiersSchema, "test_config_validators": testServeResourceTypeConfigValidatorsSchema, "test_import_state": testServeResourceTypeImportStateSchema, + "test_upgrade_state": testServeResourceTypeUpgradeStateSchema, "test_validate_config": testServeResourceTypeValidateConfigSchema, }, DataSourceSchemas: map[string]*tfprotov6.Schema{ @@ -1402,6 +1406,199 @@ func TestServerValidateResourceConfig(t *testing.T) { } } +func TestServerUpgradeResourceState(t *testing.T) { + t.Parallel() + + ctx := context.Background() + schema, _ := testServeResourceTypeUpgradeState{}.GetSchema(ctx) + schemaType := schema.TerraformType(ctx) + + validRawStateJSON, err := json.Marshal(map[string]string{ + "id": "test-id-value", + "required_string": "test-required-value", + }) + + if err != nil { + t.Fatalf("unable to create RawState JSON: %s", err) + } + + validRawState := tfprotov6.RawState{ + JSON: validRawStateJSON, + } + + validUpgradedState, err := tfprotov6.NewDynamicValue(schemaType, tftypes.NewValue(schemaType, map[string]tftypes.Value{ + "id": tftypes.NewValue(tftypes.String, "test-id-value"), + "optional_string": tftypes.NewValue(tftypes.String, nil), + "required_string": tftypes.NewValue(tftypes.String, "test-required-value"), + })) + + if err != nil { + t.Fatalf("unable to create UpgradedState: %s", err) + } + + testCases := map[string]struct { + request *tfprotov6.UpgradeResourceStateRequest + expectedResponse *tfprotov6.UpgradeResourceStateResponse + expectedError error + }{ + "nil": { + request: nil, + expectedResponse: &tfprotov6.UpgradeResourceStateResponse{}, + }, + "RawState-Flatmap": { + request: &tfprotov6.UpgradeResourceStateRequest{ + TypeName: "test_upgrade_state", + RawState: &tfprotov6.RawState{ + Flatmap: map[string]string{ + "flatmap": "is not supported", + }, + }, + }, + expectedResponse: &tfprotov6.UpgradeResourceStateResponse{ + Diagnostics: []*tfprotov6.Diagnostic{ + { + Severity: tfprotov6.DiagnosticSeverityError, + Summary: "Unable to Read Previously Saved State for UpgradeResourceState", + Detail: "There was an error reading the saved resource state using the current resource schema. " + + "This resource was implemented in a Terraform Provider SDK that does not support upgrading resource state yet.\n\n" + + "If the resource previously implemented different resource state versions, the provider developers will need to revert back to the previous implementation. " + + "If this resource state was last refreshed with Terraform CLI 0.11 and earlier, it must be refreshed or applied with an older provider version first. " + + "If you manually modified the resource state, you will need to manually modify it to match the current resource schema. " + + "Otherwise, please report this to the provider developer:\n\n" + + "flatmap states cannot be unmarshaled, only states written by Terraform 0.12 and higher can be unmarshaled", + }, + }, + }, + }, + "RawState-JSON-passthrough": { + request: &tfprotov6.UpgradeResourceStateRequest{ + TypeName: "test_upgrade_state", + RawState: &validRawState, + }, + expectedResponse: &tfprotov6.UpgradeResourceStateResponse{ + UpgradedState: &validUpgradedState, + }, + }, + "RawState-JSON-mismatch": { + request: &tfprotov6.UpgradeResourceStateRequest{ + TypeName: "test_upgrade_state", + RawState: &tfprotov6.RawState{ + JSON: []byte(`{"nonexistent_attribute":"value"}`), + }, + }, + expectedResponse: &tfprotov6.UpgradeResourceStateResponse{ + Diagnostics: []*tfprotov6.Diagnostic{ + { + Severity: tfprotov6.DiagnosticSeverityError, + Summary: "Unable to Read Previously Saved State for UpgradeResourceState", + Detail: "There was an error reading the saved resource state using the current resource schema. " + + "This resource was implemented in a Terraform Provider SDK that does not support upgrading resource state yet.\n\n" + + "If the resource previously implemented different resource state versions, the provider developers will need to revert back to the previous implementation. " + + "If this resource state was last refreshed with Terraform CLI 0.11 and earlier, it must be refreshed or applied with an older provider version first. " + + "If you manually modified the resource state, you will need to manually modify it to match the current resource schema. " + + "Otherwise, please report this to the provider developer:\n\n" + + "ElementKeyValue(tftypes.String): unsupported attribute \"nonexistent_attribute\"", + }, + }, + }, + }, + "RawState-empty": { + request: &tfprotov6.UpgradeResourceStateRequest{ + RawState: &tfprotov6.RawState{}, + TypeName: "test_upgrade_state", + }, + expectedResponse: &tfprotov6.UpgradeResourceStateResponse{ + Diagnostics: []*tfprotov6.Diagnostic{ + { + Severity: tfprotov6.DiagnosticSeverityError, + Summary: "Unable to Read Previously Saved State for UpgradeResourceState", + Detail: "There was an error reading the saved resource state using the current resource schema. " + + "This resource was implemented in a Terraform Provider SDK that does not support upgrading resource state yet.\n\n" + + "If the resource previously implemented different resource state versions, the provider developers will need to revert back to the previous implementation. " + + "If this resource state was last refreshed with Terraform CLI 0.11 and earlier, it must be refreshed or applied with an older provider version first. " + + "If you manually modified the resource state, you will need to manually modify it to match the current resource schema. " + + "Otherwise, please report this to the provider developer:\n\n" + + "RawState had no JSON or flatmap data set", + }, + }, + }, + }, + "RawState-missing": { + request: &tfprotov6.UpgradeResourceStateRequest{ + TypeName: "test_upgrade_state", + }, + expectedResponse: &tfprotov6.UpgradeResourceStateResponse{}, + }, + "TypeName-missing": { + request: &tfprotov6.UpgradeResourceStateRequest{}, + expectedResponse: &tfprotov6.UpgradeResourceStateResponse{ + Diagnostics: []*tfprotov6.Diagnostic{ + { + Severity: tfprotov6.DiagnosticSeverityError, + Summary: "Resource not found", + Detail: "No resource named \"\" is configured on the provider", + }, + }, + }, + }, + "TypeName-unknown": { + request: &tfprotov6.UpgradeResourceStateRequest{ + TypeName: "unknown", + }, + expectedResponse: &tfprotov6.UpgradeResourceStateResponse{ + Diagnostics: []*tfprotov6.Diagnostic{ + { + Severity: tfprotov6.DiagnosticSeverityError, + Summary: "Resource not found", + Detail: "No resource named \"unknown\" is configured on the provider", + }, + }, + }, + }, + } + + for name, testCase := range testCases { + name, testCase := name, testCase + + t.Run(name, func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + testProvider := &testServeProvider{} + testServer := &server{ + p: testProvider, + } + + got, err := testServer.UpgradeResourceState(ctx, testCase.request) + + if err != nil { + if testCase.expectedError == nil { + t.Fatalf("expected no error, got: %s", err) + } + + if !strings.Contains(err.Error(), testCase.expectedError.Error()) { + t.Fatalf("expected error %q, got: %s", testCase.expectedError, err) + } + } + + if err == nil && testCase.expectedError != nil { + t.Fatalf("got no error, expected: %s", testCase.expectedError) + } + + // TODO: Implement with UpgradeResourceState support + // Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/42 + // if testCase.request != nil && testProvider.upgradeResourceStateCalledResourceType != testCase.request.TypeName { + // t.Errorf("expected to call resource %q, called: %s", testCase.request.TypeName, testProvider.upgradeResourceStateCalledResourceType) + // return + // } + + if diff := cmp.Diff(got, testCase.expectedResponse); diff != "" { + t.Errorf("unexpected difference in response: %s", diff) + } + }) + } +} + func TestServerReadResource(t *testing.T) { t.Parallel() From ef9f28923341aaba6e4d20d0e1cd4e9cda017afd Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Thu, 17 Feb 2022 12:17:50 -0500 Subject: [PATCH 2/3] Update CHANGELOG for #263 --- .changelog/{pending.txt => 263.txt} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .changelog/{pending.txt => 263.txt} (100%) diff --git a/.changelog/pending.txt b/.changelog/263.txt similarity index 100% rename from .changelog/pending.txt rename to .changelog/263.txt From 42cdb5dda91ee5e2f8d04443753b2b643b202aba Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Thu, 17 Feb 2022 12:20:53 -0500 Subject: [PATCH 3/3] Comment unused testing code for now --- tfsdk/serve_resource_upgrade_state_test.go | 24 +++++++++++----------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tfsdk/serve_resource_upgrade_state_test.go b/tfsdk/serve_resource_upgrade_state_test.go index 0851024b1..56fc8f37b 100644 --- a/tfsdk/serve_resource_upgrade_state_test.go +++ b/tfsdk/serve_resource_upgrade_state_test.go @@ -71,19 +71,19 @@ var testServeResourceTypeUpgradeStateSchema = &tfprotov6.Schema{ }, } -var testServeResourceTypeUpgradeStateTftype = tftypes.Object{ - AttributeTypes: map[string]tftypes.Type{ - "id": tftypes.String, - "optional_string": tftypes.String, - "required_string": tftypes.String, - }, -} +// var testServeResourceTypeUpgradeStateTftype = tftypes.Object{ +// AttributeTypes: map[string]tftypes.Type{ +// "id": tftypes.String, +// "optional_string": tftypes.String, +// "required_string": tftypes.String, +// }, +// } -type testServeResourceUpgradeStateData struct { - Id string `tfsdk:"id"` - OptionalString *string `tfsdk:"optional_string"` - RequiredString string `tfsdk:"required_string"` -} +// type testServeResourceUpgradeStateData struct { +// Id string `tfsdk:"id"` +// OptionalString *string `tfsdk:"optional_string"` +// RequiredString string `tfsdk:"required_string"` +// } type testServeResourceUpgradeState struct { provider *testServeProvider