From 3b7f82e040abf302e9af5a83010f91ea1f4654c0 Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Mon, 1 Aug 2022 15:21:32 -0500 Subject: [PATCH] nsd: add support for specifying check.method in nomad service checks Unblock 'check.method' in service validation. Add tests around making sure this value gets plumbed through. --- .../serviceregistration/checks/client_test.go | 106 +++++++++++++++++- helper/funcs.go | 19 ++++ helper/funcs_test.go | 28 +++++ nomad/structs/services.go | 10 +- nomad/structs/services_test.go | 47 +++++++- 5 files changed, 204 insertions(+), 6 deletions(-) diff --git a/client/serviceregistration/checks/client_test.go b/client/serviceregistration/checks/client_test.go index 2f6cb7fb4..9f0221889 100644 --- a/client/serviceregistration/checks/client_test.go +++ b/client/serviceregistration/checks/client_test.go @@ -12,6 +12,7 @@ import ( "time" "github.com/hashicorp/nomad/ci" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/freeport" "github.com/hashicorp/nomad/helper/testlog" "github.com/hashicorp/nomad/nomad/mock" @@ -20,6 +21,13 @@ import ( "oss.indeed.com/go/libtime/libtimetest" ) +func splitURL(u string) (string, string) { + // get the address and port for http server + tokens := strings.Split(u, ":") + addr, port := strings.TrimPrefix(tokens[1], "//"), tokens[2] + return addr, port +} + func TestChecker_Do_HTTP(t *testing.T) { ci.Parallel(t) @@ -49,8 +57,7 @@ func TestChecker_Do_HTTP(t *testing.T) { defer ts.Close() // get the address and port for http server - tokens := strings.Split(ts.URL, ":") - addr, port := strings.TrimPrefix(tokens[1], "//"), tokens[2] + addr, port := splitURL(ts.URL) // create a mock clock so we can assert time is set now := time.Date(2022, 1, 2, 3, 4, 5, 6, time.UTC) @@ -200,6 +207,101 @@ func bigResponse() (string, string) { return s, s[:outputSizeLimit] } +func TestChecker_Do_HTTP_extras(t *testing.T) { + ci.Parallel(t) + + // record the method, body, and headers of the request + var ( + method string + body []byte + headers map[string][]string + ) + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + method = r.Method + body, _ = io.ReadAll(r.Body) + headers = helper.CopyMap(r.Header) + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + + // get the address and port for http server + addr, port := splitURL(ts.URL) + + // make headers from key-value pairs + makeHeaders := func(more ...[2]string) http.Header { + h := make(http.Header) + for _, extra := range more { + h.Set(extra[0], extra[1]) + } + return h + } + + encoding := [2]string{"Accept-Encoding", "gzip"} + agent := [2]string{"User-Agent", "Go-http-client/1.1"} + + cases := []struct { + name string + method string + body string + headers map[string][]string + }{ + { + name: "method GET", + method: "GET", + headers: makeHeaders(encoding, agent), + }, + { + name: "method Get", + method: "Get", + headers: makeHeaders(encoding, agent), + }, + { + name: "method HEAD", + method: "HEAD", + headers: makeHeaders(agent), + }, + } + + for _, tc := range cases { + qc := &QueryContext{ + ID: "abc123", + CustomAddress: addr, + ServicePortLabel: port, + Networks: nil, + NetworkStatus: mock.NewNetworkStatus(addr), + Ports: nil, + Group: "group", + Task: "task", + Service: "service", + Check: "check", + } + + q := &Query{ + Mode: structs.Healthiness, + Type: "http", + Timeout: 1 * time.Second, + AddressMode: "auto", + PortLabel: port, + Protocol: "http", + Path: "/", + Method: tc.method, + } + + logger := testlog.HCLogger(t) + c := New(logger) + ctx := context.Background() + result := c.Do(ctx, qc, q) + must.Eq(t, http.StatusOK, result.StatusCode, + must.Sprintf("test.URL: %s", ts.URL), + must.Sprintf("headers: %v", tc.headers), + ) + must.Eq(t, tc.method, method) + must.Eq(t, tc.body, string(body)) + must.Eq(t, tc.headers, headers) + } +} + func TestChecker_Do_TCP(t *testing.T) { ci.Parallel(t) diff --git a/helper/funcs.go b/helper/funcs.go index d379a142d..ec3842f4c 100644 --- a/helper/funcs.go +++ b/helper/funcs.go @@ -3,6 +3,7 @@ package helper import ( "crypto/sha512" "fmt" + "net/http" "path/filepath" "reflect" "regexp" @@ -716,3 +717,21 @@ func NewSafeTimer(duration time.Duration) (*time.Timer, StopFunc) { return t, cancel } + +// IsMethodHTTP returns whether s is a known HTTP method, ignoring case. +func IsMethodHTTP(s string) bool { + switch strings.ToUpper(s) { + case http.MethodGet: + case http.MethodHead: + case http.MethodPost: + case http.MethodPut: + case http.MethodPatch: + case http.MethodDelete: + case http.MethodConnect: + case http.MethodOptions: + case http.MethodTrace: + default: + return false + } + return true +} diff --git a/helper/funcs_test.go b/helper/funcs_test.go index 685430210..79fd90026 100644 --- a/helper/funcs_test.go +++ b/helper/funcs_test.go @@ -546,3 +546,31 @@ func Test_NewSafeTimer(t *testing.T) { <-timer.C }) } + +func Test_IsMethodHTTP(t *testing.T) { + t.Run("is method", func(t *testing.T) { + cases := []string{ + "GET", "Get", "get", + "HEAD", "Head", "head", + "POST", "Post", "post", + "PUT", "Put", "put", + "PATCH", "Patch", "patch", + "DELETE", "Delete", "delete", + "CONNECT", "Connect", "connect", + "OPTIONS", "Options", "options", + "TRACE", "Trace", "trace", + } + for _, tc := range cases { + result := IsMethodHTTP(tc) + must.True(t, result) + } + }) + + t.Run("is not method", func(t *testing.T) { + not := []string{"GETTER", "!GET", ""} + for _, tc := range not { + result := IsMethodHTTP(tc) + must.False(t, result) + } + }) +} diff --git a/nomad/structs/services.go b/nomad/structs/services.go index 757c3185d..5d06ef9e1 100644 --- a/nomad/structs/services.go +++ b/nomad/structs/services.go @@ -200,6 +200,7 @@ func (sc *ServiceCheck) Canonicalize(serviceName string) { sc.Args = nil } + // Ensure empty slices are nil if len(sc.Header) == 0 { sc.Header = nil } else { @@ -210,10 +211,12 @@ func (sc *ServiceCheck) Canonicalize(serviceName string) { } } + // Ensure a default name for the check if sc.Name == "" { sc.Name = fmt.Sprintf("service: %q check", serviceName) } + // Ensure OnUpdate defaults to require_healthy (i.e. healthiness check) if sc.OnUpdate == "" { sc.OnUpdate = OnUpdateRequireHealthy } @@ -344,15 +347,16 @@ func (sc *ServiceCheck) validateNomad() error { } if sc.Type == "http" { - if sc.Method != "" && sc.Method != "GET" { - // unset turns into GET - return fmt.Errorf("http checks may only use GET method in Nomad services") + if sc.Method != "" && !helper.IsMethodHTTP(sc.Method) { + return fmt.Errorf("method type %q not supported in Nomad http check", sc.Method) } + // todo(shoenig) support headers if len(sc.Header) > 0 { return fmt.Errorf("http checks may not set headers in Nomad services") } + // todo(shoenig) support body if len(sc.Body) > 0 { return fmt.Errorf("http checks may not set Body in Nomad services") } diff --git a/nomad/structs/services_test.go b/nomad/structs/services_test.go index b44c9baef..3833f5c2b 100644 --- a/nomad/structs/services_test.go +++ b/nomad/structs/services_test.go @@ -57,6 +57,41 @@ func TestServiceCheck_Hash(t *testing.T) { }) } +func TestServiceCheck_Canonicalize(t *testing.T) { + ci.Parallel(t) + + t.Run("defaults", func(t *testing.T) { + sc := &ServiceCheck{ + Args: []string{}, + Header: make(map[string][]string), + Method: "", + OnUpdate: "", + } + sc.Canonicalize("MyService") + must.Nil(t, sc.Args) + must.Nil(t, sc.Header) + must.Eq(t, `service: "MyService" check`, sc.Name) + must.Eq(t, "", sc.Method) + must.Eq(t, OnUpdateRequireHealthy, sc.OnUpdate) + }) + + t.Run("check name set", func(t *testing.T) { + sc := &ServiceCheck{ + Name: "Some Check", + } + sc.Canonicalize("MyService") + must.Eq(t, "Some Check", sc.Name) + }) + + t.Run("on_update is set", func(t *testing.T) { + sc := &ServiceCheck{ + OnUpdate: OnUpdateIgnore, + } + sc.Canonicalize("MyService") + must.Eq(t, OnUpdateIgnore, sc.OnUpdate) + }) +} + func TestServiceCheck_validate_PassingTypes(t *testing.T) { ci.Parallel(t) @@ -268,7 +303,17 @@ func TestServiceCheck_validateNomad(t *testing.T) { Path: "/health", Method: "HEAD", }, - exp: `http checks may only use GET method in Nomad services`, + }, + { + name: "http unknown method type", + sc: &ServiceCheck{ + Type: ServiceCheckHTTP, + Interval: 3 * time.Second, + Timeout: 1 * time.Second, + Path: "/health", + Method: "Invalid", + }, + exp: `method type "Invalid" not supported in Nomad http check`, }, { name: "http with headers",