-
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
Compliance dashboard UI and API #171312
Compliance dashboard UI and API #171312
Conversation
Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security) |
@Omolola-Akinleye Is the PR ready for review? the build seems to be failing |
c3934a9
to
a2d884d
Compare
@maxcold PR is now ready for review sorry for the hold up. |
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 started with the CR, but couldn't finish it in one go, will continue tomorrow
@@ -0,0 +1,18 @@ | |||
/* |
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 some basic unit tests for these utils
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.
also maybe some comments on why we need these mapping utils, it's was a bit hard to figure it out just from looking at the code
export const MAPPING_VERSION_DELIMITER = '_'; | ||
|
||
export const toBenchmarkDocFieldKey = (benchmarkId: string, benchmarkVersion: string) => { | ||
if (benchmarkVersion.includes(MAPPING_VERSION_DELIMITER)) |
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: seems like the linter is ok with it but I'm personally not a big fan of if
without {}
, it's confusing to read and prone to errors. I'd either have it like
if (benchmarkVersion.includes(MAPPING_VERSION_DELIMITER)) {
return `${benchmarkId};${benchmarkVersion.replaceAll('_', '.')}`;
}
return `${benchmarkId};${benchmarkVersion}`;
or have a ternary in the return
…ance-dashboard-data-api
…inleye/kibana into compliance-dashboard-data-api
getStats(logger, esClient, query, pitId, runtimeMappings), | ||
getGroupedFindingsEvaluation(logger, esClient, query, pitId, runtimeMappings), | ||
getClusters(logger, esClient, query, pitId, runtimeMappings), | ||
getTrends(logger, esClient, policyTemplate), |
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: logger usually passes as the last argument in our plugin.
if (!Array.isArray(version.aggs_by_benchmark_name.buckets)) | ||
throw new Error('missing aggs by benchmark name'); | ||
|
||
if (version.aggs_by_benchmark_name && version.aggs_by_benchmark_name.buckets.length > 0) { | ||
benchmarkName = version.aggs_by_benchmark_name.buckets[0].key; | ||
} |
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 check if there are missing value at once
if (!Array.isArray(version.aggs_by_benchmark_name.buckets)) | |
throw new Error('missing aggs by benchmark name'); | |
if (version.aggs_by_benchmark_name && version.aggs_by_benchmark_name.buckets.length > 0) { | |
benchmarkName = version.aggs_by_benchmark_name.buckets[0].key; | |
} | |
if (!Array.isArray(version.aggs_by_benchmark_name.buckets)) | |
throw new Error('missing aggs by benchmark name'); | |
if (version.aggs_by_benchmark_name && version.aggs_by_benchmark_name.buckets.length > 0) { | |
benchmarkName = version.aggs_by_benchmark_name.buckets[0].key; | |
} | |
if (!Array.isArray(version.aggs_by_benchmark_name.buckets)) | |
throw new Error('missing aggs by benchmark name'); | |
if (version.aggs_by_benchmark_name && version.aggs_by_benchmark_name.buckets.length > 0) { | |
benchmarkName = version.aggs_by_benchmark_name.buckets[0].key; | |
} | |
if (!Array.isArray(resourcesTypesAggs)) | |
throw new Error('missing aggs by resource type per benchmark'); |
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.
@CohenIdo I'm not sure what you are asking here. Do you mind elaborating?
const resourcesTypesAggs = version.aggs_by_resource_type.buckets; | ||
if (!Array.isArray(resourcesTypesAggs)) | ||
throw new Error('missing aggs by resource type per benchmark'); | ||
const groupedFindingsEvaluation = getFailedFindingsFromAggs(resourcesTypesAggs); |
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.
getFailedFindingsFromAggs
is confusing name for a function that returns
{
name: bucket.key,
totalFindings: bucket.doc_count,
totalFailed,
totalPassed,
postureScore: calculatePostureScore(totalPassed, totalFailed),
}
I know it's not part of the PR, but is a good chance to rename it for more meaningful name.
}; | ||
aggs_by_benchmark_name: Aggregation<KeyDocCount>; | ||
} | ||
export interface FailedFindingsBucket extends KeyDocCount { |
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.
Could you please rename the interface name? The bucket not only includes failed findings.
score_by_benchmark_id: Record< | ||
string, | ||
Record< | ||
string, | ||
{ | ||
total_findings: number; | ||
passed_findings: number; | ||
failed_findings: number; | ||
} | ||
> | ||
>; |
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'm suggesting to refactor the interface in order to avoid code duplication and using ovject instead of Record
:
interface FindingsDetails {
total_findings: number;
passed_findings: number;
failed_findings: number;
}
interface ScoreByClusterId {
[clusterId: string]: FindingsDetails;
}
interface ScoreByBenchmarkId {
[benchmarkId: string]: {
[key: string]: FindingsDetails;
};
}
export interface ScoreTrendDoc {
'@timestamp': string;
total_findings: number;
passed_findings: number;
failed_findings: number;
score_by_cluster_id: ScoreByClusterId;
score_by_benchmark_id: ScoreByBenchmarkId;
}
// large number that should be sufficient for 24 hours considering we write to the score index every 5 minutes | ||
size: 999, | ||
// Amount of samples of the last 24 hours (accounting that we take a sample every 5 minutes) | ||
size: (24 * 60) / 5, |
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 use please the identical parameter currently utilized in the task manager configuration to stay synchronized with any future modifications. It might necessitate confirming that the calculation result is an integer by rounding the outcome.
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.
Hey @CohenIdo, this is a score trends aggregation query which will return the size of the number of trend documents. I don't see how I can apply the 5m interval here.
…inleye/kibana into compliance-dashboard-data-api
💚 Build Succeeded
Metrics [docs]Async chunks
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.
LGTM, just some nits that can be addressed in the future
// large number that should be sufficient for 24 hours considering we write to the score index every 5 minutes | ||
size: 999, | ||
// Amount of samples of the last 24 hours (accounting that we take a sample every 5 minutes) | ||
size: (24 * 60) / CSPM_FINDINGS_STATS_INTERVAL, |
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 improvement 🚀
|
||
const CSPM_FINDINGS_STATS_TASK_ID = 'cloud_security_posture-findings_stats'; | ||
const CSPM_FINDINGS_STATS_TASK_TYPE = 'cloud_security_posture-stats_task'; | ||
const CSPM_FINDINGS_STATS_INTERVAL = '5m'; | ||
export const CSPM_FINDINGS_STATS_INTERVAL = 5; |
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: move that to the common/constants.ts
file
</> | ||
); | ||
|
||
const NonCompactPercentageLabels = ({ |
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: this could be called PercentageLabels instead of NonCompactPercentageLabels
## Summary Summarize your PR. If it involves visual changes include a screenshot or gif. Address nit comments from [Compliance Dashboard UI/API PR](#171312)
Summary
Summarize your PR. If it involves visual changes include a screenshot or gif.
score_by_benchmark_id
to the Benchmark Scores Index this will show posture stats for each benchmark idTo test PR with API versioning, in Kibana client -
x-pack/plugins/cloud_security_posture/public/common/api/use_stats_api.ts