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 sortlist in the presence of CNAME #5615

Merged
merged 4 commits into from Aug 16, 2017

Conversation

Projects
None yet
3 participants
@ahupowerdns
Member

ahupowerdns commented Aug 14, 2017

In #5357 @killerwhile discovered we were missorting CNAME records when using sortlist.
With this commit, we should get it right by moving to stable_sort and being more careful about type equivalence.

Short description

Fix sortlist in the presence of CNAME

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • checked that this code was merged to master
Fix sortlist in the presence of CNAME
In #5357 @killerwhile discovered we were missorting CNAME records when using sortlist.
With this commit, we should get it right by moving to stable_sort and being more careful about type equivalence.

@ahupowerdns ahupowerdns added the rec label Aug 14, 2017

@ahupowerdns ahupowerdns requested a review from Habbie Aug 14, 2017

@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie Aug 14, 2017

Member

Unrelated to this PR: https://doc.powerdns.com/recursor/lua-config/sortlist.html#addsortlist does not mention this disables the packet cache.

Member

Habbie commented Aug 14, 2017

Unrelated to this PR: https://doc.powerdns.com/recursor/lua-config/sortlist.html#addsortlist does not mention this disables the packet cache.

@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie Aug 15, 2017

Member
;; ANSWER SECTION:
order.example.com.	120	IN	MX	25 mx.example.com.
ordercname.example.com.	120	IN	CNAME	order.example.com.

This fix works for A/AAAA but not for other types.

Member

Habbie commented Aug 15, 2017

;; ANSWER SECTION:
order.example.com.	120	IN	MX	25 mx.example.com.
ordercname.example.com.	120	IN	CNAME	order.example.com.

This fix works for A/AAAA but not for other types.

@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie Aug 15, 2017

Member

This fix works for A/AAAA but not for other types.

Corrected in 'two non-address records sort equal'

Member

Habbie commented Aug 15, 2017

This fix works for A/AAAA but not for other types.

Corrected in 'two non-address records sort equal'

@Habbie

Habbie approved these changes Aug 15, 2017

With my correction (already pushed) this looks correct to me. A test would be nice, I'll have a go at that.

@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie Aug 15, 2017

Member

Simplification proposal: -only- move address records around, leave everything else where it is:

-  // anything address related is always 'larger', rest is equal
-
-  if(aAddr && !bAddr)
-    return false;
-  else if(!aAddr && bAddr)
-    return true;
-  else if(!aAddr && !bAddr)
-    return false;
-
+  if(!aAddr || !bAddr) return false;

It turns out this is wrong.

Member

Habbie commented Aug 15, 2017

Simplification proposal: -only- move address records around, leave everything else where it is:

-  // anything address related is always 'larger', rest is equal
-
-  if(aAddr && !bAddr)
-    return false;
-  else if(!aAddr && bAddr)
-    return true;
-  else if(!aAddr && !bAddr)
-    return false;
-
+  if(!aAddr || !bAddr) return false;

It turns out this is wrong.

@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie Aug 15, 2017

Member

Now with test.

Member

Habbie commented Aug 15, 2017

Now with test.

@Habbie Habbie referenced this pull request Aug 16, 2017

Merged

fix reading of ednsflags in recursor testing #5617

3 of 7 tasks complete

@ahupowerdns ahupowerdns merged commit 8f7a7e6 into PowerDNS:master Aug 16, 2017

1 check passed

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

@ahupowerdns ahupowerdns deleted the ahupowerdns:sortorder-cname branch Aug 16, 2017

@killerwhile

This comment has been minimized.

Show comment
Hide comment
@killerwhile

killerwhile Sep 1, 2017

I successfully tested in on my side (backported to 4.0.6). Thanks!

killerwhile commented Sep 1, 2017

I successfully tested in on my side (backported to 4.0.6). Thanks!

@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie Sep 4, 2017

Member

Thank you for reporting back!

Member

Habbie commented Sep 4, 2017

Thank you for reporting back!

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