Skip to content
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

[Cloud Security] Asset Inventory table flyout controls #208452

Merged
merged 20 commits into from
Feb 11, 2025

Conversation

JordanSh
Copy link
Contributor

@JordanSh JordanSh commented Jan 27, 2025

Resolves #211365
Resolves https://github.com/elastic/security-team/issues/11445

Summary

Screen.Recording.2025-02-03.at.16.11.40.mov

Adding base controls to open entity flyouts based on entity type:

  • Open flyout on expand click
  • Open next flyout without closing current flyout
  • Close flyout on clicking the same expand button
  • Close on hitting ESC or X
  • Integrates with table's row highlighting and expand icon state
  • Open universal/user/host/service flyout based on entity.type, defaults to universal
  • Added mock UniversalEntityEcs

UniversalEntityFlyout content, real UniversalEntityEcs and dynamic flyout closing will be added next.
So the content files are just placeholders right now, you dont have to review them

const entityFlyoutParams = getFlyoutParamsByEntity(entity, scopeId, contextId);

// User, Host, and Service entity flyouts rely on entity name to fetch required data
if (entity.type !== 'universal' && !entity.name) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (entity.type !== 'universal' && !entity.name) {
if (entity.type !== UNIVERSAL_ENTITY_TYPE_UNIVERSAL && !entity.name) {

Not sure if this needs a constant right now, mainly because I would prefer it to be coming from ECS and not some local value

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer it to be coming from ECS and not some local value

I agree, it would be very beneficial for consistency to have the entity.category types under ECS, just like the event field.

For consistency, I also think there's a huge benefit from having the entity.category values defined from the ECS schema, as we currently do for the event.category field. That would help us to track the source of truth outside of Kibana. cc @tinnytintin10

Comment on lines +101 to +103
const closeDynamicFlyout = () => {
closeFlyout();
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we'll add the details and preview panels, this function will expand. right now its mostly for comfort so you dont have to import both this hook and the expandable flyout api separately just to close the flyout

renderDocumentView={renderDocumentView}
expandedDoc={expandedDoc}
setExpandedDoc={onExpandDocClick}
renderDocumentView={EmptyComponent}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This table has an opinionated way to open the flyout, it expects a components to render and to save the ref into a state. I tried a bunch of changes to the table to introduce some unopinionated control options, but ended up going with the same solution Timelines used in order to keep code consistency and not having to change core logic and add new tests. We might decide to do it later if someone thinks its needed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, while the UnifiedDataTable component doesn't require renderDocumentView to be defined there's a conditional logic that relies on the presence of the renderDocumentView to display the flyout button.

I guess the usage of the EmptyComponent is ok for now, we should just test if the flyout is properly cleaned when the page is unmounted.

@JordanSh JordanSh self-assigned this Feb 3, 2025
@JordanSh JordanSh added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v9.0.0 Team:Cloud Security Cloud Security team related labels Feb 3, 2025
@JordanSh JordanSh marked this pull request as ready for review February 3, 2025 16:01
@JordanSh JordanSh requested review from a team as code owners February 3, 2025 16:01
@JordanSh JordanSh requested a review from hop-dev February 3, 2025 16:01
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security)

Copy link
Contributor

@albertoblaz albertoblaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, nice stuff!! 👏 @JordanSh

Copy link
Contributor

@hop-dev hop-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm is this behind a feature flag @JordanSh? Why merge these controls before the flyout is done, just to keep the PRs smaller?

@JordanSh
Copy link
Contributor Author

Just to confirm is this behind a feature flag @JordanSh? Why merge these controls before the flyout is done, just to keep the PRs smaller?

The entire page is behind a feature flag and for now we are still pushing small PRs which are not fully functioning. Both to keep PRs smaller, but mainly to have features integration between us

Comment on lines 10 to 16
// TODO: Asset Inventory - This file is a placeholder for the ECS schema that will be used in the Asset Inventory app
export interface UniversalEntityEcs {
id: string;
name: string;
type: 'universal' | 'user' | 'host' | 'service';
timestamp: Date;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will a universal_entity field be added to ECS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but actually it will be just named entity, i will refactor with a rename

Copy link
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JordanSh JordanSh enabled auto-merge (squash) February 11, 2025 13:31
Comment on lines 11 to 16
export interface UniversalEntityEcs {
id: string;
name: string;
type: 'universal' | 'user' | 'host' | 'service';
timestamp: Date;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that instead of universal entity, this file should reflect the entity field that's going to be added to ECS, so we could add the file under entity/index.ts,

Also according to the ECS Entity PR, the category is what we plan to use to make a high-level classification of the entity (universal, host, user, etc...), while type is going to be used for more granular classification (i.e aws_s3_bucket, gcp_cloud_storage_bucket, etc).

Suggested change
export interface UniversalEntityEcs {
id: string;
name: string;
type: 'universal' | 'user' | 'host' | 'service';
timestamp: Date;
}
export interface EntityEcs {
id: string;
source: string;
category: 'universal' | 'user' | 'host' | 'service';
type: string;
name: string;
}

Copy link
Contributor Author

@JordanSh JordanSh Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name change done, will add refactor to use category instead of type
edit: check second comment regarding this field name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be a secondary consideration, but i noticed this on observeability inventory page:

image

Might be a bit confusing for users.

Since @tinnytintin10 is on PTO currently and the ECS suggestion is still in draft, it might get changed, im ok with merging it as type and refactoring later if its needed. lmk if you think otherwise

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree let's defer that until @tinnytintin10 is back

const entityFlyoutParams = getFlyoutParamsByEntity(entity, scopeId, contextId);

// User, Host, and Service entity flyouts rely on entity name to fetch required data
if (entity.type !== 'universal' && !entity.name) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer it to be coming from ECS and not some local value

I agree, it would be very beneficial for consistency to have the entity.category types under ECS, just like the event field.

For consistency, I also think there's a huge benefit from having the entity.category values defined from the ECS schema, as we currently do for the event.category field. That would help us to track the source of truth outside of Kibana. cc @tinnytintin10

renderDocumentView={renderDocumentView}
expandedDoc={expandedDoc}
setExpandedDoc={onExpandDocClick}
renderDocumentView={EmptyComponent}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, while the UnifiedDataTable component doesn't require renderDocumentView to be defined there's a conditional logic that relies on the presence of the renderDocumentView to display the flyout button.

I guess the usage of the EmptyComponent is ok for now, we should just test if the flyout is properly cleaned when the page is unmounted.

@JordanSh JordanSh disabled auto-merge February 11, 2025 14:02
@JordanSh JordanSh enabled auto-merge (squash) February 11, 2025 16:29
@elasticmachine
Copy link
Contributor

elasticmachine commented Feb 11, 2025

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 6698 6702 +4

Async chunks

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

id before after diff
securitySolution 21.4MB 24.9MB ⚠️ +3.5MB

Page load bundle

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

id before after diff
securitySolution 92.3KB 92.5KB +219.0B

History

cc @JordanSh

@JordanSh JordanSh merged commit 000e913 into elastic:main Feb 11, 2025
9 checks passed
kapral18 added a commit to agusruidiazgd/kibana that referenced this pull request Feb 11, 2025
…on-206439

* main: (402 commits)
  [Search]: Fix Number type field to have correct property (elastic#210462)
  Change filter for rule monitoring gaps (elastic#209983)
  Update Logs Explorer deprecation messages (elastic#201307)
  [APM] Remove `error.id` in `getErrorGroupMainStatistics` query as it's not used (elastic#210613)
  [Embeddable] Fix presentation panel styles (elastic#210113)
  [ci] enable Scout reporter for on-merge-unsupported-ftrs (elastic#210627)
  [Fix][Synonyms UI]Add navigation link to the Detail breadcrumb. (elastic#209574)
  chore(dep): bump `store2` from `2.12.0` to `2.14.4` (elastic#210530)
  [scout] adding test helper `@kbn/scout-oblt` package and uptate onboarding tests (elastic#209761)
  [Cloud Security] Asset Inventory table flyout controls  (elastic#208452)
  [ML] Fix model deployment check in file uploader (elastic#209585)
  Updates archive again (elastic#209828)
  [Security Solution] Added concurrency limits and request throttling to prebuilt rule routes (elastic#209551)
  [Search] [Onboarding] Update search api to use EventEmitter instead of Provider (elastic#209784)
  [maps] lazy load map actions (elastic#210252)
  [Cloud Security] Adding telemetry collection condition based on render condition (elastic#208758)
  [Solution nav] Use flyout for Stack Management in Search and Observability solutions (elastic#208632)
  [Search] Fix Add Inference Endpoint API call (elastic#210243)
  [Agentless Connectors] Integration overview panel (elastic#210222)
  [Lens] Restore dynamic colouring by value for Last value agg (elastic#209110)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v9.0.0 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open Existing Security Flyouts from Asset Inventory
8 participants