Allow null settings in ES when using SetClusterSetting

es.go

@@ -761,37 +761,53 @@ func (c *Client) SetAllocation(allocation string) (string, error) {
 	return allocationVal.String(), nil
 }
 
-//Set a new value for a cluster setting
+// Set a new value for a cluster setting. Returns existing value and new value as well as error, in that order
+// If the setting is not set in Elasticsearch (it's falling back to default configuration) SetClusterSetting's existingValue will be nil.
+// If the value provided is nil, SetClusterSetting will remove the setting so that Elasticsearch falls back on default configuration for that setting.
 //
 //Use case: You've doubled the number of nodes in your cluster and you want to increase the number of shards the cluster can relocate at one time. Calling `SetClusterSetting("cluster.routing.allocation.cluster_concurrent_rebalance", "100")` will update that value with the cluster. Once data relocation is complete you can decrease the setting by calling `SetClusterSetting("cluster.routing.allocation.cluster_concurrent_rebalance", "20")`.
-func (c *Client) SetClusterSetting(setting string, value string) (string, string, error) {
-
+func (c *Client) SetClusterSetting(setting string, value *string) (*string, *string, error) {
+	var existingValue *string
+	var newValue *string
 	settingsBody, err := handleErrWithBytes(c.buildGetRequest(clusterSettingsPath))
 
 	if err != nil {
-		return "", "", err
+		return existingValue, newValue, err
 	}
 
-	existingValues := gjson.GetManyBytes(settingsBody, fmt.Sprintf("transient.%s", setting), fmt.Sprintf("persistent.%s", setting))
+	existingResults := gjson.GetManyBytes(settingsBody, fmt.Sprintf("transient.%s", setting), fmt.Sprintf("persistent.%s", setting))
+
+	var newSettingBody string
+
+	if value == nil {
+		newSettingBody = fmt.Sprintf(`{"transient" : { "%s" : null}}`, setting)
+	} else {
+		newSettingBody = fmt.Sprintf(`{"transient" : { "%s" : "%s"}}`, setting, *value)
+	}
 
 	agent := c.buildPutRequest(clusterSettingsPath).
 		Set("Content-Type", "application/json").
-		Send(fmt.Sprintf(`{"transient" : { "%s" : "%s"}}`, setting, value))
+		Send(newSettingBody)
 
 	body, err := handleErrWithBytes(agent)
 
 	if err != nil {
-		return "", "", err
+		return existingValue, newValue, err
 	}
 
-	newValue := gjson.GetBytes(body, fmt.Sprintf("transient.%s", setting)).String()
-
-	var existingValue string
+	newResults := gjson.GetBytes(body, fmt.Sprintf("transient.%s", setting)).String()
+	if newResults != "" {
+		newValue = &newResults
+	}
 
-	if existingValues[0].String() == "" {
-		existingValue = existingValues[1].String()
+	if existingResults[0].String() == "" {
+		if existingResults[1].String() != "" {
+			value := existingResults[1].String()
+			existingValue = &value
+		}
 	} else {
-		existingValue = existingValues[0].String()
+		value := existingResults[0].String()
+		existingValue = &value
 	}
 
 	return existingValue, newValue, nil

es_test.go

@@ -28,6 +28,10 @@ func buildTestServer(t *testing.T, setups []*ServerSetup, tls bool) *httptest.Se
 
 		matched := false
 		for _, setup := range setups {
+			// Extra piece of debug incase there's a typo in your test's response, like a rogue space somewhere
+			if r.Method == setup.Method && r.URL.EscapedPath() == setup.Path && requestBody != setup.Body {
+				t.Errorf("request body not matching: %s != %s", requestBody, setup.Body)
+			}
 			if r.Method == setup.Method && r.URL.EscapedPath() == setup.Path && requestBody == setup.Body {
 				matched = true
 				if setup.HTTPStatus == 0 {
@@ -77,6 +81,8 @@ func setupTestTLSServers(t *testing.T, setups []*ServerSetup) (string, int, *htt
 	return url.Hostname(), port, ts
 }
 
+func stringToPointer(v string) *string { return &v }
+
 func TestGetClusterExcludeSettings(t *testing.T) {
 
 	testSetup := &ServerSetup{
@@ -680,9 +686,9 @@ func TestSetClusterSettings(t *testing.T) {
 		GetResponse string
 		PutResponse string
 		Setting     string
-		SetValue    string
+		SetValue    *string
 		HTTPStatus  int
-		OldValue    string
+		OldValue    *string
 	}{
 		{
 			// Tests for behavior with existing transient setting.
@@ -691,8 +697,8 @@ func TestSetClusterSettings(t *testing.T) {
 			Body:        `{"transient":{"cluster.routing.allocation.exclude._name":"10.0.0.99"}}`,
 			PutResponse: `{"persistent":{},"transient":{"cluster":{"routing":{"allocation":{"exclude":{"_name":"10.0.0.99"}}}}}}`,
 			Setting:     "cluster.routing.allocation.exclude._name",
-			SetValue:    "10.0.0.99",
-			OldValue:    "10.0.0.2",
+			SetValue:    stringToPointer("10.0.0.99"),
+			OldValue:    stringToPointer("10.0.0.2"),
 		},
 
 		{
@@ -702,8 +708,8 @@ func TestSetClusterSettings(t *testing.T) {
 			Body:        `{"transient":{"cluster.routing.allocation.exclude._name":"10.0.0.99"}}`,
 			PutResponse: `{"persistent":{},"transient":{"cluster":{"routing":{"allocation":{"exclude":{"_name":"10.0.0.99"}}}}}}`,
 			Setting:     "cluster.routing.allocation.exclude._name",
-			SetValue:    "10.0.0.99",
-			OldValue:    "10.0.0.2",
+			SetValue:    stringToPointer("10.0.0.99"),
+			OldValue:    stringToPointer("10.0.0.2"),
 		},
 
 		{
@@ -713,8 +719,30 @@ func TestSetClusterSettings(t *testing.T) {
 			Body:        `{"transient":{"cluster.routing.allocation.exclude._name":"10.0.0.99"}}`,
 			PutResponse: `{"persistent":{},"transient":{"cluster":{"routing":{"allocation":{"exclude":{"_name":"10.0.0.99"}}}}}}`,
 			Setting:     "cluster.routing.allocation.exclude._name",
-			SetValue:    "10.0.0.99",
-			OldValue:    "",
+			SetValue:    stringToPointer("10.0.0.99"),
+			OldValue:    nil,
+		},
+
+		{
+			// Tests for behavior when removing setting (null'ing it).
+			Name:        "Removing Existing Persistent Setting",
+			GetResponse: `{"transient":{},"persistent":{"cluster":{"routing":{"allocation":{"exclude":{"_name":"10.0.0.2"}}}}}}`,
+			Body:        `{"transient":{"cluster.routing.allocation.exclude._name":null}}`,
+			PutResponse: `{"transient":{},"persistent":{}}`,
+			Setting:     "cluster.routing.allocation.exclude._name",
+			SetValue:    nil,
+			OldValue:    stringToPointer("10.0.0.2"),
+		},
+
+		{
+			// Tests for behavior when removing setting (null'ing it) that is already null.
+			Name:        "Removing Null Persistent Setting",
+			GetResponse: `{"transient":{},"persistent":{}}`,
+			Body:        `{"transient":{"cluster.routing.allocation.exclude._name":null}}`,
+			PutResponse: `{"transient":{},"persistent":{}}`,
+			Setting:     "cluster.routing.allocation.exclude._name",
+			SetValue:    nil,
+			OldValue:    nil,
 		},
 	}
 
@@ -743,14 +771,35 @@ func TestSetClusterSettings(t *testing.T) {
 				st.Errorf("Expected error to be nil, %s", err)
 			}
 
-			if oldSetting != x.OldValue {
-				st.Errorf("Unexpected old value, got %s", oldSetting)
+			if oldSetting == nil {
+				if x.OldValue != nil {
+					st.Fatalf("Unexpected old value: expected old value to be %s, got nil", *x.OldValue)
+				}
+			}
+
+			if oldSetting != nil {
+				if x.OldValue == nil {
+					st.Fatalf("Unexpected old value: expected old value to be nil, got %v", *oldSetting)
+				}
+				if *oldSetting != *x.OldValue {
+					st.Errorf("Unexpected old value: expected %s, got %s", *x.OldValue, *oldSetting)
+				}
 			}
 
-			if newSetting != "10.0.0.99" {
-				st.Errorf("Unexpected new value, got %s", newSetting)
+			if newSetting == nil {
+				if x.SetValue != nil {
+					st.Fatalf("Unexpected new value, got nil, expected %s", *x.SetValue)
+				}
 			}
 
+			if newSetting != nil {
+				if x.SetValue == nil {
+					st.Errorf("Unexpected new value, got %s, expected nil", *newSetting)
+				}
+				if *newSetting != *x.SetValue {
+					st.Errorf("Unexpected new value, got %v, expected %v", newSetting, x.SetValue)
+				}
+			}
 		})
 	}
 }
@@ -831,7 +880,7 @@ func TestSetClusterSetting_BadRequest(t *testing.T) {
 	defer ts.Close()
 	client := NewClient(host, port)
 
-	_, _, err := client.SetClusterSetting("cluster.routing.allocation.enable", "foo")
+	_, _, err := client.SetClusterSetting("cluster.routing.allocation.enable", stringToPointer("foo"))
 
 	if err == nil {
 		t.Errorf("Expected error to not be nil, %s", err)

go.mod

@@ -13,6 +13,7 @@ require (
 	github.com/pkg/errors v0.8.0 // indirect
 	github.com/smartystreets/goconvey v1.6.4 // indirect
 	github.com/spf13/cobra v0.0.5
+	github.com/spf13/pflag v1.0.5 // indirect
 	github.com/spf13/viper v1.3.2
 	github.com/stretchr/testify v1.4.0 // indirect
 	github.com/tidwall/gjson v1.1.3

go.sum

@@ -61,6 +61,8 @@ github.com/spf13/jwalterweatherman v1.0.0 h1:XHEdyB+EcvlqZamSM4ZOMGlc93t6AcsBEu9
 github.com/spf13/jwalterweatherman v1.0.0/go.mod h1:cQK4TGJAtQXfYWX+Ddv3mKDzgVb68N+wFjFa4jdeBTo=
 github.com/spf13/pflag v1.0.3 h1:zPAT6CGy6wXeQ7NtTnaTerfKOsV6V6F8agHXFiazDkg=
 github.com/spf13/pflag v1.0.3/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnInEg4=
+github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA=
+github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
 github.com/spf13/viper v1.3.2 h1:VUFqw5KcqRf7i70GOzW7N+Q7+gxVBkSSqiXB12+JQ4M=
 github.com/spf13/viper v1.3.2/go.mod h1:ZiWeW+zYFKm7srdB9IoDzzZXaJaI5eL9QjNiN/DMA2s=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=

pkg/cli/entrypoint.go

@@ -5,11 +5,11 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"fmt"
-	"github.com/github/vulcanizer"
 	"io"
 	"io/ioutil"
 	"os"
 
+	"github.com/github/vulcanizer"
 	"github.com/olekukonko/tablewriter"
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
@@ -113,6 +113,13 @@ func getClient() *vulcanizer.Client {
 	return v
 }
 
+func printableNil(ptrValue *string) string {
+	if ptrValue == nil {
+		return "null"
+	}
+	return *ptrValue
+}
+
 func renderTable(rows [][]string, header []string) string {
 	var result bytes.Buffer
 	table := tablewriter.NewWriter(&result)

pkg/cli/setting.go

@@ -9,6 +9,8 @@ import (
 
 var settingToUpdate, valueToUpdate string
 
+var removeValue bool
+
 func init() {
 
 	cmdSettingUpdate.Flags().StringVarP(&settingToUpdate, "setting", "s", "", "Elasticsearch cluster setting to update (required)")
@@ -18,12 +20,9 @@ func init() {
 		os.Exit(1)
 	}
 
-	cmdSettingUpdate.Flags().StringVarP(&valueToUpdate, "value", "v", "", "Value of the Elasticsearch cluster setting to update (required)")
-	err = cmdSettingUpdate.MarkFlagRequired("value")
-	if err != nil {
-		fmt.Printf("Error binding value configuration flag: %s \n", err)
-		os.Exit(1)
-	}
+	cmdSettingUpdate.Flags().StringVarP(&valueToUpdate, "value", "v", "", "Value of the Elasticsearch cluster setting to update (can't be used with \"--remove\")")
+
+	cmdSettingUpdate.Flags().BoolVar(&removeValue, "remove", false, "Remove provided cluster setting, resetting it to default configuration (can't be used with \"--value|-v\")")
 
 	cmdSetting.AddCommand(cmdSettingUpdate)
 	rootCmd.AddCommand(cmdSetting)
@@ -41,18 +40,36 @@ var cmdSettingUpdate = &cobra.Command{
 	Long:  `This command will update the cluster's settings with the provided value.`,
 	Run: func(cmd *cobra.Command, args []string) {
 
+		if cmd.Flags().Changed("value") && cmd.Flags().Changed("remove") {
+			fmt.Println("Can't set both \"--value|-v\" and \"--remove\" options")
+			fmt.Print(cmd.UsageString())
+			os.Exit(1)
+		}
+		if !cmd.Flags().Changed("value") && !cmd.Flags().Changed("remove") {
+			fmt.Println("Error: requires one of \"--value|-v\" or \"--remove\"")
+			fmt.Print(cmd.UsageString())
+			os.Exit(1)
+		}
 		v := getClient()
 
-		existingValue, newValue, err := v.SetClusterSetting(settingToUpdate, valueToUpdate)
+		var ptrValueToUpdate *string
+
+		if removeValue {
+			ptrValueToUpdate = nil
+		} else {
+			ptrValueToUpdate = &valueToUpdate
+		}
+
+		existingValue, newValue, err := v.SetClusterSetting(settingToUpdate, ptrValueToUpdate)
 
 		if err != nil {
-			fmt.Printf("Error when trying to update \"%s\" to \"%s\n", settingToUpdate, valueToUpdate)
+			fmt.Printf("Error when trying to update \"%s\" to \"%s\"\n", settingToUpdate, printableNil(ptrValueToUpdate))
 			fmt.Printf("Error is: %s\n", err)
 			os.Exit(1)
 		}
 
 		fmt.Printf("Updated setting %s\n", settingToUpdate)
-		fmt.Printf("\tOld value: %s\n", existingValue)
-		fmt.Printf("\tNew value: %s\n", newValue)
+		fmt.Printf("\tOld value: %s\n", printableNil(existingValue))
+		fmt.Printf("\tNew value: %s\n", printableNil(newValue))
 	},
 }

script/integration-test

@@ -1,6 +1,18 @@
 #!/bin/bash
 set -e
 
+# Before we start, check if "vm.max_map_count" is correctly set, or the Docker containers will silently fail and lock the test
+maxMap="$(sysctl vm.max_map_count | awk -F "= " '{print $2}')"
+if [[ $maxMap -lt 262144 ]]
+then 
+    # incorrect config, won't work
+    echo "You need to increase vm.max_map_count to at least 262144 before Elasticsearch will start"
+    echo "Currently set to $maxMap"
+    echo "Run \"sudo sysctl -w vm.max_map_count=262144\" and start the test again."
+    exit 1
+fi
+
+
 # Run regular unit tests first
 ./script/test