feat: support webhook provider in Chart

[mloiseleur]

Description

Adds support to the Helm chart for running a webhook provider as a sidecar.
Fixes #4025
Alternative implementation of #3966.

Checklist

• End user documentation updated

[mloiseleur]

@stevehipwell @johngmyers I submit this alternative implementation in the hope to find an agreement between your two positions.

I have tried to make this PR:

1. Follows current API
2. Avoid extraneous parameters

It tries to reuse as much as possible from main container:

• Same Secret, since it's possible to set multiple files into the same Secret
• Same readinessProbe & livenessProbe

It's breaking because now, with webhook provider, a provider can have specific args, env, securityContext and so on...

For the probes, I noticed that ionos and adguard uses /health instead of /healthz
@akrieg-ionos @muhlba91 What do you think about unifying probes path of your providers under /healthz ?

[muhlba91]

sounds good to me, and it makes it easier for users to switch providers.

i suggest we also add this in https://github.com/kubernetes-sigs/external-dns/blob/master/docs/tutorials/webhook-provider.md as kind of a convention that if a health endpoint is provided by the provider it should listen to /healthz?

[stevehipwell]

@mloiseleur I like this approach, I do have a couple of comments/questions.

• How would this work with an external webhook provider?
• Could we not make this a breaking change by supporting provider: <NAME> with a type test?

[mloiseleur]

How would this work with an external webhook provider?

For instance, on _ionos_

with those values:

provider:
  name: ionos
  env:
    - name: LOG_LEVEL
      value: debug
    - name: IONOS_API_KEY
      valueFrom:
        secretKeyRef:
          name: ionos-credentials
          key: api-key
    - name: SERVER_HOST
      value: "0.0.0.0"
    - name: IONOS_DEBUG
      value: "true"

It produces this output

      [...]
      containers:
        - name: external-dns
          securityContext:
            allowPrivilegeEscalation: false
            capabilities:
              drop:
              - ALL
            readOnlyRootFilesystem: true
            runAsNonRoot: true
            runAsUser: 65534
          image: registry.k8s.io/external-dns/external-dns:v0.13.6
          imagePullPolicy: IfNotPresent
          args:
            - --log-level=info
            - --log-format=text
            - --interval=1m
            - --source=service
            - --source=ingress
            - --policy=upsert-only
            - --registry=txt
            - --provider=webhook
          ports:
            - name: http
              protocol: TCP
              containerPort: 7979
          livenessProbe:
            failureThreshold: 2
            httpGet:
              path: /healthz
              port: http
            initialDelaySeconds: 10
            periodSeconds: 10
            successThreshold: 1
            timeoutSeconds: 5
          readinessProbe:
            failureThreshold: 6
            httpGet:
              path: /healthz
              port: http
            initialDelaySeconds: 5
            periodSeconds: 10
            successThreshold: 1
            timeoutSeconds: 5
        - name: ionos
          image: ghcr.io/ionos-cloud/external-dns-ionos-webhook:v0.6.0
          imagePullPolicy: IfNotPresent
          env:
            - name: LOG_LEVEL
              value: debug
            - name: IONOS_API_KEY
              valueFrom:
                secretKeyRef:
                  key: api-key
                  name: ionos-credentials
            - name: SERVER_HOST
              value: 0.0.0.0
            - name: IONOS_DEBUG
              value: "true"
          ports:
            - name: http
              protocol: TCP
              containerPort: 8888
          livenessProbe:
            failureThreshold: 2
            httpGet:
              path: /healthz
              port: http
            initialDelaySeconds: 10
            periodSeconds: 10
            successThreshold: 1
            timeoutSeconds: 5
          readinessProbe:
            failureThreshold: 6
            httpGet:
              path: /healthz
              port: http
            initialDelaySeconds: 5
            periodSeconds: 10
            successThreshold: 1
            timeoutSeconds: 5

Could we not make this a breaking change by supporting provider: <NAME> with a type test?

There is an implementation without breaking change through e00eb22. It makes the code more complex.

[stevehipwell]

@mloiseleur an external webhook provider would be one running at a URL (separate pod or external to cluster etc) so would only need the args and not the sidecar container.

[johngmyers]

A webhook outside the pod is explicitly not supported. That would require external-dns to support authenticating to the webhook, otherwise attackers could make changes to DNS by making requests directly to the webhook.

[johngmyers]

Secrets should not be shared across the main container and sidecar. They should only be readable to containers that need them in order to reduce the risk of compromise. Most of the time (when running a webhook provider) only the webhook sidecar will need secrets, though there are a few obscure sources that could need secrets.

[mloiseleur]

Most of the time (when running a webhook provider) only the webhook sidecar will need secrets.

Oh That's right !
🤔 Wdyt then about mounting the secret only on external-dns for in-tree provider and only on the sidecar for webhook providers ?
cc @stevehipwell @mrueg

[stevehipwell]

I would be strongly in favour of not supporting secretConfiguration for the webhook sidecar as secrets should preferably be managed outside of the Helm chart and then interacted with via the extraVolumes & extraVolumeMounts values.

[johngmyers]

The cloudfoundry source takes a password, but I suppose that can only come in through args, not a mounted secret. So perhaps we could say it would be unlikely for a future source to require a mounted secret.

I wouldn't want to have secretConfiguration only work for in-tree providers. That would be an odd discontinuity.

[mloiseleur]

I added specific extraVolumeMounts for sidecar with 9109597

[stevehipwell]

I still don't think we should be explicitly using secretConfiguration here, the secret COULD be mounted by the extraVolumeMounts but we should be recommending external secrets be used if needed.

[stevehipwell]

@mloiseleur sorry it's taken me a while to get back to this. I've replied to some existing comments and then added an inline comment on the values. I'd like to get the values spec defined before reviewing anything else.

[stevehipwell]

A webhook outside the pod is explicitly not supported. That would require external-dns to support authenticating to the webhook, otherwise attackers could make changes to DNS by making requests directly to the webhook.

@johngmyers the docs only recommend using a sidecar and I can think of a number of configurations which would allow the webhook to run externally while still being secure.

Docs:

The ideal setup for providers is to run as a sidecar in the same pod of the ExternalDNS container, listening only on localhost. This is not strictly a requirement, but we do not recommend other setups.

[johngmyers]

I would be in favor of modifying the docs to make a stronger statement.

Unless we add support for outbound authentication to the webhook provider, securing an externally-deployed provider would require something like a service mesh's network policy. It's complicated, likely to be left out, and deploying externally doesn't give any worthwhile advantages.

[mrueg]

Unless we add support for outbound authentication to the webhook provider, securing an externally-deployed provider would require something like a service mesh's network policy. It's complicated, likely to be left out, and deploying externally doesn't give any worthwhile advantages.

If we default to tight coupling, we could add support for unix sockets and use a shared mount between external-dns and the provider.
CC: @Raffo if you have any thoughts?

[Raffo]

I would be in favor of modifying the docs to make a stronger statement.

We can totally do that. I think it should be allow to go on the really hard path of running the webhook outside or the pod, but that should be strongly discouraged.

CC: @Raffo if you have any thoughts?

I don't think there's any need for now to support that. We only have to provide the right information IMO.

[mloiseleur]

I updated values (and code) following review comments.
I'm unsure what to do about secret.
I'll update the VALUES.md once we have an agreement on values spec.

[Raffo]

@stevehipwell do you think we should have the other PR merged first? If so can we speed that up? Or can we instead merge this one first? The reason I am asking is that I am starting to think of the next release and we still don't have the chart out for v0.14.0 and I'd love to get that fixed.

[stevehipwell]

@stevehipwell do you think we should have the other PR merged first?

@Raffo which PR is that?

[stevehipwell]

@mloiseleur I think we're down to two outstanding discussions; the provider.name value comment and the use of secretConfiguration within the webhook container.

CC @Raffo

[mloiseleur]

@mloiseleur I think we're down to two outstanding discussions; the provider.name value comment and the use of secretConfiguration within the webhook container.

CC @Raffo

I tried to fix the first point.
For the second point, I understood following previous comments that you would depreciate secret config in an other PR.
As soon as it's deprecated, I can then remove it in this PR.
Did I missed something ?

[stevehipwell]

I tried to fix the first point.
For the second point, I understood following #4032 (comment) that you would depreciate secret config in an other PR.
As soon as it's deprecated, I can then remove it in this PR.
Did I missed something ?

@mloiseleur I think we got out of sync on this; I was assuming we'd get this PR merged and then I'd do the deprecation and any other tidy up in a PR before we release.

[Raffo]

Can we get this merged now that the confusion has been clarified?

[stevehipwell]

@Raffo I think we can merge this once the value comment commit is on this branch. I'd prefer to also remove the secretConfiguration config from the webhook container on this branch so it never hits master but I could remove this afterwards as part of the deprecation if required.

[Raffo]

Cool. @mloiseleur are you OK to make the requested changes?

[mloiseleur]

I think we can merge this once the value comment commit is on this branch.

@stevehipwell : I'm sorry, it seems I missed that. What is this "value comment" that I should add and commit ?

On my side, I prefer to merge it with consistency on secretConfiguration, knowing that you will remove both afterwards.

[stevehipwell]

@stevehipwell : I'm sorry, it seems I missed that. What is this "value comment" that I should add and commit ?

@mloiseleur this was the commit you commented to me, and it looks like it's been applied to the branch.

I'm not comfortable merging something (webhook support for secretConfiguration) which is going to be removed before it's ever released and may lead to end-user confusion; so I've opened #4161 now to be merged before this PR.

[k8s-ci-robot]

PR needs rebase.

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/test-infra repository.

[mloiseleur]

@stevehipwell @Raffo This PR is ready for merge review.

[stevehipwell]

Thanks for all of your work on this @mloiseleur.

/approve
/label tide/merge-method-squash

[stevehipwell]

@Raffo there are non chart changes in the PR so I can't approve (I'll LGTM instead).

/lgtm

[stevehipwell]

@appkins once this is merged could you can rebase and squash #4073 so we can release the chart? Remember the CHANGELOG items need to be copied through to the chart annotations.

[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