search

datalust / helm.datalust.co

Helm charts hosted on helm.datalust.co
Apache License 2.0
10 stars 16 forks source link

Add `ingressClassName` and segregate generated `Ingress` resources #24

Open pinkfloydx33 opened 2 years ago

pinkfloydx33 commented 2 years ago

Please consider adding support for spec.ingressClassName when creating Ingress resources. This field has replaced the deprecated ingress class annotations. There is currently no way to specify this on the generated resources and so must be managed manually. Please add first-class support for this feature.

Additionally, I'd ask that you consider splitting the UI/Ingestion ingresses into separate Ingress resources so that a different value for ingressClassName may be specified for each of them.

Why I need the latter: we run two different ingress controllers in our environment; one is for public traffic and the other is for internal/private network traffic. We expose the Seq UI publicly via the first controller (mainly to support AzureAD) and run the auxiliary ingestion endpoint over the private network only.

I must currently manage the Seq ingresses since I both need the ingressClassName feature and for it to differ between the two ingresses

KodrAus commented 2 years ago

Thanks for the suggestions @pinkfloydx33 :+1: So we should be replacing the kubernetes.io/ingress.class annotation with an ingressClassName on the spec itself? That sounds pretty straightforward. Splitting the ingresses also seems reasonable if it doesn't result in too much complexity to continue supporting the case where they're unified. If it does, then managing your own ingress to support your specific environment is probably the right thing to do.

If you (or anybody else) would like to take a stab at this PRs are most welcome, otherwise we'll take a look next time we do a round of work on the chart.

caiohasouza commented 2 years ago

Hi,

I created a PR with this feature (https://github.com/datalust/helm.datalust.co/pull/25), coudl your analyze/aprove please?

Regards

caiohasouza commented 2 years ago

@KodrAus

The PR #25 was approved and merged but i can't use because it don't released a new helm chart version, it's possible create a new version with this feature?

Regards

KodrAus commented 2 years ago

Ah, sorry @caiohasouza I missed the branch name in #25, we should have targeted main rather than a release branch. I'll get that sorted out. We're putting together an initial 2022 release shortly so can roll this into that. I'll let you know when it's all ready to go.

caiohasouza commented 2 years ago

Hi @KodrAus

Cool, do you have some estimate time to release new version? Sorry but i have a necessity to add the ingressClassName to be possible proceed with Kubernetes 1.22 upgrade.

Regards

KodrAus commented 2 years ago

Ah no problems, I'll get a release of the Helm chart together for you now.

caiohasouza commented 2 years ago

Hi @KodrAus

Perfect, thank you so much, i'm waiting.

Regards

KodrAus commented 2 years ago

Thanks for your patience on this one @caiohasouza. We had some trouble getting a build through on Friday we could release, but have one ready to go now. I'll drop another update here once it's all published.

KodrAus commented 2 years ago

Alrighty, we've just published 2022.1.7311-pre that includes these changes. You can use this version of the chart without having to use a preview version of Seq itself by setting the image.repository variable to a specific tag.

caiohasouza commented 2 years ago

Hi @KodrAus

Perfect, thank you so much!

Regards