From c7496c565262c98e68cdee28af80491f5c2b0695 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Mon, 26 Sep 2016 16:00:00 -0700 Subject: [PATCH] Change delete CAS behavior to require ModifyIndex --- command/kv_delete.go | 36 ++++------ command/kv_delete_test.go | 72 +++++++++++++++++-- .../docs/commands/kv/delete.html.markdown.erb | 12 +--- 3 files changed, 81 insertions(+), 39 deletions(-) diff --git a/command/kv_delete.go b/command/kv_delete.go index 197f62eaa2..4fa620a213 100644 --- a/command/kv_delete.go +++ b/command/kv_delete.go @@ -43,8 +43,7 @@ KV Delete Options: the next query. The default value is false. -modify-index= Unsigned integer representing the ModifyIndex of the - key. This is often combined with the -cas flag, but it - can be specified for any key. The default value is 0. + key. This is used in combination with the -cas flag. -recurse Recursively delete all keys with the path. The default value is false. @@ -57,7 +56,6 @@ func (c *KVDeleteCommand) Run(args []string) int { cmdFlags.Usage = func() { c.Ui.Output(c.Help()) } datacenter := cmdFlags.String("datacenter", "", "") token := cmdFlags.String("token", "", "") - stale := cmdFlags.Bool("stale", false, "") cas := cmdFlags.Bool("cas", false, "") modifyIndex := cmdFlags.Uint64("modify-index", 0, "") recurse := cmdFlags.Bool("recurse", false, "") @@ -94,15 +92,22 @@ func (c *KVDeleteCommand) Run(args []string) int { return 1 } + // ModifyIndex is required for CAS + if *cas && *modifyIndex == 0 { + c.Ui.Error("Must specify -modify-index with -cas!") + return 1 + } + + // Specifying a ModifyIndex for a non-CAS operation is not possible. + if *modifyIndex != 0 && !*cas { + c.Ui.Error("Cannot specify -modify-index without -cas!") + } + // It is not valid to use a CAS and recurse in the same call if *recurse && *cas { c.Ui.Error("Cannot specify both -cas and -recurse!") return 1 } - if *recurse && *modifyIndex != 0 { - c.Ui.Error("Cannot specify both -modify-index and -recurse!") - return 1 - } // Create and test the HTTP client conf := api.DefaultConfig() @@ -133,23 +138,6 @@ func (c *KVDeleteCommand) Run(args []string) int { ModifyIndex: *modifyIndex, } - // If the user did not supply a -modify-index, but wants a check-and-set, - // grab the current modify index and store that on the key. - if pair.ModifyIndex == 0 { - currentPair, _, err := client.KV().Get(key, &api.QueryOptions{ - Datacenter: *datacenter, - Token: *token, - AllowStale: *stale, - }) - if err != nil { - c.Ui.Error(fmt.Sprintf("Error! Could not get current key: %s", err)) - return 1 - } - if currentPair != nil { - pair.ModifyIndex = currentPair.ModifyIndex - } - } - success, _, err := client.KV().DeleteCAS(pair, wo) if err != nil { c.Ui.Error(fmt.Sprintf("Error! Did not delete key %s: %s", key, err)) diff --git a/command/kv_delete_test.go b/command/kv_delete_test.go index 8768538ca2..ac8572aadd 100644 --- a/command/kv_delete_test.go +++ b/command/kv_delete_test.go @@ -1,6 +1,7 @@ package command import ( + "strconv" "strings" "testing" @@ -25,12 +26,16 @@ func TestKVDeleteCommand_Validation(t *testing.T) { output string }{ "-cas and -recurse": { - []string{"-cas", "-recurse"}, + []string{"-cas", "-modify-index", "2", "-recurse", "foo"}, "Cannot specify both", }, - "-modify-index and -recurse": { - []string{"-modify-index", "2", "-recurse"}, - "Cannot specify both", + "-cas no -modify-index": { + []string{"-cas", "foo"}, + "Must specify -modify-index", + }, + "-modify-index no -cas": { + []string{"-modify-index", "2", "foo"}, + "Cannot specify -modify-index without", }, "no key": { []string{}, @@ -141,3 +146,62 @@ func TestKVDeleteCommand_Recurse(t *testing.T) { } } } + +func TestKVDeleteCommand_CAS(t *testing.T) { + srv, client := testAgentWithAPIClient(t) + defer srv.Shutdown() + waitForLeader(t, srv.httpAddr) + + ui := new(cli.MockUi) + c := &KVDeleteCommand{Ui: ui} + + pair := &api.KVPair{ + Key: "foo", + Value: []byte("bar"), + } + _, err := client.KV().Put(pair, nil) + if err != nil { + t.Fatalf("err: %#v", err) + } + + args := []string{ + "-http-addr=" + srv.httpAddr, + "-cas", + "-modify-index", "1", + "foo", + } + + code := c.Run(args) + if code == 0 { + t.Fatalf("bad: expected error") + } + + data, _, err := client.KV().Get("foo", nil) + if err != nil { + t.Fatal(err) + } + + // Reset buffers + ui.OutputWriter.Reset() + ui.ErrorWriter.Reset() + + args = []string{ + "-http-addr=" + srv.httpAddr, + "-cas", + "-modify-index", strconv.FormatUint(data.ModifyIndex, 10), + "foo", + } + + code = c.Run(args) + if code != 0 { + t.Fatalf("bad: %d. %#v", code, ui.ErrorWriter.String()) + } + + data, _, err = client.KV().Get("foo", nil) + if err != nil { + t.Fatal(err) + } + if data != nil { + t.Fatalf("bad: %#v", data) + } +} diff --git a/website/source/docs/commands/kv/delete.html.markdown.erb b/website/source/docs/commands/kv/delete.html.markdown.erb index e9ec96590a..7d05bd7558 100644 --- a/website/source/docs/commands/kv/delete.html.markdown.erb +++ b/website/source/docs/commands/kv/delete.html.markdown.erb @@ -26,8 +26,7 @@ Usage: `consul kv delete [options] KEY_OR_PREFIX` will be used on the next query. The default value is false. * `-modify-index=` - Unsigned integer representing the ModifyIndex of the - key. This is often combined with the -cas flag, but it can be specified for - any key. The default value is 0. + key. This is used in combination with the -cas flag. * `-recurse` - Recursively delete all keys with the path. The default value is false. @@ -64,15 +63,6 @@ $ consul kv delete -cas -modify-index=456 redis/config/connections Success! Deleted key: redis/config/connections ``` -It is also possible to have Consul fetch the current ModifyIndex before making -the query, by omitting the `-modify-index` flag. If the data is changed between -the initial read and the write, the operation will fail. - -``` -$ consul kv delete -cas redis/config/connections -Success! Deleted key: redis/config/connections -``` - To recursively delete all keys that start with a given prefix, specify the `-recurse` flag: