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

Traffic ipv6 translation support (clean) #7985

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ashus
Copy link

@Ashus Ashus commented Oct 18, 2024

This adds a support for resolving IPv6 reverse addresses.

Copy link
Member

@AdSchellevis AdSchellevis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably need test cases (examples) first to assess what your current issue is, looking in the upstream documentation of the library it should already work

reverse_ip = '.'.join(reversed(address.split("."))) + ".in-addr.arpa"

tasks.append(dnsResolver.resolve(reverse_ip, "PTR"))
except Exception as e:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to catch explicit exception types, unexpected errors are better be thrown so you know what to look for later

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really write in python, imo any exception here is acceptable to skip that address in the lookup process.


tasks.append(dnsResolver.resolve(reverse_ip, "PTR"))
except Exception as e:
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

continue? the next address might be a valid one according to the loop

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I fixed this in my next commit.

@@ -94,15 +95,32 @@ async def request_ittr(self, addresses):
dnsResolver.timeout = 2
tasks = []
for address in addresses:
tasks.append(dnsResolver.resolve_address(address))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you trying to re-implement the function of resolve_address() in the code below? According to the upstream documentation (https://dnspython.readthedocs.io/en/latest/resolver-class.html), resolve_address() should already return a valid response for both address families.

Can you offer some example addresses where the code below offers your expected outcome and resolve_address() doesn't?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't trying to re-implement it. But the following part was hard-coded to work only with IPv4 (.in-addr.arpa). My goal was only to allow translation for any IP address that comes up.
I was surprised to find that low-level translation to PTR there and only expanded on it. If there is a better solution, I'll gladly accept it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please offer examples of addresses where your code offers different outcomes than the existing code, https://github.com/rthalley/dnspython/blob/740ae72d28e5bdadafbda80169eea255bbde16ac/dns/asyncresolver.py#L113-L139 suggests it does work on both, digging deeper in the code (https://github.com/rthalley/dnspython/blob/740ae72d28e5bdadafbda80169eea255bbde16ac/dns/reversename.py#L31-L66) doesn't appear to do anything else than support both either....

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you mean why not use this:

    async def request_ittr(self, addresses):
        self._results = {}
        try:
            dnsResolver = Resolver()
        except dns.resolver.NoResolverConfiguration:
            return
        dnsResolver.timeout = 2
        tasks = []
        for address in addresses:
            tasks.append(dnsResolver.resolve_address(address))

        responses = await asyncio.gather(*tasks, return_exceptions=True)

        for response, address in zip(responses, addresses):
            if isinstance(response, dns.resolver.Answer):
                if ":" in address:
                    addr = str(ipaddress.IPv6Address(address))
                else:
                    addr = ".".join(reversed(response.canonical_name.to_text().replace('.in-addr.arpa.', '').split('.')))

                for item in response.response.answer:
                    if isinstance(item, dns.rrset.RRset) and len(item.items) > 0:
                        self._results[addr] = str(list(item.items)[0])

        return self._results

I tried it, and in console it works fine. But on the web it just loops with an empty JSON response.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I found the correct logfile (configd) and found out, that git's auto CRLF newlines were the problem when uploading directly from my IDE. I tested it and it seems to work fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the risk of repeating myself, please offer example addresses for comparison.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't any difference that I know of.
I made a partial revert, would you like me to squash it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't going to be merged, these commits look highly suspicious, without test examples (which I repeatedly asked for), we will stop our efforts in these PR's.

@Ashus
Copy link
Author

Ashus commented Oct 21, 2024

Static local IPv6 addresses from ISC DHCPv6 service were not translating and that was the initial reason to do any changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants