Introduce InstanceStateStrategy CtyInstanceState flag to fix pulumi-aws#1423 #887

t0yv0 · 2023-03-13T20:36:30Z

It appears that func (s v2InstanceState) Object is using customized github.com/pulumi/terraform-diff-reader/sdk-v2 code that is behind upstream TF source edits, and is causing pulumi-aws to misbehave. In particular when it reads and applies a diff like the following:

    rule.1860330423.name ResourceAttrDiff{New: "josh-test-aws-rules"}
    rule.1860330423.override_action.# ResourceAttrDiff{New: "1"}
    rule.1860330423.override_action.0.count.# ResourceAttrDiff{New: "1"}
    rule.1860330423.override_action.0.none.# ResourceAttrDiff{New: "0"}

it incorrectly decides to populate count with a nil value instead of the expected value of an empty object {}. This cause confusion. Switching to more higher-level API from TF SDKv2 resolves the problem.

However, using this API seems to break some other tests in the repo and causes several observable differences. For example, Pulumi starts returning explicit "foo": null fields for objects (distinct from maps) when it previously elided them. It also causes some set element ordering issues.

Given that it is not a clean replacement, opting to be conservative and feature-flagging the change in a way that it can be activated for a single resource only.

I've tested pulumi/pulumi-aws#1423 on top of this change plus a one-line configuration in resources.go and verified that the problem is resoled, moreover the resource responds correctly to further edits. Configuration line:

	shimv2.SetInstanceStateStrategy(prov.P.ResourcesMap().Get("aws_wafv2_web_acl"), sdkv2.CtyInstanceState)

Previous attempt to fix 1423 by editing terraform-diff-reader/sdk-v2 did not work end to end and is reverted here.

github-actions · 2023-03-13T20:42:28Z

Diff for pulumi-random with merge commit f09578e

github-actions · 2023-03-13T20:43:33Z

Diff for pulumi-azuread with merge commit f09578e

github-actions · 2023-03-13T20:52:49Z

Diff for pulumi-azuread with merge commit 514ba39

github-actions · 2023-03-13T20:52:51Z

Diff for pulumi-gcp with merge commit f09578e

github-actions · 2023-03-13T20:52:55Z

Diff for pulumi-random with merge commit 514ba39

github-actions · 2023-03-13T20:56:58Z

Diff for pulumi-azure with merge commit f09578e

github-actions · 2023-03-13T21:01:12Z

Diff for pulumi-gcp with merge commit 514ba39

github-actions · 2023-03-13T21:05:41Z

Diff for pulumi-random with merge commit 970e172

github-actions · 2023-03-13T21:07:31Z

Diff for pulumi-azuread with merge commit 970e172

github-actions · 2023-03-13T21:09:39Z

Diff for pulumi-azure with merge commit 514ba39

github-actions · 2023-03-13T21:15:44Z

Diff for pulumi-gcp with merge commit 970e172

t0yv0 · 2023-03-13T21:19:11Z

From tracing SetHash

diffToShim rule.283902798.override.0.count.# => ..
diffToShim rule.283902798.priority => ..
diffToShim rule.1219255360.name => ..
diffToShim rule.283902798.override.# => ..
diffToShim rule.1219255360.override.0.none.# => ..
diffToShim rule.1219255360.priority => ..
diffToShim rule.283902798.override.0.none.# => ..
diffToShim rule.283902798.name => ..
diffToShim rule.1219255360.override.# => ..
diffToShim rule.1219255360.override.0.count.# => ..
SetHash(map[name:josh-test-aws-rules override:[<nil>] priority:1]) => 487022137
SetHash(map[name:josh-test-aws-rules override:[map[count:[<nil>] none:[]]] priority:1]) => 283902798
SetHash(map[name:josh-test-aws-rules override:[<nil>] priority:1]) => 487022137
SetHash(map[name:josh-test-aws-rules override:[map[count:[<nil>] none:[]]] priority:1]) => 283902798
SetHash(map[name:josh-test-aws-rules override:[<nil>] priority:1]) => 487022137

t0yv0 · 2023-03-13T21:21:00Z

If I replace Set Hash function with a constant value, we get the desired diff None, so I think this is a bug about set element hashing, especially influenced by the override_action parameter in the schema, that affects the hash. This parameter is a little weird. A list of objects of emptySchema. I'm guessing there's a discrepancy in how TF vs Pulumi computes set hashes for these.

github-actions · 2023-03-13T21:27:54Z

Diff for pulumi-azure with merge commit 970e172

thomas11

A test is a great start - any reason it's a draft PR?

t0yv0 · 2023-03-14T14:16:14Z

I thought maybe we can find a quick fix! But it's not so easy :)

github-actions · 2023-04-25T19:16:06Z

Diff for pulumi-random with merge commit 76d8da3

github-actions · 2023-04-25T19:17:43Z

Diff for pulumi-azuread with merge commit 76d8da3

github-actions · 2023-04-25T19:27:27Z

Diff for pulumi-gcp with merge commit 76d8da3

github-actions · 2023-04-25T19:33:12Z

Diff for pulumi-azure with merge commit 76d8da3

t0yv0 · 2023-04-25T20:49:55Z

Debugging this a little further. @danielrbradley if you're interested.

This till reproduces. I think the bug is actually in Create method. We send this to create

          "overrideAction": {
            "__defaults": [],
            "count": {
              "__defaults": []
            }
          },

But it sends this back:

          "overrideAction": {
            "count": null,
            "none": null
          },

That's different. That's as if count property wasn't set.

My program is

import * as aws from "@pulumi/aws";

const args: aws.wafv2.WebAclArgs = {
    scope: "REGIONAL",
    rules: [
        {
            name: "rule-1",
            priority: 1,
            overrideAction: {
                count: {},
            },
            statement: {
                managedRuleGroupStatement: {
                    name: "AWSManagedRulesCommonRuleSet",
                    vendorName: "AWS",
                }
            },
            visibilityConfig: {
                cloudwatchMetricsEnabled: false,
                metricName: "friendly-rule-metric-name",
                sampledRequestsEnabled: false,
            },
        }
    ],
    defaultAction: {
        allow: {},
    },
    visibilityConfig: {
        cloudwatchMetricsEnabled: false,
        metricName: "friendly-rule-metric-name",
        sampledRequestsEnabled: false,
    },
};

const acl = new aws.wafv2.WebAcl("aclw", args);

export const arn = acl.arn;

pkg/tfshim/sdk-v2/cty.go

t0yv0 · 2023-04-26T02:01:05Z

pkg/tfshim/sdk-v2/instance_state.go

@@ -36,68 +37,41 @@ func (s v2InstanceState) ID() string {
 }

 func (s v2InstanceState) Object(sch shim.SchemaMap) (map[string]interface{}, error) {


This method wan't doing quite the right thing. The empty object value ended up parsed as nil. This caused problems because a List[EmptyObj] value under MaxItems=1 flattening got translated to nil which is indistinguishable from a nil list.

The new implementation leans into TF primitives. I wonder how to ramp up testing to know we can make this replacement.

The new implementation trips up some tests because it now generates "foo": nil entries in maps instead of eliding them.

github-actions · 2023-04-26T02:01:35Z

Diff for pulumi-random with merge commit a69f697

github-actions · 2023-04-26T02:02:17Z

Diff for pulumi-azuread with merge commit a69f697

github-actions · 2023-04-26T02:16:13Z

Diff for pulumi-gcp with merge commit a69f697

github-actions · 2023-04-26T02:17:27Z

Diff for pulumi-azure with merge commit a69f697

github-actions · 2023-04-26T02:47:18Z

Diff for pulumi-azuread with merge commit 93dabc7

github-actions · 2023-06-28T03:11:48Z

Diff for pulumi-azuread with merge commit 9436cec

github-actions · 2023-06-28T03:12:32Z

Diff for pulumi-random with merge commit 9436cec

github-actions · 2023-06-28T03:17:43Z

Diff for pulumi-azuread with merge commit b8875b7

github-actions · 2023-06-28T03:18:20Z

Diff for pulumi-random with merge commit b8875b7

t0yv0 · 2023-06-28T03:20:23Z

Opting to flag it so we can deploy the fix for the motivating issue, without taking the risk wholesale.

t0yv0 · 2023-06-28T03:38:15Z

https://gist.github.com/t0yv0/3c25627a43e5326ccfd176f7e481f02c temp investigation of TF expected behavior in a unit test.

github-actions · 2023-06-28T12:58:57Z

Diff for pulumi-azuread with merge commit d5cbecb

pkg/tfshim/sdk-v2/instance_state.go

pkg/tfshim/sdk-v2/instance_state_read_strategy.go

iwahbe · 2023-06-28T16:19:21Z

pkg/tfshim/sdk-v2/instance_state_read_strategy.go

+		return ClassicInstanceState, false, nil
+	}
+	p, err := ParseInstanceStateStrategy(s)
+	return p, err == nil, err


It feels weird to have all 3 values returned here. Why is (InstanceStateStrategy, error) not enough?

There's a difference between env var not being set and env var being set to a value that can't be parsed. Copied from ParseDiffStrategyFromEnv.

github-actions · 2023-06-28T17:41:44Z

Diff for pulumi-azuread with merge commit d0e38f6

github-actions · 2023-06-28T17:44:40Z

Diff for pulumi-azuread with merge commit af3a979

t0yv0 · 2023-06-28T17:45:14Z

@iwahbe addressed the comments, ready for another look.

github-actions · 2023-06-28T17:48:51Z

Diff for pulumi-azuread with merge commit 7afb6ab

AaronFriel · 2023-06-28T20:11:10Z

pkg/tests/regress_aws_1423_test.go

@@ -107,6 +107,8 @@ func TestRegressAws1423(t *testing.T) {
 		[]byte{}, /* pulumiSchema */
 	)

+	shimv2.SetInstanceStateStrategy(p.ResourcesMap().Get("aws_wafv2_web_acl"), shimv2.CtyInstanceState)


Do we need to regenerate the test case, or should we not expect a diff below this line?

The test case does discriminate the functionality. It fails the test once the changes to undo the previous non-fix made to pulumi-terraform-diff-reader are made. It's incomplete in a sense that it couldn't discriminate the pulumi-terraform-diff-reader non-fix from a real fix.

thomas11 approved these changes Mar 14, 2023

View reviewed changes

t0yv0 force-pushed the t0yv0/repro-aws-1423 branch from 8c62f41 to 9d104b0 Compare April 25, 2023 19:12

t0yv0 changed the title ~~Regress 1423~~ WIP: fix pulumi-aws#1423 Apr 26, 2023

t0yv0 commented Apr 26, 2023

View reviewed changes

pkg/tfshim/sdk-v2/cty.go Outdated Show resolved Hide resolved

t0yv0 commented Apr 26, 2023

View reviewed changes

t0yv0 force-pushed the t0yv0/repro-aws-1423 branch from 9ff0379 to 09822b0 Compare June 28, 2023 03:06

Fix tests

e64f488

Lint

13bd478

t0yv0 marked this pull request as ready for review June 28, 2023 03:20

t0yv0 changed the title ~~WIP: fix pulumi-aws#1423~~ Introduce InstanceStateStrategy CtyInstanceState flag to fix pulumi-aws#1423 Jun 28, 2023

t0yv0 requested review from AaronFriel and aq17 June 28, 2023 03:22

t0yv0 added 2 commits June 28, 2023 05:49

Merge master

70f06c2

go mod tidy

d00f35b

t0yv0 requested a review from iwahbe June 28, 2023 13:52

iwahbe reviewed Jun 28, 2023

View reviewed changes

t0yv0 added 2 commits June 28, 2023 10:37

Revert change to exported func

c0bade9

PR feedback

b8a542a

PR feedback

8a069b6

AaronFriel reviewed Jun 28, 2023

View reviewed changes

AaronFriel approved these changes Jun 28, 2023

View reviewed changes

t0yv0 merged commit ccaffb7 into master Jun 28, 2023

t0yv0 deleted the t0yv0/repro-aws-1423 branch June 28, 2023 20:43

t0yv0 mentioned this pull request Jun 30, 2023

Fix pulumi-aws#1423 pulumi/pulumi-aws#2581

Closed

		@@ -36,68 +37,41 @@ func (s v2InstanceState) ID() string {
		}

		func (s v2InstanceState) Object(sch shim.SchemaMap) (map[string]interface{}, error) {

Introduce InstanceStateStrategy CtyInstanceState flag to fix pulumi-aws#1423 #887

Introduce InstanceStateStrategy CtyInstanceState flag to fix pulumi-aws#1423 #887

Conversation

t0yv0 commented Mar 13, 2023 • edited Loading

github-actions bot commented Mar 13, 2023

github-actions bot commented Mar 13, 2023

github-actions bot commented Mar 13, 2023

github-actions bot commented Mar 13, 2023

github-actions bot commented Mar 13, 2023

github-actions bot commented Mar 13, 2023

github-actions bot commented Mar 13, 2023

github-actions bot commented Mar 13, 2023

github-actions bot commented Mar 13, 2023

github-actions bot commented Mar 13, 2023

github-actions bot commented Mar 13, 2023

t0yv0 commented Mar 13, 2023

t0yv0 commented Mar 13, 2023

github-actions bot commented Mar 13, 2023

thomas11 left a comment

Choose a reason for hiding this comment

t0yv0 commented Mar 14, 2023

github-actions bot commented Apr 25, 2023

github-actions bot commented Apr 25, 2023

github-actions bot commented Apr 25, 2023

github-actions bot commented Apr 25, 2023

t0yv0 commented Apr 25, 2023

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 Apr 26, 2023

github-actions bot commented Apr 26, 2023

github-actions bot commented Apr 26, 2023

github-actions bot commented Apr 26, 2023

github-actions bot commented Apr 26, 2023

github-actions bot commented Jun 28, 2023

github-actions bot commented Jun 28, 2023

github-actions bot commented Jun 28, 2023

github-actions bot commented Jun 28, 2023

t0yv0 commented Jun 28, 2023

t0yv0 commented Jun 28, 2023

github-actions bot commented Jun 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 28, 2023

github-actions bot commented Jun 28, 2023

t0yv0 commented Jun 28, 2023

github-actions bot commented Jun 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented Mar 13, 2023 •

edited

Loading