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.

Sign up for GitHub

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

Jump to bottom

feat: Run a separate endpoint for /health #77

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: Run a separate endpoint for /health #77

wants to merge 6 commits into from
+66 −50

Conversation

NickLarsenNZ
Copy link

@NickLarsenNZ NickLarsenNZ commented Feb 6, 2025 •
edited
Loading

Fixes #73

Caution

This change is blocked on external-dns helm-chart compatibility. See: kubernetes-sigs/external-dns#5071

This allows the privileged webhook server to be restricted to localhost (for sidecar mode), while exposing the /health endpoint to the kubelet.

  • Run the /health endpoint.
  • Add consistency between the webhook, health, and metrics servers.
  • Update docs.

Important

In 1c42814, I have removed the SERVER_HOST variable and exposed ports, so the webhook listens on localhost (default). But after looking at some of the test scripts, it appears it might need to be exposed.

I can revert this commit, and instead add warnings to the exposed ports for people who might copy/paste them.

@NickLarsenNZ NickLarsenNZ mentioned this pull request Feb 6, 2025
Open
@NickLarsenNZ NickLarsenNZ changed the title feat: Run separate health endpoint feat: Run a separate endpoint for /health Feb 6, 2025
@zak905
Copy link
Contributor

zak905 commented Feb 10, 2025

Hi @NickLarsenNZ, thanks for the contribution! We appreciate.

After discussing with the team, we found that there are some tunings to be made to comply with the official documentation: https://kubernetes-sigs.github.io/external-dns/latest/docs/tutorials/webhook-provider/#provider-endpoints ( as pointed out in the response of the issue you posted: kubernetes-sigs/external-dns#5071 (comment)

  • With the current implementation there actually three servers: the webhook server, the metrics server, and the health server
  • The documentation states that the metrics and the health endpoints should be served at the same port. At the end there should be only two servers: The webhook server (by default served at 8888) and the metrics/health server (served at 8080 by default and bound to 0.0.0.0)
  • As you can see, we have made a recent change to allow the end user to configure whether the /metrics is served at the same or at a different port than webhook server: https://github.com/ionos-cloud/external-dns-ionos-webhook/blob/main/cmd/webhook/init/server/server.go#L45. With the current information that we have, it makes sens to disable this possiblity, because having the metrics served at the same port, would imply also that the health endpoint is served at the same port.

I hope that makes sens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SECURITY: The /health endpoint should be on a different port than the webhook
2 participants
@NickLarsenNZ @zak905