Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Added support for NodePort port numbers configuration #286

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bidord
Copy link
Contributor

@bidord bidord commented Aug 13, 2023

Fixes #28
Relates to fastlorenzo#25

This PR:

  • adds the ability to customize the nodePort of each mail service when front.externalService.type=NodePort
  • changes the default nodePort numbers to values within the default range 30000-32767

As I commented on #268 (comment), using NodePort numbers < 1024 requires configuring the cluster in a way that is not allowed by many providers.

I think that admins that want to expose mail services on the default port directly on their Kubernetes nodes should use front.hostPort.enabled=true instead. NodePort services should be preferred when using an external load balancer.

My personal Mailu instance is a "2 nodes cluster + NodePort service + haproxy" setup (for now, I have disabled externalService in Helm and added my own Service resource). It took me some digging to configure all of this properly. I started reorganizing my personal notes into a piece of documentation : https://github.com/bidord/mailu-helm-charts/blob/feat-add-documentation-for-nodeport/mailu/README.md#running-behind-an-external-load-balancer-with-a-nodeport-service Tell me if you are interested in me adding it to this PR.

@fastlorenzo
Copy link
Collaborator

@bidord thanks for the PR, this looks good! Might be interesting that you add as well the content in the README (could be useful to others as well)

@fastlorenzo
Copy link
Collaborator

@bidord could you rebase your code to master? There are some changes in the values files that blocks this merge
Thanks!

@fastlorenzo fastlorenzo self-requested a review June 28, 2024 21:19
@fastlorenzo fastlorenzo added the enhancement New feature or request label Jun 28, 2024
@bidord bidord marked this pull request as draft June 29, 2024 08:59
@bidord bidord force-pushed the feat-add-support-for-nodeport branch 2 times, most recently from 402f1db to d4c0782 Compare June 29, 2024 09:35
@bidord
Copy link
Contributor Author

bidord commented Jun 29, 2024

@fastlorenzo I just rebased in order to add front.externalService.nodePorts.manageSieve.

I also wanted to modify the documentation part to make it work with latest changes. But I reckon this is still a work in progress? (In master branch, helper function mailu.proxyProtocolPorts is defined but not used anywhere yet.) So for now, I just removed the documentation part from this PR.

Additionally: I think port 465 should be called submissions in values files instead of smtps. This would match better RFC8314 and autoconfig SRV records as provided by Mailu.

@bidord bidord marked this pull request as ready for review June 29, 2024 10:13
@DrPsychick
Copy link

@fastlorenzo this also looks good, can we get it merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pending answer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace front daemonset with hostport by a front deployment with a service
3 participants