Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ipaddress property to get name of reverse DNS PTR record #64679

Closed
leonn mannequin opened this issue Feb 2, 2014 · 23 comments
Closed

Add ipaddress property to get name of reverse DNS PTR record #64679

leonn mannequin opened this issue Feb 2, 2014 · 23 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@leonn
Copy link
Mannequin

leonn mannequin commented Feb 2, 2014

BPO 20480
Nosy @ncoghlan, @pitrou, @ericvsmith
Files
  • ipaddress_reverse_names.patch: patch implementing the feature
  • ipaddress_reverse_names_v2.patch: patch adding the reverse_pointer property
  • ipaddress_reverse_names_v3.patch: patch with updated documentation
  • ipaddress_reverse_names_v3_alt.patch: alternative patch without trailing dots
  • ipaddress_reverse_names_v4.patch: no trailing dots, with consistent variable naming
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/ericvsmith'
    closed_at = <Date 2014-04-14.16:59:19.574>
    created_at = <Date 2014-02-02.01:22:28.083>
    labels = ['type-feature', 'library']
    title = 'Add ipaddress property to get name of reverse DNS PTR record'
    updated_at = <Date 2014-04-14.16:59:19.574>
    user = 'https://bugs.python.org/leonn'

    bugs.python.org fields:

    activity = <Date 2014-04-14.16:59:19.574>
    actor = 'eric.smith'
    assignee = 'eric.smith'
    closed = True
    closed_date = <Date 2014-04-14.16:59:19.574>
    closer = 'eric.smith'
    components = ['Library (Lib)']
    creation = <Date 2014-02-02.01:22:28.083>
    creator = 'leonn'
    dependencies = []
    files = ['33851', '33852', '33865', '33870', '33940']
    hgrepos = []
    issue_num = 20480
    keywords = ['patch']
    message_count = 23.0
    messages = ['209933', '209935', '209936', '209937', '209938', '209939', '209941', '209960', '209962', '209963', '209977', '209980', '209981', '209982', '209986', '209987', '209995', '209996', '210000', '210049', '210378', '210379', '216112']
    nosy_count = 6.0
    nosy_names = ['ncoghlan', 'pitrou', 'eric.smith', 'pmoody', 'python-dev', 'leonn']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue20480'
    versions = ['Python 3.5']

    @leonn
    Copy link
    Mannequin Author

    leonn mannequin commented Feb 2, 2014

    I was missing a method to compute the reverse DNS name for an IP address, and I felt this is something that would belong in the ipaddress module; so here’s a patch for the ipaddress module adding a reverse_name property to IPv?Address.

    This is an example:

    >>> ipaddress.ip_address("127.0.0.1").reverse_name
    '1.0.0.127.in-addr.arpa.'
    >>> ipaddress.ip_address("2001:db8::1").reverse_name
    '1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.'

    Would this be an acceptable feature for inclusion in the ipaddress module?

    @leonn leonn mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Feb 2, 2014
    @ericvsmith
    Copy link
    Member

    I think the functionality is reasonable for this module.

    When I originally read the bug title, I was concerned that it was actually doing a reverse DNS lookup, which would not be appropriate. But now I realize it's just computing the name that would be used for the reverse lookup.

    I'm not sure "reverse_name" is the best name for this, but I don't have any better suggestions, and I could live with this name.

    Changing version to 3.5, since that's the first version this could be added to.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Feb 2, 2014

    Heh, my initial reaction based on the issue title was the same as Eric's, but yes, I agree the pure text manipulation proposed in the patch is actually a good fit.

    Rather than "reverse_name" (which I feel is ambiguous about whether or not it does the DNS lookup to resolve back to the canonical name for the address), I would suggest the attribute name "reverse_pointer" or "reverse_record" (with a slight preference for the former, as indicated in the updated issue title).

    @ncoghlan ncoghlan changed the title Add ipaddress property to get reverse DNS name Add ipaddress property to get name of reverse DNS pointer Feb 2, 2014
    @pitrou
    Copy link
    Member

    pitrou commented Feb 2, 2014

    Thanks for the patch, Leon. Is the trailing dot actually desired?

    >>> ipaddress.ip_address("127.0.0.1").reverse_name
    '1.0.0.127.in-addr.arpa.'

    Also, to accept your contribution, we will need you to fill a contributor's agreement. See http://www.python.org/psf/contrib/

    @leonn
    Copy link
    Mannequin Author

    leonn mannequin commented Feb 2, 2014

    Thanks for the feedback, I agree "reverse_pointer" is a better, less ambiguous name for the property. I’ve amended the patch to reflect this suggestion.

    Regarding the trailing dot, I felt it more appropriate to have it that to leave it out, but I don’t have a strong opinion on this. It has to be there in the DNS query, but most tools will automatically add it to the query if it’s not specified. The "host" tool from bind-utils is undecided as well when printing the output, it includes the trailing dot when querying IPv4 addresses, but not for IPv6 addresses.

    Including the trailing dot has the advantage that the output could be directly fed into other code that strictly requires it, like the dnspython module.

    I’ll sign and submit the contributor’s agreement as soon as I’m within reach of a printer (likely within the next 24 hours).

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Feb 2, 2014

    Gah, we still haven't fixed the contributor license docs on the main
    CLA page (hopefully we'll finally have that sorted later this month).
    In the meantime, if you go directly to
    http://www.python.org/psf/contrib/contrib-form/ it should give you the
    option to sign electronically:
    http://www.python.org/psf/contrib/contrib-form/

    @leonn
    Copy link
    Mannequin Author

    leonn mannequin commented Feb 2, 2014

    Oh nice, then fewer trees have to die. I’ve now signed the contributor’s agreement.

    @ericvsmith
    Copy link
    Member

    I'd prefer to leave the trailing dots. It's the technically correct thing to do.

    And as they say, technically correct is the best kind of correct!

    @ericvsmith ericvsmith changed the title Add ipaddress property to get name of reverse DNS pointer Add ipaddress property to get name of reverse DNS PTR record Feb 2, 2014
    @ericvsmith
    Copy link
    Member

    The patch looks good to me, save for a versionadded tag in the docs. I've mentioned it on Rietveld.

    Not to get in to bikeshedding on the name, but I'd prefer something with PTR in the name. I'm an old-timer, and think of these as "PTR lookups" (because they are!). If I were looking for it, that's what I'd search for.

    And if not in the property name, then at least the documentation should mention it.

    @ericvsmith
    Copy link
    Member

    Or, now that I think about it some more, maybe leave the name as reverse_pointer and just mention PTR records in the documentation.

    @pitrou
    Copy link
    Member

    pitrou commented Feb 2, 2014

    I'd prefer to leave the trailing dots. It's the technically correct thing to do.

    It depends what we're talking about. Hostnames (in the general sense)
    don't carry a trailing dot except in DNS queries. So I'd argue the
    trailing dot is more part of the DNS protocol than of the hostname
    itself.

    As for what the "host" command does, it doesn't add any trailing dots
    here:

    $ host 127.0.0.1
    1.0.0.127.in-addr.arpa domain name pointer XXX.
    $ host ::1
    1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.ip6.arpa domain name pointer XXX.

    (this is bind9-host from Ubuntu)

    The Wikipedia page doesn't use trailing dots:
    http://en.wikipedia.org/wiki/Reverse_DNS_lookup

    Neither do the examples from the RIPE itself:
    http://www.ripe.net/data-tools/dns/reverse-dns/how-to-set-up-reverse-delegation

    @leonn
    Copy link
    Mannequin Author

    leonn mannequin commented Feb 2, 2014

    I’ve changed the wording in the documentation a bit and added an explanatory sentence, how about this?

    I think this should make pretty clear what it does and does not, and also easy to find in the documentation when searching for “reverse”, “PTR” and “pointer” (which is what I can think of what one would be looking for).

    (Not sure whether to add further patches in roundup or rietveld, but a quick google search reveals patches are periodically added to rietveld automatically, so I’ll just add it here. Please let me know if this is not correct :))

    @leonn
    Copy link
    Mannequin Author

    leonn mannequin commented Feb 2, 2014

    As for what the "host" command does, it doesn't add any trailing dots
    here:

    Oh, interesting. It does on Fedora 19 with version 9.9.3, but not on my Ubuntu installation with version 9.8.1. So it seems this was added between 9.8.1 and 9.9.3.

    @pitrou
    Copy link
    Member

    pitrou commented Feb 2, 2014

    Oh, interesting. It does on Fedora 19 with version 9.9.3, but not on
    my Ubuntu installation with version 9.8.1. So it seems this was added
    between 9.8.1 and 9.9.3.

    According to apt-cache, the version I tried with is
    1:9.9.3.dfsg.P2-4ubuntu1

    @leonn
    Copy link
    Mannequin Author

    leonn mannequin commented Feb 2, 2014

    According to apt-cache, the version I tried with is
    1:9.9.3.dfsg.P2-4ubuntu1

    Ok, then it depends on something else apparently. It’s not so relevant anyway because even with the trailing dot, “host” is inconsistent between IPv4 and IPv6, and we should choose a consistent variant.

    I’m still slightly in favour of having the trailing dot, because it is the correct representation of the DNS name. RFC1034 section 3.1 explicitly states that “Since a complete domain name ends with the root label, this leads to a printed form which ends in a dot.”. It goes on to explain that relative names (names not ending in a dot) “appear mostly at the user interface, where their interpretation varies from implementation to implementation” and that the most common interpretation is relative to the root zone.

    So leaving the dot off is merely a convention, which is good enough for Wikipedia and RIPE documents; however I think in an API we should implement the more strict option. Also, I don’t see anything breaking with the dot, while without the dot, a user needs to take extra care when feeding the name to dnspython’s dns.resolver.query(), which would do undesired things without the trailing dot (append a search domain if there is one in /etc/resolv.conf).

    Any other opinions?

    @pitrou
    Copy link
    Member

    pitrou commented Feb 2, 2014

    Well that convention is also used in existing APIs:

    >>> socket.gethostbyaddr("8.8.8.8")
    ('google-public-dns-a.google.com', [], ['8.8.8.8'])

    (note that this merely reflects a POSIX API)

    @ericvsmith
    Copy link
    Member

    I concede the point on the trailing dot.

    The reason we used to always use it is that it prevented the local resolver from first trying to append the host's default domain name (as, for example, defined in /etc/resolv.conf as "search"). I have no idea if resolvers still do that. Some quick research shows that that's probably not their behavior any more, at least if the domain name contains at least one dot.

    @ericvsmith
    Copy link
    Member

    The documentation change looks good to me.

    @leonn
    Copy link
    Mannequin Author

    leonn mannequin commented Feb 2, 2014

    I can live without the trailing dot, although I’d find it “more correct” to have it.

    I’ve attached an alternative patch that doesn’t return a trailing dot, it’s up to you guys to decide which one you prefer.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Feb 2, 2014

    From a "principle of least surprise" point of view, non-specialists would
    likely be puzzled by a trailing dot and wonder why it was there. By
    contrast, specialists that know the trailing dot is technically *supposed*
    to be there are already going to be accustomed to seeing it left out
    (because it is already left out in so many other contexts).

    So I'm +1 for leaving out the trailing dot, as that likely won't surprise
    *any* users.

    @leonn
    Copy link
    Mannequin Author

    leonn mannequin commented Feb 6, 2014

    I noticed an inconsistency between IPv4 and IPv6 in my temporary variable names when looking at the patch again after a few days. Here’s an updated patch. Not that it’s important, but I sleep better that way :)

    As consensus seems to have settled on leaving out the trailing dot, I only updated the version without the dot.

    @ericvsmith
    Copy link
    Member

    Looks good to me. Once 3.5 is open for new features, I'll commit this. (But feel free to ping me if you think it's lingering!)

    Thanks!

    @ericvsmith ericvsmith self-assigned this Feb 6, 2014
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 14, 2014

    New changeset 5992d2c9522c by Eric V. Smith in branch 'default':
    Issue bpo-20480: Add ipaddress.reverse_pointer. Patch by Leon Weber.
    http://hg.python.org/cpython/rev/5992d2c9522c

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants