Skip to content

[SIEM] Source/Destination Ip Table on Ip Details#47608

Merged
stephmilovic merged 18 commits intoelastic:masterfrom
stephmilovic:source-dest-ip-details
Oct 14, 2019
Merged

[SIEM] Source/Destination Ip Table on Ip Details#47608
stephmilovic merged 18 commits intoelastic:masterfrom
stephmilovic:source-dest-ip-details

Conversation

@stephmilovic
Copy link
Contributor

@stephmilovic stephmilovic commented Oct 8, 2019

Summary

Resolves #44777

Implements source and destination IP table on the IP Details page, and removes the Domains table. Added an option argument to the NetworkTopNFlowQuery to filter by ip.

Screen Shot 2019-10-11 at 8 43 14 AM

Checklist

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

For maintainers

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

let tableType: networkModel.TopNTableType;
let headerTitle: string;

// eslint-disable-next-line @typescript-eslint/no-explicit-any
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if anyone can help figure out this type... wrestled with it for too long on my own

Copy link
Contributor

Choose a reason for hiding this comment

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

let updateTableActivePage: NetworkTopNFlowTableDispatchProps["updateIpDetailsTableActivePage"] | NetworkTopNFlowTableDispatchProps["updateNetworkPageTableActivePage"]; maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2019-10-11 at 8 00 36 AM

JK this fails too :(

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@willemdh
Copy link

willemdh commented Oct 11, 2019

@stephmilovic Imho, the autonomous system column, which is most of the time empty takes too much horizontal space.. Maybe rename to AS?

@cwurm
Copy link
Contributor

cwurm commented Oct 11, 2019

Sorry for looking at this so late. I think we can remove the last column (Source/Destination IPs). Since this is on the IP Details page, it will always be 1 (since we filter down to just one IP).

@spong spong self-requested a review October 11, 2019 14:18
@stephmilovic
Copy link
Contributor Author

@stephmilovic Imho, the autonomous system column, which is most of the time empty takes too much horizontal space.. Maybe rename to AS?

makes sense to me, but lets see what @cwurm thinks

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

activePage: number;
tableType: networkModel.IpDetailsTableType;
}>;
updateNetworkPageTableActivePage: ActionCreator<{
Copy link
Member

Choose a reason for hiding this comment

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

Instead of having separate actions for each page, consolidating to a single action that also takes NetworkType should simplify the logic and make it simpler to add this component to another page in the future. Should take care of the typing issue below as well! As above the Authentications table has a good example of this:

updateTableActivePage: ActionCreator<{
activePage: number;
hostsType: hostsModel.HostsType;
tableType: hostsModel.HostsTableType;
}>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

having type issues trying to implement this. the reason we dont have the same issue in hosts is because they display the same table, where as there is a difference between network and ip details

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spong and i paired on this a while today, thought we came up with a solution but it was a farce! Reconvening Monday

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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, tested locally, and performed code review.

Thanks for taking the time to clean up our typings and making it even easier to use the Source/Destination tables on new pages in the future LGTM! 👍

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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.

[SIEM] IP Details: Replace Domains table with Source/Destination IP tables

6 participants