Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: dns
    • Labels:
      None

      Description

      Hello,

      Here are the first set of changes I'd like to contribute to the
      protocol-dns subproject. Here's a list of things I've done.

      1) Converted the enumeration types ServiceType, ProtocolType,
      RecordClass, OpCode, RecordType, MessageType and ResponseCode into
      actual Java 5 enums. Because we need to encode and decode the actual
      values we can't rely on the ordinals the compiler will assign to them,
      so I've used a technique I found on the Java Specialists site
      (http://www.javaspecialists.co.za/archive/newsletter.do?issue=113), to
      efficiently do a conversion between enums and values with minimal code
      duplication.

      2) Removed QuestionRecords and ResourceRecords classes since they were
      really just type-safe wrappers around Lists and replaced their usage
      with generic Lists.

      3) Moved any encoding and decoding code that was in the MINA
      ProtocolEncoder and ProtocolDecoder to classes in the
      o.a.d.s.dns.io.encoder and io.decoder packages called DnsMessageEncoder
      and DnsMessageDecoder, respectively.

      4) Removed the dependency by the encoders and decoders in the
      o.a.d.s.dns.io packages on MINA. The only thing they really used were
      the ByteBuffers from MINA and it was a pretty simple task to just switch
      to java.nio.ByteBuffers instead. The only hurdle I had to overcome was
      in the allocation of temporary buffers for things like enecoding domain
      names, which leads me to...

      5) Instead of creating (or having to pull them from the MINA ByteBuffer
      pool) several ByteBuffers during encoding, I simply wrote directly to
      the buffer that was passed in. From what I could tell, the main reason
      for allocating the temporary buffers was to do the actual encoding of
      the domain name, get the byte array that was written to the buffer,
      write the length of the data to the actual ByteBuffer, then write the
      byte array from the temporary ByteBuffer. Instead, what I did was to
      record the position of the ByteBuffer, increment it by one, write the
      data (the domain name for instance), find the difference between the
      starting position + 1 and the current position, then go back to the
      starting position and write the length of the data and finally jump back
      to the end of the data and go onto the next bit of encoding. This
      should be more efficient because we're not constantly having to get
      ByteBuffers from a pool or have new ones allocated, and we also don't
      have to then create byte arrays and have the data from those temporary
      ByteBuffers read into the byte arrays.

      6) Moved all the (get|put)Unsigned(Byte|Short|Int) methods into a
      utility class called ByteBufferUtil.

      7) Changed the DnsMessage.transactionId property to be an int because it
      actually needs to be an unsigned short.

      8) Fixed a bug with decoding domain names when they are compressed. The
      new position to jump to wasn't being calculated right all the time.
      There also seemed to be an assumption that the first thing to do after
      jumping was to read a label, but it could be the case that the new
      position is another jump so we need to call recurseDomainName() right
      after doing the jump in position.

      9) Updated existing tests to actually check that what is encoded and
      decoded are correct. For this I also had to add equals() and hashCode()
      methods to classes that didn't already have them, like DnsMessage,
      ResourceRecordImpl, and others. I simply used the commons-lang equals
      and hashCode builders for this because I've found them to be
      tremendously useful for this in the past.

      10) Added additional test for MX query and response parsing. I used the
      MX records from apache.org.

      11) Added decoders to make the tests added in (10) pass. these are the
      IPv6RecordDecoder, MailExchangeRecordDecoder, AddressRecordDecoder, and
      NameServerRecordDecoder.

      That should cover it all. I'd like to add a few more decoders for
      things like TXT and SOA records. That should cover about 90% of the
      records in use on most DNS servers.

      Then I'd like to get things separated a little bit more and create
      subprojects within protocol-dns. I'm thinking something like
      protocol-dns/
      core/
      mina-shared/
      server/
      shared/
      mina/
      store/
      client/

      core would contain all the DNS message and record types as well as the
      decoders and encoders in dns.io right now. mina-shared would contain
      the ProtocolCodec stuff in dns.protocol.

      server/shared would contain the DnsConfiguration and other stuff that
      would be shard among server implementations, whether they use MINA or
      some other IO framework and whether they use ApacheDS for the store or
      some other backend like Bind zone files.

      server/mina would contain the DnsProtocolHandler from dns.protocol as
      well as the DnsServer class itself.

      server/store would contain the the backend store type stuff. It would
      probably wind up being separate subprojects itself, one for LDAP, JDBC,
      or Bind zone files.

      client would obviously contain any code for developers to generate and
      send queries to dns servers, and maybe an implementation of the dig
      command line tool.

      I wouldn't mind actually seeing this becoming a separate project of it's
      own, like dns.apache.org. That would be really cool.

      What do you think of these changes?

      1. patch2.diff
        24 kB
        Richard Wallace
      2. MX-TRAFFIC.libpcap
        0.4 kB
        Richard Wallace
      3. MX-RESPONSE.pdu
        0.3 kB
        Richard Wallace
      4. MX-QUERY.pdu
        0.0 kB
        Richard Wallace
      5. patch.diff
        165 kB
        Richard Wallace

        Activity

        Hide
        Enrique Rodriguez added a comment -

        Second patch applied, adding javadocs and replacing ByteBufferUtil with MINA's ByteBuffer:
        URL: http://svn.apache.org/viewvc?view=rev&rev=501531

        Show
        Enrique Rodriguez added a comment - Second patch applied, adding javadocs and replacing ByteBufferUtil with MINA's ByteBuffer: URL: http://svn.apache.org/viewvc?view=rev&rev=501531
        Hide
        Richard Wallace added a comment -

        I removed the ByteBufferUtil and just used the MINA ByteBuffer class instead.

        I added comments to the decoders and a few other places.

        Show
        Richard Wallace added a comment - I removed the ByteBufferUtil and just used the MINA ByteBuffer class instead. I added comments to the decoders and a few other places.
        Hide
        Richard Wallace added a comment -

        I can work on doing that. I also plan on adding more tests cause I hate doing refactorings without having a safety net.

        Show
        Richard Wallace added a comment - I can work on doing that. I also plan on adding more tests cause I hate doing refactorings without having a safety net.
        Hide
        Emmanuel Lecharny added a comment -

        Is it possible that some javadoco being added to the patch ? it's a 164 Kb patch without doco, the best way to be not supported by any of us ...

        Patches are good to have, but patches without doco is like bing lost in a foreign city without map ...

        Show
        Emmanuel Lecharny added a comment - Is it possible that some javadoco being added to the patch ? it's a 164 Kb patch without doco, the best way to be not supported by any of us ... Patches are good to have, but patches without doco is like bing lost in a foreign city without map ...
        Hide
        Enrique Rodriguez added a comment -

        Binary test PDU's for MX-QUERY and MX-RESPONSE were applied and the unit tests were enabled.

        Patch applied:
        URL: http://svn.apache.org/viewvc?view=rev&rev=500956

        Binary PDU's added and tests enabled:
        URL: http://svn.apache.org/viewvc?view=rev&rev=501165

        Show
        Enrique Rodriguez added a comment - Binary test PDU's for MX-QUERY and MX-RESPONSE were applied and the unit tests were enabled. Patch applied: URL: http://svn.apache.org/viewvc?view=rev&rev=500956 Binary PDU's added and tests enabled: URL: http://svn.apache.org/viewvc?view=rev&rev=501165
        Hide
        Enrique Rodriguez added a comment -

        I wouldn't consider backporting anything unless there was demand for it, like a critical bug fix or security flaw or a highly desired feature. For now, DNS is unstable and so anyone working with it will be building from the latest trunk. So, unless there's a good reason I'd rather not support 2 versions, especially since I doubt anyone is using it in earlier versions.

        Show
        Enrique Rodriguez added a comment - I wouldn't consider backporting anything unless there was demand for it, like a critical bug fix or security flaw or a highly desired feature. For now, DNS is unstable and so anyone working with it will be building from the latest trunk. So, unless there's a good reason I'd rather not support 2 versions, especially since I doubt anyone is using it in earlier versions.
        Hide
        Richard Wallace added a comment -

        Here is the libpcap file of the traffic generated during the request. I'm including it for completeness and to follow with what is already there for DNS-TRAFFIC.libpcap.

        Show
        Richard Wallace added a comment - Here is the libpcap file of the traffic generated during the request. I'm including it for completeness and to follow with what is already there for DNS-TRAFFIC.libpcap.
        Hide
        Richard Wallace added a comment -

        Here are the MX-* files for the tests I added. Just drop them in the same directory as the DNS-QUERY.pdu and they should be good to go.

        Show
        Richard Wallace added a comment - Here are the MX-* files for the tests I added. Just drop them in the same directory as the DNS-QUERY.pdu and they should be good to go.
        Hide
        Emmanuel Lecharny added a comment -

        Thanks Enrique for handling this issue.

        Just a question : do we have to backport it to 1.0-runks or not? At this point, I don't know if it's a good idea. Let's say there are pros and cons
        pros :

        • 1.0 is stable (well, almost and we are clse to cut a 1.0.1 version
        • the patch should not be too complicated to apply to 1.0.1
          cons :
        • 1.0 is stable, so should we consider the risk of destabilize it ?
        • managing 2 version is a burden

        Well, I don't know... wdyt ?

        Show
        Emmanuel Lecharny added a comment - Thanks Enrique for handling this issue. Just a question : do we have to backport it to 1.0-runks or not? At this point, I don't know if it's a good idea. Let's say there are pros and cons pros : 1.0 is stable (well, almost and we are clse to cut a 1.0.1 version the patch should not be too complicated to apply to 1.0.1 cons : 1.0 is stable, so should we consider the risk of destabilize it ? managing 2 version is a burden Well, I don't know... wdyt ?
        Hide
        Enrique Rodriguez added a comment -

        1) Patch applied on revision 500956.
        2) Decoding tests disabled pending submission of missing binary test PDU's.

        Show
        Enrique Rodriguez added a comment - 1) Patch applied on revision 500956. 2) Decoding tests disabled pending submission of missing binary test PDU's.

          People

          • Assignee:
            Enrique Rodriguez
            Reporter:
            Richard Wallace
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development