Comparing value in TXN on non-existing key always returns failure.

[purpleidea]

Bug report criteria

• This bug report is not security related, security issues should be disclosed privately via etcd maintainers.
• This is not a support request or question, support requests or questions should be raised in the etcd discussion forums.
• You have read the etcd bug reporting guidelines.
• Existing open issues along with etcd frequently asked questions have been checked and this is not a duplicate.

What happened?

Etcd has a .Succeeded field on a Txn response to know which branch of the transaction (then or else) was run. This is critically important to know what happened.

If you have an OpTxn then the child .Succeeded values are NOT SET and we seem to always see the false zero value.

A full code reproducer is included below.

What did you expect to happen?

At least one of the two branches should see a true. Neither does.

How can we reproduce it (as minimally and precisely as possible)?

Run etcd plainly in a terminal. Then run this code:
I left in my hacking mess for anyone curious. At least one of the "SUCCESS" lines should print true. They always print false.

// etcdbug

package main

import (
	"context"
	"fmt"

	pb "go.etcd.io/etcd/api/v3/etcdserverpb"
	clientv3 "go.etcd.io/etcd/client/v3"
)

func main() {

	client, err := clientv3.New(clientv3.Config{
		Endpoints: []string{"localhost:2379"},
	})
	if err != nil {
		panic(err)
	}
	defer client.Close()

	ops := []clientv3.Op{}
	{
		ifs := []clientv3.Cmp{}
		thn := []clientv3.Op{}
		els := []clientv3.Op{}

		path := "/foo"
		data := "hello"
		ifs = append(ifs, clientv3.Compare(clientv3.Value(path), "!=", data)) // THIS ONE IS !=
		els = append(els, clientv3.OpPut(path, data))

		op := clientv3.OpTxn(ifs, thn, els)
		ops = append(ops, op)
	}
	{
		ifs := []clientv3.Cmp{}
		thn := []clientv3.Op{}
		els := []clientv3.Op{}

		path := "/bar"
		data := "world"
		ifs = append(ifs, clientv3.Compare(clientv3.Value(path), "=", data)) // THIS ONE IS =
		els = append(els, clientv3.OpPut(path, data))

		op := clientv3.OpTxn(ifs, thn, els)
		ops = append(ops, op)
	}

	txn := client.Txn(context.Background())
	txnResp, err := txn.If().Then(ops...).Else().Commit()
	if err != nil {
		panic(err)
	}

	//
	// Who even knows the correct way to pull out nested data from the
	// protobuf mess? Various approaches have been tried, thoughts welcome.
	//

	realOut := txnResp.OpResponse().Txn()

	fmt.Printf("XXX: OUT(%T): %+v\n", realOut, realOut)
	if !realOut.Succeeded {
		// else branch happened
		panic("unexpected branch")
	}

	//responses := realOut.GetResponses() // doesn't work
	responses := realOut.Responses

	fmt.Printf("XXX: NUMBER OF RESPONSES: %+v\n", len(responses))

	for _, resp := range responses {
		fmt.Printf("XXX: RESP(%T): %+v\n", resp, resp)

		x := resp.GetResponse()
		fmt.Printf("X (%T) %+v\n", x, x)

		thing, ok := x.(*pb.ResponseOp_ResponseTxn)
		if !ok {
			panic("woops")
		}

		//x := pb.ResponseOp_ResponseTxn(resp)
		fmt.Printf("THING (%T) %+v\n", thing, thing)

		// eventually we should see `true` in one of the loop iterations
		fmt.Printf("SUCCESS1? %+v\n", thing.ResponseTxn.GetSucceeded())
		fmt.Printf("SUCCESS2? %+v\n", resp.GetResponseTxn().Succeeded)

		fmt.Printf("---------------------------------\n\n")
	}
}

Anything else we need to know?

No response

Etcd version (please run commands below)

$ etcd --version
# paste output here

$ etcdctl version
# paste output here

Etcd configuration (command line flags or environment variables)

paste your configuration here

Etcd debug information (please run commands below, feel free to obfuscate the IP address or FQDN in the output)

$ etcdctl member list -w table
# paste output here

$ etcdctl --endpoints=<member list> endpoint status -w table
# paste output here

Relevant log output

[siyuanfoundation]

/cc @siyuanfoundation

[siyuanfoundation]

This is working as expected

etcd/server/etcdserver/txn/txn.go

Line 584 in c5a5afd

// Always fail if comparing a value on a key/keys that doesn't exist;

[purpleidea]

This is working as expected

You mis-read the above code. It should be true in at least one variant.

[serathius]

I think you need to simplify code to provide minimal reproduction.

[purpleidea]

I think you need to simplify code to provide minimal reproduction.

Simpler?

TL;DR: There's no way I've found to ever get the .Succeeded variable to be true, when it's a nested OpTxn. I assume those are not getting set and golang default to the zero value for the bool which is false.

[siyuanfoundation]

@purpleidea Can you help me understand your question?
The db starts empty.
Then in the two nested Txn,
Txn1: if current get("/foo") != "hello", do nothing, else put("/foo", "hello")
Txn2: if current get("/bar") == "world", do nothing, else put("/bar", "world")
And you expect one of Txn1.Succeeded or Txn2.Succeeded should be true?

[purpleidea]

The .Succeeded value tells the user which branch in the Txn ran. It should be true if the "then" branch runs, otherwise it should be false if the "else" branch runs.

I've made an example with two Txn's. They should have opposite .Succeeded values. Neither is ever true.

This works properly when using a Txn directly, but it does NOT work when the Txn is nested. That's the bug.

HTH

[siyuanfoundation]

If you start the db empty, both of the following Txn.Succeeded would return false, because Txn.Succeeded always returns true if the key does not exist, even though logically get("/foo") != "hello" if the key does not exist.
Txn1: if current get("/foo") != "hello", do nothing, else put("/foo", "hello")
Txn2: if current get("/bar") == "world", do nothing, else put("/bar", "world")

If you question is not related to the non-existing key, can you also write down the test case when it works when Txn is used directly as expected?

[purpleidea]

@siyuanfoundation Try and write down any code example where the OpTxn has a .Succeeded to true.

[serathius]

@siyuanfoundation Try and write down any code example where the OpTxn has a .Succeeded to true.

Please note that etcd is an open source and any response you get is by volunteers spending their own personal time to help community. There should be no expectation that someone will read your 100 lines of code and try to understand your intention when you cannot explain it otherwise.

Wrote very simple repro that shows what is happening here:

etcd $ ./bin/etcdctl txn -i 
compares:
val("/key") = ""

success requests (get, put, del):

failure requests (get, put, del):

FAILURE
etcd $ ./bin/etcdctl txn -i 
compares:
val("/key") != ""

success requests (get, put, del):

failure requests (get, put, del):

FAILURE
etcd $ ./bin/etcdctl put "/key" ""
OK
etcd $ ./bin/etcdctl txn -i 
compares:
val("/key") = "" 

success requests (get, put, del):

failure requests (get, put, del):

SUCCESS
etcd $ ./bin/etcdctl txn -i 
compares:
val("/key") != "1"           

success requests (get, put, del):

failure requests (get, put, del):

SUCCESS

You cannot compare value on non existing key, it will always return failure. You need to put the key for value comparison to work.

I expect it works like this to distinguish between key not existing and value being empty. That make "value" comparison not consistent with other type like mod revision.

etcd $ ./bin/etcdctl txn -i 
compares:
mod("/key") = "0"

success requests (get, put, del):

failure requests (get, put, del):

SUCCESS
etcd $ ./bin/etcdctl txn -i 
compares:
mod("/key") != "0"

success requests (get, put, del):

failure requests (get, put, del):

FAILURE
etcd $ ./bin/etcdctl put "/key" ""
OK
etcd $ ./bin/etcdctl txn -i 
compares:
mod("/key") = "0"

success requests (get, put, del):

failure requests (get, put, del):

FAILURE
etcd $ ./bin/etcdctl txn -i 
compares:
mod("/key") != "0"

success requests (get, put, del):

failure requests (get, put, del):

SUCCESS

[serathius]

To fix this we would need to treat a non existing key as key with empty value. This solves fixes the issue with the equality comparison not being symmetrical, but breaks detecting if key exists for empty value. We have two options, either:

• Be ok with breaking empty value case.
• Forbid setting empty value.

No matter what we do, this will be a breaking change for user if they depend on success result. The options differ by if change the response in a way it's not visible to user or explicitly return error. I would prefer that if we commit to make a breaking change we should make it explicit.

cc @ahrtr @fuweid for opinion.