Description
What would you like to be added?
(Below applies to only 3.6, so current state of the main branch)
After investigating performance problems in #15993 and implementing tests in #16005 we found several things that could be improved. This feature request should track what we want to improve on, also for further discussion on what not.
Opportunities for improvement
1. Testability of the auth package
As seen in #16005, it's currently not even possible to fully mock out the required dependencies to run an authstore. While the changes in the PR are a good start, there can be some more convenience methods around supplying users, passwords and other state necessary to test the auth applier properly.
2. Type conversion in Lease Keys
(Not directly related to auth)
The use of []string lease keys instead of the more common []byte in etcd is causing heavy GC and type conversion costs as seen in #15993.
AFAIU, this has been done in order to use a golang map, which does not work on []byte.
Let's discuss, whether it makes sense to have an alternative map implementation that supports this, or increase the memory usage and store the []byte alongside (eg in a parallel map, even though that might not actually save all/enough conversion costs).
3. Too much coupling in code that's adjacent to Leases
(Not strictly related to auth, also not exhaustive)
One example is that the Lease struct is not fully encapsulated, there are many places where the lock is taken and acquired and the itemSet is entirely replaced or updated.
https://github.com/etcd-io/etcd/blob/main/server/lease/lease.go#L37-L39
This is causing problems, because code using/testing the leases from other packages can't change that state. Going through the lessor requires a fully functioning backend/cluster and is cumbersome to mock.
Just from another PR, #16035 is touching a similar path which seems to be a memory leak and showcases that coupling pretty well.
4. Avoid using auth applier when auth is disabled
Currently the authApplier is part of the uberApplier chain, which is unnecesary overhead when auth isn't even enabled.
Auth can be enabled at runtime, so it makes sense to make it swap in and out depending on the current state.
Controlling whether auth is enabled through this chain allows us to avoid checking the same in the authStore all the time (under a lock in every method).
5. Test coverage in apply_auth.go is zero
As figured in #16005, there are no unit tests, even in the whole package there are none. This is risky, especially because the auth applier contains a lot of ACL logic.
Why is this needed?
I'll leave this up for a general debate, but I think each point individually can be its own ticket. Some of those refactorings can also be good for new joiners to the project, adding tests is always helpful to start with a codebase.