Allow `hosts` to be used to configure http monitors #13703

vjsamuel · 2019-09-16T18:45:28Z

Currently only the http monitor allows configuring endpoints to hit via urls. Everyone else relies on hosts similar to metricbeat. This PR ensures that we also allow configuration with hosts. Eventually, the urls field can be deprecated.

elasticmachine · 2019-09-16T18:45:30Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2019-09-16T18:45:30Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

heartbeat/monitors/active/http/config.go

andrewvc · 2019-09-16T19:07:19Z

I'm a +1 on this, but we'll need to patch heartbeat to support host-like syntax, where hostname:port would be treated as either http|https depending on whether ssl.enabled are present. The urls field requires the protocol.

It creates some additional questions. Do we now support paths in the hosts field? Is hosts: ["foo:123/my/path"] a valid path? Do hosts and urls share the same config?

I think the simplest answer to these questions is to change the behavior as follows:

Use the same parsing logic for urls and hosts
For HTTP hosts will first attempt to parse the input as an URL, if it passes it will use it. If it does not it will attempt to prefix the string with http:// or https:// based on whether ssl.enabled is present, then attempt to parse that as a URL.

Thoughts @vjsamuel ?

andrewvc

Looking pretty good! I think it needs just a few tweaks and a handful more tests however.

andrewvc · 2019-09-16T19:41:40Z

heartbeat/monitors/active/http/config.go

+
+	for i := 0; i < len(c.Hosts); i ++ {
+		host := c.Hosts[i]
+		if _, err := url.ParseRequestURI(host); err != nil {


This could use a comment like:

For consistency we allow users to enter both URLs and host:port pairs in the hosts field, since internally HTTP checks must operate on hosts. Rather than parsing these using separate parsers, we simply see if the host is a valid URL, and if not, see if it becomes valid if http(s) is added in front of it.

andrewvc · 2019-09-16T19:42:52Z

heartbeat/monitors/active/http/config.go

+		c.Hosts = append(c.Hosts, c.URLs...)
+	}
+
+	for i := 0; i < len(c.Hosts); i ++ {


We should break this out to a separate function, something like normalizeHostURL(str string) and add unit tests for it.

andrewvc · 2019-09-16T19:43:26Z

heartbeat/monitors/active/http/http_test.go

@@ -55,7 +55,7 @@ func testRequest(t *testing.T, testURL string) *beat.Event {
 // an empty string no cert will be set.
 func testTLSRequest(t *testing.T, testURL string, extraConfig map[string]interface{}) *beat.Event {
 	configSrc := map[string]interface{}{
-		"urls":    testURL,
+		"hosts":   testURL,


We still need to test the behavior of the url parameter, so we should add new test permutations here handling that.

elasticmachine · 2019-09-16T20:56:19Z

Pinging @elastic/uptime

houndci-bot · 2019-09-16T21:51:54Z

heartbeat/monitors/active/http/config.go

+	updateScheme := func(host string) string {
+		if c.TLS != nil && *c.TLS.Enabled == true {
+			return fmt.Sprint("https://", host)
+		} else {


if block ends with a return statement, so drop this else and outdent its block

houndci-bot · 2019-09-16T21:51:54Z

heartbeat/monitors/active/http/config.go

+	updateScheme := func(host string) string {
+		if c.TLS != nil && *c.TLS.Enabled == true {
+			return fmt.Sprint("https://", host)
+		} else {


if block ends with a return statement, so drop this else and outdent its block

houndci-bot · 2019-09-16T21:51:55Z

heartbeat/monitors/active/http/config.go

+	updateScheme := func(host string) string {
+		if c.TLS != nil && *c.TLS.Enabled == true {
+			return fmt.Sprint("https://", host)
+		} else {


if block ends with a return statement, so drop this else and outdent its block

andrewvc

Much improved! Just needs one final integration test as noted.

Great work here @vjsamuel , it's much appreciated!

heartbeat/tests/system/test_monitor.py

andrewvc

LGTM WFG

Currently only the http monitor allows configuring endpoints to hit via urls. Everyone else relies on hosts similar to metricbeat. This PR ensures that we also allow configuration with hosts. Eventually, the urls field can be deprecated. (cherry picked from commit 717771f)

vjsamuel requested a review from a team as a code owner September 16, 2019 18:45

houndci-bot reviewed Sep 16, 2019

View reviewed changes

heartbeat/monitors/active/http/config.go Show resolved Hide resolved

houndci-bot reviewed Sep 16, 2019

View reviewed changes

heartbeat/monitors/active/http/config.go Show resolved Hide resolved

vjsamuel force-pushed the make_urls_hosts_http branch from 1b07533 to 55cee9c Compare September 16, 2019 18:47

andrewvc requested changes Sep 16, 2019

View reviewed changes

andrewvc added Team:obs-ds-hosted-services Label for the Observability Hosted Services team enhancement Heartbeat labels Sep 16, 2019

houndci-bot reviewed Sep 16, 2019

View reviewed changes

vjsamuel force-pushed the make_urls_hosts_http branch 2 times, most recently from 7650b72 to 3d4f2e4 Compare September 16, 2019 23:39

andrewvc requested changes Sep 17, 2019

View reviewed changes

heartbeat/tests/system/test_monitor.py Outdated Show resolved Hide resolved

vjsamuel force-pushed the make_urls_hosts_http branch from 3d4f2e4 to 7d98946 Compare September 17, 2019 16:56

andrewvc approved these changes Sep 17, 2019

View reviewed changes

vjsamuel force-pushed the make_urls_hosts_http branch from 7d98946 to 9309149 Compare September 17, 2019 17:04

vjsamuel added 4 commits September 17, 2019 10:06

Allow to be used to configure http monitors

6c57515

Add documentation changes

50d71b9

Incorporate review comments

12626a0

Update test cases

3cbef32

vjsamuel force-pushed the make_urls_hosts_http branch from 9309149 to 3cbef32 Compare September 17, 2019 17:29

andrewvc merged commit 717771f into elastic:master Sep 17, 2019

andrewvc added the v7.5.0 label Sep 17, 2019

andrewvc self-assigned this Sep 17, 2019

vjsamuel deleted the make_urls_hosts_http branch September 17, 2019 21:15

andrewvc mentioned this pull request Oct 17, 2019

Fix misleading message when protocol is not set #12304

Closed

andrewvc mentioned this pull request Feb 24, 2020

HTTP hosts should be urls #15010

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow `hosts` to be used to configure http monitors #13703

Allow `hosts` to be used to configure http monitors #13703

vjsamuel commented Sep 16, 2019

elasticmachine commented Sep 16, 2019

elasticmachine commented Sep 16, 2019

andrewvc commented Sep 16, 2019 •

edited

andrewvc left a comment

andrewvc Sep 16, 2019

andrewvc Sep 16, 2019

andrewvc Sep 16, 2019

elasticmachine commented Sep 16, 2019

houndci-bot Sep 16, 2019

houndci-bot Sep 16, 2019

houndci-bot Sep 16, 2019

andrewvc left a comment

andrewvc left a comment

Allow hosts to be used to configure http monitors #13703

Allow hosts to be used to configure http monitors #13703

Conversation

vjsamuel commented Sep 16, 2019

elasticmachine commented Sep 16, 2019

elasticmachine commented Sep 16, 2019

andrewvc commented Sep 16, 2019 • edited

andrewvc left a comment

Choose a reason for hiding this comment

andrewvc Sep 16, 2019

Choose a reason for hiding this comment

andrewvc Sep 16, 2019

Choose a reason for hiding this comment

andrewvc Sep 16, 2019

Choose a reason for hiding this comment

elasticmachine commented Sep 16, 2019

houndci-bot Sep 16, 2019

Choose a reason for hiding this comment

houndci-bot Sep 16, 2019

Choose a reason for hiding this comment

houndci-bot Sep 16, 2019

Choose a reason for hiding this comment

andrewvc left a comment

Choose a reason for hiding this comment

andrewvc left a comment

Choose a reason for hiding this comment

Allow `hosts` to be used to configure http monitors #13703

Allow `hosts` to be used to configure http monitors #13703

andrewvc commented Sep 16, 2019 •

edited