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

Fix connection.remote_host and NXDOMAIN result #1716

Merged
merged 2 commits into from Nov 14, 2016

Conversation

Projects
None yet
3 participants
@smfreegard
Collaborator

smfreegard commented Nov 14, 2016

The legacy property connection.remote_host was broken by 8e412b1 and was undefined if the DNS lookup returned an error.

Additionally, dns.NXDOMAIN was incorrect - as per the Node docs, this should be dns.NOTFOUND, so it would appear this has been broken since day one.

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Nov 14, 2016

Member

Actually, NXDOMAIN -> NOTFOUND is a rather recent node.js change, so I'd instead have BOTH listed in that case statement. IE:

    case dns.NXDOMAIN:
    case dns.NOTFOUND:
        your_stuff();
Member

msimerson commented Nov 14, 2016

Actually, NXDOMAIN -> NOTFOUND is a rather recent node.js change, so I'd instead have BOTH listed in that case statement. IE:

    case dns.NXDOMAIN:
    case dns.NOTFOUND:
        your_stuff();
@smfreegard

This comment has been minimized.

Show comment
Hide comment
@smfreegard

smfreegard Nov 14, 2016

Collaborator

Are you sure about that? I actually looked through the source code of Node 0.10.x and couldn't find NXDOMAIN was valid.

Collaborator

smfreegard commented Nov 14, 2016

Are you sure about that? I actually looked through the source code of Node 0.10.x and couldn't find NXDOMAIN was valid.

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Nov 14, 2016

Member

Are you sure about that?

It wouldn't have come from the node.js source, it was coming from c-ares. There's some big long threads in the node.js docs discussing this change. I gave this change a +1 in that thread sometime ago, so yeah, I'm pretty sure.

Member

msimerson commented Nov 14, 2016

Are you sure about that?

It wouldn't have come from the node.js source, it was coming from c-ares. There's some big long threads in the node.js docs discussing this change. I gave this change a +1 in that thread sometime ago, so yeah, I'm pretty sure.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 14, 2016

Current coverage is 36.11% (diff: 0.00%)

Merging #1716 into master will decrease coverage by 0.02%

@@             master      #1716   diff @@
==========================================
  Files            24         24          
  Lines          6054       6058     +4   
  Methods         781        781          
  Messages          0          0          
  Branches       1531       1531          
==========================================
  Hits           2188       2188          
- Misses         3866       3870     +4   
  Partials          0          0          

Powered by Codecov. Last update 1cb1e97...383088d

codecov-io commented Nov 14, 2016

Current coverage is 36.11% (diff: 0.00%)

Merging #1716 into master will decrease coverage by 0.02%

@@             master      #1716   diff @@
==========================================
  Files            24         24          
  Lines          6054       6058     +4   
  Methods         781        781          
  Messages          0          0          
  Branches       1531       1531          
==========================================
  Hits           2188       2188          
- Misses         3866       3870     +4   
  Partials          0          0          

Powered by Codecov. Last update 1cb1e97...383088d

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Nov 14, 2016

Member

Also, "oops." Sorry to have broken remote_host setting. Missed that one...

Member

msimerson commented Nov 14, 2016

Also, "oops." Sorry to have broken remote_host setting. Missed that one...

@msimerson msimerson added the Bug Fix label Nov 14, 2016

@smfreegard

This comment has been minimized.

Show comment
Hide comment
@smfreegard

smfreegard Nov 14, 2016

Collaborator

I've double checked and NXDOMAIN has either never been valid or it was valid on Node < 0.8 as:

[root@relay1 ~]# echo "var dns = require('dns'); console.log(process.version); console.log('NXDOMAIN => ' + dns.NXDOMAIN); console.log('NOTFOUND => ' + dns.NOTFOUND);" | node
v0.8.16
NXDOMAIN => undefined
NOTFOUND => ENOTFOUND

Also, "oops." Sorry to have broken remote_host setting. Missed that one...

No worries. It had me doubting my own sanity until I realised what the issue was.

Collaborator

smfreegard commented Nov 14, 2016

I've double checked and NXDOMAIN has either never been valid or it was valid on Node < 0.8 as:

[root@relay1 ~]# echo "var dns = require('dns'); console.log(process.version); console.log('NXDOMAIN => ' + dns.NXDOMAIN); console.log('NOTFOUND => ' + dns.NOTFOUND);" | node
v0.8.16
NXDOMAIN => undefined
NOTFOUND => ENOTFOUND

Also, "oops." Sorry to have broken remote_host setting. Missed that one...

No worries. It had me doubting my own sanity until I realised what the issue was.

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Nov 14, 2016

Member

I just double checked too, and the thread in question is actually replacing the node.js customized error (NOTFOUND) with the actual IETF specified DNS error (from c-ares). So I had the direction of the change inverted. That thread is currently stalled, but I'd put that case in there now with this PR because it's likely to match in the future.

Member

msimerson commented Nov 14, 2016

I just double checked too, and the thread in question is actually replacing the node.js customized error (NOTFOUND) with the actual IETF specified DNS error (from c-ares). So I had the direction of the change inverted. That thread is currently stalled, but I'd put that case in there now with this PR because it's likely to match in the future.

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Nov 14, 2016

Member

Amusingly, there's quite a few places in our code where we've (at least you and I) have been using both for quite a long time.

Member

msimerson commented Nov 14, 2016

Amusingly, there's quite a few places in our code where we've (at least you and I) have been using both for quite a long time.

@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson

msimerson Nov 14, 2016

Member

Ooooohhhhhhhhhhhh. One reason that we'd have gotten NXDOMAIN results sometimes and NOTFOUND errors at other times is because of the use of dns.lookup (which uses getaddrinfo) vs dns.anythingElse which uses c-ares.

We (haraka) have long avoided using dns.lookup, but that likely explains why we've got 22 instances of NXDOMAIN in our source code.

Member

msimerson commented Nov 14, 2016

Ooooohhhhhhhhhhhh. One reason that we'd have gotten NXDOMAIN results sometimes and NOTFOUND errors at other times is because of the use of dns.lookup (which uses getaddrinfo) vs dns.anythingElse which uses c-ares.

We (haraka) have long avoided using dns.lookup, but that likely explains why we've got 22 instances of NXDOMAIN in our source code.

@smfreegard

This comment has been minimized.

Show comment
Hide comment
@smfreegard

smfreegard Nov 14, 2016

Collaborator

I think in my case - I always used the rdns_respond function as a boilerplate and when I had issues I added 'ENOTFOUND' instead and bypassed dns.NOTFOUND altogether.

Anyway - I'll add dns.NXDOMAIN back in, to preempt the Node change (thanks for the reference).

Collaborator

smfreegard commented Nov 14, 2016

I think in my case - I always used the rdns_respond function as a boilerplate and when I had issues I added 'ENOTFOUND' instead and bypassed dns.NOTFOUND altogether.

Anyway - I'll add dns.NXDOMAIN back in, to preempt the Node change (thanks for the reference).

@msimerson msimerson merged commit 5aedbbf into haraka:master Nov 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@msimerson

This comment has been minimized.

Show comment
Hide comment
@msimerson
Member

msimerson commented Nov 14, 2016

thanks @smfreegard

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