Skip to content
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

Jump to bottom

Provider2 upgrade state rewrite #1998

Merged
merged 7 commits into from
May 21, 2024
Merged

Provider2 upgrade state rewrite #1998

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d3aa2e3
provider2 upgrade state rewrite
VenelinMartinov May 20, 2024
8933992
address review comments
VenelinMartinov May 21, 2024
b2b198f
upgrade state tests
VenelinMartinov May 21, 2024
899ba31
towards an integration test
VenelinMartinov May 21, 2024
f726145
remove max items one, add version meta
VenelinMartinov May 21, 2024
2ff4869
add todos
VenelinMartinov May 21, 2024
b2a8a00
unskip cross tests
VenelinMartinov May 21, 2024
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
66 changes: 53 additions & 13 deletions pkg/tfshim/sdk-v2/provider2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"encoding/json"
"fmt"
"strconv"

"github.com/hashicorp/go-cty/cty"
"github.com/hashicorp/go-cty/cty/msgpack"
Expand Down Expand Up @@ -322,31 +323,70 @@ func (p *planResourceChangeImpl) upgradeState(
) (shim.InstanceState, error) {
res := p.tf.ResourcesMap[t]
state := p.unpackInstanceState(t, s)
instanceState, err := res.ShimInstanceStateFromValue(state.stateValue)

ty := res.CoreConfigSchema().ImpliedType()

jsonIsh, err := schema.StateValueToJSONMap(state.stateValue, ty)
Copy link
Member

Choose a reason for hiding this comment

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

Calling schema.StateValueToJSONMap looses information. The implementation of schema.StateValueToJSONMap is:

// StateValueToJSONMap converts a cty.Value to generic JSON map via the cty JSON
// encoding.
func StateValueToJSONMap(val cty.Value, ty cty.Type) (map[string]interface{}, error) {
	js, err := ctyjson.Marshal(val, ty)
	if err != nil {
		return nil, err
	}

	var m map[string]interface{}
	if err := json.Unmarshal(js, &m); err != nil {
		return nil, err
	}

	return m, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which information is lost here?

Copy link
Member

Choose a reason for hiding this comment

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

Appreciate unpacking the concern a bit so we can address!

This is extracted from #1971 which we started taking as far as downstream tests.

I noticed in your previous rewrite @iwahbe you prefer passing the information via flatmap

https://github.com/pulumi/pulumi-terraform-bridge/pull/1735/files#diff-1c55ab32bd51c4f37c224239b498efca8a5b19c33cbb244231a5d6f02d301affR44

IN here we don't have easy access to flatmap but we're working with cty.Value:

type v2InstanceState2 struct {
	resourceType string
	stateValue   cty.Value
	// Also known as private state.
	meta map[string]interface{}
}

If there's a way to go from cty.Value to flatmap that's better than going from cty.Value to json-ish we could use it here.

Copy link
Member

Choose a reason for hiding this comment

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

Also noting that ty := res.CoreConfigSchema().ImpliedType() is not good and we need to revisit this. This is introducing the current provider's schema into the process of serializing inputs for the state upgrader which is not good - something that's going to break big time when the schema is changing.

For now we should consider using the current value's implied schema, at least that is closer to the desired outcome in spirit:

state.stateValue.Type()

Ideally we'd have some flatmap representation that is as-is preserved from the last write to Pulumi state but we do not have that yet without further work.

The fact that this was not caught also makes me think we need a test that exercises actually changing the provider schema in a breaking way and using a state upgrader to compensate for this; this would verify that bridged providers can support this. This test would have probably failed here.

Copy link
Member

Choose a reason for hiding this comment

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

We don't need to use flatmap. I used it previously because that was what we were doing before. We should use JSON for inputs.

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking state.stateValue.Type() can backfire as well as it can't get schema Set functions from schema etc.. Eh, no good options here until #1667 lands. Should at least comment and link that.

if err != nil {
return nil, err
}
// Looks like this definitely can happen, but upgradeResourceState assumes a non-nil map.
if instanceState.Attributes == nil {
instanceState.Attributes = map[string]string{}
}
instanceState.Meta = state.meta
newInstanceState, err := upgradeResourceState(ctx, p.tf, res, instanceState)
jsonBytes, err := json.Marshal(jsonIsh)
if err != nil {
return nil, err
}
if newInstanceState == nil {
return nil, nil

version := int64(0)
if versionValue, ok := state.meta["schema_version"]; ok {
versionString, ok := versionValue.(string)
if !ok {
return nil, fmt.Errorf("unexpected type %T for schema_version", versionValue)
}
v, err := strconv.ParseInt(versionString, 0, 32)
if err != nil {
return nil, err
}
version = v
}
ty := res.CoreConfigSchema().ImpliedType()
stateValue, err := newInstanceState.AttrsAsObjectValue(ty)

//nolint:lll
// https://github.com/opentofu/opentofu/blob/2ef3047ec6bb266e8d91c55519967212c1a0975d/internal/tofu/upgrade_resource_state.go#L52
if version > int64(res.SchemaVersion) {
return nil, fmt.Errorf(
"State version %d is greater than schema version %d for resource %s. "+
"Please upgrade the provider to work with this resource.",
version, res.SchemaVersion, t,
)
}

// Note upgrade is always called, even if the versions match
//nolint:lll
// https://github.com/opentofu/opentofu/blob/2ef3047ec6bb266e8d91c55519967212c1a0975d/internal/tofu/upgrade_resource_state.go#L72

resp, err := p.server.gserver.UpgradeResourceState(ctx, &tfprotov5.UpgradeResourceStateRequest{
TypeName: t,
RawState: &tfprotov5.RawState{JSON: jsonBytes},
Version: version,
})
if err != nil {
return nil, err
}

newState, err := msgpack.Unmarshal(resp.UpgradedState.MsgPack, ty)
if err != nil {
return nil, err
}

newMeta := make(map[string]interface{})
// copy old meta into new meta
for k, v := range state.meta {
newMeta[k] = v
}
newMeta["schema_version"] = strconv.Itoa(res.SchemaVersion)

return &v2InstanceState2{
resourceType: t,
stateValue: stateValue,
meta: newInstanceState.Meta,
stateValue: newState,
meta: newMeta,
}, nil
}

Expand Down