-
Notifications
You must be signed in to change notification settings - Fork 146
Improvements with hostPort and Proxy Protocol #421
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves the Helm chart configuration for Mailu by addressing three key issues: missing hostPort exposure for ManageSieve, unnecessary coupling between Proxy Protocol and external services, and incorrect validation logic for Proxy Protocol configuration.
Key changes:
- Added hostPort exposure for ManageSieve (port 4190) when hostPort is enabled
- Moved proxy protocol and TLS configuration from
ingress.*tofront.*namespace - Fixed proxy protocol validation to allow empty
realIpHeaderwhen using Proxy Protocol
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| utils/check_env_vars.py | Updated TLS_FLAVOR environment variable mapping from ingress to front namespace |
| charts/mailu/values.yaml | Moved proxy protocol, realIpHeader, realIpFrom, and tlsFlavorOverride settings from ingress to front section |
| charts/mailu/templates/front/deployment.yaml | Added hostPort 4190 for ManageSieve when hostPort is enabled and fixed port formatting |
| charts/mailu/templates/envvars-configmap.yaml | Updated environment variable references to use front namespace instead of ingress |
| charts/mailu/templates/_services.tpl | Removed dependency on externalService for proxy protocol configuration and updated validation logic |
| charts/mailu/templates/_helpers.tpl | Updated TLS flavor helper to reference front namespace and fixed deprecated value detection |
| charts/mailu/README.md | Updated documentation to reflect the configuration namespace changes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| enabled: true | ||
|
|
||
| ## @param front.realIpHeader Sets the value of `REAL_IP_HEADER` environment variable in the `front` pod | ||
| realIpHeader: "" |
Copilot
AI
Aug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on line 665 states 'Enabling any of these requires to have ingress.realIpFrom set' but should be updated to reference 'front.realIpFrom' since the configuration has been moved to the front namespace.
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
Bump |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 14 days. |



When using hostPort, the ManageSieve port was not actually exposed — I believe this was simply forgotten in the template.
The second issue is that Proxy Protocol and publicly exposed ports do not necessarily require a Service. When using hostPort and deploying via DaemonSet, front.externalService can be disabled. I had to edit _helpers.tpl to account for this. The advantage of not using an externalService with an upstream Ingress Controller is that it avoids an unnecessary proxy layer.
The third issue occurs when using Proxy Protocol: by default, the chart outputs an error stating that ingress.realIpHeader should not have any value. However, with Proxy Protocol (which is likely the most common case when proxying TCP traffic), this field must indeed be left empty.
Additionally, I moved values that did not belong under ingress. to front..
These are breaking changes.