On Thu, Jan 19, 2012 at 2:36 PM, Alan DeKok <aland@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