secrets: Allow adding additional middlewares #70
Conversation
|
cc @andizzle |
fishy
force-pushed
the
secrets-additional-middleware
branch
from
e39e93e
to
834efa6
Compare
| // AddMiddlewares call is not thread-safe, it should not be called concurrently. | ||
| func (s *Store) AddMiddlewares(middlewares ...SecretMiddleware) { | ||
| s.secretHandler(middlewares...) | ||
| s.secretHandlerFunc(s.getSecrets()) |
There was a problem hiding this comment.
As commented this would also trigger existing middlewares. What do you think have this function accepts SecretHandlerFunc, call them and wrap them as SecretMiddleware?
https://github.com/reddit/baseplate.go/pull/71/files#diff-1a3fa2e612f9f352390e03634cc256d0R82
There was a problem hiding this comment.
triggering existing middlewares shouldn't be a problem, because we are triggering them with the same secret data.
If we want to accept SecretHandlerFunc instead of SecretMiddleware as the arg, we should make it consistent and also SecretMiddleware in NewStore as well (and not export SecretMiddleware at all).
| } | ||
|
|
||
| // GetCredentialSecret loads secrets from watcher, and fetches a credential secret from secrets | ||
| func (s *Store) GetCredentialSecret(path string) (CredentialSecret, error) { | ||
| secrets, ok := s.watcher.Get().(*Secrets) |
There was a problem hiding this comment.
I don't think we should remove the type check. I don't like making assumptions based on upstream code that may or may not change in the future. If the type would be set in stone just a few lines above I'd be okay with it but this is a different package.
There was a problem hiding this comment.
- This is the behavior of stdlib's
atomic.Value, I'm pretty sure that's part of the the go 1 backward compatibility guarantee - We control the parser implementation, and there's no reason we would return a different type in the parser.
There was a problem hiding this comment.
But on these lines I don't see that it's using atomic.Value, it's referencing a different package. It's just defensive programming, doesn't really hurt to keep the practice of checking the type when you type assert. The opposite, does.
There was a problem hiding this comment.
Even though, the second point still stands, that we control what are the values stored in the same package (parser implementation).
There was a problem hiding this comment.
| } | ||
|
|
||
| // TODO 1: Patch upstream to support key rotation natively: | ||
| // TODO: Patch upstream to support key rotation natively: |
There was a problem hiding this comment.
Nice, can we add an issue in our repository to keep track of this?
secrets/store.go
Outdated
| // but can still access the secrets as they were before Stop is called. | ||
| // | ||
| // It's OK to call Stop multiple times. Calls after the first one are no-ops. | ||
| func (s *Store) Stop() { |
There was a problem hiding this comment.
I wonder if we can find a method name that fits the semantic more. I was a bit confused about stopping the store. It doesn't really stop the store but just the updating of the underlying file I guess?
There was a problem hiding this comment.
StopUpdating? Release? Do you have any suggestions?
There was a problem hiding this comment.
Release sounds good or maybe Close?
There was a problem hiding this comment.
In which scenario would this usually be called?
There was a problem hiding this comment.
defer this right after a store is initialized to have an clean exit.
There was a problem hiding this comment.
I mean for instance in production code. I saw the usage in test, I was just wondering it's purpose on the public interface of store :)
There was a problem hiding this comment.
You can (and should) still defer this in the main function in prod code.
There was a problem hiding this comment.
Now where I understand this better I think both Close() or Stop() are fine.
fishy
force-pushed
the
secrets-additional-middleware
branch
from
324b0bd
to
47d28ae
Compare
fishy
force-pushed
the
secrets-additional-middleware
branch
2 times, most recently
from
7c7dc07
to
81f6180
Compare
| } | ||
|
|
||
| // GetCredentialSecret loads secrets from watcher, and fetches a credential secret from secrets | ||
| func (s *Store) GetCredentialSecret(path string) (CredentialSecret, error) { | ||
| secrets, ok := s.watcher.Get().(*Secrets) |
Also: 1. Remove the type check after filewatcher.Get, which would never fail. 2. Add Store.Close()
fishy
force-pushed
the
secrets-additional-middleware
branch
from
81f6180
to
76c32df
Compare
Also: