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.
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
Fix aliasing children #12848
Fix aliasing children #12848
Conversation
func (a *Alias) NoParent() bool { | ||
return a.Parent == "" | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, this is a breaking change. However, I think it's worth making the change because it's unlikely anyone else is actually using this directly.
The alternative is to keep the NoParent
method as-is (potentially marking it as deprecated) and add a new bool field for NoParent
with some other name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this, most of the things in /common aren't really supposed to be used for public consumption.
The engine change fixes the non-Node.js SDKs. For Node.js, if you're using the fixed engine with an old Node.js SDK, or a new Node.js SDK with an old engine, you may see some behavior differences depending on how you're using Some possible mitigations:
Although, the simplest answer would be to just update both your CLI and SDK to the latest version. |
func (a *Alias) NoParent() bool { | ||
return a.Parent == "" | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this, most of the things in /common aren't really supposed to be used for public consumption.
When support for alias computation was implemented in the engine in a5233f4, the existing alias tests in `pulumi_test.go` were duplicated to test the new spec-based functionality. The existing tests are in `TestAliasURNs` and new duplicated tests were added in `TestAliases`. However, the new tests used the spec's `URN` field rather than the new spec fields like `Name` and `Type`, so we're missing some coverage of these fields. This commit updates the duplicated spec tests in `TestAliases` to use the new alias spec fields rather than `URN`, where appropriate.
This commit adds more coverage for aliases with parent/child relationships, with new spec-based and equivalent URN-based tests. The spec-based tests fail without a fix in the engine.
Split out the "generate aliases" functionality from `generateSteps` to its own method, to make it possible to test just `generateAliases`.
Add some tests for the split-out `generateAliases` method. These tests fail without the fix in the engine.
There is an issue with how the engine computes the aliases when the resource is a child and doesn't have `Parent` set on the alias spec (and the parent doesn't have any aliases). ```python class FooResource(pulumi.ComponentResource): def __init__(self, name, opts=None): super().__init__("my:module:FooResource", name, None, opts) class ComponentResource(pulumi.ComponentResource): def __init__(self, name, opts=None): super().__init__("my:module:ComponentResource", name, None, opts) FooResource("childrenamed", pulumi.ResourceOptions( parent=self, aliases=[pulumi.Alias(name="child")] )) ``` In the example above, `ComponentResource` has a child `FooResource` which was renamed from `child` to `childrenamed`. The engine does not compute the correct alias: ``` expected: urn:pulumi:stack::project::my:module:ComponentResource$my:module:FooResource::child actual: urn:pulumi:stack::project::my:module:FooResource::child ``` The problem is due to: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/pkg/resource/deploy/step_generator.go#L370-L382 ... and: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/sdk/go/common/resource/alias.go#L24-L26 Because the alias spec doesn't have `parent` specified, the parent type is not being included the computed alias URN. Existing tests such as https://github.com/pulumi/pulumi/tree/master/tests/integration/aliases/python/rename_component_and_child didn't catch the problem because the alias specifies both the `name` and `parent`: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/tests/integration/aliases/python/rename_component_and_child/step2/__main__.py#L15 In this case, specifying `parent` on the alias shouldn't be necessary. However, even after removing `parent` from the alias spec, the test still succeeds because the parent itself has an alias: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/tests/integration/aliases/python/rename_component_and_child/step2/__main__.py#L18 ... and parent aliases are inherited as part of a child's aliases, so we still get an alias that works from the inheritance. If we change the test to make no changes to the parent such that it doesn't have any aliases, then we get the failure as we'd expect. A similar problem will happen when retyping a child. **Fix** The fix involves using the child's parent in the calculated alias URN when `Parent` isn't specified for the alias. As part of this, we need to properly handled `NoParent` because right now the engine is not correctly using it. The struct representing an alias in the engine does not have a `NoParent` field: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/sdk/go/common/resource/alias.go#L8-L15 And therefore does not copy it over in the gRPC request: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/pkg/resource/deploy/source_eval.go#L1082-L1088 Instead, the `Alias` struct has an incorrect `NoParent` method which returns `true` if the `Parent` field has a value of `""`: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/sdk/go/common/resource/alias.go#L24-L26
The Node.js SDK is not sending the right `Parent` and `NoParent` alias spec. https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/sdk/nodejs/runtime/resource.ts#L332-L361 - It does not set `NoParent` when it should, but instead sets `Parent` to empty. - It sets `NoParent` to true and leaves `Parent` empty when both the alias and resource have no `Parent` specified. - If the resource has a parent and `Parent` isn't set on the alias, it sends the parent's URN for the `Parent`. This commit fixes these issues so that the Node.js SDK sends correctly specified alias specs. However, we continue to set the alias's `Parent` to the resource's parent if the alias does not specify a `Parent`. We shouldn't have to do this, but doing it allows us to maintain backwards compatibility in the case when the Node.js SDK is upgraded, but not the CLI.
Both alias tests use the same `upgradeProgramWithResources` local. Factor it out into a top-level function that both can reuse rather than duplicating the code. We'll be reusing this in another alias test added in a subsequent commit.
1026e7b
to
aea5835
Compare
This change updates the engine to detect if a `RegisterResource` request is coming from an older Node.js SDK that is using incorrect alias specs and, if so, transforms the aliases to be correct. This allows us to maintain compatibility for users who have upgraded their CLI but are still using an older version of the Node.js SDK with incorrect alias specs. We detect if the request is from a Node.js SDK by looking at the gRPC request's metadata headers, specifically looking at the "pulumi-runtime" and "user-agent" headers. First, if the request has a "pulumi-runtime" header with a value of "nodejs", we know it's coming from the Node.js plugin. The Node.js language plugin proxies gRPC calls from the Node.js SDK to the resource monitor and the proxy now sets the "pulumi-runtime" header to "nodejs" for `RegisterResource` calls. Second, if the request has a "user-agent" header that starts with "grpc-node-js/", we know it's coming from the Node.js SDK. This is the case for inline programs in the automation API, which connects directly to the resource monitor, rather than going through the language plugin's proxy. We can't just look at "user-agent", because in the proxy case it will have a Go-specific "user-agent". Updated Node.js SDKs set a new `aliasSpecs` field on the `RegisterResource` request, which indicates that the alias specs are correct, and no transforms are needed.
I've pushed some additional commits to maintain behavior for users of the Node.js SDK. I looked at these two cases:
New CLI + Old Node.js SDKThe new The mitigation for this is ugly and I initially felt uneasy about it, but it is a practical way to address the issue, and I'm personally OK with it. If the engine detects a It does this by looking at the gRPC request's metadata headers, specifically looking at the "pulumi-runtime" and "user-agent" headers. First, if the request has a "pulumi-runtime" header with a value of "nodejs", we know it's coming from the Node.js plugin. The Node.js language plugin proxies gRPC calls from the Node.js SDK to the resource monitor, and the proxy now sets the "pulumi-runtime" header to "nodejs" Second, if the request has a "user-agent" header that starts with "grpc-node-js/", we know it's coming from the Node.js SDK. This is the case for inline programs in the automation API, which connects directly to the resource monitor, rather than going through the language plugin's proxy. We can't just look at "user-agent", because in the proxy case it will have a Go-specific "user-agent", and it doesn't look like you can modify the "user-agent". Updated Node.js SDKs set a new Old CLI + New Node.js SDKThe new The mitigation for this is simple. We just have the Node.js SDK continue to set the resource's parent in the alias when the alias itself doesn't have a parent specified. If following the spec correctly, this isn't necessary, but there's no harm in doing it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nodejs workarounds are a bit sad but all looks reasonable to keep things working
bors merge |
12848: Fix aliasing children r=justinvp a=justinvp **Note:** This is a large PR that I've separated into many smaller commits to make it easier to review commit by commit. ## Background Originally, each SDK would compute alias URNs from alias "specs" and the alias URNs would be sent to the engine. #10819 added support for having the engine compute the alias URNs by enabling SDKs to send just the alias specs to the engine. #11206 updated the Node.js SDK to leverage this new capability, and subsequent PRs enabled it for the other SDKs. Unfortunately, there are a couple of issues: 1. Engine: A bug in alias computation 2. Node.js SDK: A bug in the alias specs sent to the engine ## Engine Bug There is an issue with how the engine computes the aliases when the resource is a child and doesn't have `Parent` set on the alias spec (and the parent doesn't have any aliases). ```python class FooResource(pulumi.ComponentResource): def __init__(self, name, opts=None): super().__init__("my:module:FooResource", name, None, opts) class ComponentResource(pulumi.ComponentResource): def __init__(self, name, opts=None): super().__init__("my:module:ComponentResource", name, None, opts) FooResource("childrenamed", pulumi.ResourceOptions( parent=self, aliases=[pulumi.Alias(name="child")] )) ``` In the example above, `ComponentResource` has a child `FooResource` which was renamed from `child` to `childrenamed`. The engine does not compute the correct alias: ``` expected: urn:pulumi:stack::project::my:module:ComponentResource$my:module:FooResource::child actual: urn:pulumi:stack::project::my:module:FooResource::child ``` The problem is due to: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/pkg/resource/deploy/step_generator.go#L370-L382 ... and: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/sdk/go/common/resource/alias.go#L24-L26 Because the alias spec doesn't have `Parent` specified, the parent type is not being included the computed alias URN. Existing tests such as https://github.com/pulumi/pulumi/tree/master/tests/integration/aliases/python/rename_component_and_child didn't catch the problem because the alias specifies both the `name` and `parent`: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/tests/integration/aliases/python/rename_component_and_child/step2/__main__.py#L15 In this case, specifying `parent` on the alias shouldn't be necessary. However, even after removing `parent` from the alias spec, the test still succeeds because the parent itself has an alias: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/tests/integration/aliases/python/rename_component_and_child/step2/__main__.py#L18 ... and parent aliases are inherited as part of a child's aliases, so we still get an alias that works from the inheritance. If we change the test to make no changes to the parent such that it doesn't have any aliases, then we get the failure as we'd expect. A similar problem will happen when retyping a child. ### Fix The fix involves using the child's parent in the calculated alias URN when `Parent` isn't specified for the alias. As part of this, we need to properly handled `NoParent` because right now the engine is not correctly using it. The struct representing an alias in the engine does not have a `NoParent` field: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/sdk/go/common/resource/alias.go#L8-L15 And therefore does not copy it over in the gRPC request: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/pkg/resource/deploy/source_eval.go#L1082-L1088 Instead, the `Alias` struct has an incorrect `NoParent` method which returns `true` if the `Parent` field has a value of `""`: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/sdk/go/common/resource/alias.go#L24-L26 ## Node.js SDK Bug The Node.js SDK is not sending the right `Parent` and `NoParent` alias spec fields. https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/sdk/nodejs/runtime/resource.ts#L332-L361 - It's only setting `NoParent` when the alias doesn't have a `Parent` set and the resource doesn't have a parent. - If the resource has a parent and `Parent` isn't set on the resource, it sends the parent's URN for `Parent`. Instead: - It should set `NoParent` when `Parent` is set to `undefined`. - It should only send `Parent` if the user's alias has specified a `Parent`. This makes the Node.js SDK behave like the other SDKs. Fixes #12662 Co-authored-by: Justin Van Patten <jvp@justinvp.com>
Build failed: |
bors retry |
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
Note: This is a large PR that I've separated into many smaller commits to make it easier to review commit by commit.
Background
Originally, each SDK would compute alias URNs from alias "specs" and the alias URNs would be sent to the engine. #10819 added support for having the engine compute the alias URNs by enabling SDKs to send just the alias specs to the engine. #11206 updated the Node.js SDK to leverage this new capability, and subsequent PRs enabled it for the other SDKs.
Unfortunately, there are a couple of issues:
Engine Bug
There is an issue with how the engine computes the aliases when the resource is a child and doesn't have
Parent
set on the alias spec (and the parent doesn't have any aliases).In the example above,
ComponentResource
has a childFooResource
which was renamed fromchild
tochildrenamed
.The engine does not compute the correct alias:
The problem is due to:
pulumi/pkg/resource/deploy/step_generator.go
Lines 370 to 382 in 117955c
... and:
pulumi/sdk/go/common/resource/alias.go
Lines 24 to 26 in 117955c
Because the alias spec doesn't have
Parent
specified, the parent type is not being included the computed alias URN.Existing tests such as https://github.com/pulumi/pulumi/tree/master/tests/integration/aliases/python/rename_component_and_child didn't catch the problem because the alias specifies both the
name
andparent
:pulumi/tests/integration/aliases/python/rename_component_and_child/step2/__main__.py
Line 15 in 117955c
In this case, specifying
parent
on the alias shouldn't be necessary. However, even after removingparent
from the alias spec, the test still succeeds because the parent itself has an alias:pulumi/tests/integration/aliases/python/rename_component_and_child/step2/__main__.py
Line 18 in 117955c
... and parent aliases are inherited as part of a child's aliases, so we still get an alias that works from the inheritance.
If we change the test to make no changes to the parent such that it doesn't have any aliases, then we get the failure as we'd expect.
A similar problem will happen when retyping a child.
Fix
The fix involves using the child's parent in the calculated alias URN when
Parent
isn't specified for the alias.As part of this, we need to properly handled
NoParent
because right now the engine is not correctly using it. The struct representing an alias in the engine does not have aNoParent
field:pulumi/sdk/go/common/resource/alias.go
Lines 8 to 15 in 117955c
And therefore does not copy it over in the gRPC request:
pulumi/pkg/resource/deploy/source_eval.go
Lines 1082 to 1088 in 117955c
Instead, the
Alias
struct has an incorrectNoParent
method which returnstrue
if theParent
field has a value of""
:pulumi/sdk/go/common/resource/alias.go
Lines 24 to 26 in 117955c
Node.js SDK Bug
The Node.js SDK is not sending the right
Parent
andNoParent
alias spec fields.pulumi/sdk/nodejs/runtime/resource.ts
Lines 332 to 361 in 117955c
NoParent
when the alias doesn't have aParent
set and the resource doesn't have a parent.Parent
isn't set on the resource, it sends the parent's URN forParent
.Instead:
NoParent
whenParent
is set toundefined
.Parent
if the user's alias has specified aParent
.This makes the Node.js SDK behave like the other SDKs.
Fixes #12662