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

azure: add related.entity field to activitylogs default ingest pipeline #11233

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

orouz
Copy link
Contributor

@orouz orouz commented Sep 24, 2024

Proposed commit message

this PR is part of the cloud security CDR epic. it adds a painless script processor that appends both principal_id (event/action origin) and resource_id (event/action target) to a new field - related.entity

the related.entity field is an upcoming ECS field meant to facilitate pivoting around a piece of data.

Related issues


benchmark (30 runs)

Metric      
Description Main append processor painless processor
Commit 00ed51b 21b7189 bd6cde2
Average EPS 10333.47787 9546.633648 10726.24097
Stddev 1549.043495 1967.80285 1281.125397
Min 3460.207612 3759.398496 6211.180124
Max 11764.70588 11627.90698 11627.90698
EPS Change to baseline   -7.61% 3.80% (?)

@orouz orouz added enhancement New feature or request Integration:azure Azure Logs labels Sep 24, 2024
@orouz orouz force-pushed the azure_activitylogs_cdr_pipeline branch from 47f2eea to 21b7189 Compare September 24, 2024 12:33
@orouz
Copy link
Contributor Author

orouz commented Sep 25, 2024

/test

@orouz orouz force-pushed the azure_activitylogs_cdr_pipeline branch from dd66af9 to f34cd4a Compare September 26, 2024 15:17
@orouz orouz force-pushed the azure_activitylogs_cdr_pipeline branch from f34cd4a to da652ec Compare September 26, 2024 15:18
@elastic-vault-github-plugin-prod

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@orouz orouz marked this pull request as ready for review September 26, 2024 15:42
@orouz orouz requested review from a team as code owners September 26, 2024 15:42
@andrewkroh andrewkroh added the Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team [elastic/obs-infraobs-integrations] label Sep 27, 2024
@@ -1,3 +1,8 @@
- version: "1.17.1-preview01"
Copy link
Member

Choose a reason for hiding this comment

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

What's the plan for when this change is safe to be included in a non pre-release version? Is this a pre-release because of elastic/ecs#2360 not being finalized?

Based on semver.org rules, as an enhancement, I would expect the versioning to be based on 1.18 and not a patch version.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've commented on this kind of thing before, but now having experienced the impact of it first hand, I'll repeat that the use of preview commits with an undefined TTL based on external constraints and that cannot become part of a new release without potentially breaking users as a bad idea from an ongoing maintenance perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

initially the plan was to release a preview version so we can test it before it hits users, but i see now how it affects other development teams so i've updated it to 1.18.0 as suggested and did the testing using elastic package install

@andrewkroh andrewkroh requested a review from a team October 7, 2024 13:54
Comment on lines 320 to 324
on_failure:
- set:
description: Add error reason
field: error.message
value: "{{{ _ingest.on_failure_message }}}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks incomplete. Also conventionally, on_failure handlers are placed last.

Comment on lines 316 to 332
- script:
description: Appends principal_id and resource_id to `related.entity`
lang: painless
ignore_failure: true
on_failure:
- set:
description: Add error reason
field: error.message
value: "{{{ _ingest.on_failure_message }}}"
source: |
ctx.related = ctx.related ?: [:];
ctx.related.entity = new HashSet();
ctx.related.entity.add(field("azure.activitylogs.identity.authorization.evidence.principal_id").get(null));
ctx.related.entity.add(field("azure.resource_id").get(null));
ctx.related.entity.remove("");
ctx.related.entity.remove(null);

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest rather the simpler

Suggested change
- script:
description: Appends principal_id and resource_id to `related.entity`
lang: painless
ignore_failure: true
on_failure:
- set:
description: Add error reason
field: error.message
value: "{{{ _ingest.on_failure_message }}}"
source: |
ctx.related = ctx.related ?: [:];
ctx.related.entity = new HashSet();
ctx.related.entity.add(field("azure.activitylogs.identity.authorization.evidence.principal_id").get(null));
ctx.related.entity.add(field("azure.resource_id").get(null));
ctx.related.entity.remove("");
ctx.related.entity.remove(null);
- append:
field: related.entity
value: '{{{azure.resource_id}}}'
allow_duplicates: false
if: >
ctx.azure?.resource_id != null && ctx.azure.resource_id != ''
- append:
field: related.entity
value: '{{{azure.activitylogs.identity.authorization.evidence.principal_id}}}'
allow_duplicates: false
if: >
ctx.azure?.activitylogs?.identity?.authorization?.evidence?.principal_id != null &&
ctx.azure.activitylogs.identity.authorization.evidence.principal_id != ''

which is shorter, does not require an error handler, and does not allocate in the case that the fields are absent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i did this, but currently if one of these append processors fails, the pipeline won't continue, which is something i'd like to avoid. i think adding an empty on_failure field to the processors should cover that, but i didn't manage to test this with invalid values. wdyt? how could i ensure a failure won't stop the pipeline from executing the next steps, but still log the error? (ignore_failure is also not preferable)

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the failure mode that could lead to an append failing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tbh i don't know. i don't see how it can fail now, but as a general good measure i assumed it'd be better to account for it in anyway. but if you're ok with it then i guess it's fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm OK with it.

contains multiple entities, identifiers belonging to different entities
will be present. Example identifiers include cloud resource IDs, ARNs,
email addresses, or hostnames.
type: keyword
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add final new line.

Copy link

Quality Gate failed Quality Gate failed

Failed conditions
47.6% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube

@elasticmachine
Copy link

💚 Build Succeeded

History

@efd6 efd6 changed the title Add related.entity field to azure activitylogs default ingest pipeline azure: add related.entity field to activitylogs default ingest pipeline Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Integration:azure Azure Logs Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team [elastic/obs-infraobs-integrations]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants