-
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] Versioning CSP rules type and api #172674
Conversation
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 🚀
I added bunch of nit's
Prefer to use import type
when possible
And I think of maybe rename CspRule
to CspBenchmarkRule
. To give more context to any kibana developer outside of our team. wdyt?
x-pack/plugins/cloud_security_posture/public/pages/rules/rules_container.test.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/cloud_security_posture/server/routes/rules/find/find_csp_rule.ts
Outdated
Show resolved
Hide resolved
request: { | ||
query: findCspRuleRequest, | ||
}, | ||
}, |
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: consider to add response validation on the next PR/open a tech debt issue
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.
x-pack/plugins/cloud_security_posture/server/routes/rules/find/v1.ts
Outdated
Show resolved
Hide resolved
x-pack/test/api_integration/apis/cloud_security_posture/find_csp_rule.ts
Outdated
Show resolved
Hide resolved
...test_serverless/api_integration/test_suites/security/cloud_security_posture/find_csp_rule.ts
Outdated
Show resolved
Hide resolved
export const getSortedCspRulesTemplates = (cspRulesTemplates: CspRule[]) => { | ||
return cspRulesTemplates.slice().sort((a, b) => { | ||
const ruleNumberA = a?.metadata?.benchmark?.rule_number; | ||
const ruleNumberB = b?.metadata?.benchmark?.rule_number; | ||
|
||
const versionA = semverValid(ruleNumberA); | ||
const versionB = semverValid(ruleNumberB); | ||
|
||
if (versionA !== null && versionB !== null) { | ||
return semverCompare(versionA, versionB); | ||
} else { | ||
return String(ruleNumberA).localeCompare(String(ruleNumberB)); | ||
} | ||
}); | ||
}; | ||
|
||
export const getBenchmarkIdFromPackagePolicyId = async ( | ||
soClient: SavedObjectsClientContract, | ||
packagePolicyId: string | ||
): Promise<string> => { | ||
const res = await soClient.get<NewPackagePolicy>( | ||
PACKAGE_POLICY_SAVED_OBJECT_TYPE, | ||
packagePolicyId | ||
); | ||
return getBenchmarkFromPackagePolicy(res.attributes.inputs); | ||
}; |
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.
move the a utility.ts
/common.ts
under the same folder to prevent circular dependencies
I agree, will do. |
95324e8
to
8cd05f0
Compare
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.
💯
added another couple of comments
@@ -7,20 +7,20 @@ | |||
import React, { useState, useMemo } from 'react'; | |||
import { EuiPanel, EuiSpacer } from '@elastic/eui'; | |||
import { useParams } from 'react-router-dom'; | |||
import { CspRuleTemplate } from '../../../common/schemas'; | |||
import { CspBenchmarkRule } from '../../../common/types/latest'; |
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.
use import type
export const getSortedCspBenchmarkRulesTemplates = (cspBenchmarkRules: CspBenchmarkRule[]) => { | ||
return cspBenchmarkRules.slice().sort((a, b) => { | ||
const ruleNumberA = a?.metadata?.benchmark?.rule_number; | ||
const ruleNumberB = b?.metadata?.benchmark?.rule_number; | ||
|
||
const versionA = semverValid(ruleNumberA); | ||
const versionB = semverValid(ruleNumberB); | ||
|
||
if (versionA !== null && versionB !== null) { | ||
return semverCompare(versionA, versionB); | ||
} else { | ||
return String(ruleNumberA).localeCompare(String(ruleNumberB)); | ||
} | ||
}); | ||
}; | ||
|
||
export const getBenchmarkIdFromPackagePolicyId = async ( | ||
soClient: SavedObjectsClientContract, | ||
packagePolicyId: string | ||
): Promise<string> => { | ||
const res = await soClient.get<NewPackagePolicy>( | ||
PACKAGE_POLICY_SAVED_OBJECT_TYPE, | ||
packagePolicyId | ||
); | ||
return getBenchmarkFromPackagePolicy(res.attributes.inputs); | ||
}; |
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.
move the a utility.ts/common.ts under the same folder to prevent circular dependencies
2cad88c
to
fcf536a
Compare
0b31a76
to
92a0e64
Compare
67aef70
to
d9d2723
Compare
fdd45a0
to
cbfab6d
Compare
06f697a
to
636d53a
Compare
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
FindCspBenchmarkRuleRequest, | ||
FindCspBenchmarkRuleResponse, | ||
findCspBenchmarkRuleRequestSchema, | ||
} from '../../../../common/types/latest'; |
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.
better to explicitly use v1 here instead
think of the use case of adding a newer version
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.
👍 will do as part of #173066
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.
import { PACKAGE_POLICY_SAVED_OBJECT_TYPE } from '../../benchmarks/benchmarks'; | ||
import { getBenchmarkFromPackagePolicy } from '../../../../common/utils/helpers'; | ||
|
||
import type { |
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 should explicitly use v1 here instead (FindCspBenchmarkRuleRequest
and FindCspBenchmarkRuleResponse
only. the SO should always be the latest)
think of the use case of adding a newer version
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.
👍 will do as part of #173066
Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security) |
Hey team, bunch of PRs were missing the team label. I added the labels in. Thanks! |
This PR includes:
csp_rule_template
tocsp_benchmark_rule
throughout the code.