Skip to content
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

Cherry-pick #17549 to 7.x: [Heartbeat] Refactor TCP Monitor #17849

Merged
merged 1 commit into from
Apr 21, 2020

Conversation

andrewvc
Copy link
Contributor

Cherry-pick of PR #17549 to 7.x branch. Original message:

What does this PR do?

Refactors the TCP monitor to make the code easier to follow, more testable, and fixes #17123 where TLS server name was not correctly sent. This is important because the code had accrued a lot of cruft and become very hard to follow. There were many wrappers and intermediate variable names that often subtly changed names as they crossed various functions. When debugging #17123 I frequently found myself lost tracing the execution.

This new code should be simpler to understand for a few reasons:

  1. Less code (almost a 2x reduction) and fewer, simpler, better organized functions
  2. Less variable passing/renaming due to use of struct for key config variables
  3. More consistent and descriptive variable names
  4. Creation of the dialer as late as possible, to remove the confusing partial states, and clarity as to when which dialer layers are used.
  5. Adds (frustratingly tricky) integration tests for Heartbeat TLS servername set incorrectly #17123 using mismatched TLS certs, and also against a real SOCKS5 proxy
  6. Adds, for testing only, the ability to override the real network resolver for TCP checks which is invaluable in debugging TLS checks that depend on setting hostnames correctly. In the future if we decide to let users use a custom DNS resolver this will be nice.
  7. Reorganized giant TCP test file into multiple files

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

Run the sample config in #17123

Refactors the TCP monitor to make the code easier to follow, more testable, and fixes elastic#17123 where TLS server name was not correctly sent. This is important because the code had accrued a lot of cruft and become very hard to follow. There were many wrappers and intermediate variable names that often subtly changed names as they crossed various functions. When debugging elastic#17123 I frequently found myself lost tracing the execution.

This new code should be simpler to understand for a few reasons:

    Less code (almost a 2x reduction) and fewer, simpler, better organized functions
    Less variable passing/renaming due to use of struct for key config variables
    More consistent and descriptive variable names
    Creation of the dialer as late as possible, to remove the confusing partial states, and clarity as to when which dialer layers are used.
    Adds (frustratingly tricky) integration tests for elastic#17123 using mismatched TLS certs, and also against a real SOCKS5 proxy
    Adds, for testing only, the ability to override the real network resolver for TCP checks which is invaluable in debugging TLS checks that depend on setting hostnames correctly. In the future if we decide to let users use a custom DNS resolver this will be nice.
    Reorganized giant TCP test file into multiple files

(cherry picked from commit dfe8c4b)
@andrewvc andrewvc requested a review from a team as a code owner April 20, 2020 22:19
@andrewvc andrewvc added backport review Team:obs-ds-hosted-services Label for the Observability Hosted Services team Heartbeat labels Apr 20, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/uptime (:uptime)

@andrewvc andrewvc merged commit b54f503 into elastic:7.x Apr 21, 2020
@andrewvc andrewvc deleted the backport_17549_7.x branch April 21, 2020 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Heartbeat review Team:obs-ds-hosted-services Label for the Observability Hosted Services team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants