external-dns
external-dns copied to clipboard
kubernetes-sigs
Do not risk exposing unauthenticated webhook port on container
Description
The current doc for the webhook provider expect the k8s probe to call localhost but the normal Kubernetes behavior is to call the pods IP so the current doc doesn't work.
Some implementers work around this by expecting users to override the livenessProbe and readinessProbe (see example external-dns-ionos-webhook) but this might as well be upstreamed
Checklist
- [ ] Unit tests updated
- [x] End user documentation updated
Hi @kimsondrup. Thanks for your PR.
I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
I understand the commands that are listed here.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.
/ok-to-test /assign @stevehipwell Apart my little suggestion, it looks good.
In addition to the inline question, shouldn't the webhook be providing both a
/livezand a/readyzendpoint instead of a/healthzendpoint to follow the K98s idiomatic pattern?
I don't think it matters much at the current moment as the base external-dns serves only a /healthz probe. You might want to create an additional PR to bring us within this pattern @stevehipwell .
It also wouldn't cause much of a problem to add these probes as it is a simple addition. I already have it implemented on the external-dns-unifi-webhook, but at a quick glance a couple webhook providers are only supplying the /healthz probe.
In addition to the inline question, shouldn't the webhook be providing both a
/livezand a/readyzendpoint instead of a/healthzendpoint to follow the K98s idiomatic pattern?
Since External DNS webhook only receive traffic from metrics scraping on the pod IP and its webhook API on localhost an separate endpoints for readiness probe will not add much value if any at all.
The External DNS process will call localhost:8888 without regard for the readiness probe and I am not even sure if metrics scraping even respects the readiness probe.
As @kashalls said, I think this should be in another PR.
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: Raffo, stevehipwell
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [Raffo]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
I don't know if it was intentional or not, but this does break existing setups utilizing the webhook provider (since the port name changed).
I don't know if it was intentional or not, but this does break existing setups utilizing the webhook provider (since the port name changed).
Anything under major version v1.0.0 is considered an always breaking version.
Not to be pedantic, but the chart version is 1.15.0.
My bad, I saw the release was v0.15.0.