Skip to content

Conversation

polarctos
Copy link

Adds a config parameter --metrics-https (or env METRICS_HTTPS) that serves the metrics via HTTPS instead of HTTP.
It is reusing the TLS server certificates of the existing configured services of this proxy.

The metrics enablement parameter was named just --metrics-port instead of --metrics-http-port.
Thus now this is just an additional bool config flag to toggle HTTPS. To match the general port config parameter names, it could have also be named --metrics-https-port. On the other hand I see no benefit of actually allowing to run the metrics on two ports for HTTPS and HTTP.

To reduce the need for more config parameters for the metrics server certificate and key files, it just reuses the already configured matching service certificates.

Started a discussion about this here: #101 (comment)

Copy link
Collaborator

@kevinmcconnell kevinmcconnell left a comment

Choose a reason for hiding this comment

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

@polarctos thanks for the PR.

I was curious: could you describe your setup a bit where you would use this? I'd normally expect the metrics to be consumed and routed internally, whereas the regular service traffic is externally accessible. Which makes me wonder how common it is that the same cert setup is likely to be useful for both cases.

Happy to ship this if it'll be solving a common need; I'd just like to understand more about when this would be useful. Thanks!

runCommand.cmd.Flags().IntVar(&globalConfig.HttpPort, "http-port", getEnvInt("HTTP_PORT", server.DefaultHttpPort), "Port to serve HTTP traffic on")
runCommand.cmd.Flags().IntVar(&globalConfig.HttpsPort, "https-port", getEnvInt("HTTPS_PORT", server.DefaultHttpsPort), "Port to serve HTTPS traffic on")
runCommand.cmd.Flags().IntVar(&globalConfig.MetricsPort, "metrics-port", getEnvInt("METRICS_PORT", 0), "Publish metrics on the specified port (default zero to disable)")
runCommand.cmd.Flags().BoolVar(&globalConfig.MetricsHttps, "metrics-https", getEnvBool("METRICS_HTTPS", false), "Enable HTTPS for the metrics port (default false for HTTP)")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small thing, but I think we could refer to this as METRICS_TLS. That's really the layer we're adding here, and it's how we refer to this when deploying services as well.

I agree with your thoughts that we don't need two ports here; just being able to switch on or off TLS seems sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants