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

nixos/geoipupdate: Init the module #73767

Open
wants to merge 1 commit into
base: master
from

Conversation

@dasJ
Copy link
Member

dasJ commented Nov 19, 2019

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

Copy link
Contributor

mmilata left a comment

Thanks for writing this module! Appreciate the systemd sandboxing options.

description = "Maxmind GeoIP updater";
after = [ "networking.target" ];
wantedBy = [ "multi-user.target" ];
startAt = "weekly";

This comment has been minimized.

Copy link
@mmilata

mmilata Dec 24, 2019

Contributor

RandomizedDelaySec for the timer might be useful to avoid DDoSing the Maxmind servers. The original service uses 1 hour.

default = "0";
};

licenseKey = mkOption {

This comment has been minimized.

Copy link
@mmilata

mmilata Dec 24, 2019

Contributor

Not sure whether to treat this as a password and use something like licenseKeyFile which would be a path to the file containing license key so as not to end up in nix store.

This comment has been minimized.

Copy link
@dasJ

dasJ Dec 29, 2019

Author Member

Yeah, but their config file format doesn't allow for imports sadly

This comment has been minimized.

Copy link
@mmilata

mmilata Jan 15, 2020

Contributor

You can generate the final config file in preStart (see e.g. nixos/modules/services/misc/gogs.nix), though it's a bit ugly.

This comment has been minimized.

Copy link
@mmilata

mmilata Jan 15, 2020

Contributor

Other possibility would be to add an option which accepts path to the complete configuration file, which the user can place outside of /nix/store if desired.

Type = "oneshot";
ExecStart = "${pkgs.geoipupdate}/bin/geoipupdate -d /var/lib/GeoIP -f ${configFile}";

ProtectHome = true;

This comment has been minimized.

Copy link
@mmilata

mmilata Dec 24, 2019

Contributor

Would ProtectSystem = "strict" break the service?

This comment has been minimized.

Copy link
@dasJ

dasJ Dec 29, 2019

Author Member

Oh yes I forgot that :( as well as PrivateTmp

enable = mkEnableOption "the automated GeoIP updater";

accountID = mkOption {
description = "Account ID of your Maxmind account (or 0 for free GeoLite DBs)";

This comment has been minimized.

Copy link
@mmilata

mmilata Dec 24, 2019

Contributor

Nitpick: they seem to capitalize the name as MaxMind.

@dasJ dasJ force-pushed the helsinki-systems:geoipupdate-service branch from fb4ef5f to 1e4bd71 Dec 29, 2019

extraConfig = mkOption {
description = "Extra configuration to append to the configuration file";
type = attrsOf str;

This comment has been minimized.

Copy link
@Infinisil

Infinisil Jan 7, 2020

Member

Instead of an extraConfig I suggest the approach in NixOS/rfcs#42: Have a settings option (of the same type as this one) where all settings are defined, including AccountID and such. These can then be defaulted in the config section below, e.g. services.geoipupdate.settings.AccountID = mkDefault cfg.accountID

StateDirectory = "GeoIP";
StateDirectoryMode = "0755";

User = "geoipupdate";

This comment has been minimized.

Copy link
@Infinisil

Infinisil Jan 7, 2020

Member

Does DynamicUser work? That would be preferable over creating a persistent user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.