sdk/py/ComponentResource: Propagate provider to children #12292
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
|
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
Changelog[uncommitted] (2023-03-03)Bug Fixes
|
justinvp
approved these changes
Mar 3, 2023
It's currently possible in Pulumi to pass a ProviderResource as a Provider option to a ComponentResource. The ComponentResource will record the Provider for later, and propagate it to its child resources. This is currently the behavior in [all SDKs except Python][1]. [1]: #12161 (comment) The reason this didn't work was because in Python's providers merging logic, we use the incorrect package name when saving the provider. Instead of using the package name reported by the provider, we were using the package name of the current resource (the ComponentResource in this case). To fix this, verify that the package name of the provider matches the package name of the resource we're building. If it doesn't, we will still save the provider for child resources. Resolves #12161
abhinav
force-pushed
the
abhinav/provider-prop-py
branch
from
787bda0
to
db4c071
Compare
|
bors merge |
|
Build succeeded: |
abhinav
added a commit
that referenced
this pull request
Mar 14, 2023
In #12296, we started correctly interpreting the Provider argument per other SDKs. but this introduced a regression: the Provider field is now set for resources with mismatched types. This results in a scenario where a provider foo for package X is passed to a resource bar with package Y, with the intent of plumbing it to bar's descendants, but bar attempts to incorrectly use the provider directly. foo := NewXProvider() bar := NewYThing("bar", Provider(foo)) // ... baz := NewXThing("baz", Parent(bar)) // should use foo This worked previously, but with #12296, this fails because NewYThing attempts to use Provider foo directly. To fix this, we need to prevent NewYThing from using a provider that does not match the package that it belongs to. We had to make a similar change to the Python SDK in #12292. https://github.com/pulumi/pulumi/blob/477e0c97660bdbeb28a142057899792d52e63a0b/sdk/python/lib/pulumi/resource.py#L925-L927 The regression test is specific to remote component resources per #12430, but the issue would be caused even for normal component resources before this. Resolves #12430
abhinav
added a commit
that referenced
this pull request
Mar 14, 2023
In #12296, we started correctly interpreting the Provider argument per other SDKs. but this introduced a regression: the Provider field is now set for resources with mismatched types. This results in a scenario where a provider foo for package X is passed to a resource bar with package Y, with the intent of plumbing it to bar's descendants, but bar attempts to incorrectly use the provider directly. foo := NewXProvider() bar := NewYThing("bar", Provider(foo)) // ... baz := NewXThing("baz", Parent(bar)) // should use foo This worked previously, but with #12296, this fails because NewYThing attempts to use Provider foo directly. To fix this, we need to prevent NewYThing from using a provider that does not match the package that it belongs to. We had to make a similar change to the Python SDK in #12292. https://github.com/pulumi/pulumi/blob/477e0c97660bdbeb28a142057899792d52e63a0b/sdk/python/lib/pulumi/resource.py#L925-L927 The regression test is specific to remote component resources per #12430, but the issue would be caused even for normal component resources before this. Resolves #12430
abhinav
added a commit
that referenced
this pull request
Mar 14, 2023
In #12296, we started correctly interpreting the Provider argument per other SDKs. but this introduced a regression: the Provider field is now set for resources with mismatched types. This results in a scenario where a provider foo for package X is passed to a resource bar with package Y, with the intent of plumbing it to bar's descendants, but bar attempts to incorrectly use the provider directly. foo := NewXProvider() bar := NewYThing("bar", Provider(foo)) // ... baz := NewXThing("baz", Parent(bar)) // should use foo This worked previously, but with #12296, this fails because NewYThing attempts to use Provider foo directly. To fix this, we need to prevent NewYThing from using a provider that does not match the package that it belongs to. We had to make a similar change to the Python SDK in #12292. https://github.com/pulumi/pulumi/blob/477e0c97660bdbeb28a142057899792d52e63a0b/sdk/python/lib/pulumi/resource.py#L925-L927 The regression test is specific to remote component resources per #12430, but the issue would be caused even for normal component resources before this. Resolves #12430
bors bot
added a commit
that referenced
this pull request
Mar 15, 2023
12426: filestate/internal: Use stack reference, not name r=abhinav a=abhinav filestate backend currently operates exclusively with stack names. All its internal pass around just the stack name, and nothing else. This makes it a bit difficult to add project support to the backend. This is a refactor in advance of adding project support, changing the internals of filestate to pass a stack reference around. It inspects the reference directly for all its operations. Note: This contains no behavioral changes. Name and FullyQualifiedName currently both return just the stack name. In a future change, once project name is incorporated into the object, FullyQualifiedName will be able to return `organization/$project/$name`. Change extracted from #12134 12433: sdk/go: Don't use Provider for type if package doesn't match r=abhinav a=abhinav In #12296, we started correctly interpreting the Provider argument per other SDKs. but this introduced a regression: the Provider field is now set for resources with mismatched types. This results in a scenario where a provider foo for package X is passed to a resource bar with package Y, with the intent of plumbing it to bar's descendants, but bar attempts to incorrectly use the provider directly. foo := NewXProvider() bar := NewYThing("bar", Provider(foo)) // ... baz := NewXThing("baz", Parent(bar)) // should use foo This worked previously, but with #12296, this fails because NewYThing attempts to use Provider foo directly. To fix this, we need to prevent NewYThing from using a provider that does not match the package that it belongs to. We had to make a similar change to the Python SDK in #12292. https://github.com/pulumi/pulumi/blob/477e0c97660bdbeb28a142057899792d52e63a0b/sdk/python/lib/pulumi/resource.py#L925-L927 The regression test is specific to remote component resources per #12430, but the issue would be caused even for normal component resources before this. Resolves #12430 12435: Pin CI to Node 19.7.x to avoid hitting assert in 19.8.0 r=abhinav a=justinvp Node 19.8.0 was released ~5 hours ago and we're hitting an internal assert. Pin to 19.7.x to unblock CI until we understand what's going on with 19.8.0. Part of #12434 Co-authored-by: Fraser Waters <fraser@pulumi.com> Co-authored-by: Abhinav Gupta <abhinav@pulumi.com> Co-authored-by: Justin Van Patten <jvp@justinvp.com>
bors bot
added a commit
that referenced
this pull request
Mar 15, 2023
12433: sdk/go: Don't use Provider for type if package doesn't match r=abhinav a=abhinav In #12296, we started correctly interpreting the Provider argument per other SDKs. but this introduced a regression: the Provider field is now set for resources with mismatched types. This results in a scenario where a provider foo for package X is passed to a resource bar with package Y, with the intent of plumbing it to bar's descendants, but bar attempts to incorrectly use the provider directly. foo := NewXProvider() bar := NewYThing("bar", Provider(foo)) // ... baz := NewXThing("baz", Parent(bar)) // should use foo This worked previously, but with #12296, this fails because NewYThing attempts to use Provider foo directly. To fix this, we need to prevent NewYThing from using a provider that does not match the package that it belongs to. We had to make a similar change to the Python SDK in #12292. https://github.com/pulumi/pulumi/blob/477e0c97660bdbeb28a142057899792d52e63a0b/sdk/python/lib/pulumi/resource.py#L925-L927 The regression test is specific to remote component resources per #12430, but the issue would be caused even for normal component resources before this. Resolves #12430 12435: Pin CI to Node 19.7.x to avoid hitting assert in 19.8.0 r=abhinav a=justinvp Node 19.8.0 was released ~5 hours ago and we're hitting an internal assert. Pin to 19.7.x to unblock CI until we understand what's going on with 19.8.0. Part of #12434 Co-authored-by: Abhinav Gupta <abhinav@pulumi.com> Co-authored-by: Justin Van Patten <jvp@justinvp.com>
bors bot
added a commit
that referenced
this pull request
Mar 15, 2023
12433: sdk/go: Don't use Provider for type if package doesn't match r=abhinav a=abhinav In #12296, we started correctly interpreting the Provider argument per other SDKs. but this introduced a regression: the Provider field is now set for resources with mismatched types. This results in a scenario where a provider foo for package X is passed to a resource bar with package Y, with the intent of plumbing it to bar's descendants, but bar attempts to incorrectly use the provider directly. foo := NewXProvider() bar := NewYThing("bar", Provider(foo)) // ... baz := NewXThing("baz", Parent(bar)) // should use foo This worked previously, but with #12296, this fails because NewYThing attempts to use Provider foo directly. To fix this, we need to prevent NewYThing from using a provider that does not match the package that it belongs to. We had to make a similar change to the Python SDK in #12292. https://github.com/pulumi/pulumi/blob/477e0c97660bdbeb28a142057899792d52e63a0b/sdk/python/lib/pulumi/resource.py#L925-L927 The regression test is specific to remote component resources per #12430, but the issue would be caused even for normal component resources before this. Resolves #12430 Co-authored-by: Abhinav Gupta <abhinav@pulumi.com>
bors bot
added a commit
that referenced
this pull request
Mar 15, 2023
12433: sdk/go: Don't use Provider for type if package doesn't match r=Frassle a=abhinav In #12296, we started correctly interpreting the Provider argument per other SDKs. but this introduced a regression: the Provider field is now set for resources with mismatched types. This results in a scenario where a provider foo for package X is passed to a resource bar with package Y, with the intent of plumbing it to bar's descendants, but bar attempts to incorrectly use the provider directly. foo := NewXProvider() bar := NewYThing("bar", Provider(foo)) // ... baz := NewXThing("baz", Parent(bar)) // should use foo This worked previously, but with #12296, this fails because NewYThing attempts to use Provider foo directly. To fix this, we need to prevent NewYThing from using a provider that does not match the package that it belongs to. We had to make a similar change to the Python SDK in #12292. https://github.com/pulumi/pulumi/blob/477e0c97660bdbeb28a142057899792d52e63a0b/sdk/python/lib/pulumi/resource.py#L925-L927 The regression test is specific to remote component resources per #12430, but the issue would be caused even for normal component resources before this. Resolves #12430 Co-authored-by: Abhinav Gupta <abhinav@pulumi.com>
bors bot
added a commit
that referenced
this pull request
Mar 15, 2023
12433: sdk/go: Don't use Provider for type if package doesn't match r=Frassle a=abhinav In #12296, we started correctly interpreting the Provider argument per other SDKs. but this introduced a regression: the Provider field is now set for resources with mismatched types. This results in a scenario where a provider foo for package X is passed to a resource bar with package Y, with the intent of plumbing it to bar's descendants, but bar attempts to incorrectly use the provider directly. foo := NewXProvider() bar := NewYThing("bar", Provider(foo)) // ... baz := NewXThing("baz", Parent(bar)) // should use foo This worked previously, but with #12296, this fails because NewYThing attempts to use Provider foo directly. To fix this, we need to prevent NewYThing from using a provider that does not match the package that it belongs to. We had to make a similar change to the Python SDK in #12292. https://github.com/pulumi/pulumi/blob/477e0c97660bdbeb28a142057899792d52e63a0b/sdk/python/lib/pulumi/resource.py#L925-L927 The regression test is specific to remote component resources per #12430, but the issue would be caused even for normal component resources before this. Resolves #12430 Co-authored-by: Abhinav Gupta <abhinav@pulumi.com>
bors bot
added a commit
that referenced
this pull request
Mar 15, 2023
12433: sdk/go: Don't use Provider for type if package doesn't match r=Frassle a=abhinav In #12296, we started correctly interpreting the Provider argument per other SDKs. but this introduced a regression: the Provider field is now set for resources with mismatched types. This results in a scenario where a provider foo for package X is passed to a resource bar with package Y, with the intent of plumbing it to bar's descendants, but bar attempts to incorrectly use the provider directly. foo := NewXProvider() bar := NewYThing("bar", Provider(foo)) // ... baz := NewXThing("baz", Parent(bar)) // should use foo This worked previously, but with #12296, this fails because NewYThing attempts to use Provider foo directly. To fix this, we need to prevent NewYThing from using a provider that does not match the package that it belongs to. We had to make a similar change to the Python SDK in #12292. https://github.com/pulumi/pulumi/blob/477e0c97660bdbeb28a142057899792d52e63a0b/sdk/python/lib/pulumi/resource.py#L925-L927 The regression test is specific to remote component resources per #12430, but the issue would be caused even for normal component resources before this. Resolves #12430 Co-authored-by: Abhinav Gupta <abhinav@pulumi.com>
abhinav
added a commit
that referenced
this pull request
Mar 27, 2023
bors bot
added a commit
that referenced
this pull request
Apr 13, 2023
12639: sdk/py: Propagate provider from local and remote component resources r=abhinav a=abhinav Each commit is individually reviewable. The first commit just re-applies #12292. --- This change brings back #12292 (fix for #12161), which was reverted in #12522 because it broke MLCs (#12520) and corrects the issue that caused that failure. - #12161 manifests as local component resources not propagating the `provider` option to their children - #12520 manifests as remote component resources (MLCs) not propagating the `provider` option to their children Roughly, given: class MyComponentResource: def __init__(self, ...): # ... self.child = aws.s3.Bucket(..., opts=ResourceOptions(parent=self)) MyComponentResource(opts=ResourceOptions(provider=my_aws_provider)) Both bugs took the form of the `aws.s3.Bucket` above not getting `my_aws_provider`. We previously fixed #12161 by keeping using the provider for the resource only if its package matched the resource's type; otherwise, we placed it in the providers bag for the children of that resource. This had the effect of dropping the provider for component resources because: - `_get_providers` returns a provider (None for component resources) and a bag of providers for children of the resource. It is stored on the resource: https://github.com/pulumi/pulumi/blob/f76bd5463b512222b62d9e60191d93ab2a66290e/sdk/python/lib/pulumi/resource.py#L866 - A request is made to `register_resource`, which calls `prepare_resource`: https://github.com/pulumi/pulumi/blob/f76bd5463b512222b62d9e60191d93ab2a66290e/sdk/python/lib/pulumi/resource.py#L880-L882 https://github.com/pulumi/pulumi/blob/f76bd5463b512222b62d9e60191d93ab2a66290e/sdk/python/lib/pulumi/runtime/resource.py#L811 - `prepare_resource` re-interprets the `provider` and `providers` resource options, based on what's in the **original** `opts` struct completely ignoring the second result of `_get_providers`: https://github.com/pulumi/pulumi/blob/f76bd5463b512222b62d9e60191d93ab2a66290e/sdk/python/lib/pulumi/runtime/resource.py#L210 So in short, the reason the original change broke was: It turned `provider=x` into `providers=[x]` when appropriate, but the consuming code did not consume the transformed option. To fix this, we put the bag of providers produced by `_get_providers` back into the `opts` struct so that `prepare_resource` and friends can use it. Testing: An integration test is added that defines an MLC, and uses it from Pulumi programs in Go, Python, and Node that instantiate the MLC with no provider, provider option, and providers option, and verifies that resources were instantiated with the correct providers. Resolves #12593 Resolves #12161 Co-authored-by: Abhinav Gupta <abhinav@pulumi.com>
bors bot
added a commit
that referenced
this pull request
Apr 14, 2023
12639: sdk/py: Propagate provider from local and remote component resources r=abhinav a=abhinav Each commit is individually reviewable. The first commit just re-applies #12292. --- This change brings back #12292 (fix for #12161), which was reverted in #12522 because it broke MLCs (#12520) and corrects the issue that caused that failure. - #12161 manifests as local component resources not propagating the `provider` option to their children - #12520 manifests as remote component resources (MLCs) not propagating the `provider` option to their children Roughly, given: class MyComponentResource: def __init__(self, ...): # ... self.child = aws.s3.Bucket(..., opts=ResourceOptions(parent=self)) MyComponentResource(opts=ResourceOptions(provider=my_aws_provider)) Both bugs took the form of the `aws.s3.Bucket` above not getting `my_aws_provider`. We previously fixed #12161 by keeping using the provider for the resource only if its package matched the resource's type; otherwise, we placed it in the providers bag for the children of that resource. This had the effect of dropping the provider for component resources because: - `_get_providers` returns a provider (None for component resources) and a bag of providers for children of the resource. It is stored on the resource: https://github.com/pulumi/pulumi/blob/f76bd5463b512222b62d9e60191d93ab2a66290e/sdk/python/lib/pulumi/resource.py#L866 - A request is made to `register_resource`, which calls `prepare_resource`: https://github.com/pulumi/pulumi/blob/f76bd5463b512222b62d9e60191d93ab2a66290e/sdk/python/lib/pulumi/resource.py#L880-L882 https://github.com/pulumi/pulumi/blob/f76bd5463b512222b62d9e60191d93ab2a66290e/sdk/python/lib/pulumi/runtime/resource.py#L811 - `prepare_resource` re-interprets the `provider` and `providers` resource options, based on what's in the **original** `opts` struct completely ignoring the second result of `_get_providers`: https://github.com/pulumi/pulumi/blob/f76bd5463b512222b62d9e60191d93ab2a66290e/sdk/python/lib/pulumi/runtime/resource.py#L210 So in short, the reason the original change broke was: It turned `provider=x` into `providers=[x]` when appropriate, but the consuming code did not consume the transformed option. To fix this, we put the bag of providers produced by `_get_providers` back into the `opts` struct so that `prepare_resource` and friends can use it. Testing: An integration test is added that defines an MLC, and uses it from Pulumi programs in Go, Python, and Node that instantiate the MLC with no provider, provider option, and providers option, and verifies that resources were instantiated with the correct providers. Resolves #12593 Resolves #12161 Co-authored-by: Abhinav Gupta <abhinav@pulumi.com>
bors bot
added a commit
that referenced
this pull request
Apr 14, 2023
12639: sdk/py: Propagate provider from local and remote component resources r=abhinav a=abhinav Each commit is individually reviewable. The first commit just re-applies #12292. --- This change brings back #12292 (fix for #12161), which was reverted in #12522 because it broke MLCs (#12520) and corrects the issue that caused that failure. - #12161 manifests as local component resources not propagating the `provider` option to their children - #12520 manifests as remote component resources (MLCs) not propagating the `provider` option to their children Roughly, given: class MyComponentResource: def __init__(self, ...): # ... self.child = aws.s3.Bucket(..., opts=ResourceOptions(parent=self)) MyComponentResource(opts=ResourceOptions(provider=my_aws_provider)) Both bugs took the form of the `aws.s3.Bucket` above not getting `my_aws_provider`. We previously fixed #12161 by keeping using the provider for the resource only if its package matched the resource's type; otherwise, we placed it in the providers bag for the children of that resource. This had the effect of dropping the provider for component resources because: - `_get_providers` returns a provider (None for component resources) and a bag of providers for children of the resource. It is stored on the resource: https://github.com/pulumi/pulumi/blob/f76bd5463b512222b62d9e60191d93ab2a66290e/sdk/python/lib/pulumi/resource.py#L866 - A request is made to `register_resource`, which calls `prepare_resource`: https://github.com/pulumi/pulumi/blob/f76bd5463b512222b62d9e60191d93ab2a66290e/sdk/python/lib/pulumi/resource.py#L880-L882 https://github.com/pulumi/pulumi/blob/f76bd5463b512222b62d9e60191d93ab2a66290e/sdk/python/lib/pulumi/runtime/resource.py#L811 - `prepare_resource` re-interprets the `provider` and `providers` resource options, based on what's in the **original** `opts` struct completely ignoring the second result of `_get_providers`: https://github.com/pulumi/pulumi/blob/f76bd5463b512222b62d9e60191d93ab2a66290e/sdk/python/lib/pulumi/runtime/resource.py#L210 So in short, the reason the original change broke was: It turned `provider=x` into `providers=[x]` when appropriate, but the consuming code did not consume the transformed option. To fix this, we put the bag of providers produced by `_get_providers` back into the `opts` struct so that `prepare_resource` and friends can use it. Testing: An integration test is added that defines an MLC, and uses it from Pulumi programs in Go, Python, and Node that instantiate the MLC with no provider, provider option, and providers option, and verifies that resources were instantiated with the correct providers. Resolves #12593 Resolves #12161 Co-authored-by: Abhinav Gupta <abhinav@pulumi.com>
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.
It's currently possible in Pulumi to pass a ProviderResource
as a Provider option to a ComponentResource.
The ComponentResource will record the Provider for later,
and propagate it to its child resources.
This is currently the behavior in all SDKs except Python.
The reason this didn't work was because in Python's
providers merging logic, we use the incorrect package name
when saving the provider.
Instead of using the package name reported by the provider,
we were using the package name of the current resource
(the ComponentResource in this case).
To fix this, verify that the package name of the provider
matches the package name of the resource we're building.
If it doesn't, we will still save the provider
for child resources.
Resolves #12161