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.

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

feat(sdk/nodejs): delegates alias computation to the engine #11206

Merged
merged 1 commit into from Dec 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1,4 @@
changes:
- type: feat
scope: sdk/nodejs
description: Delegates alias computation to engine for Node SDK
2 changes: 1 addition & 1 deletion pkg/resource/deploy/step_generator.go
Expand Up @@ -415,7 +415,7 @@ func (sg *stepGenerator) inheritedChildAlias(
return resource.NewURN(
sg.deployment.Target().Name.Q(),
sg.deployment.source.Project(),
parentAlias.Type(),
parentAlias.QualifiedType(),
childType,
aliasName)
}
Expand Down
2 changes: 1 addition & 1 deletion sdk/go/common/util/env/env.go
Expand Up @@ -17,7 +17,7 @@
//
// Declaring a variable is as simple as declaring a module level constant.
//
// var Var = env.Bool("VAR", "A boolean variable")
// var Var = env.Bool("VAR", "A boolean variable")
//
// Typed values can be retrieved by calling `Var.Value()`.
package env
Expand Down
6 changes: 1 addition & 5 deletions sdk/nodejs/resource.ts
Expand Up @@ -17,7 +17,6 @@ import { Input, Inputs, interpolate, Output, output } from "./output";
import { getResource, readResource, registerResource, registerResourceOutputs } from "./runtime/resource";
import { getProject, getStack } from "./runtime/settings";
import { getStackResource } from "./runtime/state";
import * as stacklib from "./runtime/stack";
import { unknownValue } from "./runtime/rpc";
import * as utils from "./utils";
import * as log from "./log";
Expand Down Expand Up @@ -215,7 +214,7 @@ export abstract class Resource {
* @internal
*/
// eslint-disable-next-line @typescript-eslint/naming-convention,no-underscore-dangle,id-blacklist,id-match
private readonly __name?: string;
readonly __name?: string;

/**
* The set of providers to use for child resources. Keyed by package name (e.g. "aws").
Expand Down Expand Up @@ -333,9 +332,6 @@ export abstract class Resource {
opts.protect = parent.__protect;
}

// Update aliases to include the full set of aliases implied by the child and parent aliases.
opts.aliases = allAliases(opts.aliases || [], name, t, parent, parent.__name!);

this.__providers = parent.__providers;
}

Expand Down
73 changes: 63 additions & 10 deletions sdk/nodejs/runtime/resource.ts
Expand Up @@ -20,6 +20,8 @@ import * as utils from "../utils";
import { getAllResources, Input, Inputs, Output, output } from "../output";
import { ResolvedResource } from "../queryable";
import {
Alias,
allAliases,
ComponentResource,
ComponentResourceOptions,
createUrn,
Expand Down Expand Up @@ -53,13 +55,15 @@ import {
getStack,
isDryRun,
isLegacyApplyEnabled,
monitorSupportsAliasSpecs,
rpcKeepAlive,
serialize,
terminateRpcs,
} from "./settings";

const gstruct = require("google-protobuf/google/protobuf/struct_pb.js");
const resproto = require("../proto/resource_pb.js");
const aliasproto = require("../proto/alias_pb.js");

interface ResourceResolverOperation {
// A resolver for a resource's URN.
Expand All @@ -84,9 +88,11 @@ interface ResourceResolverOperation {
// all be URNs of custom resources, not component resources.
propertyToDirectDependencyURNs: Map<string, Set<URN>>;
// A list of aliases applied to this resource.
aliases: URN[];
aliases: (Alias | URN)[];
// An ID to import, if any.
import: ID | undefined;
// Any important feature support from the monitor.
monitorSupportsStructuredAliases: boolean;
}

/**
Expand Down Expand Up @@ -274,6 +280,42 @@ export function readResource(res: Resource, parent: Resource | undefined, t: str
}), label);
}

function getParentURN(parent?: Resource | Input<string>) {
if (Resource.isInstance(parent)) {
return parent.urn;
}
return output(parent);
}

function mapAliasesForRequest(aliases: (URN | Alias)[] | undefined, parentURN?: URN) {
if (aliases === undefined) {
return [];
}

return Promise.all(aliases.map(async a => {
const newAlias = new aliasproto.Alias();
if (typeof a === "string") {
newAlias.setUrn(a);
} else {
const newAliasSpec = new aliasproto.Alias.Spec();
const noParent = !a.hasOwnProperty("parent") && !parentURN;
newAliasSpec.setName(a.name);
newAliasSpec.setType(a.type);
newAliasSpec.setStack(a.stack);
newAliasSpec.setProject(a.project);
if (noParent) {
newAliasSpec.setNoparent(noParent);
} else {
const aliasParentUrn = a.hasOwnProperty("parent") ? getParentURN(a.parent) : output(parentURN);
const urn = await aliasParentUrn.promise();
newAliasSpec.setParenturn(urn);
}
newAlias.setSpec(newAliasSpec);
}
return newAlias;
}));
}

/**
* registerResource registers a new resource object with a given type t and name. It returns the auto-generated
* URN and the ID that will resolve after the deployment has completed. All properties will be initialized to property
Expand All @@ -285,7 +327,7 @@ export function registerResource(res: Resource, parent: Resource | undefined, t:
log.debug(`Registering resource: t=${t}, name=${name}, custom=${custom}, remote=${remote}`);

const monitor = getMonitor();
const resopAsync = prepareResource(label, res, parent, custom, remote, props, opts);
const resopAsync = prepareResource(label, res, parent, custom, remote, props, opts, t, name);

// In order to present a useful stack trace if an error does occur, we preallocate potential
// errors here. V8 captures a stack trace at the moment an Error is created and this stack
Expand All @@ -312,7 +354,12 @@ export function registerResource(res: Resource, parent: Resource | undefined, t:
req.setAcceptsecrets(true);
req.setAcceptresources(!utils.disableResourceReferences);
req.setAdditionalsecretoutputsList((<any>opts).additionalSecretOutputs || []);
req.setAliasurnsList(resop.aliases);
if (resop.monitorSupportsStructuredAliases) {
const aliasesList = await mapAliasesForRequest(resop.aliases, resop.parentURN);
req.setAliasesList(aliasesList);
} else {
req.setAliasurnsList(resop.aliases);
}
req.setImportid(resop.import || "");
req.setSupportspartialvalues(true);
req.setRemote(remote);
Expand Down Expand Up @@ -425,8 +472,7 @@ export function registerResource(res: Resource, parent: Resource | undefined, t:
* properties.
*/
async function prepareResource(label: string, res: Resource, parent: Resource | undefined, custom: boolean, remote: boolean,
props: Inputs, opts: ResourceOptions): Promise<ResourceResolverOperation> {

props: Inputs, opts: ResourceOptions, type?: string, name?: string): Promise<ResourceResolverOperation> {
// add an entry to the rpc queue while we prepare the request.
// automation api inline programs that don't have stack exports can exit quickly. If we don't do this,
// sometimes they will exit right after `prepareResource` is called as a part of register resource, but before the
Expand Down Expand Up @@ -582,12 +628,18 @@ async function prepareResource(label: string, res: Resource, parent: Resource |
propertyToDirectDependencyURNs.set(propertyName, urns);
}

// Wait for all aliases. Note that we use `res.__aliases` instead of `opts.aliases` as the former has been processed
// in the Resource constructor prior to calling `registerResource` - both adding new inherited aliases and
// simplifying aliases down to URNs.
const monitorSupportsStructuredAliases = await monitorSupportsAliasSpecs();
let computedAliases;
if (!monitorSupportsStructuredAliases && parent) {
computedAliases = allAliases(opts.aliases || [], name!, type!, parent, parent.__name!);
} else {
computedAliases = opts.aliases || [];
}

// Wait for all aliases.
const aliases = [];
const uniqueAliases = new Set<string>();
for (const alias of (res.__aliases || [])) {
const uniqueAliases = new Set<Alias | URN>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little strange having Set<Alias> given that Alias is an object type which I believe means object reference equality will be used to compare what's already in the Set<Alias>.

I wonder if rather than keeping this code here, it should move into the !monitorSupportsStructuredAliases && parent case above, e.g. something like (I'm writing in GH without compiler help):

let aliases;
if (!monitorSupportsStructuredAliases && parent) {
    const computedAliases = allAliases(opts.aliases || [], name!, type!, parent, parent.__name!);
    aliases = [];
    const uniqueAliases = new Set<URN>();
    for (const alias of computedAliases) {
        const aliasVal = await output(alias).promise();
        if (!uniqueAliases.has(aliasVal)) {
            uniqueAliases.add(aliasVal);
            aliases.push(aliasVal);
        }
    }
} else {
    aliases = opts.aliases || [];
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, missed that you'd still need to loop through and do const aliasVal = await output(alias).promise(); for the non-computed case. I think it's simpler to leave it the way you had it.

for (const alias of (computedAliases || [])) {
const aliasVal = await output(alias).promise();
if (!uniqueAliases.has(aliasVal)) {
uniqueAliases.add(aliasVal);
Expand All @@ -607,6 +659,7 @@ async function prepareResource(label: string, res: Resource, parent: Resource |
propertyToDirectDependencyURNs: propertyToDirectDependencyURNs,
aliases: aliases,
import: importID,
monitorSupportsStructuredAliases,
};

} finally {
Expand Down
11 changes: 11 additions & 0 deletions sdk/nodejs/runtime/settings.ts
Expand Up @@ -518,6 +518,17 @@ export async function monitorSupportsDeletedWith(): Promise<boolean> {
return monitorSupportsFeature("deletedWith");
}

/**
* monitorSupportsAliasSpecs returns a promise that when resolved tells you if the resource monitor we are
* connected to is able to support alias specs across its RPC interface. When it does, we marshal aliases
* in a special way.
*
* @internal
*/
export async function monitorSupportsAliasSpecs(): Promise<boolean> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mark this as @internal? I don't think we'd expect anyone outside this library to use this.

We should probably go back and make the other exported ones @internal at some point as well (breaking anyone who happened to use it; so maybe not until a major version bump; though, unlikely anyone else is using them directly).

return monitorSupportsFeature("aliasSpecs");
}

// sxsRandomIdentifier is a module level global that is transfered to process.env.
// the goal is to detect side by side (sxs) pulumi/pulumi situations for inline programs
// and fail fast. See https://github.com/pulumi/pulumi/issues/7333 for details.
Expand Down
Expand Up @@ -11,7 +11,21 @@ class MyResource extends pulumi.CustomResource {
name,
{},
{
aliases: aliases,
aliases,
parent
}
);
}
}

class MyOtherResource extends pulumi.CustomResource {
constructor(name, aliases, parent) {
super(
"test:index:MyOtherResource",
name,
{},
{
aliases,
parent
}
);
Expand All @@ -21,7 +35,22 @@ class MyResource extends pulumi.CustomResource {
const resource1Aliases = Array.from(new Array(1000).keys()).map(key => `my-alias-name-${key}`);
const resource1 = new MyResource("testResource1", resource1Aliases);
resource1.__aliases.map(alias => alias.apply(aliasName => assert(resource1Aliases.includes(aliasName))));
assert.equal(resource1.__aliases.length, 1000);

const resource2Aliases = Array.from(new Array(1000).keys()).map(key => `my-other-alias-name-${key}`);
const resource2 = new MyResource("testResource2", resource2Aliases, resource1)
resource2.__aliases.map(alias => alias.apply(aliasName => assert(resource2Aliases.includes(aliasName))));
assert.equal(resource2.__aliases.length, 1000);

const resource3Aliases = Array.from(new Array(1000).keys()).map(key => {
return {
name: `my-alias-${key}`,
stack: "my-stack",
project: "my-project",
type: "test:index:MyOtherResource",
}
});
const resource3 = new MyOtherResource("testResource2", resource3Aliases, resource2)
assert.equal(resource3.__aliases.length, 1000);
// We want to ensure that the parent's type is included in computed aliases from the engine
resource3.__aliases[0].apply(aliasName => assert(aliasName.includes("test:index:MyResource")));
17 changes: 8 additions & 9 deletions sdk/nodejs/tests/runtime/langhost/run.spec.ts
Expand Up @@ -1249,14 +1249,13 @@ describe("rpc", () => {
return { urn: makeUrn(t, name), id: undefined, props: undefined };
},
},
/** Skipping this test case as it requires limiting the alias multiplication which occurs */
// "large_alias_lineage_chains": {
// program: path.join(base, "072.large_alias_lineage_chains"),
// expectResourceCount: 1,
// registerResource: (ctx: any, dryrun: boolean, t: string, name: string, res: any, ...args: any) => {
// return { urn: makeUrn(t, name), id: undefined, props: undefined };
// },
// }
"large_alias_lineage_chains": {
program: path.join(base, "072.large_alias_lineage_chains"),
expectResourceCount: 3,
registerResource: (ctx: any, dryrun: boolean, t: string, name: string, res: any, ...args: any) => {
return { urn: makeUrn(t, name), id: undefined, props: undefined };
},
},
};

for (const casename of Object.keys(cases)) {
Expand Down Expand Up @@ -1418,7 +1417,7 @@ describe("rpc", () => {
// SupportsFeature callback
(call: any, callback: any) => {
const resp = new resproto.SupportsFeatureResponse();
resp.setHassupport(false);
resp.setHassupport(true);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure why this was set to false before - do we need to support that? If so, I can just add an opt to switch this on-and-off per test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's OK to set it to true. I don't think we have any tests that assume it's false.

callback(undefined, resp);
},
);
Expand Down
2 changes: 1 addition & 1 deletion tests/go.mod
Expand Up @@ -12,7 +12,7 @@ require (
github.com/blang/semver v3.5.1+incompatible
github.com/golang/protobuf v1.5.2
github.com/hexops/autogold v1.3.0
github.com/pulumi/pulumi/pkg/v3 v3.34.1
github.com/pulumi/pulumi/pkg/v3 v3.49.0
github.com/pulumi/pulumi/sdk/v3 v3.49.0
github.com/stretchr/testify v1.8.0
google.golang.org/grpc v1.51.0
Expand Down
Expand Up @@ -49,7 +49,7 @@ new Component2("unparented", {
class Component3 extends pulumi.ComponentResource {
constructor(name: string, opts: pulumi.ComponentResourceOptions = {}) {
super("my:module:Component3", name, {}, opts);
new Component2(name + "-child", { aliases: [{ parent: opts.parent}], parent: this });
new Component2(name + "-child", { aliases: [{ parent: opts.parent }], parent: this });
}
}

Expand Down