Opened 9 years ago

Closed 9 years ago

#609 closed task (complete)

Check BIND10 responses with real-world traffic

Reported by: stephen Owned by: vorner
Priority: medium Milestone: Sprint-20110419
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 4.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Replaying real-world traffic towards a small zone (like root) we can check responses are correct.

Subtickets

Attachments (7)

report.txt (62.8 KB) - added by hanfeng 9 years ago.
bind10_compared_with_bind9.tar.gz (134.2 KB) - added by hanfeng 9 years ago.
Test Report_in-memory db.pdf (168.7 KB) - added by hanfeng 9 years ago.
Test Report_sqlite db.pdf (156.6 KB) - added by hanfeng 9 years ago.
query_cmp.tar (490.0 KB) - added by hanfeng 9 years ago.
example.com.txt.signed (292.6 KB) - added by hanfeng 9 years ago.
example.com.unsigned.norm (51.3 KB) - added by hanfeng 9 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 4

In the estimating session, the consensus seemed to be that the best way to do this was to send real world traffic, one packet at a time, to both a BIND9 and BIND10 server and compare the responses. Things to be aware of:

  • The QID is likely to be different
  • The order of RRs in an RRset may well be different.

comment:2 Changed 9 years ago by jinmei

Comment: we can probably use BIND 9's "digcomp" tool (a perl script). It's in bind9/bin/tests/system/.

comment:3 Changed 9 years ago by zzchen_pku

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

Changed 9 years ago by hanfeng

Changed 9 years ago by hanfeng

comment:4 Changed 9 years ago by hanfeng

We use created zone and script to test the return package from bind9 and bind10, the detail report, zone files and script is in attachment.

comment:5 Changed 9 years ago by hanfeng

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

comment:6 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:7 follow-up: Changed 9 years ago by vorner

Hello

Looking at it, I have some comments and questions.

The report:

It is nice we have a report that looks formally and that, but I guess the reason for this task wasn't to have a report we can store somewhere (we could guess that bind9 and bind10 would give different responses sometime). We probably should do something about the differences. So, would you mind going through the differences and looking if they are problems or not (eg. the difference might or might not be OK regarding the RFC) and possibly creating some tickets for them if they are problematic?

Anyway, what do you mean that some rtypes are not supported? That they can not be loaded? Or that they can be loaded and parsed, but don't get into the response for some reason? Do you think this is due to lack of existence of their classes in the code, or because the classes are broken somehow?

With the problems on TCP, are they relevant to UDP as well (eg. the problem with TCP, does it happen on UDP as well)?

Did you compare bind9 with the in-memory zone or with sqlite3?

The .txt file seems to have slightly broken codepage, there are some odd symbols sometimes. And it is impossible to store gray background in .txt, would it be possible to mark the lines with something else, like putting them into quotes or brackets or something?

The scripts

Would it be possible to integrate them into the source tree? It would be nice if we could run that again. So, if you included them into the tests or tools directory, it might be helpful and your work would come to more use ;-).

But if they should be included, there would be some notes about their quality.

  • They would use some comments and documentation. Reading them is kind of hard without them.
  • Would it be possible to use our own dns libraries? It might be good for the „own dog food“ reasons.
  • Many functions there take an argument where they fill in a result, but they don't return anything otherwise. Is there a reason for this unusual behaviour, or would it be ok if they just created the result and returned it?
  • This can be changed from:
    	if v1 & flagbit == v2 & flagbit:
                     return(True)
    	return(False)
    
    to:
    	return v1 & flagbit…
    
  • What do these magic constants mean? Would it be possible to use some symbolic constant? And why just these numbers?
    pos_total = 80
    pos_title = 14
    pos_left = (pos_total - pos_title) / 2
    
  • The part where you:
    if diff['id']: return(0)
    if diff['qr']: return(0)
    …
    
    Could it be rewritten as a loop with the list variable? Shouldn't it return True/False?? After all, the result doesn't seem to be count of anything, so why use numbers?
  • What does array_cmp do?
  • rr_cmp has some really strange docstring.
  • The README could use some description of what is the purpose of the tool in the directory.
  • I'm not sure if we want tests here. Maybe not, as this is kind of test itself. But if that should become a general purpose tool to compare two servers, we might want to provide some.

Sorry for being so long (which is probably due to quite a lot of code). Though, I don't know if I didn't overlook something.

Thanks

comment:8 Changed 9 years ago by vorner

  • Owner changed from vorner to hanfeng

comment:9 in reply to: ↑ 7 Changed 9 years ago by hanfeng

Replying to vorner:

Hello

Looking at it, I have some comments and questions.

The report:

It is nice we have a report that looks formally and that, but I guess the reason for this task wasn't to have a report we can store somewhere (we could guess that bind9 and bind10 would give different responses sometime). We probably should do something about the differences. So, would you mind going through the differences and looking if they are problems or not (eg. the difference might or might not be OK regarding the RFC) and possibly creating some tickets for them if they are problematic?...

I will create some tickets based on the report.

The scripts

Would it be possible to integrate them into the source tree? It would be nice if we could run that again. So, if you included them into the tests or tools directory, it might be helpful and your work


The script is written by test team in CNNIC, I haven't looked at it. They are familiar with dnspython
so they use it. I don't find document for python binding of BIND10, but the test team colleague will start to look at the source code and some tests in BIND10, then modify the script accordingly. Once they finished, I will upload them.

Thanks for your review.

comment:10 Changed 9 years ago by stephen

  • Milestone A-Team-Task-Backlog deleted

Milestone A-Team-Task-Backlog deleted

comment:11 Changed 9 years ago by shane

  • Milestone set to Sprint-20110405

Changed 9 years ago by hanfeng

Changed 9 years ago by hanfeng

Changed 9 years ago by hanfeng

Changed 9 years ago by hanfeng

Changed 9 years ago by hanfeng

comment:12 Changed 9 years ago by hanfeng

The script is rewritten using bind10 DNS python binding by Jenny.

Test report splits into sqllite part and in memory part which also using different zone file.

To be clear, the test is based on old version of bind10, some bugs can't be reproduced by current version.

comment:13 Changed 9 years ago by hanfeng

  • Owner changed from hanfeng to vorner

comment:14 Changed 9 years ago by vorner

  • Owner changed from vorner to hanfeng

Hello

I looked trough it again. I don't remember seeing so many tickets for the differences found, did anybody create them? (I might have simply overlooked them, of course).

About the code, it's nicer, but there are still few minor points:

  • The _wait_for function, if EINTR happens, it will throw timeout, if I read correctly.
  • Why is the socket in send udp non-blocking? It doesn't matter, you wait for it to become ready anyway, so it can't block and even if it would, you don't really check for EAGAIN. Anyway, checking a new port is ready for write with UDP is useless anyway, it will be just ready (so this might simplify the code little bit).
  • header_cmp: There's bunch of lines like diff['id'] = header1['id'] != header2['id']. They could be written like:
       for header in list:
    	   diff[header] = header1[header] != header2[header]
    

Is it possible to put into a branch in git, so it could be merged? We might want to run the tests again some time in future. Or is there a problem with copyright?

Thanks

comment:15 Changed 9 years ago by hanfeng

  • Owner changed from hanfeng to jenny

comment:16 Changed 9 years ago by jenny

  • Owner changed from jenny to vorner

The test scripts are placed under tools directory of bind10 source tree, named query_cmp.

The branch is trac609 which is already pushed to master.

Pls review and thanks.

Last edited 9 years ago by jenny (previous) (diff)

comment:17 Changed 9 years ago by vorner

  • Owner changed from vorner to jenny

Hello

The code seems OK. Actually, it is not pushed to master, master is the „main branch“ in git.

Anyway, feel free to merge it.

Thanks

comment:18 Changed 9 years ago by jenny

  • Owner changed from jenny to vorner

Hi, vorner

I have already merged trac609 and deleted the branch.

Pls review and thanks.

comment:19 Changed 9 years ago by vorner

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

ACK, thanks. Let's close it.

Note: See TracTickets for help on using tickets.