From acedbe0633f799afd59ab70e8f9dd8fed9f74345 Mon Sep 17 00:00:00 2001 From: Daniel Nelson Date: Mon, 26 Aug 2019 16:29:45 -0700 Subject: [PATCH] Promote the use of http as the scheme over tcp in health output (#6311) --- plugins/outputs/health/README.md | 13 ++-- plugins/outputs/health/health.go | 103 ++++++++++++++++---------- plugins/outputs/health/health_test.go | 82 +++++++++++++++++++- 3 files changed, 151 insertions(+), 47 deletions(-) diff --git a/plugins/outputs/health/README.md b/plugins/outputs/health/README.md index 5ef30fd57..0a56d5192 100644 --- a/plugins/outputs/health/README.md +++ b/plugins/outputs/health/README.md @@ -11,9 +11,9 @@ must fail in order for the resource to enter the failed state. ```toml [[outputs.health]] ## Address and port to listen on. - ## ex: service_address = "tcp://localhost:8080" + ## ex: service_address = "http://localhost:8080" ## service_address = "unix:///var/run/telegraf-health.sock" - # service_address = "tcp://:8080" + # service_address = "http://:8080" ## The maximum duration for reading the entire request. # read_timeout = "5s" @@ -51,11 +51,14 @@ must fail in order for the resource to enter the failed state. #### compares The `compares` check is used to assert basic mathematical relationships. Use -it by choosing a field key and one or more comparisons. All comparisons must -be true on all metrics for the check to pass. If the field is not found on a -metric no comparison will be made. +it by choosing a field key and one or more comparisons that must hold true. If +the field is not found on a metric no comparison will be made. + +Comparisons must be hold true on all metrics for the check to pass. #### contains The `contains` check can be used to require a field key to exist on at least one metric. + +If the field is found on any metric the check passes. diff --git a/plugins/outputs/health/health.go b/plugins/outputs/health/health.go index c7c2cc547..a6db63183 100644 --- a/plugins/outputs/health/health.go +++ b/plugins/outputs/health/health.go @@ -3,6 +3,7 @@ package health import ( "context" "crypto/tls" + "errors" "log" "net" "net/http" @@ -24,9 +25,9 @@ const ( var sampleConfig = ` ## Address and port to listen on. - ## ex: service_address = "tcp://localhost:8080" + ## ex: service_address = "http://localhost:8080" ## service_address = "unix:///var/run/telegraf-health.sock" - # service_address = "tcp://:8080" + # service_address = "http://:8080" ## The maximum duration for reading the entire request. # read_timeout = "5s" @@ -78,9 +79,12 @@ type Health struct { Contains []*Contains `toml:"contains"` checkers []Checker - wg sync.WaitGroup - server *http.Server - origin string + wg sync.WaitGroup + server *http.Server + origin string + network string + address string + tlsConf *tls.Config mu sync.Mutex healthy bool @@ -94,8 +98,31 @@ func (h *Health) Description() string { return "Configurable HTTP health check resource based on metrics" } -// Connect starts the HTTP server. -func (h *Health) Connect() error { +func (h *Health) Init() error { + u, err := url.Parse(h.ServiceAddress) + if err != nil { + return err + } + + switch u.Scheme { + case "http", "https": + h.network = "tcp" + h.address = u.Host + case "unix": + h.network = u.Scheme + h.address = u.Path + case "tcp4", "tcp6", "tcp": + h.network = u.Scheme + h.address = u.Host + default: + return errors.New("service_address contains invalid scheme") + } + + h.tlsConf, err = h.ServerConfig.TLSConfig() + if err != nil { + return err + } + h.checkers = make([]Checker, 0) for i := range h.Compares { h.checkers = append(h.checkers, h.Compares[i]) @@ -104,11 +131,11 @@ func (h *Health) Connect() error { h.checkers = append(h.checkers, h.Contains[i]) } - tlsConf, err := h.ServerConfig.TLSConfig() - if err != nil { - return err - } + return nil +} +// Connect starts the HTTP server. +func (h *Health) Connect() error { authHandler := internal.AuthHandler(h.BasicUsername, h.BasicPassword, onAuthError) h.server = &http.Server{ @@ -116,15 +143,15 @@ func (h *Health) Connect() error { Handler: authHandler(h), ReadTimeout: h.ReadTimeout.Duration, WriteTimeout: h.WriteTimeout.Duration, - TLSConfig: tlsConf, + TLSConfig: h.tlsConf, } - listener, err := h.listen(tlsConf) + listener, err := h.listen() if err != nil { return err } - h.origin = h.getOrigin(listener, tlsConf) + h.origin = h.getOrigin(listener) log.Printf("I! [outputs.health] Listening on %s", h.origin) @@ -145,25 +172,12 @@ func onAuthError(rw http.ResponseWriter, code int) { http.Error(rw, http.StatusText(code), code) } -func (h *Health) listen(tlsConf *tls.Config) (net.Listener, error) { - u, err := url.Parse(h.ServiceAddress) - if err != nil { - return nil, err - } - - network := "tcp" - address := u.Host - if u.Host == "" { - network = "unix" - address = u.Path - } - - if tlsConf != nil { - return tls.Listen(network, address, tlsConf) +func (h *Health) listen() (net.Listener, error) { + if h.tlsConf != nil { + return tls.Listen(h.network, h.address, h.tlsConf) } else { - return net.Listen(network, address) + return net.Listen(h.network, h.address) } - } func (h *Health) ServeHTTP(rw http.ResponseWriter, req *http.Request) { @@ -205,23 +219,30 @@ func (h *Health) Origin() string { return h.origin } -func (h *Health) getOrigin(listener net.Listener, tlsConf *tls.Config) string { - switch listener.Addr().Network() { - case "tcp": - scheme := "http" - if tlsConf != nil { - scheme = "https" +func (h *Health) getOrigin(listener net.Listener) string { + scheme := "http" + if h.tlsConf != nil { + scheme = "https" + } + if h.network == "unix" { + scheme = "unix" + } + + switch h.network { + case "unix": + origin := &url.URL{ + Scheme: scheme, + Path: listener.Addr().String(), } + return origin.String() + default: origin := &url.URL{ Scheme: scheme, Host: listener.Addr().String(), } return origin.String() - case "unix": - return listener.Addr().String() - default: - return "" } + } func (h *Health) setHealthy(healthy bool) { diff --git a/plugins/outputs/health/health_test.go b/plugins/outputs/health/health_test.go index 234b0251c..5bf35ad83 100644 --- a/plugins/outputs/health/health_test.go +++ b/plugins/outputs/health/health_test.go @@ -12,6 +12,8 @@ import ( "github.com/stretchr/testify/require" ) +var pki = testutil.NewPKI("../../../testutil/pki") + func TestHealth(t *testing.T) { type Options struct { Compares []*health.Compares `toml:"compares"` @@ -105,7 +107,11 @@ func TestHealth(t *testing.T) { output.Compares = tt.options.Compares output.Contains = tt.options.Contains - err := output.Connect() + err := output.Init() + require.NoError(t, err) + + err = output.Connect() + require.NoError(t, err) err = output.Write(tt.metrics) require.NoError(t, err) @@ -122,3 +128,77 @@ func TestHealth(t *testing.T) { }) } } + +func TestInitServiceAddress(t *testing.T) { + tests := []struct { + name string + plugin *health.Health + err bool + origin string + }{ + { + name: "port without scheme is not allowed", + plugin: &health.Health{ + ServiceAddress: ":8080", + }, + err: true, + }, + { + name: "path without scheme is not allowed", + plugin: &health.Health{ + ServiceAddress: "/tmp/telegraf", + }, + err: true, + }, + { + name: "tcp with port maps to http", + plugin: &health.Health{ + ServiceAddress: "tcp://:8080", + }, + }, + { + name: "tcp with tlsconf maps to https", + plugin: &health.Health{ + ServiceAddress: "tcp://:8080", + ServerConfig: *pki.TLSServerConfig(), + }, + }, + { + name: "tcp4 is allowed", + plugin: &health.Health{ + ServiceAddress: "tcp4://:8080", + }, + }, + { + name: "tcp6 is allowed", + plugin: &health.Health{ + ServiceAddress: "tcp6://:8080", + }, + }, + { + name: "http scheme", + plugin: &health.Health{ + ServiceAddress: "http://:8080", + }, + }, + { + name: "https scheme", + plugin: &health.Health{ + ServiceAddress: "https://:8080", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + output := health.NewHealth() + output.ServiceAddress = tt.plugin.ServiceAddress + + err := output.Init() + if tt.err { + require.Error(t, err) + return + } + require.NoError(t, err) + }) + } +}