8gears / n8n-helm-chart

A Kubernetes Helm chart for n8n a Workflow Automation Tool. Easily automate tasks across different services.
https://artifacthub.io/packages/helm/open-8gears/n8n
Apache License 2.0
187 stars 97 forks source link

"webhook.enabled" means "webhook disabled" #39

Closed zmatthias closed 1 year ago

zmatthias commented 1 year ago

Hello,

in line 264 values.yaml: https://github.com/8gears/n8n-helm-chart/blob/master/values.yaml .Values.scaling.webhook.enabed is set to false:

 webhook:
    enabled: false
    count: 1

However, in line 95-97 _helpers.tpl: https://github.com/8gears/n8n-helm-chart/blob/master/templates/_helpers.tpl the environment variable N8N_DISABLE_PRODUCTION_MAIN_PROCESS is set to true, if .Values.scaling.webhook.enabled is set to true:

{{- if .Values.scaling.webhook.enabled }}
- name: "N8N_DISABLE_PRODUCTION_MAIN_PROCESS"
  value: "true"
{{ end }}

Description of N8N_DISABLE_PRODUCTION_MAIN_PROCESS:

Disable production webhooks from main process. This helps ensure no HTTP traffic load to main process when using webhook-specific processes.

so seemlingly "enabling" webhooks actually disables them for production use. Debugging this caused us hours of headache.

Vad1mo commented 1 year ago

hmm, I see, what would you suggest solving the situation, and save many people a lot of debug time?

zmatthias commented 1 year ago

For example: rename .Values.scaling.webhook.enabed to .Values.scaling.webhook.disabled in values.yaml and in _helpers.tpl


By the way, test webhooks are not affected by this setting. Only production webhooks are. So a more descriptive name might be: .Values.scaling.productionWebhook.disabled. But that's more a matter of taste.

Vad1mo commented 1 year ago

This has a few flaws. Every service has the notation service.enabled. This is used here and is practice in other charts. It is also advised to do the same in general programming

It is also not backwards compatible

Any other ideas?

swarnat commented 1 year ago

This is only 50% a view of this topic.

Because when you set .Values.scaling.webhook.enabled you disable Webhooks from Main process. So you are true, this disables the processing on this instance. But you enable the processing on a different Webhook instance, which is described on this section of docs: https://docs.n8n.io/hosting/scaling/queue-mode/#webhook-processors and deployed here: https://github.com/8gears/n8n-helm-chart/blob/master/templates/deployment.webhooks.yaml

This is completely optional, so you don't need to activate this scaling feature and process the webhooks on main instance. But in this moment misses > 1 instances in this moment. In my eyes, the "enabled" attribute is correct. Maybe the "webhook" name can be changed to something like "webbhook_distribution".

zmatthias commented 1 year ago

Alright, I see. Then I think a simple comment in values.yaml would be sufficient and shouldn't do any harm :)

Edit: Sorry, I didn't intend to close the issue.

Vad1mo commented 1 year ago

thanks for the positive discussion and resolution. I updated the values file referencing this discussion and @swarnat explanation and impact. https://github.com/8gears/n8n-helm-chart/blob/master/values.yaml#L262