luxolus
/
open-vault
2
0
Fork 0
Code Issues 1 Pull Requests Releases Activity

Fix various read only storage errors 

* Fix various read only storage errors

A mistake we've seen multiple times in our own plugins and that we've
seen in the GCP plugin now is that control flow (how the code is
structured, helper functions, etc.) can obfuscate whether an error came
from storage or some other Vault-core location (in which case likely it
needs to be a 5XX message) or because of user input (thus 4XX). Error
handling for functions therefore often ends up always treating errors as
either user related or internal.

When the error is logical.ErrReadOnly this means that treating errors as
user errors skips the check that triggers forwarding, instead returning
a read only view error to the user.

While it's obviously more correct to fix that code, it's not always
immediately apparent to reviewers or fixers what the issue is and fixing
it when it's found both requires someone to hit the problem and report
it (thus exposing bugs to users) and selective targeted refactoring that
only helps that one specific case.

If instead we check whether the logical.Response is an error and, if so,
whether it contains the error value, we work around this in all of these
cases automatically. It feels hacky since it's a coding mistake, but
it's one we've made too multiple times, and avoiding bugs altogether is
better for our users.
This commit is contained in:
Jeff Mitchell 2019-07-05 18:12:34 -04:00
parent 19910f6c77
commit ffce5ca702
3 changed files with 6 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
vault/request_handling.go
View File

@ -556,10 +556,8 @@ func isControlGroupRun(req *logical.Request) bool {
func (c *Core) doRouting(ctx context.Context, req *logical.Request) (*logical.Response, error) {
// If we're replicating and we get a read-only error from a backend, need to forward to primary
resp, err := c.router.Route(ctx, req)
if err != nil {
if shouldForward(c, err) {
return forward(ctx, c, req)
}
if shouldForward(c, resp, err) {
return forward(ctx, c, req)
}
atomic.AddUint64(c.counters.requests, 1)
return resp, err

2
vault/request_handling_util.go
View File

@ -19,7 +19,7 @@ func checkErrControlGroupTokenNeedsCreated(err error) bool {
return false
}
func shouldForward(c *Core, routeErr error) bool {
func shouldForward(c *Core, resp *logical.Response, err error) bool {
return false
}

5
vault/rollback.go
View File

@ -224,7 +224,7 @@ func (m *RollbackManager) attemptRollback(ctx context.Context, fullPath string,
var cancelFunc context.CancelFunc
ctx, cancelFunc = context.WithTimeout(ctx, DefaultMaxRequestDuration)
_, err = m.router.Route(ctx, req)
resp, err := m.router.Route(ctx, req)
if grabStatelock && releaseLock {
m.core.stateLock.RUnlock()
}
@ -236,7 +236,8 @@ func (m *RollbackManager) attemptRollback(ctx context.Context, fullPath string,
err = nil
}
// If we failed due to read-only storage, we can't do anything; ignore
if err != nil && strings.Contains(err.Error(), logical.ErrReadOnly.Error()) {
if (err != nil && strings.Contains(err.Error(), logical.ErrReadOnly.Error())) ||
(resp.IsError() && strings.Contains(resp.Error().Error(), logical.ErrReadOnly.Error())) {
err = nil
}
if err != nil {