Skip to content

fix(chart): remove unused volume mounts and DB secret from nginx container - #170

Open
danielqb wants to merge 1 commit into
eftechcombr:mainfrom
danielqb:fix/nginx-dead-volume-mounts
Open

fix(chart): remove unused volume mounts and DB secret from nginx container#170
danielqb wants to merge 1 commit into
eftechcombr:mainfrom
danielqb:fix/nginx-dead-volume-mounts

Conversation

@danielqb

Copy link
Copy Markdown
Contributor

Problem

The nginx container in glpi-deployment.yaml mounted the files, marketplace,
and etc PersistentVolumeClaims (ReadWriteOnce) even though nginx's actual
config (default.conf) never reads from those paths - it only serves static
assets from /var/www/html/public (baked into the image) and proxies
everything else to php-fpm via fastcgi_pass. It also pulled glpi-secret
(DB credentials) via envFrom despite never connecting to the database.

Since both nginx and php-fpm mounted the same RWO PVCs, a cluster without
explicit pod affinity forcing them onto the same node could schedule them
on different nodes, causing FailedMount/ContainerCreating for whichever
pod lands second.

Fix

Removed the files/marketplace/etc volumeMounts and volumes, and the
glpi-secret envFrom, from the nginx container. Only php-fpm (which is
the only container that actually touches GLPI_VAR_DIR/marketplace/etc
and the database) keeps these. nginx now only mounts nginx-conf.

This also reduces blast radius: nginx no longer has DB credentials in
its environment (least privilege).

Testing

  • helm lint: 0 failures
  • helm template: nginx Deployment renders with only nginx-conf volume,
    no envFrom block

@eduardofraga

Copy link
Copy Markdown
Contributor

This project requires RWX persistent volume claim because php-fpm and nginx access same volume.

@eduardofraga eduardofraga added the enhancement New feature or request label Jul 28, 2026
@danielqb
danielqb force-pushed the fix/nginx-dead-volume-mounts branch from dc2ced2 to 458fc92 Compare August 5, 2026 04:22
@danielqb

danielqb commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the pushback - I re-verified this carefully before updating, since I want to make sure I'm not missing something.

Checked the actual shipped nginx config (docker/nginx/conf.d/default.conf, which matches helm/templates/glpi-configmap.yaml's nginx-conf exactly): root /var/www/html/public; is fixed, with only two location blocks - static try_files against that root, and fastcgi_pass to php-fpm for everything else. /var/lib/glpi, /var/www/html/marketplace, and /etc/glpi are never referenced anywhere in nginx's own config - they're separate, non-overlapping paths from root.

The nginx image itself is built with COPY --from=BUILD /var/www/html /var/www/html (docker/nginx/Dockerfile) - GLPI's web root is baked into the nginx image layer at build time, independent of any PVC.

Verified live on a kind cluster with these mounts removed:

  • Login page: HTTP 200, real GLPI title
  • /css_compiled/css_glpi.min.css (compiled/minified CSS - a good stress test since it could plausibly be runtime-generated): HTTP 200, and response headers (Last-Modified, ETag, Accept-Ranges) confirm nginx serves it as a static file directly from its own image layer, not proxied through PHP - completely unaffected by the removed PVC mounts either way

Happy to do any additional verification you'd like (e.g. testing a specific marketplace plugin scenario) if there's a case I'm not accounting for - genuinely want to get this right rather than just push back. Rebased onto the new helm/ structure and re-tested with helm lint --strict + helm template.

@eduardofraga eduardofraga left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try replace hardcoded to extraEnv, extraVolumes and extraVolumeMounts in values

… extraEnv/extraVolumes/extraVolumeMounts

Addresses maintainer's latest review ("Try replace hardcoded to extraEnv,
extraVolumes and extraVolumeMounts in values") on top of the original
fix, rebased onto the current main (post eftechcombr#174/eftechcombr#175/eftechcombr#176/eftechcombr#177/eftechcombr#191/eftechcombr#193
merges + the _helpers.tpl hotfix).

## Problem

The nginx container mounted the files, marketplace, and etc
PersistentVolumeClaims (ReadWriteOnce) and pulled glpi-secret/glpi-config
(DB credentials, cache DSN) via envFrom, despite nginx's shipped config
never reading from those paths or using those values (verified
previously: root is fixed to /var/www/html/public, baked into the nginx
image at build time, independent of any PVC; live kind test confirmed
GLPI's login page and a real compiled-CSS asset both serve correctly
with these removed).

## Fix

- Removed the hardcoded files/marketplace/etc volumeMounts+volumes and
  the glpi-config/glpi-secret envFrom from the nginx container - back to
  the original proposal, now confirmed correct against the current chart.
- Added `glpi.phpfpm.extraEnv` / `extraVolumeMounts` / `extraVolumes` and
  `glpi.nginx.extraEnv` / `extraVolumeMounts` / `extraVolumes` (matching
  the exact naming convention already used by the helmforge/mariadb and
  helmforge/valkey subcharts' own extraEnv/extraVolumes/extraVolumeMounts
  values) - all empty by default, so this is the escape hatch the
  maintainer asked for: an operator with a custom nginx-conf that DOES
  need direct filesystem access to one of these paths (e.g. serving
  marketplace plugin assets directly instead of through PHP) can add it
  back via values, without needing a chart fork or the chart
  hardcoding it for everyone by default.
- php-fpm's existing conditional `env: MARIADB_PASSWORD` block (from
  eftechcombr#191) is merged into the same single `env:` list as extraEnv, rather
  than adding a second env: key (which would silently shadow the first
  under YAML's last-key-wins behavior).

## Testing

- helm lint --strict: 0 failures
- helm template (default): nginx renders with only the nginx-conf
  volume/mount, no envFrom/env at all
- helm template with glpi.nginx.extraEnv/extraVolumeMounts/extraVolumes
  and glpi.phpfpm.extraEnv/extraVolumeMounts/extraVolumes all populated:
  all six render correctly on their respective containers/pods; verified
  php-fpm's MARIADB_PASSWORD (mariadb.enabled=true) and a custom extraEnv
  entry coexist correctly in the same env: list
- Live kind cluster (kindest/node:v1.35.0), default values: helm install
  completed (STATUS: deployed), HTTP 200 with GLPI's real login page
@danielqb
danielqb force-pushed the fix/nginx-dead-volume-mounts branch from 458fc92 to 7ec016c Compare August 7, 2026 18:16
@danielqb

danielqb commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Updated per your suggestion. Kept the original fix (nginx no longer mounts files/marketplace/etc or pulls glpi-secret/glpi-config by default, since its shipped config never uses them), and added the escape hatch you asked for:

  • glpi.phpfpm.extraEnv / extraVolumeMounts / extraVolumes
  • glpi.nginx.extraEnv / extraVolumeMounts / extraVolumes

Matching the exact naming convention already used by helmforge/mariadb/helmforge/valkey's own extraEnv/extraVolumes/extraVolumeMounts. So by default nginx stays lean (matching what its actual config needs), but anyone with a custom nginx-conf that needs direct filesystem access to one of those paths (e.g. serving marketplace assets directly instead of through PHP) can add it back via values, without a fork.

Rebased onto current main (post #174/#175/#176/#177/#191/#193). Re-tested with helm lint --strict, helm template (default + with all six extra* values populated), and a live kind install (HTTP 200, real login page).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants