Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support netmask/gateway for file plugin #147

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

skoef
Copy link
Contributor

@skoef skoef commented Nov 29, 2021

In my setup, I have multiple ranges on the same network. I'm using the file plugin to give each host a static IP and rely on the netmask and router plugin to set the right netmask/gateway to the DHCP offer. The config looks like this:

leases_1.txt:

00:11:22:33:44:55 10.0.0.100

leases_2.txt:

11:22:33:44:55:66 10.1.0.150

config.yaml:

server4:
  plugins:
    - netmask: 255.255.255.0
    - router: 10.0.0.1
    - file: leases_1.txt
    - netmask: 255.255.255.128
    - router: 10.1.0.129
    - file: leases_2.txt

So a request from 00:11:22:33:44:55 gets a netmask set to 255.255.255.0 and router to 10.0.0.1 and then get the address set by the file plugin. This plugin then tells coredhcp to stop processing this request as it has found a match.

A request from 11:22:33:44:55:66 gets a netmask set to 255.255.255.0, router to 10.0.0.1. The file plugin for leases_1.txt does not find a match. The netmask is updated to 255.255.255.128, router to 10.1.0.129 and then the file plugin for leases_2.txt finds a match and the processing of this request is done.

While this works, configuring this can become quite verbose when you have more than a couple of ranges in the same network. This PR allows to specify the netmask and gateway for IPv4 records directly in the leases file (IPv6 leases get their prefixes from router advertisements) so the configuration would become:

leases.txt:

00:11:22:33:44:55 10.0.0.100 255.255.255.0 10.0.0.1
11:22:33:44:55:66 10.1.0.150 255.255.255.128 10.1.0.129

config.yaml:

server4:
  plugins:
    - file: leases.txt

Since LoadDHCPv4Records and LoadDHCPv6Records were almost identical by now, I merged them into loadDHCPRecords while at it.

@codecov
Copy link

codecov bot commented Nov 29, 2021

Codecov Report

Merging #147 (20e149d) into master (dcca4ae) will decrease coverage by 0.42%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
- Coverage   44.74%   44.31%   -0.43%     
==========================================
  Files          15       15              
  Lines         789      783       -6     
==========================================
- Hits          353      347       -6     
  Misses        389      389              
  Partials       47       47              
Flag Coverage Δ
unittests 44.31% <100.00%> (-0.43%) ⬇️

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

Impacted Files Coverage Δ
plugins/file/plugin.go 84.68% <100.00%> (-1.39%) ⬇️
plugins/netmask/plugin.go 100.00% <100.00%> (ø)

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 dcca4ae...20e149d. Read the comment docs.

Signed-off-by: Reinier Schoof <reinier@skoef.nl>
@Natolumin
Copy link
Member

Sorry it took me a while to get back to this. I have a few opinions but generally think this is a feature we want, but we need to make it easier to use.

  • Specifying options with space-separated strings is already a mess in every other place we use it, and this is showing it too as soon as there are more than 2 items per line: you need to remember the order things come in and you can't skip any unless they're at the end, so it's fairly painful and error-prone imo
  • I think the file plugin should not be made for production/complex uses. What inevitably happens is that it becomes an intermediate step and people will write code to generate the file and feed it to coredhcp; that is something we actively don't want and should be a separate plugin instead.
    What that means to me is that the format of the input to the file plugin should be first and foremost optimized for reading and writing as a human, for example something like yaml would be fairly adequate I think
  • Then, thinking about file as a casual/experimental/manual way of specifying configuration, it'd be nice to be able to specify any arbitrary option (at least for ipv4 where they fall into a couple predefined types and we have support for in the dhcp library) in similar ways that other dhcp servers let you, so that we don't need to hardcode support for the gateway/netmask.
  • I'd rather not arbitrarily break compatibility with the existing format as we change things though, so we probably want to keep supporting the simple key-value existing format. But then I doubly don't want to add additional options to it to avoid removing them later.
    That might be not be a reasonable point, we've never done any numbered release yet and we don't make any compatibility promises; especially since this is a more advanced feature it's probably not going to have many users anyway

Note these aren't instructions for what to do here, just me writing out what makes me uncomfortable in this approach. You've probably been using coredhcp more than I have so your feedback would definitely be helpful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants