New Identity Model in the CLI #1226

ellismg · 2018-04-18T10:54:23Z

This set of changes removes the need to do pulumi init and adopts the new stack identity model when pulumi init has not been run.

We retain pulumi init for now, as customers are using it and it's the only way to access stacks created under the old model.

A side effect of this work is we've had to move around some of our bookkeeping data, and when upgrading to this version of the CLI (or newer), customers with have to pulumi stack select their stack again, as the location of the book-keeping file has moved and we did not do the work to add the migration code.

I'll write more detailed changelog notes shortly.

Upcoming work to remove the need for `pulumi init` makes testing the upgrade code much harder than it did in the past (since workspace data is moving to a different location on the file system, as well as some other changes). Instead of trying to maintain the code and test, let's just remove it. Folks who haven't migrated (LM and the PPC deployment in the service) should use the 0.11.3 or earlier CLI to migrate their projects (simply by logging in and running a pulumi command) or move things forward by hand.

Our normal lifecycle tests always call pulumi stack rm, but some of the tests that used the more barebones framework did not do so. This was "ok" in the past, since all bookkeeping data about a stack was stored next to the Pulumi program itself and we deleted that folder once the test passed. As part of removing `pulumi init` workspace tracking will move to ~/.pulumi/workspaces and so we'd like to have a gesture that actually removes the stack, which will cause the workspace file to be removed as well, instead of littering ~/.pulumi/workspaces with tests.

ellismg · 2018-04-18T10:54:52Z

CI will fail here until we promote testing to staging, as we now generate stack names larger than 40 characters.

This change removes the need to `pulumi init` when targeting the local backend. A fair amount of the change lays the foundation that the next set of changes to stop having `pulumi init` be used for cloud stacks as well. Previously, `pulumi init` logically did two things: 1. It created the bookkeeping directory for local stacks, this was stored in `<repository-root>/.pulumi`, where `<repository-root>` was the path to what we belived the "root" of your project was. In the case of git repositories, this was the directory that contained your `.git` folder. 2. It recorded repository information in `<repository-root>/.pulumi/repository.json`. This was used by the cloud backend when computing what project to interact with on Pulumi.com The new identity model will remove the need for (2), since we only need an owner and stack name to fully qualify a stack on pulumi.com, so it's easy enough to stop creating a folder just for that. However, for the local backend, we need to continue to retain some information about stacks (e.g. checkpoints, history, etc). In addition, we need to store our workspace settings (which today just contains the selected stack) somehere. For state stored by the local backend, we change the URL scheme from `local://` to `local://<optional-root-path>`. When `<optional-root-path>` is unset, it defaults to `$HOME`. We create our `.pulumi` folder in that directory. This is important because stack names now must be unique within the backend, but we have some tests using local stacks which use fixed stack names, so each integration test really wants its own "view" of the world. For the workspace settings, we introduce a new `workspaces` directory in `~/.pulumi`. In this folder we write the workspace settings file for each project. The file name is the name of the project, combined with the SHA1 of the path of the project file on disk, to ensure that multiple pulumi programs with the same project name have different workspace settings. This does mean that moving a project's location on disk will cause the CLI to "forget" what the selected stack was, which is unfortunate, but not the end of the world. If this ends up being a big pain point, we can certianly try to play games in the future (for example, if we saw a .git folder in a parent folder, we could store data in there). With respect to compatibility, we don't attempt to migrate older files to their newer locations. For long lived stacks managed using the local backend, we can provide information on where to move things to. For all stacks (regardless of backend) we'll require the user to `pulumi stack select` their stack again, but that seems like the correct trade-off vs writing complicated upgrade code.

To prepare for a world where stack names must be unique across an owner, add some randomness to the names we use for stacks as part of our integration tests.

Long term, a stack name alone will not be sufficent to address a stack. Introduce a new `backend.StackReference` interface that allows each backend to give an opaque stack reference that can be used across operations.

This change introduces support for using the cloud backend when `pulumi init` has not been run. When this is the case, we use the new identity model, where a stack is referenced by an owner and a stack name only. There are a few things going on here: - We add a new `--owner` flag to `pulumi stack init` that lets you control what account a stack is created in. - When listing stacks, we show stacks owned by you and any organizations you are a member of. So, for example, I can do: * `pulumi stack init my-great-stack` * `pulumi stack init --owner pulumi my-great-stack` To create a stack owned by my user and one owned by my organization. When `pulumi stack ls` is run, you'll see both stacks (since they are part of the same project). - When spelling a stack on the CLI, an owner can be optionally specified by prefixing the stack name with an owner name. For example `my-great-stack` means the stack `my-great-stack` owned by the current logged in user, where-as `pulumi/my-great-stack` would be the stack owned by the `pulumi` organization - `--all` can be passed to `pulumi stack ls` to see *all* stacks you have access to, not just stacks tied to the current project.

joeduffy · 2018-04-18T14:54:36Z

CI will fail here until we promote testing to staging, as we now generate stack names larger than 40 characters.

@lukehoban @chrsmith Didn't we promote testing to staging yesterday?

chrsmith

This looks good. It's kind of a bummer how much churn was involved, but then again, stack identity is pretty fundamental to everything.

I'd like to take another look at specific the CLI UX changes (parameter names, help text) and the impact to our existing test suite. But don't block submitting on that if you don't hear back fro me.

chrsmith · 2018-04-18T15:16:42Z

pkg/backend/cloud/backend.go

+	var stackName tokens.QName
+
+	if len(split) == 1 {
+		stackName = tokens.QName(split[0])


Can we remove the reference to tokens.QName here, like we have in all the other places? I assume it's just changing cloudBackendReference?

Thanks for pointing this out. I changed the local to be of type string and then convert it to a tokens.QName when constructing the cloudBackendReference and the code is a littler clearer now.

chrsmith · 2018-04-18T15:18:16Z

pkg/backend/cloud/backend.go

-	if err != nil {
-		return nil, err
+// StackReferenceParseOptions is an optional bag of options specific to parsing stack references.
+type StackReferenceParseOptions struct {


Since this type is actually specific to the "cloud" backend, would it be more appropriate to call this CloudBackendStackReferenceParseOptions. That's clearly too long, but arguably less confusing then wondering why StackReferenceParseOptions is in pkg/backends/cloud instead of just pkg/backend when dealing with non-cloud stacks.

Yup. I'll change this. We might end up ripping this type out if we don't want to support --owner in pulumi stack init and instead just force folks to pass the owner like <owner>/<new-stack-name>, but if the type ends up staying around, this is a nice improvement.

Actually, thinking on this a little more, I'm not going to do this. The type is already in the cloud package, and we don't do a similar thing for cloud.CreateStackOptions, which is also an options bag that we pass through an interface{} and is specific to the cloud back-end.

And, talking with Joe, I think we'll just remove the need for this type in our implementation today, by not needing to support --owner on pulumi stack init and instead just have folks pass the owner name as part of the stack name (e.g. pulumi stack init pulumi/my-great-stack)

chrsmith · 2018-04-18T15:19:16Z

pkg/backend/cloud/backend.go

@@ -921,24 +972,40 @@ func getCloudProjectIdentifier() (client.ProjectIdentifier, error) {
 	}

 	repo := w.Repository()
+	// If we have repository information (this is the case when `pulumi init` has been run, use that)


Nit: End with a period.

chrsmith · 2018-04-18T15:20:59Z

pkg/backend/cloud/backend.go

+	}
+
+	if owner == "" {
+		owner, err = b.client.DescribeUser()


Could we rename DescribeUser to be more accurate? Such as GetUserLogin or GetPulumiAccountName or something? Previously we were using DescribeUser to just confirm an access token was valid (and not caring about the result really), and now it is a critical part of how we identify stacks. So we should be more specific in describing what the method is supposed to do.

I'll change this to GetPulumiAccountName()

chrsmith · 2018-04-18T15:23:02Z

pkg/backend/backend.go

@@ -12,44 +14,53 @@ import (
 	"github.com/pulumi/pulumi/pkg/workspace"
 )

+type StackReference interface {
+	fmt.Stringer
+	EngineName() tokens.QName


Could you please add a doc comment for this exported type? What does a StackReference do and how do I use it? i.e. is String() going to return "owner/name"? If I have a StackReference how can I get its owner value? (Or do we expect this type to be vague enough so that for local stacks there is no notion of "owner".)

Similarly, why does EngineName exist and when should I use it?

You and @joeduffy where both confused about this, which is a pretty good indication I screwed up here. The goal is that a StackReference is an opaque value to the CLI, but it something that makes sense to the backend. A backend is able to take a string a user provides on the command line (e.g ellismg/my-great-stack) and turn that into a StackReference which provides enough context for other engine operations.

In this world, asking for the Owner of a stack reference from within the CLI layer isn't meaninful. The String() method via fmt.Stringer provides a user presentable stack name (which in some cases would include an owner name).

EngineName() gives you the name of the stack as seen by the engine itself, which works its way into URNs and the like. In the case of the cloud backend, this does not include any owner information, as things were before. I'll come up with a better name here and add some comments, but let me know if the above doesn't seem to make sense.

chrsmith · 2018-04-18T15:24:49Z

pkg/backend/cloud/client/client.go

-	if err := pc.restCall("POST", getProjectPath(project, "stacks"), nil, &createStackReq, &createStackResp); err != nil {
-		return apitype.Stack{}, err
+	if project.Repository != "" {
+		if err := pc.restCall("POST", getProjectPath(project, "stacks"), nil,


Nit: IMHO, if it is possible to just put err := ... on one line, then I'd personally prefer to have that then the line break in the middle of an expression in the middle of an if-statement. As it's just slightly harder to mentally parse. (But feel free to ignore if you disagree.)

I do not disagree. However, the line breaking is to appease the long lines linter.

Does this read better (putting all the arguments on a single line) ?

if err := pc.restCall( "POST", getProjectPath(project, "stacks"), nil, &createStackReq, &createStackResp); err != nil { return apitype.Stack{}, err }

chrsmith · 2018-04-18T15:25:58Z

pkg/backend/cloud/stack.go

-	b         *cloudBackend    // a pointer to the backend this stack belongs to.
+	name      backend.StackReference // the stack's name.
+	cloudURL  string                 // the URL to the cloud containing this stack.
+	orgName   string                 // the organization that owns this stack.


I know we don't want to go too deep just refactoring code, but perhaps renaming this to "owner" would be more consistent with the way we refer to it elsewhere in the code? i.e. just standardize on stack "owner" and stack "name"?

I'll do this, as an isolated commit to keep history tidy.

joeduffy · 2018-04-18T15:01:21Z

cmd/stack_init.go

 			return err
 		}),
 	}
 	cmd.PersistentFlags().StringVarP(
-		&ppc, "ppc", "p", "", "A Pulumi Private Cloud (PPC) name to initialize this stack in (if not --local)")
+		&owner, "owner", "o", "", "The owner for the new stack (defaults to current user)")


I thought we agreed we would parse owners as part of the stack names: <stack> means "current user" is the owner, while <owner>/<stack> means <owner> is the owner.

joeduffy · 2018-04-18T15:02:23Z

cmd/stack_ls.go

 				return errors.New("no Pulumi.yaml found; please run this command in a project directory")
 			}

+			proj, err := workspace.LoadProject(projPath)


Could this sequence (including the above calls) be simplified to simply use workspace.DetectProject?

joeduffy · 2018-04-18T15:03:34Z

cmd/stack_ls.go

@@ -104,4 +115,8 @@ func newStackLsCmd() *cobra.Command {
 			return result
 		}),
 	}
+	cmd.PersistentFlags().BoolVar(


-a as a short flag?

joeduffy · 2018-04-18T15:05:00Z

pkg/backend/backend.go

@@ -12,44 +14,53 @@ import (
 	"github.com/pulumi/pulumi/pkg/workspace"
 )

+type StackReference interface {
+	fmt.Stringer
+	EngineName() tokens.QName


Can you comment the exported parts of this interface? (The interface itself, and the EngineName function.) I haven't yet been able to piece together what exactly EngineName represents...

joeduffy · 2018-04-18T15:05:24Z

pkg/backend/cloud/backend.go

+	var owner string
+
+	if opts != nil {
+		parseOpts, ok := opts.(StackReferenceParseOptions)


Should this be an assertion?

Removing this type.

joeduffy · 2018-04-18T15:13:05Z

pkg/backend/cloud/client/client.go

@@ -172,8 +199,16 @@ func (pc *Client) CreateStack(
 	}

 	var createStackResp apitype.CreateStackResponseByName
-	if err := pc.restCall("POST", getProjectPath(project, "stacks"), nil, &createStackReq, &createStackResp); err != nil {
-		return apitype.Stack{}, err
+	if project.Repository != "" {


More legacy behavior? If yes, comment and/or TODO?

joeduffy · 2018-04-18T15:13:36Z

pkg/backend/cloud/stack.go

-	b         *cloudBackend    // a pointer to the backend this stack belongs to.
+	name      backend.StackReference // the stack's name.
+	cloudURL  string                 // the URL to the cloud containing this stack.
+	orgName   string                 // the organization that owns this stack.


I assume we should eventually rename this to owner?

joeduffy · 2018-04-18T15:15:11Z

pkg/backend/cloud/stack.go

+func (c cloudBackendReference) String() string {
+	curUser, err := c.b.client.DescribeUser()
+	if err != nil {
+		curUser = ""


If err != nil, won't curUser already be ""?

Are you sure we want to just show an empty string here in the face of an error?

I think I'd prefer that the backend reference get populated with a user eagerly, rather than introducing a latent failure later on. Perhaps the client should be fetching the user and caching it at creation time?

Yeah, I think this is a good idea, I'll do that.

joeduffy · 2018-04-18T15:16:05Z

pkg/backend/local/backend.go

-// localBackendURL is fake URL we use to signal we want to use the local backend vs a cloud one.
-const localBackendURL = "local://"
+// localBackendURL is fake URL scheme we use to signal we want to use the local backend vs a cloud one.
+const localBackendURLPrefix = "local://"


Random comment; as I've used the new CLI behavior, I wondered, why don't we just call this local? It's not a valid URL, and having local:// is a little funny because it looks like a protocol start of a URL when it's not.

Note that with these changes, I took your advice and you now can specify a path after the local:// part, and if you don't it defaults as if you had passed $HOME there. From f75b62a's commit message:

For state stored by the local backend, we change the URL scheme from `local://` to `local://<optional-root-path>`. When `<optional-root-path>` is unset, it defaults to `$HOME`. We create our `.pulumi` folder in that directory. This is important because stack names now must be unique within the backend, but we have some tests using local stacks which use fixed stack names, so each integration test really wants its own "view" of the world.

I will keep this as is for now.

Ah, neat, didn't realize that.

joeduffy · 2018-04-18T15:32:12Z

pkg/backend/local/backend.go

-func (b *localBackend) RemoveStack(stackName tokens.QName, force bool) (bool, error) {
-	_, snapshot, _, err := getStack(stackName)
+func (b *localBackend) RemoveStack(stackRef backend.StackReference, force bool) (bool, error) {
+	stackName := stackRef.EngineName()


I think I've only now figured out what EngineName is ... Is this the stack's name, as interpreted by the engine? Not sure why this is required, but I think it probably needs a different name. As it stands, it appears to be the name of the engine, which confused me (since I wasn't aware that our engine can have a name...)

Yeah, bad name. The reason we need it is that when we preform engine operations, we still need to give a tokens.QName for the stack. We also don't want that name to include any owner information information so our URNs are shaped the same was as before.

lukehoban · 2018-04-19T23:29:37Z

CI will fail here until we promote testing to staging, as we now generate stack names larger than 40 characters.

@lukehoban @chrsmith Didn't we promote testing to staging yesterday?

We should have everything in staging now. @ellismg Anything still blocking getting tests green here?

ellismg · 2018-04-20T06:49:24Z

@ellismg Anything still blocking getting tests green here?

Nope, had to merge in master to deal with some changes, but got that sorted out, got a clean CI and merged it.

ellismg added 2 commits April 18, 2018 03:29

ellismg added the impact/changelog label Apr 18, 2018

ellismg requested review from justinvp, joeduffy and chrsmith April 18, 2018 10:54

joeduffy added the in progress label Apr 18, 2018

ellismg mentioned this pull request Apr 18, 2018

Add some randomness to stack names in our integration tests #1166

Closed

ellismg added 5 commits April 18, 2018 04:53

Don't leak a file from asset_test.go

b5129db

Add some randomness to stack names in our integration tests

50982e8

To prepare for a world where stack names must be unique across an owner, add some randomness to the names we use for stacks as part of our integration tests.

Introduce backend.StackReference

c0b2c4f

Long term, a stack name alone will not be sufficent to address a stack. Introduce a new `backend.StackReference` interface that allows each backend to give an opaque stack reference that can be used across operations.

chrsmith approved these changes Apr 18, 2018

View reviewed changes

joeduffy reviewed Apr 18, 2018

View reviewed changes

ellismg force-pushed the ellismg/identity branch from 601dc9e to 8d99119 Compare April 19, 2018 23:08

ellismg added 2 commits April 20, 2018 01:31

Address code review feedback

04e5dfd

Merge remote-tracking branch 'origin/master' into ellismg/identity

cc938a3

ellismg force-pushed the ellismg/identity branch from 8d99119 to cc938a3 Compare April 20, 2018 05:57

Address code review feedback

15e2ad2

ellismg force-pushed the ellismg/identity branch from 397dab0 to 15e2ad2 Compare April 20, 2018 06:34

ellismg merged commit 17faae5 into master Apr 20, 2018

joeduffy removed the in progress label Apr 20, 2018

ellismg mentioned this pull request Apr 20, 2018

Eliminate pulumi init in its current form #830

Closed

ellismg deleted the ellismg/identity branch April 25, 2018 23:54

lindydonna added the impact/breaking Fixing this issue will require a breaking change label Apr 27, 2018

ellismg mentioned this pull request Oct 10, 2018

Change stack identity from <org>/<stackname> to <org>/<project>/<stackname> #2039

Closed

New Identity Model in the CLI #1226

New Identity Model in the CLI #1226

Conversation

ellismg commented Apr 18, 2018

ellismg commented Apr 18, 2018

joeduffy commented Apr 18, 2018

chrsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukehoban commented Apr 19, 2018

ellismg commented Apr 20, 2018