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

yaml-url test has broken names #478

Closed
lblackstone opened this issue Mar 7, 2019 · 17 comments · Fixed by #483
Closed

yaml-url test has broken names #478

lblackstone opened this issue Mar 7, 2019 · 17 comments · Fixed by #483
Assignees
Labels
kind/bug Some behavior is incorrect or out of spec p1 A bug severe enough to be the next item assigned to an engineer
Milestone

Comments

@lblackstone
Copy link
Member

At some point recently, the yaml-url test started printing the following errors for resource names:

urn=urn:pulumi:dev::guestbook::kubernetes:yaml:ConfigFile$kubernetes:apps/v1:Deployment::Calling [toString] on an [Output<T>] is not supported.

To get the value of an Output<T> as an Output<string> consider either:
1: o.apply(v => `prefix${v}suffix`)
2: pulumi.interpolate `prefix${v}suffix`

See https://pulumi.io/help/outputs for more details.
This function may throw in a future version of @pulumi/pulumi./frontend]

Full output looks like this:

$ pulumi up --skip-preview
Updating (yaml-url-step1):

     Type                           Name
 +   pulumi:pulumi:Stack            guestbook-yaml-url-step1
 +   ├─ kubernetes:yaml:ConfigFile  guestbook
 +   │  └─ kubernetes:core:Service  Calling [toString] on an [Output<T>] is not supported.

 +   │  ├─ kubernetes:core:Service  Calling [toString] on an [Output<T>] is not supported.
 +   └─ kubernetes:core:Namespace   test-namespace
 +   │  └─ kubernetes:core:Service  Calling [toString] on an [Output<T>] is not supported.
     Type                              Name
 +   pulumi:pulumi:Stack               guestbook-yaml-url-step1
 +   ├─ kubernetes:yaml:ConfigFile     guestbook
 +   │  ├─ kubernetes:core:Service     Calling [toString] on an [Output<T>] is not supported.
 +   │  ├─ kubernetes:core:Service     Calling [toString] on an [Output<T>] is not supported.
 +   │  ├─ kubernetes:core:Service     Calling [toString] on an [Output<T>] is not supported.
To get the value of an Output<T> as an Output<string> consider either:
 +   │  └─ kubernetes:apps:Deployment  Calling [toString] on an [Output<T>] is not supported.
1: o.apply(v
 +   │  ├─ kubernetes:apps:Deployment  Calling [toString] on an [Output<T>] is not supported.
 +   └─ kubernetes:core:Namespace      test-namespace
 +   │  └─ kubernetes:apps:Deployment  Calling [toString] on an [Output<T>] is not supported.
1: o.apply(v
 +   │  ├─ kubernetes:apps:Deployment  Calling [toString] on an [Output<T>] is not supported.
 +   └─ kubernetes:core:Namespace      test-namespace
 +   │  └─ kubernetes:apps:Deployment  Calling [toString] on an [Output<T>] is not supported.
1: o.apply(v
To get the value of an Output<T> as an Output<string> consider either:
 +   └─ kubernetes:core:Namespace      test-namespace

Resources:
    + 9 created

Duration: 13s

Here's the code for the test that shows the error'd name output:

import * as k8s from "@pulumi/kubernetes";

// Create a test namespace to allow test parallelism.
const namespace = new k8s.core.v1.Namespace("test-namespace");

function addNamespace(o: any) {
  if (o !== undefined) {
    if (o.metadata !== undefined) {
      o.metadata.namespace = namespace.metadata.name;
    } else {
      o.metadata = {namespace: namespace.metadata.name}
    }
  }
}

// Create resources from standard Kubernetes guestbook YAML example in the test namespace.
new k8s.yaml.ConfigFile("guestbook", {
  file: "https://raw.githubusercontent.com/pulumi/pulumi-kubernetes/master/tests/examples/yaml-guestbook/yaml/guestbook.yaml",
  transformations: [addNamespace]
});

but removing the namespace transformation makes it work as expected

import * as k8s from "@pulumi/kubernetes";

// Create a test namespace to allow test parallelism.
const namespace = new k8s.core.v1.Namespace("test-namespace");

// Create resources from standard Kubernetes guestbook YAML example in the test namespace.
new k8s.yaml.ConfigFile("guestbook", {
  file: "https://raw.githubusercontent.com/pulumi/pulumi-kubernetes/master/tests/examples/yaml-guestbook/yaml/guestbook.yaml",
});

Here's the package.json:

{
  "name": "guestbook",
  "version": "0.1.0",
  "dependencies": {
    "@pulumi/pulumi": "dev"
  },
  "devDependencies": {
    "@types/node": "^9.3.0",
    "typescript": "^3.0.0"
  },
  "peerDependencies": {
    "@pulumi/kubernetes": "latest"
  },
  "license": "MIT"
}
@lblackstone lblackstone added kind/bug Some behavior is incorrect or out of spec priority/P1 labels Mar 7, 2019
@metral
Copy link
Contributor

metral commented Mar 7, 2019

I'm hitting this on v0.16.16 w/ no dev packages in my package-lock.json and not using transformation actions. I'm getting them on pulumi & AWS resources:

  • aws:ec2:SecurityGroup
  • cloud:service:Service
  • pulumi:pulumi:Stack

@lukehoban
Copy link
Member

lukehoban commented Mar 7, 2019

Looks like this shipped in almost all recent versions of @pulumi/pulumi:

luke:~/dd/pulumidemos/tostringtest
$ curl -s $(npm view @pulumi/pulumi@v0.16.16 dist.tarball) | zgrep -a "is not supported"

luke:~/dd/pulumidemos/tostringtest
$ curl -s $(npm view @pulumi/pulumi@v0.16.17 dist.tarball) | zgrep -a "is not supported"

luke:~/dd/pulumidemos/tostringtest
$ curl -s $(npm view @pulumi/pulumi@v0.16.18 dist.tarball) | zgrep -a "is not supported"
            const message = `Calling [toString] on an [Output<T>] is not supported.
            const message = `Calling [toJSON] on an [Output<T>] is not supported.

luke:~/dd/pulumidemos/tostringtest
$ curl -s $(npm view @pulumi/pulumi@v0.16.19 dist.tarball) | zgrep -a "is not supported"
            const message = `Calling [toString] on an [Output<T>] is not supported.
            const message = `Calling [toJSON] on an [Output<T>] is not supported.

luke:~/dd/pulumidemos/tostringtest
$ curl -s $(npm view @pulumi/pulumi@v0.17.0 dist.tarball) | zgrep -a "is not supported"
            const message = `Calling [toString] on an [Output<T>] is not supported.
            const message = `Calling [toJSON] on an [Output<T>] is not supported.

luke:~/dd/pulumidemos/tostringtest
$ curl -s $(npm view @pulumi/pulumi@v0.17.1 dist.tarball) | zgrep -a "is not supported"
            const message = `Calling [toString] on an [Output<T>] is not supported.
            const message = `Calling [toJSON] on an [Output<T>] is not supported.

@lukehoban lukehoban added this to the 0.21 milestone Mar 7, 2019
@hausdorff hausdorff changed the title yarl-url test has broken names yaml-url test has broken names Mar 7, 2019
@CyrusNajmabadi
Copy link
Contributor

Looks like this shipped in almost all recent versions of @pulumi/pulumi:

We return that string from toString/toJSON. But we don't log that message. Logging was only in 16.18 and was rolled back in 16.19.

@CyrusNajmabadi
Copy link
Contributor

Reopening since someone is still calling toString

@CyrusNajmabadi CyrusNajmabadi removed their assignment Mar 7, 2019
@CyrusNajmabadi
Copy link
Contributor

CyrusNajmabadi commented Mar 7, 2019

removing my assignment. if you still get htis after moving to 16.19, i can help you determine who is doing this.

One thing you can do to help track this down is change the code inside Output.toString to instead call throw new Error(message);. Then you'll get a stack with the offending logic.

@hausdorff
Copy link
Contributor

Sorry, just to clarify my understanding:

  • pulumi-kubernetes seems like it's calling toString
  • We should not be, therefore this issue should remain open
  • Kube team can investigate, and we should definitely solve this for M21; this does not seem hard to track down.

@hausdorff hausdorff reopened this Mar 7, 2019
@CyrusNajmabadi
Copy link
Contributor

pulumi-kubernetes seems like it's calling toString

It could be anyone doing this. including @pulumi/pulumi.

@hausdorff
Copy link
Contributor

Do you think that @metral running into this problem is evidence that this happening somewhere other than pulumi-kubernetes?

@metral
Copy link
Contributor

metral commented Mar 7, 2019

Can confirm that the message goes away after blowing away package-lock.json & node_modules, and re-doing an npm install. This now pins @pulumi/pulumi to v0.16.19 (instead of v0.16.18 before) and resolves the issue.

@lblackstone
Copy link
Member Author

So I'm not quite sure how to fix it, but I tracked down the offending line by throwing an error in the toString method.

Here's the stack trace:

    error: Running program '/Users/levi/go/src/github.com/pulumi/pulumi-kubernetes/tests/integration/yaml-url/step1' failed with an unhandled exception:
    error: ReferenceError: BaseError is not defined
        at Proxy.OutputImpl.toString (/Users/levi/go/src/github.com/pulumi/pulumi-kubernetes/tests/integration/yaml-url/step1/node_modules/@pulumi/kubernetes/node_modules/@pulumi/pulumi/output.js:59:23)
        at parseYamlObject (/Users/levi/go/src/github.com/pulumi/pulumi-kubernetes/tests/integration/yaml-url/step1/node_modules/@pulumi/kubernetes/provider.js:256:21)
        at parseYamlDocument (/Users/levi/go/src/github.com/pulumi/pulumi-kubernetes/tests/integration/yaml-url/step1/node_modules/@pulumi/kubernetes/provider.js:133:33)
        at ConfigFile.resources.pulumi.output.apply.t (/Users/levi/go/src/github.com/pulumi/pulumi-kubernetes/tests/integration/yaml-url/step1/node_modules/@pulumi/kubernetes/provider.js:123:61)
        at OutputImpl.<anonymous> (/Users/levi/go/src/github.com/pulumi/pulumi-kubernetes/tests/integration/yaml-url/step1/node_modules/@pulumi/kubernetes/node_modules/@pulumi/pulumi/output.js:103:47)
        at next (native)
        at /Users/levi/go/src/github.com/pulumi/pulumi-kubernetes/tests/integration/yaml-url/step1/node_modules/@pulumi/kubernetes/node_modules/@pulumi/pulumi/output.js:20:71
        at __awaiter (/Users/levi/go/src/github.com/pulumi/pulumi-kubernetes/tests/integration/yaml-url/step1/node_modules/@pulumi/kubernetes/node_modules/@pulumi/pulumi/output.js:16:12)
        at exports.Output.promise.then (/Users/levi/go/src/github.com/pulumi/pulumi-kubernetes/tests/integration/yaml-url/step1/node_modules/@pulumi/kubernetes/node_modules/@pulumi/pulumi/output.js:90:70)
        at process._tickCallback (internal/process/next_tick.js:109:7)
    error: an unhandled error occurred: Program exited with non-zero exit code: 1

And here's the offending code:

const meta = obj["metadata"];
let id: string = meta["name"];
const namespace = meta["namespace"] || undefined;
if (namespace !== undefined) {
id = `${namespace}/${id}`;
}

(Line 2200)

@CyrusNajmabadi
Copy link
Contributor

It sounds like this has found an actual bug. One, or both, of 'id' or 'namespace' are Outputs. So doing this concatenation is just not valid. If these can be Outputs, then this should be written as id = pulumi.all([namespace, id]).apply(([namespace, id]) => ${namespace}/${id}).

However, this will lead to further problems based on how 'id' is used below.

@lblackstone
Copy link
Member Author

Yeah, confirmed that namespace is an Output in this case. Trying to figure out the best way to fix this, as it has wide-ranging effects on the surrounding code.

@lblackstone
Copy link
Member Author

Given the wider impact caused by fixing the buggy line, this won't be fixed in M21, but will be addressed early in M22

@lblackstone lblackstone modified the milestones: 0.21, 0.22 Mar 8, 2019
@lukehoban
Copy link
Member

Does this affect users? Or is it specific to this test? Also - was this always broken (as in did it previously emit [object Object] in names? I can’t quite follow how it could have worked correctly before but not anymore.

@lblackstone
Copy link
Member Author

@lukehoban It looks like it's been broken since the feature was introduced last August, but it wasn't on the common path. #409 is caused by the same underlying bug.

In the common case, namespace was a string; this bug only manifests in cases where namespace was a computed value, like using a transformation on a yaml file.

@hausdorff
Copy link
Contributor

This specific problem happens when you do something like programmatically set a value (such as namespace) in transforms -- if that value is Output<T>, this cause us to be sad. Looking at the code, it looks like if we change parseYamlObject to return Output, this will cause parseYamlDocument to return Output, which should be fine because CollectionComponentResource#resources is already an Output.

We should be able to fix this quickly, I'm guessing...

@lukehoban
Copy link
Member

#409 is caused by the same underlying bug.

Indeed - this is precisely a dupe of #409 then - just the details of the text that get rendered changed.

@infin8x infin8x added the p1 A bug severe enough to be the next item assigned to an engineer label Jul 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Some behavior is incorrect or out of spec p1 A bug severe enough to be the next item assigned to an engineer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants