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/httpd: remove deprecated extraSubservices option #71067

Merged
merged 1 commit into from
Oct 21, 2019

Conversation

aanderse
Copy link
Member

Motivation for this change

Cleaning up the httpd module.

I would appreciate the efforts of any httpd users who would run their configuration through this branch, compare the generated httpd.conf to their own, and then share the results.

Things done

I have compared several httpd configuration files generated by this new build compared to what 19.09 builds and they are pretty much identical.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 @

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Oct 13, 2019
@aanderse
Copy link
Member Author

@Izorkin do you run an httpd instance you could generate configuration and test this against?

@Izorkin
Copy link
Contributor

Izorkin commented Oct 15, 2019

@aanderse checked on one site - it works. Also used patch #56304

--- /home/user/works/src-nix/nixpkgs/nixos/modules/services/web-servers/apache-httpd/default.nix  2019-10-15 22:41:27.021710619 +0300
+++ /home/user/works/nixops/config-defs/modules/web-servers/apache-httpd/default.nix      2019-10-15 22:42:24.238755660 +0300
@@ -241,7 +241,7 @@

     DefaultRuntimeDir ${mainCfg.stateDir}/runtime

-    PidFile ${mainCfg.stateDir}/httpd.pid
+    PidFile /run/httpd/httpd.pid

     ${optionalString (mainCfg.multiProcessingModule != "prefork") ''
       # mod_cgid requires this.
@@ -260,9 +260,6 @@
       in concatStrings uniqueListen
     }

-    User ${mainCfg.user}
-    Group ${mainCfg.group}
-
     ${let
         load = {name, path}: "LoadModule ${name}_module ${path}\n";
         allModules = map (name: {inherit name; path = "${httpd}/modules/mod_${name}.so";}) apacheModules
@@ -429,7 +426,7 @@

       stateDir = mkOption {
         type = types.path;
-        default = "/run/httpd";
+        default = "/var/spool/httpd";
         description = ''
           Directory for Apache's transient runtime state (such as PID
           files).  It is created automatically.  Note that the default,
@@ -591,6 +588,11 @@
         date.timezone = "${config.time.timeZone}"
       '';

+    systemd.tmpfiles.rules = [
+      "d '${mainCfg.stateDir}' 0750 ${mainCfg.user} ${mainCfg.group} - -"
+      "d '${mainCfg.logDir}' 0750 ${mainCfg.user} ${mainCfg.group} - -"
+     ];
+
     systemd.services.httpd =
       { description = "Apache HTTPD";

@@ -605,31 +607,24 @@
           optionalAttrs mainCfg.enablePHP { PHPRC = phpIni; }
           // optionalAttrs mainCfg.enableMellon { LD_LIBRARY_PATH  = "${pkgs.xmlsec}/lib"; };

-        preStart =
-          ''
-            mkdir -m 0750 -p ${mainCfg.stateDir}
-            [ $(id -u) != 0 ] || chown root.${mainCfg.group} ${mainCfg.stateDir}
-
-            mkdir -m 0750 -p "${mainCfg.stateDir}/runtime"
-            [ $(id -u) != 0 ] || chown root.${mainCfg.group} "${mainCfg.stateDir}/runtime"
-
-            mkdir -m 0700 -p ${mainCfg.logDir}
-
-            # Get rid of old semaphores.  These tend to accumulate across
-            # server restarts, eventually preventing it from restarting
-            # successfully.
-            for i in $(${pkgs.utillinux}/bin/ipcs -s | grep ' ${mainCfg.user} ' | cut -f2 -d ' '); do
-                ${pkgs.utillinux}/bin/ipcrm -s $i
-            done
-          '';
-
-        serviceConfig.ExecStart = "@${httpd}/bin/httpd httpd -f ${httpdConf}";
-        serviceConfig.ExecStop = "${httpd}/bin/httpd -f ${httpdConf} -k graceful-stop";
-        serviceConfig.ExecReload = "${httpd}/bin/httpd -f ${httpdConf} -k graceful";
-        serviceConfig.Type = "forking";
-        serviceConfig.PIDFile = "${mainCfg.stateDir}/httpd.pid";
-        serviceConfig.Restart = "always";
-        serviceConfig.RestartSec = "5s";
+        serviceConfig = {
+          ExecStart = "${httpd}/bin/httpd -f '${httpdConf}'";
+          ExecStop = "${httpd}/bin/httpd -f '${httpdConf}' -k graceful-stop";
+          ExecReload = "${httpd}/bin/httpd -f '${httpdConf}' -k graceful";
+          Type = "forking";
+          PIDFile = "/run/httpd/httpd.pid";
+          Restart = "always";
+          RestartSec = "5s";
+          # User and group
+          User = mainCfg.user;
+          Group = mainCfg.group;
+          # Runtime directory and mode
+          RuntimeDirectory = "httpd";
+          RuntimeDirectoryMode = "0750";
+          # Capabilities
+          AmbientCapabilities = [ "CAP_NET_BIND_SERVICE" "CAP_SETGID CAP_SETUID" ];
+          CapabilityBoundingSet = [ "CAP_NET_BIND_SERVICE" "CAP_SETGID CAP_SETUID" ];
+        };
       };

   };

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/68

@@ -233,6 +233,7 @@ with lib;
(mkRemovedOptionModule [ "services" "mysql" "rootPassword" ] "Use socket authentication or set the password outside of the nix store.")
(mkRemovedOptionModule [ "services" "zabbixServer" "dbPassword" ] "Use services.zabbixServer.database.passwordFile instead.")
(mkRemovedOptionModule [ "systemd" "generator-packages" ] "Use systemd.packages instead.")
(mkRemovedOptionModule [ "services" "httpd" "extraSubservices" ] "Most existing subservices have been ported to the NixOS module system. Please update your configuration accordingly.")
Copy link
Member

Choose a reason for hiding this comment

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

See #61570, I think this would be better in the relevant module directly

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't know about that PR. Thank you, I'll update accordingly.

@infinisil
Copy link
Member

The changes look good, they seem to really only remove that functionality. One thing I'm wondering: Could removing this option be a problem for people because they defined their own services with it? What should they use instead in that case?

@aanderse
Copy link
Member Author

Thanks for the review @infinisil.

Could removing this option be a problem for people because they defined their own services with it?

Yes.

What should they use instead in that case?

They should create a NixOS module, or manage their web applications imperatively like they would on any other distribution. Fortunately migration to imperative management is very straight forward. The subservices functionality is seriously lacking and users aren't any worse off simply managing their web applications imperatively.

@infinisil
Copy link
Member

What's an example of a NixOS module to do this?

@aanderse
Copy link
Member Author

aanderse commented Oct 20, 2019

Examples of NixOS modules serving over httpd include:

  • limesurvey
  • mediawiki
  • moodle
  • wordpress
  • zabbixWeb

I'm hoping to land some changes in master before 20.03 to improve the httpd module and bring it up to the quality nginx is at. This PR is setting the stage for such PRs.

@infinisil
Copy link
Member

Ah right, it's the services you converted to proper modules recently :D

Sounds good then, feel free to merge yourself when you moved the mkRemovedOptionModule

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants