Make FreeRADIUS 3.0.21 the target version for config

[nward]

The current configuration templates are taken from a mixture of FreeRADIUS 3.0.4, and the experimental 3.1.x branch.

This change updates the templates to match the configuration for FreeRADIUS 3.0.21 across the board.

This is marked as WIP as there is an open question in item 3 below.

In doing so, there are some changes:

1. Change the optional style, so that the documentation for an option, and generally the documented example, are included irregardless of whether the option is enabled or not. FreeRADIUS leans quite heavily on using the configuration as documentation, so we should try and preserve this so engineers looking at Puppet built systems can still see what options are available even if they are not set (or if the default is set).
2. Take the tabs/space cues from FreeRADIUS - this makes some stuff inconsistent, but I think copying FreeRADIUS makes it easier to update the configuration to target future FreeRADIUS versions.
3. Remove options which are specific to v3.1.x. This is a breaking change and I can see this being contentious. I'm proposing this here as FreeRADIUS 3.1.x is an experimental branch and in theory shouldn't be in production so a puppet module should not need to target it. I think that should be the general philosophy here, however, given these options already exist if there is objection here then I propose that I write some conditions to allow these only if v3.1.x is requested.

[djjudas21]

I'm away from home for a couple of days so I won't get to review this properly until tomorrow, but I just wanted to say re: point 3, I don't think there is a problem with breaking changes provided we tag a new release with a new major version to reflect this.

As far as I recall, I only built in support for 3.1.x because at the time it was the only branch that had fixes for some very specific bugs I reported to the FreeRADIUS project, so we were running this in production. I'm not sure how many other organisations would be using it, but I think there was quite a lot of chatter about 3.1.x at the time so I wouldn't be surprised if there were several. Off the top of my head I can think of several universities and hospitals. I think it would be worth keeping the support for 3.1.x but toggling with optional switches etc. It would be fine to introduce new switches or change defaults if we do this in a major release bump.

[nward]

Yep that makes sense.

I had a bit more of a think about this over the weekend as well - I think what makes the most sense is to include those configuration options and for now but them generate a warning if the specified or detected FR version is not 3.1.
The warning would be something like This configuration option is not supported in FreeRADIUS <version>. In the future attempting to configure it may fail..

I was thinking it might make sense to be able to pass a hash or array of "extra" options to allow for special cases. puppet/nginx has this with their *_cfg_append options and it works well. That might be a good place to move the 3.1.x options "functionality" to as part of a major version bump? I can create a new issue with some sort of "future" tag to track that if that sounds like a useful thing.

[nward]

I've given this a go as described - I've added this warning, and in addition I've made each of the params only show in the config if it is explicitly set, OR if we detect that we are on FR3.1.x and there is a non-undef default. That version check considers both the freeradius_version fact, and the package_ensure param.

ps. don't take my activity on these as urgency or anything, I'm just getting things out of my head and on to paper - enjoy your time away!

[djjudas21]

Hey, I've just realised this is still open. Was the work completed etc? Thanks 😄

[nward]

Hey, I've just realised this is still open. Was the work completed etc? Thanks 😄

Haha - yeah I thought about it the other day as well. Let me get back to you tomorrow (2345 here). We’ve been running in production with a number of improvements and so on that I’ve made like this one, so will revisit it and look at some PRs for the others.

[djjudas21]

Awesome, thanks. Talk soon

[nward]

OK - I've done some further work on this. In previous iterations the logic was broken for displaying warnings.

Now:

• if it detects FR3.0.x it will fail to compile if an FR3.1.x option is included
• if it detects FR3.1.x it will warn if an FR3.1.x option is included, logic being that 3.1.x is experimental and may not always be supported

I have had some trouble getting the testing to work with this, however. I'm unable to catch the warning() or fail() here. I'm not convinced that the PDK test unit stuff is working properly, as it has some errors I'm not seeing on other projects.

I've left my attempts commented out here. Wondering if the right approach is to merge, then raise fixing this testing as an issue. Thoughts?

[djjudas21]

I think it's absolutely fine to merge something that's incomplete, provided it's incrementally closer to our desired state, and doesn't make things worse in the meantime 👍

[nward]

This will be good to go (I think) once I've pulled in changes from master once the validation_fixes branch is merged.

[djjudas21]

I've approved this as the checks are passing but haven't given it any testing. Happy to hit the merge button when you've confirmed that all the work on this branch is finished, as it's quite a big one. Then I guess we should talk about rolling a new release 😄

[nward]

Yep - this work is all looking good to me. I can push the merge button now - my first one !