muhlba91 / external-dns-provider-adguard

External DNS webhook provider for Adguard
Apache License 2.0
32 stars 4 forks source link

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

Closed Skaronator closed 1 month ago

Skaronator commented 1 month ago

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. https://github.com/kubernetes-sigs/external-dns/issues/4764#issuecomment-2387739526

muhlba91 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

the functionality is:

the change breaks backwards-compatibility!

Skaronator commented 1 month ago

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.