Skip to content

Commit

Permalink
Merge #12848
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
bors[bot] and justinvp committed Jun 14, 2023
2 parents 5d7785e + f5b1175 commit b89cfb6
Show file tree
Hide file tree
Showing 72 changed files with 2,982 additions and 177 deletions.
@@ -0,0 +1,4 @@
changes:
- type: fix
scope: engine
description: Fix aliasing children
@@ -0,0 +1,4 @@
changes:
- type: fix
scope: sdk/nodejs
description: Fix Parent/NoParent aliases

0 comments on commit b89cfb6

Please sign in to comment.