Skip to content

Conversation

enocom
Copy link
Member

@enocom enocom commented Aug 31, 2022

In Kubernetes, the convention is to bind HTTP probes and Prometheus endpoints to
0.0.0.0 (both lo and eth0). Since people might want to run this code on a GCE
VM, default to localhost, but otherwise support binding to both interfaces.

Fixes #1359.

@enocom enocom requested review from a team and hessjcg August 31, 2022 16:10
@enocom
Copy link
Member Author

enocom commented Aug 31, 2022

This probably should be changed such that Prometheus's endpoint is available on all interfaces, but the remaining endpoints are localhost only.

enocom added 2 commits August 31, 2022 16:02
With this commit, callers can bind the HTTP server for Prometheus
metrics separately from the HTTP server that provides health checks.
cmd/root_test.go Outdated
c.SetArgs([]string{"--prometheus", "my-project:my-region:my-instance"})
c.SetArgs([]string{
"--prometheus",
"--prometheus-address", "localhost",
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need a test where this isn't localhost. That's the default value

cmd/root.go Outdated
if needsHTTPServer {
server := &http.Server{
srv := &http.Server{
Addr: fmt.Sprintf("localhost:%s", cmd.httpPort),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be putting 0.0.0.0 here?

cmd/root.go Outdated
prometheus bool
prometheusNamespace string
prometheusAddress string
prometheusPort string

Choose a reason for hiding this comment

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

[minor] Since it’s prometheusPort, what about healthCheckPort instead of httpPort?

cmd/root.go Outdated
telemetryPrefix string
prometheus bool
prometheusNamespace string
prometheusAddress string

Choose a reason for hiding this comment

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

We also need httpAddress, right? Otherwise, the probe will fail (am I missing something?).

@enocom
Copy link
Member Author

enocom commented Sep 4, 2022

After going back and forth on this, I think the convention is to bind HTTP probes to 0.0.0.0 (both lo and eth0) in a kubernetes context. Since people might want to run this on a GCE VM, I'll leave the default to localhost, but otherwise support binding to both interfaces.

See the docs where it says that the kublet defaults to the pod ID for HTTP probes,

Copy link

@daquinoaldo daquinoaldo left a comment

Choose a reason for hiding this comment

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

Sounds perfect! 🙌

Copy link
Collaborator

@hessjcg hessjcg left a comment

Choose a reason for hiding this comment

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

LGTM

@enocom enocom merged commit b53d77f into main Sep 6, 2022
@enocom enocom deleted the anyhost branch September 6, 2022 20:37
enocom added a commit to GoogleCloudPlatform/alloydb-auth-proxy that referenced this pull request Sep 13, 2022
In Kubernetes, the convention is to bind HTTP probes and Prometheus endpoints to
0.0.0.0 (both lo and eth0). Since people might want to run this code on a GCE
VM, default to localhost, but otherwise support binding to both interfaces.

This is a port of GoogleCloudPlatform/cloud-sql-proxy#1365.
enocom added a commit to GoogleCloudPlatform/alloydb-auth-proxy that referenced this pull request Sep 14, 2022
In Kubernetes, the convention is to bind HTTP probes and Prometheus endpoints to
0.0.0.0 (both lo and eth0). Since people might want to run this code on a GCE
VM, default to localhost, but otherwise support binding to both interfaces.

This is a port of GoogleCloudPlatform/cloud-sql-proxy#1365.
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.

[v2] prometheus and health checks listen on localhost instead of anyhost
3 participants