-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Security solution] [Endpoint] Fix bad artifact migration #111294
[Security solution] [Endpoint] Fix bad artifact migration #111294
Conversation
…e creating the new one from fleet
…tArtifact and deleteArtifact as there is an ES error in those methods if not mocked
@elasticmachine merge upstream |
Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt) |
@@ -77,10 +82,12 @@ export const generateEsRequestErrorApiResponseMock = ( | |||
); | |||
}; | |||
|
|||
export const generateArtifactEsGetSingleHitMock = (): SearchHit<ArtifactElasticsearchProperties> => { | |||
export const generateArtifactEsGetSingleHitMock = ( |
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.
FYI: Just making this mock provider a bit more robust by accepting on input an optional artifact
Pinging @elastic/fleet (Team:Fleet) |
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.
🔥 🔥
artifact.attributes.body = ( | ||
await inflateAsync(Buffer.from(artifact.attributes.body, 'base64')) | ||
).toString('base64'); | ||
} |
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.
is there a case where is not compressed? if so, do we have tests to cover it?
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.
@paul-tavares correct me if I'm wrong, but if this script is executed just one time it shouldn't be non compressed artifacts right?
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.
@academo no, there is no case where an Artifact would be stored in the Saved Object with out being compressed. I think this check is just to be safe, but agree is not technically needed.
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.
Changes to Fleet mock LGTM
expect.objectContaining({ | ||
...soArtifactEntry, | ||
compressionAlgorithm: 'zlib', | ||
body: 'eJyrVkrNKynKTC1WsoqOrQUAJxkFKQ==', |
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 noticed this (thanks for pointing it out.
Can you remove this and instead changing the migration change to not mutate the body
prop? like:
if (isCompressed(artifact.attributes)) {
artifact.attributes = {
...artifact.attributes,
body: (
await inflateAsync(Buffer.from(artifact.attributes.body, 'base64'))
).toString('base64'),
};
}
Or - capture a copy of the soArtifatEntry
here in the test prior to calling migration (above like 115) and then use that to check again on like 119 (and remove body
from here)
@@ -57,6 +65,15 @@ export const migrateArtifactsToFleet = async ( | |||
} | |||
|
|||
for (const artifact of artifactList) { | |||
if (isCompressed(artifact.attributes)) { | |||
artifact.attributes = { | |||
...artifact.attributes, |
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 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.
🚢
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
…1294) * Working test that validate migrated artifact has same properties as SO artifact * Checks if artifact is compressed and uncompress it if necessary before creating the new one from fleet Co-authored-by: Paul Tavares <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
…1294) * Working test that validate migrated artifact has same properties as SO artifact * Checks if artifact is compressed and uncompress it if necessary before creating the new one from fleet Co-authored-by: Paul Tavares <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
…1294) * Working test that validate migrated artifact has same properties as SO artifact * Checks if artifact is compressed and uncompress it if necessary before creating the new one from fleet Co-authored-by: Paul Tavares <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
…111432) * Working test that validate migrated artifact has same properties as SO artifact * Checks if artifact is compressed and uncompress it if necessary before creating the new one from fleet Co-authored-by: Paul Tavares <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: David Sánchez <[email protected]> Co-authored-by: Paul Tavares <[email protected]>
…111430) * Working test that validate migrated artifact has same properties as SO artifact * Checks if artifact is compressed and uncompress it if necessary before creating the new one from fleet Co-authored-by: Paul Tavares <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: David Sánchez <[email protected]> Co-authored-by: Paul Tavares <[email protected]>
…111431) * Working test that validate migrated artifact has same properties as SO artifact * Checks if artifact is compressed and uncompress it if necessary before creating the new one from fleet Co-authored-by: Paul Tavares <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: David Sánchez <[email protected]> Co-authored-by: Paul Tavares <[email protected]>
…-link-to-kibana-app * 'master' of github.com:elastic/kibana: (61 commits) [Logs UI] Fix alert previews for thresholds of `0` (elastic#111150) [Archive Migration][Partial] discover apps-discover (elastic#110437) [APM] Set start date of APM ML job to -4 weeks (elastic#111375) [ML] APM Latency Correlations: Code consolidation. (elastic#110790) [Discover] Fix indices permission for multiline test (elastic#111284) [Detection Rules] Add 7.15 rules (elastic#111464) [Security Solution][Endpoint][Host Isolation] Hide isolate host option in alert details rather than disabling (elastic#111064) React version of angular license view (elastic#111317) [APM] Fix link in readme (elastic#111362) [Security Solution] add agent field to generator (elastic#111428) [Dashboard] Retain Tags on Quicksave (elastic#111015) Reorder App Search ingestion methods (elastic#111361) Port performance docs to new docs system. (elastic#111063) [Security Solution][RAC] Fixes updatedAt loading bug (elastic#111010) [sample data] update web log geo.src field to match country code of geo.coordinates (elastic#110885) [Security solution] [Endpoint] Fix bad artifact migration (elastic#111294) Fix copy typo. (elastic#111203) [build] Remove empty optimize directory (elastic#111393) [Maps] fix term join not updating when editing right field (elastic#111030) [Fleet] Set default settings in component template instead of the index template (elastic#111197) ... # Conflicts: # x-pack/plugins/reporting/public/management/__snapshots__/report_listing.test.tsx.snap # x-pack/plugins/reporting/public/management/report_listing.test.tsx
…1294) * Working test that validate migrated artifact has same properties as SO artifact * Checks if artifact is compressed and uncompress it if necessary before creating the new one from fleet Co-authored-by: Paul Tavares <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Summary
Zip compression was removed during artifacts generation as it was done on the fleet side.
We need to keep this behaviour for the migration script as it is taking the old ones already zip compressed.
fixes #111210
For maintainers