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

syncthing: expand declarative config #5616

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

karaolidis
Copy link

@karaolidis karaolidis commented Jul 7, 2024

Description

This PR expands the Syncthing config to allow declarative settings. Code mostly pulled from Nixpkgs module. Addresses #4049.

Changes to Nix Module

Removed the following options:

  • user, group, systemService: Unnecessary since Syncthing always runs as the user declaring the config.

  • dataDir configDir, databaseDir: Pointed to ~/.local/state/syncthing, the default Syncthing directory.

  • openDefaultPorts: We don't have access to the system firewall.

Furthermore, multiple changes to systemd services were made to maintain consistency with other home-manager modules, sandboxing options might need to be reviewed further.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added.

  • Commit messages are formatted

Notes

I have marked the PR as a draft while I test it over the next week or so.

Maintainer CC

@rycee

@karaolidis karaolidis force-pushed the syncthing-declarative branch 3 times, most recently from 94da8f1 to 178325d Compare July 8, 2024 12:55
@karaolidis karaolidis marked this pull request as ready for review July 11, 2024 08:54
@karaolidis
Copy link
Author

Things appear to be working so far :)

@zorrobert
Copy link
Contributor

zorrobert commented Jul 15, 2024

@karaolidis This looks great! I'm using syncthing as well and it would be great to manage it via home-manager. Thanks for your effort!

@bphenriques
Copy link

I was looking for this as well! This is great, thank you!

@karaolidis karaolidis force-pushed the syncthing-declarative branch 2 times, most recently from 6f825e5 to 091a99d Compare July 31, 2024 06:52
@qenya
Copy link

qenya commented Aug 6, 2024

Ah, this is exactly what I was looking for too! Thank you for writing this!

@karaolidis
Copy link
Author

karaolidis commented Aug 7, 2024

Updated branch to master and made syncthing folders relative to the user's home :)

...
                  path = mkOption {
                    type = str // {
                      check = x:
                        str.check x && !strings.hasPrefix "/" x
                        && !strings.hasPrefix "~" x;
                      description = str.description
                        + " not starting with / or ~/";
                    };
                    apply = x: "${config.home.homeDirectory}/${x}";
                    default = name;
                    description = ''
                      The path to the folder which should be shared relative to the
                      user's home directory.
                    '';
                  };
...

@wrvsrx
Copy link

wrvsrx commented Aug 26, 2024

Users might have syncthing folders whose paths are not in home.

@karaolidis
Copy link
Author

Hm, I guess that's fair. I just thought that making the options consistent with e.g. home.file would be the way to go.

Should I undo that change?

@wrvsrx
Copy link

wrvsrx commented Aug 26, 2024

Thanks for you work. I think original version would be better.

Besides, I was wondering if you can add an option to use a local file to specify password, so that hashed password won't appear in nix store.

Here's my patch:
34ea327

@karaolidis karaolidis marked this pull request as draft August 26, 2024 11:07
@karaolidis karaolidis force-pushed the syncthing-declarative branch 3 times, most recently from 186ac7a to 1b4b58e Compare August 26, 2024 11:18
@karaolidis
Copy link
Author

Done, thanks for the feedback :)

Changes made:

Testing now and will mark as Ready for review after that.

@karaolidis
Copy link
Author

karaolidis commented Aug 26, 2024

Would adding a gui section in the options that allows users to set both user and either a password or passwordFile make sense, or am I overthinking this?

cc @wrvsrx

Never mind, settings.gui.user is a thing...

@pitkling
Copy link
Contributor

pitkling commented Aug 26, 2024

@karaolidis Thanks for the work on this PR! I'm using it in my private home-manager fork and it works rather well so far. One question concerning the handling of the guiAddress setting (which actually also applies to the original NixOS module): Is there a reason for having this as a separate option that is given to the synching service via the command line as opposed to setting it via settings.gui.address and have it handled "natively" by syncthing via its configuration file?

@karaolidis
Copy link
Author

Is there a reason for having this as a separate option that is given to the synching service via the command line as opposed to setting it via settings.gui.address and have it handled "natively" by syncthing via its configuration file?

I am assuming that since the module sets the settings semi-imperatively using curls, we need to pass in the GUI address in the command line, otherwise Syncthing will start with its default listen address. I think that technically this could also be set through settings.gui.address instead of guiAddress, but the underlying implementation would remain the same.

@karaolidis karaolidis marked this pull request as ready for review August 26, 2024 14:02
@pitkling
Copy link
Contributor

Is there a reason for having this as a separate option that is given to the synching service via the command line as opposed to setting it via settings.gui.address and have it handled "natively" by syncthing via its configuration file?

I am assuming that since the module sets the settings semi-imperatively using curls, we need to pass in the GUI address in the command line, otherwise Syncthing will start with its default listen address. I think that technically this could also be set through settings.gui.address instead of guiAddress, but the underlying implementation would remain the same.

I see, thanks. I had a look at the implementation and I think you're right. In particular, the very first time the synching configuration is run, when there is no configuration file yet, the command line argument is required to ensure that syncthing starts with the correct address.

While this is probably out of scope for this PR, it would be nice to avoid the command-line argument. It can be quite confusing that the address given in the config file is completely ignored by the systemd service (that's how I stumbled upon this). E.g., one could create a minimal configuration if there is no config file present that has the correct gui address before starting syncthing. This would potentially also avoid other artifacts of the current implementation, like a "~/Sync" folder being created (by the default configuration of synching) on the very first boot, even though the declarative syncthing configuration does not have such a folder.

Anyway, thanks again for the PR!

@karaolidis
Copy link
Author

This would potentially also avoid other artifacts of the current implementation, like a "~/Sync" folder being created (by the default configuration of synching) on the very first boot, even though the declarative syncthing configuration does not have such a folder.

I know that this doesn't really solve the problem but, FYI, you can work around this with extraOptions = [ "-no-default-folder" ]; :)

@pitkling
Copy link
Contributor

This would potentially also avoid other artifacts of the current implementation, like a "~/Sync" folder being created (by the default configuration of synching) on the very first boot, even though the declarative syncthing configuration does not have such a folder.

I know that this doesn't really solve the problem but, FYI, you can work around this with extraOptions = [ "-no-default-folder" ]; :)

Good tip, thanks! Actually, shouldn't this be a default command-line argument in a declarative syncthing configuration (that specifies at least one folder)?

@karaolidis
Copy link
Author

This would potentially also avoid other artifacts of the current implementation, like a "~/Sync" folder being created (by the default configuration of synching) on the very first boot, even though the declarative syncthing configuration does not have such a folder.

I know that this doesn't really solve the problem but, FYI, you can work around this with extraOptions = [ "-no-default-folder" ]; :)

Good tip, thanks! Actually, shouldn't this be a default command-line argument in a declarative syncthing configuration (that specifies at least one folder)?

The nixpkgs module doesn't add it, so I didn't add it here for consistency. I did think about making a PR for the original module, but that wouldn't really be backwards compatible if someone was relying on the default behavior :/

modules/services/syncthing.nix Outdated Show resolved Hide resolved
Comment on lines +634 to +639
Environment =
mkIf (cfg.all_proxy != null) { inherit (cfg) all_proxy; };
Copy link
Contributor

Choose a reason for hiding this comment

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

The original NixOS module adds config.networking.proxy.envVars to the environment, which seems to contain not only all_proxy but also other, global proxy variables, at least some of which (like http_proxy and https_proxy) are seemingly also used by Syncthing. I'm not really familiar with proxy settings, but is there a reason not to add the remaining proxy variables like the original module does?

Copy link
Author

@karaolidis karaolidis Aug 31, 2024

Choose a reason for hiding this comment

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

In the home-manager module, I think config does not give us access to networking.proxy.envVars as it points to the home-manager configuration itself, and I am not quite sure how to access items in the root config. I would appreciate any ideas on how to do this.

osConfig.whatever

Copy link
Contributor

Choose a reason for hiding this comment

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

There is the osConfig module argument (see Home Manager manual). Although I'm not sure how this is handled if Home Manager is used standalone. In fact, some searching revealed that it's simply not available in a standalone installation.

I guess one could bind the module parameters via something like @args{ config, lib, pkgs, ... } and check for osConfig via args ? osConfig to include the system proxy variables if available.

Copy link
Author

@karaolidis karaolidis Aug 31, 2024

Choose a reason for hiding this comment

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

I force-pushed a version with osConfig, but a quick search doesn't appear to show any other modules using it, probably because it doesn't exist at all in standalone installs as you said 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I force-pushed a version with osConfig, but a quick search doesn't appear to show any other modules using it 🤔

I would assume that's because it's little known (I think the installation part of the manual is the only place where it is mentioned, and it's not the first place one would look for it) and since it works only if HM is used as a nixOS or nix-darwin module.

Personally I think it's ok to use as long as it's safe-guarded via something like args ? osConfig, to make sure the module does not break on standalone HM, but not sure how important the system proxy settings are.

modules/services/syncthing.nix Show resolved Hide resolved
Copy link
Contributor

@pitkling pitkling left a comment

Choose a reason for hiding this comment

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

I've been using this to configure several systems during the past few weeks and didn't find any problems. Would be great if this gets merged eventually 🙂.

@karaolidis karaolidis force-pushed the syncthing-declarative branch 3 times, most recently from fa3d6bc to 3dd31ba Compare October 17, 2024 15:12
Signed-off-by: Nikolaos Karaolidis <[email protected]>
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.

6 participants