Skip to content

Conversation

@eltariel
Copy link

@eltariel eltariel commented Dec 12, 2025

🗣 Description

This change introduces an environment variable, ${CONTAINER_UMASK}, to control the umask of the node process. If this variable is set, launcher.sh passes it to the umask command before launching node.

💭 Motivation and context

This change makes it easier to manage shared assets between containers without needing them all run as the same user. Setting CONTAINER_UMASK controls what permissions are used for new (e.g. uploaded) files, allowing them to e.g be group or world writable.

🧪 Testing

To check if umask is set:

  • Start container using docker compose.
  • Check logs for umask messages
  • Run docker compose exec foundry find -L /proc -maxdepth 2 -name exe -samefile /usr/local/bin/node -print -execdir grep Umask status \; to check the umask of the foundry process - there should only be one match.
  • Stop the container.

To check the umask is applied correctly:

  • create a file in foundry (e.g. upload, make a scene, etc)
  • check the permissions of the file using ls -la

Repeat the checks for at least these situations:

  • CONTAINER_UMASK unset => no log, umask is 0022
  • CONTAINER_UMASK=0002 => info message printed, umask is 0002
  • CONTAINER_UMASK="u=rwx,g=rwx,o=rx" => info message printed, umask matches (will need to check how to quote it properly for compose.yml)
  • CONTAINER_UMASK=oops => info message, warning message, umask is 0022. Container starts as normal.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated
    to reflect the changes in this PR.
  • Tests have been added and/or modified to cover the changes in this PR.
  • All new and existing tests pass.

✅ Pre-merge checklist

  • Revert dependencies to default branches.
  • Finalize version.

✅ Post-merge checklist

  • Add a tag or create a release.

@felddy
Copy link
Owner

felddy commented Dec 15, 2025

Thank you for taking the time to write this excellent PR!

I hope to test it this week and get it merged.

@eltariel
Copy link
Author

I'm glad you like it, it was fairly simple to put together but I wanted to make sure it was robust and useful. I picked CONTAINER_UMASK for the variable name because I found a discussion from aaaages ago when I went looking for anything at all about umask in issues/PRs/whatever 🤣

For testing, I'm currently using this on some of my foundry instances and it works as expected. I have my host login and a user for each container in a foundry group. I'm currently running three containers with the same host directory with permissions 0775 bind-mounted to /data/assets/shared/:

  • Container 1 has the current released image. Files uploaded to the shared dir have permissions 0644.
  • Container 2 has my PR with CONTAINER_UMASK=0022. Files uploaded to the shared dir have permissions 0644.
  • Container 3 has my PR with CONTAINER_UMASK=0002. Files uploaded to the shared dir have permissions 0664.

I'm eventually going to change all of them to CONTAINER_UMASK=0002 because it makes the shared assets a lot easier to manage.

Because CONTAINER_UMASK is just passed as-is to the umask bash builtin the symbolic representation (e.g u=rwx,g=rwx,o=rx) should just work as well, but I haven't tested that yet and it's a far less common way of representing umask anyway.

@eltariel eltariel marked this pull request as ready for review December 16, 2025 00:26
@eltariel eltariel requested a review from felddy as a code owner December 16, 2025 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants