[Heartbeat] Refactor TCP Monitor #17549

andrewvc · 2020-04-06T19:54:44Z

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:

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 Heartbeat TLS servername set incorrectly #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

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

elasticmachine · 2020-04-06T19:54:46Z

Pinging @elastic/uptime (:uptime)

Socks5 tests added

heartbeat/monitors/active/tcp/tcp.go

urso · 2020-04-14T10:39:10Z

Loads of complexity comes from the amount of jobs (permutations) created per monitor configuration. Settings contributing to this are mode: all, hosts, and ports. For 8.0 we might consider to deprecate some?

mode:all: Most complexity and dialer chain have been introduced thanks to this setting. If all is set a DNS is request is made to collect all known IPs for a hostname, and create a dialer and follow up task per IP. Initially this has been a hard requirement. Is this still the case?
hosts: we allow multiple hosts to be configured per monitor. hosts instead of host was introduced for consistency with other Beats. Users have only one config, at the cost of some more 'complexity' in code on the initial setup. I think that's ok and I would keep the setting.
ports: This is the one I regret the most. If one only wants to check if TCP connectivity is there, ports might be convenient for users. But once you introduce 'contents' checks to your configuration, you might prefer to test one port anyways. Maybe we can deprecate this in favor of port, and move the responsibility to the user/UI/configuration template to create multiple monitoring configs if this is required.

urso · 2020-04-14T10:45:00Z

heartbeat/monitors/active/tcp/tcp.go

+			if err != nil {
+				return nil, err
+			}
+			jobs = append(jobs, wrappers.WithURLField(url, endpointJob))


I wonder if this is a change in semantics introduced here or a while ago already. By creating a job per complete URL, we will have each Job do it's own DNS request. Originally only one job was created to do the initial DNS lookup, with follow up tasks per IP:Port combination. On Windows/MacOS DNS is cached automatically, but we have had incidents on Linux overloading DNS servers, due to missing caching.

I took a look at the code here, and I do think we can fix this so we restore the behavior before this patch. However, I agree with you that it's not a clear win for ports because that code is tricky. I think for mode: all it still makes sense to keep the behavior, because more people use that (and it provides benefits for icmp/tcp/http not just tcp).

However, for TCP, given that this is a very rarely used feature AFAICT I don't think the code complexity is worth maintaining the single DNS lookup. Additionally, there are workarounds in the rare case where DNS load is too high. One could always install a caching resolver on the same box as heartbeat in the worst case.

urso · 2020-04-14T10:58:26Z

heartbeat/monitors/resolver.go

+	mapping map[string][]net.IP
+}
+
+func CreateStaticResolver() staticResolver {


Do not return private types from an exported function/method. Why do you need CreateStaticResolver to be exported? If it is only for testing, how about having a special package with testing utilities, to make the intent more clear?

When introducing something like this resolver (so we don't have to modify the /etc/hosts file), then the resolver should become 'stackable', so we can fallback to another resolver if the hostname has not been configured.

If we use this for testing, how about accepting a map[string][]net.IP as parameter to CreateStaticResolver?

It doesn't need to be exported, just an oversight :)

I take it back, it does need to be exported because it's used by the tcp monitor package tests. I could move it there, but I think it makes more sense to keep it here since we'll eventually use it in tests for http/icmp.

Alternatively I could extract all this resolver stuff to its own package, but I'm indifferent on that matter.

If it's purpose is to be used for testing only, then we should at least have this reflected in the package name.

Well, StdResolver is used for production, StaticResolver is used in another package for test purposes. I could break it out to its own package, but that seems a bit excessive, since that package would have to export it.

We could put StaticResolver in a test file in the tcp package, but in the future we'll probably have tests that want to reuse it from the http and icmp monitors.

What do you think the best way forward is?

Can we think of other testing helpers? E.g. some helper that would drive a 'job' with it's followup tasks?

This has been moved to the tcp test package for now.

andrewvc · 2020-04-15T01:14:57Z

@urso thanks for the review. I'd rather not make too many changes in this PR re mode/ports/hosts . The original purpose here was fixing the TLS hostname bug, and we've already gone far past that point, and cleaned up the code substantially.

I agree that it's worth considering deprecating ports, but I'd ask that we have that discussion in a separate issue since we'd be waiting till 8.0 to remove the code.

mtojek

Only two nit-picks. Ship it!

mtojek · 2020-04-20T13:18:46Z

heartbeat/monitors/active/dialchain/net.go

@@ -45,7 +45,7 @@ import (
 //    }
 //  }
 func TCPDialer(to time.Duration) NetDialer {


nit: I see that the file net.go contains only dialers. Do you think we can rename it to dialers.go?
Also, there are some functions in util.go that refer to transport.Dialer, I wonder if we can divide the stuff in these two files into dialers.go and layers.go. We could get rid of utils ;)

Yep, good call.

mtojek · 2020-04-20T13:24:37Z

heartbeat/monitors/active/tcp/tcp_test.go

 	"strconv"
 	"testing"
-	"time"
+
+	"github.com/elastic/go-lookslike/validator"


nit: please sort imports

++, I reset my IDE and lost my auto-sorter.

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)

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: 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 #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 added bug in progress Pull request is currently in progress. Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team technical debt labels Apr 6, 2020

andrewvc self-assigned this Apr 6, 2020

andrewvc changed the title ~~[Heartbeat] Refactor tcp~~ [Heartbeat] Refactor TCP Monitor Apr 6, 2020

andrewvc removed the in progress Pull request is currently in progress. label Apr 6, 2020

andrewvc added 14 commits April 6, 2020 21:43

Move TCP dialchain builder into TCP package

3e6a95a

Move some TCP setup code into tcpMonitor struct

579e946

Move more setup code into object

1821b51

Rename ConnCheck to less confusing DataCheck

8fbe821

Factor out loadConfig in tcpMonitor

751d388

Initialize dialer late, use tcpMonitor for dialer construction

1f39e60

Late no more builder for plain TCP dials w/o SOCKS5

6fd2c13

TCP Dialer now layers host correctly

760ebb9

Cleanup weird code around scheme/endpoints

5da7f86

More cleanup + socks5 support is back

5b49fb8

Code cleanup

f292bb0

FMT

d7a2cf8

Cleanup byHostname jobs

5dc587e

Passing socks5 test

f14e864

Socks5 tests added

andrewvc force-pushed the refactor-tcp branch from 3190cc7 to f14e864 Compare April 7, 2020 02:44

More comments

8c44e6a

andresrc added [zube]: Inbox [zube]: In Review and removed [zube]: Inbox labels Apr 7, 2020

andrewvc added 2 commits April 7, 2020 16:19

Add TLS cert mismatch test + custom resolver

0d2bb51

Split up test files

1252f47

urso reviewed Apr 14, 2020

View reviewed changes

heartbeat/monitors/active/tcp/tcp.go Outdated Show resolved Hide resolved

urso reviewed Apr 14, 2020

View reviewed changes

andrewvc added 2 commits April 14, 2020 19:58

Incorporate PR feedback for resolvers

bef1e8c

Check server teardown errors and use defer

7ee6e3c

andrewvc added 10 commits April 14, 2020 20:21

Incorporate PR feedback

520618a

Incorporate PR feedback

9c2dac7

go fmt

0d3d86a

Merge remote-tracking branch 'origin/master' into refactor-tcp

c016696

Incorporate PR feedback

754100d

Fix FMT

1765973

Fix resolvers

376770d

Fix resolver ref

cd4ed05

Merge remote-tracking branch 'origin/master' into refactor-tcp

5eb9374

Merge remote-tracking branch 'origin/master' into refactor-tcp

07d091d

urso approved these changes Apr 20, 2020

View reviewed changes

mtojek approved these changes Apr 20, 2020

View reviewed changes

andrewvc added 2 commits April 20, 2020 11:00

Rename net.go for clarity, fix imports

7543cba

Merge remote-tracking branch 'origin/master' into refactor-tcp

eb7f476

andrewvc merged commit dfe8c4b into elastic:master Apr 20, 2020

andrewvc deleted the refactor-tcp branch April 20, 2020 21:17

zube bot added [zube]: Done and removed [zube]: In Review labels Apr 20, 2020

andrewvc mentioned this pull request Apr 20, 2020

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

Merged

4 tasks

andrewvc added the v7.8.0 label Apr 20, 2020

andrewvc removed the [zube]: Done label Apr 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Heartbeat] Refactor TCP Monitor #17549

[Heartbeat] Refactor TCP Monitor #17549

andrewvc commented Apr 6, 2020 •

edited

Loading

elasticmachine commented Apr 6, 2020

urso commented Apr 14, 2020

urso Apr 14, 2020

andrewvc Apr 15, 2020

urso Apr 14, 2020

andrewvc Apr 14, 2020

andrewvc Apr 15, 2020

andrewvc Apr 15, 2020

urso Apr 15, 2020

andrewvc Apr 15, 2020

urso Apr 16, 2020

andrewvc Apr 20, 2020

andrewvc commented Apr 15, 2020

mtojek left a comment

mtojek Apr 20, 2020

andrewvc Apr 20, 2020

mtojek Apr 20, 2020

andrewvc Apr 20, 2020

[Heartbeat] Refactor TCP Monitor #17549

[Heartbeat] Refactor TCP Monitor #17549

Conversation

andrewvc commented Apr 6, 2020 • edited Loading

What does this PR do?

Checklist

How to test this PR locally

elasticmachine commented Apr 6, 2020

urso commented Apr 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewvc commented Apr 15, 2020

mtojek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewvc commented Apr 6, 2020 •

edited

Loading