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

waynegemmell · 2022-02-24T19:53:57Z

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
[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

myii

@waynegemmell Thanks for the PR. Still some work needed here.

salt-lint violation needs to be resolved.
- https://gitlab.com/saltstack-formulas/dhcpd-formula/-/jobs/2136040745#L99

[201] Trailing whitespace
dhcpd/files/subnet.jinja:82
    {%- for range_def in pool.range %}

Test(s) will need to be updated, since all the instances are failing.
- https://gitlab.com/saltstack-formulas/dhcpd-formula/-/pipelines/478940491
It would be helpful if you could link to the upstream docs, showing how this is supposed to be defined.

Pools should allow multiple ranges. It's in the docs but doesn't work in practice.

waynegemmell · 2022-02-25T08:33:18Z

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

myii · 2022-02-25T08:48:35Z

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:

dhcpd-formula/test/integration/default/controls/config_spec.rb

Lines 187 to 194 in 7ba856e

    
                     pool { 
        
                       allow members of "foo"; 
        
                       range 10.17.224.10 10.17.224.250; 
        
                     } 
        
                     pool { 
        
                       deny members of "foo"; 
        
                       range 10.0.29.10 10.0.29.230; 
        
                     }

Unless you see something I've missed.

Other references:

waynegemmell · 2022-02-25T09:37:41Z

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 · 2022-02-25T14:44:01Z

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

dhcpd-formula/pillar.example

Lines 44 to 54 in 7ba856e

    
             subnets: 
        
               10.152.187.0: 
        
                 comment: |- 
        
                   No service will be given on this subnet, but declaring it helps the 
        
                   DHCP server to understand the network topology. 
        
                 netmask: 255.255.255.0 
        
                 pools: 
        
                   - failover_peer: dhcp-failover 
        
                     range: 
        
                       - 10.152.187.1 
        
                       - 10.152.187.254

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 · 2022-02-28T08:52:46Z

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 · 2022-02-28T09:41:40Z

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

myii · 2022-02-28T16:06:35Z

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 9115b05, 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 · 2022-02-28T16:10:20Z

@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:

dhcpd-formula/pillar.example

Lines 52 to 54 in 7ba856e

    
           range: 
        
             - 10.152.187.1 
        
             - 10.152.187.254

dhcpd-formula/pillar.example

Lines 59 to 61 in 7ba856e

    
           range: 
        
             - 10.254.239.10 
        
             - 10.254.239.20

dhcpd-formula/pillar.example

Lines 72 to 74 in 7ba856e

    
           range: 
        
             - 10.254.239.40 
        
             - 10.254.239.60

dhcpd-formula/pillar.example

Lines 81 to 83 in 7ba856e

    
           range: 
        
             - 10.5.5.26 
        
             - 10.5.5.30

dhcpd-formula/pillar.example

Lines 152 to 161 in 7ba856e

    
           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

waynegemmell · 2022-03-01T09:28:08Z

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 · 2022-03-01T10:02:05Z

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 · 2022-03-01T11:01:22Z

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 · 2022-03-01T19:28:50Z

@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 fix(subnet.jinja): multiple ranges allowed in pool as per docs #59 (comment).
- 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 · 2022-03-03T11:51:36Z

@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 · 2022-03-03T12:13:59Z

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

myii · 2022-03-03T21:12:10Z

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

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

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

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

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:

dhcpd-formula/pillar.example

Lines 59 to 61 in 7ba856e

    
           range: 
        
             - 10.254.239.10 
        
             - 10.254.239.20

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 · 2022-03-04T14:02:34Z

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 · 2022-03-10T16:21:25Z

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 · 2022-03-17T14:35:53Z

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

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

1451d8f

waynegemmell requested a review from sticky-note as a code owner February 24, 2022 19:53

myii requested changes Feb 25, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

waynegemmell commented Feb 24, 2022 •

edited

Loading

myii left a comment •

edited

Loading

waynegemmell commented Feb 25, 2022

myii commented Feb 25, 2022 •

edited

Loading

waynegemmell commented Feb 25, 2022

myii commented Feb 25, 2022

waynegemmell commented Feb 28, 2022

waynegemmell commented Feb 28, 2022

myii commented Feb 28, 2022 •

edited

Loading

myii commented Feb 28, 2022 •

edited

Loading

waynegemmell commented Mar 1, 2022

myii commented Mar 1, 2022

waynegemmell commented Mar 1, 2022

myii commented Mar 1, 2022

myii commented Mar 3, 2022

waynegemmell commented Mar 3, 2022

myii commented Mar 3, 2022

waynegemmell commented Mar 4, 2022

myii commented Mar 10, 2022

waynegemmell commented Mar 17, 2022

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

Are you sure you want to change the base?

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

Conversation

waynegemmell commented Feb 24, 2022 • edited Loading

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

Related issues and/or pull requests

Describe the changes you're proposing

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

Testing checklist

Additional context

myii left a comment • edited Loading

Choose a reason for hiding this comment

waynegemmell commented Feb 25, 2022

myii commented Feb 25, 2022 • edited Loading

waynegemmell commented Feb 25, 2022

myii commented Feb 25, 2022

waynegemmell commented Feb 28, 2022

waynegemmell commented Feb 28, 2022

myii commented Feb 28, 2022 • edited Loading

myii commented Feb 28, 2022 • edited Loading

waynegemmell commented Mar 1, 2022

myii commented Mar 1, 2022

waynegemmell commented Mar 1, 2022

myii commented Mar 1, 2022

myii commented Mar 3, 2022

waynegemmell commented Mar 3, 2022

myii commented Mar 3, 2022

Examples using lists

Examples using strings

waynegemmell commented Mar 4, 2022

myii commented Mar 10, 2022

waynegemmell commented Mar 17, 2022

waynegemmell commented Feb 24, 2022 •

edited

Loading

Does this PR introduce a `BREAKING CHANGE`?

myii left a comment •

edited

Loading

myii commented Feb 25, 2022 •

edited

Loading

myii commented Feb 28, 2022 •

edited

Loading

myii commented Feb 28, 2022 •

edited

Loading