Add status endpoint #333

pixeleet · 2017-03-30T10:54:16Z

I'd like to add a sysdig app chcek on the router, but it only installs the stub_status module, doesn't use it to expose the endpoint or provides configuration for it.

@krancour What else would I need to submit to get this in? Is this even correct at all?

deis-admin · 2017-03-30T10:54:17Z

Thanks for the contribution! Please ensure your commits follow our style guide. This code will be tested once a Deis maintainer reviews it.

deis-bot · 2017-03-30T10:54:19Z

@krancour, @lachie83 and @arschles are potential reviewers of this pull request based on my analysis of git blame information. Thanks @pixeleet!

codecov-io · 2017-03-30T11:10:46Z

Codecov Report

Merging #333 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #333     +/-   ##
=========================================
+ Coverage   56.47%   56.57%   +0.1%     
=========================================
  Files           6        6             
  Lines         425      426      +1     
=========================================
+ Hits          240      241      +1     
  Misses        159      159             
  Partials       26       26

Impacted Files	Coverage Δ
nginx/config.go	`56.86% <ø> (ø)`	⬆️
model/model.go	`49.38% <100%> (+0.2%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8e207e...5015154. Read the comment docs.

krancour · 2017-03-30T15:44:12Z

nginx/config.go

@@ -293,6 +293,17 @@ http {
 		proxy_pass {{$builderConfig.ServiceIP}}:2222;
 	}
 }{{ end }}
+
+{{ if $routerConfig.StatusEndpoint }}
+  sevrer {


Also, probably this shouldn't be its own vhost / server block. As it stands, I don' think any traffic will ever reach this endpoint. The router uses hostname-based routing... so every request either goes to an application or to the default vhost. This new server block is neither of those things. Probably it would be better to add this to the existing default vhost. Once you do, the only way to access this endpoint will be by making a request to the router that doesn't match any other application-- e.g. bogus.example.com/nginx_status. Is that suitable?

Or I'm wondering if maybe we shouldn't reserve the router-status subdomain (relative to your platform domain) for use by the router (i.e. add it to the short list of names that the controller doesn't allow for user apps). Then router status could consistently be accesses at router-status.example.com...

We'd probably want to enable whitelists on that endpoint as well.

Hm, I think it would be easy to say that the router can assume a domain for itself, that could be the default vhost, if the statusEndpoint option is true there we could expose it under the default /nginx_status. Is that a viable approach?

I imagine it would make some things easier, like explaining how to hit the router from outside. But now this is becoming a new thing => Adding a router domain, or something along those lines, && enabling status endpoint on default vhost if I'm not mistaken.

Unless @mboersma objects, I am unopposed to router.<domain> (or maybe deis-router.<domain>) becoming the router's official address, much as deis.<domain is the address of the controller and deis-builder.<domain> is the address of the builder. We'd have to add router (or deis-router) to the list of reserved names in the controller.

That being said... that can be implemented independently (i.e. in another PR) of this feature. router.<domain> will already get you to the default vhost today unless you have a userland app called router. Giving the router its own domain is just a formality of sorts. So, while I am unopposed, let's consider it on its own merits. Would you mind opening an issue?

In the meantime, this PR can proceed just by moving the /nginx_status endpoint to the existing default vhost.

mboersma · 2017-04-03T17:30:00Z

@krancour opined:

Probably it would be better to add this to the existing default vhost...

This PR still has some outstanding review questions and isn't ready quite yet, so I'm going to move it to the next milestone.

pixeleet · 2017-04-04T13:23:24Z

Added a new PR per the discussion with @krancour moved the endpoint into the default vhost and removed the restrictions on it as it should be managed by the whitelist held by the router

krancour · 2017-04-04T13:34:25Z

nginx/config.go

@@ -186,6 +186,9 @@ http {
 		location / {
 			return 404;
 		}
+		location /nginx_status {
+			stub_status on;
+		}


You still want this to be conditional though, yes? Also, should this location (but not the entire default vhost) be either locked down to a configurable whitelist, or possibly protected with a configurable username/password. I tend to imagine we don't want to expose these stats to the world.

I'll read up on whitelisting in the router, pushed a single commit for the configurability

krancour · 2017-04-04T13:55:11Z

@pixeleet, your last commit seems to have a lot of (breaking) changes in it that appear to be beyond the scope of this PR. Accidental?

pixeleet · 2017-04-04T13:58:21Z

nginx/config.go

@@ -61,7 +58,7 @@ http {
 	real_ip_header X-Forwarded-For;
 	{{- end }}

-	log_format upstreaminfo '{{ $routerConfig.LogFormat }}';
+	log_format upstreaminfo '[$time_iso8601] - $app_name - $remote_addr - $remote_user - $status - "$request" - $bytes_sent - "$http_referer" - "$http_user_agent" - "$server_name" - $upstream_addr - $http_host - $upstream_response_time - $request_time';


I'm not sure how this got here

pixeleet · 2017-04-04T14:05:34Z

I cleaned it up into a single commit, and will look into what we talked about whitelisting or a basic auth addition to it.

krancour · 2017-04-04T14:05:46Z

nginx/config.go

@@ -186,6 +186,11 @@ http {
 		location / {
 			return 404;
 		}
+		{{ if $routerConfig.statusEndpoint }}


I see you groomed the commits and force-pushed... I think you lost the model changes that complement this.

Damn indeed. :) That's stupid.

I would consider moving this section up between the health check and the location / block. I know Nginx works by making the best/closest match, but for human readability, it usually makes the most sense to have the more specific patterns listed first so that you can think of unmatched requests as progressively "falling through" the list for a possible match to a more general pattern... if you don't mind.

krancour · 2017-04-04T14:10:37Z

@pixeleet until #334 is addressed, for good measure, the same location block should be conditionally added right here to account for the case that someone has provided an application for use as the default vhost.

krancour · 2017-04-04T14:13:14Z

nginx/config.go

+      stub_status on;
+    }
+    {{ end }}
+


Can the leading whitespace be made consistent with what's above and below?

Sure, just a quick question, are you fine with just restricting /nginx_status to 127.0.0.1 for now and deny all? the use case primarily being monitoring this should be fine

Yes. We've already done that very same thing here: https://github.com/deis/router/blob/master/nginx/config.go#L202-L207

This makes me wonder a few things.

Are you aware that lots of router metrics are already available in Grafana? (Just making sure.)

Are the metrics that are already exposed at 127.0.0.1:9090/stats something you can work with? i.e. Is this new endpoint needed for something you can't already get?

Purely to satisfy my own curiosity, what agent / metric collector would you be running that would be hitting this from the router's own loopback interface?

Is 127.0.0.1:9090/nginx_status a better place for this? I ask because the whole purpose of that server block listening on 9090 is to have a block that's guaranteed to never be using PROXY protocol. Anything listening for traffic from the outside world (e.g. the default vhost) may very well have that enabled-- which means it wouldn't be able to handle traffic coming at it from somewhere other than a proxy / LB that also has that enabled.

1, yes I'm aware, we're using mainly sysdig and the grafana part we skipped from our setup
2, sysdig wants me to have the nginx stub status exposed
3, sysdig cloud for the containers and newrelic for api performance metrics
4, 127.0.0.1:9090/nginx_status works just fine for me! :)

127.0.0.1:9090/nginx_status works just fine for me!

Ok. Awesome. Let's start there. If, as time goes on, there is some compelling reason to expose this beyond that, we can take it up then.

krancour · 2017-04-04T16:36:24Z

@pixeleet I think, probably based on our conclusion of putting this endpoint at 127.0.0.1:9090/nginx_status, there is no remaining need to make this conditional. Thoughts?

pixeleet · 2017-04-04T16:44:30Z

Sounds good to me :)

pixeleet · 2017-04-04T16:49:13Z

I'll close this PR and will open a separate clean one just for the /nginx_status location on 9090

krancour reviewed Mar 30, 2017

View reviewed changes

mboersma added the awaiting review label Mar 30, 2017

mboersma added this to the v2.13 milestone Mar 30, 2017

mboersma modified the milestones: v2.14, v2.13 Apr 3, 2017

krancour reviewed Apr 4, 2017

View reviewed changes

krancour mentioned this pull request Apr 4, 2017

Claiming a reserved name for the router #334

Open

pixeleet commented Apr 4, 2017

View reviewed changes

add status endpoint

450d3f1

pixeleet force-pushed the feature/status-endpoint branch from 3fc48e6 to 450d3f1 Compare April 4, 2017 14:04

krancour reviewed Apr 4, 2017

View reviewed changes

add StatusEndpoint to model

6dec5a9

add statusEndpoint for default app too

041b2c3

krancour reviewed Apr 4, 2017

View reviewed changes

restrict to localhost

5015154

pixeleet closed this Apr 4, 2017

deis-admin removed the awaiting review label Apr 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add status endpoint #333

Add status endpoint #333

pixeleet commented Mar 30, 2017

deis-admin commented Mar 30, 2017

deis-bot commented Mar 30, 2017

codecov-io commented Mar 30, 2017 •

edited

Loading

krancour Mar 30, 2017 •

edited

Loading

pixeleet Mar 30, 2017

krancour Mar 30, 2017

pixeleet Apr 4, 2017

krancour Apr 4, 2017 •

edited

Loading

mboersma commented Apr 3, 2017

pixeleet commented Apr 4, 2017

krancour Apr 4, 2017

pixeleet Apr 4, 2017

krancour commented Apr 4, 2017

pixeleet Apr 4, 2017

pixeleet commented Apr 4, 2017

krancour Apr 4, 2017

pixeleet Apr 4, 2017

krancour Apr 4, 2017

krancour commented Apr 4, 2017

krancour Apr 4, 2017

pixeleet Apr 4, 2017

krancour Apr 4, 2017 •

edited

Loading

pixeleet Apr 4, 2017

krancour Apr 4, 2017

krancour commented Apr 4, 2017

pixeleet commented Apr 4, 2017

pixeleet commented Apr 4, 2017

Add status endpoint #333

Add status endpoint #333

Conversation

pixeleet commented Mar 30, 2017

deis-admin commented Mar 30, 2017

deis-bot commented Mar 30, 2017

codecov-io commented Mar 30, 2017 • edited Loading

Codecov Report

krancour Mar 30, 2017 • 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

krancour Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

mboersma commented Apr 3, 2017

pixeleet commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krancour commented Apr 4, 2017

Choose a reason for hiding this comment

pixeleet commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krancour commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krancour Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krancour commented Apr 4, 2017

pixeleet commented Apr 4, 2017

pixeleet commented Apr 4, 2017

codecov-io commented Mar 30, 2017 •

edited

Loading

krancour Mar 30, 2017 •

edited

Loading

krancour Apr 4, 2017 •

edited

Loading

krancour Apr 4, 2017 •

edited

Loading