Eliminate authz plugin disable race #33655
Conversation
Also, this removes the use of a questionable golang range feature which corrects for mutation of a slice during iteration over that slice. This makes the filter operation easier to read and reason about. Signed-off-by: David Sheets <dsheets@docker.com>
Signed-off-by: David Sheets <dsheets@docker.com>
dsheets
force-pushed
the
authz-disable-race
branch
from
c21d542
to
2426469
Compare
| // RemovePlugin removes a single plugin from this authz middleware chain | ||
| func (m *Middleware) RemovePlugin(name string) { | ||
| m.mu.Lock() | ||
| defer m.mu.Unlock() |
There was a problem hiding this comment.
Not sure if performance is important in this part, but the m.mu.Unlock() could be moved to the end of the function instead of using a defer
There was a problem hiding this comment.
golang/go#14939 (comment) seems to indicate that the performance of defer has recently improved and is slated to be optimized further in future releases. As I don't think this method should be particularly hot and defer ensures that all exit paths are handled regardless of future changes, I'm inclined to leave it as-is.
There was a problem hiding this comment.
Ah, yes, I should probably focussing less on defer now. Anyway; it was just a nit 😄
| // SetPlugins sets the plugin used for authorization | ||
| func (m *Middleware) SetPlugins(names []string) { | ||
| m.mu.Lock() | ||
| m.plugins = newPlugins(names) | ||
| m.mu.Unlock() | ||
| } | ||
|
|
||
| // RemovePlugin removes a single plugin from this authz middleware chain | ||
| func (m *Middleware) RemovePlugin(name string) { |
There was a problem hiding this comment.
Wondering if the RemovePlugin name is confusing (as in: could be interpreted as actually removing the plugin). Don't have good suggestions for an alternative though, without becoming very verbose (RemoveFromChain?)
There was a problem hiding this comment.
Perhaps it is confusing. I tend to think it's OK as the receiver will be an authz middleware chain so hopefully that context will make clear the scope (ha) of the removal. Of course, I will defer (ha) to your judgment.
|
ping @cpuguy83 |
- What I did
I eliminated a race during authz plugin removal from the middleware chain. Previously, it was possible for multiple authz plugin disable requests to interleave with some authz plugins marked disabled but still present in the middleware chain (due to overwrites).
- How I did it
I moved the entirety of the authz middleware chain filter into a critical section in an authz middleware chain method. I also simplified the filter operation, removed the now unused
SetAuthzPluginsmethod, and madeGetAuthzPluginsprivategetAuthzPlugins.- How to verify it
The race is hard to trigger and basically impossible to write a reliable and efficient automated test for. The best way to confirm this is correct is careful reasoning and continued test suite success.
- Description for the changelog
I don't think this change is particularly changelog worthy but if you want to mention it:
"Eliminated authz plugin disable race that could result in authz plugins not being actually disabled"
- A picture of a cute animal (not mandatory but encouraged)
Microcebus berthae, Madame Berthe's mouse lemur, is the smallest primate in the world with an average body length of 9.2cm and seasonal weight of 30g. It's only found in one forest in Madagascar and is currently endangered.