Add Kubernetes ingress support#11
Conversation
| kind: Ingress | ||
| metadata: | ||
| {{- with .Values.n8n.ingress.annotations }} | ||
| annotations: |
There was a problem hiding this comment.
For the websockets to work reliably, I recommend adding these annotations
There was a problem hiding this comment.
Hi @netroy,
These can be passed in values.yaml file.
My idea was to keep the templates clean, and everyone can provide their own annotations.
For example, I tested this Helm chart in the home lab with the Traefik ingress controller, and I used this:
n8n:
ingress:
enabled: true
className: traefik
annotations:
traefik.ingress.kubernetes.io/router.entrypoints: websecure
traefik.ingress.kubernetes.io/router.tls: "true"
traefik.ingress.kubernetes.io/router.tls.certresolver: le
hosts:
- host: n8n.balutoiu.com
paths:
- path: /
pathType: Prefix
webhook:
ingress:
enabled: true
className: traefik
annotations:
traefik.ingress.kubernetes.io/router.entrypoints: websecure
traefik.ingress.kubernetes.io/router.tls: "true"
traefik.ingress.kubernetes.io/router.tls.certresolver: le
hosts:
- host: n8n-webhook.balutoiu.com
paths:
- path: /
pathType: PrefixAnd it worked fine.
However, I believe it would be appropriate to have these annotations in the values.yaml embedded with the helm chart. And, other people can override them if they don't use Nginx ingress controller.
Also, I see that the ingress resource from here is shared between the main service and the webhook service. That's a great idea!
I believe we can have the templates/ingress.yaml from here, and my only improvement would be keeping the template file generic, and adding values for ingress annotations / className (these depend on the ingress controller deployed in the Kubernetes cluster).
What do you think?
There was a problem hiding this comment.
Since the ingress class specific configuration is important working on n8n. could we instead wrap the annotations in checks like this?
{{- if eq .Values.n8n.ingress.className "nginx" }}That way the chart includes the best practices in the templates, and the values file could be much smaller.
There was a problem hiding this comment.
Since the ingress class specific configuration is important working on n8n. could we instead wrap the annotations in checks like this?
{{- if eq .Values.n8n.ingress.className "nginx" }}That way the chart includes the best practices in the templates, and the values file could be much smaller.
I agree! Making the changes to update this PR.
There was a problem hiding this comment.
Thanks ❤️
also, can you please move the ingress to a separate file(s)?
one file for each ingress, or a combined file, that's your decision.
There was a problem hiding this comment.
Thanks ❤️ also, can you please move the ingress to a separate file(s)? one file for each ingress, or a combined file, that's your decision.
Please see the updated PR.
I like the combined ingress more because we can re-use the same DNS from .Values.n8n.domainName for both test / production webhooks.
Also, there's still a enabled: false flag on the ingress, since the ingress controller is not deployed all the time with the Kubernetes clusters.
There was a problem hiding this comment.
This is how my homelab n8n with traefik ingress looks now:
ingress:
enabled: true
className: traefik
annotations:
traefik.ingress.kubernetes.io/router.entrypoints: websecure
traefik.ingress.kubernetes.io/router.tls: "true"
traefik.ingress.kubernetes.io/router.tls.certresolver: levs before:
n8n:
ingress:
enabled: true
className: traefik
annotations:
traefik.ingress.kubernetes.io/router.entrypoints: websecure
traefik.ingress.kubernetes.io/router.tls: "true"
traefik.ingress.kubernetes.io/router.tls.certresolver: le
hosts:
- host: n8n.balutoiu.com
paths:
- path: /
pathType: Prefix
webhook:
ingress:
enabled: true
className: traefik
annotations:
traefik.ingress.kubernetes.io/router.entrypoints: websecure
traefik.ingress.kubernetes.io/router.tls: "true"
traefik.ingress.kubernetes.io/router.tls.certresolver: le
hosts:
- host: n8n-webhook.balutoiu.com
paths:
- path: /
pathType: PrefixIn my opinion, it is a lot cleaner with a combined ingress resource.
There was a problem hiding this comment.
Do you think it makes sense to add traefik specific entries in the values file, and move the annotations in the template?
so if ingress.traefik.tls is true the add the router.entrypoints and and router.tls annotations, and unless ingress.traefik.tls.certresolver is set to another value, that annotation could also default to let's encrypt.
I'm assuming that this is a fairly common setup, and it should be okay to support this in the templates just like with nginx specific annotations.
That said, I'm also fine leaving it this way.
Since we don't have a CI setup for this yet. I'd like to test this manually later just to be sure, and then we can merge it.
Thanks 💟
There was a problem hiding this comment.
Do you think it makes sense to add traefik specific entries in the values file, and move the annotations in the template? so if
ingress.traefik.tlsis true the add therouter.entrypointsand androuter.tlsannotations, and unlessingress.traefik.tls.certresolveris set to another value, that annotation could also default to let's encrypt.I'm assuming that this is a fairly common setup, and it should be okay to support this in the templates just like with nginx specific annotations.
We could do this, but I believe it's cleaner the way it is this way now. Because these traefik annotations are specific to my homelab setup, and another person with Treaefik might have different annotations (that's why I wouldn't include them in templates).
However, I would still have these traefik annotations commented in the values.yaml file, for reference, like this:
ingress:
enabled: false
className: nginx
annotations: {}
#
# Traefik ingress example
#
# className: traefik
# annotations:
# traefik.ingress.kubernetes.io/router.entrypoints: websecure
# traefik.ingress.kubernetes.io/router.tls: "true"
# traefik.ingress.kubernetes.io/router.tls.certresolver: le
This way, other people can use them as inspiration when configuring Traefik ingress with n8n.
That said, I'm also fine leaving it this way. Since we don't have a CI setup for this yet. I'd like to test this manually later just to be sure, and then we can merge it.
Thanks 💟
No problem. Thank-you for the review!
7a078bd to
1344232
Compare
Allow optional configuration of ingress Kubernetes resource for the main n8n service, and the webhook service. Signed-off-by: Ionut Balutoiu <ionut@balutoiu.com>
1344232 to
3481639
Compare
|
@netroy keep in mind when making changes to this repo that it's used as the basis for a lot of our server setup guides: https://docs.n8n.io/hosting/installation/server-setups/ Changes here may necessitate changes to those guides. |
Allow optional configuration of ingress Kubernetes resource for the main n8n service, and the webhook service.
Signed-off-by: Ionut Balutoiu ionut@balutoiu.com