Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Packetbeat: add support for EDNS/DNSSEC #1292

Merged
merged 1 commit into from
Apr 20, 2016

Conversation

McStork
Copy link
Contributor

@McStork McStork commented Apr 3, 2016

EDNS
  • Add EDNS OPT meta-RR information in dns.opt field
  • Document EDNS fields
  • Do some EDNS checks (UDP packet size, OPT RR present in request and response)
  • Update vendor miekg/dns (EDNS fix)
DNSSEC
  • Add DNSSEC RRs
Other
  • Refactor rrToMapStr/rrToString to use only one switch case
  • Instantiate a logp.MakeDebug and use it throughout the dns package
  • Add new tests: begin names_test.go for RRs specific tests

@elasticsearch-release
Copy link

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'.

data, ok := mapStr["data"]
delete(mapStr, "data")

// Warn: Fields order is not guaranteed...if we want keys could be sorted though...
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Order is not guaranteed but this loop frees us of implementing a second switch case for the different RR types. This makes RR type code maintenance easier.
Maybe we could find a way to guaranty the order. An alternative would be to autogenerate rrToMapStr/rrToString functions.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. if keys to be extracted are known beforehand:

var keys := []string{ ... }
for _, k := range keys {
    v, ok := mapStr[k]
    if !ok {
      continue
    }

    ...
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely think we should sort by the map keys so that the string is rendered in a deterministic manner each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keys are known beforehand but they are numerous and for code maintenance it would be best to only set the keys in one spot, such as in rrToMapStr.

If we want it to be deterministic and sorted, can we consider using sort.Strings on the keys?
Of course that is not ideal because it runs yet another loop on the hashMap elements. But it would get the job done and ease RR types maintenance.

I guess the best solution would be to generate code with Golang from a file containing the RR types. But I have never used Golang code generation so I don't see it done in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want it to be deterministic and sorted, can we consider using sort.Strings on the keys?

Yeah, iterate over the map and put the keys into an slice, call sort.Strings on the slice of keys, then iterate over the sorted keys when building up the string.

@McStork
Copy link
Contributor Author

McStork commented Apr 3, 2016

The OPT RR is referred to as a pseudo-RR because it contains metadata.
We always map a field in "data" so for this OPT RR, I set it with the edns version. But it doesn't make much sense.
At some point in the future, I suggest to get rid of this "data" field and name the field with the actual mapped field name. Currently, users have to guess what is mapped in "data", or look into the source code.

If this is introduced, a []mapStr could be used (something like this) in the unit tests instead of the current []string for Answers, Additionals,...

@@ -24,6 +24,10 @@ import (
"golang.org/x/net/publicsuffix"
)

var (
debug = logp.MakeDebug("dns")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 consider naming debugf as some linters might check the format string then

@andrewkroh
Copy link
Member

The OPT RR is referred to as a pseudo-RR because it contains metadata.
We always map a field in "data" so for this OPT RR, I set it with the edns version. But it doesn't make much sense.
At some point in the future, I suggest to get rid of this "data" field and name the field with the actual mapped field name. Currently, users have to guess what is mapped in "data", or look into the source code.

Sounds like a good idea.

For the specific case of the OPT RR, another option would be to move the contents of this RR to dns.opt. From what I understand there should only ever be a single one of these included in the "additionals" section. This would make OPT data a bit more usable from Kibana because it would not be embedded in an array like it is now being inside of dns.additionals. You would then be free to not have a dns.opt.data field and could make it dns.opt.version.

@McStork
Copy link
Contributor Author

McStork commented Apr 7, 2016

move the contents of this RR to dns.opt

Terrific. I think I will go in this direction. Users capturing DNS traffic will most likely want to know how many EDNS or DNSSEC (dns.opt.do) messages they got.
EDNS should become more and more popular, so it probably is a good idea to give it more visibility by setting it in a field dns.opt.

@McStork
Copy link
Contributor Author

McStork commented Apr 9, 2016

dependency miekg/dns very recently got a fix for edns, so vendor will be updated on this PR

@McStork
Copy link
Contributor Author

McStork commented Apr 10, 2016

Waiting for miekg/dns#341 to close before updating vendor again.

* Add EDNS OPT meta-RR information in `dns.opt` field
* Document EDNS fields
* Do some EDNS checks (UDP packet size, OPT RR present in request and response)
* Update vendor miekg/dns (EDNS fix)

* Add DNSSEC RRs
* Add new tests: begin `names_test.go` for RRs specific tests

* Refactor rrToMapStr/rrToString to use only one switch case
* Instantiate a logp.MakeDebug and use it throughout the dns package
@McStork
Copy link
Contributor Author

McStork commented Apr 19, 2016

Squashed and rebased

// It would require some changes in unit tests
func rrToString(rr mkdns.RR) string {
var st string
var keys []string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small optimization would be to allocate the slice once since we know the number of keys. keys = make([]string, 0, len(mapStr))

@andrewkroh
Copy link
Member

LGTM. I'm going to deploy the nightlies and test it out. 😄 Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants