Helm chart support for webhook sidecar #3966

johngmyers · 2023-09-30T22:53:47Z

Description

Adds support to the Helm chart for running a webhook provider as a sidecar

Checklist

Unit tests updated
End user documentation updated

k8s-ci-robot · 2023-09-30T22:54:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from johngmyers. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

charts/OWNERS

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

johngmyers · 2023-09-30T22:54:32Z

/cc @Raffo

charts/external-dns/templates/deployment.yaml

+          securityContext:
+            {{- toYaml . | nindent 12 }}
+          {{- end }}
+          image: {{ .Values.webhook.image . }}


mloiseleur · 2023-10-03T06:53:04Z

charts/external-dns/templates/deployment.yaml

+            {{- toYaml .Values.webhook.livenessProbe | nindent 12 }}
+          readinessProbe:
+            {{- toYaml .Values.webhook.readinessProbe | nindent 12 }}
+          {{- if or .Values.webhook.secretConfiguration.enabled .Values.webhook.extraVolumeMounts }}


.Values.webhook.secretConfiguration.enabled is not in the values.yaml

Changed to use mountPath instead of enabled, to match the external-dns container and the condition two lines below.

This does mean that setting either mountPath without also setting enabled will result in mounting a nonexistent volume. We could make the condition for the volume definition be an or of the two mountPath, leaving enabled to just control creating the Secret.

mloiseleur · 2023-10-03T06:59:27Z

charts/external-dns/templates/deployment.yaml

+            {{- toYaml . | nindent 12 }}
+          {{- end }}
+        {{- end }}
+      {{- if or .Values.secretConfiguration.enabled .Values.webhook.secretConfiguration.enabled .Values.extraVolumes }}
      volumes:
        {{- if .Values.secretConfiguration.enabled }}


Suggested change

{{- if .Values.secretConfiguration.enabled }}

{{- if or .Values.secretConfiguration.enabled .Values.webhook.secretConfiguration.enabled }}

I had decided against having a .Values.webhook.secretConfiguration.enabled. The .Values.secretConfiguration.enabled setting defines the creation of the Secret, whereas each of the containers has its own mountPath setting. It is most likely that any secret will only be needed by the webhook (to authenticate to the provider's API).

mloiseleur · 2023-10-03T07:06:44Z

@johngmyers With my 3 suggested changes and this values files:

webhook:
  image: "myhook:v1.2"
  args: ["-c","-v"]
  env:
    - name: TEST
      value: "TESTVAL"
  extraVolumeMounts: []
  resources:
    requests:
      cpu: 10m
      memory: 50Mi

  secretConfiguration:
    enabled: true
    mountPath: "/etc/mysecret"

It produced the expected output, with secret mounted on sidecar and not on main container.

stevehipwell

I think this PR works in general but I don't think using .Values.webhook.image as the flag to enable this behaviour is a good API. Initially I was thinking that there should be .Values.webhook.enabled but this still leave the behaviour of .Values.provider as ambiguous.

I think that this PR should use eq .Values.provider "webhook" as the flag for this behaviour as it's consistent with the current API.

johngmyers · 2023-10-03T15:19:09Z

@stevehipwell to enable the webhook provider you need to provide an image and there's no purpose to providing an image except to enable the webhook provider. So providing an image is equivalent to enabling the webhook provider.

So a .Values.webhook.enabled would be extraneous, serving no purpose other than to allow users to misconfigure the chart. Similarly, using eq .Values.provider "webhook" serves no purpose other than to allow users to misconfigure the chart.

mloiseleur · 2023-10-04T07:19:26Z

@johngmyers I agree with @stevehipwell. The code would be more clear and consistent using eq .Values.provider "webhook".

For misconfiguration, it's possible to use fail when user has set webhook provider without a webhook image. See for instance how it's done on this HPA template when it's been enabled without the required maxReplicas.

As an alternative, there is also the required function.

johngmyers · 2023-10-04T14:15:30Z

Keying off of eq .Values.provider "webhook" allows for the error where the user sets the webhook image but doesn't set provider: "webhook". In that case they will unexpectedly get the aws provider despite their stated intention to use a webhook image.

Keying off of eq .Values.provider "webhook" requires the user to configure two settings in order to enable a webhook image, whereas keying off of .Values.webhook.image only requires them to configure one. Keying off of .Values.provider confers disadvantage to the user without any compensating advantage.

mloiseleur · 2023-10-05T07:00:29Z

Following the use of webhook.secretConfiguration.mountPath, with this values:

secretConfiguration:
  data:
    credentials: |
      [default]
      my_secret_access = $MY_SECRET_KEY

webhook:
  image: "myhook:v1.2"
  args: ["-c","-v"]
  env:
    - name: TEST
      value: "TRUC"
  resources:
    requests:
      cpu: 10m
      memory: 50Mi

  secretConfiguration:
    mountPath: "/etc/mysecret"

Generated yaml is missing the annotation checksum/secret on Deployment, allowing to rollout when the secret content is updated.

johngmyers · 2023-10-09T00:02:31Z

If secretConfiguration.enabled isn't true, then the chart doesn't render a Secret and secretConfiguration.data isn't referenced. In that case, the Secret would have to be deployed external to the chart and the chart wouldn't have a way to generate a useful checksum.

I do think it would be useful for the chart to support SecretProviderClass, but that's a separate PR.

mloiseleur · 2023-10-09T07:14:13Z

Yes, that's the issue. And even if the user provides it externally, it won't be smooth : one will have to restart manually the pod each time the secret is updated.

With the current state of the PR, Secret (really) works:

on external-dns when secretConfiguration.enabled, secretConfiguration.data, secretConfiguration.mountPath are set
on webhook when secretConfiguration.enabled, secretConfiguration.data and webhook.secretConfiguration.mountPath are set.

Following this idea to enable it with mountPath, wdyt about also remove secretConfiguration.enabled and enforce with fail or required that user should set both secretConfiguration.data and ( secretConfiguration.mountPath or webhook.secretConfiguration.mountPath) ?

stevehipwell · 2023-10-09T20:42:42Z

@johngmyers the primary behaviour for this chart is controlled by .Values.provider so having the provider be modified by a different value is not a consistent API. If you want to configure a webhook you need to set provider to webhook; if you haven't set an image this will fail and that's fine and a consistent API as there are other providers which need additional inputs.

johngmyers · 2023-10-12T05:47:56Z

@mloiseleur I agree that there's no benefit to having secretConfiguration.enabled. I'm unsure as to whether that should be part of this PR or whether it's out of scope and thus for a separate PR.

A theoretical feature to support SecretesProviderClass wouldn't be able to update an annotation either. A provider could be written to poll the contents of the mounted secret. I don't think it's necessary to ban configurations that aren't able to automatically rolling-update the deployment if the secret changes.

johngmyers · 2023-11-06T18:48:35Z

We do not have agreement with this. I strongly disagree with @stevehipwell 's stance on this. The stance is not even logical: the entire point of the webhook provider is to change how providers are specifed, so how built-in providers used to be specified is not relevant to how external providers should be specified.

Users configuring an external provider should not be surprised by external-dns using the AWS provider because they failed to specify something that the chart could easily have figured out itself.

stevehipwell · 2023-11-06T21:17:40Z

@Raffo there is no consensus between @johngmyers and myself. I stand by my review and am very much opposed to breaking the existing chart API; I can't see any benefit while I can see a significant number of downsides. I'm happy to provide a PR to add webhook support within the existing chart API; we can support single container, sidecar and external service patterns.

johngmyers · 2023-11-06T22:16:47Z

This change does not break the existing chart API. All preexisting use cases continue to work with no change to config.

From a user experience perspective, this change has all benefit and no downside. There is no reason or benefit for a user of a sidecar provider to configure the provider setting. A user specifying webhook.image would not want to run the AWS route53 (or any other built-in provider) and doing so would be punishing them for no good reason.

The argument against this change is nonsensical. That it changes a previous configuration model that did not allow for the feature being added is unavoidable.

florianmutter · 2023-11-13T10:56:22Z

I think it would be confusing and inconsistent if the chart works different to what is described here: https://github.com/kubernetes-sigs/external-dns/blob/master/docs/tutorials/webhook-provider.md. The documentation says there is a new provider webhook and it says you should use --provider=webhook and I think it should be the same for the chart.

johngmyers · 2023-11-14T06:42:49Z

Moved all the settings under provider and used @mloiseleur's template to provide backwards compatibility to existing string provider values.

mloiseleur · 2023-11-14T07:31:44Z

charts/external-dns/templates/deployment.yaml

+          ports:
+            - name: provider-metrics
+              protocol: TCP
+              containerPort: 8080


Is this port expected on all webhook providers ?
FTM, I have seen they are using one port (8888) by default, to serve provider methods and probes.

That is my proposal. We'll want them to have a separate port for metrics because security depends on the provider methods not being accessible outside of the pod. I don't see any benefit in letting webhook providers choose different metrics ports.

charts/external-dns/templates/deployment.yaml

stevehipwell · 2023-11-20T21:23:44Z

@johngmyers @mloiseleur what is the situation with this PR and #4032?

johngmyers · 2023-11-21T07:25:51Z

The situation between this and #4032 is that we have not yet converged.

johngmyers · 2023-11-21T08:14:12Z

charts/external-dns/templates/servicemonitor.yaml

@@ -22,32 +22,34 @@ spec:
    matchLabels:
      {{- include "external-dns.selectorLabels" . | nindent 6 }}
  endpoints:
-    - port: http
+    {{- range (list "http" "provider-metrics" }}


I need to make the "provider-metrics" part of this conditional on there being a provider sidecar.

k8s-ci-robot · 2023-12-07T23:40:44Z

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.

k8s-ci-robot · 2024-01-03T18:16:08Z

@johngmyers: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-external-dns-licensecheck	`d179660`	link	true	`/test pull-external-dns-licensecheck`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

mloiseleur · 2024-02-11T10:13:43Z

Since #4032 and #4085 are merged, I close this PR.
Feel free to reopen one if we missed something.
/close

k8s-ci-robot · 2024-02-11T10:13:48Z

@mloiseleur: Closed this PR.

In response to this:

Since #4032 and #4085 are merged, I close this PR.
Feel free to reopen one if we missed something.
/close

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.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 30, 2023

k8s-ci-robot requested review from stevehipwell and szuecs September 30, 2023 22:54

k8s-ci-robot requested a review from Raffo September 30, 2023 22:54

johngmyers force-pushed the webhook-chart branch 3 times, most recently from 427869c to 51e3b34 Compare October 1, 2023 18:03

mloiseleur reviewed Oct 2, 2023

View reviewed changes

charts/external-dns/templates/deployment.yaml Show resolved Hide resolved

mloiseleur reviewed Oct 2, 2023

View reviewed changes

charts/external-dns/templates/deployment.yaml Show resolved Hide resolved

mloiseleur reviewed Oct 3, 2023

View reviewed changes

charts/external-dns/templates/deployment.yaml Outdated

securityContext:

{{- toYaml . | nindent 12 }}

{{- end }}

image: {{ .Values.webhook.image . }}

This comment was marked as resolved.

Sign in to view

mloiseleur reviewed Oct 3, 2023

View reviewed changes

stevehipwell suggested changes Oct 3, 2023

View reviewed changes

k8s-ci-robot assigned stevehipwell Oct 3, 2023

johngmyers changed the title ~~WIP Helm chart support for webhook sidecar~~ Helm chart support for webhook sidecar Oct 4, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 4, 2023

johngmyers force-pushed the webhook-chart branch from a772c35 to badc6c3 Compare October 4, 2023 15:54

johngmyers force-pushed the webhook-chart branch from d5b5948 to 007d2e4 Compare November 6, 2023 18:58

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2023

Raffo mentioned this pull request Nov 8, 2023

Release chart for v0.14.0 #4025

Closed

mloiseleur mentioned this pull request Nov 10, 2023

feat: support webhook provider in Chart #4032

Merged

1 task

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2023

johngmyers force-pushed the webhook-chart branch from d4adb0c to b61bb65 Compare November 14, 2023 06:13

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2023

johngmyers force-pushed the webhook-chart branch 3 times, most recently from dac9319 to 90b69ea Compare November 14, 2023 06:18

Helm chart support for provider images

cc1c5f5

johngmyers force-pushed the webhook-chart branch from 90b69ea to cc1c5f5 Compare November 14, 2023 06:29

mloiseleur reviewed Nov 14, 2023

View reviewed changes

mrueg reviewed Nov 14, 2023

View reviewed changes

charts/external-dns/templates/deployment.yaml Show resolved Hide resolved

Also scrape metrics from provider-metrics port

d179660

johngmyers commented Nov 21, 2023

View reviewed changes

stevehipwell mentioned this pull request Dec 1, 2023

feat(chart): Added complex provider support #4085

Merged

2 tasks

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2023

k8s-ci-robot closed this Feb 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Helm chart support for webhook sidecar #3966

Helm chart support for webhook sidecar #3966

johngmyers commented Sep 30, 2023

k8s-ci-robot commented Sep 30, 2023

johngmyers commented Sep 30, 2023

This comment was marked as resolved.

mloiseleur Oct 3, 2023

johngmyers Oct 4, 2023

mloiseleur Oct 3, 2023

johngmyers Oct 4, 2023

mloiseleur commented Oct 3, 2023

stevehipwell left a comment

johngmyers commented Oct 3, 2023

mloiseleur commented Oct 4, 2023

johngmyers commented Oct 4, 2023

mloiseleur commented Oct 5, 2023 •

edited

Loading

johngmyers commented Oct 9, 2023

mloiseleur commented Oct 9, 2023

stevehipwell commented Oct 9, 2023

johngmyers commented Oct 12, 2023 •

edited

Loading

johngmyers commented Nov 6, 2023

stevehipwell commented Nov 6, 2023

johngmyers commented Nov 6, 2023

florianmutter commented Nov 13, 2023

johngmyers commented Nov 14, 2023

mloiseleur Nov 14, 2023 •

edited

Loading

johngmyers Nov 15, 2023

stevehipwell commented Nov 20, 2023

johngmyers commented Nov 21, 2023

johngmyers Nov 21, 2023

k8s-ci-robot commented Dec 7, 2023

k8s-ci-robot commented Jan 3, 2024

mloiseleur commented Feb 11, 2024

k8s-ci-robot commented Feb 11, 2024

	{{- if .Values.secretConfiguration.enabled }}
	{{- if or .Values.secretConfiguration.enabled .Values.webhook.secretConfiguration.enabled }}

Helm chart support for webhook sidecar #3966

Helm chart support for webhook sidecar #3966

Conversation

johngmyers commented Sep 30, 2023

k8s-ci-robot commented Sep 30, 2023

johngmyers commented Sep 30, 2023

This comment was marked as resolved.

mloiseleur Oct 3, 2023

Choose a reason for hiding this comment

johngmyers Oct 4, 2023

Choose a reason for hiding this comment

mloiseleur Oct 3, 2023

Choose a reason for hiding this comment

johngmyers Oct 4, 2023

Choose a reason for hiding this comment

mloiseleur commented Oct 3, 2023

stevehipwell left a comment

Choose a reason for hiding this comment

johngmyers commented Oct 3, 2023

mloiseleur commented Oct 4, 2023

johngmyers commented Oct 4, 2023

mloiseleur commented Oct 5, 2023 • edited Loading

johngmyers commented Oct 9, 2023

mloiseleur commented Oct 9, 2023

stevehipwell commented Oct 9, 2023

johngmyers commented Oct 12, 2023 • edited Loading

johngmyers commented Nov 6, 2023

stevehipwell commented Nov 6, 2023

johngmyers commented Nov 6, 2023

florianmutter commented Nov 13, 2023

johngmyers commented Nov 14, 2023

mloiseleur Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

johngmyers Nov 15, 2023

Choose a reason for hiding this comment

stevehipwell commented Nov 20, 2023

johngmyers commented Nov 21, 2023

johngmyers Nov 21, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 7, 2023

k8s-ci-robot commented Jan 3, 2024

mloiseleur commented Feb 11, 2024

k8s-ci-robot commented Feb 11, 2024

mloiseleur commented Oct 5, 2023 •

edited

Loading

johngmyers commented Oct 12, 2023 •

edited

Loading

mloiseleur Nov 14, 2023 •

edited

Loading