plugin refactor and refresh on SIGHUP

[skoef]

rewrote Plugin object to an interface that every plugin needs to implement

this allows for keeping arbitrary state inside the plugin that can be leveraged
in the refresh functions that are called on SIGHUP

NOTE: this PR is merely an idea, but I wanted to propose reimplementing the plugin structure.

[codecov]

Codecov Report

Merging #143 (50419d6) into master (17ea625) will decrease coverage by 0.55%.
The diff coverage is 27.50%.

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
- Coverage   41.85%   41.29%   -0.56%     
==========================================
  Files          14       14              
  Lines         743      770      +27     
==========================================
+ Hits          311      318       +7     
- Misses        387      407      +20     
  Partials       45       45              

Flag	Coverage Δ
unittests	41.29% <27.50%> (-0.56%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
plugins/range/plugin.go	0.00% <0.00%> (ø)
plugins/prefix/plugin.go	47.57% <25.00%> (-0.92%)	⬇️
plugins/staticroute/plugin.go	68.96% <25.00%> (-7.04%)	⬇️
plugins/dns/plugin.go	26.66% <33.33%> (+0.74%)	⬆️
plugins/searchdomains/plugin.go	90.00% <33.33%> (-10.00%)	⬇️
plugins/serverid/plugin.go	18.51% <33.33%> (+0.56%)	⬆️
plugins/file/plugin.go	83.33% <50.00%> (-3.13%)	⬇️

---

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 17ea625...50419d6. Read the comment docs.

[skoef]

Apart from the whole SIGHUP/refresh feature, the refactor of the plugin into an interface seems like a good idea to me anyway. That way you can easily keep state of the instance of the plugin instead of the plugin globally.

For instance the DNS plugin has two global variables, dnsServers6 and dnsServers4 to keep track of the list of DNS servers given as arguments to the plugin for DHCPv6 and DHCPv4 respectively. This can be replaced with a dnsServers field to the Plugin struct. Perhaps in the future it could be possible to load plugins more than once for DHCPv4 and once for DHCPv6, for instance if you would use several lease files or multiple ranges. That wouldn't be possible in the current structure of the plugins, but it would after the rewrite.

Also, I can see a ratelimit plugin happening easily this way, the instance of the plugin could keep track of requests per MAC address and you wouldn't have to create a global variable per plugin instance for that.

[Natolumin]

I'm not really convinced by the plugin-as-interface refactor at this point

Apart from the whole SIGHUP/refresh feature, the refactor of the plugin into an interface seems like a good idea to me anyway. That way you can easily keep state of the instance of the plugin instead of the plugin globally.

That's already possible! There are 2 different things here: Plugin is somewhat misnamed, and contains functions/information/state for plugin initialization. The plugin runtime state is actually stored in the handler functions.
For example, see the prefix plugin, where every time you call a setup function you'll get a separate instance of the plugin: https://github.com/coredhcp/coredhcp/blob/master/plugins/prefix/plugin.go#L81

A more useful refactor here imo would be to make the handlers returned by the setup functions, either concrete structs or interfaces rather than just a single function

For instance the DNS plugin has two global variables, dnsServers6 and dnsServers4 to keep track of the list of DNS servers given as arguments to the plugin for DHCPv6 and DHCPv4 respectively. This can be replaced with a dnsServers field to the Plugin struct. Perhaps in the future it could be possible to load plugins more than once for DHCPv4 and once for DHCPv6, for instance if you would use several lease files or multiple ranges.

Absolutely agree, that's been a desired improvement for a while

That wouldn't be possible in the current structure of the plugins, but it would after the rewrite.

It is already possible (see above / the prefix plugin). I'd argue the current change doesn't make it easier at all: in main, you now need to create the plugin objects, before parsing the configuration but they now also hold the plugin instance state. To spawn the same plugin twice if the per-plugin Plugin object holds state, you now need to create the Plugin objects multiple times; instead of the current state where just calling setup[46] multiple times for a plugin that supports it will give you independent instances