From 3f5e20452e01a5374e897701337f5c862183837f Mon Sep 17 00:00:00 2001 From: Kyle Havlovitz Date: Mon, 25 Mar 2019 16:58:41 -0700 Subject: [PATCH 1/2] http: use the correct check duration fields when converting txn ops --- agent/txn_endpoint.go | 7 ++- agent/txn_endpoint_test.go | 124 +++++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 3 deletions(-) diff --git a/agent/txn_endpoint.go b/agent/txn_endpoint.go index b471c8a954..664a34fd6f 100644 --- a/agent/txn_endpoint.go +++ b/agent/txn_endpoint.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "strings" + "time" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" @@ -239,9 +240,9 @@ func (s *HTTPServer) convertOps(resp http.ResponseWriter, req *http.Request) (st Header: check.Definition.Header, Method: check.Definition.Method, TCP: check.Definition.TCP, - Interval: check.Definition.IntervalDuration, - Timeout: check.Definition.TimeoutDuration, - DeregisterCriticalServiceAfter: check.Definition.DeregisterCriticalServiceAfterDuration, + Interval: time.Duration(check.Definition.Interval), + Timeout: time.Duration(check.Definition.Timeout), + DeregisterCriticalServiceAfter: time.Duration(check.Definition.DeregisterCriticalServiceAfter), }, RaftIndex: structs.RaftIndex{ ModifyIndex: check.ModifyIndex, diff --git a/agent/txn_endpoint_test.go b/agent/txn_endpoint_test.go index a9bacd8597..2667f798be 100644 --- a/agent/txn_endpoint_test.go +++ b/agent/txn_endpoint_test.go @@ -8,8 +8,11 @@ import ( "reflect" "strings" "testing" + "time" + "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/testrpc" + "github.com/pascaldekloe/goe/verify" "github.com/hashicorp/consul/agent/structs" ) @@ -400,3 +403,124 @@ func TestTxnEndpoint_KV_Actions(t *testing.T) { } }) } + +func TestTxnEndpoint_UpdateCheck(t *testing.T) { + t.Parallel() + a := NewTestAgent(t, t.Name(), "") + defer a.Shutdown() + testrpc.WaitForTestAgent(t, a.RPC, "dc1") + + // Make sure the fields of a check are handled correctly when both creating and updating. + buf := bytes.NewBuffer([]byte(fmt.Sprintf(` +[ + { + "Check": { + "Verb": "set", + "Check": { + "Node": "%s", + "CheckID": "nodecheck", + "Name": "Node http check", + "Status": "critical", + "Notes": "Http based health check", + "Output": "", + "ServiceID": "", + "ServiceName": "", + "Definition": { + "Interval": "6s", + "Timeout": "6s", + "DeregisterCriticalServiceAfter": "6s", + "HTTP": "http://localhost:8000", + "TLSSkipVerify": true + } + } + } + }, + { + "Check": { + "Verb": "set", + "Check": { + "Node": "%s", + "CheckID": "nodecheck", + "Name": "Node http check", + "Status": "passing", + "Notes": "Http based health check", + "Output": "success", + "ServiceID": "", + "ServiceName": "", + "Definition": { + "Interval": "10s", + "Timeout": "10s", + "DeregisterCriticalServiceAfter": "15m", + "HTTP": "http://localhost:9000", + "TLSSkipVerify": false + } + } + } + } +] +`, a.config.NodeName, a.config.NodeName))) + req, _ := http.NewRequest("PUT", "/v1/txn", buf) + resp := httptest.NewRecorder() + obj, err := a.srv.Txn(resp, req) + if err != nil { + t.Fatalf("err: %v", err) + } + if resp.Code != 200 { + t.Fatalf("expected 200, got %d", resp.Code) + } + + txnResp, ok := obj.(structs.TxnResponse) + if !ok { + t.Fatalf("bad type: %T", obj) + } + if len(txnResp.Results) != 2 { + t.Fatalf("bad: %v", txnResp) + } + index := txnResp.Results[0].Check.ModifyIndex + expected := structs.TxnResponse{ + Results: structs.TxnResults{ + &structs.TxnResult{ + Check: &structs.HealthCheck{ + Node: a.config.NodeName, + CheckID: "nodecheck", + Name: "Node http check", + Status: api.HealthCritical, + Notes: "Http based health check", + Definition: structs.HealthCheckDefinition{ + Interval: 6 * time.Second, + Timeout: 6 * time.Second, + DeregisterCriticalServiceAfter: 6 * time.Second, + HTTP: "http://localhost:8000", + TLSSkipVerify: true, + }, + RaftIndex: structs.RaftIndex{ + CreateIndex: index, + ModifyIndex: index, + }, + }, + }, + &structs.TxnResult{ + Check: &structs.HealthCheck{ + Node: a.config.NodeName, + CheckID: "nodecheck", + Name: "Node http check", + Status: api.HealthPassing, + Notes: "Http based health check", + Output: "success", + Definition: structs.HealthCheckDefinition{ + Interval: 10 * time.Second, + Timeout: 10 * time.Second, + DeregisterCriticalServiceAfter: 15 * time.Minute, + HTTP: "http://localhost:9000", + TLSSkipVerify: false, + }, + RaftIndex: structs.RaftIndex{ + CreateIndex: index, + ModifyIndex: index, + }, + }, + }, + }, + } + verify.Values(t, "", txnResp, expected) +} From 716a20d8a68b01a4842b323a2ac72948f08cc3f6 Mon Sep 17 00:00:00 2001 From: Kyle Havlovitz Date: Tue, 26 Mar 2019 10:44:02 -0700 Subject: [PATCH 2/2] Re-add logic to handle the undocumented duration fields --- agent/txn_endpoint.go | 34 +++++++++++++++----- agent/txn_endpoint_test.go | 66 +++++++++++++++++++++++++++++++------- 2 files changed, 81 insertions(+), 19 deletions(-) diff --git a/agent/txn_endpoint.go b/agent/txn_endpoint.go index 664a34fd6f..eaad624065 100644 --- a/agent/txn_endpoint.go +++ b/agent/txn_endpoint.go @@ -221,6 +221,24 @@ func (s *HTTPServer) convertOps(resp http.ResponseWriter, req *http.Request) (st } check := in.Check.Check + + // Check if the internal duration fields are set as well as the normal ones. This is + // to be backwards compatible with a bug where the internal duration fields were being + // deserialized from instead of the correct fields. + // See https://github.com/hashicorp/consul/issues/5477 for more details. + interval := check.Definition.IntervalDuration + if dur := time.Duration(check.Definition.Interval); dur != 0 { + interval = dur + } + timeout := check.Definition.TimeoutDuration + if dur := time.Duration(check.Definition.Timeout); dur != 0 { + timeout = dur + } + deregisterCriticalServiceAfter := check.Definition.DeregisterCriticalServiceAfterDuration + if dur := time.Duration(check.Definition.DeregisterCriticalServiceAfter); dur != 0 { + deregisterCriticalServiceAfter = dur + } + out := &structs.TxnOp{ Check: &structs.TxnCheckOp{ Verb: in.Check.Verb, @@ -235,14 +253,14 @@ func (s *HTTPServer) convertOps(resp http.ResponseWriter, req *http.Request) (st ServiceName: check.ServiceName, ServiceTags: check.ServiceTags, Definition: structs.HealthCheckDefinition{ - HTTP: check.Definition.HTTP, - TLSSkipVerify: check.Definition.TLSSkipVerify, - Header: check.Definition.Header, - Method: check.Definition.Method, - TCP: check.Definition.TCP, - Interval: time.Duration(check.Definition.Interval), - Timeout: time.Duration(check.Definition.Timeout), - DeregisterCriticalServiceAfter: time.Duration(check.Definition.DeregisterCriticalServiceAfter), + HTTP: check.Definition.HTTP, + TLSSkipVerify: check.Definition.TLSSkipVerify, + Header: check.Definition.Header, + Method: check.Definition.Method, + TCP: check.Definition.TCP, + Interval: interval, + Timeout: timeout, + DeregisterCriticalServiceAfter: deregisterCriticalServiceAfter, }, RaftIndex: structs.RaftIndex{ ModifyIndex: check.ModifyIndex, diff --git a/agent/txn_endpoint_test.go b/agent/txn_endpoint_test.go index 2667f798be..7a84bb04f7 100644 --- a/agent/txn_endpoint_test.go +++ b/agent/txn_endpoint_test.go @@ -410,7 +410,8 @@ func TestTxnEndpoint_UpdateCheck(t *testing.T) { defer a.Shutdown() testrpc.WaitForTestAgent(t, a.RPC, "dc1") - // Make sure the fields of a check are handled correctly when both creating and updating. + // Make sure the fields of a check are handled correctly when both creating and + // updating, and test both sets of duration fields to ensure backwards compatibility. buf := bytes.NewBuffer([]byte(fmt.Sprintf(` [ { @@ -456,9 +457,31 @@ func TestTxnEndpoint_UpdateCheck(t *testing.T) { } } } + }, + { + "Check": { + "Verb": "set", + "Check": { + "Node": "%s", + "CheckID": "nodecheck", + "Name": "Node http check", + "Status": "passing", + "Notes": "Http based health check", + "Output": "success", + "ServiceID": "", + "ServiceName": "", + "Definition": { + "IntervalDuration": "15s", + "TimeoutDuration": "15s", + "DeregisterCriticalServiceAfterDuration": "30m", + "HTTP": "http://localhost:9000", + "TLSSkipVerify": false + } + } + } } ] -`, a.config.NodeName, a.config.NodeName))) +`, a.config.NodeName, a.config.NodeName, a.config.NodeName))) req, _ := http.NewRequest("PUT", "/v1/txn", buf) resp := httptest.NewRecorder() obj, err := a.srv.Txn(resp, req) @@ -473,7 +496,7 @@ func TestTxnEndpoint_UpdateCheck(t *testing.T) { if !ok { t.Fatalf("bad type: %T", obj) } - if len(txnResp.Results) != 2 { + if len(txnResp.Results) != 3 { t.Fatalf("bad: %v", txnResp) } index := txnResp.Results[0].Check.ModifyIndex @@ -487,11 +510,11 @@ func TestTxnEndpoint_UpdateCheck(t *testing.T) { Status: api.HealthCritical, Notes: "Http based health check", Definition: structs.HealthCheckDefinition{ - Interval: 6 * time.Second, - Timeout: 6 * time.Second, + Interval: 6 * time.Second, + Timeout: 6 * time.Second, DeregisterCriticalServiceAfter: 6 * time.Second, - HTTP: "http://localhost:8000", - TLSSkipVerify: true, + HTTP: "http://localhost:8000", + TLSSkipVerify: true, }, RaftIndex: structs.RaftIndex{ CreateIndex: index, @@ -508,11 +531,32 @@ func TestTxnEndpoint_UpdateCheck(t *testing.T) { Notes: "Http based health check", Output: "success", Definition: structs.HealthCheckDefinition{ - Interval: 10 * time.Second, - Timeout: 10 * time.Second, + Interval: 10 * time.Second, + Timeout: 10 * time.Second, DeregisterCriticalServiceAfter: 15 * time.Minute, - HTTP: "http://localhost:9000", - TLSSkipVerify: false, + HTTP: "http://localhost:9000", + TLSSkipVerify: false, + }, + RaftIndex: structs.RaftIndex{ + CreateIndex: index, + ModifyIndex: index, + }, + }, + }, + &structs.TxnResult{ + Check: &structs.HealthCheck{ + Node: a.config.NodeName, + CheckID: "nodecheck", + Name: "Node http check", + Status: api.HealthPassing, + Notes: "Http based health check", + Output: "success", + Definition: structs.HealthCheckDefinition{ + Interval: 15 * time.Second, + Timeout: 15 * time.Second, + DeregisterCriticalServiceAfter: 30 * time.Minute, + HTTP: "http://localhost:9000", + TLSSkipVerify: false, }, RaftIndex: structs.RaftIndex{ CreateIndex: index,