Skip to content

[SIEM] - Top countries by source/dest tables#48179

Merged
stephmilovic merged 16 commits intoelastic:masterfrom
stephmilovic:top-countries
Oct 16, 2019
Merged

[SIEM] - Top countries by source/dest tables#48179
stephmilovic merged 16 commits intoelastic:masterfrom
stephmilovic:top-countries

Conversation

@stephmilovic
Copy link
Contributor

@stephmilovic stephmilovic commented Oct 14, 2019

Summary

Resolves https://github.com/elastic/siem-team/issues/464

Implements Top Countries table on both Network and IP Details page. This was largely a copy/pasta of the TopNFlow table/query.

The table is in a tab on the Network page:
Screen Shot 2019-10-14 at 5 00 22 PM

And between IPs and Users on the IP Details page:
Screen Shot 2019-10-15 at 11 57 36 AM

A good link that has a lengthy response of countries on the IP details page: http://localhost:5601/app/siem#/network/ip/10.142.0.7?_g=()&timerange=(global:(linkTo:!(timeline),timerange:(from:1570949998180,kind:absolute,to:1571093998180)),timeline:(linkTo:!(global),timerange:(from:1570949998180,kind:absolute,to:1571093998180)))

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cwurm
Copy link
Contributor

cwurm commented Oct 15, 2019

On the IP Details page, we don't need Destination IPs on the Source Countries and Source IPs on Destination Countries - since we filter down to just one IP they'll always be 1.

I personally imagined the countries tables to be below the IP tables on the Network page as well - not in its own tab, but together in a Flows tab. Not sure what others thought? @tsg, @andrewkroh, @MichaelMarcialis?

@stephmilovic
Copy link
Contributor Author

On the IP Details page, we don't need Destination IPs on the Source Countries and Source IPs on Destination Countries - since we filter down to just one IP they'll always be 1.

@cwurm it is already filtered down, that was an old screenshot. please reference the updated screenshot ;)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

sortable: true,
render: flows => {
if (flows != null) {
return numeral(flows).format('0,000');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No change necessary since we're not doing it anywhere else, but I wonder if we should be using the format:number:defaultPattern Kibana advanced setting to format these numerals?

}>(({ countryCode, displayCountryNameOnHover = false }) => {
const [localesLoaded, setLocalesLoaded] = useState(false);
useEffect(() => {
if (isEmpty(countries.getNames('en'))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be using the format:number:defaultLocale Kibana advanced setting for all these countries.getNames() calls?

e.g.

console.log("US (Alpha-2) => " + countries.getName("US", "en")); // United States of America
console.log("US (Alpha-2) => " + countries.getName("US", "de")); // Vereinigte Staaten von Amerika

from the i18n-iso-countries docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We then need to load all languages. I think we talked about this @cwurm but can't remember why we ended up with english only??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... If that's the case, we could use typescript's dynamic import to resolve that issue pretty easily.

As for not previously using it -- perhaps it was because country names are often stored as their English counterpart in Elasticsearch directly? Doesn't look like we're using the localized country name in any DnD/AddToKql components, so that should be fine too?

Either way, just something to note 🙂

}

interface NetworkTopCountriesTableDispatchProps {
updateIpDetailsTableActivePage: ActionCreator<{
Copy link
Contributor

@XavierM XavierM Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think by creating one updateNetworksTableActivePage and adding the networkType in your action as an attribute will simplify some type issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got an issue for this: #48332
Along with the selectors: #48330

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked out, performed code review, and tested locally -- from pagination, DnD, column sorting, Network vs IP Details, timefilter/refresh updates, KQL changes, tab changes, etc...

Fantastic work here @stephmilovic -- I officially declare you Champion of the Tables! I've left a few nits and cleanups that might be good to take care of now while you have a chance, but nothing major holding up this PR (you may want to open an issue to track the column sorts being persistent between pages though).

Thanks for the efficient introduction of the new Countries tables! 🎉 🚀 🙂

L G T M
o o o e
o o !
k d
s

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stephmilovic stephmilovic merged commit de1f5d0 into elastic:master Oct 16, 2019
@stephmilovic stephmilovic deleted the top-countries branch October 16, 2019 03:51
spong pushed a commit to spong/kibana that referenced this pull request Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants