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

Allow separate listeners for metrics and healthchecks #2380

Closed
jedsalazar opened this issue Mar 24, 2020 · 6 comments · Fixed by #2407
Closed

Allow separate listeners for metrics and healthchecks #2380

jedsalazar opened this issue Mar 24, 2020 · 6 comments · Fixed by #2407
Labels
area/deployment Issues or PRs related to deployment tooling or infrastructure. blocked/needs-design Categorizes the issue or PR as blocked because it needs a design document. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.

Comments

@jedsalazar
Copy link

Please describe the problem you have
We understand that Envoy will listen on 8002, both /ready and /stats/* endpoints get presented through here. It is /stats (and everything else under /stats/*) that we need to protect, but not /ready.

Our Contour/Envoy configuration requires /ready must be HTTP and not HTTPS due to #1503, and it has to be unprotected so load balancers can query it.

We looked at the Prometheus.io annotations in the Envoy deployment spec, but this does not seem to do anything.

Tried to configure the Prometheus stats address/port as command line arguments to contour. Specifically, something like:

--stats-address=0.0.0.0 --stats-port=8003

Unfortunately the flag did not work. /ready and /stats are stilled tied together after bouncing Contour and Envoy, and I believe this to be hard-coded.

If we look up "statsPort", we can find it here: https://github.com/projectcontour/contour/blob/v1.2.1/cmd/contour/servecontext.go#L121

We see it used here, creating a new listener: https://github.com/projectcontour/contour/blob/v1.2.1/cmd/contour/serve.go#L184

Which is this function: https://github.com/projectcontour/contour/blob/v1.2.1/internal/contour/listener.go#L213

And finally we see where our listener payload is generated: https://github.com/projectcontour/contour/blob/v1.2.1/internal/envoy/stats.go#L27

It seems /ready and /stats are tied together, and we need these to be separated. I believe we are looking towards a feature request.

@jpeach
Copy link
Contributor

jpeach commented Mar 24, 2020

@jedsalazar What is your goal here? When you say you are trying to protect endpoints, can you clarity the policy that you would like to implement? How would having /ready and /stats on different ports help you?

@jedsalazar
Copy link
Author

jedsalazar commented Mar 24, 2020

@jpeach the goal is to bind /metrics to a discrete port which would disallow anonymous access to metrics endpoints by using kube-rbac-proxy for the specified /metrics port.

@jedsalazar
Copy link
Author

Calico has implemented similar functionality.
https://docs.tigera.io/v2.7/reference/felix/configuration

This refers to similar functionality to their "FELIX_PROMETHEUSMETRICSHOST" and "FELIX_PROMETHEUSMETRICSPORT" options.

@jpeach jpeach changed the title protecting /metrics endpoints using kube-rbac-proxy Allow separate listeners for metrics and healthchecks Mar 24, 2020
@jpeach jpeach added the area/deployment Issues or PRs related to deployment tooling or infrastructure. label Mar 24, 2020
@jpeach
Copy link
Contributor

jpeach commented Mar 24, 2020

That makes sense, retitling for clarity. The ask here is for operators to be able to specify separate listeners for the health check and metrics services. This will involve changing the command line flags and the deployment YAML, and considering how backwards compatibility can be supported.

@youngnick youngnick added blocked/needs-design Categorizes the issue or PR as blocked because it needs a design document. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Mar 24, 2020
@poidag-zz
Copy link
Contributor

Beginnings or draft design doc

@stevesloka
Copy link
Member

@pickledrick I think a design doc is fine, but for this, I'd recommend we just expose additional args to contour serve to allow the metrics endpoint to be changed.

The default should be to keep the current behavior consistent.

jpeach pushed a commit that referenced this issue Apr 13, 2020
This provides the ability to separate the HTTP listeners for the health
and metrics services.  Both address and port are configurable for each
service. Defaults are maintained to support backward compatibility.

This fixes #2380.

Signed-off-by: Peter Grant <pegrant@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/deployment Issues or PRs related to deployment tooling or infrastructure. blocked/needs-design Categorizes the issue or PR as blocked because it needs a design document. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants