nixos/tomcat: add purifyOnStart option #44303

pvgoran · 2018-08-01T13:23:09Z

With this option enabled, before creating file/directories/symlinks in baseDir according to configuration, old occurences of them are removed.

This prevents remainders of an old configuration (libraries, webapps, you name it) from persisting after activating a new configuration.

Things done

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 nox --run "nox-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)
Fits CONTRIBUTING.md.

Mic92 · 2018-08-01T13:54:22Z

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

@@ -185,6 +188,10 @@ in
      after = [ "network.target" ];

      preStart = ''
+        # Delete the existing base directory, if any
+        # (we don't want remainders of old configuration!)
+        rm -rf ${cfg.baseDir}


Would this not also delete logs?

Right, it would, and it's definitely not wanted.

pvgoran · 2018-08-01T14:41:19Z

The new version specifically keeps logs in place. Not sure if find is the best way to do it (with all the options), but it works fine.

pvgoran · 2018-08-01T14:50:50Z

Now, it is possible that people added their own files into baseDir and expect them to stay there after restart or reconfiguration; this patch will break their configuration. However, leaving bits of old configuration (especially old libraries or old webapps) may be even worse. Not sure how to handle this.

This PR, while it very much makes sense by itself, is a pre-requisite of another PR which would add management of the tomcat-users.xml file.

Mic92 · 2018-08-01T16:00:34Z

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

+        # Delete everything but logs from the existing base directory
+        # (we don't want remainders of old configuration!)
+        [[-e ${cfg.baseDir} ]] \
+          && find ${cfg.baseDir} -mindepth 1 -maxdepth 1 -not -name logs -exec rm -rf '{}' '+'


Is it possible to do it more explicit?

rm -rf ${cfg.baseDir}/{conf,virtualhosts,temp,lib,shared/lib,webapps,work}

Are there any files in those directory that should be not deleted? I am not an expert when it comes to tomcat related applications.

Is it possible to do it more explicit?

Yes, this can work as well. Though I would also include bin which is a symlink created by the script. And work needs to be excluded (see below).

Are there any files in those directory that should be not deleted?

In fact, there are. The session files, named SESSIONS.ser by default, can exist inside the work directory, and they shouldn't be deleted.

On the other hand, work serves as a cache for compiled JSP files, and in certain corner cases Tomcat can improperly use the cached files from an old deployment. But these corner cases should be very uncommon (if at all possible), so probably it doesn't make much spending code lines on handling them.

joachifm · 2018-08-06T14:36:54Z

If deleting specific stuff is too clunky, an alternative is to write state into versioned dirs and use symlinks to point to whatever is current. That way old stuff can be garbage collected manually --- the tradeoff obviously being accumulating lots of junk over time & the overhead of managing the links ...

pvgoran · 2018-08-09T19:45:44Z

@Mic92 I replaced removal of "everything except logs" with removal of specific directories/symlinks.

pvgoran · 2018-08-09T19:51:16Z

@joachifm Did you mean versioning the whole tomcat directory? This would prevent loss of old files, but it's a solution that is way too "heavy" to be practical, and it doesn't prevent the main problem at hand: unexpected change of behaviour of the startup procedure.

pvgoran · 2018-09-23T07:01:08Z

For some reason, the previous commit actually did not include the advertised changes ("I replaced removal of "everything except logs" with removal of specific directories/symlinks"), now this is corrected by
fefcd78.

Any chance for merging this soon?

7c6f434c · 2018-10-21T07:58:35Z

Just in case — @danbst do you want to comment on the list of directories to clean?

danbst · 2018-10-22T00:09:09Z

@pvgoran @7c6f434c
I think all good, but only enable if services.tomcat.purifyOnStart = true; is set (option to be created). Tomcat management is essentially stateful (the whole startup script is an example of this madness) and so many rely on this. For example, I have mutable webapp deploy to webapps dir - after this PR my webapp would be removed on tomcat restart (I can't put webapp into configuration.nix)

danbst · 2018-10-22T00:09:23Z

@pvgoran @7c6f434c
I think all good, but only enable if services.tomcat.purifyOnStart = true; is set (option to be created). Tomcat management is essentially stateful (the whole startup script is an example of this madness) and so many rely on this. For example, I have mutable webapp deploy to webapps dir - after this PR my webapp would be removed on tomcat restart (I can't put webapp into configuration.nix)

danbst · 2018-10-22T00:09:44Z

@pvgoran @7c6f434c
I think all good, but only enable if services.tomcat.purifyOnStart = true; is set (option to be created). Tomcat management is essentially stateful (the whole startup script is an example of this madness) and so many rely on this. For example, I have mutable webapp deploy to webapps dir - after this PR my webapp would be removed on tomcat restart (I can't put webapp into configuration.nix)

danbst · 2018-10-22T00:37:25Z

@pvgoran @7c6f434c
I think all good, but only enable if services.tomcat.purifyOnStart = true; is set (option to be created). Tomcat management is essentially stateful (the whole startup script is an example of this madness) and so many rely on this. For example, I have mutable webapp deploy to webapps dir - after this PR my webapp would be removed on tomcat restart (I can't put webapp into configuration.nix)

pvgoran · 2018-10-22T03:36:06Z

@danbst Makes sense. This new option is supposed to be disabled by default, right?

7c6f434c · 2018-10-22T06:21:37Z

OK, so GitHub sends notifications correctly but displays the new comments one in five reloads or something. Yay eventual consistency, I guess. I can merge once the option is added, please ping me then.

danbst · 2018-10-22T16:18:37Z

@pvgoran @7c6f434c I think all good, but only enable if `services.tomcat.purifyOnStart = true;` is set (option to be created). Tomcat management is essentially stateful (the whole startup script is an example of this madness) and so many rely on this. For example, I have mutable webapp deploy to `webapps` dir - after this PR my webapp would be removed on tomcat restart (I can't put webapp into configuration.nix) нд, 21 жовт. 2018 о 10:58 Michael Raskin <notifications@github.com> пише:

…

Just in case — @danbst <https://github.com/danbst> do you want to comment on the list of directories to clean? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44303 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAtWkbFmH5s6mdZADsJUjO6IECZMJg1Sks5unCkvgaJpZM4VqfSm> .

danbst · 2018-10-22T16:34:41Z

@pvgoran right! It is pretty destructive feature. I also am not in favor of changing defaults. пн, 22 жовт. 2018 о 07:09 pvgoran <notifications@github.com> пише:

…

@danbst <https://github.com/danbst> Makes sense. This new option is supposed to be disabled by default, right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44303 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAtWkb3O-J0JH4vLrBPIw8ZPPmY-OZMLks5unUT-gaJpZM4VqfSm> .

7c6f434c · 2018-10-22T17:19:25Z

«Once the option is added» was precisely about purifyOnStart

With this option enabled, before creating file/directories/symlinks in baseDir according to configuration, old occurences of them are removed. This prevents remainders of an old configuration (libraries, webapps, you name it) from persisting after activating a new configuration.

pvgoran · 2018-10-29T11:47:43Z

@danbst @Mic92 I added the purifyOnStart option to protect against removing things by default.

Now that it's in place, I'm thinking about going back to deleting everything (except the specifically preserved subdirectories) from baseDir, instead of deleting explicitly listed items. What do you think about it?

7c6f434c · 2018-10-29T19:21:37Z

What is more likely, that they add one more directory we want to keep or that they add one more directory we want to clean?

Intuitively I would consider an explicit removal of known-problematic subdirectories (with a comment about what and why we still keep) the cleanest approach.

pvgoran · 2018-10-29T19:48:36Z

What is more likely, that they add one more directory we want to keep or that they add one more directory we want to clean?

Both of these look improbable to me, if we are talking about Tomcat developers adding something. NixOS developers, on the other hand, could add something; in this case, I think it will be rather something we want to clean.

But this is not how I see this choice. I think it's more about what to do with unknown files which were not created by NixOS: keep them or remove them? The currently implemented approach is "keep", but personally I'd prefer "remove".

7c6f434c · 2018-10-29T19:56:26Z

But this is not how I see this choice. I think it's more about what to do with unknown files which were *not* created by NixOS: keep them or remove them? The currently implemented approach is "keep", but personally I'd prefer "remove".

What is your mental model of where these files would actually (probably) come from? Is there a need to add an option to extend the whitelist?

danbst · 2018-10-29T23:05:12Z

@pvgoran yes, you can remove everything. Those who don't want this behavior should remove purifyOnStart = true; declaration.
EDIT: maybe leave logs?

@7c6f434c

Is there a need to add an option to extend the whitelist?

Don't think so. Tomcat is pretty much stabilized software, which hasn't changed conceptually for ages.

pvgoran · 2018-10-30T19:04:47Z

What is your mental model of where these files would actually (probably) come from? Is there a need to add an option to extend the whitelist?

It could be something like custom logs, or data files/directories created by web applications. In this case, an explicit whitelist would be beneficial. However, I don't think this use case will appear (or at least appear frequently), so it's not worth adding an option now, until someone actually needs/requests it.

maybe leave logs?

Yes, and also work which (by default) contains session files.

@Mic92 Previously I switched from the "whitelist" approach (delete everything except selected directories) to the "blacklist" approach (delete only known directories that we create) by your request. Now, do you agree with going back to the "whitelist" approach with introduction of the purifyOnStart option?

7c6f434c · 2018-10-30T19:28:30Z

> What is your mental model of where these files would actually (probably) come from? Is there a need to add an option to extend the whitelist? It could be something like custom logs, or data files/directories created by web applications. In this case, an explicit whitelist would be beneficial. However, I don't think this use case will appear (or at least appear frequently), so it's not worth adding an option now, until someone actually needs/requests it.

To me that sounds like an argument for a blacklist (if anything extra is created it is custom logs or data files, and we keep the known logs and session data files). But I won't use the module anyway.

pvgoran · 2018-10-30T19:42:39Z

To me that sounds like an argument for a blacklist (if anything extra is created it is custom logs or data files, and we keep the known logs and session data files). But I won't use the module anyway.

The thing is, this use case is just a speculation on my part, "what if someone does this". I have no evidence someone would actually need to organize files like this.

pvgoran · 2018-11-04T07:16:44Z

I implemented the "whitelist" approach (removing of all baseDir contents except for logs and work) in #49729. I keep this PR as it is in case it will be preferred.

My preference is the "whitelist" approach (#49729), and I don't see strong arguments against it, now that the purifyOnStart option was added.

@Mic92 @7c6f434c @danbst Please review this and commit one of these two PRs, at last.

7c6f434c · 2018-11-04T17:27:39Z

@danbst I am OK with the current state of either PR, I marginally prefer this one, and I will merge whichever you ask me to merge (please do a direct mention in that comment).

danbst · 2018-11-04T17:46:48Z

@7c6f434c @pvgoran on second thought, I'd prefer this one, "blacklist" approach. Because we allow users to change baseDir, someone can set it to /home/XXX, and eventually add purifyOnStart. Which will be uh-oh. With current approach the disaster won't be that large, as with "whitelist" approach.

7c6f434c · 2018-11-04T17:56:53Z

I do not claim arguments for blacklist are strong, it is all about reasonable arguments on both sides. And now this bikeshed is freshly painted. Thanks to everyone.

nixos-discourse · 2018-11-19T12:15:55Z

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/nixops-and-tomcat/1472/2

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

Mic92 reviewed Aug 1, 2018

View reviewed changes

pvgoran force-pushed the tomcat-clean-basedir branch from 2027e6d to f958aa9 Compare August 1, 2018 14:35

pvgoran changed the title ~~nixos/tomcat: Delete baseDir before (re-)creating it~~ nixos/tomcat: Clean up baseDir before filling it Aug 1, 2018

Mic92 reviewed Aug 1, 2018

View reviewed changes

pvgoran force-pushed the tomcat-clean-basedir branch from f958aa9 to f9ad617 Compare August 9, 2018 19:38

pvgoran force-pushed the tomcat-clean-basedir branch from f9ad617 to fefcd78 Compare September 23, 2018 06:55

NixOS deleted a comment from danbst Oct 22, 2018

pvgoran force-pushed the tomcat-clean-basedir branch from fefcd78 to a57bbf4 Compare October 29, 2018 11:26

pvgoran changed the title ~~nixos/tomcat: Clean up baseDir before filling it~~ nixos/tomcat: add purifyOnStart option Oct 29, 2018

pvgoran mentioned this pull request Nov 4, 2018

nixos/tomcat: add purifyOnStart option (alt) #49729

Closed

9 tasks

7c6f434c merged commit 6b8252d into NixOS:master Nov 4, 2018

aanderse mentioned this pull request Apr 5, 2020

tomcat: rewrite module #84377

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/tomcat: add purifyOnStart option #44303

nixos/tomcat: add purifyOnStart option #44303

pvgoran commented Aug 1, 2018 •

edited

Mic92 Aug 1, 2018

pvgoran Aug 1, 2018

pvgoran commented Aug 1, 2018

pvgoran commented Aug 1, 2018

Mic92 Aug 1, 2018

pvgoran Aug 1, 2018

joachifm commented Aug 6, 2018

pvgoran commented Aug 9, 2018

pvgoran commented Aug 9, 2018

pvgoran commented Sep 23, 2018

7c6f434c commented Oct 21, 2018

danbst commented Oct 22, 2018

danbst commented Oct 22, 2018

danbst commented Oct 22, 2018

danbst commented Oct 22, 2018

pvgoran commented Oct 22, 2018

7c6f434c commented Oct 22, 2018

danbst commented Oct 22, 2018 via email

danbst commented Oct 22, 2018 via email

7c6f434c commented Oct 22, 2018 •

edited

pvgoran commented Oct 29, 2018

7c6f434c commented Oct 29, 2018

pvgoran commented Oct 29, 2018

7c6f434c commented Oct 29, 2018 via email

danbst commented Oct 29, 2018 •

edited

pvgoran commented Oct 30, 2018 •

edited

7c6f434c commented Oct 30, 2018 via email

pvgoran commented Oct 30, 2018

pvgoran commented Nov 4, 2018

7c6f434c commented Nov 4, 2018

danbst commented Nov 4, 2018

7c6f434c commented Nov 4, 2018

nixos-discourse commented Nov 19, 2018

nixos/tomcat: add purifyOnStart option #44303

nixos/tomcat: add purifyOnStart option #44303

Conversation

pvgoran commented Aug 1, 2018 • edited

Things done

Mic92 Aug 1, 2018

Choose a reason for hiding this comment

pvgoran Aug 1, 2018

Choose a reason for hiding this comment

pvgoran commented Aug 1, 2018

pvgoran commented Aug 1, 2018

Mic92 Aug 1, 2018

Choose a reason for hiding this comment

pvgoran Aug 1, 2018

Choose a reason for hiding this comment

joachifm commented Aug 6, 2018

pvgoran commented Aug 9, 2018

pvgoran commented Aug 9, 2018

pvgoran commented Sep 23, 2018

7c6f434c commented Oct 21, 2018

danbst commented Oct 22, 2018

danbst commented Oct 22, 2018

danbst commented Oct 22, 2018

danbst commented Oct 22, 2018

pvgoran commented Oct 22, 2018

7c6f434c commented Oct 22, 2018

danbst commented Oct 22, 2018 via email

danbst commented Oct 22, 2018 via email

7c6f434c commented Oct 22, 2018 • edited

pvgoran commented Oct 29, 2018

7c6f434c commented Oct 29, 2018

pvgoran commented Oct 29, 2018

7c6f434c commented Oct 29, 2018 via email

danbst commented Oct 29, 2018 • edited

pvgoran commented Oct 30, 2018 • edited

7c6f434c commented Oct 30, 2018 via email

pvgoran commented Oct 30, 2018

pvgoran commented Nov 4, 2018

7c6f434c commented Nov 4, 2018

danbst commented Nov 4, 2018

7c6f434c commented Nov 4, 2018

nixos-discourse commented Nov 19, 2018

pvgoran commented Aug 1, 2018 •

edited

7c6f434c commented Oct 22, 2018 •

edited

danbst commented Oct 29, 2018 •

edited

pvgoran commented Oct 30, 2018 •

edited