haraka / Haraka

A fast, highly extensible, and event driven SMTP server
https://haraka.github.io
MIT License
5.09k stars 661 forks source link

Make folder/node_modules plugins load config from folder by default #1331

Closed baudehlo closed 8 years ago

baudehlo commented 8 years ago

This is a placeholder before a PR.

The basic problem is that a module installed via npm or even just in a folder in /plugins needs some sort of default config.

The big problem for coding this is that the code currently is given a full path to the config, so deciding to load something different is rather hard, because the code isn't written for this scenario so it'll be a big change, and config.get() is kinda super ultra critical to Haraka working :)

The questions are how the override works. The perfect scenario in my mind is:

For json/ini/yaml config: core config is your defaults. Custom config walks that tree and replaces anything in the custom file.

For flat list/single files, if the file exists it overrides completely.

A simpler version would be override is just if the local file exists, it overrides completely, no matter what.

This will also allow Haraka to stop copying config to /config, which will also make the updates to tlds/suffixes etc easier.

Thoughts?

msimerson commented 8 years ago

Another issue is that NPM modules can be loaded from $global/node_modules and $global/Haraka/node_modules, depending on whether the plugin was installed with 'npm install -g' or via a Haraka/package.json listing. A plugin might even be loaded installed by another plugin, meaning the possible install paths could be any of:

baudehlo commented 8 years ago

Well no, we don't support any of that.

On Feb 5, 2016, at 8:25 PM, Matt Simerson notifications@github.com wrote:

Another issue is that NPM modules can be loaded from $global/node_modules and $global/Haraka/node_modules, depending on whether the plugin was installed with 'npm install -g' or via a Haraka/package.json listing. A plugin might even be loaded by another plugin, meaning the possible install paths could be any of:

lib/node_modules/ lib/node_modules/Haraka/node_modules lib/node_modules/haraka-plugin-parent/node_modules lib/node_modules/Haraka/node_modules/haraka-plugin-parent/node_modules — Reply to this email directly or view it on GitHub.

baudehlo commented 8 years ago

I take a bit of that back - we support $global/Haraka/node_modules.

The big issue I really need decided right now is what behaviour we want for overrides. That will affect how I write the code, so we need to make a decision on it.

@msimerson @smfreegard

msimerson commented 8 years ago

The big issue I really need decided right now is what behaviour we want for overrides.

I'm not done thinking about this.

thoughts

msimerson commented 8 years ago
msimerson commented 8 years ago
smfreegard commented 8 years ago

our current ini parser doesn't support namespaces ([blah], [blah.second]), which is a desirable feature

Not if you want to use a domain name as a section name e.g. [foo.bar.com] which several plugins already do. If you were going to namespace - you'd have to use a something other than a dot. Personally I don't see the need for that right now anyway.

what "style" of config loading would be more fun to write a haraka-config --plugin $name --setting=value utility for?

If we have to manage all this with a separate haraka-config utility then we've probably made it too complicated.

The big issue I really need decided right now is what behaviour we want for overrides. That will affect how I write the code, so we need to make a decision on it.

I haven't given this a massive amount of thought - off the top of my head though:

1) A npm-haraka-plugin must use a postinstall script https://docs.npmjs.com/misc/scripts which copies the relevant configuration to $HARAKA_DIR/config when it's initially installed. I guess the trick here is how we determine where $HARAKA_DIR is when the module is installed.

2) We could extend plugin.register() to pass it the $HARAKA_DIR thereby allowing the plugin to determine how it should load the configuration itself during register(). e.g. this.config.get() would read $HARAKA_DIR/config and it could require('./configfile'); to read config from it's own directories.

what pattern of config loading would be more fun to write a web GUI for?

I used smtp.json and a separate file which was loaded by my GUI which described each variable along with it's default value and it's type. If I were going to do this again and improve it - I'd modify the plugins so they describe their own variables and defaults, so you could dump it all via something like haraka -c /etc/haraka --dump-config which would output something like:

{
    core: {
         _version: 2.7.3,
        config: {
            main: {
                listen: {
                   default: '[::0]:25',
                   description: 'ip:port to listen on, specify 0.0.0.0:25 for IPv4 only or [::0]:25 for IPv6/IPv4',
                   type: 'text'
                },
                ....
           }
    },
     plugins: {
         'helo.checks': {
             'description': 'Various checks on the HELO/EHLOs used',
             _version: '1.0.0',
             config: {
                 ....
             }
         }
     }
}

That way a separate web GUI could load this JSON file and present all of the config and allow it to be edited - it can then spit out an smtp.json file (e.g. one global for everything) or it could write individual files (I chose the former as it was way easier to manage, plus a lot of my config comes from key->value maps and not via Haraka's config loaders).

When you upgrade Haraka or load additional plugins - you'd simply regenerate the file which would update all the GUI options.

msimerson commented 8 years ago

If I were going to do this again and improve it - I'd modify the plugins so they describe their own variables and defaults

A very excellent point.

more thoughts

There's some really good lessons to be learned from the distributed configuration management systems: etcd / zookeeper and the descendants of launchd (upstart, systemd, relaunchd, etc.):

  1. config management is hard.
  2. the best solutions isolate the data store from the APIs
  3. complex data stores are a mistake (I've read anecdotes from launchd authors with regrets of choosing XML over simpler formats like JSON).

If we had a DSL of sorts, where plugins declared their config settings and defaults like Steve suggested, it would be straight forward to assemble a runtime "config store" where everything that wanted to get/set a config setting could. There's no longer a need for a "default config file," legacy config settings can generate errors or warnings, and changing a config setting can be done via web and/or CLI utilities that update whatever storage mechanism is backing it. Shucks, the config store can be updated during run time with no file watching stuff necessary.

If our app and plugins get their settings with calls such as config.get('plugin.spf.relay.context'), we could put the safety checks in the function call so every single plugin doesn't need to perform if (spf.relay && spf.relay.context !== undefined) type checks to avoid potential run-time exceptions. That could remove an entire class of recurrent bugs from Haraka.

The backing could be a series of config files, a single big JSON file, or even a distributed config store. It doesn't matter so long as our get/set API remains consistent. If the config DSL also has a description property, then the config portions of the documentation could be automatically generated, removing an entire class of bugs (code/docs/config mismatches) from Haraka.

baudehlo commented 8 years ago

On Feb 7, 2016, at 6:19 PM, Matt Simerson notifications@github.com wrote:

If I were going to do this again and improve it - I'd modify the plugins so they describe their own variables and defaults

Moving plugins to npm modules and solving the override issue fixed this problem while still using config files.

baudehlo commented 8 years ago

My current proposal:

Pass the plugin loader two directories: core and local. Local can be undefined.

In git/uninstalled mode: core is Where the module was found (either the main Haraka folder or its node_modules folder). Local is the main Haraka folder, or undefined if the plugin was a plain .js file in ./plugins.

In installed mode: core is where the module was found as above. Local is the install folder unless the plugin was a plain .js file in the installdir/plugins folder, then it can be undefined.

Flat config files are a full override. Other types are a walk-the-tree override.

Thoughts?

On Feb 7, 2016, at 6:19 PM, Matt Simerson notifications@github.com wrote:

If I were going to do this again and improve it - I'd modify the plugins so they describe their own variables and defaults

A very excellent point.

more thoughts

There's some really good lessons to be learned from the distributed configuration management systems: etcd / zookeeper and the descendants of launchd (upstart, systemd, relaunchd, etc.):

config management is hard. the best solutions isolate the data store from the APIs complex data stores are a mistake (I've read anecdotes from launchd authors with regrets of choosing XML over simpler formats like JSON). If we had a DSL of sorts, where plugins declared their config settings and defaults like Steve suggested, it would be straight forward to assemble a runtime "config store" where everything that wanted to get/set a config setting could. There's no longer a need for a "default config file," legacy config settings can generate errors or warnings, and changing a config setting can be done via web and/or CLI utilities that update whatever storage mechanism is backing it. Shucks, the config store can be updated during run time with no file watching stuff necessary.

If our app and plugins get their settings with calls such as config.get('plugin.spf.relay.context'), we could put the safety checks in the function call so every single plugin doesn't need to perform if (spf.relay && spf.relay.context !== undefined) type checks to avoid potential run-time exceptions. That could remove an entire class of recurrent bugs from Haraka.

The backing could be a series of config files, a single big JSON file, or even a distributed config store. It doesn't matter so long as our get/set API remains consistent. If the config DSL also has a description property, then the config portions of the documentation could be automatically generated, removing an entire class of bugs (code/docs/config mismatches) from Haraka.

— Reply to this email directly or view it on GitHub.

msimerson commented 8 years ago

Pass the plugin loader two directories: core and local. Local can be undefined.

I say, "do it." The bigger ideas we have for config are pie-in-the-sky future ideas outside the scope of this issue.

smfreegard commented 8 years ago

I say, "do it." The bigger ideas we have for config are pie-in-the-sky future ideas outside the scope of this issue.

+1

Dexus commented 8 years ago

I say, "do it." The bigger ideas we have for config are pie-in-the-sky future ideas outside the scope of this issue.

+1

baudehlo commented 8 years ago

I created a PR. Let me know what you think of the code - I like how I managed to keep it small (no tests yet though): #1335

msimerson commented 8 years ago

PR merged.