feat: add support for multiple hosts and tls configurations in ingress #218
Conversation
Signed-off-by: Marcus Söderberg <[email protected]>
Signed-off-by: Marcus Söderberg <[email protected]>
|
|
This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution! |
|
Still waiting for review so please keep it open. |
| @@ -20,11 +20,16 @@ spec: | |||
| {{- if .Values.ingress.className }} | |||
| ingressClassName: {{ .Values.ingress.className | quote }} | |||
| {{- end }} | |||
| {{- if .Values.ingress.tls.enabled }} | |||
| {{- if or .Values.ingress.tls.enabled .Values.ingress.extraTls }} | |||
There was a problem hiding this comment.
Is there a reason we're adding the or condition here? I would have thought just the checking if .Values.ingress.tls.enabled is enabled should be enough. We have the extraTLS check below anyways and if you were adding extra TLS, you would already have the .tls.enabled set to true anyways.
There was a problem hiding this comment.
Having the or condition allows you to enable TLS only for extra hosts (e.g. with item(s) defined in ingress.extraTls and ingress.tls.enabled: false). I don't know if that's a use case worth supporting though, so I'm open to change it if that's preferable. If I change it I think it makes sense to move the extraTls object from ingress.extraTls to ingress.tls.extraTls. Let me know what you think!
There was a problem hiding this comment.
Yep, let's go with the ingress.extraTls to ingress.tls.extraTls, perhaps keeping the use case simple at the moment with the most likely scenario is best for now. If it's needed in future we can always change to add TLS only for extra hosts.
There was a problem hiding this comment.
Having the or condition allows you to enable TLS only for extra hosts (e.g. with item(s) defined in ingress.extraTls and ingress.tls.enabled: false). I don't know if that's a use case worth supporting though, so I'm open to change it if that's preferable. If I change it I think it makes sense to move the extraTls object from ingress.extraTls to ingress.tls.extraTls. Let me know what you think!
good catch!
Yep, let's go with the ingress.extraTls to ingress.tls.extraTls
isn't ingress.tls.extraTls a bit redundant? Looking at other charts ingress.extraTls seems the way to go, but happy to discuss.
There was a problem hiding this comment.
@vinzscam Yep, seems to have my values wrong way around there, good catch!
There was a problem hiding this comment.
Thanks for the feedback! I created a PR with the change (remove the or condition) to this branch just to make sure we're all on the same page: msoderberg#1.
Description of the change
Adds
ingress.extraHostsandingress.extraTlswhich allows you to add extrahostandtlsitems to the generated Ingress resource.Existing or Associated Issue(s)
Additional Information
We run Backstage in EKS and are currently adding a condition annotation in the Ingress to configure the AWS ALB with an extra hostname. The downside with this approach is that we have to specify the AWS ACM certificate ARN:s for the domains (we loose the certificate discovery functionality).
There are other workarounds as well (e.g. using
extraDeployto create multiple Ingress resources), but having native support for multiple hosts in the Helm chart would simplify our life a bit.Checklist
Chart.yamlaccording to semver.values.yamland added to the README.md. The helm-docs utility can be used to generate the necessary content. Usehelm-docs --dry-runto preview the content.ct lintcommand.