/juju

cmd/juju/controller: allow registering public controller #6382

Merged
merged 1 commit into from Oct 6, 2016

Conversation

Reviewers

@reedobrien reedobrien

@axw axw

@mhilton mhilton

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@rogpeppe @reedobrien @jujubot @axw @mhilton
@rogpeppe
Owner

rogpeppe commented Oct 5, 2016 •
Edited 1 time

This allows a controller with an officially signed certificate to be
registered with:

juju register <hostname>

Because it's got an officially signed certificate, we can trust it to
obtain the controller UUID, and external user authentication lets us
know what user has been chosen without needing to select one explicitly.

This caused a significant reshuffle of the register command. In
particular, we now ask for the password before asking for
the controller name because we can't get the controller UUID
without the password. That moves us in the direction of fixing
https://bugs.launchpad.net/juju/+bug/1614010 because we no longer prompt
for the controller name if we know that the controller cannot be stored.

We also update the controller-name input code so that it loops until a
correct controller name is supplied rather than immediately returning
an error.

The tests needed significant change because all the tests that previously
assumed that the controller name was prompted before any interaction
with the server now need a mock server.

To make this a bit more straightforward, we add a the Prompter types
in cmd/testing which means that the interaction requirements can be
specified more declaratively. This has the side effect of testing that
the read/write interleaving is exactly as expected.

QA

Bootstrap a controller with an officially signed certificate and externally configured users:

juju bootstrap --debug --config 'autocert-dns-name=<your domain name>' --config 'api-port=443' --config identity-url=https://api.jujucharms.com/identity ec2 aws

Grant everyone permission to access the controller and create models:

juju grant everyone@external addmodel

Then configure the DNS record for your domain name to point to the IP address of the newly
bootstrapped controller.

Then, on a fresh juju client instance (or with the new controller entry removed from controller.yaml and accounts.yaml) run:

juju register <your domain name>

That should succeed and log in as your USSO username@external. You should then be able to create models and list models etc.
@reedobrien

reedobrien suggested changes Oct 5, 2016
View changes

Mostly LGTM. A few trivial nits.
One big issue: the register test in featuretests fails.

I like the testing.Prompter too. Nice work.

I'll see if I can figure out how to do the qa steps.

cmd/juju/controller/register.go
+To complete the user registration process, you should have been provided
+with a base64-encoded blob of data (the output of 'juju add-user') which
+can be copied and pasted as the <string> argument to 'register'.
+You will be prompted for a password, which, once set, causes the
@reedobrien

reedobrien Oct 5, 2016

Contributor

nit: These paragraphs look like they are wrapped inconsistenly. Maybe just how they look in the review bits.

@rogpeppe

rogpeppe Oct 6, 2016

Owner

done

cmd/juju/controller/register.go
registration string to be voided. In order to start using Juju the user can now
either add a model or wait for a model to be shared with them. Some machine
providers will require the user to be in possession of certain credentials in
order to add a model.
+When adding a controller at a public address, authentication via some
+external third party (for example Ubuntu USSO) will be required, usually
@reedobrien

reedobrien Oct 5, 2016

Contributor

s/Ubuntu USSO/ Ubuntu SSO ?

@rogpeppe

rogpeppe Oct 6, 2016

Owner

done

cmd/juju/controller/register.go
@@ -87,7 +99,7 @@ See also:
func (c *registerCommand) Info() *cmd.Info {
return &cmd.Info{
Name: "register",
- Args: "<string>",
+ Args: "<string>|<controller host name>",
@reedobrien

reedobrien Oct 5, 2016

Contributor

If I'm reading this right it means a base64 encoded registration string or controller hostname. If I didn't know anything I would think any string would suffice. Maybe make this <registration string>|<cotroller host name> or something.

@rogpeppe

rogpeppe Oct 6, 2016

Owner

Good idea. Done.

cmd/juju/controller/register.go
+ return jujuclient.ControllerDetails{
+ APIEndpoints: []string{host},
+ ControllerUUID: conn.ControllerTag().Id(),
+ // TODO(rogpeppe) it might be good to update the non-official CA cert here too
@reedobrien

reedobrien Oct 5, 2016

Contributor

I am being told in reviews to use the format:
// TODO(id) yyyy-mm-dd [#bugnumber] <comment>

So I suppose it should be used here, too.

@rogpeppe

rogpeppe Oct 6, 2016

Owner

I've removed the TODO. I'm not sure it's the right thing to do anyway and - it doesn't justify a bug report. (BTW only 12% of TODOs in the source code fit this format. I'm not entirely convinced it's worth having an issue for every TODO).

cmd/juju/controller/register.go
- return errors.Annotate(err, "storing model details")
+ for name, ctl := range all {
+ if ctl.ControllerUUID == controllerDetails.ControllerUUID {
+ // TODO(rogpeppe) Succeed but override the account details in this case?
@reedobrien

reedobrien Oct 5, 2016 •
Edited 1 time

Contributor

TODO format... Also, have you ansered your question? I don't think I quite understand it in context.

@rogpeppe

rogpeppe Oct 6, 2016

Owner

I've added a bug number.

cmd/juju/controller/register.go
@@ -233,74 +348,81 @@ func (c *registerCommand) maybeSetCurrentModel(ctx *cmd.Context, store jujuclien
if err != nil {
return errors.Trace(err)
}
- fmt.Fprintf(ctx.Stderr, "\nCurrent model set to %q.\n\n", modelName)
- } else {
@reedobrien

reedobrien Oct 5, 2016 •
Edited 1 time

Contributor

Thanks :)

cmd/juju/controller/register.go
- fmt.Fprintf(ctx.Stderr, "\nCurrent model set to %q.\n\n", modelName)
- } else {
- fmt.Fprintf(ctx.Stderr, `
+ fmt.Fprintf(ctx.Stderr, "\nCurrent model set to %s.\n", modelName)
@reedobrien

reedobrien Oct 5, 2016

Contributor

Is there a particular reason to stop quoting the model name in output?

@rogpeppe

rogpeppe Oct 6, 2016

Owner

Well, we know the model name is well formed (because it's been verified and it's a very restricted syntax) so there's no danger of ambiguity, and it looks slightly cleaner unquoted IMHO.

@axw

axw Oct 6, 2016

Member

Please revert, so we are consistent with use of %q for controller names in sentences.

cmd/juju/controller/register.go
+ // Looks like a host name - no URL-encoded base64 string should
+ // contain a dot and every public controller name should.
+ // Allow localhost for development purposes.
+ params.publicHost = c.Arg
@reedobrien

reedobrien Oct 5, 2016

Contributor

can't you just return here?

@axw

axw Oct 6, 2016

Member

+1

@rogpeppe

rogpeppe Oct 6, 2016

Owner

Yup, good catch - it's a hangover from when we fetched the controller name in both cases.

cmd/juju/controller/register.go
- return nil, errors.Trace(err)
+ if params.publicHost != "" {
+ // No need for password shenanigans if we're using a public controller.
+ return &params, nil
@reedobrien

reedobrien Oct 5, 2016 •
Edited 1 time

Contributor

Can't this just return up where params.publicHost is set ~line 400?

@rogpeppe

rogpeppe Oct 6, 2016

Owner

Yup, done

@reedobrien
Contributor

reedobrien commented Oct 5, 2016

Unable to QA. I was unable to resolve the endpoints in http://paste.ubuntu.com/23080517/ to bootstrap. Updating them to be https://ec2..amazonaws.com/... resovled them but then I failed for other reasons (not authorized to perform action on one machine and failed to bootstrap model: cannot start bootstrap instance: no "xenial" images in us-west-1 with arches [amd64] on another.
@reedobrien
Contributor

reedobrien commented Oct 5, 2016

Tried on a 3rd computer and I could bootstrap. I guess that my local DNS here is hiding the upstream changes. I bootstrapped with a domain name I had control of, then had to add DNS pointing at the external IP of the bootstrapped controller.

Then I got a link to a paste in my browser which took me to a page, "login successful as user ".

So IIUC, QA checks out.
@mhilton

mhilton approved these changes Oct 6, 2016
View changes

👍

cmd/juju/controller/register.go
Examples:
juju register MFATA3JvZDAnExMxMDQuMTU0LjQyLjQ0OjE3MDcwExAxMC4xMjguMC4yOjE3MDcwBCBEFCaXerhNImkKKabuX5ULWf2Bp4AzPNJEbXVWgraLrAA=
+ juju register public-controller.somewhere.com
@mhilton

mhilton Oct 6, 2016

Member

isn't example.com supposed to be the standard for this sort of thing?

@rogpeppe

rogpeppe Oct 6, 2016

Owner

done

@axw

axw approved these changes Oct 6, 2016
View changes

LGTM with a bunch of little things. Please also verify that add-user/register still works.

cmd/juju/controller/register.go
@@ -55,27 +56,38 @@ type registerCommand struct {
apiOpen api.OpenFunc
listModelsFunc func(_ jujuclient.ClientStore, controller, user string) ([]base.UserModel, error)
store jujuclient.ClientStore
- EncodedData string
+ Arg string
+ ControllerName string
@axw

axw Oct 6, 2016

Member

ControllerName does not appear to be used. Delete?

@rogpeppe

rogpeppe Oct 6, 2016

Owner

done

cmd/juju/controller/register.go
-referred to in Usage.
-
-The user will be prompted for a password, which, once set, causes the
+The register command adds a details of a controller to the local system.
@axw

axw Oct 6, 2016 •
Edited 1 time

Member

s/adds a/adds the/ ?

@rogpeppe

rogpeppe Oct 6, 2016

Owner

done

cmd/juju/controller/register.go
+ return c.maybeSetCurrentModel(ctx, store, controllerName, accountDetails.User, models)
+}
+
+func friendlyUserName(user string) string {
@axw

axw Oct 6, 2016 •
Edited 1 time

Member

Ian's got a branch in the works that gets rid of @Local, so you'll be able to drop this.

@rogpeppe

rogpeppe Oct 6, 2016

Owner

Yay!

cmd/juju/controller/register.go
+ errRet := func(err error) (jujuclient.ControllerDetails, jujuclient.AccountDetails, error) {
+ return jujuclient.ControllerDetails{}, jujuclient.AccountDetails{}, err
+ }
+ if !strings.Contains(host, ":") {
@axw

axw Oct 6, 2016

Member

I think it would be a bit clearer if we assigned host to another var, say "apiAddr".

apiAddr := host
if !strings.Contains(apiAddr, ":") {
    apiAddr += ":443"
}
...
@rogpeppe

rogpeppe Oct 6, 2016

Owner

shrug done

cmd/juju/controller/register.go
- return errors.Annotate(err, "storing model details")
+ for name, ctl := range all {
+ if ctl.ControllerUUID == controllerDetails.ControllerUUID {
+ // TODO(rogpeppe) Succeed but override the account details in this case?
@axw

axw Oct 6, 2016

Member

I think that would be sensible, if a little unusual. Can you please file a bug for that and reference?

@rogpeppe

rogpeppe Oct 6, 2016

Owner

Done (bug was already filed)

cmd/juju/controller/register.go
- fmt.Fprintf(ctx.Stderr, "\nCurrent model set to %q.\n\n", modelName)
- } else {
- fmt.Fprintf(ctx.Stderr, `
+ fmt.Fprintf(ctx.Stderr, "\nCurrent model set to %s.\n", modelName)
@reedobrien

reedobrien Oct 5, 2016

Contributor

Is there a particular reason to stop quoting the model name in output?

@rogpeppe

rogpeppe Oct 6, 2016

Owner

Well, we know the model name is well formed (because it's been verified and it's a very restricted syntax) so there's no danger of ambiguity, and it looks slightly cleaner unquoted IMHO.

@axw

axw Oct 6, 2016

Member

Please revert, so we are consistent with use of %q for controller names in sentences.

cmd/juju/controller/register.go
+ // Looks like a host name - no URL-encoded base64 string should
+ // contain a dot and every public controller name should.
+ // Allow localhost for development purposes.
+ params.publicHost = c.Arg
@reedobrien

reedobrien Oct 5, 2016

Contributor

can't you just return here?

@axw

axw Oct 6, 2016

Member

+1

@rogpeppe

rogpeppe Oct 6, 2016

Owner

Yup, good catch - it's a hangover from when we fetched the controller name in both cases.

cmd/juju/controller/register.go
-const errControllerConflicts = `WARNING: You already have a controller registered with the name %q. Please choose a different name for the new controller.
-
-`
-
func (c *registerCommand) promptControllerName(store jujuclient.ClientStore, suggestedName string, stderr io.Writer, stdin io.Reader) (string, error) {
_, err := store.ControllerByName(suggestedName)
@axw

axw Oct 6, 2016

Member

Please skip this if suggestedName == "". It would previously always be non-empty, but that's not the case now.

@rogpeppe

rogpeppe Oct 6, 2016

Owner

done

cmd/juju/controller/register.go
+ }
+ name = suggestedName
+ }
+ _, err = store.ControllerByName(name)
@axw

axw Oct 6, 2016 •
Edited 1 time

Member

Skip this if name == suggestedName?

@rogpeppe

rogpeppe Oct 6, 2016

Owner

I think I prefer to keep the logic simpler - it does no harm to check in all cases.

cmd/testing/prompt.go
@@ -0,0 +1,220 @@
+package testing
@axw

axw Oct 6, 2016

Member

copyright

@rogpeppe

rogpeppe Oct 6, 2016

Owner

done

cmd/testing/prompt.go
+ gc "gopkg.in/check.v1"
+)
+
+var logger = loggo.GetLogger("cmd.testing")
@axw

axw Oct 6, 2016

Member

doesn't matter much, but should start with "juju." (juju.cmd.testing)

@rogpeppe

rogpeppe Oct 6, 2016

Owner

done

cmd/testing/prompt.go
+
+var logger = loggo.GetLogger("cmd.testing")
+
+// NewSeqPrompter returns a prompter that expected prompt text matching
@axw

axw Oct 6, 2016

Member

I'm having trouble parsing this. Maybe reword?

@rogpeppe

rogpeppe Oct 6, 2016

Owner

deleted

cmd/testing/prompt.go
+// all the items in expectPrompts in sequence, and will reply with
+// the associated text in replies when requested.
+//
+// The expectPrompts slice may be one longer than replies,
@axw

axw Oct 6, 2016

Member

this is out of date, there's no expectPrompts

@rogpeppe

rogpeppe Oct 6, 2016

Owner

This whole comment is a relic. Deleted.

cmd/testing/prompt.go
+// marker on that line, the checker will cause io.EOF to be returned for
+// that prompt.
+//
+// The returned interactionChecker wraps a Prompter and checks that each
@axw

axw Oct 6, 2016

Member

s/interactionChecker/SeqPrompter/? and below

@rogpeppe

rogpeppe Oct 6, 2016

Owner

done

cmd/testing/prompt.go
+// The returned interactionChecker wraps a Prompter and checks that each
+// read and write corresponds to the expected action in the sequence.
+//
+// After all interaction is done, CheckDone or AssertDone should be closed to
@axw

axw Oct 6, 2016

Member

s/closed/called/

@rogpeppe

rogpeppe Oct 6, 2016

Owner

done

@rogpeppe
cmd/juju/controller: allow registering public controller 
This allows a controller with an officially signed certificate to be
registered with:

	juju register <hostname>

Because it's got an officially signed certificate, we can trust it to
obtain the controller UUID, and external user authentication lets us
know what user has been chosen without needing to select one explicitly.

This caused a significant reshuffle of the register command. In
particular, we now ask for the password before asking for
the controller name because we can't get the controller UUID
without the password.  That moves us in the direction of fixing
https://bugs.launchpad.net/juju/+bug/1614010 because we no longer prompt
for the controller name if we know that the controller cannot be stored.

We also update the controller-name input code so that it loops until a
correct controller name is supplied rather than immediately returning
an error.

The tests needed significant change because all the tests that previously
assumed that the controller name was prompted before any interaction
with the server now need a mock server.

To make this a bit more straightforward,  we add a the Prompter types
in cmd/testing which means that the interaction requirements can be
specified more declaratively.  This has the side effect of testing that
the read/write interleaving is exactly as expected.
a92148d
@rogpeppe
Owner

rogpeppe commented Oct 6, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Oct 6, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 5d5f2d2 into juju:master Oct 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment