Add all and first values to ptr parameter of dns::record::a

[ghost]

This updates the ptr parameter of the dns::record::a type to allow first or all values. first is the same as true (or any other str2bool true values) and will act as the traditional ptr parameter did. With all, a dns::record::ptr resource is created for all of the ip addresses in data, not just the first. This uses the defined type dns::record::ptr::by_ip to actually create the dns::record::ptr resources so that we can get iteration in the older puppet parsers.

This also creates a set of rspec tests for the dns::record::a defined type.

[solarkennedy]

you have to put this in a dns_reverse_ptr_record file, you can't tuck it in init.pp.

How about manifests/record/reverse_ptr.pp?

[ghost]

So it actually does work like this (I tested it), it gets through lint, and it's used by other puppet labs modules as a way to do a private resource type.

I'll make the change if you want it that way for stylistic reasons. I was just trying to keep it private.

[solarkennedy]

It works only because the init class is parsed first. Otherwise puppet would not be able to find this class because it is "hidden" in the init.pp. It violates the autoloading behavior:
https://docs.puppetlabs.com/puppet/latest/reference/lang_namespaces.html#autoloader-behavior

Can you link to the other pupetlabs modules that do this? I've never seen this before in a major module.

I don't think "private" is a much as a concern to me as being not-surprising. We could bump our version of stdlib and use the private keyword directly:
https://docs.puppetlabs.com/guides/style_guide.html#public-and-private

But yes please. Style-wise this is important to me. Both puppet and humans need to be able to find code in a non-surprising location. How about dns::record::reverse_ptr? (dns/manifests/record/reverse_ptr.pp)

[ghost]

I went back and checked: my mistake - it's not used by puppetlabs modules. it's used by the module razorsedge-network.

In any case, I will rework to use a defined type in a standard location and namespace :)

[ghost]

I reworked the "private" defined type to be dns::record::ptr::by_ip, with parameters to make that defined type a suitable equivalent to dns::record::ptr (which dns::record::ptr::by_ip invokes to do its work).

[solarkennedy]

What is this @example syntax? Is that some puppet-strings thing?

[ghost]

I have no idea - it's part of the standard puppet module template that puppet module generate produces, so I've gotten in the habit of including it.

[ghost]

I just checked back on this PR. Is there anything I need to address?

[ghost]

still working on this - will continue after i get home from work

[ghost]

sigh. reverted all my changes in this PR for now. I should remember not to use my PR branches as the source for my production DNS servers.

[ghost]

Okay, the last commit updated the dns::record::ptr::by_ip type (the intermediate type now used by dns::record::a to handle the ptr option) to check if there's an already-defined dns::zone for the class A, B, or C network that holds the IP address in question. If there is, it splits the IP address into the appropriate network and host parts to create the underlying dns::record::ptr resource; if not, it assumes a class C network.

[solarkennedy]

Thanks! I think this is pretty comprehensive.

[ITler]

@jearls: nice idea with checking for an already defined dns::zone. Obviously this will only work on DNS server node, but won't help, if A record is defined on client node and transferred via puppet exported resources. Correct?

[solarkennedy]

@ITler that is correct. if defined() stuff kinda sucks :(

[ghost]

Sorry about that. I've never been able to use PuppetDB due to work policies, so I don't tend to think in terms of exported resources.

@ITler do you have any recommendations about other ways to accomplish this? I can think of two ways; the first is ugly, and the second is a nearly complete rewrite of the module's functionality.

1. Add an option to the dns::record::a type to indicate the class of the reverse subnet.
2. Turn the dns::record types into custom types (defined in ruby code rather than puppet). The provider code would be able to check the catalog to see what exists. But this has a whole lot of other implications and, like i said, is basically a complete rewrite of the module.

[ghost]

Actually, would a third possibility be to define a virtual dns::zone resource in the client node for the reverse zones? that way, the zone would be defined for the check, but would not have to be realized. but then you have to have every client know about your reverse dns structure. :P

[ITler]

@jearls
no need for a sorry.

my proposal:

Let dns::record::a accept ptr => (A,B,C|true) and calculate reverse zone name depending on that setting (the define already has all necessary information to do that)

Then defensively check, if reverse zone exists (explicitly defined somewhere else in the catalog). If not, create it - Or maybe better, fail with a convenient message to instruct the admin to explicitly define the zone. (Maybe this is better for compatibility).

In my opinion, this should allow to use dns::record::a as exported resource, too and should be quite easy to implement

[ghost]

@ITler
Thanks for your proposal. I have two comments on it:

This PR was originally all about changing ptr to allow all as an option to create PTR records for all IP addresses listed in data instead of just the first, so right now ptr allows the options all, first, true (an alias for first), or false. Adding the class designator into the ptr option itself breaks this model. I would prefer to add an additional option, something like ptr_class => A|B|C|auto (defaulting to auto), that would indicate the class of PTR record to create.

Second, creating the reverse zone or failing if it doesn't exist would have implications on its own for exported resources, since that check would happen on the client node, not on the server node.