Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.5] clientv3: revert the client side change in 14547 #14995

Merged
merged 1 commit into from
Dec 15, 2022

Conversation

ahrtr
Copy link
Member

@ahrtr ahrtr commented Dec 14, 2022

In order to fix #12385, PR #14322 introduced a change in which the client side may retry based on the error message returned from server side.

This is not good, as it's too fragile and it's also changed the protocol between client and server. Please see the discussion in kubernetes/kubernetes#114403

Note: The issue #12385 only happens when auth is enabled, and client side reuse the same client to watch.

So we decided to rollback the change on 3.5, reasons:

  1. K8s doesn't enable auth at all. It has no any impact on K8s.
  2. It's very easy for client application to workaround the issue.
    The client just needs to create a new client each time before watching.

Signed-off-by: Benjamin Wang wachao@vmware.com

cc @mitake @ptabor @serathius @spzala @liggitt @dims @lavalamp @deads2k

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

In order to fix etcd-io#12385,
PR etcd-io#14322 introduced a change
in which the client side may retry based on the error message
returned from server side.

This is not good, as it's too fragile and it's also changed the
protocol between client and server. Please see the discussion
in kubernetes/kubernetes#114403

Note: The issue etcd-io#12385 only
happens when auth is enabled, and client side reuse the same client
to watch.

So we decided to rollback the change on 3.5, reasons:
1.K8s doesn't enable auth at all. It has no any impact on K8s.
2.It's very easy for client application to workaround the issue.
  The client just needs to create a new client each time before watching.

Signed-off-by: Benjamin Wang <wachao@vmware.com>
@ahrtr
Copy link
Member Author

ahrtr commented Dec 14, 2022

Reference: #14547

@dims
Copy link
Contributor

dims commented Dec 14, 2022

yes please. thanks!

Copy link
Contributor

@mitake mitake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, sorry for the issue...

@mitake mitake merged commit f1842b6 into etcd-io:release-3.5 Dec 15, 2022
@ahrtr
Copy link
Member Author

ahrtr commented Dec 15, 2022

Thanks @mitake for the quick review.

@jeansimonbarry
Copy link

jeansimonbarry commented Jan 10, 2023

@ahrtr Sorry but I just want to confirm, as we are seeing the same issue in our cluster where we enabled auth. Is the correct way to fix this by instead creating a new client for every Watch call made? Or I guess we can upgrade to 3.5.6 which does have the fix?

@ahrtr
Copy link
Member Author

ahrtr commented Jan 10, 2023

Is the correct way to fix this by instead creating a new client for every Watch call made?

Yes, it should can workaround this issue.

Or I guess we can upgrade to 3.5.6 which does have the fix?

3.5.6 has this fix, but we reverted the change in 3.5.7( to be released soon).

We are working on a more formal solution in #15058. I am not sure whether we will backport the fix to 3.5 eventually for now. If lots of people request backporting, we can do it.

@jeansimonbarry
Copy link

Awesome thanks so much 👍

@ahrtr ahrtr changed the title clientv3: revert the client side change in 14547 [3.5] clientv3: revert the client side change in 14547 Mar 26, 2023
tjungblu pushed a commit to tjungblu/etcd that referenced this pull request Jul 26, 2023
clientv3: revert the client side change in 14547
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants