fix: skip the vendor tree when fixing permissions on startup

[methakon]

Fixes the slow startup with a custom PUID/PGID reported in #557.

Problem

On every start 03-config.sh chowns the whole ${LIBRENMS_PATH}/vendor tree — 13,704 entries on a fresh image. Those files live in the image layer, so every chown copies the file up: startup stalls for 90s+ on fast disks and many minutes on slower/network storage (#524). The existing find can't converge here: a freshly created container always starts from the build-time ownership, so the tree needs "fixing" again on every start.

Change

• 03-config.sh: the vendor tree and the composer* files are no longer chowned at startup — the performance penalty CrazyMax pointed out in Add missing ext-xmlwriter extension and fix permissions #510. The runtime fix still covers the paths that need ownership while the container runs (config.d, bootstrap, logs, storage, /data/...).
• Dockerfile: vendor and the composer files are made writable for any uid at build time (chmod -R a+rwX vendor, chmod a+rw composer.json composer.lock), keeping plugin/composer installs working under a custom PUID/PGID (the use case from Add missing ext-xmlwriter extension and fix permissions #510) without any runtime chown.

Measured (custom PUID/PGID, fresh container, same machine)

	entries covered	time
before	13,704	did not complete in >15 min (stopped)
after	355	33s

13,704 → 355 entries is the point: the remaining work is per-file overlay copy-up (~90ms/file on this machine's storage — also why the before case never completed; the 90s in #557 is the same 13.7k copy-ups on faster disks).

Verified

• image builds; path modes set as intended (vendor dirs writable/traversable, composer files writable, existing exec bits kept)
• fresh container as librenms with PUID/PGID=805: writes OK into bootstrap/cache, storage, storage/framework/cache, logs, /data/logs (including a root-owned log file)
• plugin/composer flow as the same user: create + append inside vendor, append to composer.lock — all OK
• test/ compose stack boots to "ready to handle connections" with the patched image

[CLAassistant]

All committers have signed the CLA.

[murrant]

And this fix is always correct? it will never result in a broken container?

[methakon]

Fair question, and I would rather give you the precise answer than a reassurance.

It cannot produce a container that fails to start. The change removes vendor and composer* from the startup chown, and adds chmod -R a+rwX vendor plus chmod a+rw composer.json composer.lock in the Dockerfile at build time. So a path that used to be re-owned at startup is now world-writable in the image. Nothing in the change makes a start path conditional, and nothing removes a directory or file that the app needs at runtime, so there is no state in which a previously-working container stops starting because of this.

But your question exposes the real risk, and it is not "will it start". It is: does anything in the container need to write to vendor under a custom PUID/PGID, and does a+rwX cover it?

a+rwX is the important detail. The capital X sets the execute bit on directories only, and leaves it alone on regular files. So this is rwx for every directory and rw for every file, for any uid. That covers a non-root PUID writing into vendor, and it covers traversal. The thing it deliberately does not do is make every file executable, which chmod -R a+rwX avoids and a naive chmod -R a+rwx would not.

Where I would be careful, honestly:

1. a+rwX is broader than the ownership it replaces. Previously those files were librenms:librenms; now they are world-writable. For a container running as an arbitrary PUID that is the workable option and it is a common pattern, but it is a real change in the security posture of those paths. If you would rather not ship world-writable files, the alternative is to keep the runtime chown but scope it to a find -newer or only re-own on first start, which recovers most of the startup win without loosening permissions at all. I am happy to switch to that if you prefer it.

2. I have tested the startup path, not every plugin-install path. The paths that keep their ownership fix are untouched. The paths that no longer get it are only written by plugin and composer installs, per the comment in the diff. I have not exercised a live composer install or plugin install as a non-root PUID against this build, so I would call that tested-by-reasoning rather than tested.

3. If someone runs the container as root and then drops privileges, the pre-existing chown was what made that transition safe for those files. World-writable is still safe there, just less tidy.

So: it will not start a broken container, and the case you are actually worried about - a valid container that used to start and now does not - is not reachable through this change. The open question is whether you are comfortable with the permission broadening, and if not I have a narrower alternative ready.

#510 raised the startup cost this removes; I agree with that and I think the build-time approach is the right trade. Happy to be told otherwise.

[methakon]

Following up on my last comment, since I do not want it to sit unanswered.

To restate the one decision that is actually yours: the change makes vendor and the composer files world-writable in the image, where before they were librenms:librenms. That is what lets the startup chown go away, and it is the only part of this that is a real trade rather than a pure win.

If you would rather not ship world-writable files, I have a narrower version ready that keeps the permissions as they were and still removes the first-start penalty: re-own only what is still root:root at startup, using chown -R --from=root:root librenms:librenms on the vendor tree. That is the approach tomaskir benchmarked in #557, and it converges after the first start because a recreated container comes back with the image's ownership. It would be a few more lines in 03-config.sh and no chmod in the Dockerfile, at the cost of a slower first start.

Either is fine by me - I went with the build-time chmod because it removes the cost entirely rather than deferring it, but the --from=root:root version is the more conservative change if you would rather keep the permission model intact.

If you are happy with the current approach, a review would be appreciated, and I am happy to rebase onto main if anything has moved.

[murrant]

That sounds like this is reverting a previous change... maybe we are going in circles here?

[methakon]

Good instinct to check that, and I want to answer it precisely rather than hand-wave, because the distinction matters.

To be clear about what is and is not a revert: #540 is still in place. master today still runs the find with \( ! -user librenms -o ! -group librenms \) over the full set including vendor, and this PR does not undo that - it removes vendor and composer* from the set and sets the permissions at build time instead. The find remains for every other path, so the second-and-later-starts improvement from #540 is preserved.

Where the circling risk actually is, and it is your point rather than mine: the narrower alternative I offered, chown -R --from=root:root, is close in spirit to what Zugschlus proposed in #524, which you said did not work in your testing. So offering it was not a great suggestion, and I should not have put it back in front of you without saying that first. That was the actual mistake.

The distinction I was trying to draw is that find ... ! -user and chown --from=root:root are not the same thing. The find form walks the tree and stats every entry on every start, so it costs a full traversal of ~13k paths even when nothing needs changing. The --from form is the one case where I was wrong: it still walks the tree, and the win is only that it does not chown entries that are already correct, which is much the same win the find already provides. So it would not have delivered the remaining gain, and it would have reintroduced the pattern that did not work for you. I withdraw it.

That leaves the build-time chmod in this PR as the only version of the idea that actually removes the traversal rather than narrowing it, which is why the open question I care about is still the permission broadening: vendor and the composer files go from librenms:librenms to world-writable in the image.

So, three things and no more variants from me:

• If you are fine with a+rwX on those paths, this is ready to review as it stands.
• If you would rather not ship world-writable files, then the honest answer is that the startup cost stays, and I would close the PR rather than propose a third approach that is another variation on what did not work.
• If vendor genuinely needs to stay owned by librenms:librenms, then the correct home for this is a different mechanism entirely (an entrypoint that re-owns only on the first start, with a sentinel), and I would want to agree that shape with you before writing it rather than guess again.

To close the loop on your first question, since I owe you that directly: no, I cannot claim the change is always correct. What I can say is that the case you were worried about - a previously working container that now fails to start - is not reachable through this diff, because nothing in it makes a start step conditional and nothing removes a file the app needs. What I cannot claim is that the permission model is acceptable to you, and that is the actual decision.

[murrant]

I really hate LLM generated text.