resource: Initial MoveResourceState RPC support #917

bflad · 2024-02-01T22:32:49Z

This change adds initial support for the MoveResourceState RPC to the framework, including:

Adding the framework shared server implementation for the new RPC
Adding the protocol version 5 and 6 server implementations for the new RPC
Adding the type conversion logic for the framework types to/from the protocol types
Exposing a new resource.ResourceWithMoveState interface that providers can implement to support the new RPC
Adding a website documentation page for the new functionality

A state move using the new RPC occurs when a Terraform 1.8 and later configuration includes a moved configuration block such as the following:

moved {
    from = "examplecloud_source.XXX"
    to   = "examplecloud_target.XXX"
}

There are no restrictions on the source resource types, but target resource types must opt into support to prevent data loss. Target resources can support moves from multiple, differing source resources, so the framework implementation is exposed as an ordered list to developers.

The framework implementation for the new RPC is as follows:

If no state move support is defined for the resource, the framework returns an error diagnostic.
If state move support is defined for the resource, each provider-defined state move implementation is called until one responds with error diagnostics or state data.
If all provider-defined state move implementations return without error diagnostics and state data, the framework returns an error diagnostic.

The protocol server unit testing shows the end-to-end handling of the new RPC, including the provider-defined resource implementations, the type conversion logic, and the framework shared server implementation. The exposed resource.ResourceWithMoveState implementation is intentionally similar to the existing resource.ResourceWithUpgradeState handling, both in internal details and the exposed API. This is to provide a smoother experience for provider developers familiar with the other functionality and maintainers of the framework.

This initial implementation exposes a lot of the request/response handling as-is, however there is likely potential for introducing native helper functionality for developers, such as implementing one to simplify "aliasing"/"renaming" an existing resource.Resource within the same provider codebase.

This change adds initial support for the `MoveResourceState` RPC to the framework, including: * Adding the framework shared server implementation for the new RPC * Adding the protocol version 5 and 6 server implementations for the new RPC * Adding the type conversion logic for the framework types to/from the protocol types * Exposing a new `resource.ResourceWithMoveState` interface that providers can implement to support the new RPC * Adding a website documentation page for the new functionality A state move using the new RPC occurs when a Terraform 1.8 and later configuration includes a `moved` configuration block such as the following: ```terraform moved { from = "examplecloud_source.XXX" to = "examplecloud_target.XXX" } ``` There are no restrictions on the source resource types, but target resource types must opt into support to prevent data loss. Target resources can support moves from multiple, differing source resources, so the framework implementation is exposed as an ordered list to developers. The framework implementation for the new RPC is as follows: * If no state move support is defined for the resource, the framework returns an error diagnostic. * If state move support is defined for the resource, each provider-defined state move implementation is called until one responds with error diagnostics or state data. * If all provider-defined state move implementations return without error diagnostics and state data, the framework returns an error diagnostic. The protocol server unit testing shows the end-to-end handling of the new RPC, including the provider-defined resource implementations, the type conversion logic, and the framework shared server implementation. The exposed `resource.ResourceWithMoveState` implementation is intentionally similar to the existing `resource.ResourceWithUpgradeState` handling, both in internal details and the exposed API. This is to provide a smoother experience for provider developers familiar with the other functionality and maintainers of the framework.

austinvalle

Looks great! I had a couple notes and questions but nothing major.

austinvalle · 2024-02-02T14:12:19Z

internal/fwserver/server_moveresourcestate.go

+	SourcePrivate         *privatestate.Data
+	SourceProviderAddress string
+	SourceSchemaVersion   int64
+	SourceRawState        *tfprotov6.RawState


This was initially confusing to see in the PR, but I looked through the upgrade resource request code and found this comment:

terraform-plugin-framework/internal/fwserver/server_upgraderesourcestate.go

Lines 23 to 25 in f2e8b33

// TODO: Create framework defined type that is not protocol specific.

// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/340

RawState *tfprotov6.RawState

Do we want to add a comment here describing why it's okay to keep it protocol version specific for future maintainers? Seems like it's unlikely we'll come back and refactor this since the issue is closed and glancing through it, seems like a non-zero effort.

Adding a comment here is an excellent suggestion, will do.

Took a swing at this in the latest changes -- please reach out if you have other considerations or suggestions!

austinvalle · 2024-02-02T14:21:47Z

internal/toproto6/moveresourcestate.go

+		return nil
+	}
+
+	proto5 := &tfprotov6.MoveResourceStateResponse{


super nit: proto6 :=

🦅 👀 : Fixed in 46a1487.

austinvalle · 2024-02-02T14:51:13Z

internal/fwserver/server_moveresourcestate.go

+			if err != nil {
+				logging.FrameworkDebug(
+					ctx,
+					"Error unmarshalling SourceRawState using the provided SourceSchema for source "+
+						req.SourceProviderAddress+" resource type "+
+						req.SourceTypeName+" with schema version "+
+						strconv.FormatInt(req.SourceSchemaVersion, 10)+". "+
+						"This is not a fatal error since resources can support multiple source resources which cause this type of error to be unavoidable, "+
+						"but due to this error the SourceState will not be populated for the implementation.",
+					map[string]any{
+						logging.KeyError: err,
+					},
+				)


Curious q: is there a reason to not just continue here after logging? Are there scenarios where a source schema would be present, it doesn't match, but the implementation of move state would still work and would return a target state?

I think you may have found out later in reading the implementation but yeah, the comment at the beginning of this error message conditional here could probably use a little more verbiage. This logic essentially will always run if SourceSchema is set but before its ever passed off to StateMover provider-defined logic which might "skip" based on the other request fields (source resource type, source provider address, etc.).

We could technically support some upfront "filtering" via framework logic before it reaches here by adding SourceTypeName, etc. fields next to SourceSchema in StateMover that have some opinionated behaviors, but I was hesitant to immediately introduce that. 😄

I think you may have found out later in reading the implementation

Ah yes! My main issue with reviewing PRs is not returning to previous in-progress comments lol.. 😅

This all makes sense to me and the additional docs on SourceState I think closes the loop nicely with this log message 👍🏻

Took a swing at this in the latest changes -- please reach out if you have other considerations or suggestions!

austinvalle · 2024-02-02T15:03:47Z

internal/fwserver/server_moveresourcestate_test.go

Do we want/need to have tests for some of the error "fail-forward" conditions when there are multiple implementations? Like:

StateMover 1 schema doesn't match source, StateMover 2 schema matches + returns target state == success

StateMover 1 schema doesn't match source, StateMover 2 doesn't define schema + returns target state == success

StateMover 1 schema matches + returns target state, StaveMover 2 schema matches + would return target state == first is called, second is not

Aye -- yes! Thank you. I had meant to do that (and even added a code comment alluding to that in the protocol server unit testing), but neglected to actually do this. Will add these scenarios to the shared framework server unit testing to give us some more confidence that it does what it should while catching regressions in the future for these behaviors.

This actually found an interesting quirk and thank you for talking with me about it! Due to our usage of IgnoreUndefinedAttributes it is possible to declare a schema that will not error on conversion (generally caused by invalid types), but instead will have null values. Due to this we are thinking to adjust our wording around checking SourceState for nil, since its not always guaranteed to be that with a different schema. The good news is that a subsequent plan against the target resource should catch data oddities, if for example null values were unexpectedly being moved.

austinvalle · 2024-02-02T15:08:20Z

resource/move_state.go

+	// TODO: Create framework defined type that is not protocol specific.
+	// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/340
+	SourceRawState *tfprotov6.RawState


I mentioned this in another comment, but this issue is closed now. Do we need to re-open it or remove this comment?

I'm guessing our best action right now is removing it from the public API docs and keeping some form of it on internal/fwserver.MoveResourceStateRequest.SourceRawState as you mentioned in your other comment. 👍

Took a swing at this in the latest changes -- please reach out if you have other considerations or suggestions!

austinvalle · 2024-02-02T15:14:20Z

resource/move_state.go

+	// If this request matches the intended implementation, the implementation
+	// logic must set [MoveStateResponse.TargetState] as it is intentionally not
+	// copied automatically.
+	SourceState *tfsdk.State


I made a comment above about this field. If it's possible that the StateMover implementation may be called with this field as nil when StateMover.SourceSchema doesn't match, we may want to document that here.

Ah yes, I see what you are getting at. Great suggestion. Will update to mention that, along with the fact that the framework will log those errors instead of returning them, similar to what is mentioned in the SourceSchema field documentation.

Took a swing at this in the latest changes -- please reach out if you have other considerations or suggestions!

austinvalle · 2024-02-02T15:20:55Z

resource/state_mover.go

+	// SourceSchema is an optional schema for the intended source resource state
+	// and schema version. While not required, setting this will populate
+	// [MoveStateRequest.SourceState] when possible similar to other [Resource]
+	// types. State conversion errors based on this schema are only logged at
+	// DEBUG level as there may be multiple [StateMover] implementations on the
+	// same target resource for differing source resources.
+	//
+	// If not set, source state data is only available in
+	// [MoveStateRequest.SourceRawState].
+	SourceSchema *schema.Schema


Should we mention explicitly that state conversion errors will not cause the StateMover function to be skipped? It's kind of implied but was something that came to my mind when reading the interface.

Definitely appreciate being more explicit 👍

Took a swing at this in the latest changes -- please reach out if you have other considerations or suggestions!

austinvalle · 2024-02-02T15:34:36Z

website/docs/plugin/framework/resources/state-move.mdx

+
+Note these caveats when implementing the `MoveState` method:
+
+* An error is returned if the response state contains unknown values. Set all attributes to either null or known values in the response.


Curious q: Is this validation implemented on the Terraform core side?

I had asked about it early on since this was being implemented at the same time as that new checking logic, but if it isn't already, we can certainly make the quick change on that side. 👍

Looks like this should be good! https://github.com/hashicorp/terraform/blob/48148eda5f5111fe4d8fdd28882472076227691a/internal/refactoring/cross_provider_move.go#L165-L173

austinvalle · 2024-02-02T15:34:55Z

website/docs/plugin/framework/resources/state-move.mdx

Docs look great ❤️

bflad

Will take care of these things shortly (likely tomorrow 😄 ).

bflad · 2024-02-05T19:46:10Z

internal/toproto6/moveresourcestate.go

+		return nil
+	}
+
+	proto5 := &tfprotov6.MoveResourceStateResponse{


🦅 👀 : Fixed in 46a1487.

bflad · 2024-02-05T19:48:43Z

resource/move_state.go

+	// If this request matches the intended implementation, the implementation
+	// logic must set [MoveStateResponse.TargetState] as it is intentionally not
+	// copied automatically.
+	SourceState *tfsdk.State


Ah yes, I see what you are getting at. Great suggestion. Will update to mention that, along with the fact that the framework will log those errors instead of returning them, similar to what is mentioned in the SourceSchema field documentation.

bflad · 2024-02-05T19:50:28Z

resource/state_mover.go

+	// SourceSchema is an optional schema for the intended source resource state
+	// and schema version. While not required, setting this will populate
+	// [MoveStateRequest.SourceState] when possible similar to other [Resource]
+	// types. State conversion errors based on this schema are only logged at
+	// DEBUG level as there may be multiple [StateMover] implementations on the
+	// same target resource for differing source resources.
+	//
+	// If not set, source state data is only available in
+	// [MoveStateRequest.SourceRawState].
+	SourceSchema *schema.Schema


Definitely appreciate being more explicit 👍

bflad · 2024-02-05T19:51:49Z

website/docs/plugin/framework/resources/state-move.mdx

+
+Note these caveats when implementing the `MoveState` method:
+
+* An error is returned if the response state contains unknown values. Set all attributes to either null or known values in the response.


I had asked about it early on since this was being implemented at the same time as that new checking logic, but if it isn't already, we can certainly make the quick change on that side. 👍

bflad · 2024-02-05T19:53:24Z

resource/move_state.go

+	// TODO: Create framework defined type that is not protocol specific.
+	// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/340
+	SourceRawState *tfprotov6.RawState


I'm guessing our best action right now is removing it from the public API docs and keeping some form of it on internal/fwserver.MoveResourceStateRequest.SourceRawState as you mentioned in your other comment. 👍

bflad · 2024-02-05T19:55:32Z

internal/fwserver/server_moveresourcestate_test.go

Aye -- yes! Thank you. I had meant to do that (and even added a code comment alluding to that in the protocol server unit testing), but neglected to actually do this. Will add these scenarios to the shared framework server unit testing to give us some more confidence that it does what it should while catching regressions in the future for these behaviors.

bflad · 2024-02-15T21:19:26Z

internal/fwserver/server_moveresourcestate.go

+			if err != nil {
+				logging.FrameworkDebug(
+					ctx,
+					"Error unmarshalling SourceRawState using the provided SourceSchema for source "+
+						req.SourceProviderAddress+" resource type "+
+						req.SourceTypeName+" with schema version "+
+						strconv.FormatInt(req.SourceSchemaVersion, 10)+". "+
+						"This is not a fatal error since resources can support multiple source resources which cause this type of error to be unavoidable, "+
+						"but due to this error the SourceState will not be populated for the implementation.",
+					map[string]any{
+						logging.KeyError: err,
+					},
+				)


I think you may have found out later in reading the implementation but yeah, the comment at the beginning of this error message conditional here could probably use a little more verbiage. This logic essentially will always run if SourceSchema is set but before its ever passed off to StateMover provider-defined logic which might "skip" based on the other request fields (source resource type, source provider address, etc.).

We could technically support some upfront "filtering" via framework logic before it reaches here by adding SourceTypeName, etc. fields next to SourceSchema in StateMover that have some opinionated behaviors, but I was hesitant to immediately introduce that. 😄

bflad · 2024-02-15T21:20:57Z

internal/fwserver/server_moveresourcestate.go

+	SourcePrivate         *privatestate.Data
+	SourceProviderAddress string
+	SourceSchemaVersion   int64
+	SourceRawState        *tfprotov6.RawState


Adding a comment here is an excellent suggestion, will do.

github-actions · 2024-03-18T02:05:18Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

bflad added the enhancement New feature or request label Feb 1, 2024

bflad added this to the v1.6.0 milestone Feb 1, 2024

Update CHANGELOG for #917

67e1ae8

bflad marked this pull request as ready for review February 1, 2024 22:37

bflad requested a review from a team as a code owner February 1, 2024 22:37

austinvalle approved these changes Feb 2, 2024

View reviewed changes

internal/toproto6: Fix variable naming

46a1487

bflad commented Feb 15, 2024

View reviewed changes

bflad added 3 commits February 16, 2024 14:25

Improve SourceSchema and SourceRawState Go documentation

b321d89

Additional SourceSchema testing and clarify documentation

09ebdb8

Merge branch 'main' into bflad/MoveResourceState

31d9d6c

bflad merged commit 86b2acb into main Feb 16, 2024
28 checks passed

bflad deleted the bflad/MoveResourceState branch February 16, 2024 22:05

github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2024

	// TODO: Create framework defined type that is not protocol specific.
	// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/340
	RawState *tfprotov6.RawState


		Note these caveats when implementing the `MoveState` method:

		* An error is returned if the response state contains unknown values. Set all attributes to either null or known values in the response.

resource: Initial MoveResourceState RPC support #917

resource: Initial MoveResourceState RPC support #917

Conversation

bflad commented Feb 1, 2024 • edited Loading

austinvalle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 18, 2024

bflad commented Feb 1, 2024 •

edited

Loading

bflad Feb 16, 2024 •

edited

Loading