DHCP + sqlpippol test, v2.1.x

[Fajar A. Nugraha]

On Thu, Jan 19, 2012 at 2:36 PM, Alan DeKok <aland at deployingradius.com> wrote:
> Fajar A. Nugraha wrote:
>> I'm testing some modifications to make DHCP able to allocate dynamic
>> IP address using sqlippool. It's only tested with one client, but it
>> should be able to assign dynamic IP address (smallest ip address
>> first, due to sorting). The modification is purely config files
>> addition/modification, no source code change. Current version is here:
>> https://github.com/fajarnugraha/freeradius-server/commit/8af7142
>
>  It looks very nice.  I'm a bit surprised that it's that simple.

Glad to hear it :D
Most of my changes are simple ones,

>
>> Static IP allocation SHOULD also be possible by having expiration time
>> far in the future (e.g. 2100-01-01 00:00:00) on radippool table, and
>> modifying the query to NOT overwrite the expiration time on that
>> scenario. This hasn't been implemented though.
>>

I have updated the code so that static IP assignment works fine as
well by setting expiration in the future. The only essential change to
the last one is sql query change:

https://github.com/fajarnugraha/freeradius-server/commit/b657ce3#diff-6

>> Any thoughts on these changes? Perhaps there a simpler way to implement it.
>
>  The changes could be simpler.  A lot of the code in policy.conf is
> site-specific, and doesn't need to go into the server core.  My $0.02:

Yes. Which brings me to two other questions:
1) dhcp functionality is a good example that uses policy. It SHOULD be
site specific, but currently all policies must be defined on
policy.conf. So currently I must either:
a. write the same block twice (as in your original
sites-available/dhcp example. OR
b. put all changes on policy.conf. OR
c. Implement something like polcies directory (or, following the
recent changes on modules, policies-available and policies-enabled).

Option (1.c) would be cleaner, but we ended up with many of -enabled
directory. Especially since the next question also needs that.
Currently I use (1.b)

2) I need a place to store centralized configuration variable, where
the same variable is used by policy, module, and sites. It can be:
a. write the same value many times. Not good.
b. edit radiusd.conf (or policy.conf) and place it there. Messy.
c. create a new file included by radiusd.conf just before modules
section. Works, but it still requires adding 1 line to radiusd.conf.
d. Implement configs-available and configs-enabled directory, loaded
just before modules section on radiusd.conf.

In this code update I use (2.c.) to store dhcp-related configuration
variables (config-dhcp.conf). Option (2.d) would've been much cleaner.

Current version of dhcp config code (with static IP, central config,
and comment changes), diff against v2.1.x:
https://github.com/fajarnugraha/freeradius-server/commit/fdff057

... or diff against the previous version that you reviewed:
https://github.com/fajarnugraha/freeradius-server/commit/b657ce3

>
> dhcp.sqlippool_request
>  - this is good, it "patches" the DHCP packet so that the SQLIPPool
>   module understands it
>
> dhcp.sqlippool_reply
>  - this is good, too

I wasn't sure this approach was acceptable. Glad you like it.

>
>  The sites-available/dhcp-sqliipool text is not necessary.  Just put
> the sqlippool examples into the existing DHCP virtual server.

The original intention was to push the changes in my ppa build as an
alternative example, leaving the existing example untouched. If both
examples can be merged then it's great.

>
>  The SQL sample configurations for MySQL are good.
>
>  I'll pull part of it into the v2.1.x branch.  Please check that in a
> day or so, to see what needs to be done in order for it to work.

Please also take a look at my last update, at least the sql change
part since it enables static IP assignment.

As a side note, I noticed you closed pull request
https://github.com/alandekok/freeradius-server/pull/40 , but it hasn't
been merged in v2.1.x. Without that, debian package build still fails.
Are you planning to commit an alternate fix? This is different btw
from pull request #41, which you've applied (thanks for that).

-- 
Fajar