Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional port restriction for /metrics endpoint #1

Merged

Conversation

nohwnd
Copy link
Contributor

@nohwnd nohwnd commented Feb 24, 2019

I would like to restrict metrics to be only accessible inside of Kubernetes cluster and not outside. One way to do this is to setup authentication and authorization for the metrics endpoint, but that adds unnecessary management overhead.

A simpler way to do this is to expose the metrics only on port that is reachable within the cluster and not outside. This PR adds such filter by checking the port in the incoming request and returning 404 if it does not match.

An example usage in the default ASP.NET Core 2.2 app with https redirection:

## file Startup.cs
# put this before the https redirection (and also before authentication)
# use the new Port property on the options to specify the port to filter to
app.UsePrometheusServer(o => o.Port = 5005);
app.UseHttpsRedirection();
app.UseMvc();
## file Program.cs
# specify default and our monitoring ports to listen on
WebHost.CreateDefaultBuilder(args)
                .UseUrls(
                    "http://*:5000",
                    "https://*:5001",
                    "http://*:5005")
                .UseStartup<Startup>();
    }

To test this with the standard app you can run those queries. (e.g. via VSCode rest client)

# returns metrics
GET http://localhost:5005/metrics

###

# returns 404
GET https://localhost:5001/metrics

###

# returns 404
GET http://localhost:5000/metrics

### 

# returns values
GET https://localhost:5001/api/values

###

# returns 307 redirect to https 
# and then values
GET http://localhost:5000/api/values

###

# accessing the rest of the api via the monitoring port
# is not restricted so this alse returns 307 redirect to https 
# and then values
GET http://localhost:5005/api/values

@nohwnd
Copy link
Contributor Author

nohwnd commented Feb 24, 2019

I need to find a better mechanism for determining the port. This implementation apparently takes it from Host header and that is easily falsifiable. So instead I need to get the real port this request was bound to. I hope that is possible.

GET https://localhost:5001/metrics
Host: localhost:5005

@nohwnd
Copy link
Contributor Author

nohwnd commented Feb 24, 2019

The real port is exposed by Connection, so it is all fixed now.

@phnx47
Copy link
Member

phnx47 commented Feb 24, 2019

It is middleware. Use for all urls.

If you want to change use only one url and port, you need use special endpoint:
https://github.com/PrometheusClientNet/Prometheus.Client.MetricServer

@phnx47 phnx47 closed this Feb 24, 2019
@nohwnd
Copy link
Contributor Author

nohwnd commented Feb 24, 2019

Well that was harsh.

The middleware registration method already makes the basic setup simple. And this would be another step in that direction. The pattern of exposing the main functionality and maintenance functionality on different ports is used often in docker images and this would make it easy to do, without adding any overhead.

@phnx47
Copy link
Member

phnx47 commented Feb 24, 2019

I will think about this functional. But, I think we need use 'MapWhen'. Just return 'NotFound' is not Production ready

@nohwnd
Copy link
Contributor Author

nohwnd commented Feb 24, 2019

I am already doing it with MapWhen in my project to emulate this functionality I just did not want to change too much in this PR. I can rewrite it easily if you'd consider merging it :)

@phnx47
Copy link
Member

phnx47 commented Feb 24, 2019

@nohwnd I would like to review and test it.

@nohwnd
Copy link
Contributor Author

nohwnd commented Feb 24, 2019

Sure. I will update the PR tomorrow.

@phnx47 phnx47 reopened this Feb 24, 2019
@phnx47
Copy link
Member

phnx47 commented Feb 24, 2019

Ok, I'm waiting update

@nohwnd nohwnd force-pushed the restrict-access-to-metrics-by-port branch from 5311898 to e6ac027 Compare February 25, 2019 07:59
@nohwnd nohwnd force-pushed the restrict-access-to-metrics-by-port branch from e6ac027 to df6fd87 Compare February 25, 2019 08:16
@phnx47 phnx47 self-requested a review February 25, 2019 08:43
Copy link
Member

@phnx47 phnx47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good! I will test and publish it.

@phnx47 phnx47 merged commit 26a6e47 into prom-client-net:master Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants