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.

Sign up for GitHub

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

Jump to bottom

*: handle auth invalid token and old revision errors in watch #14322

Merged
merged 2 commits into from
Sep 19, 2022
Merged

*: handle auth invalid token and old revision errors in watch #14322

merged 2 commits into from
Sep 19, 2022

Conversation

mitake
Copy link
Contributor

@mitake mitake commented Aug 7, 2022 •
edited
Loading

Fix #12385

It's still a WIP PR, please do not merge. Remaining todos:

  • clean up code
  • write a test

@mitake mitake mentioned this pull request Aug 7, 2022
Closed
@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2022 •
edited
Loading

Codecov Report

Merging #14322 (c561452) into main (b886bbc) will decrease coverage by 0.18%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##             main   #14322      +/-   ##
==========================================
- Coverage   75.39%   75.21%   -0.19%     
==========================================
  Files         457      457              
  Lines       37207    37235      +28     
==========================================
- Hits        28053    28005      -48     
- Misses       7394     7455      +61     
- Partials     1760     1775      +15
Flag Coverage Δ
all 75.21% <72.72%> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
client/v3/watch.go 92.59% <66.66%> (-1.19%) ⬇️
server/etcdserver/api/v3rpc/watch.go 84.29% <77.77%> (-2.00%) ⬇️
client/v3/leasing/util.go 91.66% <0.00%> (-6.67%) ⬇️
client/v3/namespace/watch.go 87.87% <0.00%> (-6.07%) ⬇️
raft/rafttest/node.go 95.00% <0.00%> (-5.00%) ⬇️
client/v3/concurrency/session.go 88.63% <0.00%> (-4.55%) ⬇️
server/etcdserver/api/v3rpc/util.go 70.96% <0.00%> (-3.23%) ⬇️
server/etcdserver/cluster_util.go 70.35% <0.00%> (-3.17%) ⬇️
server/proxy/grpcproxy/watch.go 93.64% <0.00%> (-2.90%) ⬇️
pkg/traceutil/trace.go 96.15% <0.00%> (-1.93%) ⬇️
... and 16 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ahrtr ahrtr marked this pull request as draft August 8, 2022 20:10
@ahrtr
Copy link
Member

ahrtr commented Aug 8, 2022

Thanks @mitake , just marked this PR as draft. Please feel free to mark it as ready for review once it's ready.

@kafuu-chino
Copy link
Contributor

I clone locally, tried a run and the problem was solved, the previous watch will also be send by nextResume.

However, because the default WatchID = 0 is returned by error, this will cause WatchID = 0 to be deleted once in ids, although it will be recreated later. See #14296.

I also read the committed code, instead of using a string, would try passing an error code between the client and the server?
And why not

err := sws.isWatchPermitted(creq)
if err != nil {

@mitake
Copy link
Contributor Author

mitake commented Aug 11, 2022

Thanks for trying this PR @kafuu-chino .

I also read the committed code, instead of using a string, would try passing an error code between the client and the server?

It might be simpler but the error codes don't have unique integer IDs. So keeping the type of cancelReason as string is good.

I'll also change the return value type of isWatchPermitted().

@mitake mitake mentioned this pull request Sep 9, 2022
Merged
@mitake mitake changed the title WIP, DO NOT MERGE *: handle auth invalid token and old revision errors in watch *: handle auth invalid token and old revision errors in watch Sep 14, 2022
@mitake mitake marked this pull request as ready for review September 14, 2022 14:14
@mitake
Copy link
Contributor Author

mitake commented Sep 14, 2022 •
edited
Loading

@ahrtr @kafuu-chino I think this PR is ready to be reviewed, could you check when you have time?

ahrtr
ahrtr reviewed Sep 15, 2022
View reviewed changes
server/etcdserver/api/v3rpc/watch.go Outdated Show resolved Hide resolved
server/etcdserver/api/v3rpc/watch.go Outdated Show resolved Hide resolved
@ahrtr
Copy link
Member

ahrtr commented Sep 15, 2022

The pipeline failure should be caused by this PR. Please fix the failures.

ahrtr
ahrtr approved these changes Sep 17, 2022
View reviewed changes
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

Thank you @mitake

kafuu-chino
kafuu-chino reviewed Sep 19, 2022
View reviewed changes
Copy link
Contributor

@kafuu-chino kafuu-chino left a comment

Choose a reason for hiding this comment

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

LGTM
I think it's okay and the test is fine.
Thanks for solving my problem :)

@mitake
Copy link
Contributor Author

mitake commented Sep 19, 2022

@ahrtr @kafuu-chino Thanks for reviewing! The failed check caused by the previous version of the PR was e2e IIRC, but I couldn't reproduce the failure on my local env, I guess it was a non deterministic issue. Let me merge this branch.

@mitake mitake merged commit c793f18 into etcd-io:main Sep 19, 2022
@mitake mitake deleted the watch-auth-err branch September 19, 2022 14:33
@mitake
Copy link
Contributor Author

mitake commented Sep 19, 2022

I'll open PRs for backporting the change to stable releases later.

This was referenced Oct 3, 2022
Merged
Merged
@serathius serathius mentioned this pull request Nov 14, 2022
Closed
22 tasks
ahrtr added a commit to ahrtr/etcd that referenced this pull request Dec 14, 2022
@ahrtr
clientv3: revert the client side change in 14547
1fdfb42 
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 <[email protected]>
@ahrtr ahrtr mentioned this pull request Dec 14, 2022
Merged
@ahrtr ahrtr mentioned this pull request Dec 26, 2022
Closed
This was referenced Mar 21, 2023
Closed
Merged
tjungblu pushed a commit to tjungblu/etcd that referenced this pull request Jul 26, 2023
@ahrtr @tjungblu
clientv3: revert the client side change in 14547
c619d9c 
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 <[email protected]>
tjungblu pushed a commit to tjungblu/etcd that referenced this pull request Jul 26, 2023
@ahrtr @tjungblu
clientv3: revert the client side change in 14547
89a9ad2 
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 <[email protected]>
@ahrtr ahrtr mentioned this pull request Feb 6, 2024
Merged
@hugoShaka hugoShaka mentioned this pull request Jun 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kafuu-chino kafuu-chino kafuu-chino left review comments

@ahrtr ahrtr ahrtr approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

watch stream return "permission denied" if token expired
4 participants
@mitake @codecov-commenter @ahrtr @kafuu-chino