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

Update Zeek DNS pipeline with ECS DNS fields #13324

Merged
merged 6 commits into from Aug 27, 2019

Conversation

andrewkroh
Copy link
Member

@andrewkroh andrewkroh commented Aug 22, 2019

This adds ECS DNS fields to the Zeek DNS fileset (but does not change or remove any existing ones).

Relates #13320

Needs:

  • dns.resolved_ip
  • dns.question.registered_domain

@andrewkroh andrewkroh mentioned this pull request Aug 22, 2019
6 tasks
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

Looking good as well so far 👍

Left a few comments for small fixes I think are needed.

x-pack/filebeat/module/zeek/dns/config/dns.yml Outdated Show resolved Hide resolved
function addDnsAnswers(evt) {
var answers = evt.Get("zeek.dns.answers");
var ttls = evt.Get("zeek.dns.TTLs");
if (!answers || !ttls || answers.length != ttls.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would hope that doesn't happen, but good check.

But if it does happen, I would suggest appending an error message to that effect to error.message instead of just returning.

Not a blocker, though. Can also be improved later.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'll wait on this. I'd like to find out how common of a condition this will be.

x-pack/filebeat/module/zeek/dns/config/dns.yml Outdated Show resolved Hide resolved
@webmat
Copy link
Contributor

webmat commented Aug 23, 2019

Test failures seem caused by cisco module (ASA?)

cc @adriansr

@andrewkroh andrewkroh marked this pull request as ready for review August 26, 2019 15:38
@andrewkroh andrewkroh requested a review from a team as a code owner August 26, 2019 15:38
@elasticmachine
Copy link
Collaborator

Pinging @elastic/siem

@andrewkroh andrewkroh changed the title Update zeek dns pipeline with ECS DNS fields Update Zeek DNS pipeline with ECS DNS fields Aug 26, 2019
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

LGTM, once changelog is rebased.

Jenkins appears to be flakiness (winlogbeat and libbeat).

Not sure about Travis CI failing in Filebeat:

======================================================================
ERROR: Test stopping filebeat under load: wait for all events being published.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/go/src/github.com/elastic/beats/filebeat/tests/system/test_shutdown.py", line 57, in test_shutdown_wait_ok
    max_timeout=15)
  File "/go/src/github.com/elastic/beats/filebeat/tests/system/../../../libbeat/tests/system/beat/beat.py", line 387, in wait_log_contains
    name=name)
  File "/go/src/github.com/elastic/beats/filebeat/tests/system/../../../libbeat/tests/system/beat/beat.py", line 352, in wait_until
    "Waited {} seconds.".format(max_timeout))
TimeoutError: Timeout waiting for 'log_contains' to be true. Waited 15 seconds.
----------------------------------------------------------------------
XML: /go/src/github.com/elastic/beats/filebeat/build/TEST-system.xml
[success] 16.36% test_autodiscover.TestAutodiscover.test_default_settings: 85.2112s
[success] 7.96% test_autodiscover.TestAutodiscover.test_docker: 41.4820s
[error] 4.88% test_shutdown.Test.test_shutdown_wait_ok: 25.4172s

This adds ECS DNS fields to the Zeek DNS fileset (but does not change or remove any existing ones).

Relates elastic#13320
Use event.original
Remove Z flag
Add registered_domain
Ensure destination.port is a long
@andrewkroh
Copy link
Member Author

It looks like flakiness in unrelated tests. I rebased it to fix the merge conflicts so we'll get another run of CI and check the results again.

@webmat webmat mentioned this pull request Aug 27, 2019
1 task
CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
@andrewkroh andrewkroh merged commit 3a69204 into elastic:master Aug 27, 2019
@cwurm
Copy link
Contributor

cwurm commented Aug 27, 2019

Should we still fill dns.question.etld_plus_one as proposed in elastic/kibana#43757?

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.

None yet

5 participants