[teleport] Update event-groups ingest pipeline to manage cloud fields if already present #12851

mrodm · 2025-02-20T11:22:01Z

Proposed commit message

Update event-groups ingest pipeline to manage cloud fields (like cloud.region, cloud.istance.id...) if they were already set by filebeat or other processors.

These errors were detected while testing the validation based on mappings: https://buildkite.com/elastic/integrations/builds/22313

But sometimes it was not detected, and it was due to elastic-package was not waiting to get all the docs defined in the test folder to be ingested. After being ingested a few documents, elastic-package started the validation of those docs found.

This PR has two goals:

adding the required configuration to change the behavior of the system tests to wait for the 270 documents defined in _dev/deploy/docker/sample_logs/.
~~adding the override setting into the rename processors to avoid throwing exceptions.~~
- IMPORTANT: The corresponding cloud fields will be set with the value of the last rename processor.
adding the if setting into the rename processors to avoid throwing exceptions if those fields were already present in the document.
- IMPORTANT: The corresponding cloud fields will keep the original values (if they exist), or the value from the first rename processor applicable.
- Related discussion: [teleport] Update event-groups ingest pipeline to manage cloud fields if already present #12851 (review)

Errors found when elastic-package waits for all 270 documents (buildkite link - pr link):

[0] found error.message in event: [Processor rename with tag  in pipeline logs-teleport.audit-1.2.1-event-groups failed with message: field [cloud.instance.id] already exists]
[1] found error.message in event: [Processor rename with tag  in pipeline logs-teleport.audit-1.2.1-event-groups failed with message: field [cloud.region] already exists]

In another test running just validation based on mappings, these were the fields undefined (buildkite link):

[0] field "teleport.audit.account_id" is undefined: field definition not found
[1] field "teleport.audit.aws_host" is undefined: field definition not found
[2] field "teleport.audit.aws_region" is undefined: field definition not found
[3] field "teleport.audit.aws_service" is undefined: field definition not found
[4] field "teleport.audit.exit_code" is undefined: field definition not found
[5] field "teleport.audit.instance_id" is undefined: field definition not found
[6] field "teleport.audit.region" is undefined: field definition not found
[7] field "teleport.audit.status" is undefined: field definition not found
[8] field "teleport.audit.target" is undefined: field definition not found

Related discussions from previous PR:

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 verified that Kibana version constraints are current according to guidelines.
I have verified that any added dashboard complies with Kibana's Dashboard good practices

Author's Checklist

Test with elastic-package enabled mappings https://buildkite.com/elastic/integrations/builds/22587
Remove debug changes .buildkite folder and go.mod/go.sum files.

How to test this PR locally

Related issues

Follows Update security service integrations packages mappings #12624

elastic-vault-github-plugin-prod · 2025-02-20T11:39:26Z

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

mrodm · 2025-02-20T17:07:44Z

packages/teleport/data_stream/audit/elasticsearch/ingest_pipeline/event-groups.yml

+      # This could fail if `cloud.account.id` field already exists in the doc. Forced to override the value to avoid throw an exception.
+      override: true


cloud.instance.id could be set in three different processors using different source fields:

teleport.audit.aws_host

teleport.audit.db_gcp_instance_id

teleport.audit.instance_id

mrodm · 2025-02-20T17:08:07Z

packages/teleport/data_stream/audit/elasticsearch/ingest_pipeline/event-groups.yml

  - rename:
      field: teleport.audit.region
      target_field: cloud.region
      ignore_missing: true
-      ignore_failure: true
+      # This could fail if `cloud.region` field already exists in the doc. Forced to override the value to avoid throw an exception.
+      override: true


cloud.region could be set in three different processors using different source fields:

teleport.audit.aws_region

teleport.audit.db_aws_region

teleport.audit.region

mrodm · 2025-02-20T17:08:29Z

packages/teleport/data_stream/audit/elasticsearch/ingest_pipeline/event-groups.yml

@@ -1426,11 +1438,14 @@ processors:
      field: teleport.audit.account_id
      target_field: cloud.account.id
      ignore_missing: true
+      # This could fail if `cloud.account.id` field already exists in the doc.  Forced to override the value to avoid throw an exception.
+      override: true


There is no other processor updating cloud.account.id.

mrodm · 2025-02-20T17:08:54Z

packages/teleport/data_stream/audit/elasticsearch/ingest_pipeline/event-groups.yml

  - rename:
      field: teleport.audit.aws_service
      target_field: cloud.service.name
      ignore_missing: true
+      # This could fail if `cloud.service.name` field already exists in the doc. Forced to override the value to avoid throw an exception.
+      override: true


No other processor updating cloud.service.name.

mrodm · 2025-02-20T17:17:09Z

packages/teleport/data_stream/audit/elasticsearch/ingest_pipeline/event-groups.yml

  - rename:
      field: teleport.audit.aws_host
      target_field: cloud.instance.id
      ignore_missing: true
+      # This could fail if `cloud.instance.id` field already exists in the doc. Forced to override the value to avoid throw an exception.
+      override: true


Adding override: true into these rename processors makes that the corresponding cloud field values will be set with the value of the latest rename processor executed.

Is that ok/expected?

Direct link to the discussion in the original PR: #12624 (comment).

If we enabled an approach like the solution that was implemented for the qualys case (user can specify what they are interested in), or a hard "use the data from the data source" approach, then those two fields cease to be an issue. The remaining fields, cloud.region and cloud.instance.id look like they would then just fall out (depending on how teleport handles the teleport.audit.{region,instance_id} cases (are they disjoint with the explicitly named fields, teleport.audit.{aws,db_aws} and teleport.audit.{aws_host,db_gcp_instance_id} respectively. Unfortunately the test cases don't give any indication about this and I cannot find any reference for it in the teleport documentation; my feeling is that the more generic case should be overridden by the more specific case, but if an override occurs, then an error should be written in and the pipeline be continued. In the case that the we see apparently inconsistent fields being set (e.g. teleport.audit.aws_host, teleport.audit.db_gcp_instance_id) this would just be an error. I'm not sure what the behaviour of teleport.audit.aws_region v. teleport.audit.db_aws_region should be; are these mutually exclusive?

So, I don't have neither the context about teleport to answer that question nor to provide an alternative solution for the errors found in this ingest pipeline 😞

According to what you're suggesting (or at least I understand about it), do you mean to add an on_failure action in each rename processor modified here?
However, how could it be distinguished that the value to be override is the one set by filebeat or the one set by a previous processor?

Could I get some help here to move forward? 🙏

Another option, it could be done the opposite, add ignore_failure: true instead of overwrite. IIUC that would keep the values set by filebeat instead, losing the values from teleport data.

The answer to the question is unfortunately a question. The issue is one of whether we know what the user wants from the data set. In the case of Qualys it was fairly clear that at least some users wanted the data source's values for these fields rather than the values that are added by the agent processor. ISTM that this is likely to be the case here too, but I don't know this for a fact.

mrodm · 2025-02-20T17:26:20Z

go.mod

 	k8s.io/utils v0.0.0-20241104100929-3ea5e8cea738 // indirect
 	sigs.k8s.io/json v0.0.0-20241010143419-9aa6b5e7a4b3 // indirect
 	sigs.k8s.io/kustomize/api v0.18.0 // indirect
 	sigs.k8s.io/kustomize/kyaml v0.18.1 // indirect
 	sigs.k8s.io/structured-merge-diff/v4 v4.4.2 // indirect
 	sigs.k8s.io/yaml v1.4.0 // indirect
 )
+
+replace github.com/elastic/elastic-package => github.com/elastic/elastic-package v0.109.2-0.20250220090015-bbd8bcadb505


To be removed before merging.
The same for the changes in files under .buildkite folder.

These are required to test with the validation based on mappings.

elasticmachine · 2025-02-21T10:57:32Z

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

andrewkroh

Today, we know that no overrides are happening because that would result in an error.

Instead of adding override: true, which could change the behavior of cloud fields, how about adding a guard that checks if the target field exists to prevent the error (e.g., if: ctx.cloud?.x == null)?

I think the ideal solution is the one that was discussed, where the user gets to make the choice (i.e., what was added to Qualys). If we can't get this solution in time to unblock @mrodm's work on enhancing elastic-package, then I propose we replace override: true with an if, and then open an issue for the ideal solution.

side-note: We need to audit every system test that uses canned logs to find the ones that are missing assert.hit_count.

mrodm · 2025-02-27T10:38:38Z

Today, we know that no overrides are happening because that would result in an error.

Instead of adding override: true, which could change the behavior of cloud fields, how about adding a guard that checks if the target field exists to prevent the error (e.g., if: ctx.cloud?.x == null)?

Ok, I'll replace those overrides by if conditions, thanks!

As a note, there would be some kind of change of behavior due to the fact that until now that pipeline was interrupted with an error if there ware cloud.* fields defined.

mrodm · 2025-02-27T10:55:34Z

I think the ideal solution is the one that was discussed, where the user gets to make the choice (i.e., what was added to Qualys). If we can't get this solution in time to unblock @mrodm's work on enhancing elastic-package, then I propose we replace override: true with an if, and then open an issue for the ideal solution.

Applied changes to use if in 3e84dbd

Created issue #12918
Feel free to update the description/title if you think it is missing any information.

mrodm · 2025-02-27T11:35:33Z

packages/teleport/data_stream/audit/elasticsearch/ingest_pipeline/event-groups.yml

+      if: ctx.cloud?.region == null
+  - remove:
+      field: teleport.audit.region
+      ignore_missing: true


When using if conditions, as the source fields is not renamed, it must be added a new remove processor for each one.

Could be in a single remove processor block, but I think this is less prone to future damage.

packages/teleport/changelog.yml

Co-authored-by: Dan Kortschak <[email protected]>

elasticmachine · 2025-02-28T09:35:49Z

💚 Build Succeeded

Buildkite Build
Commit: 1784669

History

💚 Build #22820 succeeded 7eea2ed
💔 Build #22817 failed 8911f47
💚 Build #22816 succeeded 53425f1
💔 Build #22811 failed 3e84dbd
💚 Build #22600 succeeded ebe8db9
💔 Build #22599 failed 8b14c29

cc @mrodm

elastic-sonarqube · 2025-02-28T09:35:55Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

mrodm · 2025-03-03T09:47:25Z

@efd6 @andrewkroh could I get another review for this PR? 😊

Thanks in advance!!

efd6

Thanks

efd6 · 2025-03-04T09:16:13Z

packages/teleport/data_stream/audit/elasticsearch/ingest_pipeline/event-groups.yml

+      if: ctx.cloud?.region == null
+  - remove:
+      field: teleport.audit.region
+      ignore_missing: true


Could be in a single remove processor block, but I think this is less prone to future damage.

elastic-vault-github-plugin-prod · 2025-03-04T09:58:38Z

Package teleport - 1.2.2 containing this change is available at https://epr.elastic.co/package/teleport/1.2.2/

mrodm added 2 commits February 20, 2025 12:08

Update event-groups ingest pipeline - teleport

68c69fd

Teleport - Ensure system tests wait to be ingestd all test docs/logs

356b31c

mrodm self-assigned this Feb 20, 2025

Upate version and add changelog entry

69b1050

mrodm force-pushed the update_teleport_pipeline branch from 1449981 to 69b1050 Compare February 20, 2025 11:22

mrodm added a commit to mrodm/integrations that referenced this pull request Feb 20, 2025

Remove changes from teleport - moved to elastic#12851

9283cd3

mrodm mentioned this pull request Feb 20, 2025

Update security service integrations packages mappings #12624

Merged

10 tasks

mrodm added 4 commits February 20, 2025 12:55

Test with elastic-package - main bbd8bcadb505

be48391

Update pipeline

7989320

Test failure

8b14c29

Update comments for override parameters added

ebe8db9

mrodm commented Feb 20, 2025

View reviewed changes

mrodm marked this pull request as ready for review February 20, 2025 17:55

mrodm requested review from a team as code owners February 20, 2025 17:55

mrodm added Integration:teleport Teleport Team:Security-Service Integrations Security Service Integrations Team [elastic/security-service-integrations] labels Feb 21, 2025

andrewkroh reviewed Feb 27, 2025

View reviewed changes

Replace overrides by if conditions

3e84dbd

mrodm mentioned this pull request Feb 27, 2025

[teleport] Allow the user to decide which values should be set in cloud.* fields #12918

Open

Add remove processors

53425f1

mrodm commented Feb 27, 2025

View reviewed changes

mrodm added 2 commits February 27, 2025 12:36

Remove debug changes to test mapping-based validation

8911f47

Restore same deps as in upstream/main

7eea2ed

mrodm requested review from efd6 and andrewkroh February 27, 2025 14:03

efd6 reviewed Feb 27, 2025

View reviewed changes

packages/teleport/changelog.yml Outdated Show resolved Hide resolved

Update changelog description

1784669

Co-authored-by: Dan Kortschak <[email protected]>

mrodm requested a review from efd6 February 28, 2025 14:18

mrodm mentioned this pull request Mar 3, 2025

Bump github.com/elastic/elastic-package from 0.109.1 to 0.110.0 #12939

Closed

efd6 approved these changes Mar 4, 2025

View reviewed changes

mrodm merged commit 7439dfd into elastic:main Mar 4, 2025
7 checks passed

mrodm deleted the update_teleport_pipeline branch March 4, 2025 09:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[teleport] Update event-groups ingest pipeline to manage cloud fields if already present #12851

[teleport] Update event-groups ingest pipeline to manage cloud fields if already present #12851

mrodm commented Feb 20, 2025 •

edited

Loading

elastic-vault-github-plugin-prod bot commented Feb 20, 2025 •

edited

Loading

mrodm Feb 20, 2025

mrodm Feb 20, 2025

mrodm Feb 20, 2025

mrodm Feb 20, 2025

mrodm Feb 20, 2025

efd6 Feb 20, 2025

mrodm Feb 21, 2025 •

edited

Loading

mrodm Feb 21, 2025

efd6 Feb 27, 2025

mrodm Feb 20, 2025

elasticmachine commented Feb 21, 2025

andrewkroh left a comment •

edited

Loading

mrodm commented Feb 27, 2025

mrodm commented Feb 27, 2025

mrodm Feb 27, 2025

efd6 Mar 4, 2025

elasticmachine commented Feb 28, 2025

elastic-sonarqube bot commented Feb 28, 2025

mrodm commented Mar 3, 2025

efd6 left a comment

efd6 Mar 4, 2025

elastic-vault-github-plugin-prod bot commented Mar 4, 2025

		# This could fail if `cloud.account.id` field already exists in the doc. Forced to override the value to avoid throw an exception.
		override: true

[teleport] Update event-groups ingest pipeline to manage cloud fields if already present #12851

[teleport] Update event-groups ingest pipeline to manage cloud fields if already present #12851

Conversation

mrodm commented Feb 20, 2025 • edited Loading

Proposed commit message

Checklist

Author's Checklist

How to test this PR locally

Related issues

elastic-vault-github-plugin-prod bot commented Feb 20, 2025 • edited Loading

🚀 Benchmarks report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrodm Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Feb 21, 2025

andrewkroh left a comment • edited Loading

Choose a reason for hiding this comment

mrodm commented Feb 27, 2025

mrodm commented Feb 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Feb 28, 2025

💚 Build Succeeded

History

elastic-sonarqube bot commented Feb 28, 2025

Quality Gate passed

mrodm commented Mar 3, 2025

efd6 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elastic-vault-github-plugin-prod bot commented Mar 4, 2025

mrodm commented Feb 20, 2025 •

edited

Loading

elastic-vault-github-plugin-prod bot commented Feb 20, 2025 •

edited

Loading

mrodm Feb 21, 2025 •

edited

Loading

andrewkroh left a comment •

edited

Loading