Read operations related to auth and role should use linearizableReadNotify?

qsyqian commented 2 months ago

Bug report criteria

[X] This bug report is not security related, security issues should be disclosed privately via etcd maintainers.
[X] This is not a support request or question, support requests or questions should be raised in the etcd discussion forums.
[X] You have read the etcd bug reporting guidelines.
[X] Existing open issues along with etcd frequently asked questions have been checked and this is not a duplicate.

What happened?

I found that read operations related to auth directly use raftRequest to transfer the operation to backend. So if request to one etcd member, all the members will do query from the backend. eg:

func (s *EtcdServer) UserGet(ctx context.Context, r *pb.AuthUserGetRequest) (*pb.AuthUserGetResponse, error) {
    resp, err := s.raftRequest(ctx, pb.InternalRaftRequest{AuthUserGet: r})
    if err != nil {
        return nil, err
    }
    return resp.(*pb.AuthUserGetResponse), nil
}

I think we should change this func to this. First, request one readIndex from leader, then do query with store.

func (s *EtcdServer) UserGet(ctx context.Context, r *pb.AuthUserGetRequest) (*pb.AuthUserGetResponse, error) {
    if err := s.linearizableReadNotify(ctx); err != nil {
        return nil, err
    }
    return s.AuthStore().UserGet(r)
}

So, only the member which receive the request will do query from backend.

Anyone who can double check this, please let me know if i am wrong.

What did you expect to happen?

Please see description above.

How can we reproduce it (as minimally and precisely as possible)?

N/A

Anything else we need to know?

Nothing.

Etcd version (please run commands below)

3.6.0-alpha.0

Etcd configuration (command line flags or environment variables)

# paste your configuration here

Etcd debug information (please run commands below, feel free to obfuscate the IP address or FQDN in the output)

```console $ etcdctl member list -w table # paste output here $ etcdctl --endpoints= endpoint status -w table # paste output here ```

Relevant log output

No response

ahrtr commented 2 months ago

It seems like a valid request.

It was added in https://github.com/etcd-io/etcd/pull/5492 more than 8 years ago.

Actually the Range was also following the same pattern in the first place, but was changed to use linearizableReadNotify later.

https://github.com/etcd-io/etcd/blob/5609fdb9a83beb84cead36672f73332abb864b8a/etcdserver/v3_server.go#L71-L84

serathius commented 2 months ago

Agree request is valid, serving Auth reads by waiting for read index should improve performance of operations, my only concerns are:

Is this change simplifying code or adding complexity? If it adds complexity then what is the use case for the change? Do we need better performance for Auth? or is the current state ok?
Do we have enough test coverage for Auth to feel safe? I don't think Auth is covered by robustness tests, and we had previous issues with auth correctness. Would prefer to invest into testing before we start changing the code.

ahrtr commented 2 months ago

One specific technical concern is that etcd supports MVCC for key space, but not for other buckets, i.e. Auth. Processing Auth read requests (i.e. UserGet, RoleGet, etc.) via raft can guarantee linearizability although it may lead to unnecessary backend queries.

Indeed, we haven't received any complaint on the Auth performance yet. Also K8s doesn't enable auth at all.

So we might not want to make any change for Auth for now.

qsyqian commented 2 months ago

Thanks for the confirm. Agree with your comments that we do not make any change for Auth now. Maybe we can do this if we meet some performance issue for Auth later. Thx again.

qsyqian commented 2 months ago

Close this issue, if anyone has any questions, please feel free to reopen it.

etcd-io / etcd