Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#859 closed defect (fixed)

Search cache for lowest reachable delegation

Reported by: vorner Owned by: vorner
Priority: medium Milestone: Sprint-20110503
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 3.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This is part of #833, using cache to get to the lowest known delegation point instead of starting from root.

Subtickets

Change History (7)

comment:1 Changed 9 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from new to reviewing

Hello

The code is ready. It took me some time to think it over, but it turned out to be quite simple in the end.

I decided I don't want to take the lowest NS in cache unconditionally. Because if I did, the A/AAAA records for it might have expired. The NSAS would get the NS from cache, asked for IP addresses, which would generate query to the same zone, and we would get a loop. So I check that the NS has some address, if not, I move up to the next known delegation. This should not increase the number of queries, because we should get the A/AAAA with the delegation as glue anyway.

Unluckily, I can't test it in reality, administrator of my network blocks port 53 except his own nameserver. So, if someone could have a look if this works and if it solves #833? (It should, in theory, I counted that we could have quadratic number of queries in the number of delegations before).

I propose this changelog entry:

[func]		vorner
We use cache to find lowest usable delegation point, reducing number of
sent queries.

comment:2 Changed 9 years ago by zhanglikun

  • Owner changed from UnAssigned to zhanglikun

comment:3 Changed 9 years ago by zhanglikun

  • Owner changed from zhanglikun to vorner

Hi vorner, some comments

  1. In line 306, recursive_query.cc
       if (!foundAddress && tryName.getLabelCount() > 1) {
    

We don't need to check "!foundAddress" here, since if it foundAddress is true, it has breaked.

  1. in recursive_query_unittest.cc

If you would like to replace MockResolver? with TestResolver?(), I suggest to remove the definition of MockResolver? from the file.

  1. I can't understand what you are trying to test in the following code
926     EXPECT_NO_THROW(EXPECT_EQ(nsUpper->getName(),
927                               (*resolver_)[0]->getName()) <<
928                     "It starts resolving at the wrong place") <<
929         "It does not ask NSAS anything, how does it know where to send?";

If you want to test whether resolver has used the lowerest delegation point, my opinion is to seperate the code (line 275 to 309, recursive_query.cc) to a function with a name getLowerestDelegationInCache( too long? ), then add unittest for it.

others looks ok.

comment:4 Changed 9 years ago by vorner

  • Owner changed from vorner to zhanglikun

Hello

About the foundAddress, actually, it was needed. The break would jump out of the internal for cycle, containing directly by the if. Anyway, current version doesn't need it, since there's a return.

The mock class was removed.

I split if off into a separate function (I noticed it's complicated enough to deserve it's own function anyway, but I named it simply deepestDelegation) and it is tested. But I left the original test there as well and added a comment that explains the way the test works. Is it OK now?

Thanks

comment:5 Changed 9 years ago by zhanglikun

  • Owner changed from zhanglikun to vorner

Unluckily, I can't test it in reality, administrator of my network blocks port 53 except his own nameserver. So, if someone could have a look if this works and if it solves #833? (It should, in theory, I counted that we could have quadratic number of queries in the number of delegations before).

I have tested, it works as you expected.

Good job, please go ahead to merge.

comment:6 Changed 9 years ago by vorner

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:7 Changed 9 years ago by vorner

  • Estimated Difficulty changed from 0.0 to 3
Note: See TracTickets for help on using tickets.