Add SMTP configuration and environment support#129
Conversation
Updated copyright year from 2023 to 2026.
DavidePrincipi
left a comment
There was a problem hiding this comment.
Since discover-smarthost runs under multiple unit instances, separate temporary files are required to avoid conflicts when smarthost.env is written.
…nd add argument check
…r command argument
Co-authored-by: Davide Principi <davide.principi@nethesis.it>
…ument requirement
There was a problem hiding this comment.
Pull request overview
Adds smarthost/SMTP settings discovery to the webserver module so php-fpm containers can consume centrally-managed SMTP configuration, and updates the container/runtime setup accordingly (issue: NethServer/dev#7859).
Changes:
- Add
discover-smarthostscript to read smarthost settings from Redis and generate an SMTP environment file. - Update
phpfpm@.serviceto load the new env file and attempt to pass SMTP variables into the php-fpm container; add an event handler to restart php-fpm on smarthost changes. - Install
opensslin the PHP images and refactorbuild-images.shto reduce duplication.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| imageroot/systemd/user/phpfpm@.service | Loads optional smarthost env file, runs discovery pre-start, and attempts to forward SMTP env vars into the container. |
| imageroot/events/smarthost-changed/10restart_phpfpm | New handler to restart configured php-fpm instances when smarthost changes. |
| imageroot/bin/discover-smarthost | New script to generate smarthost.env from Redis smarthost settings. |
| container/Containerfile | Adds openssl to runtime packages. |
| build-images.sh | Refactors repeated image builds into a function. |
| README.md | Documents smarthost discovery behavior and environment propagation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| TimeoutStopSec=70 | ||
| ExecStartPre=/bin/rm -f %t/php%i-fpm.pid %t/php%i-fpm.ctr-id | ||
| ExecStartPre=/usr/bin/mkdir -p %S/state/php%i-fpm-custom.d | ||
| ExecStartPre=-runagent discover-smarthost |
There was a problem hiding this comment.
ExecStartPre=-runagent discover-smarthost uses runagent without an absolute path, while other units (e.g. nginx.service) use /usr/local/bin/runagent. In systemd units the PATH can be minimal, so this may fail with “runagent: not found”; use the same absolute path consistently.
| ExecStartPre=-runagent discover-smarthost | |
| ExecStartPre=-/usr/local/bin/runagent discover-smarthost |
| --pod-id-file %t/webserver.pod-id --replace -d --name php%i-fpm \ | ||
| --volume websites:/var/www/html:z \ | ||
| --volume %S/state/php%i-fpm-custom.d:/usr/local/etc/php-fpm.d:z \ | ||
| --env SMTP_* \ |
There was a problem hiding this comment.
podman run --env SMTP_* will pass a literal variable name SMTP_* (systemd does not do shell glob expansion for ExecStart arguments, and * is not a valid env var name). As a result, SMTP variables won’t be propagated into the container. Prefer --env-file pointing to the generated env file, or explicitly list each SMTP_... variable to forward.
| --env SMTP_* \ | |
| --env-file %S/state/smarthosts/smarthost.env \ |
| ListConfigurations = glob.glob(folder+'/dyn-*.conf') | ||
| if ListConfigurations : | ||
| subprocess.run(["download-php-fpm",ConfiguredServices[0].replace('php','')]) | ||
| subprocess.run(["systemctl", "--user", "restart", "phpfpm@"+ConfiguredServices[0].replace('php','')+".service"]) |
There was a problem hiding this comment.
This handler assumes re.findall(...) always returns at least one match and then indexes ConfiguredServices[0]; if the folder name format ever differs, this will raise IndexError. Also, subprocess.run(...) results are ignored, so a failed download/restart won’t be detected. Add a guard for empty matches and run subprocesses with error handling (e.g. check=True or inspecting returncode).
| ListConfigurations = glob.glob(folder+'/dyn-*.conf') | |
| if ListConfigurations : | |
| subprocess.run(["download-php-fpm",ConfiguredServices[0].replace('php','')]) | |
| subprocess.run(["systemctl", "--user", "restart", "phpfpm@"+ConfiguredServices[0].replace('php','')+".service"]) | |
| if not ConfiguredServices: | |
| # Skip folders that do not match the expected php version pattern | |
| continue | |
| php_version = ConfiguredServices[0].replace('php', '') | |
| ListConfigurations = glob.glob(folder + '/dyn-*.conf') | |
| if ListConfigurations: | |
| try: | |
| subprocess.run(["download-php-fpm", php_version], check=True) | |
| subprocess.run( | |
| ["systemctl", "--user", "restart", f"phpfpm@{php_version}.service"], | |
| check=True, | |
| ) | |
| except subprocess.CalledProcessError as e: | |
| print(f"Error handling php-fpm service for {php_version}: {e}", flush=True) |
| `phpfpm@.service` starts, the command `bin/discover-smarthost` runs and refreshes | ||
| the `state/smarthost.env` file with fresh values from Redis. | ||
|
|
||
| Furthermore if smarthost setup is changed when webserver is already | ||
| running, the event handler `events/smarthost-changed/10reload_services` |
There was a problem hiding this comment.
Documentation doesn’t match the implementation: the unit references state/smarthosts/smarthost.env (and discover-smarthost writes under smarthosts/), but the README mentions state/smarthost.env. Also, the documented handler name events/smarthost-changed/10reload_services doesn’t exist in this PR (added file is 10restart_phpfpm). Update the paths/names to reflect the actual files.
| - `state/smarthost.env` - SMTP and smarthost configuration (optional, prefixed with `SMTP_`) | ||
|
|
||
| These environment variables are automatically passed to the corresponding PHP-FPM container instance, allowing configuration to be centrally managed through environment files that apply to all PHP versions (8.5, 8.4, 8.3, etc.). | ||
| This setting discovery is just an example to understand how the module is expected to work: | ||
| ``` | ||
| cat smarthost.env |
There was a problem hiding this comment.
This list item and the example cat smarthost.env command don’t reflect where the file is actually created/loaded (the unit uses state/smarthosts/smarthost.env). Update the bullet and example command/path to match the real location.
| EnvironmentFile=%S/state/environment | ||
| EnvironmentFile=%S/state/image_url_tag.env | ||
| WorkingDirectory=%S/state | ||
| EnvironmentFile=-%S/state/smarthosts/smarthost.env |
There was a problem hiding this comment.
EnvironmentFile is evaluated before any ExecStartPre commands run, so the file generated by ExecStartPre=...discover-smarthost will not be loaded into the unit environment for this start. To ensure fresh values are used, either pass the generated file directly to Podman (e.g., via --env-file), or generate/update the env file in a separate unit that runs before phpfpm@.service starts.
Introduce a script to discover SMTP settings and create an environment file for smarthost configuration. Update the container setup to include OpenSSL and ensure the service can access the new SMTP environment variables.
NethServer/dev#7859