-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Gateway Node proxies user requests to Vault Node #18420
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Gateway Node proxies user requests to Vault Node #18420
Conversation
9b22a5d to
c80c729
Compare
…way-vault-flow-with-mocks
100cde6 to
188b76b
Compare
188b76b to
b4437b5
Compare
| cfg.RequestTimeoutSec = 30 | ||
| } | ||
|
|
||
| userRateLimiter, _ := ratelimit.NewRateLimiter(cfg.UserRateLimiterConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the second value that is being omitted here? (Not an error I hope 😬 ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is error. we should handle it
| ErrNotAllowlisted = errors.New("sender not allowlisted") | ||
| ErrRateLimited = errors.New("rate-limited") | ||
|
|
||
| promRequestInternalError = promauto.NewCounterVec(prometheus.CounterOpts{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use beholder rather than prometheus :)
|
|
||
| userRateLimiter *ratelimit.RateLimiter | ||
| nodeRateLimiter *ratelimit.RateLimiter | ||
| requestTimeoutSec int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use a time.Duration here directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the timeout being used or enforced in our logic.
The gateway has a separate timeout for user request anyway.
If we want to use this timeout to just keep our pendingRequests queue bounded, we will need to implement a cleanup on timeout.
Currently the timeout is just used on a context when calling the don.SendToNode() method, but that doesn't affect our queue here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the timeout to affect the queue instead of just the don.SendToNode. This also effectively prunes the queue.
e0eabae to
50b9b60
Compare
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌2 Results
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
|
| func (h *handler) Start(ctx context.Context) error { | ||
| return h.StartOnce("VaultHandler", func() error { | ||
| h.lggr.Info("starting vault handler") | ||
| ctx, _ := h.stopCh.NewCtx() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should call the cancel func by deferring it inside the go func on line 145
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(And maybe calling <-ctx.Done() on line 151 for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll address this in a follow up PR




This PR introduces a new
VaultHandlerimplementation and updates to error handling.TODOs