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

server/auth: invalidate range permission cache during recovering from… #13920

Closed
wants to merge 1 commit into from
Closed

server/auth: invalidate range permission cache during recovering from… #13920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions server/auth/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,8 @@ func (as *authStore) Recover(be AuthBackend) {
as.tokenProvider.enable()
}
as.enabledMu.Unlock()

as.rangePermCache = make(map[string]*unifiedRangePermissions)
Copy link
Member

Choose a reason for hiding this comment

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

Two comments:

  1. Suggest to call clearCachedPerm;
  2. There is a potential race condition, some requests (such as v3_server.go#L128 and watch.go#L235 ) coming from API (outside of the applying workflow) may be accessing the rangePermCache concurrently. It seems that we need to add a lock to protect it.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1. Good catch.

In general the existing locking strategy seems week here:

  • The access to this cache is protected by readtx.Lock()
  • readTx is owned by backend that we are swapping here.
  • so seems theoretically possible that there would be 2 transactions concurrently accessing/modifying the cache... one having tx on old backend, the other the new backend. Closing of the old backend is fully asynchronic:

    etcd/server/etcdserver/server.go

    Lines 1000 to 1008 in 2e034d2

    go func() {
    lg.Info("closing old backend file")
    defer func() {
    lg.Info("closed old backend file")
    }()
    if err := oldbe.Close(); err != nil {
    lg.Panic("failed to close old backend", zap.Error(err))
    }
    }()
  • Thus seems cache should have its own lock instead of piggybacking on transaction lock.

Copy link
Member

Choose a reason for hiding this comment

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

Thus seems cache should have its own lock instead of piggybacking on transaction lock

Exactly! Please note that the cache can't be protected by the readtx.Lock(), because a batchTx and a readRx can execute concurrently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing out that! I think it's an independent issue, let me open a dedicated PR for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened an independent PR here: #13954 It's great if you can review.

}

func (as *authStore) selectPassword(password string, hashedPassword string) ([]byte, error) {
Expand Down
4 changes: 4 additions & 0 deletions server/auth/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,10 @@ func TestRecover(t *testing.T) {
if !as.IsAuthEnabled() {
t.Fatalf("expected auth enabled got disabled")
}

if len(as.rangePermCache) != 0 {
t.Fatalf("expected range permission cache is empty")
}
}

func TestCheckPassword(t *testing.T) {
Expand Down