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

Fix #1020: accessing GetRawPlan no longer panics #1033

Merged
merged 6 commits into from
Apr 25, 2023
Merged

Fix #1020: accessing GetRawPlan no longer panics #1033

merged 6 commits into from
Apr 25, 2023

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Apr 25, 2023 •
edited
Loading

Fixes #1020

There was a problem with resources accessing GetRawPlan in DiffSuppressFunc; in Pulumi this plan was nil causing the provider to panic. TF CLI populates this plan via PlanState operation. Ultimately our plan is to do the same in Pulumi bridged providers, but that work is currently feature-flagged as we work through potential issues. For now a good workaround is to assume RawPlan=RawConfig instead of passing nil.

I've tested pulumi-aws built against this change and it is able to create, update and delete the affected resource without a panic.

Test program:

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

new aws.wafv2.IpSet("ip6_sample", {
    addresses: ["1.2.3.4/32", "5.6.7.9/32"],
    description: "Step to reproduce issue",
    ipAddressVersion: "IPV4",
    scope: "REGIONAL",
});

I will follow up with a separate PR duplicating the tests for the DiffStrategy=PlanState branch.

t0yv0
t0yv0 commented Apr 25, 2023
View reviewed changes
pkg/tests/regress_1020_test.go Outdated
Elem: &schema.Schema{Type: schema.TypeString},
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
if d.GetRawPlan().IsNull() {
panic(fmt.Sprintf("%s", "NULL GetRawPlan"))
Copy link
Member Author

Choose a reason for hiding this comment

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

d.GetRawPlan() is null but should not be, therefore d.GetRawPlan().GetAttr("addresses") panics.

Digging further.

Copy link
Member Author

Choose a reason for hiding this comment

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

#1021 is probably related.

@github-actions
Copy link

Diff for pulumi-random with merge commit d2b0f48

@github-actions
Copy link

Diff for pulumi-azuread with merge commit d2b0f48

@github-actions
Copy link

Diff for pulumi-gcp with merge commit d2b0f48

@github-actions
Copy link

Diff for pulumi-azure with merge commit d2b0f48

@github-actions
Copy link

Diff for pulumi-azuread with merge commit b5fae22

@github-actions
Copy link

Diff for pulumi-random with merge commit b5fae22

@github-actions
Copy link

Diff for pulumi-gcp with merge commit b5fae22

@github-actions
Copy link

Diff for pulumi-azure with merge commit b5fae22

@github-actions
Copy link

Diff for pulumi-random with merge commit 89826d3

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 89826d3

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 89826d3

@github-actions
Copy link

Diff for pulumi-azure with merge commit 89826d3

@t0yv0 t0yv0 changed the title Write a test for the panic in #1020 Fixes #1020: accessing GetRawPlan no longer panics Apr 25, 2023
@github-actions
Copy link

Diff for pulumi-azuread with merge commit 4371d96

@github-actions
Copy link

Diff for pulumi-random with merge commit 4371d96

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 1f14378

@github-actions
Copy link

Diff for pulumi-random with merge commit 1f14378

@t0yv0 t0yv0 marked this pull request as ready for review April 25, 2023 18:47
@t0yv0 t0yv0 requested a review from a team April 25, 2023 18:47
@github-actions
Copy link

Diff for pulumi-gcp with merge commit 4371d96

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 1f14378

@github-actions
Copy link

Diff for pulumi-azure with merge commit 4371d96

@t0yv0 t0yv0 changed the title Fixes #1020: accessing GetRawPlan no longer panics Fix #1020: accessing GetRawPlan no longer panics Apr 25, 2023
@github-actions
Copy link

Diff for pulumi-azure with merge commit 1f14378

iwahbe
iwahbe approved these changes Apr 25, 2023
View reviewed changes
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

LGTM

@t0yv0 t0yv0 merged commit 834da2c into master Apr 25, 2023
@t0yv0 t0yv0 deleted the t0yv0/fix-1020 branch April 25, 2023 22:24
@t0yv0 t0yv0 mentioned this pull request Apr 27, 2023
Merged
@t0yv0 t0yv0 mentioned this pull request Aug 7, 2023
Closed
@t0yv0 t0yv0 mentioned this pull request Jan 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iwahbe iwahbe iwahbe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

WAF IpSet broken with IPV6 for Cloudfront Distribution on v5.35
2 participants
@t0yv0 @iwahbe