[ADDED] base path for monitoring endpoints #1392

guilherme-santos · 2020-05-13T13:19:29Z

Link to issue, e.g. Resolves #NNN
Documentation added (if applicable)
Tests added
Branch rebased on top of current master (git pull --rebase origin master)
Changes squashed to a single commit (described here)
Build is green in Travis CI
You have certified that the contribution is your original work and that you license the work to the project under the Apache 2 license

Resolves #

Changes proposed in this pull request:

Add flag http_base_path to be able to access nats monitoring endpoint behind a proxy (e.g. nginx)

/cc @nats-io/core

derekcollison

In general I think this looks good. Couple of questions on locking. Will let @kozlovic or @matthiashanel take a look as well and sign off.

Thanks!

derekcollison · 2020-05-13T14:47:06Z

server/server.go

@@ -1566,6 +1569,10 @@ const (
 	StackszPath  = "/stacksz"
 )

+func (s *Server) basePath(p string) string {
+	return path.Join(s.httpBasePath, p)


Does this need the server lock?

Same comment than above.

derekcollison · 2020-05-13T14:47:49Z

server/monitor.go

@@ -1081,7 +1082,7 @@ func myUptime(d time.Duration) string {
 // HandleRoot will show basic info and links to others handlers.
 func (s *Server) HandleRoot(w http.ResponseWriter, r *http.Request) {
 	// This feels dumb to me, but is required: https://code.google.com/p/go/issues/detail?id=4799
-	if r.URL.Path != "/" {
+	if r.URL.Path != s.httpBasePath {


May have to protect here with a lock.

I don't think we need because this is immutable. It is set when the server object is created and since it is not added to things that can be reloaded, then it should be fine. But we could protect for the future indeed.

kozlovic

Thank you, very thorough PR!
I am wondering if base_path really need to be added to varz output, and if we need the option in the command line. If we do, you have some changes to do.
As for the locking, I think it is fine right now, but we could certainly use the protection for the future.

server/monitor.go

kozlovic · 2020-05-13T15:09:56Z

server/monitor.go

@@ -1081,7 +1082,7 @@ func myUptime(d time.Duration) string {
 // HandleRoot will show basic info and links to others handlers.
 func (s *Server) HandleRoot(w http.ResponseWriter, r *http.Request) {
 	// This feels dumb to me, but is required: https://code.google.com/p/go/issues/detail?id=4799
-	if r.URL.Path != "/" {
+	if r.URL.Path != s.httpBasePath {


I don't think we need because this is immutable. It is set when the server object is created and since it is not added to things that can be reloaded, then it should be fine. But we could protect for the future indeed.

server/opts.go

kozlovic · 2020-05-13T15:13:03Z

server/server.go

@@ -1566,6 +1569,10 @@ const (
 	StackszPath  = "/stacksz"
 )

+func (s *Server) basePath(p string) string {
+	return path.Join(s.httpBasePath, p)


Same comment than above.

main.go

kozlovic

Not a blocker, just wondering if we should rename the Option, config name, etc.. with starting with http to make it clear this is related to the http(s) monitoring endpoints?

kozlovic · 2020-05-13T18:11:34Z

server/configs/test.conf

@@ -6,6 +6,8 @@ listen: 127.0.0.1:4242

 http: 8222

+base_path: /nats


Sorry to notice that now, but should we call it http_base_path or http_root or something like that but with leading http_ to make it clear that this is a http option?

Good you mentioned, because I was also struggling with that, I was not really sure.
I kind of started with http_ because the monitoring part starts with http_. But then I started to notice that you have other http handlers (like profiling), that my base path doesn't include them, in the end I decided to remove the prefix.

I'd use http_base_path other than http_root because I think it's more common in other projects.

Let's go with http_base_path and HTTPBasePath for option, etc..

kozlovic · 2020-05-13T18:12:02Z

server/const.go

@@ -97,6 +97,9 @@ const (
 	// DEFAULT_HTTP_PORT is the default monitoring port.
 	DEFAULT_HTTP_PORT = 8222

+	// DEFAULT_BASE_PATH is the default base path for monitoring.
+	DEFAULT_BASE_PATH = "/"


If we agree on the http naming, you could also have this be DEFAULT_HTTP_xxx

kozlovic · 2020-05-13T18:13:01Z

server/opts.go

@@ -593,6 +595,8 @@ func (o *Options) processConfigFileLine(k string, v interface{}, errors *[]error
 		o.HTTPPort = int(v.(int64))
 	case "https_port":
 		o.HTTPSPort = int(v.(int64))
+	case "base_path":


You can have both, we sometimes do that. For instance:

case "http_base_path", "base_path": ..

to support multiple config names.

I'd keep only http_base_path as we don't really need to be compatible with anything.

Let's go with that and renaming of option, etc.. Will review again when the PR is updated.

matthiashanel · 2020-05-13T19:25:59Z

@guilherme-santos the failure you are seeing has nothing to do with this change.
As for the review I am good if Ivan is good.

kozlovic

LGTM. Thanks for your contribution!

Related to nats-io/nats-server#1392.

derekcollison reviewed May 13, 2020

View reviewed changes

derekcollison requested review from kozlovic and matthiashanel May 13, 2020 14:48

kozlovic requested changes May 13, 2020

View reviewed changes

matthiashanel reviewed May 13, 2020

View reviewed changes

main.go Outdated Show resolved Hide resolved

guilherme-santos force-pushed the master branch from de30b46 to 7e76c54 Compare May 13, 2020 16:15

kozlovic reviewed May 13, 2020

View reviewed changes

Implement basePath for monitoring endpoints

25858cb

guilherme-santos force-pushed the master branch from 7e76c54 to 25858cb Compare May 13, 2020 21:29

kozlovic approved these changes May 13, 2020

View reviewed changes

kozlovic changed the title ~~Implement basePath for monitoring endpoints~~ [ADDED] base path for monitoring endpoints May 13, 2020

kozlovic merged commit 54e0140 into nats-io:master May 13, 2020

kozlovic added a commit to nats-io/nats.docs that referenced this pull request Jun 25, 2020

Add http_base_path config

d9dcedc

Related to nats-io/nats-server#1392.

kozlovic mentioned this pull request Jun 25, 2020

Add http_base_path config nats-io/nats.docs#119

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ADDED] base path for monitoring endpoints #1392

[ADDED] base path for monitoring endpoints #1392

guilherme-santos commented May 13, 2020 •

edited

Loading

derekcollison left a comment

derekcollison May 13, 2020

kozlovic May 13, 2020

derekcollison May 13, 2020

kozlovic May 13, 2020

kozlovic left a comment

kozlovic May 13, 2020

kozlovic May 13, 2020

kozlovic left a comment

kozlovic May 13, 2020

guilherme-santos May 13, 2020

kozlovic May 13, 2020

kozlovic May 13, 2020

kozlovic May 13, 2020

guilherme-santos May 13, 2020

kozlovic May 13, 2020

matthiashanel commented May 13, 2020

kozlovic left a comment

		@@ -6,6 +6,8 @@ listen: 127.0.0.1:4242

		http: 8222

		base_path: /nats

[ADDED] base path for monitoring endpoints #1392

[ADDED] base path for monitoring endpoints #1392

Conversation

guilherme-santos commented May 13, 2020 • edited Loading

Changes proposed in this pull request:

derekcollison 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

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozlovic 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

matthiashanel commented May 13, 2020

kozlovic left a comment

Choose a reason for hiding this comment

guilherme-santos commented May 13, 2020 •

edited

Loading