fix(subnet.jinja): multiple ranges allowed in pool as per docs

waynegemmell commented 2 years ago

PR progress checklist (to be filled in by reviewers)

[ ] Changes to documentation are appropriate (or tick if not required)
[ ] Changes to tests are appropriate (or tick if not required)
[ ] Reviews completed

What type of PR is this?

Primary type

[ ] [build] Changes related to the build system
[ ] [chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
[ ] [ci] Changes to the continuous integration configuration
[ ] [feat] A new feature
[x] [fix] A bug fix
[ ] [perf] A code change that improves performance
[ ] [refactor] A code change that neither fixes a bug nor adds a feature
[ ] [revert] A change used to revert a previous commit
[ ] [style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

[ ] [docs] Documentation changes
[ ] [test] Adding missing or correcting existing tests

Does this PR introduce a `BREAKING CHANGE`?

No.

Related issues and/or pull requests

None

Describe the changes you're proposing

Pools should allow multiple ranges. It's in the docs but doesn't work in practice. I've just added a for loop to fix that.

Pillar / config required to test the proposed changes

From pillar.example

  pools:
    # And no, those quotation marks won't get stripped:
    - allow: members of "foo"
      range:
        - 10.17.224.10
        - 10.17.224.250
    - deny: members of "foo"
      range:
        - 10.0.29.10
        - 10.0.29.230

Debug log showing how the proposed changes work

Documentation checklist

[ ] Updated the README (e.g. Available states).
[ ] Updated pillar.example.

Testing checklist

[ ] Included in Kitchen (i.e. under state_top).
[ ] Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
[ ] Updated the relevant test pillar.

Additional context

waynegemmell commented 2 years ago

1) Will sort trivial whitespace issue asap. 2) I'm not sure about the tests. I'll have a look. 3) The documentation in question is the pillar.example. It's already there.

myii commented 2 years ago

Will sort trivial whitespace issue asap.

I'm not sure about the tests. I'll have a look.

@waynegemmell Might not be necessary, see below.

The documentation in question is the pillar.example. It's already there.

I meant the upstream documentation. I've searched for it myself:

https://kb.isc.org/v1/docs/isc-dhcp-44-manual-pages-dhcpdconf#examples

...

subnet 204.254.239.0 netmask 255.255.255.224 {
  subnet-specific parameters...
  range 204.254.239.10 204.254.239.30;
}

subnet 204.254.239.32 netmask 255.255.255.224 {
  subnet-specific parameters...
  range 204.254.239.42 204.254.239.62;
}

subnet 204.254.239.64 netmask 255.255.255.224 {
  subnet-specific parameters...
  range 204.254.239.74 204.254.239.94;
}

...

So it looks like the formula is already working as expected:

https://github.com/saltstack-formulas/dhcpd-formula/blob/7ba856e805e7ad4e907b11e1f1de199f9898d6cc/test/integration/default/controls/config_spec.rb#L187-L194

Unless you see something I've missed.

Other references:

waynegemmell commented 2 years ago

For one thing, I have that in production. The definition is as follows.

The range statement

range [ dynamic-bootp ] low-address [ high-address];

For any subnet on which addresses will be assigned dynamically, there must be at least one range statement. The range statement gives the lowest and highest IP addresses in a range. All IP addresses in the range should be in the subnet in which the range statement is declared. The dynamic-bootp flag may be specified if addresses in the specified range may be dynamically assigned to BOOTP clients as well as DHCP clients. When specifying a single address, high-address can be omitted.

So at least one, which to me implies more than one . I do this in practice and it works fine.

myii commented 2 years ago

@waynegemmell Perhaps this is easier to understand with a comparison. Taking the block from pillar.example, which is used in the CI here:

https://github.com/saltstack-formulas/dhcpd-formula/blob/7ba856e805e7ad4e907b11e1f1de199f9898d6cc/pillar.example#L44-L54

Before this PR, this results in:

subnet 10.152.187.0 netmask 255.255.255.0 {
  pool {
    failover peer "dhcp-failover";
    range 10.152.187.1 10.152.187.254;
  }
}

After this PR, we get:

subnet 10.152.187.0 netmask 255.255.255.0 {
  pool {
    failover peer "dhcp-failover";
    range 10.152.187.1; 
    range 10.152.187.254;
  }
}

Taking what you've quoted above:

range [ dynamic-bootp ] low-address [ high-address];

Then we can see that the range isn't being set correctly, if this PR was merged.

However, there is something that still needs to be fixed. The current implementation does not allow for a range to be set using only a low-address. Say we wanted the following instead:

subnet 10.152.187.0 netmask 255.255.255.0 {
  pool {
    failover peer "dhcp-failover";
    range 10.152.187.1; 
  }
}

So we could adjust this PR in order to provide that functionality, depending on how the data is provided in the pillar/config. There are two obvious ways to deal with this.

Allow a single entry (low-address only) in the list, e.g.:

       pools: 
         - failover_peer: dhcp-failover 
           range: 
             - 10.152.187.1

Or avoid using a list when there's only a low-address required:

       pools: 
         - failover_peer: dhcp-failover 
           range: 10.152.187.1

I've seen the second method used in other formulas and it seems more explicit (string vs. list).

waynegemmell commented 2 years ago

I definitely think the string is the way to go with this, mostly to cater for all the possible variation that you mentioned. TBH I implemented it as a string on my side and that's why I was quite certain that it worked fine. My only worry with changing this to a string is that it's a breaking change.

waynegemmell commented 2 years ago

Having it in a list really seems like the hard way to do it.

myii commented 2 years ago

Having it in a list really seems like the hard way to do it.

@waynegemmell This method was introduced ~8 years ago in #4.

In fact, looking at 9115b0541f0f9fec48fef9f4da3b3aa820b19ad2, you can see that the intent was to replace the string-based method with the list-based method.

I definitely think the string is the way to go with this, mostly to cater for all the possible variation that you mentioned. TBH I implemented it as a string on my side and that's why I was quite certain that it worked fine. My only worry with changing this to a string is that it's a breaking change.

As I mentioned above, there's another way that's used in other formulas. Allow the end user to supply the data as a string or a list; process it accordingly using an if block. That has the additional benefit of avoiding a breaking change, so no one has to change their existing pillar/config files. Furthermore, by allowing a string-based method, we can supply a range with only a low-address. Basically, it should solve all of the problems that have been raised by this PR.

Try out this block:

    {%- if 'range' in pool %}
    {%-   if pool.range | is_list %}
    range {{ pool.range[0] }} {{ pool.range[1] }};
    {%-   else %}
    range {{ pool.range }};
    {%-   endif %}
    {%- endif %}

Then you can supply the data in any of the following forms.

Existing method using a list:

       pools: 
         - failover_peer: dhcp-failover 
           range: 
             - 10.152.187.1
             - 10.152.187.254

The same result by using a string:

       pools: 
         - failover_peer: dhcp-failover 
           range: 10.152.187.1 10.152.187.254

Using a string to provide only a low-address:

       pools: 
         - failover_peer: dhcp-failover 
           range: 10.152.187.1

If this works out for you, we can finalise all the updates required to pillar.example and the InSpec tests -- we should test all three cases.

myii commented 2 years ago

@waynegemmell If the method above works, for consistency, we should allow list/string configuration for all the places that the range can be set, such as:

https://github.com/saltstack-formulas/dhcpd-formula/blob/7ba856e805e7ad4e907b11e1f1de199f9898d6cc/pillar.example#L52-L54

https://github.com/saltstack-formulas/dhcpd-formula/blob/7ba856e805e7ad4e907b11e1f1de199f9898d6cc/pillar.example#L59-L61

https://github.com/saltstack-formulas/dhcpd-formula/blob/7ba856e805e7ad4e907b11e1f1de199f9898d6cc/pillar.example#L72-L74

https://github.com/saltstack-formulas/dhcpd-formula/blob/7ba856e805e7ad4e907b11e1f1de199f9898d6cc/pillar.example#L81-L83

https://github.com/saltstack-formulas/dhcpd-formula/blob/7ba856e805e7ad4e907b11e1f1de199f9898d6cc/pillar.example#L152-L161

waynegemmell commented 2 years ago

Sounds good. Can we add a ranges variable as well? It would have a list of string ranges so I can still achieve what I set out to achieve.

myii commented 2 years ago

Sounds good. Can we add a ranges variable as well? It would have a list of string ranges so I can still achieve what I set out to achieve.

@waynegemmell Would you mind providing an example of what you would like as the end (rendered) result? Based on the following snippet:

subnet 10.152.187.0 netmask 255.255.255.0 {
  pool {
    failover peer "dhcp-failover";
    range 10.152.187.1 10.152.187.254;
  }
}

Then we can figure out how the data should be provided and processed.

waynegemmell commented 2 years ago

It goes something like this

subnet 10.152.0.0 netmask 255.255.0.0 { pool { failover peer "dhcp-failover"; range 10.152.187.1 10.152.187.254; range 10.152.188.1 10.152.188.254; range 10.152.189.1 10.152.189.254; } }

myii commented 2 years ago

@waynegemmell Thanks for that example. I've also found this in the docs:

Multiple address ranges may be specified like this:

    subnet 239.252.197.0 netmask 255.255.255.0 {
      range 239.252.197.10 239.252.197.107;
      range 239.252.197.113 239.252.197.250;
    }

Reverse compatibility and multiple address ranges is now a step too far for any simple solution. Other than introducing TOFS, I reckon the cleanest solution is something like this:

Use a new ranges key, to deal with multiple address ranges.
- This will be looped through in the template, to tackle each single range using the macro that follows.
Add a macro at the top of the template, to work with any single range line to be output.
- Essentially, the if block I suggested in https://github.com/saltstack-formulas/dhcpd-formula/pull/59#issuecomment-1054411487.
- Also consider the dynamic_bootp value in this macro.
- This macro will be used for all the range lines that can possibly be configured.

I've got more to share but I'm short on time right now. Feel free to share any feedback that you have.

myii commented 2 years ago

@waynegemmell I've put something together using macros for range and ranges, appears to be working fine, reverse-compatible as well. How would you prefer to test this out? I can push it to a branch in my fork or I could force-push it to this PR's branch. Which do you prefer?

waynegemmell commented 2 years ago

Maybe just make a branch your side? I'll have a look asap. Slammed today. Hopefully tomorrow I can get back onto this.

myii commented 2 years ago

@waynegemmell This is the branch (just one commit added):

https://github.com/myii/dhcpd-formula/tree/feat/allow-multiple-ranges
- https://github.com/myii/dhcpd-formula/commit/02edec94b425830d186e95d465921010a23ea00d

This covers all the possible range-related configuration items, as mentioned in the comment above:

https://github.com/saltstack-formulas/dhcpd-formula/pull/59#issuecomment-1054415344

I haven't updated the pillar.example or tests just yet, simply to show that this method is still completely reverse compatible:

https://gitlab.com/myii/dhcpd-formula/-/pipelines/484075701

Firstly, the configuration examples from the comment above are all possible:

Existing method using a list:

       pools: 
         - failover_peer: dhcp-failover 
           range: 
             - 10.152.187.1
             - 10.152.187.254

The same result by using a string:

       pools: 
         - failover_peer: dhcp-failover 
           range: 10.152.187.1 10.152.187.254

Using a string to provide only a low-address:

       pools: 
         - failover_peer: dhcp-failover 
           range: 10.152.187.1

The interesting addition is the ranges key, which can be used to provide multiple ranges. In fact, ranges can be used to do all of what range does, so one could use this for everything going forward, even when only a single range is required. If both ranges and range keys are present, the former will take precedence over the latter.

All the examples below are based on this block:

https://github.com/saltstack-formulas/dhcpd-formula/blob/7ba856e805e7ad4e907b11e1f1de199f9898d6cc/pillar.example#L59-L61

Examples using lists

Where the basic range is:

      range:
        - 10.254.239.10
        - 10.254.239.20

This is identical using ranges:

      ranges:
        -
          - 10.254.239.10
          - 10.254.239.20

And for multiple ranges:

      ranges:
        -
          - 10.254.239.10
          - 10.254.239.14
        -
          - 10.254.239.15
          - 10.254.239.20

Examples using strings

Where the basic range is:

      range: 10.254.239.10 10.254.239.20

This is identical using ranges:

      ranges:
        - 10.254.239.10 10.254.239.20

And for multiple ranges:

      ranges:
        - 10.254.239.10 10.254.239.14
        - 10.254.239.15 10.254.239.20

waynegemmell commented 2 years ago

Hi, it looks good. For some reason the config check is breaking. If I run the check manually it's fine and if I remove the check everything is fine. I'll have another look Monday. Thanks for the effort.

myii commented 2 years ago

Hi, it looks good. For some reason the config check is breaking. If I run the check manually it's fine and if I remove the check everything is fine. I'll have another look Monday. Thanks for the effort.

@waynegemmell Did you manage to have another look? The only config check failures I faced were when I mistakenly tested ranges that were outside the relevant subnet/netmask. Perhaps something similar for you in your tests?

waynegemmell commented 2 years ago

It works well. Thanks. My issue was a slight config issue which I have corrected. Apologies.

saltstack-formulas / dhcpd-formula