Pin individual entities in graph#251299
Conversation
|
Pinging @elastic/contextual-security-apps (Team:Cloud Security) |
x-pack/solutions/security/packages/kbn-cloud-security-posture/common/schema/graph/v1.ts
Outdated
Show resolved
Hide resolved
| .join(', ')}), targetEntityId, | ||
| null | ||
| )` | ||
| : '| EVAL pinned = TO_STRING(null)'; |
x-pack/solutions/security/plugins/cloud_security_posture/server/routes/graph/fetch_graph.ts
Outdated
Show resolved
Hide resolved
x-pack/solutions/security/packages/kbn-cloud-security-posture/common/schema/graph/v1.ts
Fixed
Show fixed
Hide fixed
x-pack/solutions/security/packages/kbn-cloud-security-posture/common/schema/graph/v1.ts
Outdated
Show resolved
Hide resolved
| return getFilterValues(searchFilters, [ | ||
| EVENT_ID, | ||
| ...GRAPH_ACTOR_ENTITY_FIELDS, | ||
| ...GRAPH_TARGET_ENTITY_FIELDS, | ||
| ]).map(String); |
There was a problem hiding this comment.
Reads all kinds of entity IDs from applied filters (user.entity.id, service.target.entity.id, ...). Reads also from event.id, though we might read from _id instead. @kfirpeled WDYT?
There was a problem hiding this comment.
It is a nice workaround, but it is a workaround.
Eventually, we wish to manage state of list of nodes that are pinned
Since we don't have design yet for pinning an event. From your work here, I assume that if we had the ability to pin an event/alert we will add a filter of _id: eventDocId/alertDocId
That filter is really adds anything to the graph, except for pinning the node. So that's why it is a workaround for a missing pinning capability.
Just be aware that we haven't discussed it yet, to add a filter in order to pin a node. While it is true for actors and targets, it is not true for alert and event.
We previously did it for POC purposes, but it wasn't a requirement
x-pack/solutions/security/plugins/cloud_security_posture/server/routes/graph/types.ts
Outdated
Show resolved
Hide resolved
x-pack/solutions/security/plugins/cloud_security_posture/server/routes/graph/fetch_graph.ts
Outdated
Show resolved
Hide resolved
| }; | ||
| showUnknownTarget: boolean; | ||
| nodesLimit?: number; | ||
| pinnedIds?: string[]; |
There was a problem hiding this comment.
@albertoblaz I assume we are also going to use this feature when showing the graph in the entities flyout - in that case how would we know if we should pass the pinnedIds param to the fetch events query or the fetch entities query?
In order to solve it need to we could make the pinnedIds param an array of objects:
| pinnedIds?: string[]; | |
| pinnedIds?: Array<{ id: string; type: 'event' | 'alert' | 'entity' }> |
then we can filter the pinnedIds array for relevant types before calling fetch events or fetch entities and we also have an explicit context - each id is tagged with its type, so there’s no ambiguity.
There was a problem hiding this comment.
I don't see that necessary unless I'm missing something.
The current implementation is intentionally type-agnostic. If you check the ESQL query, when one of the IDs in pinnedIds matches an existing _id in our data, we return it. And for entities, we try to match each ID with every possible entity ID (user.entity.id, user.target.entity.id, service.entity.id, etc). IDs in our backend are inherently distinct so we can't have collisions across data types.
So I'd say in the entities flyout, it's the client's responsibility to pass entities-only when fetching entities and events-only when fetching events. And the backend will handle the filtering implicitly. Adding type metadata is unnecessary and makes the API schema more verbose without a clear benefit since the current approach already handles the separation correctly and allows for more data types without adding a new domain-related field i.e. pinnedRelationships.
But if I misunderstood you or you still think there's an edge case we're not covering here, please let me know and I'll update the code
There was a problem hiding this comment.
I agree with @albertoblaz for now, in case it is not necessary yet. we can leave it as is.
Worst case scenario, we can introduce a breaking change to the API and increase the API version.
There was a problem hiding this comment.
@alexreal1314 as we discussed in the sync, if you see it fit. you can change it later on
...ckages/kbn-cloud-security-posture/graph/src/components/graph_investigation/search_filters.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Rejecting fast: I expect to see API tests
Specifically, how it handles pinning entities. To verify if the event is not being duplicated.
UPDATE:
Looking at the second example you've attached, it seems to me it duplicates nodes like target and dedup the event action.
However, I'd expect to see only the actor being extracted. And the events should be grouped to 6.
Can you elaborate on the product/user experience reasons you chose to implement it this way?
x-pack/solutions/security/packages/kbn-cloud-security-posture/common/schema/graph/v1.ts
Outdated
Show resolved
Hide resolved
| return getFilterValues(searchFilters, [ | ||
| EVENT_ID, | ||
| ...GRAPH_ACTOR_ENTITY_FIELDS, | ||
| ...GRAPH_TARGET_ENTITY_FIELDS, | ||
| ]).map(String); |
There was a problem hiding this comment.
It is a nice workaround, but it is a workaround.
Eventually, we wish to manage state of list of nodes that are pinned
Since we don't have design yet for pinning an event. From your work here, I assume that if we had the ability to pin an event/alert we will add a filter of _id: eventDocId/alertDocId
That filter is really adds anything to the graph, except for pinning the node. So that's why it is a workaround for a missing pinning capability.
Just be aware that we haven't discussed it yet, to add a filter in order to pin a node. While it is true for actors and targets, it is not true for alert and event.
We previously did it for POC purposes, but it wasn't a requirement
|
|
||
| const pinnedParamsStr = pinnedIds.map((_id, idx) => `?pinned_id${idx}`).join(', '); | ||
|
|
||
| return `| EVAL pinned = CASE( |
| * @param pinnedIds - Array of IDs to check against (document _id, entity IDs) | ||
| * @returns ESQL statement string | ||
| */ | ||
| export const buildPinnedEsql = (pinnedIds?: string[]): string => { |
There was a problem hiding this comment.
nit: not sure if I'd place this function under esql_utils.ts
as it is not a repeated logic, there's no need to place it here and export it.
we should look at esql_utils.ts as a utility function that can be used by others
examples like: building json's string, lookup join with entity store. Are good examples that can be re-used by others.
There was a problem hiding this comment.
My point was not to pollute the main file and split it in smaller functions. But moved it back to fetch_graph.ts
| }); | ||
| }); | ||
|
|
||
| describe('Pinned IDs', () => { |
x-pack/solutions/security/plugins/cloud_security_posture/server/routes/graph/fetch_graph.ts
Outdated
Show resolved
Hide resolved
This reverts commit 506c58c.
Flaky Test Runner Stats🎉 All tests passed! - kibana-flaky-test-suite-runner#10725[✅] x-pack/solutions/security/test/cloud_security_posture_api/config.ts: 25/25 tests passed. |
kfirpeled
left a comment
There was a problem hiding this comment.
LGTM, @albertoblaz please check if you can find a good way to pin only a specific entity without its entire sub-graph
x-pack/solutions/security/plugins/cloud_security_posture/server/routes/graph/fetch_graph.ts
Outdated
Show resolved
Hide resolved
| }; | ||
| showUnknownTarget: boolean; | ||
| nodesLimit?: number; | ||
| pinnedIds?: string[]; |
There was a problem hiding this comment.
@alexreal1314 as we discussed in the sync, if you see it fit. you can change it later on
x-pack/solutions/security/plugins/cloud_security_posture/server/routes/graph/fetch_graph.ts
Outdated
Show resolved
Hide resolved
|
Waiting on #251178 to merge |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
History
cc @albertoblaz |
Summary
Part of the fixes needed to close #239954
PRs:
API allows for pinning events/alerts by document ID but we don't have a proper way to represent that in the UI so we're skipping for now
Screenshots
Pinning no entities from the group
Pinning "admin2@example.com" entity
Pinning "admin2@example.com" and "admin-user2@example.com" entities
Pinning all entities in actor node
pinnedIds field sent in the request payload
How to test
yarn es snapshot --license trial -E xpack.security.authc.api_key.enabled=trueyarn start --no-base-pathAdvanced settingsand make sure these toggles are on:securitySolution:enableGraphVisualizationsecuritySolution:enableAssetInventoryevent.actionfilter set to "google.iam.admin.v1.CreateUser", then keep addinguser.entity.idorevent.idfilters set to the IDs in the entity node using theORoperatorChecklist
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesbackport:*labels.Identify risks