Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1535 closed defect (fixed)

notify_out shouldn't look for NS addresses for out of zone NS names

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

Description

See #1430. Specifically, the notify_out modules should first perform
findZone() on the data source(s) for each NS name and then call
find() for the right zone.

Subtickets

Attachments (1)

find_nsec3.diff (2.3 KB) - added by jinmei 8 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 3

comment:2 Changed 8 years ago by jinmei

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed

comment:3 Changed 8 years ago by jelte

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed

comment:4 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120403

comment:5 Changed 8 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:6 Changed 8 years ago by jelte

  • Owner changed from jelte to UnAssigned

ok, the change itself is simple enough, but I can't really come up with a way to write tests for it; the datasource client is created on the fly; so we can't really override it. And while we should consider not doing that in the first place, it seems out of place to hack that in for the purpose of testing this change.

I did add a few out-of-zone NS records to the database but it wouldn't fail on that since those lookups would not result in success anyway.

So any suggestions on how else to define it (preferably without creating an entire mock finder tree) would be nice... (if we still intend to raise errors on find() calls of out of zone data, this would probably be very easy to test :))

comment:7 Changed 8 years ago by jelte

  • Owner changed from UnAssigned to jelte

should not have made this unassigned

comment:8 Changed 8 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Right.

I agree to the notion in the comments of ticket #1430 that ZoneFinder::find() should throw an exception when it is queried for out-of-zone data. And I with that the addition of out-of-zone data do the test database automatically tests the changes needed for this ticket.

So the second (and third commit) in this branch actually implement that; if we consider it too out of scope for this ticket, the reviewer can consider just the first commit, and we move the last two to a different branch.

comment:9 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:10 follow-up: Changed 8 years ago by jinmei

database.cc

  • you don't have to explicitly call toText() for getOrigin():
            isc_throw(OutOfZoneFind, name.toText() << " not in " <<
                                     getOrigin().toText());
    
    same for other similar cases.

memory_datasrc.cc

  • I'd catch this in findNode(). If I understand it correctly, if at the end of this method it's not either EXACTMATCH or PARTIALMATCH it's out-of-zone. I prefer this because we can integrate the out-of-zone check in the normal lookup. Since it's generally better to avoid any overhead in the lookups path for the in-memory zone for performance, and since the out-of-zone case should be exceptional, I'd like to minimize the overhead.
  • regarding the performance, I'd also like to do benchmark tests to measure the overhead of this check.

database_unittest.cc

  • ZoneFinder::FIND_DEFAULT can be omitted for ZoneFinder::find(). same for memory_datasrc_unittest.cc, and (I believe) datasrc_test.py.
  • this comment should be revised:
            // Note: find() rejects out-of-zone query name with NXDOMAIN
            // regardless of whether adding the RR succeeded, so this check
            // actually doesn't confirm it.
    
    (there may be other similar cases. maybe you want to check them by searching for "NXDOMAIN" throughout the code).

zone.h

  • OutOfZoneFind maybe renamed to OutOfZone, merged with in-memory specific exception of that name, and for an error about out-of-zone data in general? Not a strong opinion, just an idea.

notify_out.py

Code looks okay, but I think we could (should) do some tests:

  • add a test zone that contains out-of-zone NS names (from a quick look there's no such case right now, right?). Confirm that it results in exception for the current version of notify_out.py, and that it doesn't happen for the revised version.
  • also add a test zone that contains out-of-zone NS names whose zone is however also an authoritative zone of the server. check that notify_out now correctly include these addresses too.

Others

  • I made a couple of editorial changes.
  • I think we need a changelog for this task. Since it involves API change, we'd need '*'.

comment:11 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:12 in reply to: ↑ 10 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

database.cc

  • you don't have to explicitly call toText() for getOrigin():
            isc_throw(OutOfZoneFind, name.toText() << " not in " <<
                                     getOrigin().toText());
    
    same for other similar cases.

ack, fixed

memory_datasrc.cc

  • I'd catch this in findNode(). If I understand it correctly, if at the end of this method it's not either EXACTMATCH or PARTIALMATCH it's out-of-zone. I prefer this because we can integrate the out-of-zone check in the normal lookup. Since it's generally better to avoid any overhead in the lookups path for the in-memory zone for performance, and since the out-of-zone case should be exceptional, I'd like to minimize the overhead.

it's exceptional, but not for findNode() in all cases atm; it is also used in addAdditional processing during loading (where out-of-zone data is pretty common)

we could catch the exception there, instead of what I did now (copied that original check whether it's a match or subdomain to addAdditional, and skip the lookup if it's not in or below the domain), but in this case it is quite expected, and hence it seems cleaner to me to check before calling findNode in addAdditional

(btw, debugging exceptions thrown in parameterized gtests constructors is a PITA)

  • regarding the performance, I'd also like to do benchmark tests to measure the overhead of this check.

well, it's been removed now :)

database_unittest.cc

  • ZoneFinder::FIND_DEFAULT can be omitted for ZoneFinder::find(). same for memory_datasrc_unittest.cc, and (I believe) datasrc_test.py.

ack, removed

  • this comment should be revised:
            // Note: find() rejects out-of-zone query name with NXDOMAIN
            // regardless of whether adding the RR succeeded, so this check
            // actually doesn't confirm it.
    
    (there may be other similar cases. maybe you want to check them by searching for "NXDOMAIN" throughout the code).

if found a couple, hopefully that's all of them

zone.h

  • OutOfZoneFind maybe renamed to OutOfZone, merged with in-memory specific exception of that name, and for an error about out-of-zone data in general? Not a strong opinion, just an idea.

Done (second commit)

notify_out.py

Code looks okay, but I think we could (should) do some tests:

  • add a test zone that contains out-of-zone NS names (from a quick look there's no such case right now, right?). Confirm that it results in exception for the current version of notify_out.py, and that it doesn't happen for the revised version.

three should be, but it's hidden in the sqlite3 data blob; i had added 2 NS records to different zones to example.com. Since these aren't returned the test itself doesn't show that :)

but if you revert to the original notify_out.py this should show. (or you can see directly with sqlite3 select * from records where rdata="NS")

  • also add a test zone that contains out-of-zone NS names whose zone is however also an authoritative zone of the server. check that notify_out now correctly include these addresses too.

ah, did not have that one; removed the b.dns.example.com records, and updated b.dns.example.com NS to have rdata b.dns.example.net instead of b.dns.example.com
(this was the smallest difference to get the desired results; the two zones share a lot of data in terms of addresses)

Others

  • I made a couple of editorial changes.

thnx

  • I think we need a changelog for this task. Since it involves API change, we'd need '*'.

quite. Perhaps it even needs 2 (one for the API change, one for the change in behaviour of notify_out)

[bug]*
The implementations of Zonefinder::find() now throw an OutOfZone? exception when the name argument is not in or below the zone this zonefinder contains.

and
[bug]
The notify-out code now looks up notify targets in their correct zones (and no longer just in the zone that the notify is about).

comment:13 in reply to: ↑ 12 ; follow-up: Changed 8 years ago by jinmei

Replying to jelte:

memory_datasrc.cc

we could catch the exception there, instead of what I did now (copied that original check whether it's a match or subdomain to addAdditional, and skip the lookup if it's not in or below the domain), but in this case it is quite expected, and hence it seems cleaner to me to check before calling findNode in addAdditional

Ah, yes, sorry, I was aware of that but forgot to mention it in the
comment.

(btw, debugging exceptions thrown in parameterized gtests constructors is a PITA)

In which sense? Compared to typed tests? I understand parameterized
tests (whether it's TYPED_TEST or TEST_P) could make things more
complicated in general. It's a tradeoff between reusability and
complexity due to the additional layer, and I think the former
generally outweighs the latter.

If you mean it should be easier to handle with TYPED_TEST, for
example, and a specific suggestion of rewriting the tests, I have no
problem about that. I don't remember why I chose parameterized tests
instead of typed tests for that - I think I saw some difficulty with
the latter, but now I simply don't remember. In any event that would
be a subject of different task.

  • add a test zone that contains out-of-zone NS names (from a quick look there's no such case right now, right?). Confirm that it results in exception for the current version of notify_out.py, and that it doesn't happen for the revised version.

three should be, but it's hidden in the sqlite3 data blob; i had added 2 NS records to different zones to example.com. Since these aren't returned the test itself doesn't show that :)

Ah, okay, I missed the additional *.different.zone.

Now we have update APIs, we should probably avoid using binary DB
files as the primary source of test data but generate them from
human-readable source (such as textual zone files) at build or run
time. But that will be a subject of different task.

  • I think we need a changelog for this task. Since it involves API change, we'd need '*'.

quite. Perhaps it even needs 2 (one for the API change, one for the change in behaviour of notify_out)

[bug]*
The implementations of Zonefinder::find() now throw an OutOfZone? exception when the name argument is not in or below the zone this zonefinder contains.

I'm not sure this is a "bug", but I'd leave it to you (I see we could
call it a bug in that at least NXDOMAIN wasn't a good response). The
second 'zonefinder' should probably be ZoneFinder.

About the revised code:

  • I made a couple of editorial cleanups.
  • I'd suggest changing the exception for findNSEC3 against out-of-zone to OutOfZone too. Suggested patch is attached.

Otherwise the branch looks okay for merge.

Changed 8 years ago by jinmei

comment:14 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:15 in reply to: ↑ 13 ; follow-up: Changed 8 years ago by jelte

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

Replying to jinmei:

Replying to jelte:

memory_datasrc.cc

we could catch the exception there, instead of what I did now (copied that original check whether it's a match or subdomain to addAdditional, and skip the lookup if it's not in or below the domain), but in this case it is quite expected, and hence it seems cleaner to me to check before calling findNode in addAdditional

Ah, yes, sorry, I was aware of that but forgot to mention it in the
comment.

(btw, debugging exceptions thrown in parameterized gtests constructors is a PITA)

In which sense? Compared to typed tests? I understand parameterized
tests (whether it's TYPED_TEST or TEST_P) could make things more
complicated in general. It's a tradeoff between reusability and
complexity due to the additional layer, and I think the former
generally outweighs the latter.

If you mean it should be easier to handle with TYPED_TEST, for
example, and a specific suggestion of rewriting the tests, I have no
problem about that. I don't remember why I chose parameterized tests
instead of typed tests for that - I think I saw some difficulty with
the latter, but now I simply don't remember. In any event that would
be a subject of different task.

oh, i was not suggesting we change it, but the problem is that through all the templating and internal error handling of gtest, i found it quite hard to find where the exception was actually thrown.

Now we have update APIs, we should probably avoid using binary DB
files as the primary source of test data but generate them from
human-readable source (such as textual zone files) at build or run
time. But that will be a subject of different task.

ack

  • I think we need a changelog for this task. Since it involves API change, we'd need '*'.

quite. Perhaps it even needs 2 (one for the API change, one for the change in behaviour of notify_out)

[bug]*
The implementations of Zonefinder::find() now throw an OutOfZone? exception when the name argument is not in or below the zone this zonefinder contains.

I'm not sure this is a "bug", but I'd leave it to you (I see we could
call it a bug in that at least NXDOMAIN wasn't a good response). The
second 'zonefinder' should probably be ZoneFinder.

Yeah should indeed be ZoneFinder?. I do think NXDOMAIN is wrong enough to consider it a bug :)

About the revised code:

  • I made a couple of editorial cleanups.

I suspect you forgot to push this... I'm gonna apply your proposed patch and merge it now anyway (so i can see if any test build fails during the rest of my day). I assume these changes are trivial enough to go directly to master later :)

  • I'd suggest changing the exception for findNSEC3 against out-of-zone to OutOfZone too. Suggested patch is attached.

Ack, applied.

Otherwise the branch looks okay for merge.

Done

comment:16 in reply to: ↑ 15 Changed 8 years ago by jinmei

Replying to jelte:

  • I made a couple of editorial cleanups.

I suspect you forgot to push this... I'm gonna apply your proposed patch and merge it now anyway (so i can see if any test build fails during the rest of my day). I assume these changes are trivial enough to go directly to master later :)

Yeah apparently I forgot to push it. I just merged it to master and
push it to the public repo by hand. I believe it's trivial enough and
okay to skip explicit review.

comment:17 Changed 8 years ago by jinmei

  • Total Hours changed from 0 to 2.25
Note: See TracTickets for help on using tickets.