nsqadmin: base path #856

blinklv · 2017-03-02T07:31:11Z

It's difficult for me to access nsqadmin server through nginx (or other reverse proxies), because almost all URLs of HTTP request are based on root path('/'). Most of the time We can't use root path directly, so I add base-path option to change the base path of request URL.

eg:

command
nsqadmin -base-path='/i/am/new/root'

original request url
/api/topics/foo/bar

new request url
/i/am/new/root/api/topics/foo/bar

If you think it's a good idea, you can take it.

(Maybe I don't know other ways to solve this problem. If you know, please tell me.)

mreiferson · 2017-03-05T19:18:50Z

cc @jehiah thoughts?

ploxiln · 2017-03-05T19:31:24Z

With a proxy like nginx, you can translate all URLs to remove a common prefix before passing the request through to nsqadmin. The problem is the URLs in the response body, particularly if they're constructed at view time by javascript - but those can work if they're all relative. Currently, all links I've spot checked are absolute paths (but not including domain). If there are any 301 or 302 redirects in nsqadmin, that would be a trickier problem, but I don't think there are any.

So an alternative config-free "fix" (which would need someone to work on it) is to make all nsqadmin links relative. Just mentioning it as an option :)

mreiferson · 2017-03-05T19:35:41Z

Yea, making the links relative would be a simpler set of changes.

@blinklv would you be interested in taking a pass at that implementation?

jehiah · 2017-03-05T19:42:06Z

@mreiferson I'm 👍 for either approach

blinklv · 2017-03-06T02:44:04Z

@mreiferson Of course, I can create a new branch to try it.

In fact, I have already tried relative link, but some refresh operations (like empty, delete, pause) make it hard to implement (at least for me), the current directory of relative path are changed. ∑(っ °Д °;)っ .So I keep original format and add some prefixes. Maybe I need do some researches about it.

mreiferson · 2017-07-30T18:10:15Z

ping #737

AlmogBaku · 2017-09-26T23:04:43Z

any update on this?

blinklv · 2017-12-05T09:39:33Z

About six months ago, I tried to implement this feature by using relative links instead of root-relative links. But the relationship of these sites are more complicated than what I think, the set of changes aren't simpler, so I stick the original way, it has already satisfied my needs. Recently, I simplify some codes of http.go file to make it more elegant, which means a simpler set of changes. 😉

MarcErdmann · 2017-12-17T22:09:54Z

Will this be merged and released?

mreiferson · 2018-08-02T18:52:16Z

I started to dig into this one, initially trying to refactor it to use relative links, and I came away feeling similar to @blinklv — that's it's actually easier to just root everything to some base path.

So I think instead I'll just get this rebased and do a bit of cleanup.

mreiferson · 2018-08-03T15:28:34Z

I'm gonna test this out a little more, but I've pushed up my revisions.

ploxiln · 2018-08-06T01:06:51Z

Looks reasonable. I think we'll probably want to defer merging until after v1.1.0 final is released?

(and it'll need to be rebased and bindata regenerated)

ploxiln · 2018-08-06T01:10:28Z

nsqadmin/nsqadmin.go

+	}
+	// add trailing slash
+	if p[len(p)-1] != '/' {
+		p = p + "/"


I think the templated urls and such may look a bit better if instead BasePath does not end in / and we write the / after it everywhere. But I understand if you disagree, and it's OK this way too.

Well, except for /. I got this from Backbone's code and was trying to create some parity.

I do agree that aesthetically, the sites where we use basePath would look better with a preceding /. But really it's all just fugly.

We should make a decision on this...

I've noted my preference, and I leave the choice up to you :)

(I think BasePath and base_path could be blank in the default case, and the root path could be written as {{BasePath}}/ and {{base_path}}/)

(I mean, in the config as "/", but in this function turned into "" by removing the trailing (and only) slash)

blinklv · 2018-08-08T15:10:37Z

I update my branch (merge master into it) and hope these changes won't affect your work. @mreiferson

ploxiln · 2018-08-08T16:36:39Z

the rebuild of bindata.go removed over 200 more lines than it added, which is suspicious ...

mreiferson · 2018-08-08T16:39:21Z

It's ok @blinklv, I can handle the rebase and bindata.go update! Thanks :)

blinklv · 2018-08-09T04:02:36Z

I added -debug option when I first rebuilt bindata.go file, so many lines were removed. Now I have replaced it with the release version. @ploxiln 😉

mreiferson · 2018-08-20T22:46:13Z

rebased and rebuilt bindata.go

mreiferson · 2018-09-03T23:07:21Z

made this prettier @ploxiln, PTAL

mreiferson · 2018-09-03T23:10:22Z

nsqadmin/http.go

+	router.Handle("GET", bp("nodes"), http_api.Decorate(s.indexHandler, log))
+	router.Handle("GET", bp("nodes/:node"), http_api.Decorate(s.indexHandler, log))
+	router.Handle("GET", bp("counter"), http_api.Decorate(s.indexHandler, log))
+	router.Handle("GET", bp("lookup"), http_api.Decorate(s.indexHandler, log))


I missed these, they should probably all get the / prefix

mreiferson · 2018-09-03T23:14:16Z

nsqadmin/static/js/app_state.js

+        // if base path is / then don't prefix
+        var bp = this.get('BASE_PATH') == '/' ? '' : this.get('BASE_PATH');
+        // trim trailing /
+        return (bp + p).replace(/\/$/, '');


Need to do a little testing but I think there's a bug here when base path is / and this is passed a path /

such as https://github.com/nsqio/nsq/pull/856/files#diff-6c66351008f5616a7bf2b9cc9859dbbfR27

Actually, that link is to the Go template function, not the JS one, but I think my original point still stands.

mreiferson · 2018-09-03T23:46:43Z

pushed those fixes up...

ploxiln · 2018-09-04T00:52:18Z

nsqadmin/static/js/app_state.js

+        // if base path is / then don't prefix
+        var bp = this.get('BASE_PATH') == '/' ? '' : this.get('BASE_PATH');
+        // remove trailing /, but guarantee at least /
+        return (bp + p).replace(/\/$/, '') || '/';


this removes the trailing slash from p ... did you intend to remove the trailing slash from bp?

There's some unexpected mess and complexity in this approach ... I think it can be simplified to "just remove trailing slash from basepath".

If the base path is "/" then we want ""
If the base path is "/admin/" then we want "/admin"

Then, you can append anything to that, e.g. "/topics", or just "/" to get the root URL path.

I guess this function is pretty much the only problem here, the rest looks good.

In Go land, path.Join() takes care of everything after the leading /.

So I guess this is the only place where you might have to worry about a double-slash between BASE_PATH and p.

This logic seems a bit much for what should still be a trivial function. I think my initial suggestion here was appropriate, just remove trailing slash from BASE_PATH.

... e.g.

return this.get('BASE_PATH').replace(/\/$/, '') + p;

but since this is the only place BASE_PATH is used, you could do the regex just once, in index.html:

var BASE_PATH = {{basePath ""}}.replace(/\/$/, '');

and then here it's just

return this.get('BASE_PATH') + p;

The case this is addressing is, e.g. where BASE_PATH is /base and in one of our handlebars templates we have {{basePath "/"}}. What we want is /base not /base/, but if BASE_PATH is / we want / not //.

Basically, the answer to your original question is that I intended to remove any potential trailing slash from the joined path.

Also, FWIW, I think BASE_PATH (the global var that's passed into AppState) should always represent the normalized value of whatever was passed to nsqadmin --base-path.

If you can come up with an easier way to do this, I'm all ears.

Heh 😅I'm going to leave this as is, thanks for the feedback.

You just have to accept that the root page path is /base/, and all the conditionals go away.

FWIW, if I had gone in this direction, it would have added a little more cruft to the Backbone routing side to handle that special case.

So the only value of p this could be useful for is p == "/" in which case you could instead handle it like if (p == "/") return this.get('BASE_PATH')

Not just / but anything that ends in a /. I realize we don't actually do this anywhere, but it feels like a -1 for special casing?

Why would we choose a path for a new page that ends in /, only for our code to remove that / here?

There still are special cases for / in both variables - but I think a formulation like this is more clear / less tricky:

var bp = this.get('BASE_PATH'); if (bp == '/') return p; if (p == '/') return bp; return bp + p;

fwiw, I think Backbone can handle trailing slash:

Trailing slashes are treated as part of the URL, and (correctly) treated as a unique route when accessed. docs and docs/ will fire different callbacks.

http://backbonejs.org/#Router-routes

I'm a bit more surprised that it can handle the empty string when the leading slash is removed from this.route(bp('/'), 'topics'); but I think that would happen either way. (It's fun to think through this case for the default base-path: that bp() calls this basePath(), BASE_PATH is "/" so prefix with empty string, remove "/", that results in empty string so return "/" instead, finally bp() removes first slash, resulting in empty string.)

Yes, I know that Backbone can support it, I didn't mean to imply otherwise. I just think it's inconsistent with all the other routes that are not currently configured to allow it (although, again, we never link to them without a trailing slash).

This feels like a good baseline so I think we should land this. If you're so inclined, I look forward to your follow up PR where I encourage you to explore this and any other rabbit hole that piques your interest 😉. I am totally willing to land something that demonstrates a simpler overall approach.

yes, let's merge this

ploxiln · 2018-09-04T06:52:06Z

nsqadmin/nsqadmin.go

@@ -130,11 +131,24 @@ func New(opts *Options) *NSQAdmin {
 		}
 	}

+	opts.BasePath = normalizeBasePath(path.Clean(opts.BasePath))


I didn't initially realize that path.Clean() removed the trailing slash. It's part of the normalization logic so it should probably go in normalizeBasePath().

Yep, will move

nanom1t · 2018-11-19T10:42:55Z

How to set base path in nsqadmin config?

ploxiln · 2018-11-20T01:13:27Z

You can use the command-line flag --base-path=/whatever or in the config file it would be base_path = "/whatever"

This was merged after version 1.1.0 was tagged and released, so you will have to build nsqadmin from the master branch in order to get this feature. see https://nsq.io/deployment/installing.html#compiling

mreiferson changed the title ~~Nsqadmin base path~~ nsqadmin: base path Mar 3, 2017

mreiferson added feature request labels Mar 3, 2017

jhowtan mentioned this pull request Sep 20, 2017

nsqadmin: root_url #737

Closed

blinklv force-pushed the nsqadmin-base-path branch from fcfd567 to 9cda994 Compare December 4, 2017 07:48

mreiferson force-pushed the nsqadmin-base-path branch 2 times, most recently from 388d90a to 641b781 Compare August 3, 2018 15:27

ploxiln reviewed Aug 6, 2018

View reviewed changes

blinklv force-pushed the nsqadmin-base-path branch 2 times, most recently from 7f09a9e to 8700ef9 Compare August 8, 2018 14:58

blinklv force-pushed the nsqadmin-base-path branch from 8700ef9 to 018e587 Compare August 9, 2018 03:45

mreiferson force-pushed the nsqadmin-base-path branch from 018e587 to 7c71df7 Compare August 20, 2018 22:45

hxsf mentioned this pull request Aug 24, 2018

nsqadmin: add web-root option #1077

Closed

mreiferson force-pushed the nsqadmin-base-path branch from 7c71df7 to 7b6d495 Compare September 3, 2018 23:05

mreiferson reviewed Sep 3, 2018

View reviewed changes

ploxiln reviewed Sep 4, 2018

View reviewed changes

ploxiln approved these changes Sep 4, 2018

View reviewed changes

blinklv and others added 2 commits September 4, 2018 08:19

nsqadmin: add --base-path option

d69e752

nsqadmin: bindata

8a5e291

mreiferson force-pushed the nsqadmin-base-path branch from 57aa405 to 8a5e291 Compare September 4, 2018 15:19

ploxiln merged commit 565c76d into nsqio:master Sep 4, 2018

mreiferson mentioned this pull request Sep 4, 2018

nsqadmin: eslint fixes #1080

Merged

mreiferson mentioned this pull request Nov 19, 2018

nsqadmin: static files #1106

Closed

nsqadmin: base path #856

nsqadmin: base path #856

Conversation

blinklv commented Mar 2, 2017 • edited Loading

mreiferson commented Mar 5, 2017

ploxiln commented Mar 5, 2017

mreiferson commented Mar 5, 2017

jehiah commented Mar 5, 2017

blinklv commented Mar 6, 2017 • edited Loading

mreiferson commented Jul 30, 2017

AlmogBaku commented Sep 26, 2017

blinklv commented Dec 5, 2017 • edited Loading

MarcErdmann commented Dec 17, 2017

mreiferson commented Aug 2, 2018

mreiferson commented Aug 3, 2018

ploxiln commented Aug 6, 2018

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

blinklv commented Aug 8, 2018

ploxiln commented Aug 8, 2018

mreiferson commented Aug 8, 2018

blinklv commented Aug 9, 2018 • edited Loading

mreiferson commented Aug 20, 2018

mreiferson commented Sep 3, 2018

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

mreiferson commented Sep 3, 2018

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

mreiferson Sep 4, 2018 • edited Loading

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

nanom1t commented Nov 19, 2018

ploxiln commented Nov 20, 2018

blinklv commented Mar 2, 2017 •

edited

Loading

blinklv commented Mar 6, 2017 •

edited

Loading

blinklv commented Dec 5, 2017 •

edited

Loading

blinklv commented Aug 9, 2018 •

edited

Loading

mreiferson Sep 4, 2018 •

edited

Loading