-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Removing license gate keeping and displaying the table when there are findings #190285
Conversation
…led but there are findings
/ci |
/ci |
Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security) |
@@ -537,6 +540,125 @@ const IntegrationSettings = ({ onChange, fields }: IntegrationInfoFieldsProps) = | |||
</div> | |||
); | |||
|
|||
const useEnsureDefaultNamespace = ({ |
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 to line 661 did not change, i just moved it to the top because this file is hard enough to read, having utility functions both at the top of the file and at its bottom was making it really confusing
@@ -12,9 +12,10 @@ import { useKibana } from './use_kibana'; | |||
|
|||
const SUBSCRIPTION_QUERY_KEY = 'csp_subscription_query_key'; | |||
|
|||
export const useSubscriptionStatus = () => { |
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: if we rename the only hook, should we also rename the file? not like we strictly follow one hool per file policy, so just a nit
|
||
export const Configurations = () => { | ||
const location = useLocation(); | ||
const dataViewQuery = useDataView(CDR_MISCONFIGURATIONS_DATA_VIEW_ID_PREFIX); | ||
const { data: getSetupStatus, isLoading: getSetupStatusIsLoading } = useCspSetupStatusApi(); | ||
const getLatestFindings = useLatestFindings({ sort: [['asc']], enabled: true, pageSize: 1 }); |
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.
what about making this a part of the /status
API? I think we will need this check in many places, one I know about is the Contextual Flyout for the empty. I think it's better to have a well-defined API for that rather than doing it in every place again and again
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.
plus there is a check for muted/unmuted findings integrated in the useLatestFindings
and I'm not sure if we want it here. If I'm not missing smth, if there are findings but all muted, this page would show the "install CSPM integration" block, while the integration is installed. Is this correct?
For me, this is another pro of having this as a part of the status API. there we can encapsulate pure statuses, without the risk of the logic being affected by the changes in the useLatestFindings
hook
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 was having the same contemplation regarding the muted rules. One thing to keep in mind is that we only support rule muting for native findings. Meaning CDR rules will never have filtered out findings, at least on paper.
Meaning that using the current useLatestFindings is basically keeping the same logic that we always had without changing any flow, which is the same as you mentioned, all native findings muted -> integration installation prompt. Ideally we should have some "all findings are muted" prompt or something, but we don't have that yet.
I was also having the same contemplation regarding the status API tbh, ended up with what you see cause I figured I want to use exactly what the table is using so there's no chance of the API saying "have findings" while the table is like "nope". or vise versa somehow. I can't think of something that can cause that right now, but it might come up in the future and it's just an additional thing to keep in mind. I'm ok with changing it to use an API, lmk if you still think its better after i explained my reasoning
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 am in favor of defining a clear status API and then combining them in the react components according to the use case. If we need to reuse some logic between the data grid and the page itself, we can always come up with an abstraction for it. Right now there is an implicit coupling between different pages, plus there are side effects like muting logic which might not be desirable, even if for now we are ok with it
{ | ||
range: { | ||
'@timestamp': { | ||
gte: `now-${LATEST_FINDINGS_RETENTION_POLICY}`, |
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.
should we also include the retention policy as a function parameter? so the function will be truly generic to other use cases such as vulnerabilities.
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.
just a comment, LGTM overall!
const hasConfigurationFindings = | ||
hasFindings || | ||
const hasFindings = | ||
hasMisconfigurationsFindings || | ||
getSetupStatus?.kspm.status === 'indexed' || |
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: I think getSetupStatus?.kspm.status === 'indexed' || getSetupStatus?.cspm.status === 'indexed'
check is not needed anymore, I can't think of a scenario when there are no documents in the CDR index pattern, but either kspm or cspm is indexed
@@ -171,6 +208,7 @@ export const getCspStatus = async ({ | |||
installedPackagePoliciesVulnMgmt, | |||
installedPolicyTemplates, | |||
] = await Promise.all([ | |||
checkIndexHasFindings(esClient, CDR_MISCONFIGURATIONS_INDEX_PATTERN, logger), |
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 add an API integration test for the new piece of information.
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.
Looks good, also tested on a security serverless project.
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @JordanSh |
Summary
Findings are displayed without CSP integration installed
Screen.Recording.2024-08-12.at.15.54.18.mov
License check moved to integration page
Screen.Recording.2024-08-12.at.15.56.19.mov
Skips license block on serverless: