[Network Traffic] Missing ECS Field Mappings

[MakoWish]

Type of change

• Bug

What does this PR do?

This PR adds missing network-related ECS fields to prevent field mapping conflicts in the logs-* Data View.

Checklist

• I have reviewed tips for building integrations and this pull request is aligned with them.
• I have verified that all data streams collect metrics or logs.
• I have added an entry to my package's changelog.yml file.
• I have incremented the version in my package's manifest.yml file.
• I have verified that Kibana version constraints are current according to guidelines.

Related issues

• Closes [Network Traffic] Missing ECS Field Mappings #7272

Screenshots

[elasticmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2023-08-28T08:42:44.242+0000

• Duration: 68 min 0 sec

Test stats 🧪

Test	Results
Failed	0
Passed	154
Skipped	0
Total	154

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

[elasticmachine]

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

[efd6]

/test

[elasticmachine]

🌐 Coverage report

Name	Metrics % (covered/total)	Diff
Packages	100.0% (1/1)	💚
Files	100.0% (2/2)	💚
Classes	100.0% (2/2)	💚
Methods	98.276% (57/58)	👍 24.943
Lines	88.356% (129/146)	👎 -11.644
Conditionals	100.0% (0/0)	💚

[efd6]

/test

[kcreddy]

/test

[kcreddy]

LGTM

[MakoWish]

I don't have permissions to merge. Can someone do it, please?

[efd6]

Please hold off on merging this subject to internal discussion.

[efd6]

The approach that we should be using here is to turn on import_mappings. This is ~detailed here in the package spec, but there is an issue to improve the documentation for the option here. There are examples of its use in the wild, for example here.

[MakoWish]

Okay, so just so I am clear on what needs to be done, and what this would actually accomplish...

In the package's ./_dev/build/build.yml, we would specify all ECS dependencies, and it will automatically inherit all the sub-fields of that field? For instance, it could look something like this?

dependencies:
  destination:
    reference: [email protected]
    import_mappings: true
  ecs:
    reference: [email protected]
    import_mappings: true
  file:
    reference: [email protected]
    import_mappings: true
  host:
    reference: [email protected]
    import_mappings: true
  observer:
    reference: [email protected]
    import_mappings: true
  source:
    reference: [email protected]
    import_mappings: true

If so, I have been begging for this for quite some time now (see elasticsearch/issues/91370).

[efd6]

The change should be just

diff --git a/packages/network_traffic/_dev/build/build.yml b/packages/network_traffic/_dev/build/build.yml
index c8eeec8ca..c1af686e5 100755
--- a/packages/network_traffic/_dev/build/build.yml
+++ b/packages/network_traffic/_dev/build/build.yml
@@ -1,3 +1,4 @@
 dependencies:
   ecs:
     reference: [email protected]
+    import_mappings: true

I think the problem might be being misunderstood at that issue; the solution here is part of agent, rather than elasticsearch, so that may be the root of the misunderstanding. Also, import_mapping is reasonably new — it looks like it was added as an option about the time of the last comment there.

[MakoWish]

Okay, so simply adding a file ./_dev/build/build.yml with:

dependencies:
  ecs:
    reference: [email protected]
    import_mappings: true

Will enforce all ECS mappings?

Yes, I also think they are misunderstanding the other issue. I have also brought this up with support, and our CSM, but so far getting zero traction from any angle. If the above will enforce all ECS field mappings, that would be the solution I have been waiting for.

The key problem is that whoever creates an Integration may miss some mappings for fields that were not covered in test events, and it also doesn't take into account further ECS parsing the customer may do like we are with host.geo, observer,geo, etc. Since ECS is a schema, it really needs to be enforced to prevent mapping conflicts. I have previously been relying on a Python script to handle some missing ECS mappings, but that's pretty hacky. I am hoping to hear you say this is the solution I have been begging for. <CrossingFingers />

[P1llus]

@MakoWish Just to clear up a few things about this feature, so that everyone is on the same page.

A few releases ago, I created a dynamic template, that matches all the ECS fields available at that date, a dynamic template does not enforce field types, but when a field appears for the first time, and does not have any existing mapping, elasticsearch will try to auto-detect the type.
When you add a dynamic template, you can control the decisions elasticsearch makes, and since keyword is the default type, we only needed to create "rules" for any non-keyword fields: https://www.elastic.co/guide/en/elasticsearch/reference/current/dynamic-templates.html

Here is the template that is used:
https://github.com/elastic/elastic-package/blob/main/internal/builder/_static/ecs_mappings.yaml

Now import_mappings was mostly created as a way for integration developers to opt-in and test it (though it works just fine), and replaces the need to provide a ecs.yml, and helps prevent human errors with missing fields etc.

This was never meant to be the long term fix however, as enhancements to Elasticsearch dynamic templates was added a few versions ago, allowing rules to have arrays of fields, significantly decreasing the size of a template, and the more official ECS dynamic template is now bundled with the upcoming Elasticsearch binary, meaning that a ECS dynamic template component template is installed by default in future releases.

There is still some work to be done to allow integrations to use that new dynamic template component template rather than this local static version, but the move should not create any issues for users.

The more permanent solutions also allows users that do not use integrations, to simply use that new component template in their own datastream index templates.

So this change is good for now, but its important to know the history about it, which is why it was not documented initially as well, in wait for the more permanent scenario.

[MakoWish]

Okay, so are we willing to try this out on a GA integration? I am all for it, but before I remove ecs.yml from all 16 datasets on this integration and utilize import_mappings, just want to make sure we're all in. If preferred, I could try it out on my Arista NG Firewall integration first since its still in Beta.

[andrewkroh]

but before I remove ecs.yml

Until we have a solution for populating the index.query.default_field and the readme docs, perhaps we should leave the static ECS mappings in-place? import_mappings: true will function to help users that have added additional ECS fields through custom pipelines or other means.

[MakoWish]

Was going to push the change, but I see you already did it.