dig lookup plugin incorrectly handles NXDOMAIN #41192

[kcbark]

SUMMARY

Please note that I am not the original author of this issue. I copied it here from ansible/ansible#41192, since it was closed there. I hope @fghaas don't mind.

When the dig lookup plugin is given a host name whose result returns NXDOMAIN, it returns the literal value NXDOMAIN instead of bailing out with an AnsibleError

ISSUE TYPE

• Bug Report

COMPONENT NAME

dig

ANSIBLE VERSION

ansible 2.10.1
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/home/ansible/.ansible/plugins/modules', '/usr
  ansible python module location = /usr/local/lib/python3.6/dist-packages/ansible
  executable location = /usr/local/bin/ansible
  python version = 3.6.9 (default, Jul 17 2020, 12:50:27) [GCC 8.4.0]

OS / ENVIRONMENT

Distributor ID: Ubuntu
Description: Ubuntu 18.04.5 LTS
Release: 18.04
Codename: bionic

STEPS TO REPRODUCE

- name: deploy SSH key
  authorized_key:
    key: "<an SSH public key>"
    key_options: "from=\"{{ lookup('dig', 'foobar.example.com') }}\""
    user: "{{ ansible_ssh_user }}"

EXPECTED RESULTS

If foobar.example.com does not resolve to a valid DNS record, the playbook run should fail.

ACTUAL RESULTS

The user's .ssh/authorized_keys file now contains the key options from="NXDOMAIN", effectively locking out the user completely.

This is due to the fact that the lookup plugin masks the NXDOMAIN error for reasons I can't quite follow:

        except dns.resolver.NXDOMAIN:
            ret.append('NXDOMAIN')
        except dns.resolver.NoAnswer:
            ret.append("")
        except dns.resolver.Timeout:
            ret.append('')
        except dns.exception.DNSException as e:
            raise AnsibleError("dns.resolver unhandled exception %s" % to_native(e))

        return ret

I'd suggest that all of dns.resolver.NXDOMAIN, dns.resolver.NoAnswer, and dns.resolver.Timeout should just be wrapped in an AnsibleError and fail the playbook run. When people use the dig plugin, it's reasonable to assume that they want these hostnames to be actually resolved to IP addresses, and not magically turn into the empty string or the string NXDOMAIN.

Linked issue from ansible core: ansible/ansible#41192
Linked PR from ansible core: ansible/ansible#41195

[ansibullbot]

Files identified in the description:

• plugins/lookup/dig.py

If these files are inaccurate, please update the component name section of the description or use the !component bot command.

click here for bot help

[ansibullbot]

cc @jpmens
click here for bot help

[bmillemathias]

I'd suggest that all of dns.resolver.NXDOMAIN, dns.resolver.NoAnswer, and dns.resolver.Timeout should just be wrapped in an AnsibleError and fail the playbook run

Well, from my point it is legit the module does not fail NXDOMAIN, I recently had to write a playbook to check if a record would exist or not and had to check if the result returned had NXDOMAIN to do some specific process, as I would do on any other kind of language.

[jpmens]

This has been discussed at length at ansible/ansible#41195
IIRC (I've not re-read everything) we (some of us) came to the conclusion that the play should fail on resolution errors, but permit dig to have a flag which makes it do what it's being doing so far.

[felixfontein]

For backwards compatibility, the lookup (it's not a module!) should get an option which allows to make it fail on NXDOMAIN and/or failures. Changing the default of that option is something that breaks potentially a lot, and should be avoided IMO.

(The only exception I'd make would be for proper errors. For NXDOMAIN, I don't think the default behavior should change.)

[fghaas]

My opinion on this is the same as discussed in ansible/ansible#41195 (comment) and subsequent comments. My impression was and still is that the only way I'd get this merged would be to add a flag that not only allows people to preserve the current behaviour, which I consider broken, but that also defaults to preserving the current behaviour. So that would mean adding a configuration flag, but with a bad default, and I consider that a disservice to Ansible users. With apologies for being stubborn, I'd prefer to not amend this PR to do something that still I believe to be the wrong thing. :)

So, if someone would like to implement this using a flag that defaults to preserving the existing behaviour, I encourage that person to submit their own PR for that purpose and close this one, ideally linking the superseding PR.

[felixfontein]

I agree with @bcoca in the discussion of ansible/ansible#41195: changing the default breaks existing playbooks and roles, so this will have no chance of getting merged.

What is possible (also what @bcoca suggested) is to add a toggle with the current behavior as default, then deprecate the default (users need to specify the value explicitly to get rid of the deprecation warning), and once the deprecation expires (i.e. in at least two major versions, according to our policy), change the default.

[fghaas]

@felixfontein, that's perfectly fine, it's just that I disagree on the grounds that I see it not as a feature addition or removal, but as a bug fix for behavior that's currently broken. If someone else (@kcbark perhaps?) wants to pick this up and get it into a mergeable shape, they have my full blessing.

[sprnza]

Such a misleading behavior! 3 years passed and no fix. Please at least describe it in the docs with a huge red symbols.

[fghaas]

@sprnza Please feel free to take my original patch from ansible/ansible#41195, modify it to the point that core reviewers are happy with it (meaning, make it default to the current and, in my opinion, broken behavior), and re-submit it. I'm fine with it landing that way; I just don't want my name on it if it does. Thanks for respecting my stubborn stance here!

[jpmens]

@sprnza and if you do as @fghaas suggests, I'll slap a shipit onto it so this can finally be put to rest. :)

[felixfontein]

resolved_by_pr #4973