DNS over TCP

[McStork]

EARLY REVIEW please

Hi !
This is an attempt at implementing TCP, EDNS, DNSSEC.
For now my code only contains the TCP implementation for the DNS protocol. Once it's done, EDNS and DNSSEC should follow using the miekg/dns library (see #345 ).

I wrote some unit tests but failed at even running the already existing integration tests. I wish someone could help me with that:

. env/bin/activate
nosetests test_0025_mongodb_basic.py:Test.test_write_errors
E
======================================================================
ERROR: Should set status=Error on a bulk write that returns errors.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/workspace/src/github.com/elastic/packetbeat/tests/system/test_0025_mongodb_basic.py", line 197, in test_write_errors
    debug_selectors=["mongodb"])
  File "/root/workspace/src/github.com/elastic/packetbeat/tests/system/pbtests/packetbeat.py", line 84, in run_packetbeat
    stderr=subprocess.STDOUT)
  File "/usr/lib/python2.7/subprocess.py", line 679, in __init__
    errread, errwrite)
  File "/usr/lib/python2.7/subprocess.py", line 1259, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory

Any feedback on the code is welcomed. Thanks.
I also could use some help in order to know how to implement messageParser() and GapInStream(). Should the checks only rely on the decoder ? That's how it's managed for now.

[McStork]

Can the commit failures be repaired ?

[tsg]

@McStork this looks really promising!

Regarding the integration tests, I think the reason is that the "test" binary is not there. We use a special binary for the tests so we can get line coverage reports.

You have two options to create it:

• make packetbeat.test in the top folder of the repo. Note that you'll have to do this explicitly after doing code changes in Packetbeat.
• Simply run all tests once: make testsuite. This should create that binary for you as well as confirm that existing tests are ok. I just tried this in your branch and it ran fine.

[ruflin]

@McStork no worries about all the commits. You can rebase and squash at the end.

[andrewkroh]

@McStork It would make reviewing easier if you do the conversion to use miekg/dns in a separate pull request. So when you are ready, we will review this PR and get it merged in and you can open a second PR for the other changes. I briefly looked over this PR and it looks good.

[tsg]

@McStork I was wondering about the state of this PR, because we're planning to do a Github repos reorganization this week. We're merging all the official beats in a single repo. They way we do it is that we'll move the code from the other repos into this one, so this PR will still live.

However, because all Packetbeat files will be moved under a packetbeat/ folder, I expect it to result in a lot of merge/rebase conflicts. What do you think, is there any chance we could bring this to a mergeable state quickly and improve on it with future PRs? If not, I'm happy to also help with the rebase work after we do the repos reorg.

[McStork]

@tsg Well, I don't think I will be able to work on this PR this week. Right now Packetbeat is not top priority on my TODO list (but it will be), as I have a deadline coming very soon on another project.

What is left is debuging the TCP decode: the integration test doesn't decode the DNS over TCP packets, as if gopacket doesn't support it. That's what is left to do on this PR. But I can't guarantee you that I will be able to look into it this week :(.

[tsg]

Thanks for the quick reply. We'll then proceed to do the repositories reorganization and afterwards I'll try to spend some time helping with the review and the rebase to master. In any case, I'd really like to see this through :-)

[McStork]

Could solve the issue ... in order to decode the payload of DNS TCP packets, it is needed to skip the first two bytes. So basically there is an offset of 2: decodeDNS(payload[2:]). If you guys know why, please let me know.

The integration test is validated and the live capture seems to work.
However I tried to use packets generated with gopacket/layers/testcreator.py for the unit tests, but gopacket fails to decode them (with or without the offset). So all TCP unit tests are commented until I can identify why the generated packets aren't decoded by gopacket.

[tsg]

Tests are failing on Travis & Appveyor due to the repo changes we did, you can ignore those until we do the rebase to master.

No idea about the 2 bytes, sorry, but looks like you're making great progress.

[elasticsearch-release]

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

[andrewkroh]

@McStork The gopacket/layers/testcreator.py script generates byte arrays that contain everything from the Ethernet layer up. If you didn't account for this, then gopacket will probably fail to parse the data.

[McStork]

@andrewkroh Yup. Thanks, I saw the (yours?) comment at the top of the dns test file.
Every test validate now.

[andrewkroh]

@McStork Great to hear you fixed the tests. Can you rebase the PR on master and squash the commits?

[McStork]

How is that ? :/ I'm new to git, plus with the recent changes in this repo, I'm not sure if I ran the good command lines.... but somehow I doubt it.

[andrewkroh]

It looks like something went wrong with your rebase. I took the liberty of attempting the rebase for you. If you would like, you can do this to pull down my rebased version of your changes and test them to make sure things still work.

git checkout -b dns-tcp-rebase
git pull https://github.com/andrewkroh/beats.git mcstork-dns-tcp-rebase
git push

From there, the simplest thing to do would be open a new PR and we will just close this one. Another option would be to force push the dns-tcp-rebase branch to your dns branch which would update this PR.

[McStork]

I will try to open a new PR then.
Out of curiosity and to get more skilled at git, could you give me the commands you ran for the rebase ?

To use your rebase, I ran those:

git checkout -b dns-tcp-rebase
git pull https://github.com/andrewkroh/beats.git mcstork-dns-tcp-rebase
git push -f origin dns-tcp-rebase

Will the PR be ok with this forced push ? :/

If I don't force with -f and do just git push origin dns-tcp-rebase, I get:

To https://github.com/McStork/packetbeat.git
 ! [rejected]        dns-tcp-rebase -> dns-tcp-rebase (non-fast-forward)
error: failed to push some refs to 'https://github.com/McStork/packetbeat.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
hint: before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

[andrewkroh]

Well the rebase was slightly more complex because I was rebasing on your behalf. But normally you do this (there are many ways to accomplish this task):

# Get latest from elastic master
git checkout master
git pull

# Rebase master into your feature branch
git checkout feature/my-feature
git rebase master

# You have to force push because you are re-writing the remote's history
git push -f

[McStork]

@andrewkroh Thanks !