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

Failure to serialize YAML #58

Closed
lukehoban opened this issue Feb 4, 2019 · 7 comments · Fixed by #231
Closed

Failure to serialize YAML #58

lukehoban opened this issue Feb 4, 2019 · 7 comments · Fixed by #231
Assignees
Labels
help-wanted We'd love your contributions on this issue
Milestone

Comments

@lukehoban
Copy link
Member

A user reported that they started seeing the error below in CI (they believe with no code changes).

 +  pulumi:pulumi:Stack eks-cluster-eks-cluster-cd-test creating YAMLException: unacceptable kind of an object to dump [object Undefined]
 +  pulumi:pulumi:Stack eks-cluster-eks-cluster-cd-test creating     at writeNode (/builds/org/app/example/pulumi/eks-cluster/node_modules/js-yaml/lib/js-yaml/dumper.js:756:13)
 +  pulumi:pulumi:Stack eks-cluster-eks-cluster-cd-test creating     at writeBlockMapping (/builds/org/app/example/pulumi/eks-cluster/node_modules/js-yaml/lib/js-yaml/dumper.js:634:10)
 +  pulumi:pulumi:Stack eks-cluster-eks-cluster-cd-test creating     at writeNode (/builds/org/app/example/pulumi/eks-cluster/node_modules/js-yaml/lib/js-yaml/dumper.js:727:9)
 +  pulumi:pulumi:Stack eks-cluster-eks-cluster-cd-test creating     at writeBlockSequence (/builds/org/app/example/pulumi/eks-cluster/node_modules/js-yaml/lib/js-yaml/dumper.js:521:9)
 +  pulumi:pulumi:Stack eks-cluster-eks-cluster-cd-test creating     at writeNode (/builds/org/app/example/pulumi/eks-cluster/node_modules/js-yaml/lib/js-yaml/dumper.js:740:9)
 +  pulumi:pulumi:Stack eks-cluster-eks-cluster-cd-test creating     at dump (/builds/org/app/example/pulumi/eks-cluster/node_modules/js-yaml/lib/js-yaml/dumper.js:817:7)
 +  pulumi:pulumi:Stack eks-cluster-eks-cluster-cd-test creating     at Object.safeDump (/builds/org/app/example/pulumi/eks-cluster/node_modules/js-yaml/lib/js-yaml/dumper.js:823:10)
 +  pulumi:pulumi:Stack eks-cluster-eks-cluster-cd-test creating     at pulumi.all.apply (/builds/org/app/example/pulumi/eks-cluster/node_modules/@pulumi/cluster.ts:216:27)
 +  pulumi:pulumi:Stack eks-cluster-eks-cluster-cd-test creating     at Output.<anonymous> (/builds/org/app/example/pulumi/eks-cluster/node_modules/@pulumi/pulumi/resource.js:279:47)
 +  pulumi:pulumi:Stack eks-cluster-eks-cluster-cd-test creating     at Generator.next (<anonymous>)
 +  pulumi:pulumi:Stack eks-cluster-eks-cluster-cd-test creating error: Running program '/builds/org/app/example/pulumi/eks-cluster' failed with an unhandled exception:
 +  pulumi:pulumi:Stack eks-cluster-eks-cluster-cd-test creating error: YAMLException: unacceptable kind of an object to dump [object Undefined]
@lukehoban
Copy link
Member Author

The fact that jsyaml.safeDump throws on undefineds is unfortunate here. Upstream tracking for that is here: nodeca/js-yaml#325. Presumably we should be handling that failure and providing better context given how unhelpful this failure mode is.

@lukehoban lukehoban self-assigned this Feb 4, 2019
@lukehoban lukehoban added this to the 0.21 milestone Feb 4, 2019
@lukehoban lukehoban assigned metral and unassigned lukehoban Feb 11, 2019
@metral metral modified the milestones: 0.21, 0.22 Mar 6, 2019
@lukehoban lukehoban modified the milestones: 0.22, 0.23 Apr 12, 2019
@lukehoban lukehoban removed this from the 0.23 milestone May 27, 2019
@lukehoban lukehoban added the help-wanted We'd love your contributions on this issue label May 27, 2019
@lukehoban
Copy link
Member Author

As well as fixing the error reporting, it's not clear whether this indicates user error (and what user error), or whether it's a bug in the library that allows undefineds to flow into this code. @metral?

@casey-robertson
Copy link

Also having this issue. So also user error or a bug :-)

@casey-robertson
Copy link

casey-robertson commented Aug 7, 2019

OK so in my particular case it was user error. We each develop in our own AWS account and my account was not provisioned with a couple of the roles I was trying to map into the cluster roleMappings. These roles were being grabbed via aws.iam.Role and then added to the roleMappings config as variable.arn. I think this code is where the issue lies:

const roleMappings = pulumi.all([pulumi.output(args.roleMappings || []), instanceRoleMappings])

Line 317 can then possibly return bad objects.

return jsyaml.safeDump([...mappings, ...instanceMappings].map(m => ({

Line 326

if (args.userMappings !== undefined) {

that does the userMappings checks for undefined and would skip that block. So this is a use case where the code is assuming a good aws.iam.Role because under default conditions (where no roleMappings are defined), the program operates OK with an empty array,
but is not checking if I pass it a bad/invalid object.

@lukehoban lukehoban added this to the 0.26 milestone Aug 8, 2019
@lukehoban
Copy link
Member Author

These roles were being grabbed via aws.iam.Role and then added to the roleMappings config as variable.arn.

Could you share any more details on this? Do you mean you were using aws.iam.Role.get()? And that was returning a Role with an undefined arn instead of throwing in the case that you didn't have that Role in your account?

@casey-robertson
Copy link

Correct. A few of our accounts were hand-spun due to time constraints. I had assumed a couple of roles had been created when in fact they had not.

@lukehoban
Copy link
Member Author

Got it. The root cause of that is pulumi/pulumi-terraform#262 which leads to the .get calls not reporting an error and isntead allowing you to see undefineds that you didn't expect. You can of course work around that by checking for undefined yourself, but it's not clear you should have to.

We will be looking into fixing that issue, as well as improving the error reporting on the YAML serialization in this package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help-wanted We'd love your contributions on this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants