-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Cloud Security]Misconfiguration preview & Refactor CSP Plugin to include new package #190105
Conversation
/ci |
/ci |
…m:animehart/kibana into misconfiguration-preview-refactor-package
/ci |
/ci |
/ci |
2 similar comments
/ci |
/ci |
/ci |
/ci |
/ci |
Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security) |
/ci |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
"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", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ?
const queryHostName = { | ||
bool: { | ||
must: [], | ||
filter: [ | ||
{ | ||
bool: { | ||
should: [{ term: { 'host.name': { value: `${hostName}` } } }], | ||
minimum_should_match: 1, | ||
}, | ||
}, | ||
], | ||
should: [], | ||
must_not: [], | ||
}, | ||
}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neat:
// Commented this out until we have the expanded flyout | |
// Todo: Uncomment once we have the expanded flyout |
<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> |
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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; |
There was a problem hiding this comment.
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>) => { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 Nice additional context here!
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this 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:
- Continue in this PR but with all the comments it will be huge, hard to review and easy to miss bugs
- 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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({ |
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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({ |
There was a problem hiding this comment.
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(() => { |
There was a problem hiding this comment.
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 /> |
There was a problem hiding this comment.
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 }) => { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
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 |
…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 <[email protected]>
* 2.0. | ||
*/ | ||
|
||
// Add stuff here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops
// Add stuff here |
* 2.0. | ||
*/ | ||
|
||
// Add stuff here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops
// Add stuff here |
<EuiFlexItem> | ||
<EuiFlexGroup direction="column" gutterSize="none"> | ||
<EuiFlexItem> | ||
<EuiText |
There was a problem hiding this comment.
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}; |
There was a problem hiding this comment.
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(() => { |
There was a problem hiding this comment.
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
.
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.
There was a problem hiding this comment.
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 !
…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 <[email protected]>
…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 <[email protected]>
…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 <[email protected]>
Summary
This PR includes the following
Note:
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.