Make dnssec validation a configurable option.

[darkfoxprime]

Default it to "absent" on RedHat 5 whose default bind package is too old for dnssec.

[solarkennedy]

This is confusing to me. I would rather we just "fail", letting a user say one thing and then silently doing something else on a particular platform is surprising

[solarkennedy]

I would prefer this functionality to be much simpler. I vote we just leave it default to auto and then have a check to "fail" when we are centos5 if we know it can't handle it.

We don't really need the whole "$real_*" thing, I've seen that get out of hand in other modules.

[solarkennedy]

how about just using validate_re($dnssec_validation, '/^yes|no|auto$/')

[darkfoxprime]

because i didn't know about validate_re :) i'll update to reflect that.

[darkfoxprime]

Actually, I was following the pattern used several times earlier in the file. I can change this to validate_re - but should I change the previous checks that look the same?

Example:

if $check_names_master != undef and !member($valid_check_names, $check_names_master) {
  fail("The check names policy check_names_master must be ${valid_check_names}")
}

[darkfoxprime]

Is your disagreement with the code itself that determines the default value, or with the general idea of having a different default value on RHEL 5.x than on other OSes?

[solarkennedy]

The general idea that the code does more complex things for el5. I don't think the complexity is worth it.
I vote we just have a default. (auto).

I don't mind the validation for centos5, but even in our environment, just because we run centos 5 doesn't mean we run a version of bind that can't do dnssec. I think it is too presumptuous. The most presumptuous part is that if you say "yes" to this param, on centos 5 it won't show up at all. This is the surprising part.

[darkfoxprime]

if you say "yes" to this param, it will be used no matter what. the first if clause says any value that's not undef will be used; it's only if dnssec_validation is undef that the redhat5 check is made.

I can rework this so that it doesn't use the "real_" variable pattern, by putting a selector into the parameters list for the default value of dnssec_validatio instead of having it undef; to me that makes the parameter list harder to read, but I can do it that way if you prefer - or I can put that check into the dns::server::params class, have that param class included here, and use that for the default.

[solarkennedy]

How about instead just use the params pattern? That way it can give a sane default depending on the OS, and we kind of "hide" the logic about selecting the default to params.pp (which is where modules traditionally hide per-distro logic)

I see now what you are saying that the el5 logic only takes over if undef, I missed that. Yea please just move that to params and that will make it a bit easier to understand (for me) I think.

[darkfoxprime]

grr, i might need to undo the last commit.

i'm not sure if passing in undef as a parameter value will actually mean the parameter is not defined, or if the parameter will be treated as missing and therefore get the default value.

And in either case, I can't write an rspec test for this case.

[darkfoxprime]

okay, switched back to "absent" to omit the dnssec-validation option. Also added in rspec tests for the different values and for the defaults on different OSes.

If you want, I will submit separately to change the validation method used throughout the dns::server::options code to use validate_re rather than the member tests as used now; I'll do that as part of the PR to make dns::server::options a class rather than a defined type.

Note that whether it's a defined type or a class, this module won't work if dns::server::options is not invoked at least once to generate ${cfg_dir}/named.conf.options, since the named.conf file will include that options file anyway.

[solarkennedy]

True, but if this was class, we could have more control over the order in which is it parsed/generated. Hopefully that will make this module more user-friendly. (and like most other puppet modules)

[solarkennedy]

I want to say thank you very much for the flood of PR's that you have been making. It is obvious to me (and hopefully the other contributors) that you rely on this module heavily and use it for real on a platform that the rest of use don't use (centos). I'm going to recommend making you a contributor after a few more PR's that fix these kind of issues! Thank you!

(although, do you switch back and forth between darkfoxprime and jearls?)

[darkfoxprime]

Yes, darkfoxprime is my home account and jearls is my work account.

And I actually haven't quite started using the module officially yet, but I hope to soon - I'm still getting the puppet environment put together for work, but planning out how to use the different modules. And it may turn out that I don't use this module at all, if the rest of the team wants to continue managing DNS zone files directly. We'll see :)