Skip to content

[SIEM] Add TLS to Network overview page#48062

Merged
angorayc merged 22 commits intoelastic:masterfrom
angorayc:newtowrk-tls
Oct 16, 2019
Merged

[SIEM] Add TLS to Network overview page#48062
angorayc merged 22 commits intoelastic:masterfrom
angorayc:newtowrk-tls

Conversation

@angorayc
Copy link
Contributor

@angorayc angorayc commented Oct 13, 2019

Summary

Isolating the existing TLS table from ip details page and share it with network overview page.
#47581

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 Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stephmilovic
Copy link
Contributor

Right away, I think that you incorrectly resolved some conflicts. The Domains table was deleted from the repo in this PR #47608

Please ensure references to Domains are all removed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stephmilovic
Copy link
Contributor

stephmilovic commented Oct 15, 2019

You need to change tlsSelector in x-pack/legacy/plugins/siem/public/store/network/selectors.ts to take a networkType argument to determine which tls table state you are tracking, network page or network details. The activePage is getting set to 0 on every page load (i just introduced this but think i need another condition should only happen on ip details), but you can still see that the sort and limit are in sync from details to network when they should have independent state. Follow how this is done in topNFlowSelector

@stephmilovic
Copy link
Contributor

Right now, the TlsTable in x-pack/legacy/plugins/siem/public/components/page/network/tls_table/index.tsx only calls an update to networkActions.updateIpDetailsTableActivePage. It should also be able to call networkActions.updateNetworkPageTableActivePage when updating the TLS table on the network page. Add a check like in the network_top_n_flow_table to determine which updateTable you should call

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

...state,
[NetworkType.details]: {
...state[NetworkType.details],
[networkType]: {
Copy link
Contributor

Choose a reason for hiding this comment

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

I got type errors and had to do the specific check like you see in updateTopNFlowLimit. have you run a type check here, trying to run it locally and its blowing up

);
};

export const tlsSelector = (networkType: NetworkType) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

export interface TlsRequestOptions extends RequestOptionsPaginated {
ip?: string;
tlsSortField: TlsSortField;
flowTarget: FlowTarget;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be FlowTargetSourceDest?

endDate={to}
filterQuery={filterQuery}
flowTarget={flowTarget}
flowTarget={FlowTargetSourceDest.source}
Copy link
Contributor

Choose a reason for hiding this comment

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

this does not need to be dynamic?

Copy link
Contributor

@stephmilovic stephmilovic left a comment

Choose a reason for hiding this comment

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

Performed code review, ran locally, tested manually for quite a while between the different pages, activePage, limit, and sort. Everything is running smoothly. LGTM 🚀

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@angorayc
Copy link
Contributor Author

jenkins, retest

@angorayc
Copy link
Contributor Author

jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@angorayc angorayc merged commit cb091e8 into elastic:master Oct 16, 2019
angorayc added a commit to angorayc/kibana that referenced this pull request Oct 16, 2019
* add TLS table to network overview page

* isolate TLS parser

* add unit test

* add integration test

* fix types

* revert not necessary change

* remove variables for domains table

* fix for review

* fix tlsSelector

* update tls selector

* apply updateTlsLimit

* update selected property for tls selector

* add networkType as the 2nd param of updateTlsSort

* correcting pagetype

* check the page type for updateTableActivePage

* hard coded the targeting table name

* remove tls table param as property

* fix types
angorayc added a commit that referenced this pull request Oct 16, 2019
* add TLS table to network overview page

* isolate TLS parser

* add unit test

* add integration test

* fix types

* revert not necessary change

* remove variables for domains table

* fix for review

* fix tlsSelector

* update tls selector

* apply updateTlsLimit

* update selected property for tls selector

* add networkType as the 2nd param of updateTlsSort

* correcting pagetype

* check the page type for updateTableActivePage

* hard coded the targeting table name

* remove tls table param as property

* fix types
@MarkSettleES MarkSettleES changed the title [SIEM] Add TLS to Newtowrk overview page [SIEM] Add TLS to Newtwork overview page Oct 29, 2019
@MarkSettleES MarkSettleES changed the title [SIEM] Add TLS to Newtwork overview page [SIEM] Add TLS to Network overview page Oct 29, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

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.

3 participants