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

Typescript: dynamic.Resource outputs are not available if defined on Resource class #11639

Open
awoimbee opened this issue Feb 14, 2023 · 8 comments
Assignees
Labels
area/docs Improvements or additions to documentation docs/content kind/bug Some behavior is incorrect or out of spec language/javascript

Comments

@awoimbee
Copy link

awoimbee commented Feb 14, 2023

What happened?

I tried to follow the docs to create a custom resource (to generate a password and its hash), but the outputs are always undefined. I figured out that this is a bug caused by the class member definitions, as removing them make it work.

Expected Behavior

It should have worked on the first try. I closely followed https://www.pulumi.com/docs/intro/concepts/resources/dynamic-providers/#dynamic-resource-outputs.

Steps to reproduce

Original code:

import * as pulumi from "@pulumi/pulumi";
import { strict as assert } from "node:assert";
import * as crypto from "node:crypto";

const _hashedPasswordProvider: pulumi.dynamic.ResourceProvider = {
  async create (inputs: {length: number}) {
    const password = "dummy";
    const hashedPassword = "my-super-pass-hashed-with-unique-salt";

    return { id: crypto.randomUUID(), outs: { password, hashedPassword } };
  }
};

class HashedPasswordResource extends pulumi.dynamic.Resource {
  public readonly password!: pulumi.Output<string>;
  public readonly hashedPassword!: pulumi.Output<string>;

  constructor (name: string, props: {length: pulumi.Input<number>}, opts?: pulumi.CustomResourceOptions) {
    const innerOpts = pulumi.mergeOptions(opts, { additionalSecretOutputs: ["password"] });
    const innerProps = {password: undefined, hashedPassword: undefined, ...props};
    super(_hashedPasswordProvider, name, innerProps, innerOpts);
  }
}

function main() {
  const botUsername = "robotsatan";
  const botPassword = new HashedPasswordResource("robotsatan-password", { length: 30 });

  // kaboom
  assert.notEqual(botPassword.password, undefined);
  assert.notEqual(botPassword.hashedPassword, undefined);
}

Diff to make it work:

-  public readonly password!: pulumi.Output<string>;
-  public readonly hashedPassword!: pulumi.Output<string>;

-  const botPassword = new HashedPasswordResource("robotml-password", { length: 30 });
+  const botPassword = <any>new HashedPasswordResource("robotml-password", { length: 30 });

Output of pulumi about

CLI
Version      3.54.0
Go Version   go1.20
Go Compiler  gc

Plugins
NAME    VERSION
nodejs  unknown

Host
OS       arch
Version  22.0.2
Arch     x86_64

This project is written in nodejs: executable='/usr/bin/node' version='v19.5.0'

my package-lock.json says I have node_modules/@pulumi/pulumi version": "3.54.0",

Additional context

Shout-out to the poor souls @ pulumi/pulumi#2931

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

EDIT

Ok, this stuff is VERY freaking unstable.
I changed my resource to return Buffer.from(hash).toString('base64'); instead of hash => it started returning undefined again until I renamed my resource !

@awoimbee awoimbee added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team labels Feb 14, 2023
@Frassle Frassle added language/javascript and removed needs-triage Needs attention from the triage team labels Feb 16, 2023
@Frassle
Copy link
Member

Frassle commented Feb 16, 2023

I think this is a limitation of TypeScript, that's not been well documented. The code line public readonly password!: pulumi.Output<string>; doesn't actually do anything at runtime, it's just a hint to the TypeScript compiler.

So the TypeScript SDK can't use property reflection to work out what fields to set because just declaring them doesn't make a field, so instead it works out the output fields to set based on the input properties it gets given. If you look at any of the codegen'd sdks you'll see a lot of resourceInputs["stuff"] = undefined (e.g. https://github.com/pulumi/pulumi-aws/blob/master/sdk/nodejs/acm/certificate.ts#L319) which is just to tell the runtime that "stuff" is a field it should set later on.

If writing dynamic providers you need to copy that style of passing in input properties.

I'll move this over to the docs repo that we should update the examples at https://www.pulumi.com/docs/intro/concepts/resources/dynamic-providers/#dynamic-resource-outputs

@Frassle Frassle transferred this issue from pulumi/pulumi Feb 16, 2023
@susanev susanev added the area/docs Improvements or additions to documentation label Mar 27, 2023
@interurban interurban self-assigned this Aug 4, 2023
@JesusTheHun
Copy link

JesusTheHun commented Nov 3, 2023

@Frassle

class Test {
  public readonly password!: string;
}

const test = new Test();

console.log(test.hasOwnProperty('password')); // true
console.log(test); // Test { password: undefined }

So I'm confused about what the issue actually is.

EDIT : Also, a resource can have an output field that is not in the input. So how do you populate that ?

@Frassle
Copy link
Member

Frassle commented Nov 3, 2023

So I'm confused about what the issue actually is.

Huh interesting. If that's the case we can probably simplify codegen for custom resources as well.

@JesusTheHun
Copy link

@Frassle I’m very happy for you, but that doesn’t solve the issue 😅 : the resource properties are not populated with previous output

@Frassle
Copy link
Member

Frassle commented Nov 3, 2023

Hah my thought was if that's true we can simplify codegen and fix this to work with those annotated properties as well. At which point the above just works.

If you need to work with problem of outputs that don't have inputs today, the current codegen just fills in the props dictionary with undefined. e.g. https://github.com/pulumi/pulumi-aws/blob/master/sdk/nodejs/acm/certificate.ts#L321
I'd copy that.

@JesusTheHun
Copy link

@Frassle

Hah my thought was if that's true we can simplify codegen and fix this to work with those annotated properties as well. At which point the above just works.

Now I'm very confused.

The code you pointed assign default values to the inputs passed to the custom provider. Can you explain how it's related to a resource output ? My understanding was that you can have outputs that are different than inputs, which seem contradicted by your statement.

Does the resource actually receive a merge of inputs & outputs as second parameter during an update ? That's what it looks like when I read https://github.com/pulumi/pulumi-aws/blob/master/sdk/nodejs/acm/certificate.ts#L321

const botUsername = "robotsatan";
const botPassword = new HashedPasswordResource("robotsatan-password", { length: 30 });

botPassword.password.apply(pwd => {
  console.log(pwd); // undefined during 'deployment time', create or update lifecycle
});

@Frassle
Copy link
Member

Frassle commented Nov 3, 2023

My understanding was that you can have outputs that are different than inputs, which seem contradicted by your statement.

You can. The nodesdk just has this oddity of needing to also set the outputs so that the runtime knows what fields to deserialise to in the response. You don't need actual values for them, undefined works but the names need to be in the map.

I'd guess this is because hasOwnProperty either wasn't thought about or didn't work for some reason. But thus my comment that we might be able to simplify a lot here if hasOwnProperty does work for this case.

@Ernxst
Copy link

Ernxst commented Apr 21, 2024

If anyone is still interested, I fixed it by using declare. Using the original example, replace public with the declare keyword:

import * as pulumi from "@pulumi/pulumi";
import { strict as assert } from "node:assert";
import * as crypto from "node:crypto";

const _hashedPasswordProvider: pulumi.dynamic.ResourceProvider = {
  async create (inputs: {length: number}) {
    const password = "dummy";
    const hashedPassword = "my-super-pass-hashed-with-unique-salt";

    return { id: crypto.randomUUID(), outs: { password, hashedPassword } };
  }
};

class HashedPasswordResource extends pulumi.dynamic.Resource {
  declare readonly password: pulumi.Output<string>;
  declare readonly hashedPassword: pulumi.Output<string>;

  constructor (name: string, props: {length: pulumi.Input<number>}, opts?: pulumi.CustomResourceOptions) {
    const innerOpts = pulumi.mergeOptions(opts, { additionalSecretOutputs: ["password"] });
    const innerProps = {password: undefined, hashedPassword: undefined, ...props};
    super(_hashedPasswordProvider, name, innerProps, innerOpts);
  }
}

function main() {
  const botUsername = "robotsatan";
  const botPassword = new HashedPasswordResource("robotsatan-password", { length: 30 });

  // now passes
  assert.notEqual(botPassword.password, undefined);
  assert.notEqual(botPassword.hashedPassword, undefined);
}

Found this out after commenting out the property definitions then worked backwards to find a way to make them visible to TypeScript. Would be nice if the docs were updated to reflect this.

@sean1588 sean1588 transferred this issue from pulumi/pulumi-hugo May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Improvements or additions to documentation docs/content kind/bug Some behavior is incorrect or out of spec language/javascript
Projects
Status: 🧳 Backlog
Development

No branches or pull requests

6 participants