nixos/tomcat: allow verbatim config override #35443

polynomial · 2018-02-24T05:29:19Z

Motivation for this change

I am attempting to use NixOS to replace several Amazon Linux Tomcat systems. The current tomcat module in NixOS doesn't allow for configuration overrides similar to other modules (see: nginx https://github.com/NixOS/nixpkgs/blob/release-17.09/nixos/modules/services/web-servers/nginx/default.nix#L314-L322 ). The other two additions are patterns used in many enterprise tomcat deployments that:

allow you to create log directories such as 'info/' and 'error/' in the logs directory (logback/slf4j expects the configuration/deployment system to create these directories if your logback.xml uses it, it won't create them for you)
pass environment variables to tomcat which can be used to set certain configuration inside your applications

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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 nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

* add serverXml verbatim override * add environment file * add log directory creation

Mic92 · 2018-02-24T13:17:46Z

Wow, there was quite a lot of old stuff in that module. I have added some stuff that might get in conflict with yours and needs to be discussed.

Mic92 · 2018-02-24T13:27:55Z

nixos/modules/services/web-servers/tomcat.nix

+
+      environment = mkOption {
+        default = null;
+        example = "/etc/nixos/tomcat-environment";


A file of environment variables is not the nicest interface. Instead it should be a list of strings:

extraEnvironment = mkOption { type = types.listOf types.str; # ... }; # ... systemd.servies.tomcat.serviceConfig = { Environment = [ # ... ] ++ extraEnvironment; };

if somebody needs a file, it is always possible to set this manually outside the module: systemd.servies.tomcat.serviceConfig.EnvironmentFile
Or use nix to split a file in lines..

Mic92 · 2018-02-24T13:31:16Z

nixos/modules/services/web-servers/tomcat.nix

+          "JAVA_HOME=${cfg.jdk}"
+          "CATALINA_OPTS=${cfg.catalinaOpts}"
+          "ERRFILE=SYSLOG"
+          "OUTFILE=SYSLOG"


It would be nice to log to journald by default instead of log files,
but I also see why people want to have log files per vhost.
Access log is set in the module a the moment. So maybe that should be disabled unless requested.

Changing the logging scheme, should be probably guarded by stateVersion

I think I will revert logging for the time being and this can addressed in future pull requests.

Mic92 · 2018-02-24T13:41:15Z

Eval error is related to 7142440

grahamc · 2018-02-24T13:47:24Z

@GrahamcOfBorg eval

danbst · 2018-02-28T21:31:45Z

nixos/modules/services/web-servers/tomcat.nix

        default = "tomcat";
        description = "Group account under which Apache Tomcat runs.";
      };

      javaOpts = mkOption {
+        type = types.str;


I wish javaOpts would have type listOf str. This, however, requires some backwards compatible code and extra warning for old setups.

We could accept both actually.

danbst · 2018-02-28T21:32:36Z

nixos/modules/services/web-servers/tomcat.nix

+        default = "";
+        description = "
+          Verbatim server.xml configuration.
+          This is mutualyl exclusive with the virtualHosts options.


mutualyl typo

danbst · 2018-02-28T21:37:52Z

nixos/modules/services/web-servers/tomcat.nix

+
+        ${if cfg.serverXml != "" then ''
+          cat <<'EOF' > ${cfg.baseDir}/conf/server.xml
+          ${cfg.serverXml}EOF


Another way to do the same, without heredocs

cp -f ${pkgs.writeTextDir "server.xml" cfg.serverXml}/* ${cfg.baseDir}/conf/

* NixOS#35443 (review) * NixOS#35443 (review) * NixOS#35443 (review)

polynomial · 2018-03-06T22:53:39Z

Thank you so much for the feedback, and thank you @Mic92 for the additions. I think I've addressed most of the comments. I didn't do anything about #35443 (review)
#35443 (review)
Would love more direction about which specific course we want to take, and/or if we could future those as open issues and merge this as is.

Mic92 · 2018-03-07T10:12:40Z

@polynomial Could you just change these two things?

change javaOpts and catalinaOpts to type type = types.either (types.listOf types.str) types.str; and put a toStrings from builtins where they are used to convert them?
remove "ERRFILE=SYSLOG" and "OUTFILE=SYSLOG" again and make sure log files work as advertised

I think other stuff should be not solved in the scope of this pull request.

* add javaOpts to systemd service definition * remove syslog defaults

polynomial · 2018-03-07T20:16:30Z

@Mic92 Done! I tested locally and the javaOpts seemed to have been entirely broken! I think it is fixed now and I tested it locally (and the log change too). Let me know if there is anything else, have a great day. (:

Mic92 · 2018-03-08T09:10:51Z

rebased and pushed in 196e21a

fixes #35443

pvgoran · 2018-08-02T07:37:51Z

Any reason why this wasn't merged into the stable release? I want to make further changes to the Tomcat module, and it's difficult to do when the differences between stable and master are that big.

Mic92 · 2018-08-02T08:13:45Z

We are conservative about back porting to not break the stable release. And this was a rather big change.

pvgoran · 2018-08-02T08:46:06Z

@Mic92 I see. But when and how will the back porting eventually happen? What is the procedure? The change is already rather old.

pvgoran · 2018-08-02T08:50:53Z

@Mic92 @polynomial What's the deal with the type of virtualHosts in this PR? Why is its name attribute types.listOf types.str and not just types.str? And its webapps attribute is not described at all!

danbst · 2018-08-02T09:04:53Z

@pvgoran you may switch to nixos-unstable. Other than that, you can replace tomcat.nix NixOS module with one from nixos-unstable (see https://nixos.org/nixos/manual/#sec-replace-modules for more details).

But when and how will the back porting eventually happen? What is the procedure? The change is already rather old.

No backporting. It will be available in stable NixOS 18.09, which will be ready in October (2 months left to release)

What's the deal with the type of virtualHosts in this PR? Why is its name attribute types.listOf types.str and not just types.str? And its webapps attribute is not described at all!

IMO this is stuff from old author, @svanderburg. Ping him if you want some clarifications. Though webapps isn't described, I did use it for deploying tomcat webapps.

pvgoran · 2018-08-02T09:54:35Z

Other than that, you can replace tomcat.nix NixOS module with one from nixos-unstable (see https://nixos.org/nixos/manual/#sec-replace-modules for more details).

Thanks, it will help.

No backporting. It will be available in stable NixOS 18.09, which will be ready in October (2 months left to release)

It's a bit sad, but I understand that the proper release process is better suited for rolling out significant changes. By the way, is there any kid of policy document that describes what should be backported?

IMO this is stuff from old author, @svanderburg. Ping him if you want some clarifications. Though webapps isn't described, I did use it for deploying tomcat webapps.

The type for virtualHosts was introduced in this PR, in the following commit: 472f16d#diff-1ca47ff430c580c1c14cf86b7bd32028R117, apparently authored by @Mic92. So @svanderburg doesn't seem to have anything to do with it.

Now, the thing is, the type specification is indeed wrong. My configuration that works on release-18.03 breaks when I build it with a module from nixos-unstable. First it complains:

error: The option `services.tomcat.virtualHosts.[definition 1-entry 1].webapps' defined in `<somefile>.nix' does not exist.

If I remove the webapps attribute, it complains about name:

error: The option value `services.tomcat.virtualHosts.[definition 1-entry 1].name' in `<somefile>.nix' is not of type `list of strings'.

Finally, if I change name into a list of strings, it fails with:

error: cannot coerce a list to a string, at /nix/var/nix/profiles/per-user/root/channels/nixos-unstable/nixos/modules/services/web-servers/tomcat.nix:218:107

I'll prepare a correction. It would be a separate PR, since this one is long merged, right?

danbst · 2018-08-02T10:10:43Z

The type for virtualHosts was introduced in this PR, in the following commit:

Ah, right! You were talking about type, not an option.

Indeed, if you experience backwards incompatible changes, you should make issues/PRs. But please, show exceprt of original config, so issue is reproducible.

By the way, is there any kid of policy document that describes what should be backported?

There probably is one. The rule of thumb is "backport only security patches" and internal bug fixes, no package/modules featureful updates. In rare cases, maintainers way lobby some non-security stuff when they have time and need.

pvgoran · 2018-08-02T15:21:49Z

Created #44361.

Tomcat Module:

355a053

* add serverXml verbatim override * add environment file * add log directory creation

polynomial changed the title ~~Tomcat Module:~~ nixos/tomcat: allow verbatim config override Feb 24, 2018

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Feb 24, 2018

nixos/tomcat: add types + proper systemd integration

472f16d

Mic92 reviewed Feb 24, 2018

View reviewed changes

danbst reviewed Feb 28, 2018

View reviewed changes

polynomial added 3 commits March 6, 2018 13:56

Updated for review comments:

e461b33

* NixOS#35443 (review) * NixOS#35443 (review) * NixOS#35443 (review)

Remove environment setting, use list of strings instead

e5b7443

Change extraEnvironment, fix example typo

581cbdf

Updated per review comments

a7ac4fd

* add javaOpts to systemd service definition * remove syslog defaults

Mic92 closed this Mar 8, 2018

Mic92 added a commit that referenced this pull request Mar 8, 2018

nixos/tomcat: add types + proper systemd integration

196e21a

fixes #35443

sergeykranga deleted the tomcat-config branch November 14, 2023 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/tomcat: allow verbatim config override #35443

nixos/tomcat: allow verbatim config override #35443

polynomial commented Feb 24, 2018 •

edited

Mic92 commented Feb 24, 2018

Mic92 Feb 24, 2018

Mic92 Feb 24, 2018

Mic92 Feb 24, 2018

Mic92 Mar 7, 2018

Mic92 commented Feb 24, 2018

grahamc commented Feb 24, 2018 •

edited

danbst Feb 28, 2018

Mic92 Mar 1, 2018

danbst Feb 28, 2018

danbst Feb 28, 2018 •

edited

polynomial commented Mar 6, 2018

Mic92 commented Mar 7, 2018 •

edited

polynomial commented Mar 7, 2018

Mic92 commented Mar 8, 2018

pvgoran commented Aug 2, 2018

Mic92 commented Aug 2, 2018

pvgoran commented Aug 2, 2018

pvgoran commented Aug 2, 2018 •

edited

danbst commented Aug 2, 2018 •

edited

pvgoran commented Aug 2, 2018

danbst commented Aug 2, 2018 •

edited

pvgoran commented Aug 2, 2018

nixos/tomcat: allow verbatim config override #35443

nixos/tomcat: allow verbatim config override #35443

Conversation

polynomial commented Feb 24, 2018 • edited

Motivation for this change

Things done

Mic92 commented Feb 24, 2018

Mic92 Feb 24, 2018

Choose a reason for hiding this comment

Mic92 Feb 24, 2018

Choose a reason for hiding this comment

Mic92 Feb 24, 2018

Choose a reason for hiding this comment

Mic92 Mar 7, 2018

Choose a reason for hiding this comment

Mic92 commented Feb 24, 2018

grahamc commented Feb 24, 2018 • edited

danbst Feb 28, 2018

Choose a reason for hiding this comment

Mic92 Mar 1, 2018

Choose a reason for hiding this comment

danbst Feb 28, 2018

Choose a reason for hiding this comment

danbst Feb 28, 2018 • edited

Choose a reason for hiding this comment

polynomial commented Mar 6, 2018

Mic92 commented Mar 7, 2018 • edited

polynomial commented Mar 7, 2018

Mic92 commented Mar 8, 2018

pvgoran commented Aug 2, 2018

Mic92 commented Aug 2, 2018

pvgoran commented Aug 2, 2018

pvgoran commented Aug 2, 2018 • edited

danbst commented Aug 2, 2018 • edited

pvgoran commented Aug 2, 2018

danbst commented Aug 2, 2018 • edited

pvgoran commented Aug 2, 2018

polynomial commented Feb 24, 2018 •

edited

grahamc commented Feb 24, 2018 •

edited

danbst Feb 28, 2018 •

edited

Mic92 commented Mar 7, 2018 •

edited

pvgoran commented Aug 2, 2018 •

edited

danbst commented Aug 2, 2018 •

edited

danbst commented Aug 2, 2018 •

edited