[Security Solution + Files + Fleet] Add option to Files client to handle index alias and fix Endpoint/Fleet usage to set new option to true #153342

paul-tavares · 2023-03-20T22:15:50Z

Summary

Adds indexIsAlias to Files plugin client. Used when provided indexes are Aliases (changes how the documents are retrieved internally)
Changes security solution (endpoint) file service to use .search() instead of .get() when retrieving a file metadata via id
Changed Security Solution call to createEsFileClient() (Files plugin service) to set indexIsAlias to true
Changed Fleet call to createEsFileClient() (Files plugin service) to set indexIsAlias to true

Addresses the following Issues that were raised for 8.7:

… file client

paul-tavares · 2023-03-21T15:55:28Z

src/plugins/files/server/blob_storage_service/adapters/es/content_stream/content_stream.ts

        : undefined;

+      // Because `asStream` was used in retrieving the document, errors will also not be processes


FYI: this was something that had caused me some trouble in the past. When asStream is used, errors in ES are also not processed, so the call does not actually fail. We check here to see if retrieving the chunk was successful and error/log it if not.

paul-tavares · 2023-03-21T15:56:47Z

src/plugins/files/server/blob_storage_service/adapters/es/content_stream/content_stream.ts

+      // to retrieve the chunk.
+      if (decodedChunkDoc && ('error' in decodedChunkDoc || !decodedChunkDoc.found)) {
+        const err = new Error(`Failed to retrieve chunk id [${id}] from index [${chunkIndex}]`);
+        this.logger.error(err);


FYI: I normally prefer .error() instead of .debug just because it will provide imediate feedback in the logs without requiring a customer to set debugging to debug. The code base seems to (for the most part) use .debug() so let me know if I should change it.

elasticmachine · 2023-03-21T16:32:46Z

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

kpollich

Fleet change (one line/one file) LGTM 🚀

kpollich · 2023-03-21T16:37:04Z

x-pack/plugins/fleet/server/services/agents/uploads.ts

@@ -186,6 +186,7 @@ export async function getAgentUploadFile(
      metadataIndex: FILE_STORAGE_METADATA_AGENT_INDEX,
      elasticsearchClient: esClient,
      logger: appContextService.getLogger(),
+      indexIsAlias: true,


elasticmachine · 2023-03-21T16:37:22Z

Pinging @elastic/fleet (Team:Fleet)

vadimkibana

Great work! Files services code changes LGTM.

In Slack you mentioned about testing, there are integration tests in src/plugins/files/server/blob_storage_service/adapters/es/integration_tests/es.test.ts for this code.

You can run them with

yarn test:jest_integration src/plugins/files/server/blob_storage_service/adapters/es/integration_tests/es.test.ts

You can either add a test case for indexIsAlias there, or copy the whole test suite and execute it using indexIsAlias: true.

vadimkibana · 2023-03-21T16:49:04Z

Nothing actionable, just curious, why did you decide instead of fetching the chunk by "search", to instead execute a query to find the index first and then fetch by "get"?

ashokaditya

Thanks for the changes. 🔥 I only have a few nits but this is good to 🚢

ashokaditya · 2023-03-21T17:34:51Z

src/plugins/files/server/file_client/create_es_file_client.ts

@@ -30,6 +30,13 @@ export interface CreateEsFileClientArgs {
   * An elasticsearch client that will be used to interact with the cluster.
   */
  elasticsearchClient: ElasticsearchClient;
+  /**
+   * Tread the indexes provided as Aliases. If set to true, ES `search()` will be used to


nit: Treat the indices...

ashokaditya · 2023-03-21T17:35:53Z

src/plugins/files/server/file_client/create_es_file_client.ts

@@ -30,6 +30,13 @@ export interface CreateEsFileClientArgs {
   * An elasticsearch client that will be used to interact with the cluster.
   */
  elasticsearchClient: ElasticsearchClient;
+  /**
+   * Tread the indexes provided as Aliases. If set to true, ES `search()` will be used to
+   * retrieve the file info and content instead of `get()`. This is needed to ensurer the


nit: needed to ensure...

ashokaditya · 2023-03-21T17:36:59Z

src/plugins/files/server/blob_storage_service/adapters/es/content_stream/content_stream.ts

        : undefined;

+      // Because `asStream` was used in retrieving the document, errors will also not be processes


nit: ...also not be processed...

hop-dev

Thanks for this Paul!

paul-tavares · 2023-03-21T18:18:10Z

Hi @vadimkibana ,

Re:

Nothing actionable, just curious, why did you decide instead of fetching the chunk by "search", to instead execute a query to find the index first and then fetch by "get"?

I did not want to change to much in that function, so I opted to stick with the .get() and just ensure I got the correct (real) index for it. That being said, looks like asStream is also available in the .search(), so maybe I should use that instead?

kibana-ci · 2023-03-21T20:40:40Z

💚 Build Succeeded

Buildkite Build
Commit: 950f5b3

Metrics [docs]

Unknown metric groups

API count

id	before	after	diff
`files`	214	215	+1

ESLint disabled line counts

id	before	after	diff
`securitySolution`	433	436	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	513	516	+3

History

💔 Build #114416 failed 676193d
💛 Build #114232 was flaky 933636b

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @paul-tavares

kibanamachine · 2023-03-21T20:45:21Z

💔 All backports failed

Status	Branch	Result
❌	8.7	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 153342

Questions ?

Please refer to the Backport tool documentation

paul-tavares · 2023-03-21T21:02:56Z

💚 All backports created successfully

Status	Branch	Result
✅	8.7

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…dle index alias and fix Endpoint/Fleet usage to set new option to true (elastic#153342) ## Summary - Adds `indexIsAlias` to `Files` plugin client. Used when provided indexes are Aliases (changes how the documents are retrieved internally) - Changes security solution (endpoint) file service to use `.search()` instead of `.get()` when retrieving a file metadata via `id` - Changed Security Solution call to `createEsFileClient()` (`Files` plugin service) to set `indexIsAlias` to `true` - Changed Fleet call to `createEsFileClient()` (`Files` plugin service) to set `indexIsAlias` to `true` Addresses the following Issues that were raised for 8.7: - Fixes elastic#153322 - FIxes elastic#153334 (cherry picked from commit 50cc574) # Conflicts: # src/plugins/files/server/blob_storage_service/adapters/es/content_stream/content_stream.ts # src/plugins/files/server/file_client/create_es_file_client.ts

…to handle index alias and fix Endpoint/Fleet usage to set new option to true (#153342) (#153403) # Backport This will backport the following commits from `main` to `8.7`: - [[Security Solution + Files + Fleet] Add option to Files client to handle index alias and fix Endpoint/Fleet usage to set new option to true (#153342)](#153342)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)

jloleysens

Left a few post hoc thoughts @paul-tavares . Otherwise this change LGTM!

jloleysens · 2023-03-22T10:00:21Z

src/plugins/files/server/file_client/file_metadata_client/adapters/es_index.ts

+    private readonly logger: Logger,
+    private readonly indexIsAlias: boolean = false


nit: we usually try to follow the pattern of passing logger last. I understand this prevents indexIsAlias from using default and would make the refactor a little larger. Best solution may be proper DI 😄

jloleysens · 2023-03-22T10:11:58Z

src/plugins/files/server/file_client/utils.ts

+    return fileDocSearchResult.hits.hits[0] as GetResponse<TDocument>;
+  }
+
+  return esClient.get<TDocument>({


With this change, we do not have the same error behaviour for non-existent docs. A call to .get will throw where a call to .search will not.

I think we should just copy what x-pack/plugins/security_solution/server/endpoint/services/actions/action_files.ts is doing in this case.

paul-tavares · 2023-03-22T13:06:22Z

Thanks @jloleysens for the feedback.
I'm preparing another PR to add tests and will make these changes you suggested as well, now that I don't have to rush it to get into v8.7.

…tion and minor refactor (#153815) ## Summary This PR builds on top of #153342 and: - adds test for the `indexAsAlias` option that was added to `createEsFileClient()` - removes `fetchDoc()` utility (not needed) --------- Co-authored-by: kibanamachine <[email protected]>

paul-tavares added 3 commits March 20, 2023 17:52

use .search() instead of .get() to retrieve file info.

44bfda0

Endpoint Emulator tool: add delay after indexing a mock file

57316df

Fix tests and adjust error builder

933636b

paul-tavares added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.7.0 labels Mar 20, 2023

paul-tavares self-assigned this Mar 20, 2023

jen-huang mentioned this pull request Mar 21, 2023

[Fleet] Retrieving diagnostic file API returns 500 response after index has been rolled over #153322

Closed

paul-tavares added 4 commits March 21, 2023 11:10

added indexIsAlias option to es file client factory + underlying es…

5b267d9

… file client

Catch ES errors in retrieving a chunk and throw

d16905e

Added indexIsAlias to ElasticsearchBlobStorageClient

742dd8c

Update fleet to set indexIsAlias when creating a File Client

676193d

paul-tavares mentioned this pull request Mar 21, 2023

Files plugin - swap out "get by ID" calls to make it compatible with index aliases #153334

Closed

paul-tavares commented Mar 21, 2023

View reviewed changes

paul-tavares marked this pull request as ready for review March 21, 2023 16:32

paul-tavares requested review from a team as code owners March 21, 2023 16:32

paul-tavares requested review from pzl and patrykkopycinski March 21, 2023 16:32

vadimkibana requested a review from jloleysens March 21, 2023 16:35

kpollich approved these changes Mar 21, 2023

View reviewed changes

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Mar 21, 2023

vadimkibana approved these changes Mar 21, 2023

View reviewed changes

pzl approved these changes Mar 21, 2023

View reviewed changes

ashokaditya approved these changes Mar 21, 2023

View reviewed changes

hop-dev approved these changes Mar 21, 2023

View reviewed changes

patrykkopycinski approved these changes Mar 21, 2023

View reviewed changes

paul-tavares added 2 commits March 21, 2023 14:37

PR review comments

22573e5

fix tests for ContentStream

950f5b3

paul-tavares enabled auto-merge (squash) March 21, 2023 18:48

paul-tavares merged commit 50cc574 into elastic:main Mar 21, 2023

paul-tavares deleted the task/olm-6234-fix-get-file-index-access branch March 21, 2023 20:40

kibanamachine added the v8.8.0 label Mar 21, 2023

paul-tavares mentioned this pull request Mar 21, 2023

[8.7] [Security Solution + Files + Fleet] Add option to Files client to handle index alias and fix Endpoint/Fleet usage to set new option to true (#153342) #153403

Merged

jloleysens reviewed Mar 22, 2023

View reviewed changes

paul-tavares mentioned this pull request Mar 27, 2023

[Files] Tests for createEsFileClient() usage when indexAsAlias option and minor refactor #153815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution + Files + Fleet] Add option to Files client to handle index alias and fix Endpoint/Fleet usage to set new option to true #153342

[Security Solution + Files + Fleet] Add option to Files client to handle index alias and fix Endpoint/Fleet usage to set new option to true #153342

paul-tavares commented Mar 20, 2023 •

edited by kibanamachine

Loading

paul-tavares Mar 21, 2023

paul-tavares Mar 21, 2023

vadimkibana Mar 21, 2023

elasticmachine commented Mar 21, 2023

kpollich left a comment

kpollich Mar 21, 2023

elasticmachine commented Mar 21, 2023

vadimkibana left a comment

vadimkibana commented Mar 21, 2023 •

edited

Loading

ashokaditya left a comment

ashokaditya Mar 21, 2023

ashokaditya Mar 21, 2023

ashokaditya Mar 21, 2023

hop-dev left a comment

paul-tavares commented Mar 21, 2023

kibana-ci commented Mar 21, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

kibanamachine commented Mar 21, 2023

paul-tavares commented Mar 21, 2023

jloleysens left a comment

jloleysens Mar 22, 2023

jloleysens Mar 22, 2023 •

edited

Loading

paul-tavares commented Mar 22, 2023

		: undefined;

		// Because `asStream` was used in retrieving the document, errors will also not be processes

		private readonly logger: Logger,
		private readonly indexIsAlias: boolean = false

[Security Solution + Files + Fleet] Add option to Files client to handle index alias and fix Endpoint/Fleet usage to set new option to true #153342

[Security Solution + Files + Fleet] Add option to Files client to handle index alias and fix Endpoint/Fleet usage to set new option to true #153342

Conversation

paul-tavares commented Mar 20, 2023 • edited by kibanamachine Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Mar 21, 2023

kpollich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Mar 21, 2023

vadimkibana left a comment

Choose a reason for hiding this comment

vadimkibana commented Mar 21, 2023 • edited Loading

ashokaditya left a 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

hop-dev left a comment

Choose a reason for hiding this comment

paul-tavares commented Mar 21, 2023

kibana-ci commented Mar 21, 2023

💚 Build Succeeded

Metrics [docs]

API count

ESLint disabled line counts

Total ESLint disabled count

History

kibanamachine commented Mar 21, 2023

💔 All backports failed

Manual backport

Questions ?

paul-tavares commented Mar 21, 2023

💚 All backports created successfully

Questions ?

jloleysens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

paul-tavares commented Mar 22, 2023

paul-tavares commented Mar 20, 2023 •

edited by kibanamachine

Loading

vadimkibana commented Mar 21, 2023 •

edited

Loading

jloleysens Mar 22, 2023 •

edited

Loading