View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0005369unrealmodule apipublic2019-08-26 19:142019-11-27 10:42
ReporterGottem Assigned ToGottem  
PrioritynormalSeverityfeatureReproducibilityN/A
Status resolvedResolutionfixed 
Product Version5.0.0-alpha2 
Fixed in Version5.0.0-rc1 
Summary0005369: Detect missing (global) mods network-wide
DescriptionCouple o' snippets:

<@Jobe> what unreal needs is a way for servers, when linking, to detect modules that require loading globally that arent loaded on one server but are on another

<~Syzop> I was thinking [you] should probably also communicate versions

<@Jobe> if the other server sees one it doesnt have, reject the link
Except I would prolly just warn and perhaps make it configurable to be more strict.
TagsNo tags attached.
3rd party modules

Relationships

has duplicate 0004037 closedGottem [feature] require|deny module block 
child of 0005282 resolvedGottem Gottem's todo list yo 

Activities

syzop

2019-08-27 13:41

administrator   ~0020857

Last edited: 2019-08-27 14:17

 Related to an older feature request 0004037. I think we can do both: A) enforcing it in the module and B) allowing you to require things in the config.
I have no problem at all if we only do A at this point and then close this issue. Then leave 0004037 open for B.

Gottem

2019-09-11 22:21

developer   ~0020883

Last edited: 2019-09-12 20:26

 First draft is done (require-module.c), for which I've combined this and the other request. =]
Commit diff: https://github.com/unrealircd/unrealircd/commit/358a31eaee4ac359d66040fe37b62c155956fb67

It currently works by sending a couple of "REQMODS :Lmod:v1.0 Gmod2:v2.0 ..." lines in a SERVER_CONNECT hook. These lines contain *all* modules so we can check for module versions and denied modules regardless of them being marked as global. Then in the respective CMD_FUNC it'll error only on *missing* modules that are marked as global.

Marking a module as global from the mod itself is easy, just put this in MOD_INIT: MARK_AS_GLOBAL_MODULE(modinfo);
From the config it's also pretty simple:
require module {
    name "foo";
    name "bar";
};


Which just makes this module set MOD_OPT_GLOBAL on foo and bar itself. :D There's no need to have a custom reason for allowed/required modules, meaning you can specify multiple mods in the same block.

Then for denying mods:
deny module {
    name "foo";
    reason "bar";
};


The reason field is optional here, which defaults to: "A forbidden module is being used" (I simply went with "forbidden" because it sounds better in this case tbh)
You can only use *one* module per deny block though, unlike before. Keep in mind that a module's internal name is easily changed so this really only serves a purpose for catching accidentally loaded modules (e.g. you're phasing something out that might conflict with a different module).

Also, for (what I think are) obvious reasons you cannot have a server load a module *and* specify it in a deny block. The same is true for require; the server must have it loaded itself to begin with.

By default the module only warns on problems and doesn't actually kill the connection. To change that:
set {
    require-module {
        squit-on-deny yes; // Denied modules, you prolly want this set to yes
        squit-on-missing yes; // Entirely missing modules on either end
        squit-on-mismatch yes; // Version mismatch between servers
    };
};


The module will eventually be loaded by default. =]

--------
Couple o' thangs tho:
1) Both servers can kill the connection for different reasons *simultaneously* (e.g. version mismatch vs. denied module), so one server might receive a "Write error: Connection reset by peer". :D
2) It should probably also complain if the other end doesn't support REQMODS?
3) Currently only the server missing a certain module will report on it (and SQUIT if configured), which may or may not be a problem somewhere down the road. :D

Gottem

2019-09-12 20:25

developer   ~0020884

Kkk so by Syzop's suggestion the module now waits with SQUITting a server until a full SMOD line has been parsed, as opposed to quitting immediately. This speeds up error fixing because you can potentially see multiple messages. I could delay it further but then the servers have likely synchronised their users already, meaning they'd all get constant QUIT messages (รก la netsplit).

The write error is probably unavoidable because either end has to be able to close the connection. It's not a real problem (i.e. Unreal doesn't crash) and the additional line in the server console doesn't really bother me anyways. :D

Also Syzop made a couple of changes:
1) REQMODS has been renamed to SMOD
2) squit-on-deny defaults to yes
3) The module has been renamed to require-module (so NOT -module*s*)

syzop

2019-09-14 07:33

administrator   ~0020885

Last edited: 2019-09-14 07:44

 I think the default action for a module that has been denied via deny module { } should be squit, since it's an explicit action by the admin. I don't think anyone will disagree with that.
Similarly, a require module { } should squit as well if it's missing, since again.. it's an explicit action by the admin. If you say "require", it shouldn't be "oh I'll let it in anyway" (although that would be very Dutch :D).
However, I think there's something to say for a module that tags itself as global to be just a warning and not squit.
For example, when I developed the reputation module, some people wanted to try it out on their live network. But to be safe, it was loaded first on 1 server only. Now, the module only works best if all servers have it, so it makes sense to tag it as global, but it doesn't greatly misbehave if it's just on 1 server. Is that a corner case? I don't know. The same is true for channel modes, user modes, etc. We can tag them all as global if we don't squit, and raise a nice warning.

So, what does that mean:
* don't use the same flag system/system for the config-interface (that goes in .conf files) and the module-interface (that goes in .c files)
* split up the set::require-module config directives

Also, I would like to have a look myself at other names for the config directives.

Other than that, I don't think the version mismatch thing is terribly useful. The way I foresaw it being used was that you could require a minimum version somewhere in the config file. Just blindly warning or even squitting on a version mismatch is not really... well... i don't know. I suppose it serves some purpose, but, it may be seen as useless warnings, and the more useless warnings we have, the less users are going to pay attention to these and other warnings ;)
Also, if we would do the "minimum version" stuff, one would have to use a strcmp that compares in "natural" order instead of "regular" strcmp. This so a version of 1.0.9 is seen as earlier than 1.0.10 (which isn't the case for strcmp).
I see stuff with an acceptable license at https://github.com/sourcefrog/natsort, will need some refactoring, I will look into stuffing that in UnrealIRCd.
Anyway, the version stuff is a separate thing, and I don't think it needs to immediately be addresses. But feel free to do so ;)

syzop

2019-09-14 08:15

administrator   ~0020886

Last edited: 2019-09-14 08:15

 Added the natural sort shit in case you want to do the latter. Anyway, that's the lowest priority item. Rest is more important :D

Gottem

2019-11-07 22:04

developer   ~0021069

Finally found some time y0. :D

New order of checks:
1: deny module { } -- SQUIT always
2 (if module not loaded): require module { } -- SQUIT always
3 (if module not loaded): MOD_OPT_GLOBAL -- warn
4 (optional, if module loaded only): require module::min-version

The error/warning messages shown to opers were also updated to better reflect this. All of these are broadcasted globally by the server that generated them:
1: Server %s is using module '%s', which is specified in a deny module { } config block (reason: %s)
2: Required module wasn't (fully) loaded or is missing entirely: %s
3: [WARN] Module marked as global wasn't (fully) loaded or is missing entirely: %s
4: Module version mismatch for required module '%s' (should be equal to or greater than %s but we're running %s)

I've ripped out the entire set {} block because with the updated functionality those separate squit-on-* things only got confusing anyways. :D

Also, unlike before, you can no longer specify multiple module names in require module { } due to the addition of the min-version directive. This directive is optional, so if it's omitted then simply *any* version of the module is required. I'm using strnatcasecmp() by the way, just in case it might check e.g. "1.0-ALPHA" vs "1.0-alpha".

Config:
require module {
    name "foo";
};
require module {
    name "foo";
    min-version "5.1";
};
deny module {
    name "foo";
    reason "bar";
};


That more like what you had in mind? :>

syzop

2019-11-27 10:42

administrator   ~0021118

Yeah that sounds good.

I didn't test it any further but it should be OK. Let's close this feature request.

Issue History

Date Modified Username Field Change
2019-08-26 19:14 Gottem New Issue
2019-08-26 19:14 Gottem Status new => assigned
2019-08-26 19:14 Gottem Assigned To => Gottem
2019-08-26 19:14 Gottem Issue generated from: 0005368
2019-08-26 19:15 Gottem Relationship added child of 0005282
2019-08-27 13:39 syzop Relationship added related to 0004037
2019-08-27 13:41 syzop Note Added: 0020857
2019-08-27 14:17 syzop Note Edited: 0020857
2019-09-07 17:05 syzop Target Version 5.0.0-alpha2 =>
2019-09-11 22:21 Gottem Relationship replaced has duplicate 0004037
2019-09-11 22:21 Gottem Note Added: 0020883
2019-09-11 22:21 Gottem Note Edited: 0020883
2019-09-11 22:22 Gottem Note Edited: 0020883
2019-09-11 22:32 Gottem Note Edited: 0020883
2019-09-12 20:25 Gottem Note Added: 0020884
2019-09-12 20:26 Gottem Note Edited: 0020883
2019-09-12 20:52 Gottem Status assigned => has patch
2019-09-14 07:33 syzop Note Added: 0020885
2019-09-14 07:40 syzop Note Edited: 0020885
2019-09-14 07:40 syzop Note Edited: 0020885
2019-09-14 07:44 syzop Note Edited: 0020885
2019-09-14 08:15 syzop Note Added: 0020886
2019-09-14 08:15 syzop Note Edited: 0020886
2019-09-14 08:15 syzop Note Edited: 0020886
2019-11-07 22:04 Gottem Note Added: 0021069
2019-11-27 10:42 syzop Status has patch => resolved
2019-11-27 10:42 syzop Resolution open => fixed
2019-11-27 10:42 syzop Fixed in Version => 5.0.0-rc1
2019-11-27 10:42 syzop Note Added: 0021118