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] Update DNS protocol to use ECS fields #9941

Merged
merged 8 commits into from
Jan 10, 2019

Conversation

andrewkroh
Copy link
Member

@andrewkroh andrewkroh commented Jan 8, 2019

This updates the DNS protocol to have more closely follow ECS.
The DNS tunneling dashboard has been updated to work with the new
field names.

In order to better interoperate with other data sources the trailing dot
has been removed from domain names. For example, previously Packetbeat
would produce dns.question.name:elastic.co. and now it will simply produce
dns.question.name:elastic.co. It's a breaking change but it will be make it
easier to pivot with other data sources.

Part of #7968

Here's a summary of what fields changed.

Changed

  • bytes_in -> source.bytes
  • bytes_out -> destination.bytes
  • notes -> error.message
  • responsetime -> event.duration (unit are now nanoseconds)
  • transport -> network.transport

Added

  • event.end
  • event.start
  • network.bytes
  • network.community_id
  • network.protocol = dns
  • network.type

Unchanged Packetbeat Fields

  • method - dns opcode
  • query = class {{ dns.question.class }}, type {{ dns.question.type }}, {{ dns.question.name }}
  • request - text representation of the entire request
  • response - text representation of the entire response
  • resource - dns.question.name
  • status
  • type = dns (we might remove this since we have event.dataset)

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Could you add a changelog entry and update ecs-migration.yml. I assume also some updates to fields.yml for dns are needed?

@@ -532,6 +532,30 @@ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.
--------------------------------------------------------------------
Dependency: github.com/elastic/ecs
Revision: 69de90eb6493e0804405321f48adfdfa488d6498
Copy link
Member

Choose a reason for hiding this comment

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

My first though was to fix it to Beta2 but at this stage the go code was not in yet :-(

Copy link
Member Author

@andrewkroh andrewkroh Jan 8, 2019

Choose a reason for hiding this comment

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

Luckily the code generated from this commit is exactly the same as what would be in beta2 if I backported (I started a backport and realized they’d be the same).

@andrewkroh
Copy link
Member Author

The are no changes needed to fields.yml in this PR. When I finish updating the last protocol in Packetbeat then there should be several updates to removing unused fields. And then I can add aliases to ecs-migration.yml. None of the DNS specific fields were changed.

I’ll update the changelog.

@andrewkroh
Copy link
Member Author

CHANGELOG.next is now updated.

@urso urso removed the request for review from a team January 8, 2019 17:41
@ruflin
Copy link
Member

ruflin commented Jan 9, 2019

As far as I can see for example bytes_out was renamed to destination.bytes? That would mean bytes_out in the fields.yml should become an alias to destination.bytes? Or you can't change it yet because it's also used in other places / protocols?

For the ecs-migration.yml: I would prefer to have a list for each protocol so if we map bytes_out to destination.bytes multiple time, it's ok if it shows up there multiple times.

What we often do in these ECS migration PR's is also having in the PR description the list of fields which we renamed.

This updates the DNS protocol to have more closely follow ECS.
The DNS tunneling dashboard has been updated to work with the new
field names.

In order to better interoperate with other data sources the trailing dot
has been removed from domain names. For example, previously Packetbeat
would produce `dns.question.name:elastic.co.` and now it will simply produce
`dns.question.name:elastic.co`. It's a breaking change but it will be make it
easier to pivot with other data sources.

Part of elastic#7968

Here's a summary of what fields changed.

Changed

- bytes_in -> source.bytes
- bytes_out -> destination.bytes
- notes -> error.message
- responsetime -> event.duration (unit are now nanoseconds)
- transport -> network.transport

Added

- event.end
- event.start
- network.bytes
- network.community_id
- network.protocol = dns
- network.transport = udp/tcp
- network.type

Unchanged Packetbeat Fields

- method - dns opcode
- query = class {{ dns.question.class }}, type {{ dns.question.type }}, {{ dns.question.name }}
- request - text representation of the entire request
- response - text representation of the entire response
- resource - dns.question.name
- status
- type = dns (we might remove this since we have event.dataset)
No aliases yet because these fields are still used.
@andrewkroh
Copy link
Member Author

andrewkroh commented Jan 9, 2019

@ruflin I added a few fields to ecs-migration.yml (all with alias: false because the fields are still being used by other protocols). And I updated the PR description.

Can you please take another look.

There was an ID collision over the `DNS` ID.
With low resolution timers on Windows it's very possible that an event can have an event.duration=0 and we want to send that field when this occurs.
Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM

We normally don't rename the fields in the dashboard as I was hoping to do this with a script all at once. But it should not cause any issue if we have some dashboards already renamed. Good to go.

Thanks for the detailed PR description.

@andrewkroh andrewkroh merged commit 1887aff into elastic:master Jan 10, 2019
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.

2 participants