-
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
[CTI] Threat Intel Card on Overview page needs to accommodate Fleet TI integrations #115940
Conversation
@elasticmachine merge upstream |
1 similar comment
@elasticmachine merge upstream |
Pinging @elastic/security-solution (Team: SecuritySolution) |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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.
Code looks great. I had some issues with the setup and wasn't able to obtain indicators, I will review again once I can locally test the feature.
...ugins/security_solution/public/overview/components/overview_cti_links/cti_enabled_module.tsx
Outdated
Show resolved
Hide resolved
...ck/plugins/security_solution/public/overview/components/overview_cti_links/cti_no_events.tsx
Outdated
Show resolved
Hide resolved
...ck/plugins/security_solution/public/overview/components/overview_cti_links/cti_no_events.tsx
Outdated
Show resolved
Hide resolved
import { useBasePath } from '../../../common/lib/kibana'; | ||
|
||
export const useIntegrationsPageLink = () => |
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 looks like this is not a hook, just a helper function. In that case we should probably rename it not to start with use
. We could call it getIntegrationsPageLink
or something along those lines. thanks for the modularization
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.
But it is just a combination with other hookuseBasePath
from x-pack/plugins/security_solution/public/common/lib/kibana/hooks.ts
And inside it uses useKibana
which relay on useContext
.
} | ||
|
||
export const useTIIntegrations = () => { | ||
const [TIIntegrationsStatus, setTIIntegrationsStatus] = useState<TIIntegrationStatus | null>( |
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.
not sure about TIIntegrationsStatus
, I believe we don't want to start with a capital letter in this context, @rylnd seemed to have some opinions on this, maybe we can get his opinion 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.
LGTM
@elasticmachine merge upstream |
3 similar comments
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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 we can remove the 7.16.0 tag here and should target 8.0 instead, right?
@@ -33,6 +38,7 @@ export const CtiWithEventsComponent = ({ | |||
isPluginDisabled={isPluginDisabled} | |||
listItems={listItems} | |||
totalCount={totalCount} | |||
isSomeIntegrationsDisabled={isSomeIntegrationsDisabled} |
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.
Maybe at this level the prop should be closer to its semantic use:
isSomeIntegrationsDisabled={isSomeIntegrationsDisabled} | |
showIntegrationsCTA={isSomeIntegrationsDisabled} |
09459b1
to
e889d24
Compare
@elasticmachine merge upstream |
@elasticmachine merge upstream |
838a107
to
554873a
Compare
@elasticmachine merge upstream |
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.
Code review only. LGTM.
*/ | ||
|
||
import { buildTiDataSourceQuery } from './query.threat_intel_source.dsl'; | ||
import { mockOptions, expectedDsl } from './__mocks__'; |
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.
These __mocks__
files are a relic of jest auto-mocking that we aren't using, here. I would just name that file index.mock.ts
instead (or e.g. query.threat_intel_source.dsl.mock.ts
if that's more accurate)
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 move the mock value, exactly to the tests
x-pack/plugins/security_solution/public/overview/containers/overview_cti_links/api.ts
Outdated
Show resolved
Hide resolved
const datasets = result?.rawResponse?.aggregations?.dataset?.buckets ?? []; | ||
const getChildAggregationValue = (aggregation?: Bucket) => aggregation?.buckets?.[0]?.key; | ||
|
||
const integrationMap = datasets.reduce((acc: Record<string, TiDataSources>, dataset) => { |
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 assume this is going to be a relatively small number? If not, we might need to consider not creating new objects on each loop 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.
Yes, it will be about how much data source clients have, not a big number
dashboardId?: string; | ||
} | ||
interface TiDataSourcesProps extends Partial<GlobalTimeArgs> { | ||
allTiDataSources?: TiDataSources[]; |
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 this prop can be made non-optional, no?
allTiDataSources?: TiDataSources[]; | |
allTiDataSources: TiDataSources[]; |
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.
Not really, we have a case when don't pass it, because we don't have it yet
x-pack/plugins/security_solution/public/overview/containers/overview_cti_links/use_all_ti_data_sources.ts
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
References to deprecated APIs
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.
Checked just the Cypress updates, LGTM
@nkhristinin I apologize, I had to revert this PR as it was causing CI to fail. It must have been incompatible with another change that was merged since the last time you merged upstream. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
…TI integrations (elastic#115940) * Add support integrations * Fix types * fix unit tests * Fix tests and types * fix eslint * fix file case * add cy tests * Revert test * Add tests * Add support of installed integrations * Fix types * Add isntalled ingtegration case for cypress tests * Fix cypress tests * Fix comments * Fix capital naming * Fix again capital naming * Add dynamic dashboard for a new integrations packages * intermidiate changes, to keep it remote * Big refactoring * Tests and refactoring * Remove unused constanrs * Fix e2e tests * PR comments fix * fix ts * Fix translations * Remove stubs * Rename isSomeIntegrationsDisabled -> allIntegrationsInstalled * Add buildQuery tests * Fix type * Add tests for Enable Source button * Remove copied file * Move api call to api.ts * Rename fetchFleetIntegrations * Remove __mocks__ Co-authored-by: Kibana Machine <[email protected]>
…e Fleet TI integrations (elastic#115940)" This reverts commit 6640357.
Summary
Instruction on how to install fleet and elastic-agent locally: https://docs.google.com/document/d/17QQTVi3Rk_Sh0BctNKCujucV3PApZvpbTDtWUvE8AX8/edit?usp=sharing
Video demo
Screen.Recording.2021-11-22.at.18.36.43.mov
Changes
integrations
anddata source
Integrations - it's something related to fleet packages, which you can install. For example AbuseCH.
Data source - it's related to
event.dataset
field. For example, one integration AbuseCH has 3 data sources - AbuseCH URL, AbuseCH Malware, AbuseCH Malware.Added new searchStrategy, which looks for threat data sources with aggregation by
threat.feed.name
andthreat.feed.dashboard_id
Initially we get all available data sources. We use this list, to be able later to show 0 when the data source doesn't have any events in the time range.
We fetch all TI integration packages from fleet API. We show a warning if there are some not installed integrations.
Removed some components and hooks which are not used.
How to test:
At that moment, the fleet integrations don't have
threat.feed.name
, but filebeat-8 has it.Fleet integrations
threat.feed.name
Filebeat
logs-ti_*, filebeat-8*
You should see the filebeat data source on Overview card.
Checklist
Delete any items that are not applicable to this PR.