Allow having a read-only config file #1583

Wolf480pl · 2018-07-22T09:04:30Z

use a separate lockfile instead of locking the config file
add an option to run in read-only mode, which causes config to be opened read-only and is never overwritten

This should technically fix #975 for the narrow usecase described by that issue's OP.

Later I'm going to submit another pull request, taking care of #1253, which should take care of most usecases where part of the config is supposed to be provided externally, while the rest of it should be configurable through znc (eg. the users should still be allowed to edit their own settings, add networks, etc.)

codecov · 2018-07-22T12:21:34Z

Codecov Report

Merging #1583 into master will decrease coverage by <.01%.
The diff coverage is 53.19%.

@@            Coverage Diff             @@
##           master    #1583      +/-   ##
==========================================
- Coverage   37.47%   37.46%   -0.01%     
==========================================
  Files         127      127              
  Lines       31023    31026       +3     
  Branches       93       93              
==========================================
  Hits        11625    11625              
- Misses      19349    19352       +3     
  Partials       49       49

Impacted Files	Coverage Δ
include/znc/znc.h	`47.76% <100%> (+0.79%)`	⬆️
src/main.cpp	`48.67% <36.36%> (-0.39%)`	⬇️
src/znc.cpp	`49.47% <57.14%> (+0.24%)`	⬆️
include/znc/version.h	`80% <0%> (-10%)`	⬇️
modules/modperl.cpp	`44.77% <0%> (-2%)`	⬇️
src/FileUtils.cpp	`49.26% <0%> (-0.99%)`	⬇️
src/HTTPSock.cpp	`41.73% <0%> (-0.81%)`	⬇️
modules/modperl/startup.pl	`33.52% <0%> (-0.79%)`	⬇️
modules/webadmin.cpp	`7.26% <0%> (ø)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e69758...df3564e. Read the comment docs.

Instead of locking the config file, open a `<config>.lock` file and put the fcntl lock on it. Before this change, the config file needed to be opened read-write, even when we only want to read it, because of fcntl locks. The locks are meant to prevent two ZNCs from running on the same config, when they could overwrite each others' config changes. However, since any configs writes and rehashes always reopen the config by path, it's the path to the config that we want to lock, not the file itself (eg. if someone moves znc.conf to a different directory and creates a new one in our directory, we still don't want another znc instance to run using the new config, but we don't mind another znc instance running using the old config in its new location). Therefore, a separate lock file is not worse than locking the config file itself, and it will allow to open the config read-only.

Wolf480pl · 2018-07-22T20:31:56Z

As suggested by MetaNova on #znc-dev, made it take a --readonly commandline argument, instead of trying to detect if config is read-only.

Sometimes it is desirable for ZNC not to overwrite its config, eg. because the config comes from external configuration management tools. Add a commandline option to set ZNC in read-only mode, in which it will open the config file read-only, and will refuse to overwrite it. Note that this does not affect .registry files, which can still be written. Also add a big fat warning in the console when running in read-only mode.

teward · 2018-07-23T20:20:08Z

I'm not a dev, but I'm going to give this a -1 based on my testing and the discovery of some undesired behavior/results coming from my testing these changes.

The most prevalent issue is that when you try and issue /znc shutdown to gracefully stop the ZNC, when running in Read Only mode you get a hard error that prohibits shutdown process:

[2018-07-23 16:14:35] <*status> ERROR: Writing config file to disk failed! Aborting. Use SHUTDOWN FORCE to ignore.

Also, you have a case where if you do anything that'd result in a config file write, it has the tendency to be able to spam the user with error messages (if they're an admin, I assume, but that's not tested). You can end up with four messages rapidfired within a minute worth of "spam" to your notices tab or to your active screen if your client doesn't split them out into other tabs:

[2018-07-23 16:14:04] -*status- *** Writing the config file failed
[2018-07-23 16:14:10] -*status- *** Writing the config file failed
[2018-07-23 16:14:14] -*status-*** Writing the config file failed
[2018-07-23 16:14:20] -*status- *** Writing the config file failed

If we're running in read only mode, we expect that writing the config file will fail, or that it won't even attempt to write the config file, and therefore we shouldn't be getting these errors.

Further, if we're running in Read Only mode, we shouldn't have a hard failure when we attempt to use /znc shutdown to terminate the ZNC process.

My suggestions are:

(1) If we're running in Read Only mode, recognize that and don't crit-fail when you call /znc shutdown
(2) If running in Read Only mode, don't spam the admin/user about not being able to write the config file, as we don't expect that to succeed when in RO mode.

More to come as I do some more tests...

- don't call WriteConfig() outside of webadmin when in read-only mode - print a more descriptive error message about readonly mode on /znc saveconfig, and on ECONFIG_NEED_VERBOSE_WRITE (i.e. SIGUSR1) - don't print any error messages when in read-only mode and failing/skipping a regular ECONFIG_NEED_WRITE (eg. after channel join) - proceed with /znc shutdown without writing config when in read-only mode

Wolf480pl · 2018-07-23T22:17:50Z

@teward thanks for spotting this, I really should have tested my change better.
With the latest commit, the issues you've mentioned should be fixed.

I'd love if some dev looked that latest commit and told me if there's a better way to do it.
I'd rather have WriteConfig() return some tri-state (or more) enum, but haven't seen anything like that in ZNC codebase, and I'm reluctant to introduce such new pattern when the convention is to return bool.

DarthGandalf · 2018-07-26T22:21:25Z

src/znc.cpp

@@ -229,6 +229,15 @@ void CZNC::Loop() {
                // stop pending configuration timer
                DisableConfigTimer();

+                if (GetReadonlyConfig()) {
+                    if (eState == ECONFIG_NEED_VERBOSE_WRITE) {
+                        Broadcast("Writing the config skipped because "


This will regularly spam admins whenever e.g. "in-config" channel's key is changing...

I thought these changes trigger non-verbose config saves... I grepped for ECONFIG_NEED_VERBOSE_WRITE and the only place that sets it that I found was SIGUSR1 handler.

DarthGandalf · 2018-07-26T22:21:30Z

src/znc.cpp

+    m_bReadonlyConfig = bReadonlyConfig;
+
+    if (m_bReadonlyConfig) {
+        CUtils::PrintError("Running in read-only mode. Any changes to the settings, users,");


Why is it an error?

I wanted it to be a warning, but couldn't find anything like PrintWarning. I looked at what --allow-root does, and it prints its warning as an error.

DarthGandalf · 2018-07-26T22:23:16Z

Does this mode also prevent writing of .registry files?

DarthGandalf · 2018-07-26T22:24:43Z

I'd rather have WriteConfig() return some tri-state (or more) enum, but haven't seen anything like that in ZNC codebase

Many module callbacks return EModRet.

Wolf480pl · 2018-07-28T10:40:53Z

Does this mode also prevent writing of .registry files?

No. I thought it'd be better if it doesn't affect the .registry files.

Many module callbacks return EModRet.

So in this case it'd be CONTINUE for success, HALTCORE for read-only mode, and HALT for failure? If it's ok to use EModRet here, in something that isn't a module callback, stretching the semantics like that, then I guess I'll use it.

DarthGandalf · 2018-07-29T00:40:21Z

No. I thought it'd be better if it doesn't affect the .registry files.

Then some random changes will be saved, and some other changes will be not.

If it's ok to use EModRet here, in something that isn't a module callback

That's not what I meant... You wanted an example of enum return value instead of bool?

Wolf480pl · 2018-07-29T07:55:13Z

Then some random changes will be saved, and some other changes will be not.

Now that I think of it... yeah, that'll need to be read-only too.

That's not what I meant... You wanted an example of enum return value instead of bool?

I didn't want to introduce a new enum that'll be used in only one place. But maybe I should...

Wolf480pl force-pushed the readonly-config branch from 77b16e9 to f47fab0 Compare July 22, 2018 12:21

Wolf480pl force-pushed the readonly-config branch 2 times, most recently from b540b3b to c6fdf85 Compare July 22, 2018 18:47

Wolf480pl force-pushed the readonly-config branch 2 times, most recently from d8a0e8f to 47e0fb7 Compare July 22, 2018 20:25

Wolf480pl force-pushed the readonly-config branch from 47e0fb7 to 3114d7e Compare July 23, 2018 06:01

DarthGandalf reviewed Jul 26, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow having a read-only config file #1583

Allow having a read-only config file #1583

Wolf480pl commented Jul 22, 2018 •

edited

codecov bot commented Jul 22, 2018 •

edited

Wolf480pl commented Jul 22, 2018

teward commented Jul 23, 2018 •

edited

Wolf480pl commented Jul 23, 2018

DarthGandalf Jul 26, 2018

Wolf480pl Jul 28, 2018

DarthGandalf Jul 26, 2018

Wolf480pl Jul 28, 2018

DarthGandalf commented Jul 26, 2018

DarthGandalf commented Jul 26, 2018

Wolf480pl commented Jul 28, 2018

DarthGandalf commented Jul 29, 2018

Wolf480pl commented Jul 29, 2018

Allow having a read-only config file #1583

Are you sure you want to change the base?

Allow having a read-only config file #1583

Conversation

Wolf480pl commented Jul 22, 2018 • edited

codecov bot commented Jul 22, 2018 • edited

Codecov Report

Wolf480pl commented Jul 22, 2018

teward commented Jul 23, 2018 • edited

Wolf480pl commented Jul 23, 2018

DarthGandalf Jul 26, 2018

Choose a reason for hiding this comment

Wolf480pl Jul 28, 2018

Choose a reason for hiding this comment

DarthGandalf Jul 26, 2018

Choose a reason for hiding this comment

Wolf480pl Jul 28, 2018

Choose a reason for hiding this comment

DarthGandalf commented Jul 26, 2018

DarthGandalf commented Jul 26, 2018

Wolf480pl commented Jul 28, 2018

DarthGandalf commented Jul 29, 2018

Wolf480pl commented Jul 29, 2018

Wolf480pl commented Jul 22, 2018 •

edited

codecov bot commented Jul 22, 2018 •

edited

teward commented Jul 23, 2018 •

edited