Provider2 upgrade state rewrite

[VenelinMartinov]

fixes #1916
fixes #1914

This rewrites the provider2 upgrade state implementation to use the upstream GRPC method instead of the bridge implementation. This should cut out a bunch of unnecessary code and some type conversions of the data.

Originally done as part of #1971 but I'm pulling this out to merge separately.

@t0yv0 did a lot of the heavy lifting here.

[t0yv0]

Let's confirm this code is covered by tests?

I'd like to get this in quickly as it demonstrably reduces adapter bugs for PlanResourceChange resources. Hoping this is isolated enough that no downstream check failures will be triggered but we should find out.

Thank you!

[codecov]

Codecov Report

Attention: Patch coverage is 66.66667% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 60.85%. Comparing base (67fc311) to head (b2a8a00).

Files	Patch %	Lines
pkg/tfshim/sdk-v2/provider2.go	66.66%	9 Missing and 3 partials ⚠️
pkg/tests/cross-tests/input_check.go	66.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1998      +/-   ##
==========================================
+ Coverage   60.82%   60.85%   +0.02%     
==========================================
  Files         332      332              
  Lines       44727    44754      +27     
==========================================
+ Hits        27206    27235      +29     
+ Misses      15998    15994       -4     
- Partials     1523     1525       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[iwahbe]

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
}

[VenelinMartinov]

Which information is lost here?

[t0yv0]

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.

[t0yv0]

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.

[iwahbe]

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.

[t0yv0]

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.

[iwahbe]

I think this is an improvement, and would be happy to merge.

We should have at least basic unit test coverage before merging. If we believe that this fixes any bugs, then the new fixes should be under test.

---

This doesn't help with translating large numbers. To accommodate large numbers, we need to merge #2001 into #1998.

[t0yv0]

I'd actually really like to see an integration test when we're upgrading a resource with

1. no schema change
2. an actual schema change where state ugprader meaningfully compensates

unless we have one test already maybe it's just not seeing into PlanResourceChange flag.

It's important high-level functionality that's great to confirm.

If out of time I can contribute a test here.

[VenelinMartinov]

@iwahbe @t0yv0 I believe I addressed all the comments here and added some unit tests for the upgrade state function which seems to work well.

I tried to also add an integration test but that does not work and I believe it is because of #1667 - we need the old schema in order to convert the state correctly in order to pass it to upgrade state - LMK if this does not seem correct.

Overall, this certainly fixes some issues around resources which don't require an upgrade as shown in the now partly passing input cross tests, so still worth merging even if not all tf state upgrades can work.

[t0yv0]

Do you have a sense if TestPlanResourceChangeStateUpgrade doesn't work because of changes in this PR or it's something that didn't work before either?

[VenelinMartinov]

TestPlanResourceChangeStateUpgrade did not work before either and the changes here did not change it.

Unfortunately it fails during

pulumi-terraform-bridge/pkg/tfshim/sdk-v2/provider2.go

Line 49 in 012ac65

return nil, fmt.Errorf("InstanceState: %v", err)

which happens before UpgradeState is called.

[t0yv0]

Very unfortunate it's a pre-existing condition. I think historically state upgraders did generally work and slightly unfortunate we might have introduced rigidity to make them less likely to work. But onwards and upwards, once we have the proper handling of RawState they will work precisely. Thank you!

[t0yv0]

recoverAndCoerceCtyValueWithSchema failure: please note makeResourceRawConfigClassic.

This is used as a fallback for these failures.

I think I was initially very skeptical of this as it feels like an imprecise method, but if we're choosing between failing entirely and using an imprecise method, in a system like this having an imprecise fallback that increases the probability of a program working buys us time until we have retrofitted precision into the system.

Consider adding makeResourceRawConfigClassic as fallback for provider2 as well? Might turn some upgrade tests into passing tests.

[VenelinMartinov]

@t0yv0 great suggestion on the fallback, I've added it in #2002, let's discuss if we should have that there!