server stops listen watch stream requests if watch is not permitted

[dkabracadabra]

I know almost nothing about golang but i think there's a mistake/typo in server's watch stream recieve loop.
This line should be one line upper, inside case <-sws.closec: branch. Looks like currently if watch is not permitted then server just exits recieve loop and silently stops responding requests on watch stream. At least it is what I saw on client side.

[jingyih]

Reading form the code, the code execution blocks until either of the following 2 things happen before return nil statement.

• server is able to send a response which indicates permission denial back to client (via ctrlStream)
• or, the watch stream is closed

[dkabracadabra]

Yes, exactly. Thank you! Imo, return nil there should be executed only if watch stream is closed, otherwise loop should continue receiving requests. Permission denial for one watch should not affect other watches of the same watch stream. Anyway it should not silently exit receive loop and leave watch stream and other watches unaware.
Right now on client side if I got permission denial from create watch request then all subsequent requests won't be responded, however stream is open and watch notifications are still there. As a workaround in this situation I have to close watch stream and recreate all watches it had.
Imho, it's a typo and instead of

				select {
				case sws.ctrlStream <- wr:
				case <-sws.closec:
				}
				return nil

there should be

			select {
			case sws.ctrlStream <- wr:
			case <-sws.closec:
				return nil
			}

The later code is from the similar part of the same loop

[jingyih]

I see. Your explanation make sense to me. Let me reproduce this issue locally and send a fix.

[polyrabbit]

@dkabracadabra, I just sent a PR(#11754) that may fix your issue.

Just out of curiosity, could you please share your use cases where this issue happens? I find it hard to reproduce.

We used to encounter a similar problem as this one, that in some rare cases the watch request will hang forever, and we worked around this by not leveraging the etcd auth feature (it's a pity we didn't dig further).

[dkabracadabra]

@polyrabbit thank you!
I had the following:

• AKS
• 3 nodes etcd 3.4.3 cluster
• 3 nodejs apps that use mixer/etcd3 lib

The lib has a related issue issue that I patched first and then faced the issue with watchstream.

All 3 nodes should execute a task on some event, but only one at a time. The lib lacks native election and lock features of etcd. So basically it is like the following:

1. get lease
2. try create key using the lease if it is not created yet (using etcd if)
3. if key successfully created then go on and after task finished just revoke the lease
4. otherwise watch the key deleted event
5. when key deleted then go to 1

The issue was highly reproducible to me. ~50%

[polyrabbit]

Ah! it makes sense - you are not using the official Go client, the official one has some protection against the broken stream. Thus it's hard to reproduce on my end.

Thanks for sharing! @dkabracadabra