elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.57k stars 8.09k forks source link

[Cloud Security] Refactor CloudPosturePage component #154200

Open opauloh opened 1 year ago

opauloh commented 1 year ago

Summary

Currently, the cloud_security_posture plugin has a single component for handling the states in the Cloud Posture Dashboard and Findings page.

One issue about using a single component is that cloud security posture has different use cases for the Dashboard and Findings page.

Dashboard page: it needs first to check if there's license permission and then if either kspm or cspm integration is installed before showing any of the Tabs in the dashboard), and then on the specific tabs, it needs to handle the NoFindingsStates (indexing, no agent, timeout, privileges) for the specific integration.

Findings page: it also needs to check if there's license permission and then if either kspm or cspm integration is installed, and when handling the No Findings State should to consider both kspm or cspm, and the current architecture is prone to errors on that checking as it fallbacks to cspm as stated on the Screenshot below:

Image

Also, as shown in the Screenshot, the CloudSecurityPage component renders itself again on the Dashboard page, which adds complexity when updating the component (harder to test, unexpecteds side effects).

Proposed architecture

This proposal is to use a single React context instead of the components, so we can leverage data sharing across all the nested components while also handling rendering according to the states:

CloudPostureContext: Checks license to access the plugin, and in case it has an error, renders a License Error component; otherwise, returns all the status of all the CSP integrations (kspm, cspm, vulnMgmt). This data will then be able to be accessed anywhere on the cloud_security_posture plugin.

That allows each page to render what it needs based on a single check. In order to do that, we are also returning combined conditions (for example, isKspmOrCspmInstalled).

The context will also return the No Finding State for each integration so that they can be used later.

Also, the context will render a Loading component while fetching the data, having only one fetching and consolidate all the statuses in a single place will solve the bugs related to Flickering (Like in the Dashboard page)

NoFindingState Changes: Currently, the NoFindingState component handles fetching and rendering and works conditionally according to the posture type (kspm or cspm), in this proposal, that component will be changed to be a stateless component that only renders the state it received. This will allow the NoFindingState component to be reused for vulnerability management while reducing the complexity of this component.

image
elasticmachine commented 1 year ago

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

tehilashn commented 1 year ago

@opauloh - thank you for bringing this up and explaining it thoroughly!

I think it's also worth to have an in person discussion for it. @kfirpeled @opauloh - wdyt? can we set up one for after 8.8?

JordanSh commented 1 year ago

I think it's also worth to have an in person discussion for it

Would like to join this discussion as well ✋

Worth noting that CloudPosturePage was originally made only to handle the loading and error states of a query. we didn't want to write the same if functions for every query and copy-paste the error and loading renders. and since we had a bunch of places that were doing that same thing, it was combined into a single-purpose component. It looks like the subscription check is also a valid entry since it affects all of our pages regardless. but I agree this component should never concern itself with specific page needs.

I think each component can take care of its own "missing data" states, and all the CloudPosturePage is supposed to do is handle initial loading and error states.

It can be seen here that the dashboard component itself is the one that handles the no findings states by calling the NoFindingsStates when the actual dashboard cannot be displayed

https://github.com/elastic/kibana/blob/fdf5cdf2d4bba63be9fff79600c2915886978a5f/x-pack/plugins/cloud_security_posture/public/pages/compliance_dashboard/compliance_dashboard.tsx#L263-L285

making this part below, inaccurate image

Regardless, I think the option of a shared context to check for data availability should be discussed upon. My main concern is that in the proposed solution it seems that some pages would be exposed to data that may be irrelevant to them, for example, the vulnerabilities page should not care about kspm or cspm findings and thus I'm not sure they need to share the same context.