fix panic on deserializing deployment #15599
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Changelog[uncommitted] (2024-03-07)Bug Fixes
|
When a serialized deployment doesn't include a secrets provider configuration, but does include ciphertexts, currently we end up with a panic. Error out earlier if this is the case to avoid the panic.
tgummerer
force-pushed
the
tg/fix-panic-in-secrets-manager
branch
from
4bc1952
to
399c541
Compare
Frassle
reviewed
Mar 5, 2024
Co-authored-by: Fraser Waters <fraser@pulumi.com>
justinvp
reviewed
Mar 5, 2024
There was a problem hiding this comment.
What happens today (before this change) when calling DeserializeDeploymentV3 and going down this path? Do we immediately panic because of the NewPanicCrypters? Or is it possible to deserialize the deployment an not access any of the values and not have it panic?
|
Before this change we |
Frassle
reviewed
Mar 6, 2024
Done ✔️ |
Frassle
reviewed
Mar 6, 2024
Frassle
reviewed
Mar 6, 2024
| func TestDeserializeMissingSecretsManager(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| urn := "urn:pulumi:prod::acme::acme:erp:Backend$aws:ebs/volume:Volume::PlatformBackendDb" |
There was a problem hiding this comment.
I'd use just a standard test urn here like test_stack::test_project::pkgA:index:typA:test. Don't make people think there's anything complex about this test.
Frassle
approved these changes
Mar 7, 2024
Merged
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 7, 2024
Draft changelog:
### Features
- [auto/{go,nodejs,python}] Add support for suppress progress and
suppress outputs parameters in the Automation API
[#15596](#15596)
- [pkg] Make schema.NewPluginLoader respect PULUMI_DEBUG_PROVIDERS,
which enables Pulumi YAML programs to work correctly with this feature
[#15526](#15526)
- [sdk/dotnet] Update dotnet language host to 3.60.0
[#15609](#15609)
- [sdk/nodejs] Add experimental support to the NodeJS SDK for the new
transforms system.
[#15532](#15532)
- [sdk/python] Add support for asynchronous invokes via a new
`invoke_async` function
[#15602](#15602)
- [sdkgen/dotnet] Support for non-overlay components in codegen for
pulumi-kubernetes provider
[#15490](#15490)
### Bug Fixes
- [cli] Fix a panic when the secrets provider is missing from the
deployment snapshot
[#15599](#15599)
- [backend/service] Make decrypt/encrypt network calls retryable to help
work around network hiccups
[#15600](#15600)
- [cli/new] Strip credentials and query strings from template URLs saved
to project
[#15586](#15586)
- [engine] Fix an issue where snapshots could become invalid when doing
a targeted up
[#15476](#15476)
- [pkg/testing] Make ProgramTest use a temporary PULUMI_HOME for each
test
[#15568](#15568)
- [sdkgen/dotnet] Codegen fix for resources without constant input
properties
[#15488](#15488)
- [sdk/nodejs] Properly capture node:crypto and global.crypto in node
19+
- [sdk/python] Fix determining plugins for old packages in the Python
language host
[#15576](#15576)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a serialized deployment doesn't include a secrets provider configuration, but does include ciphertexts, currently we end up with a panic. Error out earlier if this is the case to avoid the panic.
This fixes the panic seen in #15547 and #14761, but it doesn't quite explain why this is happening in the first place. I asked for some more info from the users in these issues for that.
Putting this up as PR anyway in case anyone has any idea of why this could be happening in the first place. I've tried spelunking through the code, but nothing obvious stood out. It is possible that these were still v2 snapshots that included no secrets manager, but I would be somewhat surprised if those still existed in the wild.
Another potential solution here would be to try to pass the secret manager from the config in to the DeserializeDeployment function, so we could use that in these cases. It might not always be correct though, so I'm not sure it's the right thing to do.
Thoughts?