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.

Sign up for GitHub

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

Jump to bottom

[winlogbeat] Default to raw api #42275

Merged
merged 25 commits into from
Jan 20, 2025
Merged

[winlogbeat] Default to raw api #42275

merged 25 commits into from
Jan 20, 2025
+4,516 −3,777

Conversation

marc-gr
Copy link
Contributor

@marc-gr marc-gr commented Jan 9, 2025 •
edited
Loading

Proposed commit message

  • Remove old xml api
  • Defaults to use the new raw api

Since the raw api which is now default renders messages that previously were not rendered, this also:

  • Adds support to the security pipeline to handle both unrendered and rendered messages
  • Regenerates tests golden files

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.

@marc-gr marc-gr added enhancement Winlogbeat backport-skip Skip notification from the automated backport with mergify Team:Security-Windows Platform Windows Platform Team in Security Solution labels Jan 9, 2025
@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 Jan 9, 2025
@mergify mergify bot assigned marc-gr Jan 9, 2025
@marc-gr marc-gr force-pushed the winlogbeat-default-api branch from 57b79e5 to 396fc7d Compare January 9, 2025 11:18
@marc-gr marc-gr marked this pull request as ready for review January 9, 2025 11:19
@marc-gr marc-gr requested a review from a team as a code owner January 9, 2025 11:19
@elasticmachine
Copy link
Collaborator

Pinging @elastic/sec-windows-platform (Team:Security-Windows Platform)

@marc-gr marc-gr requested a review from a team as a code owner January 13, 2025 14:04
@marc-gr marc-gr requested a review from a team as a code owner January 14, 2025 08:33
@marc-gr marc-gr requested review from belimawr and rdner January 14, 2025 08:33
@cla-checker-service CLA Checker Service
Copy link

cla-checker-service bot commented Jan 14, 2025 •
edited
Loading

💚 CLA has been signed

@marc-gr marc-gr force-pushed the winlogbeat-default-api branch from 2efc2a3 to 61cb7a0 Compare January 14, 2025 13:39
@marc-gr marc-gr force-pushed the winlogbeat-default-api branch from 0132c2b to 15fae95 Compare January 15, 2025 11:38
@marc-gr
Copy link
Contributor Author

marc-gr commented Jan 15, 2025

/test

@marc-gr
Copy link
Contributor Author

marc-gr commented Jan 16, 2025

/docs

marc-gr
marc-gr commented Jan 16, 2025
View reviewed changes
winlogbeat/sys/wineventlog/format_message_test.go
// Note: This is not the case under 32-bit Windows 7.
// Disabling the assertion for now.
//assert.NotContains(t, msg, `{{eventParam $ 9}}`)
assert.NotContains(t, msg, `{{eventParam $ 9}}`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is fixed now

intxgo
intxgo approved these changes Jan 16, 2025
View reviewed changes
.buildkite/scripts/install_sysmon.ps1 Outdated Show resolved Hide resolved
winlogbeat/sys/wineventlog/metadata_store_test.go Show resolved Hide resolved
belimawr
belimawr reviewed Jan 16, 2025
View reviewed changes
Copy link
Contributor

@belimawr belimawr left a comment

Choose a reason for hiding this comment

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

I'm only looking at the files owned by the data plane

dev-tools/ecs-migration.yml
Comment on lines -1975 to -1980
- from: type
to: winlog.api
alias: true
beat: winlogbeat
rename: false

Copy link
Contributor

Choose a reason for hiding this comment

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

[Question]

Why is this not needed any more?

Copy link
Contributor Author

@marc-gr marc-gr Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We previously had wineventlog and wineventlog-experimental api's. The experimental API has been recently GA'd as wineventlog-raw in 8.18. For 9.0.0 we want to remove the old wineventlog api as it is less performant and the old way of rendering the events can be still achieved through config while using the new wineventlog-raw api.

So we will have a single api moving forward, that is why we can remove the winlog.api field references.

note that this changes are not going to be backported

@marc-gr marc-gr requested a review from belimawr January 17, 2025 15:07
belimawr
belimawr approved these changes Jan 17, 2025
View reviewed changes
Copy link
Contributor

@belimawr belimawr left a comment

Choose a reason for hiding this comment

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

The files that belong to @elastic/elastic-agent-data-plane LGTM

pazone
pazone reviewed Jan 20, 2025
View reviewed changes
.buildkite/x-pack/pipeline.xpack.winlogbeat.yml Outdated
@@ -65,6 +65,9 @@ steps:
- label: ":windows: x-pack/winlogbeat Win 2019 Unit Tests"
key: "mandatory-win-2019-unit-tests"
command: |
Push-Location -Path .buildkite/scripts
./install_sysmon.ps1
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We can pre-install sysmon to the base Windows image(when the PR is merged) We need to raise an issue for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100%, I was also thinking maybe keeping our own copy since there is no way (that I know of) to download a specific sysmon version.

@marc-gr marc-gr enabled auto-merge (squash) January 20, 2025 12:39
@marc-gr marc-gr merged commit 5ead8ab into elastic:main Jan 20, 2025
190 checks passed
@marc-gr marc-gr deleted the winlogbeat-default-api branch January 20, 2025 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nfritts nfritts nfritts approved these changes

@intxgo intxgo intxgo approved these changes

@pazone pazone pazone approved these changes

@belimawr belimawr belimawr approved these changes

@rdner rdner Awaiting requested review from rdner rdner is a code owner automatically assigned from elastic/elastic-agent-data-plane

Assignees

@marc-gr marc-gr

Labels
backport-skip Skip notification from the automated backport with mergify enhancement Team:Security-Windows Platform Windows Platform Team in Security Solution Winlogbeat
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@marc-gr @elasticmachine @belimawr @pazone @intxgo @nfritts