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

[cli] Protect against panic when using incorrect token type in pulumi import #7202

Merged
merged 1 commit into from
Jun 4, 2021

Conversation

stack72
Copy link
Contributor

@stack72 stack72 commented Jun 3, 2021

Fixes: #6990

Previously:

pulumi import aws:ec2/instance/Instance id-12345 test-instance
Previewing import (dev)

================================================================================
The Pulumi CLI encountered a fatal error. This is a bug!
We would appreciate a report: https://github.com/pulumi/pulumi/issues/
Please provide all of the below text in your report.
================================================================================
Pulumi Version:   3.4.0-alpha.1622658772+dbea8058.dirty
Go Version:       go1.16.4
Go Compiler:      gc
Architecture:     amd64
Operating System: darwin
Panic:            fatal: An assertion has failed: Module member token 'aws:ec2/instance/Instance' missing module member delimiter

After:

pulumi import aws:ec2/instance/Instance id-12345 test-instance
Previewing import (dev)

error: import type "aws:ec2/instance/Instance" is not a valid resource type - refer to the import section of the provider resource documentation.

@stack72 stack72 requested review from komalali and justinvp June 3, 2021 18:21
Copy link
Member

@lukehoban lukehoban left a comment

Choose a reason for hiding this comment

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

Can we add a test for this?

This feels like an okay fix for now to address the common user-facing issue - but I will say I think we will need to directionally do one of two things:

  1. Ensure that we validate all strings that we convert to type tokens at the point we take them from the outside world, and produce useful error messages there - and then assume that all things typed as token.Type are valid
  2. Admit that we aren't cleanly ensuring that all things typed as token.Type are valid, and change the API for token.Type to return errors instead of asserting (which would require a lot of changes to consumers to handle all the new error cases.

I personally think we should be doing (1). The changes in this PR aren't really either (1) or (2) - they are patching the problem in between these two in one place (but I'll bet there are dozens more like this as latent issues).

pkg/engine/deployment.go Outdated Show resolved Hide resolved
pkg/engine/deployment.go Show resolved Hide resolved
@stack72
Copy link
Contributor Author

stack72 commented Jun 3, 2021

  1. Ensure that we validate all strings that we convert to type tokens at the point we take them from the outside world, and produce useful error messages there - and then assume that all things typed as token.Type are valid

I definitely think we need to more in advance here - we can do something at the cli layer but we'd need to make sure that work is correct. Would you be ok for me to add a follow up issue here to make sure we investigate that but proceed with this to step any panics for now?

@lukehoban
Copy link
Member

Would you be ok for me to add a follow up issue here to make sure we investigate that but proceed with this to step any panics for now?

Yep - happy to open a follow-up issue. Basically any place we use types.Token(<string>) should actually be parsing the results and returning an error if the string is not a token.

$ grep -r --include "*.go" --exclude "*_test.go" "tokens.Type("  .
./sdk/go/common/resource/plugin/provider_server.go:     typ, name, parent := tokens.Type(req.GetType()), tokens.QName(req.GetName()), resource.URN(req.GetParent())
./sdk/go/common/resource/urn.go:        return tokens.Type(strings.Split(urn.URNName(), URNNameDelimiter)[2])
./sdk/go/common/resource/urn.go:        return tokens.Type(lastType)
./sdk/go/pulumi/mocks.go:       parentType := tokens.Type("")
./sdk/go/pulumi/mocks.go:       return string(resource.NewURN(tokens.QName(m.stack), tokens.PackageName(m.project), parentType, tokens.Type(typ),
./pkg/cmd/pulumi/import.go:             Type:    tokens.Type(typ),
./pkg/operations/operations_cloud_aws.go:       cloudFunctionType     = tokens.Type("cloud:function:Function")
./pkg/operations/operations_cloud_aws.go:       cloudLogCollectorType = tokens.Type("cloud:logCollector:LogCollector")
./pkg/operations/operations_cloud_aws.go:       cloudServiceType      = tokens.Type("cloud:service:Service")
./pkg/operations/operations_cloud_aws.go:       cloudTaskType         = tokens.Type("cloud:task:Task")
./pkg/operations/operations_gcp.go:     gcpFunctionType = tokens.Type("gcp:cloudfunctions/function:Function")
./pkg/operations/resources.go:                  childURN.Type() == tokens.Type(typ) &&
./pkg/operations/operations_aws.go:     awsFunctionType = tokens.Type("aws:lambda/function:Function")
./pkg/operations/operations_aws.go:     awsLogGroupType = tokens.Type("aws:cloudwatch/logGroup:LogGroup")
./pkg/resource/deploy/deployment.go:    parentType := tokens.Type("")
./pkg/resource/deploy/providers/reference.go:   return tokens.Type("pulumi:providers:" + pkg)
./pkg/resource/deploy/source_eval.go:           t = tokens.Type(req.GetType())

The case that causes this particular bug is in the middle of the results above. Most of the others look like they are probably safe - but should likely still be using a parsing function and propagating an error just to be sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"The Pulumi CLI encountered a fatal error. This is a bug!" while trying pulumi import
2 participants