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

fix: use hostIP also on entrypoint and healthChecks when set #992

Merged
merged 4 commits into from
Apr 4, 2024
Merged

fix: use hostIP also on entrypoint and healthChecks when set #992

merged 4 commits into from
Apr 4, 2024

Conversation

Zebradil
Copy link
Contributor

@Zebradil Zebradil commented Jan 21, 2024

What does this PR do?

This PR adds a new value ports.*.host that allows defining the entrypoint host to listen to.

Motivation

This is useful when hostNetwork is enabled, as hostIP doesn't make any effect in this case.
Without setting an entrypoint's host, service entrypoints like metrics are exposed to the world.

More

  • Yes, I updated the tests accordingly
  • Yes, I ran make test and all the tests passed

Notes

This proposal needs to be considered more carefully. Here are some data points to keep in mind:

  • I don't see any other reason to override host (e.g. using 127.0.0.1 instead of default 0.0.0.0), except when using hostNetwork.
  • Changing ports.traefik.host from the default value can break probes because they use the Pod IP by default. To make them work, httpGet.host has to be set to the same value as ports.traefik.host. However, at the moment, this helm chart doesn't provide a way to set probes' host via values.
  • At first, I considered using hostIP instead of a dedicated host, but then I realized those are two different things and should not be mixed.

Maybe it makes sense to add some logic to link hostNetwork and ports.*.host fields 🤔

@mloiseleur
Copy link
Contributor

Thanks for your interest in Traefik and this PR @Zebradil !

In PR description:

I realized those are two different things and should not be mixed.

When reading documentation, it's not very clear.
On hostIP:

Use hostIP if set. If not set, Kubernetes will default to 0.0.0.0, which means it's listening on all your interfaces and all your IPs. You may want to set this value if you need traefik to listen on specific interface only.

On host:

Listen on a specific host. It is useful together with hostNetwork: true when you want to listen on a specific interface. If not set, Traefik will listen on all interfaces.

When it's really super specific, the encouraged approach is to use additionalArguments.

Would you please detail how they are different things and should not be mixed ?

@mloiseleur
Copy link
Contributor

Since there is no answer from @Zebradil, I close this PR.
Feel free to re-open it if you need it.

@mloiseleur mloiseleur closed this Feb 26, 2024
@Zebradil
Copy link
Contributor Author

Sorry, I haven't had enough time to perform a couple of experiments that are required to provide more information.

If you don't mind, I'll ping you in this PR when it's ready.

@mloiseleur mloiseleur reopened this Feb 26, 2024
@mloiseleur
Copy link
Contributor

Sure, I re-open the PR then.

@Zebradil
Copy link
Contributor Author

Zebradil commented Apr 2, 2024

Sorry for the long delay, I have finally double-checked the case.

So for me, hostIP doesn't work at all. The only way to bind traefik port to a particular interface is to specify the corresponding host in the command line argument.

If I set hostNetwork: true, traefik by default binds all ports to 0.0.0.0 and, for example, the metrics port becomes available on all interfaces including those that are public to the internet.

Here is an example connections via an external interface (192.168.0.74) and via localhost:

$ curl -fsS localhost:9101/metrics | head -n3
# HELP go_gc_duration_seconds A summary of the pause duration of garbage collection cycles.
# TYPE go_gc_duration_seconds summary
go_gc_duration_seconds{quantile="0"} 1.3503e-05

$ curl -fsS 192.168.0.74:9101/metrics | head -n3
# HELP go_gc_duration_seconds A summary of the pause duration of garbage collection cycles.
# TYPE go_gc_duration_seconds summary
go_gc_duration_seconds{quantile="0"} 1.3503e-05

The related container configuration:

  containers:
  - args:
    - --entrypoints.metrics.address=:9101/tcp
    - ...
    image: docker.io/traefik:v2.11.0
    ports:
    - containerPort: 9101
      hostPort: 9101
      name: metrics
      protocol: TCP
    - ...
  dnsPolicy: ClusterFirstWithHostNet
  hostNetwork: true
  ...

Now, if I set hostIP using the following port configuration:

    ports:
    - containerPort: 9101
      hostIP: localhost
      hostPort: 9101
      name: metrics
      protocol: TCP

I still can connect to the metrics port as before, even though I expect it to be unavailable on the external interface:

$ curl -fsS localhost:9101/metrics | head -n3
# HELP go_gc_duration_seconds A summary of the pause duration of garbage collection cycles.
# TYPE go_gc_duration_seconds summary
go_gc_duration_seconds{quantile="0"} 1.7295e-05
curl: (23) Failure writing output to destination

$ curl -fsS 192.168.0.74:9101/metrics | head -n3
# HELP go_gc_duration_seconds A summary of the pause duration of garbage collection cycles.
# TYPE go_gc_duration_seconds summary
go_gc_duration_seconds{quantile="0"} 1.7295e-05

But if I revert hostIP and set localhost in the arguments, traefik correctly exposes the port only on the loopback interface:

  - args:
    - --entrypoints.metrics.address=localhost:9101/tcp
    - ...
$ curl -fsS localhost:9101/metrics | head -n3
# HELP go_gc_duration_seconds A summary of the pause duration of garbage collection cycles.
# TYPE go_gc_duration_seconds summary
go_gc_duration_seconds{quantile="0"} 3.2208e-05

$ curl -fsS 192.168.0.74:9101/metrics | head -n3
curl: (7) Failed to connect to 192.168.0.74 port 9101 after 3 ms: Couldn't connect to server

I wasn't able to find a proper explanation of how hostIP is supposed to work. Maybe it is a Kubernetes bug or an issue with my setup (I use k3s). However, I think it makes sense to allow users to set the host part in traefik arguments as can be done without using the chart.

My notes in the original comment are related to the overall ergonomics of this change. I see room for improvement by adding extra logic in addition to the existing change. Maybe it is better to rename host to bindAddress or something like that — to make it more distinct from the existing hostIP option.

@Zebradil
Copy link
Contributor Author

Zebradil commented Apr 2, 2024

@mloiseleur I removed the dedicated host field and used hostIP instead. I also added logic for setting host field of HTTP probes to match hostIP of the traefik entrypoint.

@mloiseleur mloiseleur changed the title feat: add entrypoint host to ports configuration fix: use hostIP also on entrypoint when set + allows to configure custom healthCheck host Apr 3, 2024
@mloiseleur mloiseleur changed the title fix: use hostIP also on entrypoint when set + allows to configure custom healthCheck host fix: use hostIP also on entrypoint when set + allows to configure custom healthChecks host Apr 3, 2024
@mloiseleur mloiseleur changed the title fix: use hostIP also on entrypoint when set + allows to configure custom healthChecks host fix: use hostIP also on entrypoint and healthChecks when set Apr 3, 2024
Copy link
Contributor

@mloiseleur mloiseleur left a comment

Choose a reason for hiding this comment

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

See my suggestions

Zebradil and others added 2 commits April 3, 2024 13:02
Co-authored-by: Michel Loiseleur <97035654+mloiseleur@users.noreply.github.com>
@Zebradil Zebradil requested a review from mloiseleur April 3, 2024 11:02
@Zebradil
Copy link
Contributor Author

Zebradil commented Apr 3, 2024

Good points, I applied all of them.

Copy link
Contributor

@darkweaver87 darkweaver87 left a comment

Choose a reason for hiding this comment

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

LGTM

@traefiker traefiker merged commit 82a3cab into traefik:master Apr 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants