-
Notifications
You must be signed in to change notification settings - Fork 423
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
Add Latest Transform - Wiz Vulnerabilities #10895
Add Latest Transform - Wiz Vulnerabilities #10895
Conversation
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
@CohenIdo, the CI is failing with error:
If you added the necessary permissions into Elasticsearch which will be merged into, say
|
packages/wiz/elasticsearch/transform/latest_cdr_vulnerabilities/fields/base-fields.yml
Outdated
Show resolved
Hide resolved
packages/wiz/elasticsearch/transform/latest_cdr_vulnerabilities/transform.yml
Show resolved
Hide resolved
packages/wiz/elasticsearch/transform/latest_cdr_vulnerabilities/transform.yml
Show resolved
Hide resolved
packages/wiz/elasticsearch/transform/latest_cdr_vulnerabilities/transform.yml
Show resolved
Hide resolved
packages/wiz/manifest.yml
Outdated
@@ -1,17 +1,17 @@ | |||
format_version: 3.0.2 | |||
name: wiz | |||
title: Wiz | |||
version: "1.7.2" | |||
version: '1.8.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.
As discussed with @CohenIdo we might want to make this change under preview
version first as the UX is not yet ready, plus as the change is quite big and requires more e2e testing
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.
lets put this on preview guys
I don't think we should release this change without a full QA cycle
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.
@kfirpeled yes, me and @maxcold were agreed on that and put it on today sync agenda sync to hear other thoughts. So it seems we all agree. will do
🚀 Benchmarks reportTo see the full report comment with |
packages/wiz/changelog.yml
Outdated
@@ -1,4 +1,9 @@ | |||
# newer versions go on top | |||
- version: "1.8.0-preview01" |
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.
Why is this not just a release?
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 the change introduces the transform we want to test the approach extensively before opening it up for users. There are also some UX adjustments we might need to do based on the real data coming from Wiz. Making the integration a preview first, will allow the team better DX for testing the features. This approach works well for us in the cloud_security_posture integration
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.
Just to make it clearer, any bug fix can be merged under 1.7.x
and any changes that are for 8.16 will be under 1.8.0-preview0x
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 bugfixes were continued to be merged under 1.7.x
, I think the users will not be seeing them because the mainfest
's kibana.version
has moved to 8.16.0
. The users need to be >= package's kibana.version
to get the fixes/upgrades.
- name: cloud.account.id | ||
external: ecs | ||
- name: cloud.region | ||
external: ecs | ||
- name: package.name | ||
external: ecs | ||
- name: package.version | ||
external: ecs | ||
- name: vulnerability.description | ||
external: ecs | ||
- name: vulnerability.id | ||
external: ecs | ||
- name: vulnerability.score.base | ||
external: ecs | ||
- name: vulnerability.score.version | ||
external: ecs | ||
- name: vulnerability.severity | ||
external: 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.
Do these need to be added?
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.
Yes, the transform destination index is not predefined in Elasticsearch (unlike the logs-*
and metrics-*
indices), so the ECS fields are not inherited automatically and must be explicitly defined.
description: Collect logs from Wiz with Elastic Agent. | ||
type: integration | ||
categories: | ||
- security | ||
- cloudsecurity_cdr | ||
conditions: | ||
kibana: | ||
version: "^8.13.0" | ||
version: "^8.16.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.
If this is the requirement, this should probably be a draft PR until we get there. If we merge this now, all the subsequent changes will either require 8.16 or have to have back-ports.
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 package will be available in Serverless as the stack version in serverless is 8.16 already. Together with the preview version, this should give the team including our product an easier way to test the new approach we are trying out on Wiz, see https://docs.google.com/document/d/1govaQj_8LwUOLtk3zrcjCEvRgyKWRuC-TUihCrk04ao/edit for more details
Are there changes planned for Wiz integration where backporting a concern?
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.
Are there changes planned for Wiz integration where backporting a concern?
It's difficult to predict bugs.
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 unexpected bug fixes can be backported to 1.7, I assume the backporting flow exists for this reason. The bumping kibana stack version while using preview versions works quite well for cloud_security_posture
, more so with the introduction of serverless. Any concern with this approach for Wiz except the potential backports?
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.
Can you explain what the feature is present in 8.16 that is not present in 8.13? If there is none, what is the reason for wanting to bump this?
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.
Can you explain what the feature is present in 8.16 that is not present in 8.13? If there is none, what is the reason for wanting to bump this?
This is the change/feature related to 8.16
thats needed: elastic/elasticsearch#112192.
The transform in this PR needs permissions to create and maintain the latest indices. These permissions are targeted for v8.16.0
in Elasticsearch.
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.
thanks @kcreddy , that's correct. Plus we release UX features that would leverage the latest transform results, without them having transform would be just a waste of resources. and yes, won't event work properly due to missing privileges
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'm going to remove my block, but despite the documented backport release workflow, in practice, making this change will in all likelihood result in reduction in fixes being released for pre-8.16-dependent versions of the package. This is just an observation on normal dev responses to additional friction from this kind of thing.
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 thanks for providing more context for your concerns. If I understand correctly, the concerns are not about fixing specific Wiz bugs, but rather about making overall improvements to multiple integrations which is hard then to backport to each of these integrations one by one. I understand this concern, and I think it is a very valid one. But still, I think it's important to have a way not to keep the prs open for months. With serverless and with increased gap between versions it becomes a must in my opinion.
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.
Hold purely to avoid a merge before the version issue is discussed within the team.
There is a simple way to backport for the pervious stack version. |
More details on supporting bugfixes on older package versions: https://www.elastic.co/guide/en/integrations-developer/current/developer-workflow-support-old-package.html |
💚 Build Succeeded
History
|
Quality Gate failedFailed conditions |
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
Package wiz - 1.9.0-preview01 containing this change is available at https://epr.elastic.co/search?package=wiz |
solves:
Summary