Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update security service integrations packages mappings #12624
Update security service integrations packages mappings #12624
Changes from 15 commits
01ba124001e81ea72f5b469d5f8aee10501612ce1f1420345ef021f2d048cc20abc3c40bcb54cf74e314698dbe91469158a9736cc2415ff275c3cec8d215d54d513db6325c32ca890672b1841118bc8ed58077e4879aa33849a22ee1ec321ae205793fa66aad18eaba67bd2ba7c3e7ed3ed832d118d694695555ec5f2640689eb49c96630c994e144a21024c9283cd366de372c481836File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a new field definition is added to a transform, should this destination index be updated (increment suffix number)? Or keep that destination index without changes?
Same doubt for the other packages updating field definitions in transforms (tychon and wiz).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @kcreddy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We increment the index to avoid any conflicts due to mapping changes.
I also simulated an integration upgrade from
keywordtomatch_only_texton same index. After the upgrade, the type remained to bekeywordand didn't change tomatch_only_text.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I guess this would mean that the destination index must be updated, am I right ? @kcreddy
In the other transforms (from other packages), there are changes related to field definitions with changes in mappings like:
Should we update also the destination index there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrodm, yes the destination index version has to be updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've bumped the version defined in the
fleet_transform_versionsetting for all the transforms that I've modified here @kcreddyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two fields are mapped as keywords but they should
date.Using external
ecsdefinition to update their mappings.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fields are safe; obtained from
dateprocessor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that this Ingest pipeline fails in some steps, but this was not detected since the trigger in the default pipeline was ignoring the failures:
integrations/packages/teleport/data_stream/audit/elasticsearch/ingest_pipeline/default.yml
Lines 55 to 58 in cfeff96
These errors can be checked if
ignore_failureis set tofalseas:Those failures cause that some fields are not removed or renamed, and therefore the mappings created in the data stream are not the expected ones. These are the errors found while testing with the sample logs in the package:
Could you check if there are other better options in the pipeline to solve these errors? In the case of the rename processor, if the name already exists, it throws an error. Here, I just set override as true to avoid that error, but I miss the context to know what it is the best approach.
This could happen in other steps for other fields too (depending on the documents ingested). Just some examples (there could be more):
teleport.audit.mfa_device.uuidis tried to be updated withrenameprocessor several times in that pipeline.teleport.audit.kubernetes.labelsteleport.audit.database.query_parametersMaybe there are other cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As tested in this PR #12801
Failures in this package are independent of the validation used.
In that PR, it is ensured that the system tests wait for all the test documents (270 in total) before running the validation, and it fails too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this is the correct approach. It looks to me like we want the teleport data rather than the agent data.
/cc @andrewkroh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested to wait for all 270 documents defined in the test folder, and it also fails with the current validation used in elastic-package (no mapping checks involved).
#12801
Locally, this does not fail for me, I guess it is related to VM agents created/used in CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The concern with the changes that I set in this PR is that for instance
cloud.instance.idcould be overwritten 3 times (depending on the values).As this is also can be reproduced with the current validation performed by elastic-package, should it be created a separated issue for it so you can take a look at it? In this PR, it is shown how to reproduce it in the CI #12801
The problem is that the errors in this package could be blocking the next release elastic-package version (when merging it into the integrations repo).
cc @efd6 @jsoriano
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were related semantic concerns in #10277 where the issue is that the user wanted cloud provider identities being stored in these fields rather than the elastic agent identity.
Summary of behaviour:
cloud.region: explicitly fromteleport.audit.aws_region,teleport.audit.db_aws_regionorteleport.audit.regioncloud.service.name: explicitly fromteleport.audit.aws_servicecloud.instance.id: explicitly fromteleport.audit.aws_host,teleport.audit.db_gcp_instance_idorteleport.audit.instance_idcloud.account.id: explicitly fromteleport.audit.account_idThe filebeat
add_cloud_metadatais always run, and it implicitly addscloud.{account.id,availability_zone,instance.id,machine.type,image.id,provider,region,service.name}explaining whycloud.service.nameandcloud.account.idwhich only have single explicit sets still have this error path behaviour.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.regionandcloud.instance.idlook like they would then just fall out (depending on how teleport handles theteleport.audit.{region,instance_id}cases (are they disjoint with the explicitly named fields,teleport.audit.{aws,db_aws}andteleport.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 ofteleport.audit.aws_regionv.teleport.audit.db_aws_regionshould be; are these mutually exclusive?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One option here to move forward could be creating an issue with the information we have now, and skip this
teleportsystem test linking to that issue.If the test is skipped, I'll remove the changes done to the
event-groupsingest pipeline in this PR.WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the
assertconfiguration to ensure thatelastic-packagewould wait for that number of docs ingested into the data stream (a21024c)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@efd6 As I'm not totally confident about the changes for this ingest pipeline, I'll create a new PR just for the
teleportchanges, so it can be checked independently.To not block merging the changes of the other packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to get @chrisberkhout to think about this when he is back.