Skip to content
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

Configurable file storage locations #886

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

mtstickney
Copy link
Contributor

I wanted to package planka for deployment as a regular system service on linux (not in a container). Packaging standards and the filesystem conventions on linux require static code, configuration, and writable data directories to be separated.

Specifically, the directories for user-uploaded files (project backgrounds, user avatars), card attachments, and the server logfile need to be outside the main directory.

There are already custom-config properties to set the location of the upload directories, but they are served with the default static-file route in Sails, which means they can only be in subdirectories of the public/ directory (this contains the app code, so in a normal deploy this directory isn't going to be writable).

To fix that, I've added two custom routes that will serve project backgrounds and user avatars from the configured directories (card attachments are already handled by a separate controller), which can now reside outside the app dir.

To make the file-upload directory configurable, I added a helper to receive files, which uses an optional custom config property to control that directory -- this gets us a configurable location for all uploaded files, including backgrounds, avatars, attachments and import files.

Finally, I added an environment variable to control the location the server tries to write logs (this doesn't seem to actually get used, but it will still try to create the file).

With those things in place, and with the right overrides of them, the app can be run from a write-only directory, with all the file-writing done elsewhere, as it would in a normal linux deployment. Without customizing any of the custom properties, the behavior should be the same as it was previously, so running it from a checkout in dev or in a container shouldn't be affected.

It may be desirable to log to a more standard location (e.g. in /var/log/),
or in some cases to turn logging to file off. To support these, use a
custom config property to determine the location of the output log file,
and default to the previous location if it is unset.
@CLAassistant
Copy link

CLAassistant commented Sep 20, 2024

CLA assistant check
All committers have signed the CLA.

@meltyshev
Copy link
Member

Hi! Thanks for your PR. I'll check it out now and work on getting it accepted :)

@meltyshev
Copy link
Member

Sorry for the question, do you have a sample configuration for saving files outside of the app folder?

I also noticed that fileUploadTmpDir has been added, but it's not specified in the config file and there's no environment variable to set it. Is it supposed to be like this?

@mtstickney
Copy link
Contributor Author

Sorry for the question, do you have a sample configuration for saving files outside of the app folder?

You'll probably want to tweak this some, and not all of these steps are strictly necessary, but this was my setup process:

  1. Create a new user with useradd -M -r -U planka
  2. mkdir /opt/planka
  3. Deploy the service there
  4. Make sure everything's owned by the planka user: chown -R planka:planka /opt/planka
  5. Make it read-only: chmod -R -w /opt/planka
  6. Create some target directories for uploads:
  • mkdir -p /var/lib/planka/tmp
  • mkdir -p /var/lib/planka/public/{user-avatars,project-background-images}
  • mkdir -p /var/lib/planka/private/attachments
  • chown -R planka:planka /var/lib/planka
  1. cd /opt/planka/server and use the command below to run it.

Probably in your case the only bit that really matters is deploying the app to a directory and making the whole thing read-only, and then making some target directories elsewhere (parent directory would do fine). You won't need an additional user, or to put it under /opt (skip sudo in the command below if you're not using a separate user). Dirs don't need to be in /var/lib either, just tweak the paths below.

I've been running the app with the following to test:

sails_custom__fileUploadTmpDir=/var/lib/planka/tmp \
sails_custom__userAvatarsPath=/var/lib/planka/public/user-avatars \
sails_custom__attachmentsPath=/var/lib/planka/private/attachments \
sails_custom__projectBackgroundImagesPath=/var/lib/planka/public/project-background-images \
LOG_FILE=/dev/null \
sudo -E -u planka ../start.sh

I also noticed that fileUploadTmpDir has been added, but it's not specified in the config file and there's no environment variable to set it. Is it supposed to be like this?

That's a fair question. I thought about adding it to the config file, but to retain the old behavior when it's not set it needs to be... null? undefined? I'm not sure of the exact value, but Sails seems to use some built-in temp directory when you don't specify that.

[Some investigating later...]

Experimentally the value is undefined. Looks like file upload is being handled by Skipper; the docs say the dirname param is "optional", but don't specify what the means specifically. Looks like anything non-string value will do per [1], so I guess we might as well just add it to the config with a null value.

[1] https://github.com/sailshq/skipper/blob/48fdeec3329f45e7246e2cf8dd9d094bba9ffada/lib/private/Upstream/build-renamer-stream.js#L52

@mtstickney
Copy link
Contributor Author

Pushed a commit to add the temp-directory parameter to the custom-config file, and to add some package-lock file rejiggering npm wanted to do (should have gone in with the addition of the serve-static dep).

I'll squash those before this gets merged, I didn't want to do a force-push if you were working with a checkout.

@meltyshev
Copy link
Member

Thanks for the answer!

I understand now, you set the paths via sails_custom__* environment variables, and that's why I thought they were missing from the .env.sample file 🙈

Everything works correctly, so ready to merge. Feel free to force push if that's easier :)

This involves a couple primary changes:
1) to make Sails' temporary file-upload directory a configurable location
   by using a common file-upload-receiving helper;
2) to create custom static routes for the file-upload locations, so they
   can be outside the application's public directory; and
3) to use the file-uploading handler everywhere that receives files, so
   config for the helper is applied to all file uploads consistently.

This is sufficient to allow the application directory to be deployed read-
only, with writable storage used for file uploads. The new config property
for Sails' temporary upload directory, combined with the existing settings
for user-avatar and background-image locations are sufficient to handle
uploads; the new custom routes handle serving those files from external
locations.

The default behavior of the application should be unchanged, with files
uploaded to, and served from, the public directory if the relevant
config properties aren't set to other values.
@mtstickney
Copy link
Contributor Author

Squashed and pushed.

The sails custom-config env vars are kind of an awkward way to do config, but at this point I'm just going for something that works.

@meltyshev meltyshev merged commit 368ead9 into plankanban:master Sep 20, 2024
1 check passed
@meltyshev
Copy link
Member

Thanks! And a special thanks for the code quality and comments, it's a next-level for me.

The sails custom-config env vars are kind of an awkward way to do config, but at this point I'm just going for something that works.

It would be easy to add this to the file with env variables if someone else asks this feature.

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.

3 participants