Close all limiters correctly in vault gateway handler#22825
Close all limiters correctly in vault gateway handler#22825prashantkumar1982 wants to merge 2 commits into
Conversation
prashantkumar1982
changed the title
|
✅ No conflicts with other open PRs targeting |
|
I see you updated files related to
|
… into fix/vault-handler-limiter-close
product-security-plaid-production
Bot
requested review from
archseer,
bolekk and
justinkaseman
|
| func newTestRequestValidatorWithFactoryLimiters(t *testing.T) *RequestValidator { | ||
| t.Helper() | ||
|
|
||
| factory := limits.Factory{Settings: cresettings.DefaultGetter} |
There was a problem hiding this comment.
Are you using this getter? The logger could be helpful though:
| factory := limits.Factory{Settings: cresettings.DefaultGetter} | |
| factory := limits.Factory{Logger: logger.Test(t)} |
|
|
||
| func assertBoundLimiterStopped[N limits.Number](t *testing.T, limiter limits.BoundLimiter[N], amount N) { | ||
| t.Helper() | ||
| err := limiter.Check(context.Background(), amount) |
There was a problem hiding this comment.
| err := limiter.Check(context.Background(), amount) | |
| err := limiter.Check(t.Context(), amount) |
| func assertBoundLimiterStopped[N limits.Number](t *testing.T, limiter limits.BoundLimiter[N], amount N) { | ||
| t.Helper() | ||
| err := limiter.Check(context.Background(), amount) | ||
| require.Error(t, err) |
There was a problem hiding this comment.
This needs to check the type of the error via require.ErrorIs since other kinds of errors may occur
| MaxIdentifierNamespaceLengthLimiter limits.BoundLimiter[pkgconfig.Size] | ||
| } | ||
|
|
||
| func (r *RequestValidator) Close() error { |
There was a problem hiding this comment.
We didn't have a Close function here because plugin passes in it's own Limiters and closes them itself. My validation/auth refactor cleans this up and introduces a Close function. I'll have a PR for it later today
There was a problem hiding this comment.
The issue was that the gateway handler wasn't closing these.
So we need to handle the cleanups from here: core/services/gateway/handlers/vault/handler.go
If your PR will fix it, happy to close this one.
3 participants
No description provided.