Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd/modelcmd: per-controller cookie jars #7294
Conversation
| @@ -19,6 +19,7 @@ func NewDumpCommand() cmd.Command { | ||
| } | ||
| type dumpCommand struct { | ||
| + // TODO shouldn't this be ModelCommandBase ? |
| @@ -15,6 +15,7 @@ import ( | ||
| "github.com/juju/juju/cmd/juju/romulus/budget" | ||
| "github.com/juju/juju/jujuclient" | ||
| coretesting "github.com/juju/juju/testing" | ||
| + "github.com/juju/persistent-cookiejar" |
| +} | ||
| + | ||
| +func (c *CommandBase) setRunStarted() { | ||
| + c.runStarted = true | ||
| } | ||
| // closeContext closes the command's API context |
| return c.store | ||
| } | ||
| -// SetControllerName implements ControllerCommand.SetControllerName. | ||
| -func (c *ControllerCommandBase) SetControllerName(controllerName string, allowDefault bool) error { | ||
| +func (c *ControllerCommandBase) initController() error { |
axw
May 1, 2017
Member
This initController/initModel business feels pretty gross, but I guess it's all we can do for now.
I think we eventually need to move away from the inheritance pattern we currently have around commands, and towards composition so that the ModelCommandBase (or whatever it's called now) Run method comes first, and delegates to the command-specific code with a separate signature, like:
package modelcmd
type ModelCommand interface {
Init(...)
SetFlags(...)
Run(modelcmd.ModelContext) error
}
type ControllerContext struct {
Store jujuclient.ClientStore
APIDialer APIDialer
ControllerName string
}
type ModelContext struct {
ControllerContext
ModelName string
}
type APIDialer interface {
DialAPI(controllerName, modelName string) (api.Connection, error)
}(implementations of ModelCommand wouldn't embed anything)
rogpeppe
May 2, 2017
Owner
I agree that it would be good to move towards a more composition-based model. FWIW I started by making it avoid calling Run if the context couldn't be initialised, but then did this lazy implementation when I realised just how many tests rely on being able to call Run without any models or controllers in the store.
| @@ -84,17 +86,25 @@ type ModelCommand interface { | ||
| // unqualified, in which case it will be assumed to be within the | ||
| // current controller. | ||
| // | ||
| + // Passing an empty model name will choose the default | ||
| + // mode, or return an error if there isn't one. |
| @@ -703,3 +707,40 @@ func (s *store) BootstrapConfigForController(controllerName string) (*BootstrapC | ||
| } | ||
| return &cfg, nil | ||
| } | ||
| + | ||
| +// CookieJar returns the cookie jar associated with the given controller. | ||
| +func (s *store) CookieJar(controllerName string) (CookieJar, error) { |
axw
May 1, 2017
Member
The RemoveController method of ClientStore implementations is expected to clean up any controller-related resources. Is there any reason to keep the cookie files around after a controller has been destroyed? If not, please remove the cookie jar file towards the bottom of that method. If it should be kept, please add a comment explaining why.
| +// JujuCookiePath is the location where cookies associated | ||
| +// with the given controller are expected to be found. | ||
| +func JujuCookiePath(controllerName string) string { | ||
| + return osenv.JujuXDGDataHomePath("cookies-" + controllerName + ".json") |
axw
May 1, 2017
Member
can we please make this a sub-directory, to avoid polluting the top level directory with too many dynamically named entries? i.e. ~/.local/share/juju/cookies/controller.json
| @@ -81,6 +82,7 @@ func (s *CmdSuite) TestDeployCommandInit(c *gc.C) { | ||
| c.Assert(err, gc.ErrorMatches, t.expectError) | ||
| continue | ||
| } | ||
| + c.Assert(err, jc.ErrorIsNil) |
| - c.Assert(err, gc.ErrorMatches, "no application name specified") | ||
| - | ||
| - // missing application name | ||
| - err = cmdtesting.InitCommand(application.NewConfigCommandForTest(s.fake), []string{"name=foo"}) |
| + "testconfig.yaml", | ||
| + }, s.dir) | ||
| + c.Assert(err, gc.ErrorMatches, `(.|\n)*All operations that change model have been disabled(.|\n)*`) | ||
| + c.Check(c.GetTestLog(), gc.Matches, "(.|\n)*TestBlockSetConfig(.|\n)*") |
rogpeppe
May 2, 2017
Owner
I can never remember the meaning of all those flags! I think the explicit form is probably better because it's more obvious what's going on.
|
I didn't look in detail - does this handle upgrading existing controllers where the cookies are stored in $HOME/.go-cookies (in fact, it may not be home dir, there's an env var that can place the cookie jar anywhere the user wants). |
No it doesn't, but I'm not entirely sure that that's needed - the macaroons can always expire anyway. This change could just be considered an early expiry. |
rogpeppe
dismissed
axw’s
stale review
May 2, 2017
All points addressed.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
jujubot
merged commit 1c6c069
into
juju:develop
May 2, 2017
1 check passed
rogpeppe
referenced this pull request
May 25, 2017
Merged
cmd/modelcmd: respect JUJU_MODEL env var #7394
added a commit
to rogpeppe/juju
that referenced
this pull request
May 26, 2017
added a commit
that referenced
this pull request
May 26, 2017
rogpeppe commentedApr 28, 2017
This makes all macaroons per-controller, so you can potentially
be logged into as a separate external user to every controller.
The cookies are stored in $JUJU_DATA/cookies-$controllername
in the same format already used for the $HOME/.go-cookies.
This disturbed quite a few invariants that tests were used to.
Here's a summary of some of the changes:
add cookiejars to jujuclient.ClientStore so that tests weren't
inadvertently creating cookie files.
there is now no access to ClientStore in commands before Run.
ControllerName and ModelName now return errors
because they resolve the current model or controller
name when called if needed. If a command tries to
use any of these methods during Init (or any method that relies
on them), it will panic.
since we now have modelcmd.InnerCommand because the modelcmd
Wrap functions now return more specific interfaces, we can simplify tests
in quite a few places. In particular, many functions that returned a pair
of objects (the wrapped command and the inner command) now
just return the wrapped command, leaving it to specific test logic
to obtain the inner command if needed (for example to test the values
of instance variables after Init)
the register command now needs to prompt for a controller
name before connecting otherwise it can't create the
cookie jar because it doesn't know where to store the cookies.
some tests (cmd/juju/subnet, cmd/juju/space) were
reusing the same command instance several times.