-
-
Notifications
You must be signed in to change notification settings - Fork 447
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
Rework file path organization and configuration #491
Conversation
Immediately I see something I don't like that I missed beforehand. The announcement shouldn't be titled |
cf656d2
to
8bf644a
Compare
Regarding the alias/service name, do we get that somewhere on the API so we know which Moonraker service is the one we are accessing when we get the known services state? |
I didn't make any changes to the API in this series, however it won't be a problem to add it. It should be possible to determine the service unit name for Klipper as well, however there is a touch more involved. |
After some initial conversations I have concluded that Moonraker does not need the I changed the default data folder name to |
This looks like a good change, and I applaud you for attempting to automate it as much as possible. I do have a request though (and I completely understand if you decline), but is there any possibility that you might delay merging this by a week? I just noticed this today (those announcements are such a great feature), and I'm leaving for vacation tomorrow, so I won't be able to test it ahead of time / make necessary changes or help RatOS users get through it on the 7th and the following 4 days. Of course that's my problem, not yours :) Thanks in advance, as always, your work is much appreciated! |
There is no harm in delaying a week, I will change to the date to the 14th. The more chance for testing the better. |
Excellent! Thank you! |
Just an observation, since I have seen this before. Calling it "printer_data", might not be the best name. What is this install manages 10 printers? When we install multiple instances of Klipper and so on the naming convention does not flow or is consistent with a single printer. If we are taking steps to "tidy" things up, could we not possibly consider a more congruent naming convention for 1, 6, or 12 printers under a single install? Please? |
When multiple instances are involved the folders will be |
fcacb9a
to
7ed03a5
Compare
Prepare to move away from configurable paths. This will resolve potential security vulnerabilities in the event that a user's access is compromised. Signed-off-by: Eric Callahan <[email protected]>
The config and logs paths are no longer configurable, they all exist as folders or symbolic links within the primary data folder. The gcode path no longer relies on Klipper to specify the location, instead Klipper's virtual_sdcard path shold be configured to the location of the "gcodes" folder in the data path. Signed-off-by: Eric Callahan <[email protected]>
Deprecate the "database_path" option. If the database does not exist, however the "database_path" does, it will be used as a fallback. Signed-off-by: Eric Callahan <[email protected]>
The secrets module will now look for "moonraker.secrets" in the data folder. If the file does not exist the deprecated "secrets_path" option will be used as a fallback. Signed-off-by: Eric Callahan <[email protected]>
Signed-off-by: Eric Callahan <[email protected]>
Allow components to register reserved paths, then perform reserved path validation it upon request. Reserved paths may be registered as read-only or no access. Any request to modify an file/folder that is either reserved or a child of a reserved path is rejected. Signed-off-by: Eric Callahan <[email protected]>
Handle exceptions raised when adding a new watch. Warn the user and skip adding the node to the watched tree. Signed-off-by: Eric Callahan <[email protected]>
Signed-off-by: Eric Callahan <[email protected]>
Signed-off-by: Eric Callahan <[email protected]>
Signed-off-by: Eric Callahan <[email protected]>
Signed-off-by: Eric Callahan <[email protected]>
Additionally, rework the systemd unit so it is not necessary to overwrite and reload systemd when changes are made to Moonraker's arguments. Use a symbolic link for the executable and an environment flle to supply the arguments. Signed-off-by: Eric Callahan <[email protected]>
Signed-off-by: Eric Callahan <[email protected]>
Add documentation for the new validation options, and document changes to the sudo APIs. Signed-off-by: Eric Callahan <[email protected]>
Signed-off-by: Eric Callahan <[email protected]>
Report the unit names for both Moonarker and Klipper. Signed-off-by: Eric Callahan <[email protected]>
Signed-off-by: Eric Callahan <[email protected]>
Differentiate instances based on the data path provided. Signed-off-by: Eric Callahan <[email protected]>
This prevents an upgrade from unintentionally populating the config directory when a legacy install exists. Signed-off-by: Eric Callahan <[email protected]>
This script provides a method for users to complete an upgrade that requires elevated privileges via ssh. Signed-off-by: Eric Callahan <[email protected]>
7ed03a5
to
d964cf2
Compare
Update: I have decided to deprecate the debug config options and move them to the command line. This will be part of tomorrow's merge. The reasoning is the same, it reduces the worst case scenario on compromised/exposed systems. There are 3 new command line options:
These can easily be be added to The Unlike the config changes, these changes aren't fully automated. The deprecated options will be removed, however the install validator won't attempt to add The |
d964cf2
to
148dbd8
Compare
Add a debug option to the command line to enable debug features. Signed-off-by: Eric Callahan <[email protected]>
Signed-off-by: Eric Callahan <[email protected]>
Allow full database access to registered debug endpoints. Signed-off-by: Eric Callahan <[email protected]>
Use the "debug" command line option to enable debug features for the update manager. Signed-off-by: Eric Callahan <[email protected]>
Signed-off-by: Eric Callahan <[email protected]>
148dbd8
to
9deb905
Compare
Small update, I have decoupled verbose logging and the debug features. So to get both Moonraker needs |
What's the best way to test this? Was hoping to switch to the branch, reset to an early commit and run the moonraker update, but of course that won't work without some way to tell moonraker that it's now syncing against another branch. I checked out the branch and restarted moonraker but absolutely nothing happened, it shows the new branch, but everything is working like before, no notifications, no modifications to any files. Update: it did create the printer_data folder and symlinked the items. Update 2: It did everything it was supposed to do, i'm just super blind. Seems like it "just works"! I'm gonna give it more of a challenge now, i've got most of the config hidden away in an include (including the |
Okay so, when the [file_manager] section resides in an included file, moonraker won't symlink the printer_data dirs, that would be nice to have, is that possible? Of course, it won't modify the included file and I wouldn't expect it to (that would actually be a bit of a problem for RatOS :)) |
It should work with included files. In my tests it both creates the symlinks and removes the deprecated options from included files. When testing, make sure you start in a pristine state. Moonraker tracks validation in the database, so that key must be removed before performing a new test. I have a script I run when I want to reset:
This script does the following:
After making these changes, moonraker can be restarted to re-test validation under different conditions. |
I thought I would provide a bit more context on how the the install validator works. It does not rely on the When Moonraker finishes loading all components it will check to see if install validation is necessary. The database stores a key to indicate if validation has been performed. If the key does not exist, Moonraker will first validate if the service file needs to be updated. It will then proceed to validate the configuration. When validating the configuration, it will fetch the value of deprecated path options and create symbolic links IF the folder/link does not exist in the data path. It will then remove the option from the configuration. Moonraker's configuration parser tracks the file location of each section/option. When a section and/or option is removed it will remove it from all files. If this is a problem for RatOS we'll need to discuss how to proceed, if possible it would be good if this can be resolved on your end. This functionality will be used in the near future by configuration APIs. Front ends will be able to use these APIs to present a UI for modifying the config. |
Yeah that'll do it! I'll try again :)
I expected I'd have to do something, which is okay. It's a bit of a chicken and egg problem though, I can't change the included config before moonraker has been updated and when moonraker has been updated the RatOS configuration would have a dirty working copy. I could read from the moonraker database and check the validate_install key, then if that has been set i can swap out the include in the user's moonraker.conf. Is there something similar to EDIT: seems like EDIT2: Just remembered i have a moonraker post-merge hook. That would allow me to checkout the old ratos-configuration moonraker.conf include in case it has been modified, and then I should be good. Sorry for using this PR as a notepad, lol. EDIT3: the post-merge hook runs too early (before moonraker is restarted and makes the changes) :( |
I see, I wasn't sure what the issue is but suspected it might be that the config exists in a git repo. One possibility would be for the post-merge hook to first check for the "validate_install" key, and if it exists do nothing (suggests that the update has already occured). If it doesn't exist it can write out a temporary script and schedule it as a cron job to run every minute. Once the script begins execution I believe it should be able to remove itself from cron, then periodically check for the validate_install key. Once its detected it can reset the branch or do whatever is necessary. |
Maybe there is another option. You could update the config in your repo to remove the deprecated options, then use the post merge hook to add them back. Presuming Moonraker does the right thing, when it removes the options the repo should be pristine again. |
Both of these are viable solutions, I already considered the cronjob, but decided against it because I was hesitant to add yet another management mechanism. The last suggestion is pretty clever though, let me think about that some more. Currently the plan was to merge the included file into the user's moonraker.conf (just replace the include section), and then later on delete hose sections and make them managed again, while creating a backup. But your solution seems more robust. I'll talk it over with the team, thank you for taking the time! |
Went ahead with you last suggestion, it's implemented, and it looks like it does the job! Fantastic! Thank you for that suggestion that made things a whole lot easier. There's the case of a dirty working copy that'll have to be reset in case moonraker is run first, but that's a minor issue and we already condition users to update RatOS first, so it's fine. I'll merge and push as soon as you merge this PR. You have a go from me! :) |
@miklschmidt Glad it worked out in tests. I will go ahead and merge this now. |
…per images. fixes #40
This pull request will significantly change how file paths are configured. There are two primary goals:
As things stand today, a compromised system could have their config and/or gcode paths reconfigured. This pull request organizes all of Moonraker's into a single data folder, named
$HOME/printer_data
by default. The folder is organized something like the following:Each instance of Moonraker should have its own data path, however its acceptable for the files and folders within it to be symbolic links. A new command line option for Moonraker,
-d <datapath>
, has been introduced to configure this location. The-c
and-l
options still exist if a user desires custom names and/or locations for the log and config.This change comes with an update to Moonraker's systemd service. The new file will look something like the following:
There are two significant changes to the service:
SV1
The environment file contains Moonraker's arguments, this will make possible to update the arguments in the future without updating the service itself. The version is for the
InstallValidator
covered below.Having learned from the polkit experience, this pull request also includes an
InstallValidator
that attempts to automate the changes required to the service unit and configuration. There will be situations where this isn't possible (ie: instances running in containers). Options are available to disable either service or config validation in themachine
section, its possible that containers can get away with only config validation.The validator will first check the service version, if it does not match it will attempt to update the unit file. It will use the current unit file's name to determine the alias, and thus the new data folder location. If this is successful, Moonraker will move to config validation. It will look for the existing path options, remove them, and create symbolic links to them in the data folder.
Everything should go smoothly if Moonraker user has passwordless sudo, however this isn't the case on MainsailOS (not sure about FluiddPi). Thus I had to come up with a means to allow users to enter their linux user password. At this time I am leveraging announcements and Moonraker's landing page to handle this. APIs are available for front ends to handle this directly in the future if they choose to do so.
When Moonraker requests sudo access, users will get some warnings and the announcement:
Mainsail:

Fluidd:


Clicking through the link will take them to the landing page:

After entering the correct password, they'll get the following message:

The data folder will look like the following after completion. Only items that are configured are linked, so if the user does not have ssl certs they won't be linked, etc :

I have done quite a bit of testing with this, but it could certainly use more. Assuming there are no objections I plan to leave this up for a few days, make an announcement, then merge a few days later.