Remove global state

[Adphi]

One of the many good things of coredns is how easy it is to embed it into another program.
Coredhcp could benefit to do the same thing by removing all global state (global variables) and expose a NewServer method.
It could also increase project adoption among developers.

[Adphi]

One part seems to be covered by #159

[Natolumin]

That's pretty vague, so I tried to look for specific uses that our current global state would prevent when embedding and problems it might cause when embedded. If I've missed some or if you have specific ideas in mind please feel free to send your own items too. Also one specific usecase of embedding could be #91

• Global plugin state (the bit addressed by plugins: each plugin instance now has its own state #159) ties the lifetime of coredhcp to the program it runs in (or specifically, the end of the lifetime. You don't have to start coredhcp with the program, it can be initialized later with plugins.registerPlugins and server.Start). It prevents shutting down the server and starting a new one in the same execution, or running multiple concurrent servers, but that's just as true for coredhcp as it is now.
  I think this one is definitely desirable regardless of embedding. It would unlock some usecases - notably running multiple servers with some shared state between them but not all, and for dynamic configuration reload. Some newer plugins already do that, and updating the others is a good idea too.
• logging is also globally defined and prefixes etc are per-file rather than per-plugin-instance, so it has the same issue - changing this one I don't think would bring anything to coredhcp, but if there's a not-too-clunky way of addressing it that plays well with embedders, why not
• The list of known plugins is global

  coredhcp/plugins/plugin.go

  Line 27 in a2552c5

  var RegisteredPlugins = make(map[string]*Plugin)

  Since go makes it very hard to load and (even worse) unload code at runtime, this seems harmless for embedding, once the code for a plugin is in the program it's fine for it to exist in that registry

Outside of the specific technical issue of global state, there are I think other questions that need to be answered first to understand what "coredhcp embedded in another executable" would look like and what interface the embedder program would use.

This is all pretty unspecified, and this discussion would benefit a lot from a concrete usecase in mind before we start making large changes without knowing why

Finally please note that right now in the current state of coredhcp, it definitely should not be widely used, and I'm not trying to encourage adoption and especially not embedding (we provide 0 API stability guarantees)

[Adphi]

I was developing a small home-router with several network interfaces including a wifi one, so I needed a dhcp server.
As I was already using coredns, I would have liked to use coredhcp as well, just to stay in the "core" spirit 😀.
In fact, it's mostly to avoid implementing it myself.
I ended up taking the handlers of the plugins I needed and re-implementing the server to support hot reloading and multiple interfaces.

All it needs is to add context.Context support and remove plugins global state. Stopping / reloading the server is done through context cancellation. The logger is passed through the context, and if the plugin needs global state accros servers, it could also be passed (synchronised) through the context.

[dulitz]

I've used coredhcp on smaller networks and am adding a number of plugins so it can be used more easily in larger ones. (I have it on good authority that it's already used in at least one larger installation that hasn't upstreamed.) I think part of the "core" (coredns, coredhcp) philosophy that appeals is that you can easily understand what it's doing. In both cases there's a widely used "kitchen sink" system out there (ISC BIND, ISC DHCP) but if it doesn't already have the config flag you need, you won't have an easy time. coredhcp is the opposite: clear design, readable, easy to change without introducing hidden errors.

I'd be interested to know @Natolumin why you wouldn't want coredhcp to be widely used.

I don't have a use case for embedding coredhcp in another executable. I think one thing that would be useful though is to separate the creation of the handler stack from the establishment of the listeners. Motivate it with dynamic config reload: it would be nice to load a brand spanking new handler stack (with its own config) and then atomically direct all packets from a listener set to the new handler stack, without having to possibly drop packets by closing the old listener sockets and opening new ones.

Any packets that arrived previously would simply keep traversing the old handler stack until they were done.

Does that sound desirable and concrete?

[Natolumin]

I'd be interested to know @Natolumin why you wouldn't want coredhcp to be widely used.

It's abandoned, in the sense that I'm (afaik) the most active maintainer (which is saying a lot already), but I don't have ownership of the repo. I can somewhat merge other people's PRs but I can't put any of my own code in, or appoint new maintainers.

I like your dynamic reload ideas, you'll note that in my original message I mentioned exactly that usecase :)

[dulitz]

It's abandoned

That's a good reason.

It's also a shame, because there are a lot of good ideas here.

I've invested a bunch of time in making coredhcp viable for our use case. (I have 3-4 PRs teed up after the initial one.) I'm willing to do more, but I don't want to if it's a dead end, and we have to decide before too long.

We're looking at building coredhcp into a DHCP snooping application that would be used with SONiC, the open source switching/routing project. Coredhcp's plugin stack design is a good match for the use case. But it's hard to make the case for it if it's abandoned. I welcome your thoughts.

[mpenning]

It's abandoned, in the sense that I'm (afaik) the most active maintainer (which is saying a lot already), but I don't have ownership of the repo. I can somewhat merge other people's PRs but I can't put any of my own code in, or appoint new maintainers.

@Natolumin if there are enough people committed to maintaining a very extensible go dhcpd, the license is MIT... so fork it, change the name and keep building...