-
Notifications
You must be signed in to change notification settings - Fork 459
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
[Cloud Security] release Wiz version 2.0.0 #11414
[Cloud Security] release Wiz version 2.0.0 #11414
Conversation
🚀 Benchmarks reportPackage
|
Data stream | Previous EPS | New EPS | Diff (%) | Result |
---|---|---|---|---|
vulnerability |
3215.43 | 2557.54 | -657.89 (-20.46%) | 💔 |
To see the full report comment with /test benchmark fullreport
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
@@ -11,7 +11,5 @@ | |||
type: text | |||
- name: remediation | |||
type: text | |||
- name: references | |||
type: text | |||
- name: reference | |||
type: 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.
The rule
fields are part of ECS. Anywhere we have an ECS field we should depend on the ECS definition through the use of external: ecs
to ensure that we always have consistent mappings. This ensures that there are never data type conflicts between data streams for ECS fields.
This is what it would look like to use external: ecs
for the ECS fields in wiz -- d3d335c.
However, after doing that clean-up it becomes easier to spot other problems (not new in this PR)
-
There are two problematic field definitions that are conflicting with ECS.
packages/wiz/data_stream/cloud_configuration_finding/fields/rule.yml:10:7 rule.description field declared as type text conflicts with the ECS data type keyword (conflict) packages/wiz/data_stream/cloud_configuration_finding/fields/rule.yml:14:7 rule.reference field declared as type text conflicts with the ECS data type keyword (conflict)
-
rule.remediation has multiple data types (keyword, text)
packages/wiz/elasticsearch/transform/latest_cdr_misconfigurations/fields/rule.yml:12:7 keyword (conflict) packages/wiz/data_stream/cloud_configuration_finding/fields/rule.yml:12:7 text (conflict)
-
duplicate definitions of the same field in a data stream
packages/wiz/elasticsearch/transform/latest_cdr_misconfigurations/fields/ecs.yml:1:3 event.created is declared 2 times (duplicate) packages/wiz/elasticsearch/transform/latest_cdr_misconfigurations/fields/ecs.yml:19:3 additional definition (duplicate) packages/wiz/elasticsearch/transform/latest_cdr_vulnerabilities/fields/fields.yml:5:3 package.name is declared 2 times (duplicate) packages/wiz/elasticsearch/transform/latest_cdr_vulnerabilities/fields/fields.yml:23:3 additional definition (duplicate)
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.
@andrewkroh thanks for the detailed feedback! I updated the mapping according to your suggestion. A couple of questions:
- how do I check for conflicts with ECS? neither
lint
norcheck
produce the same output as you have in your comment - I changed
rule.description
andrule.reference
toecs
and that means the mapping changed fromtext
tokeyword
which is a breaking change as far as I understand. What should be the guidance for users in the changelog in that case?
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.
how do I check for conflicts with ECS?
I don't know that elastic-package has this capability yet. I have an unofficial means that I occasionally use to audit the mappings.
I changed rule.description and rule.reference to ecs and that means the mapping changed from text to keyword which is a breaking change as far as I understand. What should be the guidance for users in the changelog in that case?
It's hard to prescribe one solution because it depends on how the data is being used and what problem they encounter (types of queries or aggregations they are performing). One solution for conflicts mentioned in this blog post is to apply a runtime field to the old index to make the field a keyword.
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.
Since we are adhering to ECS, should we even define the rule fields that are of external: ecs
? The logs-*
default ecs@mappings
component template should take care of them for the datastream.
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.
@kcreddy @andrewkroh Looking at the value of the ecs@mappings
in my stack I don't see it as a replacement of external: ecs
. Actually, I would like to understand the use of ecs@mappings
better, as in our team we understood that it is a replacement of manual mapping, but then not everything that is available in ECS also mapped in this component template. Eg. there are no mappings for the rule.*
fields, except the ones falling into broader mappings such as *.name
{
"dynamic_templates": [
{
"ecs_timestamp": {
"mapping": {
"ignore_malformed": false,
"type": "date"
},
"match": "@timestamp"
}
},
{
"ecs_message_match_only_text": {
"path_match": [
"message",
"*.message"
],
"mapping": {
"type": "match_only_text"
},
"unmatch_mapping_type": "object"
}
},
{
"ecs_non_indexed_keyword": {
"path_match": [
"*event.original"
],
"mapping": {
"index": false,
"type": "keyword",
"doc_values": false
}
}
},
{
"ecs_non_indexed_long": {
"path_match": [
"*.x509.public_key_exponent"
],
"mapping": {
"index": false,
"type": "long",
"doc_values": false
}
}
},
{
"ecs_ip": {
"path_match": [
"ip",
"*.ip",
"*_ip"
],
"mapping": {
"type": "ip"
},
"match_mapping_type": "string"
}
},
{
"ecs_wildcard": {
"path_match": [
"*.io.text",
"*.message_id",
"*registry.data.strings",
"*url.path"
],
"mapping": {
"type": "wildcard"
},
"unmatch_mapping_type": "object"
}
},
{
"ecs_path_match_wildcard_and_match_only_text": {
"path_match": [
"*.body.content",
"*url.full",
"*url.original"
],
"mapping": {
"fields": {
"text": {
"type": "match_only_text"
}
},
"type": "wildcard"
},
"unmatch_mapping_type": "object"
}
},
{
"ecs_match_wildcard_and_match_only_text": {
"mapping": {
"fields": {
"text": {
"type": "match_only_text"
}
},
"type": "wildcard"
},
"unmatch_mapping_type": "object",
"match": [
"*command_line",
"*stack_trace"
]
}
},
{
"ecs_path_match_keyword_and_match_only_text": {
"path_match": [
"*.title",
"*.executable",
"*.name",
"*.working_directory",
"*.full_name",
"*file.path",
"*file.target_path",
"*os.full",
"*email.subject",
"*vulnerability.description",
"*user_agent.original"
],
"mapping": {
"fields": {
"text": {
"type": "match_only_text"
}
},
"type": "keyword"
},
"unmatch_mapping_type": "object"
}
},
{
"ecs_date": {
"path_match": [
"*.timestamp",
"*_timestamp",
"*.not_after",
"*.not_before",
"*.accessed",
"created",
"*.created",
"*.installed",
"*.creation_date",
"*.ctime",
"*.mtime",
"ingested",
"*.ingested",
"*.start",
"*.end",
"*.indicator.first_seen",
"*.indicator.last_seen",
"*.indicator.modified_at",
"*threat.enrichments.matched.occurred"
],
"mapping": {
"type": "date"
},
"unmatch_mapping_type": "object"
}
},
{
"ecs_path_match_float": {
"path_match": [
"*.score.*",
"*_score*"
],
"mapping": {
"type": "float"
},
"path_unmatch": "*.version",
"unmatch_mapping_type": "object"
}
},
{
"ecs_usage_double_scaled_float": {
"path_match": "*.usage",
"mapping": {
"scaling_factor": 1000,
"type": "scaled_float"
},
"match_mapping_type": [
"double",
"long",
"string"
]
}
},
{
"ecs_geo_point": {
"path_match": [
"*.geo.location"
],
"mapping": {
"type": "geo_point"
}
}
},
{
"ecs_flattened": {
"path_match": [
"*structured_data",
"*exports",
"*imports"
],
"mapping": {
"type": "flattened"
},
"match_mapping_type": "object"
}
},
{
"all_strings_to_keywords": {
"mapping": {
"ignore_above": 1024,
"type": "keyword"
},
"match_mapping_type": "string"
}
}
]
}
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.
AFAIU the idea of ecs@mappings
component template is to have a generic set of dynamic templates that works across all ECS entities, instead of defining mappings for specific entities like rule.*
, vulnerability.*
etc.
In our case, following are rule.*
fields that are defined as external: ecs
and the corresponding dynamic template inside ecs@mappings
that is handling it:
Field in rule.yml |
Handled by dynamic template |
---|---|
rule.uuid | all_strings_to_keywords |
rule.id | all_strings_to_keywords |
rule.name | ecs_path_match_keyword_and_match_only_text |
rule.description | all_strings_to_keywords |
rule.reference | all_strings_to_keywords |
Comparing the above dynamic mappings with ECS rule fields, it matches correctly.
rule.remediation
is anyway manually defined as it is not in ECS.
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.
@kcreddy now I see how keyword
mapping is being solved in the template, thanks. Then yes, I agree, it makes sense to remove these explicit ecs mappings
Co-authored-by: Andrew Kroh <[email protected]>
packages/wiz/data_stream/cloud_configuration_finding/fields/base-fields.yml
Outdated
Show resolved
Hide resolved
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.
Change external: esc
to external: ecs
in PR commit message.
packages/wiz/data_stream/cloud_configuration_finding/fields/rule.yml
Outdated
Show resolved
Hide resolved
Co-authored-by: Krishna Chaitanya Reddy Burri <[email protected]>
@maxcold, should this PR also contain a documentation update suggesting users to update Just asking as I need to possibly document same for AWS SecurityHub. |
@kcreddy regarding the docs, the work on them is in progress, when product with docs team come up with a final version we can incorporate these changes in our PRs |
@@ -1,34 +1,36 @@ | |||
# newer versions go on top | |||
- version: "1.9.0-preview07" | |||
- version: "2.0.0" |
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 don't think this is the correct way to do this; the versions that have been released should continue to exist.
Also, what is it that makes this a 2.0.0? What is breaking here?
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.
what would be the correct way to do that? From a user's point of view if they never enabled "show beta integrations", they would upgrade from 1.8.1 (latest released non-preview version) to 2.0.0 and would get all the changes from 1.9.0-preview*
with the change, and as a user I would expect to see this as a part of 2.0.0 changelog
Also, what is it that makes this a 2.0.0? What is breaking here?
this is reflected in the changelog
- description: Rely on external ecs for ESC fields. rule.reference, rule.descipriton and rule.remediation changed from text to keyword.
type: breaking-change
link: https://github.com/elastic/integrations/pull/11414
- description: Remove rule.references field and its mapping. Please use the ECS rule.reference field instead.
type: breaking-change
link: https://github.com/elastic/integrations/pull/11414
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 change here inserts a bunch of breaking changes before a bugfix that had already been released as a 1.9.0 preview. If correct chronological order had been used and they were added as a 1.9.0, the full set of 1.9.0 changes would not have been breaking; there would have been a single new version entry looking like this:
- version: "1.9.0"
changes:
- description: Add cloud.account.name mapping to latest_cdr_vulnerabilities transform destination.
type: bugfix
link: https://github.com/elastic/integrations/pull/<SOMEOTHERPRNUMBER>
- description: Default to resource id when provider_id is missing for resource.id field in cloud_configuration_finding data stream.
type: enhancement
link: https://github.com/elastic/integrations/pull/<SOMEOTHERPRNUMBER>
- description: Add cloud.account.name mapping to latest_cdr_vulnerabilities transform destination.
type: enhancement
link: https://github.com/elastic/integrations/pull/11414
(leaving all the other already published versions in tact)
then you could have added this breaking enhancement:
- version: "2.0.0"
changes:
- description: Rely on external ecs for ESC fields. rule.reference, rule.descipriton and rule.remediation changed from text to keyword.
type: breaking-change
link: https://github.com/elastic/integrations/pull/11414
- description: Remove rule.references field and its mapping. Please use the ECS rule.reference field instead.
type: breaking-change
link: https://github.com/elastic/integrations/pull/11414
This is not an academic issue; there is a bugfix that is being hidden here from people who may not want to upgrade across a break, and also blocks them from getting non-breaking enhancements. Admittedly, there is a fix in 1.8.something hidden in a backport branch, so the bugfix is less of a concern, but the non-breaking enhancements are still an issue.
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 1.9.0-preview1 and 1.9.0-preview3 introduce the latest transforms, which we plan to release as part of the CDR initiative. These changes require the 8.16.x version of the stack; that's why we were asked to hold them in an open PR until closer to the 8.16 release date, as the flow with preview is not advised to be used. So, I can't release 1.9.0 just yet. Additionally, there are problems that we found while testing the CDR approach, together with the product folks, which require breaking changes before we can release transforms. That's why I'm skipping the 1.9.0 release and moving to 2.0.0, as all the changes there are bundled under CDR. We don't want users to be able to install the version with the transforms before the breaking changes are made. I can move the breaking change entries to the top of the changelog if you think that this would help.
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.
This is deeply dissatisfying. I have removed my block.
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.
LGTM for my comments.
Please merge after Dan's comments are addressed regarding version.
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 are several fields configured as ecs that also have a specified type. Is there a reason for this configuration?
💚 Build Succeeded
History
|
Quality Gate failedFailed conditions |
Package wiz - 2.0.0 containing this change is available at https://epr.elastic.co/search?package=wiz |
Proposed commit message
Changes:
rule.references
non-ecs as this field is not required anymore by Kibana https://github.com/elastic/security-team/issues/10793 . The fieldrule.reference
should be used insteadtext
changed tokeyword
to match ecsresource.id
is provided fromid
whenprovider_id
is missing on Wizexternal: esc
is added for ECS fieldscloud.account.name
mapping to vulnerability cdr transform destinationChecklist
changelog.yml
file.Author's Checklist
How to test this PR locally
Related issues
Screenshots