[Security Solution] [Sourcerer] Replaces references to sourcerer search strategy with api provided by kibana data views plugin

[dhurley14]

Summary

Original outline: #138181

Issues outlining the objective of this pr: #142903 and #142907

---
title: Sourcerer Deprecation Journey
---
flowchart LR
    A[8.7 matchedIndices <a href='https://github.com/elastic/kibana/pull/139671'>#139671</a>] --> B[Replace Sourcerer \n Search Strategy apis]
    subgraph you are here
    B
    end
    B --> C[Replace Sourcerer UI \nwith data view picker <a href='https://github.com/elastic/security-team/issues/6235'>#6235</a>]
    B --> D[Update code to use \n properties from \nkibana data view, \nnot sourcerer data view <a href='https://github.com/elastic/kibana/issues/155100'>#155100</a>]
    C --> E[Replace useSourcererDataView \nhook with Redux selector]
    D --> E[Replace useSourcererDataView \nhook with Redux selector]
    E --> F[Finished!]

Loading

Overview

Since the data views plugin was introduced, maintaining our own apis for fetching sourcerer saved objects (data views) and additional types has become cumbersome and inefficient. The data views plugin provides both an efficient caching of data view saved objects and a unified interface for creating ad-hoc data views (see the changes to the useFetchIndex hook in this PR) so that our code can now rely on a single type of saved object to interface with when fetching data.

This PR is another step towards replacing sourcerer with the data view picker provided by kibana platform (which benefits users by maintaining consistency around data source selection UX) and additionally provides benefits to developers in the security solution by allowing us to reduce state-management complexity in components that rely on old indexPattern types or data view types.

What does this PR do, exactly?

This PR removes references to the search strategies that were implemented before the data views plugin became available, and replaces them with their corresponding apis available in the kibana data view plugin. These search strategies provided access to the data view saved object as well as the fields associated with the data view or array of index name strings.

I have also marked the server-side search strategies as deprecated (rather than deleting them) as they are being used by other teams and might be used by software outside of elastic, and I have also marked the following properties as deprecated; browserFields, indexFields, runtimeMappings, indexPatterns, patternList, selectedPatterns, and activePatterns. Once we have removed all references to the above listed properties we can use the data view type in our code without having to extend from it.

Next steps

In a perfect world we would be able to remove all the references to the old memoized properties on the useSourcererDataView hook (like patternList) and replace them with their counterparts on the sourcererDataView object returned by that same hook (like title or w/e the non-deprecated accessor is). After that is complete, we would be able to remove the useSourcererDataView hook entirely and replace that with the data view stored in redux returned by the selector associated with sourcerer, and then finally remove the sourcerer UI component and replace it with the UI component provided by kibana.

The faster way is to update the UI and replace the sourcerer component with the kibana data view picker, and update the corresponding data view stored in redux and use that selector in the useSourcererDataView hook, much like how we are changing the backend systems in this pr and just mapping those properties from the data view SO to the expected memoized properties in the useSourcererDataView hook. Again, the end goal is really to just use the selector directly in our components and not rely on any hooks, like the following:

kibana/src/plugins/discover/public/application/main/components/top_nav/discover_topnav.tsx

Line 66 in 07ca356

const dataView = useInternalStateSelector((state) => state.dataView!);

Bonus round

Additionally, I have written up an issue for next steps to be made to the useFetchIndex hook which will allow it to take either an array of index names (as it currently does, returning an ad-hoc data view) or a data view id (and returning a data view). By making this change, components can remove extraneous logic relying on updating state with effects like this

kibana/x-pack/plugins/security_solution/public/timelines/components/fields_browser/index.tsx

Lines 52 to 71 in 2225de0

	const [dataView, setDataView] = useState<DataView | null>(null);
	const { startTransaction } = useStartTransaction();
	const { indexFieldsSearch } = useDataView();
	const {
	dataViewFieldEditor,
	data: { dataViews },
	} = useKibana().services;
	const scopeIdSelector = useMemo(() => sourcererSelectors.scopeIdSelector(), []);
	const { missingPatterns, selectedDataViewId } = useDeepEqualSelector((state) =>
	scopeIdSelector(state, sourcererScope)
	);
	useEffect(() => {
	if (selectedDataViewId != null && !missingPatterns.length) {
	dataViews.get(selectedDataViewId).then((dataViewResponse) => {
	setDataView(dataViewResponse);
	});
	}
	}, [selectedDataViewId, missingPatterns, dataViews]);

And like this:

kibana/x-pack/plugins/security_solution/public/detections/components/rules/step_define_rule/index.tsx

Lines 289 to 319 in 2225de0

	const [isIndexPatternLoading, { browserFields, indexPatterns: initIndexPattern }] = useFetchIndex(
	index,
	false
	);
	const [indexPattern, setIndexPattern] = useState<DataViewBase>(initIndexPattern);
	const { data } = useKibana().services;
	// Why do we need this? to ensure the query bar auto-suggest gets the latest updates
	// when the index pattern changes
	// when we select new dataView
	// when we choose some other dataSourceType
	useEffect(() => {
	if (dataSourceType === DataSourceType.IndexPatterns) {
	if (!isIndexPatternLoading) {
	setIndexPattern(initIndexPattern);
	}
	}
	if (dataSourceType === DataSourceType.DataView) {
	const fetchDataView = async () => {
	if (dataViewId != null) {
	const dv = await data.dataViews.get(dataViewId);
	setDataViewTitle(dv.title);
	setIndexPattern(dv);
	}
	};
	fetchDataView();
	}
	}, [dataSourceType, isIndexPatternLoading, data, dataViewId, initIndexPattern]);

And again..

kibana/x-pack/plugins/security_solution/public/detection_engine/rule_exceptions/logic/use_exception_flyout_data.tsx

Lines 47 to 120 in 2225de0

	const memoDataViewId = useMemo(
	() =>
	rules != null && isSingleRule
	? rules[0].data_view_id || null
	: `security-solution-${activeSpaceId}`,
	[isSingleRule, rules, activeSpaceId]
	);
	const memoNonDataViewIndexPatterns = useMemo(
	() =>
	!memoDataViewId && rules != null && isSingleRule && rules[0].index != null
	? rules[0].index
	: [],
	[memoDataViewId, isSingleRule, rules]
	);
	// Index pattern logic for ML
	const memoMlJobIds = useMemo(
	() => (isMLRule && isSingleRule && rules != null ? rules[0].machine_learning_job_id ?? [] : []),
	[isMLRule, isSingleRule, rules]
	);
	const { loading: mlJobLoading, jobs } = useGetInstalledJob(memoMlJobIds);
	// We only want to provide a non empty array if it's an ML rule and we were able to fetch
	// the index patterns, or if it's a rule not using data views. Otherwise, return an empty
	// empty array to avoid making the `useFetchIndex` call
	const memoRuleIndices = useMemo(() => {
	if (isMLRule && jobs.length > 0) {
	return jobs[0].results_index_name ? [`.ml-anomalies-${jobs[0].results_index_name}`] : [];
	} else if (memoDataViewId != null) {
	return [];
	} else {
	return memoNonDataViewIndexPatterns;
	}
	}, [jobs, isMLRule, memoDataViewId, memoNonDataViewIndexPatterns]);
	const [isIndexPatternLoading, { indexPatterns: indexIndexPatterns }] = useFetchIndex(
	memoRuleIndices,
	false,
	'indexFields',
	true
	);
	// Data view logic
	const [dataViewIndexPatterns, setDataViewIndexPatterns] = useState<DataViewBase | null>(null);
	useEffect(() => {
	const fetchSingleDataView = async () => {
	// ensure the memoized data view includes a space id, otherwise
	// we could be trying to fetch a data view that does not exist, which would
	// throw an error here.
	if (activeSpaceId !== '' && memoDataViewId) {
	setDataViewLoading(true);
	const dv = await data.dataViews.get(memoDataViewId);
	const fieldsWithUnmappedInfo = await data.dataViews.getFieldsForIndexPattern(dv, {
	pattern: '',
	includeUnmapped: true,
	});
	setDataViewLoading(false);
	setDataViewIndexPatterns({
	...dv,
	fields: fieldsWithUnmappedInfo,
	});
	}
	};
	fetchSingleDataView();
	}, [memoDataViewId, data.dataViews, setDataViewIndexPatterns, activeSpaceId]);
	// Determine whether to use index patterns or data views
	const indexPatternsToUse = useMemo(
	(): DataViewBase =>
	memoDataViewId && dataViewIndexPatterns != null ? dataViewIndexPatterns : indexIndexPatterns,
	[memoDataViewId, dataViewIndexPatterns, indexIndexPatterns]
	);

Checklist

Delete any items that are not applicable to this PR.

• Unit or functional tests were updated or added to match the most common scenarios

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[paul-tavares]

Changes to the management page looks good 👍

[vitaliidm]

can we avoid disabling this eslint rule?
It may lead to stale data in hook and relying business logic on eslint rule would increase cost of maintenance and will be prone to issues in future

[vitaliidm]

nit: were these tests just swap in places? I don't see any changes in them. If it so, maybe swapping it back would remove this file from PR diff

[dhurley14]

Yup just a swap. In their original format they were failing but switching them around made it pass.

[nkhristinin]

Do you know why they failed?

Do they have some dependant data?

[jpdjere]

Was this a timeout?
I wrote this tests and can't figure out why switching the order would help.
I could revisit them later if you still have some of the logs for these

[vitaliidm]

can we avoid disabling this eslint rule?
It may lead to stale data in hook and relying business logic on eslint rule would increase cost of maintenance and will be prone to issues in future

[vitaliidm]

can we use dataView.fields here since browserFields is deprecated?

[dhurley14]

The shape of the data is different between the two so unfortunately until all references to browserFields in the codebase are removed or have been updated to use the fields property on the dataview, we won't be able to remove it here.

[vitaliidm]

should loading be set to false if error happens?

[vitaliidm]

this casting looks unnecessary and can be removed

[nkhristinin]

Hey, what is the reason to use use callback here?

The idea here, is to just fetch data view once, when the component mount.

Why not do:

useEffect(() => {
    const fetchDV = async () => {...}
    fetchDV();
    };, []);

[nkhristinin]

The same question as above

[nkhristinin]

Also found that we use deprecated browserFields here

Should it be fixed? and if yes in this PR, on next steps?

[nkhristinin]

I left some comments, but it LGTM!
Great work @dhurley14, and really nice description of the story which provides a lot of context!

[kevinlog]

Thanks for the discussion offline @dhurley14

After discussion with @peluja1012 and @caitlinbetz we are OK with this change since we have required logs-* read permissions in Elastic Security for some time. Docs: https://www.elastic.co/guide/en/security/current/sec-requirements.html#_kibana_space_and_index_privileges

[jpdjere]

Detection Rules LGTM 👍

Wondering about the flakiness of those Cypress tests, and why switching them help. Of course, not a blocker, but would like to revisit later.

[michaelolo24]

Is there ever a situation where this won't be set? Fyi @kqualters-elastic

[michaelolo24]

Investigations codeowner changes. Nice work, LGTM!

[vitaliidm]

alerts area LGTM. thanks for addressing comments

[cnasikas]

ResponseOps changes LTGM.

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 786c481

Failed CI Steps

• FTR Configs #45

Test Failures

• [job] [logs] FTR Configs #45 / console app console context menu should copy as curl and show toast when copy as curl button is clicked

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
triggersActionsUi	520	522	+2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
securitySolution	9.1MB	9.1MB	-6.6KB
triggersActionsUi	1.4MB	1.4MB	-1.3KB
total			-7.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
securitySolution	60.3KB	60.4KB	+147.0B
triggersActionsUi	84.8KB	86.9KB	+2.1KB
total			+2.2KB

Unknown metric groups

API count

id	before	after	diff
triggersActionsUi	549	551	+2

ESLint disabled line counts

id	before	after	diff
enterpriseSearch	17	19	+2
securitySolution	397	402	+5
total			+7

References to deprecated APIs

id	before	after	diff
@kbn/securitysolution-data-table	0	15	+15
securitySolution	380	546	+166
total			+181

Total ESLint disabled count

id	before	after	diff
enterpriseSearch	18	20	+2
securitySolution	477	482	+5
total			+7

History

• 💔 Build #123192 failed ef23315
• 💔 Build #122958 failed 9fa7287
• 💔 Build #122721 failed 78aa4d4
• 💚 Build #122436 succeeded 4dce02b
• 💔 Build #122125 failed 4a824e6

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

cc @dhurley14

[peluja1012]

Hi @vitaliidm, we will address the comments related to the disabled es-lint rules when we refactor the Rule Creation page components. We were running into unexpected behavior with hooks otherwise.