Opened 10 years ago

Closed 9 years ago

#50 closed enhancement (complete)

Review data source and query logic

Reported by: each Owned by: jinmei
Priority: high Milestone: 02. Running, functional authoritative-only server
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description

branches/each-ds, which branched from the trunk at r754, contains a fully functional authoritative name server, including DNSSEC and ANY-query support. There is still a fair bit of cleanup needed, and a notable lack of unit tests, but if possible I'd like to get this merged to the development branch ASAP and then do additional work in new branches. Can someone review?

Subtickets

Change History (20)

comment:1 follow-ups: Changed 10 years ago by each

Jinmei suggested I break this down into more manageable pieces. I'm not sure I can actually separate this into different branches, but I can logically separate the work items. Some of these could probably be taken on by different reviewers.

Note that most of these do not have unit tests yet, and I'm informed that's a prerequisite for review. Unit tests should be completed soon.

Task 1:
src/lib/auth/cpp/data_source.{cc,h}, src/lib/auth/cpp/query.h: [new code] high-level data source and query logic

Task 2:
src/lib/auth/cpp/data_source_{static,sqlite3}.{cc,h}: [new code] concrete data source implementations

Task 3:
src/lib/dns/cpp/name.{cc,h}: [changed code] Name.reverse() (already reviewed)

Task 4:
src/lib/dns/cpp/rrsetlist.{cc,h}: [new code] class containing a vector of RRsets which can be iterated or directly accessed by an integer index or an rrtype identifier

Task 5:
src/lib/dns/cpp/{message,rrset}.{h,cc}: [changed code] rrsets now contain a pointer to an additional rrset for RRSIGs, and support new methods addRRsig(), getRRsig() and delRRsig(). (this is likely to be rendered moot by work jinmei is doing in another branch)

Task 6:
src/lib/dns/cpp/hex.{h,cc}: [new code] functions for converting a blob of data into a string of hex numbers, or vice versa; used for rendering and parsing DS records

src/lib/dns/cpp/rdata/generic/{dname,ds,dnskey,nsec}*: [new code] implemented new rdatatypes

src/lib/dns/cpp/rdata/generic/{mx,rrsig,soa}: [changed code] added missing functionality

comment:2 Changed 10 years ago by each

Unit tests have now been added for the new rdata types, hex.cc, and rrsetlist.cc. I've omitted rrset.cc as it seems likely to be superseded by work Jinmei is doing. I believe unit tests for the SQL data source are in progress, and I'll be doing the upper-level data source next. I think the changes in src/lib/dns/cpp are now ready for review, and the changes in src/lib/auth/cpp are next.

comment:3 Changed 10 years ago by shane

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

comment:4 in reply to: ↑ 1 Changed 10 years ago by jinmei

Replying to each:

Task 6:
src/lib/dns/cpp/hex.{h,cc}: [new code] functions for converting a
blob of data into a string of hex numbers, or vice versa; used for
rendering and parsing DS records

I've made some changes to hex.cc, mostly cleanup:

  • we don't need stringstream in this case.
  • since the resulting length is known it's wise to reserve the space beforehand.

(I'd also remove assert because it's pretty obvious from the code, but
didn't touch it in this commit (rather I made it a bit stricter)).

encodeHex() should accept lower case hex characters.
hex stuff needs more test including pathological cases. I've added
some, which currently fail. the implementation should be fixed so
that these will pass.

I'll give this piece back to the author at this point.

comment:5 in reply to: ↑ 1 ; follow-up: Changed 10 years ago by jinmei

Replying to each:

Task 6:
src/lib/dns/cpp/rdata/generic/{dname,ds,dnskey,nsec}*: [new code]
implemented new rdatatypes

dnskey didn't pass the test:

[ FAILED ] RdataTest?.getID_DNSKEY
8:46 rdata_unittest.cc:648: Failure
Value of: rdata_dnskey.getID()

Actual: 41676

Expected: 12892
[ FAILED ] RdataTest?.getID_DNSKEY (0 ms)

The patch copied below, which naively converts the data to the wire
format and apply the reference code described in RFC4034 Appendix B,
made it pass. I suspect it's an endian issue.

Also, if this doesn't have to be super fast, the naive patch version
may suffice as a permanent fix. Note also that we might have
wire-format "cache" when constructing it from wire if performance
matters in future versions.

And also, if this doesn't have to be super fast and we simply use the
naive version, this feature doesn't have to be a member function.

Besides, the current implementation could refer to a bogus point of
buffer:

return ((impl_->keydata_[len - 3] << 8) + impl_->keydata_[len - 2]);

because len may be smaller 3.

BTW "ID" is not the term used in RFC4034. Why not using "(key) tag"?

I've not looked into the other part of the code. Returning it to the
author at this point.

Index: dnskey_48.cc
===================================================================
--- dnskey_48.cc (revision 957)
+++ dnskey_48.cc (working copy)
@@ -172,6 +172,20 @@

const uint16_t
DNSKEY::getID() const
{

+ OutputBuffer? b(0);
+ toWire(b);
+ const unsigned char* key = static_cast<const unsigned char*>(b.getData());
+ size_t keysize = b.getLength();
+
+ uint32_t ac = 0;
+ int i;
+ for (ac = 0, i = 0; i < keysize; ++i) {
+ ac += (i & 1) ? key[i] : key[i] << 8;
+ }
+ ac += (ac >> 16) & 0xffff;
+ return (ac & 0xffff);
+
+#ifdef notworking

if (impl_->algorithm_ == 1) {

int len = impl_->keydata_.size();
return ((impl_->keydata_[len - 3] << 8) + impl_->keydata_[len - 2]);

@@ -193,6 +207,7 @@

ac += (ac >> 16) & 0xffff;
ac &= 0xffff;
return (ac);

+#endif

}


const uint16_t

comment:6 in reply to: ↑ 5 ; follow-up: Changed 10 years ago by each

  • Owner changed from each to jinmei

The patch copied below, which naively converts the data to the wire
format and apply the reference code described in RFC4034 Appendix B,
made it pass. I suspect it's an endian issue.

I would rather not convert to wire format just to compute a checksum; I'd rather fix the bug. What system were you running on?

Besides, the current implementation could refer to a bogus point of
buffer:

return ((impl_->keydata_[len - 3] << 8) + impl_->keydata_[len - 2]);

because len may be smaller 3.

Addressed, throws an exception.

BTW "ID" is not the term used in RFC4034. Why not using "(key) tag"?

"keyid" is used a lot in bind9. I don't particularly care.

I'll work on this issue, but I'm assigning the ticket back to you as there are six other rdata types to review. I could just merge them to trunk and fix bugs later, but I'd much rather someone at least reads them first...

comment:7 in reply to: ↑ 6 Changed 10 years ago by jinmei

Replying to each:

I'll work on this issue, but I'm assigning the ticket back to you as
there are six other rdata types to review. I could just merge them to
trunk and fix bugs later, but I'd much rather someone at least reads
them first...

General: please first add more detailed test cases especially for the
"from string" constructors, including many bogus ones. you'll soon
find a bunch of bugs:-) See also how the generic Rdata (for unknown RR
types) are struggling with pathological cases. [but it may be too
much for you to do this work for all of these RDATA classes. I'll
also have to take on some parts of it, I guess]

It won't be efficient to perform review/revise cycles without detailed
tests.

I've also directly fixed one minor nit in the MX implementation.

Some sanity level checks follow (it's not a complete review).

SOA: I was not sure what's new.

RRSIG: (it was done by me but:-) maybe we should also use 'key tag'
than ID for RRSIG, too. Again, I was not sure what's new.

DNSKEY:

  • unnecessary const (fixed)
  • (I'm also bad in this point but) I hear many C++ programmers say shortening variable names awkwardly is a bad "C-way" practice. In this sense I'd rename "getAlg" to "getAlgorithm".
  • from wire constructor: validate rdata_len first.
  • from wire constructor: I believe digest can be more efficiently constructed, but I'd first like to see unit tests.
  • getTag(): I still don't see the need for it:-) but aside, I don't think checking len here is appropriate. This should have been done much earlier, I guess.

DNAME: looks so trivial, so no comment:-)

DS:

  • from wire constructor: validate rdata_len first.
  • from wire constructor: I believe digest can be more efficiently constructed, but I'd first like to see unit tests.

NSEC:

  • from wire: same comments
  • toText: throwing this way seems counter intuitive. If the object is validly constructed we should be able to assert or just ignore such invalid cases in this context. (I didn't follow the code logic. it's too complicated for late night:-)

comment:8 in reply to: ↑ 1 Changed 10 years ago by jinmei

Replying to each:

Task 3:
src/lib/dns/cpp/name.{cc,h}: [changed code] Name.reverse() (already reviewed)

We need more tests here. At least we should seriously consider if
it's really sufficient. In general there should be some pathological
and unusual (very long of something, etc) cases.

I've added one simple case.

comment:9 Changed 10 years ago by jinmei

copied from bind10-staff:

To: BIND 10 Development <bind10-staff@…>
Cc: undisclosed-recipients: ;
Subject: Re: [bind10-staff] BIND 10 #50: Review data source and query logic
From: Evan Hunt <each@…>
Date: Fri, 26 Feb 2010 15:21:40 +0000

SOA: I was not sure what's new.

This was my mistake, I had merged a change from trunk and forgotten, so it
showed up as a changed file in each-ds.

RRSIG: (it was done by me but:-) maybe we should also use 'key tag'
than ID for RRSIG, too. Again, I was not sure what's new.

Oops, I thought I'd changed getID to getTag there too.

What's new is DNSSECTime. (See src/lib/cpp/dnstime.{cc,h})

Thanks for the other comments, will address today.

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

DNSSECTime:

  • unit tests please

(sanity check level comments follow)

  • I'd avoid hardcoding the magic number of 14
  • I'd stronglly suggest DNSSECTimeToText return the resulting string (for a couple of reasons, omitted in case you simply agree). passing 's' doesn't make sense in this case IMO, and, if it's really necessary, at least s.reserve(14) does a bad thing as it may reduce the buffer.
  • I'd also avoid sscanf for a couple of reasons. But especially for year1 this may be a pedantic point. I'm okay with keeping it as long as we revisit it as a post-year1 fodder.

comment:11 in reply to: ↑ 1 Changed 10 years ago by jinmei

Replying to each:

src/lib/dns/cpp/rrsetlist.{cc,h}: [new code] class containing a vector of RRsets which can be iterated or directly accessed by an integer index or an rrtype identifier

I've already created a separate ticket on this stuff and made some
comments. Since this seems to independent from others let's handle it
in the separate ticket rather than in this monolithic ticket.

http://bind10.isc.org/ticket/55

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

  • unit tests please

It's fully covered by the RRSIG unit test, but we can add another one just for dnstime if you think it's necessary.

  • I'd also avoid sscanf for a couple of reasons. But especially for year1 this may be a pedantic point. I'm okay with keeping it as long as we revisit it as a post-year1 fodder.

I think we should probably just keep sscanf(). I've used input string streams everywhere I could, but as near as I can determine, you can't use them to read numbers with no separator in-between, except by reading the whole thing as a string and then breaking the string up into pieces and running each one through a separate stringstream, which is cumbersome and inefficient. It makes sense to fall back to the C mechanism when it works substantially better than the C++ one, and in fact the C++ websites and programmers I consulted specifically recommend sscanf() for this kind of thing.

I did add a check for string length before calling sscanf(), however.

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

Replying to each:

  • unit tests please

It's fully covered by the RRSIG unit test, but we can add another one just for dnstime if you think it's necessary.

  • I'd also avoid sscanf for a couple of reasons. But especially for year1 this may be a pedantic point. I'm okay with keeping it as long as we revisit it as a post-year1 fodder.

I think we should probably just keep sscanf().

Oh, I didn't know sscanf is included in the standard C++ library (part
of "imported" C library). My main concern was portability (for a
strict C++-oriented environment) but since it doesn't seem to be an
issue in this case, I'm fine with the use of sscanf().

And it probably makes more sense as it seems to be incorporated from
the BIND 9 implementation, which is well proven to work.

comment:14 in reply to: ↑ 13 Changed 10 years ago by jinmei

Replying to jinmei:

Replying to each:

  • unit tests please

It's fully covered by the RRSIG unit test, but we can add another one just for dnstime if you think it's necessary.

As far as I can see there are only two dnstime relate tests in RRSIG, so I wouldn't call it 'fully covered'. Also, "ToText?" part isn't tested at all.

As for whether we need to create a separate test class for dnstime, yes, I think so. In general test cases of a particular module should be defined as close as to the module itself ("dnstime" in this case) rather than tests as part of a separate module.

for RRSIG, it should be sufficent to have one "bad from text" case that triggers InvalidTime?.

I've made changes according to the above policy. The added test cases uncovered some of bugs the dnstime implementations. Please fix them so that all of the tests will pass. Also, there should also be more tests. Please add them.

And one bikeshed point: public function names DNSSECTimeToText and DNSSECTimeFromText don't follow our coding guideline:

Class names are LikeThis?, methods are likeThis(), variables are like_this, and constants are LIKE_THIS.

Although there's one subtle point that in C++ global functions are not "methods".

BTW I think this module is pretty independent from other ticket #50 stuff, so I'll create a separate ticket for this so that ticket #50 is more lightweight (or simple becomes an "omnibus ticket")

comment:15 Changed 10 years ago by jinmei

NSEC Rdata implementation needs non trivial fixes. I've made a separate ticket for it (#62).

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

For task #1 and #2, I created a separate ticket (#64)

comment:17 Changed 10 years ago by shane

What is the status of this ticket? Last change was 6 weeks ago, and it's not clear if it is waiting on anything...

comment:18 in reply to: ↑ 16 ; follow-up: Changed 10 years ago by jinmei

Replying to shane:

What is the status of this ticket? Last change was 6 weeks ago, and it's not clear if it is waiting on anything...

Like the DNS library module, the problem is that this ticket is "too big".

I suggest we forget things under lib/dns in the context of this ticket and get them reviewed as part of the DNS library module.

Then, we can focus on things under lib/auth. These have been reviewed to some extent, but at that time we've been making additional changes in parallel, so they may not be considered "fully reviewed".

To be sure, it would be nice to have one more review cycle for these. I'd separate them into the following set:

  1. data_source.{h,cc} and query.{h,cc} (and their tests)
  2. sqlite3_datasrc.{h,cc} (and their tests)
  3. static_datasrc.{h,cc} (and their tests)

I had substantially modified 2 and 3 while I gave them my review, so someone else should review these.

I also substantially modified 1, but I think I can safely be a reviewer of it.

Notes:

  • There are specific open issues on this module already identified in previous reviews and ticketed.
  • One common, big issue for this module is the lack of documentation. I suspect we can complete it for the next release, so it will probably be separate open tickets.
  • As I said somewhere (in another ticket or bind10-dev), I suspect we'll need to substantially revisit the whole design and implementation of this stuff (as well as the main auth server code in src/bin/auth) anyway. Assuming we agree on this, perhaps we can simply complete a lightweight review just for the next week's release, and restart the whole work mostly from the scratch.

comment:19 in reply to: ↑ 18 Changed 10 years ago by each

Replying to jinmei:

I suggest we forget things under lib/dns in the context of this ticket and get them reviewed as part of the DNS library module.

I had the impression these things had already been split out into other tickets (and in many cases reviewed). I'm not sure this ticket needs to be open any longer.

  1. data_source.{h,cc} and query.{h,cc} (and their tests)
  2. sqlite3_datasrc.{h,cc} (and their tests)
  3. static_datasrc.{h,cc} (and their tests)

I had substantially modified 2 and 3 while I gave them my review, so someone else should review these.

I paid close attention to your modifications while you were making them. I'm not averse to going over it again, or having someone else do so, but I actually think it would be reasonable to consider these reviewed.

  • As I said somewhere (in another ticket or bind10-dev), I suspect we'll need to substantially revisit the whole design and implementation of this stuff (as well as the main auth server code in src/bin/auth) anyway. Assuming we agree on this, perhaps we can simply complete a lightweight review just for the next week's release, and restart the whole work mostly from the scratch.

I don't think I agree with "from scratch". After the initial release I went over the current code with an eye toward redesign, and I actually think what we've got isn't bad. Its major weaknesses are performance (attributable to slow and repetitive SQL queries and no cache), glitches with zone cuts, and of course non-writability. Some new API will be needed for writing to the database, and a redesign of the existing API might help with the zone cut issues, but the general approach we're taking still seems fairly reasonable to me.

comment:20 Changed 9 years ago by shane

  • Resolution set to complete
  • Status changed from assigned to closed

Based on Jinmei's and Evan's last comments, I will consider this work substantially complete, and resolve this ticket.

Thanks guys.

Note: See TracTickets for help on using tickets.