Do not risk exposing unauthenticated webhook port on container

[kimsondrup]

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
• End user documentation updated

[k8s-ci-robot]

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.

[mloiseleur]

/ok-to-test
/assign @stevehipwell
Apart my little suggestion, it looks good.

[mloiseleur]

/lgtm

[stevehipwell]

In addition to the inline question, shouldn't the webhook be providing both a /livez and a /readyz endpoint instead of a /healthz endpoint to follow the K98s idiomatic pattern?

@mloiseleur there seems to be a large number of webhook changes coming in to the Helm chart, I thought this had all been heavily tested outside of the chart before the PR to add it? I was also under the impression that it had been tested locally before it was added?

[kashalls]

In addition to the inline question, shouldn't the webhook be providing both a /livez and a /readyz endpoint instead of a /healthz endpoint 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.

[kimsondrup]

In addition to the inline question, shouldn't the webhook be providing both a /livez and a /readyz endpoint instead of a /healthz endpoint 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.

[stevehipwell]

/lgtm
/approve

[stevehipwell]

@mloiseleur could you please approve this?

[Raffo]

/approve

[k8s-ci-robot]

[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

Needs approval from an approver in each of these files:

• OWNERS [Raffo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[craigcabrey]

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).

[kashalls]

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.

[craigcabrey]

Not to be pedantic, but the chart version is 1.15.0.

[kashalls]

Not to be pedantic, but the chart version is 1.15.0.

My bad, I saw the release was v0.15.0.