Integrate with PlanResourceChange and ApplyResourceChange #1614

t0yv0 · 2024-01-08T21:08:04Z

sdk-v2 bridge has a new option that changes the implementation of resource lifecycle to go through PlanResourceChange and ApplyResourceChange TF SDKv2 gRPC methods.

// Selectively opt-in resources that pass the filter to using PlanResourceChange. Resources are
// identified by their TF type name such as aws_ssm_document.
func WithPlanResourceChange(filter func(tfResourceType string) bool) providerOption { //nolint:revive

Enables fixing: pulumi/pulumi-aws#2555

Known differences: state returned by the new method will include explicit entries "foo": null for properties that are known by the schema but not set in the state. This seems to be benign for repeated diffs and refresh applications.

t0yv0 · 2024-01-08T21:13:03Z

ignoreChanges has a bit of a chicken-and-egg problem because of how tfbridge code is written, e.g. Update.

	diff, err := p.tfc().DiffWithContext(ctx, res.TFName, state, config)
	if err != nil {
		return nil, errors.Wrapf(err, "diffing %s", urn)
	}
	if diff == nil {
		// It is very possible for us to get here with a nil diff: custom diffing behavior, etc. can cause
		// textual/structural changes not to be semantic changes. A better solution would be to change the result of
		// Diff to indicate no change, but that is a slightly riskier change that we'd rather not take at the current
		// moment.
		return &pulumirpc.UpdateResponse{Properties: req.GetOlds()}, nil
	}

	// Apply any ignoreChanges before we check that the diff doesn't require replacement or deletion since we may be
	// ignoring changes to the keys that would result in replacement/deletion.
	doIgnoreChanges(ctx, res.TF.Schema(), res.Schema.Fields, olds, news, req.GetIgnoreChanges(), diff)

	if diff.Destroy() || diff.RequiresNew() {
		return nil, fmt.Errorf("internal: expected diff to not require deletion or replacement"+
			" during Update of %s. Found delete=%t, replace=%t. This indicates a bug in provider.",
			urn, diff.Destroy(), diff.RequiresNew())
	}

	if req.Timeout != 0 {
		diff.SetTimeout(req.Timeout, shim.TimeoutUpdate)
	}

Currently PlanResourceChanges computes the terraform.InstanceDiff object AND computes the planned cty.Value that's the result of applying that diff; technically processing ignore changes (or timeouts) should modify BOTH, but this is awkward in the current API. Possibly we can refactor a bit so we push ignoreChanges processor to DiffWithContext. Or else switch this out entirely to use Pulumi-level ignoreChanges like for PF. Tricky.

t0yv0 · 2024-01-08T21:18:08Z

I need to check in an additional patch to go-sdk, missed it with go.work.

t0yv0 · 2024-01-08T21:37:21Z

pulumi/terraform-plugin-sdk#34 the necessary change.

VenelinMartinov · 2024-01-16T13:03:40Z

This sounds pretty promising to me since it'd allow us to get rid of some of our custom behaviour there. LMK if you need any help to push this through.

iwahbe

Sanity checking my understanding of the implementation, we introduce a new pkg/tfshim Provider called v2Provider2, which thinks in terms of PlanResourceChange and ApplyResourceChange. We also introduce a binary muxer that switches on resource name called providerWithPlanResourceChange.

I think the overall strategy is good. Do you have a sense of how much work is needed to ship for pulumi/pulumi-aws#2555?

Currently PlanResourceChanges computes the terraform.InstanceDiff object AND computes the planned cty.Value that's the result of applying that diff; technically processing ignore changes (or timeouts) should modify BOTH, but this is awkward in the current API. Possibly we can refactor a bit so we push ignoreChanges processor to DiffWithContext. Or else switch this out entirely to use Pulumi-level ignoreChanges like for PF. Tricky.

I'm not aware of any customer pushback on the Pulumi-level ignoreChanges, so my inclination is to do that. (for simplicity and consistency).

pkg/tfshim/sdk-v2/provider2.go

iwahbe · 2024-01-18T21:57:47Z

pkg/tfshim/sdk-v2/provider2.go

+	id string, object, meta map[string]interface{},
+) (shim.InstanceState, error) {
+	if _, gotID := object["id"]; !gotID && id != "" {
+		copy := map[string]interface{}{}


Slight performance optimization to pre-allocate space in the map.

Suggested change

copy := map[string]interface{}{}

copy := make(map[string]interface{}, len(meta))

pkg/tfshim/sdk-v2/provider2.go

pkg/tfshim/sdk-v2/provider_options.go

t0yv0 · 2024-01-18T22:32:00Z

Do you have a sense of how much work is needed to ship for pulumi/pulumi-aws#2555?

Optimistically there's 2 days left to work out ignoreChanges and refresh concerns, after which we can enable it in pulumi-aws for this resource only.

t0yv0 · 2024-01-24T20:44:53Z

Unfortunately, eh, the refresh and import support turn out to be quite coupled to the state representation, so they need to be part of the change. I'm adding some code to do something sensible but it adds to what needs to be tested out.

Still on the list - this needs to do state upgrades still and do timeouts correctly.

The intent here is to refactor the code without any observable changes to make possible further extensions such as #1614 . Specifically, before this PR: ```go type Provider interface { ... Diff(t string, s InstanceState, c ResourceConfig) (InstanceDiff, error) } type InstanceDiff interface { ... IgnoreChanges(ignored map[string]bool) } ``` That is the interface is to compute a diff first, and then side-effect to ignore changes. After this PR we have this instead: ```go type Provider interface { ... Diff(t string, s InstanceState, c ResourceConfig, opts DiffOptions) (InstanceDiff, error) } type IgnoreChanges = func() map[string]struct{} type DiffOptions struct { IgnoreChanges IgnoreChanges } ``` So the algorithm to ignore changes is specified upfront when constructing the diff. This seems to be useful to un-constrain the implementation of shim.Provider a little bit, specifically in #1614 that is trying to change the internal representation of InstanceDiff implementation in such a way that it is inconvenient to process IgnoreChanges after the InstanceDiff is constructed already.

t0yv0 · 2024-01-25T21:50:01Z

I'm not aware of any customer pushback on the Pulumi-level ignoreChanges, so my inclination is to do that. (for simplicity and consistency).

That's probably because the user base for PF-based providers is still insignificant. I'm retaining the current ignoreChanges model, we can contemplate a change as a separate work item.

t0yv0 · 2024-01-26T22:04:21Z

pkg/tfbridge/provider_test.go

-	outs, err := plugin.UnmarshalProperties(createResp.GetProperties(), plugin.MarshalOptions{KeepUnknowns: true})
+	outs, err := plugin.UnmarshalProperties(createResp.GetProperties(), plugin.MarshalOptions{
+		KeepUnknowns: true,
+		SkipNulls:    true,


This was interesting, without SkipNulls: true the PlanResourceChange version returns explicit null entries for every property in the schema that's unset.

t0yv0 · 2024-01-26T22:04:41Z

pkg/tfbridge/provider_test.go

 	assert.NoError(t, err)
-	assert.True(t, resource.PropertyMap{
-		"id":                  resource.NewStringProperty(""),


The PlanResourceChange version returns an unknown for "id", but that seems like may be OK.

t0yv0 · 2024-01-26T22:05:02Z

pkg/tfbridge/provider_test.go

-		"id":                  resource.NewStringProperty(""),
-		"stringPropertyValue": resource.NewStringProperty("foo"),
-		"setPropertyValues":   resource.NewArrayProperty([]resource.PropertyValue{resource.NewStringProperty("foo")}),
-	}.DeepEquals(outs))


Also the PlanResourceChange version returns __meta here with resource timeouts populated. That seems to be a change.

t0yv0 · 2024-01-26T22:05:33Z

pkg/tfbridge/provider_test.go

+
+func TestIgnoreChangesV2WithPlanResourceChange(t *testing.T) {
+	opt := shimv2.WithPlanResourceChange(func(string) bool { return true })
+	testIgnoreChangesV2(t, shimv2.NewProvider(testTFProviderV2, opt))


Starting to bring this option into the test matrix to get some coverage..

t0yv0 · 2024-01-29T17:03:59Z

Testing import was fruitful, some tweaks needed to handle not-found resource. I'll have to look into adding coverage for refresh and import, found and not found.

t0yv0 · 2024-01-29T19:51:01Z

Got most of the provider2 covered, Remaining coverage :

test emulating 2555 specifically
recoverDiagnostic; diagnostic validity
state upgrades
timeouts

Manual testing: testing 2555 scenario on top of the fixes; also testing import and refresh.

VenelinMartinov · 2024-01-29T20:04:18Z

pkg/tfbridge/provider.go

@@ -1329,7 +1329,7 @@ func (p *Provider) Delete(ctx context.Context, req *pulumirpc.DeleteRequest) (*p
 	}

 	// Create a new destroy diff.
-	diff := p.tf.NewDestroyDiff(ctx, string(t), shim.TimeoutOptions{


This looks like a fairly big change? Isn't urn.Type() a pulumi type, while this requires a TF type?
What does this affect?

It's a bug introduced recently in 39a92a4 specifically to unlock this PR and previously unused/untested.

Looks like a refactor to me wrt the t parameter - 39a92a4#diff-1dee36d7f4752b488fc6d27ff19bd5e68277063de183d75ef29a02b8bf989045R1332

I don't see how that commit introduces a bug there?

The refactor wasn't tested or used. NewDestroyDiff under default options ignores t. The intent though was to pass the Terraform resource name in that parameter, similarly to other methods on shim.Provider. Not the Pulumi type.

VenelinMartinov · 2024-01-29T20:15:35Z

pkg/tfbridge/provider_test.go

+	})
+}
+
+func TestRefresh(t *testing.T) {


These are the first import and refresh tests, right?

TestImport tries to emulate what happens during pulumi import, and TestRefresh during pulumi refresh. Both map to shim.Provider Read method but it's a bit difficult for me to remember which is which. Having explicit in the tests remind me. This makes sure to test both branches of isRefresh bool inside Read. Prior to this test, the not found cases were not covered by tests at all.

Motivation: pulumi/pulumi-terraform-bridge#1614 The bridge is trying to integrate deeper with (*GRPCProviderServer) PlanResourceChange so this patch makes it a little easier: expose the constructed InstanceDiff object apply Pulumi-specified ignore changes to the InstanceDiff object before it gets used to compute the plan

t0yv0 requested review from iwahbe and VenelinMartinov January 8, 2024 21:13

iwahbe reviewed Jan 18, 2024

View reviewed changes

This was referenced Jan 23, 2024

Refactor IgnoreChanges #1639

Merged

Simplify accessing PlanResourceChange pulumi/terraform-plugin-sdk#35

Merged

t0yv0 force-pushed the t0yv0/sdk-v2-shim-with-plan-apply branch from 0c38c0c to 46f4cf1 Compare January 24, 2024 18:16

t0yv0 changed the base branch from master to t0yv0/refactor-ignore-changes January 24, 2024 18:17

t0yv0 force-pushed the t0yv0/refactor-ignore-changes branch 2 times, most recently from d223e57 to 59c21dc Compare January 25, 2024 20:20

Base automatically changed from t0yv0/refactor-ignore-changes to master January 25, 2024 20:42

t0yv0 force-pushed the t0yv0/sdk-v2-shim-with-plan-apply branch 3 times, most recently from 72f0661 to 57c17db Compare January 25, 2024 22:19

t0yv0 changed the base branch from master to t0yv0/refactor-timeouts January 25, 2024 22:19

t0yv0 force-pushed the t0yv0/refactor-timeouts branch from 3fbf0e7 to 1b7d17b Compare January 25, 2024 22:20

t0yv0 force-pushed the t0yv0/sdk-v2-shim-with-plan-apply branch from 57c17db to 220a70e Compare January 25, 2024 22:21

Base automatically changed from t0yv0/refactor-timeouts to master January 26, 2024 15:36

t0yv0 force-pushed the t0yv0/sdk-v2-shim-with-plan-apply branch from c89e1f4 to 04d0dab Compare January 26, 2024 16:02

t0yv0 commented Jan 26, 2024

View reviewed changes

t0yv0 added 9 commits January 29, 2024 12:05

Toward bridging with PlanResourceChange/ApplyResourceChange

33ca507

Simplify DestroyDiff handling

2f93a4b

Handle timeouts correctly in the diff

c04631f

Ensure to handle and log diagnostics

ee0aa0a

Update the SDKv2 edited dependency everywhere

9f42c29

Start bringing code under test

438ab32

Lint

61d969e

Make sure unrelated tests do not regress

5a30925

Fix import handling of nont-found

6fe2658

t0yv0 force-pushed the t0yv0/sdk-v2-shim-with-plan-apply branch from 8a61ec0 to 6fe2658 Compare January 29, 2024 17:05

t0yv0 added 5 commits January 29, 2024 14:08

Add import tests, remove timeout key

ea40078

Cover refresh found and not found scenarios

8c30d63

Lint

b16fca4

Remove unused

6618ebe

PR feedback

5ded4d9

t0yv0 marked this pull request as ready for review January 29, 2024 19:49

t0yv0 changed the title ~~WIP: integrate with PlanResourceChange and ApplyResourceChange~~ Integrate with PlanResourceChange and ApplyResourceChange Jan 29, 2024

t0yv0 requested a review from iwahbe January 29, 2024 19:51

Cover destroy

50a0465

VenelinMartinov approved these changes Jan 29, 2024

View reviewed changes

VenelinMartinov reviewed Jan 29, 2024

View reviewed changes

Update code reference

aed0f9c

t0yv0 enabled auto-merge (squash) January 29, 2024 21:07

t0yv0 merged commit e91ed7e into master Jan 29, 2024
7 checks passed

t0yv0 deleted the t0yv0/sdk-v2-shim-with-plan-apply branch January 29, 2024 21:26

t0yv0 mentioned this pull request Jan 30, 2024

[PF] Call Read after ImportState when importing a resource #1659

Merged

t0yv0 mentioned this pull request Mar 22, 2024

[EPIC] Rollout PlanResourceChange #1785

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate with PlanResourceChange and ApplyResourceChange #1614

Integrate with PlanResourceChange and ApplyResourceChange #1614

t0yv0 commented Jan 8, 2024 •

edited

Loading

t0yv0 commented Jan 8, 2024

t0yv0 commented Jan 8, 2024

t0yv0 commented Jan 8, 2024

VenelinMartinov commented Jan 16, 2024

iwahbe left a comment

iwahbe Jan 18, 2024

t0yv0 commented Jan 18, 2024

t0yv0 commented Jan 24, 2024

t0yv0 commented Jan 25, 2024

t0yv0 Jan 26, 2024

t0yv0 Jan 26, 2024

t0yv0 Jan 26, 2024

t0yv0 Jan 26, 2024

t0yv0 commented Jan 29, 2024

t0yv0 commented Jan 29, 2024 •

edited

Loading

VenelinMartinov Jan 29, 2024

t0yv0 Jan 29, 2024

VenelinMartinov Jan 29, 2024

t0yv0 Jan 29, 2024

VenelinMartinov Jan 29, 2024

t0yv0 Jan 29, 2024

	copy := map[string]interface{}{}
	copy := make(map[string]interface{}, len(meta))

Integrate with PlanResourceChange and ApplyResourceChange #1614

Integrate with PlanResourceChange and ApplyResourceChange #1614

Conversation

t0yv0 commented Jan 8, 2024 • edited Loading

t0yv0 commented Jan 8, 2024

t0yv0 commented Jan 8, 2024

t0yv0 commented Jan 8, 2024

VenelinMartinov commented Jan 16, 2024

iwahbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented Jan 18, 2024

t0yv0 commented Jan 24, 2024

t0yv0 commented Jan 25, 2024

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

t0yv0 commented Jan 29, 2024

t0yv0 commented Jan 29, 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

t0yv0 commented Jan 8, 2024 •

edited

Loading

t0yv0 commented Jan 29, 2024 •

edited

Loading