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

Remove string dependent error handling in watch and auth #17384

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

mitake
Copy link
Contributor

@mitake mitake commented Feb 6, 2024

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

If we cannot finish #15253 before 3.6, it's better to drop the string dependent error handling from the main branch.
cc @ahrtr

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
@mitake
Copy link
Contributor Author

mitake commented Feb 6, 2024

It's better to avoid introducing the string dependent behavior in the main branch and 3.6, so reverting it now is safer.
Once people can allocate time, discussing again about #15253 and introducing the flag would be good.

@ahrtr
Copy link
Member

ahrtr commented Feb 6, 2024

Thanks @mitake

To be clearer, this PR is reverting #14935 and partially revert #14322 .

  • Since the server side change has already been backported to 3.5 (related to populating different string for cancelReason based on different errors), and it's OK to keep that part unchanged.
  • Please also remove AuthTokenTTL from tests/framework/integration/cluster.go.

Regarding the next step, we have multiple choice,

  • Keep it as it'is. In other words, do nothing (probably we need to update the doc). If applications see any issue, they just need to recreate the client.
  • Adopt solution something like etcdserverpb: add "retry" flag to WatchResponse #15253
    • Good side
      • It can fix the issue completely
    • Bad side
      • It adds new field, and complicate the use case of downgrade (e.g. 3.5 -> 3.4)
      • It isn't flexible (as compared to the adding of reasonCode), but we don't see other valid cases in which the client sides will use the reasonCode.

Given our bandwidth and it isn't a critical issue (not even a major issue), prefer to the first solution (keep it as it's)?
@mitake @serathius @lavacat @liggitt

EDIT (Feb 8, 2024): When using TLS CN based auth, this issue (#12385) can't be reproduced either.

So workaround for the issue:

  • Try to use TLS CN based auth
  • If the client is still using username:password, then re-create the client when running into the issue.

@ahrtr
Copy link
Member

ahrtr commented Feb 7, 2024

  • Please also remove AuthTokenTTL from tests/framework/integration/cluster.go.

Let me take care of this separately in a followup PR.

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

lgtm

cc. @serathius

@ahrtr ahrtr merged commit 25f91b4 into etcd-io:main Feb 8, 2024
39 checks passed
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.

3 participants