Merge pull request #5553 from hashicorp/txn-check-serialization

Use the correct check duration fields when converting transaction ops

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"
@@ -220,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,
@@ -234,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,
+							HTTP:          check.Definition.HTTP,
-							TLSSkipVerify:                  check.Definition.TLSSkipVerify,
+							TLSSkipVerify: check.Definition.TLSSkipVerify,
-							Header:                         check.Definition.Header,
+							Header:        check.Definition.Header,
-							Method:                         check.Definition.Method,
+							Method:        check.Definition.Method,
-							TCP:                            check.Definition.TCP,
+							TCP:           check.Definition.TCP,
-							Interval:                       check.Definition.IntervalDuration,
+							Interval:      interval,
-							Timeout:                        check.Definition.TimeoutDuration,
+							Timeout:       timeout,
-							DeregisterCriticalServiceAfter: check.Definition.DeregisterCriticalServiceAfterDuration,
+							DeregisterCriticalServiceAfter: deregisterCriticalServiceAfter,
 						},
 						RaftIndex: structs.RaftIndex{
 							ModifyIndex: check.ModifyIndex,

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,168 @@ 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, and test both sets of duration fields to ensure backwards compatibility.
+	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
+				}
+			}
+		}
+	},
+	{
+		"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)))
+	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) != 3 {
+		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,
+					},
+				},
+			},
+			&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,
+						ModifyIndex: index,
+					},
+				},
+			},
+		},
+	}
+	verify.Values(t, "", txnResp, expected)
+}