deis / router

Edge router for Deis Workflow
https://deis.com
MIT License
80 stars 57 forks source link

Add status endpoint #333

Closed pixeleet closed 7 years ago

pixeleet commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

codecov-io commented 7 years ago

Codecov Report

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

Impacted file tree graph

@@            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% <ø> (ø) :arrow_up:
model/model.go 49.38% <100%> (+0.2%) :arrow_up:

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.

mboersma commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

Sounds good to me :)

pixeleet commented 7 years ago

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