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

Expose /healthz on a different port to allow binding SERVER_HOST to localhost #131

Closed
Skaronator opened this issue Oct 2, 2024 · 4 comments · Fixed by #132
Closed

Expose /healthz on a different port to allow binding SERVER_HOST to localhost #131

Skaronator opened this issue Oct 2, 2024 · 4 comments · Fixed by #132
Labels
bug Something isn't working enhancement New feature or request

Comments

@Skaronator
Copy link

Skaronator commented Oct 2, 2024

By design, the external-dns webhook implementation expects that the webhook is available on port 8888. For security reasons, this port should be bound only to localhost to avoid access from other pods.

This will result that the kubelet won't be able to reach /healthz on port 8888. Therefor it would make sense to expose /healthz on port 8080 to be compatible with the offical helm-chart.

Here is the ref from the offical docs:

The default recommended port is 8888, and should listen only on localhost (ie: only accessible for k8s probes and external-dns).

https://kubernetes-sigs.github.io/external-dns/latest/docs/tutorials/webhook-provider/#implementation-requirements

Additionally:

The metrics should listen “:8080” on /metrics following Open Metrics format.

https://kubernetes-sigs.github.io/external-dns/latest/docs/tutorials/webhook-provider/#metrics-support

Background: As of now, this provider is incompatible with the external-dns helm chart. I did raise an issue to potentially fix this incompatibility, but the non-compatible helm-chart is by design. Hence, this provider needs to be adapted.
kubernetes-sigs/external-dns#4764 (comment)

@muhlba91
Copy link
Owner

muhlba91 commented Oct 2, 2024

thank you for raising this incompatibility! 🙂

the provider was designed/released before the official helm chart was finished/released. i'll see to get compatibility w.r.t. the official chart as soon as i get some more time.

@Skaronator
Copy link
Author

Thank you! No problem, I'm using a workaround for now. I’ve been using the provider for a few months already, and it’s been working great. Thank you for your hard work :)

@muhlba91 muhlba91 added bug Something isn't working enhancement New feature or request labels Oct 4, 2024
@muhlba91 muhlba91 reopened this Oct 4, 2024
@muhlba91
Copy link
Owner

muhlba91 commented Oct 4, 2024

i have separated the healthz endpoint now, and also introduced a /metrics endpoint on the same port.

the functionality is:

  • /healthz: status code 200 if the provider is up and a status call to AdGuard succeeds; else 503 error code
  • /metrics: a JSON response containing healthy (boolean) with the same conditions as above

the change breaks backwards-compatibility!

@Skaronator
Copy link
Author

Thanks for quickly fixing it. Just deployed it and it works as expected, thank you!

fyi: the /metrics endpoint should be in Open Metrics format as stated by the original docs. Possible metrics could be the response time, error rate, request count from/to adguard API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants