/juju

Add support for new gui urls #6992

Merged
merged 1 commit into from Feb 16, 2017

Conversation

Reviewers

@axw axw

@frankban frankban

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@wallyworld @jujubot @axw @frankban
@wallyworld
Owner

wallyworld commented Feb 16, 2017

Description of change

The GUI us moving to a new URL scheme
https://:17070/gui/u/user/model

We need to support that plus there's a login redirect bug that is fixed.
The existing modelUUID URLs are still used to serve static content. It's just the user navigable URLs that are changed.

QA steps

bootstrap juju 2.1
$ juju gui
Use the url and credentials printed to log into the GUI.
Logout
Login again and ensure that works

boostrap juju 2.0
$ juju gui
Check that the UUID baded URL is printed and works.

Bug reference

juju/juju-gui#2430

@wallyworld wallyworld referenced this pull request in juju/juju-gui Feb 16, 2017

Closed

Deep linking to models doesn't work if you have to log in with GUI in Juju #2430

apiserver/gui.go
@@ -354,7 +364,7 @@ func (h *guiHandler) serveConfig(w http.ResponseWriter, req *http.Request) {
w.Header().Set("Content-Type", jsMimeType)
tmpl := filepath.Join(h.rootDir, "templates", "config.js.go")
if err := renderGUITemplate(w, tmpl, map[string]interface{}{
- "base": h.baseURLPath,
+ "base": "/gui/",
@axw

axw Feb 16, 2017

Member

can you please make this a const, and use that in the guiEndpoints calls in apiserver.go?

@wallyworld

wallyworld Feb 16, 2017

Owner

done

apiserver/gui.go
@@ -354,7 +364,7 @@ func (h *guiHandler) serveConfig(w http.ResponseWriter, req *http.Request) {
w.Header().Set("Content-Type", jsMimeType)
tmpl := filepath.Join(h.rootDir, "templates", "config.js.go")
if err := renderGUITemplate(w, tmpl, map[string]interface{}{
- "base": h.baseURLPath,
+ "base": "/gui/",
@axw

axw Feb 16, 2017

Member

This change is the only bit I'm unsure about. What changed that made this necessary?

@wallyworld

wallyworld Feb 16, 2017

Owner

The base path used to contain the model uuid. The GUI appends the new model path to this and we end up with a URL where the last bit is ignored. By making the base path just be "/gui/", the GUI then appends the new model path bit and we end up with a properly formed URL that Juju recognises.

@axw

axw approved these changes Feb 16, 2017
View changes

LGTM, but I think it'd be a good idea to get someone from the GUI team to QA this as well.

@frankban

frankban approved these changes Feb 16, 2017
View changes

Hey Ian,
thanks a lot for this! Looks good with some minor changes and a slightly more important one about the base URL provided when rendering the config.js file.
Also:

  • it seems there is an extraneous drive-by fix: is this intended?
  • can we also keep a DEPRECATED --show-credentials option for "juju gui", the same as we did for --no-browser?

QA is OK, this looks really good, and the old-style URLs should just work with the little fix I described.
The only problems are:

  • if you change the password it's no longer displayed by "juju gui". IIRC this is a bug affecting other commands as well. While waiting for a fix, can we fallback more gracefully in those cases? Like by printing "password: " or somthing if password == ""?
apiserver/gui.go
@@ -354,7 +365,7 @@ func (h *guiHandler) serveConfig(w http.ResponseWriter, req *http.Request) {
w.Header().Set("Content-Type", jsMimeType)
tmpl := filepath.Join(h.rootDir, "templates", "config.js.go")
if err := renderGUITemplate(w, tmpl, map[string]interface{}{
- "base": h.baseURLPath,
+ "base": guiURLPathPrefix,
@frankban

frankban Feb 16, 2017

Member

So I think this is the reason why you are seeing that weird "https://IP:17070/gui/u/admin/foobar/1234-567-887-5554" redirection while trying this branch on the old style URL. Having the base path for the GUI as /gui/ is good enough for the /gui/u/:user/:model/ paths, but not for /gui/:uuid/ based ones, in which case we need to keep the whole /gui/:uuid/ as the base, so that the GUI will just start building on those (like /gui/1234-567-887-5554/u/admin/default/). So, based on the request, we have to dynamically switch the base from just guiURLPathPrefix to guiURLPathPrefix + h.uuid + "/" (the former for the new nice URLs, the latter for old ugly ones).

@frankban

frankban Feb 16, 2017 •
Edited 1 time

Member

Actually, I think we could support also the newer Juju/older GUI scenario here. What about this logic?
In this example, the old-style request is /gui/uuid-1234-5678/, the new-style one is /gui/u/admin/default.

if request is old-style:
  base = /gui/uuid-1234-5678/
else if gui version < 2.3.0:
  base = /gui/u/admin/default
else:
  base = /gui/
@wallyworld

wallyworld Feb 16, 2017

Owner

done

apiserver/httpcontext.go
@@ -30,13 +30,42 @@ type httpContext struct {
srv *Server
}
+func (ctxt *httpContext) modelUUIDFromRequest(r *http.Request) (string, error) {
@frankban

frankban Feb 16, 2017

Member

Add a docstring here?

@wallyworld

wallyworld Feb 16, 2017

Owner

done

cmd/juju/gui/gui.go
@@ -128,16 +138,19 @@ func (c *guiCommand) Run(ctx *cmd.Context) error {
}
// checkAvailable ensures the Juju GUI is available on the controller at the
@frankban

frankban Feb 16, 2017

Member

s/the controller at the/the controller at/

@wallyworld

wallyworld Feb 16, 2017

Owner

done

cmd/juju/gui/gui.go
@@ -151,7 +164,7 @@ func (c *guiCommand) openBrowser(ctx *cmd.Context, rawURL string, vers *version.
if vers != nil {
versInfo = fmt.Sprintf("%v ", vers)
}
- ctx.Infof("GUI %sis enabled at:\n %s", versInfo, u.String())
+ ctx.Infof("GUI %sfor model %s is enabled at:\n %s", versInfo, c.ModelName(), u.String())
@frankban

frankban Feb 16, 2017

Member

Maybe "for model %q"? [shrug]

@wallyworld

wallyworld Feb 16, 2017

Owner

done

cmd/juju/gui/gui_test.go
@@ -65,7 +65,7 @@ func (s *guiSuite) patchBrowser(f func(*url.URL) error) {
@frankban

frankban Feb 16, 2017

Member

Should we add a test for the checkAvailable fallback behavior here?

@wallyworld

wallyworld Feb 16, 2017

Owner

done

mongo/collections.go
@@ -34,6 +34,8 @@ type Collection interface {
Find(query interface{}) Query
FindId(id interface{}) Query
+ Pipe(pipeline interface{}) *mgo.Pipe
@frankban

frankban Feb 16, 2017

Member

I this a drive by? I am not sure how this change is related to this branch.

@wallyworld

wallyworld Feb 16, 2017

Owner

oops. contamination from another branch

mongo/collections.go
@@ -109,6 +111,11 @@ func (cw collectionWrapper) FindId(id interface{}) Query {
return queryWrapper{cw.Collection.FindId(id)}
}
+// FindId is part of the Collection interface.
@frankban

frankban Feb 16, 2017

Member

This is actually called Pipe, not FindId.

@wallyworld

wallyworld Feb 16, 2017

Owner

damn, this was not meant to be committed

@wallyworld
Owner

wallyworld commented Feb 16, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Feb 16, 2017

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

@jujubot jujubot merged commit f9375f5 into juju:2.1 Feb 16, 2017
1 check passed

1 check passed

github-check-merge-juju Built PR, ran unit tests, and tested LXD deploy. Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details

@wallyworld wallyworld referenced this pull request Feb 16, 2017

Merged

Account for modeluuid in URL for old GUI versions #6998

jujubot added a commit that referenced this pull request Feb 16, 2017

@jujubot
Merge pull request #6998 from wallyworld/oldgui-newcontroller 
Account for modeluuid in URL for old GUI versions

## Description of change

Followup to PR #6992
Fixes corner case of old GUI, new controller.

## QA steps

bootstrap
juju upgrade-gui 2.2.7
log into gui and ensure it works
fac4f94
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment