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

server/auth: enable tokenProvider if recoved store enables auth #13172

Merged
merged 1 commit into from
Jul 11, 2021

Conversation

cfz
Copy link
Contributor

@cfz cfz commented Jul 2, 2021

we found a lease leak issue:
if a new member(by member add) is recovered by snapshot, and then
become leader, the lease will never expire afterwards. leader will
log the revoke failure caused by "invalid auth token", since the
token provider is not functional, and drops all generated token
from upper layer, which in this case, is the lease revoking
routine.

@mitake
Copy link
Contributor

mitake commented Jul 2, 2021

@cfz thanks a lot for this PR! I think it will fix the issue, but let me confirm with it. I think I can submit my review on Sunday.

@cfz cfz force-pushed the fix-auth-store-recover branch 3 times, most recently from a672b8c to 5b000a8 Compare July 4, 2021 04:40
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, thanks! BTW it's also great if you can add an e2e test case. Can you add it? If you aren't familiar with e2e test infra, I can take it.

@mitake
Copy link
Contributor

mitake commented Jul 4, 2021

On the second thought, the change improves simple token and it's not for production use cases. I think having e2e test case is too much. It would be good just merging it.

@cfz
Copy link
Contributor Author

cfz commented Jul 4, 2021

On the second thought, the change improves simple token and it's not for production use cases. I think having e2e test case is too much. It would be good just merging it.

that's also what i'm thinking about.

actually, we enabled auth but using Common Name based authentication for client side. looks like we eventually run into the simple token code somehow....

@cfz
Copy link
Contributor Author

cfz commented Jul 4, 2021

i would also like to backport this change to v3.4, which is the actual release we are using. but not sure we can do this without a new commit, due to the path changing during 3.4 and 3.5?

@cfz
Copy link
Contributor Author

cfz commented Jul 4, 2021

seems the workflows needs another approval, since i amend my commit.... @mitake

@cfz
Copy link
Contributor Author

cfz commented Jul 8, 2021

ping @mitake 😄

@mitake
Copy link
Contributor

mitake commented Jul 9, 2021

@cfz approved, thanks!

@cfz cfz force-pushed the fix-auth-store-recover branch from 5b000a8 to ea6e34d Compare July 10, 2021 02:52
we found a lease leak issue:
if a new member(by member add) is recovered by snapshot, and then
become leader, the lease will never expire afterwards. leader will
log the revoke failure caused by "invalid auth token", since the
token provider is not functional, and drops all generated token
from upper layer, which in this case, is the lease revoking
routine.
@cfz cfz force-pushed the fix-auth-store-recover branch from ea6e34d to b12f8c1 Compare July 10, 2021 17:17
@spzala
Copy link
Member

spzala commented Jul 10, 2021

@cfz thanks for addressing my comments, lgtm.

@mitake mitake merged commit 97f2831 into etcd-io:main Jul 11, 2021
@mitake
Copy link
Contributor

mitake commented Jul 11, 2021

merged, thanks @cfz @spzala !

@cfz cfz deleted the fix-auth-store-recover branch July 11, 2021 12:54
cfz pushed a commit to cfz/etcd that referenced this pull request Jul 11, 2021
cfz added a commit to cfz/etcd that referenced this pull request Jul 11, 2021
cfz added a commit to cfz/etcd that referenced this pull request May 6, 2022
tjungblu pushed a commit to tjungblu/etcd that referenced this pull request Sep 8, 2022
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