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

Add basic wildcard/keyword fallback for upcoming ECS releases #22521

Merged
merged 5 commits into from
Nov 12, 2020

Conversation

andrewstucki
Copy link

@andrewstucki andrewstucki commented Nov 10, 2020

What does this PR do?

This sets the groundwork for #21674 by adding a simple fallback mechanism for wildcard fields in unsupported Elasticsearch versions and non-OSS Beats distros. If you're publishing to ES < 7.9.0 you get keyword fields instead of wildcard, if you're coming from OSS Beats you get the same.

This will allow us to move forward with the migration to ECS 1.7.0 with upcoming wildcard support..

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

@elasticmachine
Copy link
Collaborator

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

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Nov 10, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented Nov 10, 2020

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #22521 updated]

  • Start Time: 2020-11-12T12:33:30.169+0000

  • Duration: 74 min 53 sec

Test stats 🧪

Test Results
Failed 0
Passed 16618
Skipped 1349
Total 17967

Steps errors 1

Expand to view the steps failures

Install Go/Mage/Python/Docker/Terraform 1.14.7

  • Took 0 min 7 sec . View more details on here
  • Description: .ci/scripts/install-tools.sh

@elasticmachine
Copy link
Collaborator

elasticmachine commented Nov 10, 2020

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 16618
Skipped 1349
Total 17967

@webmat
Copy link
Contributor

webmat commented Nov 10, 2020

This is looking good for now. The next version of ECS will only introduce the wildcard basic type.

I'm not familiar enough about the codebase, but it's important that this fallback happens for any field using basic types, not just ECS fields. I don't have insight into what custom fields are in Beats or will be added, but if any of them is basic or changes to a basic type, they will need to fall back to an OSS type as well, where possible.

As a corollary to this previous point, perhaps you should consider falling back constant_keyword and version to keyword as well?

  • For reasons I won't go into, ECS will likely not introduce constant_keyword to the Beats field definitions. But if each Beat decided to migrate fields that remain constant in the current indexing strategy like agent.type to constant_keyword, a fallback would be necessary.
  • At this time version looks like a conflict with keyword, so it's unclear if any pre-existing fields will migrate to it during 7.x. But this data type is fair game for any net new field in ECS or custom fields. So being ready to perform this fallback sounds like a good idea.

So to recap, as far as the next version of ECS is concerned, only wildcard => keyword is necessary, and this seems to do it 👍 But make sure the fallback happens on all fields, not just ECS ones; and make sure to confirm whether any more fallbacks are necessary based on Beats field definitions that aren't driven by ECS.

@andrewstucki
Copy link
Author

@webmat my understanding of this code is that the Processor gets called on the final merged beats fields that are going into the Elasticsearch index template, meaning it should include both any fields specified at the module level, as well as the global ECS fields yml file that we include in libbeat/_meta/fields.ecs.yml. Just to keep the scope of the change smaller for now, I'm thinking it probably makes sense to add the constant_keyword and version fallbacks as they crop up, but if you guys really want me to, I can add it here too.

@webmat
Copy link
Contributor

webmat commented Nov 10, 2020

Not required per ECS, so feel free to move forward as is.

But as I said, I don't have insight into all of the other Beats fields. Trying each OSS Beats' ...beat setup command on an OSS cluster might be a quick way find out if there's other basic types that have been added.

@@ -21,6 +22,10 @@ import (
_ "github.com/elastic/beats/v7/x-pack/libbeat/autodiscover/providers/aws/elb"
)

func init() {
version.ElasticLicensed = true
Copy link
Contributor

Choose a reason for hiding this comment

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

This is setting a global, that we are trying to avoid cc @urso

Perhaps we should come up with a way to passing this to the beat initialization?

Copy link

Choose a reason for hiding this comment

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

Yeah, +1 on not introducing globals. Beats query the ES version and Basic queries the license on startup.

We pass the agent name and version via beat.Info. Maybe this would be the right place to put this information.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not a fan of globals either, and made the change, but due to the way that the root cobra commands are initialized in each beat and the way the info propagates from the command to the processor, the changes are quite a bit more invasive. So, I'm going to keep this conversation unresolved for some context in case we want to revert back to the global.

@andrewstucki andrewstucki requested review from a team as code owners November 11, 2020 14:50
@andrewstucki andrewstucki force-pushed the wildcard-fallback branch 3 times, most recently from 7aec4aa to c734cb9 Compare November 11, 2020 16:22
libbeat/beat/info.go Outdated Show resolved Hide resolved
Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

We are almost there, I left a minor comment on naming, apart from that this LGTM! Thank you for working on it

@simitt
Copy link
Contributor

simitt commented Nov 12, 2020

cc @elastic/apm-server when pulling this change in with a beats update we have to ensure to set the setting.ElasticLicensed accordingly.

@andrewstucki andrewstucki merged commit 0fda306 into elastic:master Nov 12, 2020
andrewstucki pushed a commit to andrewstucki/beats that referenced this pull request Nov 12, 2020
…c#22521)

* Add basic wildcard/keyword fallback for upcoming ECS releases

* update changelog

* Invert global boolean

* Remove global

* Rename XPack to ElasticLicensed

(cherry picked from commit 0fda306)
simitt added a commit to simitt/apm-server that referenced this pull request Nov 12, 2020
andrewstucki pushed a commit that referenced this pull request Nov 12, 2020
#22568)

* Add basic wildcard/keyword fallback for upcoming ECS releases

* update changelog

* Invert global boolean

* Remove global

* Rename XPack to ElasticLicensed

(cherry picked from commit 0fda306)
simitt added a commit to elastic/apm-server that referenced this pull request Nov 16, 2020
* Update to elastic/beats@0fda3061815d
Pulling in changes from elastic/beats#22521

* Set ElasticLicensed for enabling wildcard support.
bmorelli25 pushed a commit to bmorelli25/observability-docs that referenced this pull request Dec 18, 2023
* Update to elastic/beats@0fda3061815d
Pulling in changes from elastic/beats#22521

* Set ElasticLicensed for enabling wildcard support.
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.

7 participants