Skip to content

Add non-indexed fields to ecs templates#106714

Merged
flash1293 merged 8 commits intoelastic:mainfrom
flash1293:flash1293/fix-ecs-templates
Apr 3, 2024
Merged

Add non-indexed fields to ecs templates#106714
flash1293 merged 8 commits intoelastic:mainfrom
flash1293:flash1293/fix-ecs-templates

Conversation

@flash1293
Copy link
Contributor

@flash1293 flash1293 commented Mar 25, 2024

Some fields in ECS are specified to not get indexed. The existing default component template ecs@mappings isn't capturing this, so the fields will fall back to regular keyword mapping:
https://www.elastic.co/guide/en/ecs/current/ecs-event.html#field-event-original

Aligned with elastic/elastic-package#1733 - I'm not sure about the x509 fields, could you elaborate on these @jsoriano ?

This PR fixes the problem by adding dynamic templates for these fields.

@elasticsearchmachine elasticsearchmachine added v8.14.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Mar 25, 2024
@flash1293 flash1293 requested a review from ruflin March 25, 2024 11:38
@ruflin
Copy link
Contributor

ruflin commented Mar 25, 2024

@eyalkoren Would be great to get your look at this one if you find some time. I wonder why this did not pop up in the tests and how we can adjust the tests to cover this use case.

ruflin
ruflin reviewed Mar 25, 2024
View reviewed changes
x-pack/plugin/core/template-resources/src/main/resources/ecs@mappings.json Outdated
{
"ecs_x509_public_key_exponent_non_indexed_keyword": {
"mapping": {
"type": "keyword",
Copy link
Contributor

@ruflin ruflin Mar 25, 2024

Choose a reason for hiding this comment

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

it seems this is a long according to ECS? https://www.elastic.co/guide/en/ecs/8.11/ecs-x509.html#field-x509-public-key-exponent I really wonder if we should cover this "edge" case.

Copy link
Contributor Author

@flash1293 flash1293 Mar 25, 2024

Choose a reason for hiding this comment

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

I think I agree. ECS doesn't mention whether it's indexed or not - so I guess indexing is fair game (and it's left to the user to overwrite this)?

@jsoriano
Copy link
Member

jsoriano commented Mar 25, 2024

I'm not sure about the x509 fields, could you elaborate on these @jsoriano ?

They are defined as non-indexed in ECS: https://github.com/elastic/ecs/blob/ee4e0979dbaa91c915a1b31a26e1ea814bfe75aa/schemas/x509.yml#L203

@eyalkoren
Copy link
Contributor

eyalkoren commented Mar 26, 2024

@ruflin the tests currently ignore the index property of the ECS field definitions. Since they are defined in the schema, it should be easy to adjust them. I'll try to get to that this week, and bring into consideration the doc_values property as well.

@flash1293
Copy link
Contributor Author

flash1293 commented Mar 26, 2024

Thanks for the context @jsoriano - it threw me off that event.original is specifying this explicitly in the user-visible text in the docs.

@jsoriano jsoriano mentioned this pull request Mar 26, 2024
Merged
@flash1293
Copy link
Contributor Author

flash1293 commented Mar 26, 2024

/ci

@flash1293
Copy link
Contributor Author

flash1293 commented Mar 26, 2024

@elasticsearchmachine run

@flash1293
Copy link
Contributor Author

flash1293 commented Mar 26, 2024

@elasticmachine run

@ruflin
Copy link
Contributor

ruflin commented Mar 27, 2024 •
edited
Loading

  • @eyalkoren That would be great. Should we wait with this PR for the change as we might unconver other things?
  • @eyalkoren @dakrone Do we need to increase the version of the template or anything if we update it?
  • @flash1293 Try to merge in main, the failures seem to be unrelated

@flash1293 flash1293 marked this pull request as ready for review March 27, 2024 10:49
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Mar 27, 2024
@flash1293 flash1293 added :StorageEngine/Data streams Data streams and their lifecycles Team:Data Management (obsolete) DO NOT USE. This team no longer exists. >bug labels Mar 27, 2024
@elasticsearchmachine elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Mar 27, 2024
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Mar 27, 2024

Pinging @elastic/es-data-management (Team:Data Management)

@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Mar 27, 2024

Hi @flash1293, I've created a changelog YAML for you.

@ruflin ruflin requested a review from dakrone March 27, 2024 10:56
@ruflin
Copy link
Contributor

ruflin commented Mar 27, 2024

As usual, I pinged the wrong @leehinman , now added @dakrone .

@eyalkoren
Copy link
Contributor

eyalkoren commented Mar 29, 2024

@eyalkoren That would be great. Should we wait with this PR for the change as we might unconver other things?

I don't think we need to wait with the test. My intention was to do it in a separate PR anyway to see it failing and then verify it gets fixed after merging this PR's fix. And I am not just saying that because my availability is non problematic 😬

@eyalkoren @dakrone Do we need to increase the version of the template or anything if we update it?

Since this template uses a version variable which is used by all other components managed by the stack registry, I believe we would need to update the registry version, unless it was already updated after the latest release.

@eyalkoren eyalkoren removed their request for review March 31, 2024 11:32
@eyalkoren
Copy link
Contributor

eyalkoren commented Mar 31, 2024

I ended up pushing the ECS test adjustment here and verified that it fails when removing the ecs@mappings fixes.
Since I also contributed, I hereby approve the mappings fix but will let other reviewers to "officially" approve the entire PR changes.

@felixbarny felixbarny removed the request for review from leehinman April 2, 2024 08:34
felixbarny
felixbarny reviewed Apr 2, 2024
View reviewed changes
Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

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

Please increment org.elasticsearch.xpack.stack.StackTemplateRegistry#REGISTRY_VERSION, otherwise LGTM

ruflin
ruflin reviewed Apr 2, 2024
View reviewed changes
dakrone
dakrone approved these changes Apr 3, 2024
View reviewed changes
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM

@flash1293 flash1293 merged commit 6e36089 into elastic:main Apr 3, 2024
@flash1293 flash1293 mentioned this pull request Apr 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsoriano jsoriano jsoriano approved these changes

@dakrone dakrone dakrone approved these changes

@ruflin ruflin ruflin approved these changes

@felixbarny felixbarny felixbarny approved these changes

Assignees

No one assigned

Labels

>bug external-contributor Pull request authored by a developer outside the Elasticsearch team :StorageEngine/Data streams Data streams and their lifecycles Team:Data Management (obsolete) DO NOT USE. This team no longer exists. v8.14.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@flash1293 @ruflin @jsoriano @eyalkoren @elasticsearchmachine @dakrone @felixbarny