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]Misconfiguration preview & Refactor CSP Plugin to include new package #190105

Conversation

animehart
Copy link
Contributor

@animehart animehart commented Aug 8, 2024

Summary

This PR includes the following

  1. Misconfiguration Preview on Alerts hostname
  2. Refactor some CSP Plugin code to be moved to new csp package

Note:

  • I will add the other Cypress test that covers the other scenario where indices is not empty on separate PR, as this PR is already big enough & there are still some stuff that will be changed before implementing that Cypress test
353329193-5ad22c4e-81c2-4a8b-89f7-fdbc2a686c2d

UPDATE:
As per discussion shown in the comments below, We decided we will be breaking this PR down into 4 smaller chunks to make it easier for us to review.

@animehart
Copy link
Contributor Author

/ci

@animehart
Copy link
Contributor Author

/ci

@animehart
Copy link
Contributor Author

/ci

@animehart
Copy link
Contributor Author

/ci

@animehart
Copy link
Contributor Author

/ci

2 similar comments
@animehart
Copy link
Contributor Author

/ci

@animehart
Copy link
Contributor Author

/ci

@animehart
Copy link
Contributor Author

/ci

@animehart
Copy link
Contributor Author

/ci

@animehart
Copy link
Contributor Author

/ci

@animehart animehart added the Team:Cloud Security Cloud Security team related label Aug 14, 2024
@elasticmachine
Copy link
Contributor

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

@animehart animehart added v8.16.0 release_note:skip Skip the PR/issue when compiling release notes labels Aug 14, 2024
@animehart
Copy link
Contributor Author

/ci

Copy link
Contributor

@opauloh opauloh left a comment

Choose a reason for hiding this comment

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

a few comments, but it looking good overall, please add more documentation and also instruction on how to test the PR locally

@@ -0,0 +1 @@
FILL THIS IN LATER
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that should be filled now, even if it is with a simpler description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WOOPS, completely forgotten about this

if (e instanceof Error) return e.message;
if (typeof e === 'string') return e;

return defaultMessage; // TODO: i18n
Copy link
Contributor

Choose a reason for hiding this comment

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

this todo should be very simple to do now, doesn't it?

@@ -0,0 +1 @@
FILL THIS IN LATER
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with the readme, should be filled now, especially to make it clear why there are two packages.

Comment on lines +18 to +36
"kbn_references": [
"@kbn/kibana-react-plugin",
"@kbn/core",
"@kbn/cloud-security-posture-common",
"@kbn/search-types",
"@kbn/licensing-plugin",
"@kbn/data-views-plugin",
"@kbn/unified-search-plugin",
"@kbn/ui-actions-plugin",
"@kbn/field-formats-plugin",
"@kbn/data-view-field-editor-plugin",
"@kbn/data-plugin",
"@kbn/kibana-utils-plugin",
"@kbn/charts-plugin",
"@kbn/discover-plugin",
"@kbn/fleet-plugin",
"@kbn/usage-collection-plugin",
"@kbn/share-plugin",
"@kbn/es-query",
Copy link
Contributor

Choose a reason for hiding this comment

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

does it really need all these plugins? some are not used, for example, charts, licensing, unified search, discover

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isn't it being used on type CspClientPluginStartDeps ?

Comment on lines 18 to 32
const queryHostName = {
bool: {
must: [],
filter: [
{
bool: {
should: [{ term: { 'host.name': { value: `${hostName}` } } }],
minimum_should_match: 1,
},
},
],
should: [],
must_not: [],
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be simplified by removing the defaults values:

  const queryHostName = {
    bool: {
      should: [{ term: { 'host.name': { value: `${hostName}` } } }],
      minimum_should_match: 1,
    },
  };

Also, I would recommend moving this to a function outside of the component or memoizing with useCallback to avoid redefining it on every render.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually I think the query requires the use of must, should and must_ field

color: euiThemeVars.euiColorSuccess,
},
{
key: 'Failed findings',
Copy link
Contributor

Choose a reason for hiding this comment

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

if this text is going to be rendered in the user interface we should translate it

/>
</EuiText>
),
// Commented this out until we have the expanded flyout
Copy link
Contributor

Choose a reason for hiding this comment

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

neat:

Suggested change
// Commented this out until we have the expanded flyout
// Todo: Uncomment once we have the expanded flyout

Comment on lines 82 to 109
<EuiFlexItem>
<EuiFlexGroup direction="column" gutterSize="none">
<EuiFlexItem>
<EuiText
css={css`
font-size: ${euiTheme.size.l};
font-weight: ${euiTheme.font.weight.bold};
`}
>
<b>{'-'}</b>
</EuiText>
</EuiFlexItem>
<EuiFlexItem>
<EuiText
css={css`
font-size: ${euiTheme.size.base};
font-weight: ${euiTheme.font.weight.semiBold};
`}
data-test-subj="noFindingsDataTestSubj"
>
<FormattedMessage
id="xpack.securitySolution.flyout.right.insights.misconfigurations.noFindingsDescription"
defaultMessage="No Findings"
/>
</EuiText>
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlexItem>
Copy link
Contributor

Choose a reason for hiding this comment

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

It's hard to read ternary conditions that contain a component of several lines, I would suggest breaking it down for improved readability:

<EuiFlexGroup gutterSize="none">
        {passedFindings === 0 && failedFindings === 0 ? (
	<MisconfigurationsEmptyState />
        ) : (
       <MisconfigurationsScoreChart />
	)}
 </EuiFlexGroup>

@animehart animehart requested review from a team and removed request for a team August 19, 2024 09:05
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 this file is misplaced. It currently lives under the alerts folder, but should probably live under a new host folder? If I'm not mistaken, the test code checks the host flyout, not the alert flyout.

Copy link
Contributor

Choose a reason for hiding this comment

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

moving this file to a new folder (and probably adding correct codeowners for that new folder) would remove the @elastic/security-threat-hunting-investigations team from the review

Copy link
Contributor

@Omolola-Akinleye Omolola-Akinleye left a comment

Choose a reason for hiding this comment

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

Nice refactor! I just add a few comments


export const isAllIndicesEmpty = (indices: Array<IndexDetails | undefined>) => {
const notEmptyIndices = indices.find((indice) => indice?.status !== 'empty');
return notEmptyIndices ? false : true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we can use provide simple solution with using the Array.some method. Generally, is best to avoid negated variables which can hard to understand.

return indices.some((index)=> index.status !== 'empty')

};
};

export const isAllIndicesEmpty = (indices: Array<IndexDetails | undefined>) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this function is to check if there are any available index with documents or data, then one possible suggestion is to rename the function to *isIndexWithDocsAvailable . A little hard to follow from the naming and using negated notEmptyIndices. I saw later code was added for another negation {!cspAllIndicesEmpty && <InsightEntity hostName={hostName} />}

I only see this utility function once, do you plan on using it again? Could wrap this logic inside a useMemo?

}

export interface CspClientPluginStartDeps {
// required
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need //required and '//optional' comments? In typescript, ? will help differentiate between required optional properties and required properties.

export type CspStatusCode =
| 'indexed' // latest findings index exists and has results
| 'indexing' // index timeout was not surpassed since installation, assumes data is being indexed
| 'unprivileged' // user lacks privileges for the latest findings index
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀 Nice additional context here!

@animehart animehart requested a review from opauloh August 20, 2024 00:52
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cloudSecurityPosture 702 774 +72
securitySolution 5652 5732 +80
total +152

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
@kbn/cloud-security-posture - 27 +27
@kbn/cloud-security-posture-common - 50 +50
total +77

Async chunks

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

id before after diff
cloudSecurityPosture 477.4KB 689.0KB ⚠️ +211.7KB
securitySolution 20.7MB 21.6MB ⚠️ +881.3KB
total ⚠️ +1.1MB

Page load bundle

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

id before after diff
cloudSecurityPosture 16.7KB 16.4KB -212.0B
Unknown metric groups

API count

id before after diff
@kbn/cloud-security-posture - 27 +27
@kbn/cloud-security-posture-common - 50 +50
total +77

History

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

Copy link
Contributor

@maxcold maxcold left a comment

Choose a reason for hiding this comment

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

@animehart Quite a lot of comments from my side. As the PR is very big, I didn't actually get to reviewing the misconfigurations_preview component itself.
totally understandable that you started to implement the component and only in the process realised how many things should be moved to the common plugins. I see two options:

  1. Continue in this PR but with all the comments it will be huge, hard to review and easy to miss bugs
  2. Now when we know most of the things we need to move, we can move gradually, taking our parts of this PR as separate PRs. First step can be creating our new packages but "empty". This will lay down a foundation. Then you can move the types and schemas you need to the new packages. Then hooks and utils. As a last step can go the actual implementation of the preview component. This will be much cleaner, easier to review and simpler for you I think to apply changes.

I'm personally would vote for option 2, wdyt?

# @kbn/cloud-security-posture-common

This package provides common code consumed in both the browser, i.e. the
`packages/cloud-security-posture` package and `plugins/cloud_security_posture` plugin, and on the server, i.e. the
Copy link
Contributor

Choose a reason for hiding this comment

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

packages/kbn-cloud-security-posture


## Maintainers

Maintained by the Cloud Security Posture Team
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Cloud Risk Management team. Though the team name might change, so maybe worth just mentioning the overarching Cloud Security Team

* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
export const KSPM_POLICY_TEMPLATE = 'kspm';
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 that these are copied over from the cloud_security_posture plugin. The idea of the common package is that we would use the code from this package everywhere. Ideally we shouldn't have any duplicated code after we are done with the creation of shared packages. If you are concerned with the PR size, we can do it in the follow up PR, but right away, without creating tech debt tickets


export type CspBenchmarkRuleMetadata = TypeOf<typeof cspBenchmarkRuleMetadataSchema>;

export const cspBenchmarkRuleMetadataSchema = schema.object({
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 it is duplicated from the v3 rule type. Longer term I think we will need to move all the common schemas and types here, versioned and have it here as a source of truth. Let's think how to get rid of this duplication now, eg. import in the v3 from the common and reexport . But overall we still need this TODO TODO: this needs to be defined in a versioned schema because this schema should be versioned and the all the versions should live in the common package

* 2.0.
*/

export type CspStatusCode =
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make sure there are no duplications left in our plugin, we should be using types from the common package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so for this one specifically, there seems to be an issue with Telemetry where it doesn't seem to like it when I'm using this type from the package
I have discussed this issue with @CohenIdo but still can't find the fix for it, thus we decided to have 2 separate copy

@@ -44,7 +46,12 @@ export const HostPanelContent = ({
isPreviewMode,
}: HostPanelContentProps) => {
const observedFields = useObservedHostFields(observedHost);

const getSetupStatus = useCspSetupStatusApi({
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this logic in the flyout? I don't think the flyout is smth which stays open longer, so we can rely on calling status API once when it's opened, and not to check it every 10s. In our pages we have this logic for the good onboarding experience, but in my opinion, for the flyout we can make it simpler

const getSetupStatus = useCspSetupStatusApi({
refetchInterval: 10000,
});
const notEmptyIndices = useMemo(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

check out the last PR from @JordanSh , he introduced the field in the status API hasMisconfigurationsFindings for this usecase. The current logic doesn't cover the 3rd party indexes
https://github.com/elastic/kibana/pull/190285/files#diff-7e52c60ac5b5f65b46ea55ac2d44c1f2c6783499b6057e7ea4cefc32eb9f16dbR405

@@ -71,6 +78,8 @@ export const HostPanelContent = ({
observedFields={observedFields}
queryId={HOST_PANEL_OBSERVED_HOST_QUERY_ID}
/>
<EuiHorizontalRule />
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably don't want to show it either. Plus I'd move the logic of checking our indexes inside the new InsightEntity component. And would return null if there are no data. It later will have more insight blocks, this container component doesn't need to know about the internal logic of InsightEntity

import { FormattedMessage } from '@kbn/i18n-react';
import { MisconfigurationsOverview } from './misconfigurations_overview';

export const InsightEntity = <T,>({ hostName }: { hostName: string }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming suggestion: EntityInsights

/>
</EuiText>
),
// TODO: Uncomment when we have the expanded flyout
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 remove it now, and add when we work on the expanded flyout

@maxcold
Copy link
Contributor

maxcold commented Aug 20, 2024

I also noticed that the PR added 1.1Mb to the lazily loaded async chunks which doesn't look good. Again step by step small PR probably would help identify the culprit better

animehart added a commit that referenced this pull request Aug 20, 2024
…clude new package PHASE 1 (#190832)

## Summary

The previous [PR ](#190105) was
way too big and made it hard to review without missing any bugs or
potential bugs, Thus we decided we are going to make series of smaller
PR to make things more manageable

We will be splitting it into 4 PR 
Phase 1: Creating empty packages for csp and csp-common 
Phase 2: Move Types from CSP plugin to the Package + Deleting duplicates
in the CSP plugin where possible
Phase 3: Move Functions, Utils or Helpers, Hooks to Package
Phase 4: Misconfiguration Preview feature (with Cypress test and other
required test)

This is Phase 1 of the initiative

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
* 2.0.
*/

// Add stuff here
Copy link
Member

@machadoum machadoum Aug 23, 2024

Choose a reason for hiding this comment

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

oops

Suggested change
// Add stuff here

* 2.0.
*/

// Add stuff here
Copy link
Member

Choose a reason for hiding this comment

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

oops

Suggested change
// Add stuff here

<EuiFlexItem>
<EuiFlexGroup direction="column" gutterSize="none">
<EuiFlexItem>
<EuiText
Copy link
Member

@machadoum machadoum Aug 23, 2024

Choose a reason for hiding this comment

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

Setting font size using CSS isn't a good practice. EuiText has a property for that called size.

It also happens in several other places in this file.

<EuiFlexItem />
<EuiFlexItem
css={css`
margin-top: ${euiTheme.size.l};
Copy link
Member

Choose a reason for hiding this comment

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

nit: You could use <EuiSpacer> instead of a custom margin-top.

import { ExpandablePanel } from '../../../../shared/components/expandable_panel';

export const MisconfigurationsOverview = ({ hostName }: { hostName: string }) => {
const queryHostName = useCallback(() => {
Copy link
Member

Choose a reason for hiding this comment

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

This useCallback doesn't make much sense. You are always calling the function on line 38. So, it created a new object for every render. If you want to memoise the query between render to prevent unnecessary rerender, use useMemo.

Suggested change
const queryHostName = useCallback(() => {
const hostNameQuery = useMemo(() => {

And update line 38 to use hostNameQuery

It also might be a good idea to move the query-building function to another file so we don't mix UI and ES querying logic in the same file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@machadoum
woops, i forgot to close this PR but basically we will split this PR into smaller PR like this #190933 ,
please leave the review on this other PR :) Thanks !

animehart added a commit that referenced this pull request Aug 27, 2024
…clude new package PHASE 2 (#190933)

## Summary

The previous #190105 was way too
big and made it hard to review without missing any bugs or potential
bugs, Thus we decided we are going to make series of smaller PR to make
things more manageable

We will be splitting it into 4 PR
Phase 1: Creating empty packages for csp and csp-common
Phase 2: Move Types from CSP plugin to the Package + Deleting duplicates
in the CSP plugin where possible
Phase 3: Move Functions, Utils or Helpers, Hooks to Package
Phase 4: Misconfiguration Preview feature (with Cypress test and other
required test)

This is **Phase 2** of the Process

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
@opauloh opauloh closed this Aug 27, 2024
animehart added a commit that referenced this pull request Aug 28, 2024
…clude new package PHASE 3 (#191317)

The previous #190105 was way too
big and made it hard to review without missing any bugs or potential
bugs, Thus we decided we are going to make series of smaller PR to make
things more manageable

We will be splitting it into 4 PR
Phase 1: Creating empty packages for csp and csp-common
Phase 2: Move Types from CSP plugin to the Package + Deleting duplicates
in the CSP plugin where possible
Phase 3: Move Functions, Utils or Helpers, Hooks to Package
Phase 4: Misconfiguration Preview feature (with Cypress test and other
required test)

This is **Phase 3** of the Process,
This also includes moving rule versions type

This PR is the continuation of this PR
#190933

NOTE:
Merge phase 2 first before this

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
animehart added a commit that referenced this pull request Sep 4, 2024
…clude new package PHASE 4 (#191677)

The previous #190105 was way too
big and made it hard to review without missing any bugs or potential
bugs, Thus we decided we are going to make series of smaller PR to make
things more manageable

We will be splitting it into 4 PR
Phase 1: Creating empty packages for csp and csp-common
Phase 2: Move Types from CSP plugin to the Package + Deleting duplicates
in the CSP plugin where possible
Phase 3: Move Functions, Utils or Helpers, Hooks to Package
Phase 4: Misconfiguration Preview feature (with Cypress test and other
required test)

<img width="681" alt="353329193-5ad22c4e-81c2-4a8b-89f7-fdbc2a686c2d"
src="https://github.com/user-attachments/assets/b369625a-efc5-4292-a690-2c5dffb5483d">


This is Phase 4 of the Process,

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants