Add dns_lookup to stdlib. Ticket #21305

[biderbek]

Examples:

          dns_lookup(['1.2.3.4','example.com'])

  Returns: ['1.2.3.4','192.0.43.10']

          dns_lookup('1.2.3.4/32','i-dont-exist-domain.com')

  Returns: ['1.2.3.4/32','i-dont-exist-domain.com']

[HAIL9000]

@biderbek No worries, you actually had signed it which was why I deleted my comment. Usually we have a bot that will comment on a pull request if the contributor has signed the CLA, but for some reason it didn't comment on this one.

But you're all set in the CLA department!

[biderbek]

Awesome, thanks!

[adrienthebo]

Thank you very much for this contribution!

I'm interested in the behavior of this when resolution fails. If I understand the current behavior, if an address cannot be resolved then why would it be returned unresolved instead of outright failing?

In addition a use case or explanation would be good to have for this; how would you expect this to be used?

[biderbek]

For lookup failure, the thought was to leave it outside this function, and keep it specific. I guess the case can be made either way based on system configuration, nsswitch.conf, local host files etc. I was subscribing to the philosophy, resolve what's possible, and return what is not resolvable unchanged.

As far as use cases go, we are using get_var to abstract out our data (hopefully soonish to upgrade to puppet 3 and have access to hiera). Saving hostnames in yml / vars is easier to understand, but some software configuration files work better with IP's. Case and point, I have had issues with Apache proxy blocks slowing down requests due to reverse lookups done on hostnames. Firewall rules require ip's, etc. So the basic use case is, a configuration file that requires or works better with IP's, but abstraction that is easier to understand with hostnames.

[adrienthebo]

This seems like it's a pretty specific function, and I wonder how useful this would be to the majority of the users of the stdlib module. Could you see this function being used by 50% - 75% of the consumers of stdlib, or would the body of users be smaller than that?

[adrienthebo]

I noticed this hasn't seen any activity in about a week. Is there a chance you'll be able to work on this in the coming week? No pressure, just checking in.

[adrienthebo]

It looks like this pull request hasn't seen any activity in about two weeks. Is this something that you're still able to work on? We're happy to keep this pull request open if there's forward movement, and we'll leave it open for another two weeks from now. If there isn't any activity by then we may have to close this pull request due to lack of activity.

Closing the pull request wouldn't mean we don't consider this change valuable. However we try to keep all open pull requests moving towards completion, and closing stale pull requests help us focus on more active pull requests.

If you have any questions or concerns, please don't hesitate to ping us in #puppet-dev on irc.freenode.net.

[biderbek]

Sure, sorry for the delay. I was on vacation for a week. Let me do some more testing with it. As to your earlier question, I did do some research on our own code base and found 40 something instances of us resolving ip's in our manifests / templates. I have converted a few. I can convert more and report back if there are any problems.

If you research 'dns lookup puppet' in google, there are quite a few adhoc way's that my coworkers and I have used different places. I also came across this github project which has a function named the same, and extend's it further. https://github.com/dalen/puppet-dnsquery. So totally understandable if you guys aren't interested in merging it in, but I would say naming, resolution, and data structures are fundamental foundations of good systems building.

[biderbek]

Here are some more accurate numbers for instances of 'Resolv.getaddress' embedded in our code:

40 in templates.
2 inline template instances in manifests.

Plus the couple I have converted.

Note, there are no usages of 'Resolv.getname' usages in our codebase.

[slippycheeze]

I just noticed this, and took a quick look. Sorry for dropping in a
comment late in the game.

This uses a Ruby DNS client, bypassing normal NSS lookup on Unix hosts. It
will ignore, eg, /etc/hosts, LDAP lookup, and other mechanisms the
sysadmin might reasonably expect to have in play. It will also follow the
standard behaviour of the resolv library, which means that users need to
be aware of behaviour around changes to /etc/resolv.conf vs the lifetime
of the Ruby process.

While that isn't a killer problem, you really should document the way it
works. That way people won't be surprised when, eg, they don't get the
same behaviour that ping foo.example.com would give them.

I also notice that you don't do a lot to differentiate, or support, IPv6
and IPv4 hosts, or hosts with multiple addresses returned from DNS. Resolv
just returns the "first" address, which is ... not enormously well defined
in the context.

Again, not a killer problem -- just a documentation challenge that you
should address to make sure that users are aware of the limitations and
behaviour of your function.

Finally, you return unresolvable addresses unmodified, which is an
interesting error handling choice. I think that stems from a mistaken
application of this to multiple values concurrently -- better to return a
detectable error, or support a user supplied "default" value, perhaps?

At the moment there is almost no possible way I can imagine to handle
unresolvable names by, eg, failing. Including an example of how to do that
would be great -- along with examples of how you think users should
generally handle errors in the function.

[biderbek]

Ah, those are good points I hadn't considered / known. Let me do some research and see how I can improve it.

[adrienthebo]

This function definitely has merit, but I still have concerns about how widely applicable this will be to the users of puppetlabs-stdlib. This would be great to have as a module on the Puppet Forge but right now I don't think this is ready to be part of the the core stdlib functions.

Because of this I'm going to go ahead and close this pull request for the time being. Please re-open this pull request once the next actions are addressed, new information is available, or you have a question related to this pull request. We've become aware of difficulties re-opening pull requests, in the event you cannot please mention jeffmccune or adrienthebo with an @ sign in front and we'll re-open this pull request.

Closing the pull request doesn't mean we don't consider this change valuable, just that there are things that need to be addressed before it can be merged. If you have any questions or concerns, please don't hesitate to ping us in #puppet-dev on irc.freenode.net.