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 a panic when ignoring wildcard values with arrays of different length #16406

Merged
merged 1 commit into from
Jun 17, 2024
Merged

Fix a panic when ignoring wildcard values with arrays of different length #16406

merged 1 commit into from
Jun 17, 2024

Conversation

mikhailshilkov
Copy link
Member

@mikhailshilkov mikhailshilkov commented Jun 14, 2024 •
edited
Loading

The panic occurred during pulumi preview while calculating values for ignored changes with a * wildcard

{ignoreChanges: ["defaultActions[*].forward.targetGroups[*].weight"]}

when compared arrays had different length. We weren't checking the array boundaries carefully, and thus panicing when going out of bounds.

After the fix, the same repro in the issue leads to a preview error

cannot ignore changes to the following properties because one or more elements of the path are missing: "defaultActions[].forward.targetGroups[].weight"

This seems consistent with other code branches and tests, where reset returns false for non-matching shapes. User's expectation may be that we succeed in this case - curious to get a take from ones who implemented the wildcard feature in the past.

Fixes #16403

@mikhailshilkov mikhailshilkov requested a review from a team as a code owner June 14, 2024 21:30
@pulumi-bot
Copy link
Contributor

Changelog

[uncommitted] (2024-06-14)

Bug Fixes

  • [engine] Fix a panic when ignoring wildcard values with arrays of different length
    #16406

@t0yv0
Copy link
Member

t0yv0 commented Jun 14, 2024

Some curious notes:

@iwahbe has reimplemented this algo for the bridge https://github.com/pulumi/pulumi-terraform-bridge/blob/master/unstable/propertyvalue/ignore_changes.go#L24

@t0yv0
Copy link
Member

t0yv0 commented Jun 14, 2024

In the context of the ignoreChanges algorithm, I think this really has no good solution, as in, copying data from state to inputs through arrays of unequal length can't decide how to match up the elements.

IN this case it's actually worse since elements are in fact TF set elements and matching should be done semantically and not by position but that's another conversation.

If ignore changes cannot copy the data accurately warning the user about it is reasonable.

@t0yv0
Copy link
Member

t0yv0 commented Jun 14, 2024

CC @lunaris may be of interest to continue today's conversation on ignoreChanges.

iwahbe
iwahbe approved these changes Jun 14, 2024
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.

I'm amazed this bug persisted for so long.

@mikhailshilkov mikhailshilkov added this pull request to the merge queue Jun 17, 2024
Merged via the queue into master with commit 8b1882c Jun 17, 2024
50 checks passed
@mikhailshilkov mikhailshilkov deleted the mikhailshilkov/fix-properties-path-panic branch June 17, 2024 09:04
@justinvp justinvp mentioned this pull request Jun 20, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jun 21, 2024
@justinvp
Prepare for v3.121.0 release (#16430)
006a95f 
Tentative changelog:

### Features

- [sdk/go] Suggest valid attributes with similar names to unrecognised
ones when validating project definitions
  [#16097](#16097)

- [cli/new] Allow passing runtime options as args in pulumi new
  [#16346](#16346)

- [cli/new] Query language runtime for options during “pulumi new”
  [#16346](#16346)

- [sdk/nodejs] Detect pnpm workspaces when running pulumi install
  [#15525](#15525)

- [sdk/nodejs] Add options to Workspace::removeStack()
  [#16333](#16333)

- [sdk/python] Automatically convert requirements.txt to pyproject.toml
when using Poetry
  [#16346](#16346)

- [sdkgen/python] Generate TypedDict types for inputs
  [#15957](#15957)


### Bug Fixes

- [engine] Fix a panic when ignoring wildcard values with arrays of
different length
  [#16406](#16406)

- [cli/engine] Fix --continue-on-error running indefinitely when a
resource fails to be created or updated
  [#16371](#16371)

- [cli/plugin] Fix plugin install command when plugin type is tool
  [#16407](#16407)

- [sdk/python] Don't incorrectly emit deprecation warnings for
non-deprecated properties
  [#16400](#16400)

- [sdk/python] Handle extra CLI arguments passed policy packs plugins
  [#16402](#16402)

- [sdk/python] Add VIRTUAL_ENV environment variable when running inside
a virtual environment


### Miscellaneous

- [cli/new] Instruct the user to use 'pulumi install' when using
--generate-only
@justinvp justinvp mentioned this pull request Jun 21, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jun 22, 2024
@justinvp
Freeze v3.121.0 (#16447)
12ee8f2 
Waiting on #16441, but there are
also a few other fixes in the queue that I'll opportunistically wait on
as well.

Tentative changelog:

### Features

- [engine] Resolve provider in the engine before passing it to
transforms
  [#16409](#16409)

- [sdk/go] Suggest valid attributes with similar names to unrecognised
ones when validating project definitions
  [#16097](#16097)

- [cli/new] Allow passing runtime options as args in pulumi new
  [#16346](#16346)

- [cli/new] Query language runtime for options during “pulumi new”
  [#16346](#16346)

- [cli/new] Add packagemanager prompt to pulumi new for nodejs
  [#16417](#16417)

- [sdk/nodejs] Detect pnpm workspaces when running pulumi install
  [#15525](#15525)

- [sdk/nodejs] Add options to Workspace::removeStack()
  [#16333](#16333)

- [sdk/python] Automatically convert requirements.txt to pyproject.toml
when using Poetry
  [#16346](#16346)

- [sdkgen/python] Generate TypedDict types for inputs
  [#15957](#15957)


### Bug Fixes

- [engine] Fix a panic when ignoring wildcard values with arrays of
different length
  [#16406](#16406)

- [cli/engine] Fix --continue-on-error running indefinitely when a
resource fails to be created or updated
  [#16371](#16371)

- [sdk/nodejs] Avoid an unhandled error when `dependencies` is missing
from `package.json` during closure serialization
  [#16433](#16433)

- [cli/plugin] Fix plugin install command when plugin type is tool
  [#16407](#16407)

- [sdk/python] Fix Python SDK docs by escaping the trailing underscore
in a docstring
  [#14866](#14866)

- [sdk/python] Don't incorrectly emit deprecation warnings for
non-deprecated properties
  [#16400](#16400)

- [sdk/python] Handle extra CLI arguments passed policy packs plugins
  [#16402](#16402)

- [sdk/python] Add VIRTUAL_ENV environment variable when running inside
a virtual environment
  [#16425](#16425)

- [sdk/python] Don't lift dunder attributes on `Output`s


### Miscellaneous

- [sdk/{go,nodejs,python}] Add register resource transform alias for
register stack transform

- [cli/new] Instruct the user to use 'pulumi install' when using
--generate-only
  [#16411](#16411)
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v3.121.0.

@pulumi-bot pulumi-bot mentioned this pull request Jun 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgummerer tgummerer tgummerer approved these changes

@iwahbe iwahbe iwahbe approved these changes

@Frassle Frassle Awaiting requested review from Frassle

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

Successfully merging this pull request may close these issues.

CLI panic in processIgnoreChanges
5 participants
@mikhailshilkov @pulumi-bot @t0yv0 @tgummerer @iwahbe