diff --git a/changelog/22235.txt b/changelog/22235.txt new file mode 100644 index 000000000..3d62e70cb --- /dev/null +++ b/changelog/22235.txt @@ -0,0 +1,3 @@ +```release-note:improvement +core: Log rollback manager failures during unmount, remount to prevent replication failures on secondary clusters. +``` diff --git a/vault/external_tests/router/router_ext_test.go b/vault/external_tests/router/router_ext_test.go index 8b9c9d50c..da8241283 100644 --- a/vault/external_tests/router/router_ext_test.go +++ b/vault/external_tests/router/router_ext_test.go @@ -68,3 +68,41 @@ func testRouter_MountSubpath(t *testing.T, mountPoints []string) { cluster.UnsealCores(t) t.Logf("Done: %#v", mountPoints) } + +func TestRouter_UnmountRollbackIsntFatal(t *testing.T) { + coreConfig := &vault.CoreConfig{ + LogicalBackends: map[string]logical.Factory{ + "noop": vault.NoopBackendRollbackErrFactory, + }, + } + cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{ + HandlerFunc: vaulthttp.Handler, + }) + cluster.Start() + defer cluster.Cleanup() + + vault.TestWaitActive(t, cluster.Cores[0].Core) + client := cluster.Cores[0].Client + + if err := client.Sys().Mount("noop", &api.MountInput{ + Type: "noop", + }); err != nil { + t.Fatalf("failed to mount PKI: %v", err) + } + + if _, err := client.Logical().Write("sys/plugins/reload/backend", map[string]interface{}{ + "mounts": "noop", + }); err != nil { + t.Fatalf("expected reload of noop with broken periodic func to succeed; got err=%v", err) + } + + if _, err := client.Logical().Write("sys/remount", map[string]interface{}{ + "from": "noop", + "to": "noop-to", + }); err != nil { + t.Fatalf("expected remount of noop with broken periodic func to succeed; got err=%v", err) + } + + cluster.EnsureCoresSealed(t) + cluster.UnsealCores(t) +} diff --git a/vault/mount.go b/vault/mount.go index b69aba39e..4d2005b1a 100644 --- a/vault/mount.go +++ b/vault/mount.go @@ -874,9 +874,13 @@ func (c *Core) unmountInternal(ctx context.Context, path string, updateStorage b rCtx := namespace.ContextWithNamespace(c.activeContext, ns) if backend != nil && c.rollback != nil { - // Invoke the rollback manager a final time + // Invoke the rollback manager a final time. This is not fatal as + // various periodic funcs (e.g., PKI) can legitimately error; the + // periodic rollback manager logs these errors rather than failing + // replication like returning this error would do. if err := c.rollback.Rollback(rCtx, path); err != nil { - return err + c.logger.Error("ignoring rollback error during unmount", "error", err, "path", path) + err = nil } } if backend != nil && c.expiration != nil && updateStorage { @@ -1142,11 +1146,15 @@ func (c *Core) remountSecretsEngine(ctx context.Context, src, dst namespace.Moun } if !c.IsDRSecondary() { - // Invoke the rollback manager a final time + // Invoke the rollback manager a final time. This is not fatal as + // various periodic funcs (e.g., PKI) can legitimately error; the + // periodic rollback manager logs these errors rather than failing + // replication like returning this error would do. rCtx := namespace.ContextWithNamespace(c.activeContext, ns) if c.rollback != nil && c.router.MatchingBackend(ctx, srcRelativePath) != nil { if err := c.rollback.Rollback(rCtx, srcRelativePath); err != nil { - return err + c.logger.Error("ignoring rollback error during remount", "error", err, "path", src.Namespace.Path+src.MountPath) + err = nil } } diff --git a/vault/router_testing.go b/vault/router_testing.go index 78f84a6e6..56e186309 100644 --- a/vault/router_testing.go +++ b/vault/router_testing.go @@ -29,17 +29,27 @@ type NoopBackend struct { DefaultLeaseTTL time.Duration MaxLeaseTTL time.Duration BackendType logical.BackendType + + RollbackErrs bool } func NoopBackendFactory(_ context.Context, _ *logical.BackendConfig) (logical.Backend, error) { return &NoopBackend{}, nil } +func NoopBackendRollbackErrFactory(_ context.Context, _ *logical.BackendConfig) (logical.Backend, error) { + return &NoopBackend{RollbackErrs: true}, nil +} + func (n *NoopBackend) HandleRequest(ctx context.Context, req *logical.Request) (*logical.Response, error) { if req.TokenEntry() != nil { panic("got a non-nil TokenEntry") } + if n.RollbackErrs && req.Operation == "rollback" { + return nil, fmt.Errorf("no-op backend rollback has erred out") + } + var err error resp := n.Response if n.RequestHandler != nil {