From f44d7fd359760abe7c70c244ee1dd2ce68b1d2eb Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Mon, 18 May 2020 17:40:02 -0400 Subject: [PATCH 01/23] Add kvMaxValueSize field the raft storage backend --- physical/raft/raft.go | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/physical/raft/raft.go b/physical/raft/raft.go index 4cfc03b2c6f5..a2ce34034129 100644 --- a/physical/raft/raft.go +++ b/physical/raft/raft.go @@ -52,6 +52,8 @@ var ( snapshotsRetained = 2 restoreOpDelayDuration = 5 * time.Second + + defaultKVMaxValueSize = uint64(3 * raftchunking.ChunkSize) ) // RaftBackend implements the backend interfaces and uses the raft protocol to @@ -107,6 +109,10 @@ type RaftBackend struct { // permitPool is used to limit the number of concurrent storage calls. permitPool *physical.PermitPool + + // kvMaxValueSize imposes a limit on values in individual KV operations. It is + // suggested to use a value of 3x the Raft chunking size for optimal performance. + kvMaxValueSize uint64 } // LeaderJoinInfo contains information required by a node to join itself as a @@ -226,6 +232,7 @@ func NewRaftBackend(conf map[string]string, logger log.Logger) (physical.Backend var log raft.LogStore var stable raft.StableStore var snap raft.SnapshotStore + var devMode bool if devMode { store := raft.NewInmemStore() @@ -303,16 +310,27 @@ func NewRaftBackend(conf map[string]string, logger log.Logger) (physical.Backend } } + kvMaxValueSize := defaultKVMaxValueSize + if kvMaxValueSizeCfg := conf["kv_max_value_size"]; len(kvMaxValueSizeCfg) != 0 { + i, err := strconv.Atoi(kvMaxValueSizeCfg) + if err != nil { + return nil, err + } + + kvMaxValueSize = uint64(i) + } + return &RaftBackend{ - logger: logger, - fsm: fsm, - conf: conf, - logStore: log, - stableStore: stable, - snapStore: snap, - dataDir: path, - localID: localID, - permitPool: physical.NewPermitPool(physical.DefaultParallelOperations), + logger: logger, + fsm: fsm, + conf: conf, + logStore: log, + stableStore: stable, + snapStore: snap, + dataDir: path, + localID: localID, + permitPool: physical.NewPermitPool(physical.DefaultParallelOperations), + kvMaxValueSize: kvMaxValueSize, }, nil } From d8e4680e1461003ad2b7f35c89e98184d0459f15 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Mon, 18 May 2020 17:46:45 -0400 Subject: [PATCH 02/23] Add context to error --- physical/raft/raft.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/physical/raft/raft.go b/physical/raft/raft.go index a2ce34034129..3a1bd1e86627 100644 --- a/physical/raft/raft.go +++ b/physical/raft/raft.go @@ -314,7 +314,7 @@ func NewRaftBackend(conf map[string]string, logger log.Logger) (physical.Backend if kvMaxValueSizeCfg := conf["kv_max_value_size"]; len(kvMaxValueSizeCfg) != 0 { i, err := strconv.Atoi(kvMaxValueSizeCfg) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse 'kv_max_value_size': %w", err) } kvMaxValueSize = uint64(i) From 67726b590d31657d3b14701df69a26fc5f91ba4e Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Mon, 18 May 2020 17:54:13 -0400 Subject: [PATCH 03/23] Return error in RaftBackend#Put for value that is too large --- physical/raft/raft.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/physical/raft/raft.go b/physical/raft/raft.go index 3a1bd1e86627..501d54f9019e 100644 --- a/physical/raft/raft.go +++ b/physical/raft/raft.go @@ -937,6 +937,12 @@ func (b *RaftBackend) Get(ctx context.Context, path string) (*physical.Entry, er // Put inserts an entry in the log for the put operation func (b *RaftBackend) Put(ctx context.Context, entry *physical.Entry) error { defer metrics.MeasureSince([]string{"raft-storage", "put"}, time.Now()) + + valueLen := len(entry.Value) + if uint64(valueLen) > b.kvMaxValueSize { + return fmt.Errorf("value size too large; got %d bytes, max: %d bytes", valueLen, b.kvMaxValueSize) + } + command := &LogData{ Operations: []*LogOperation{ &LogOperation{ From ba3ce524b06ae593afb1ae051c3fbdf7a8cbed64 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 19 May 2020 10:34:21 -0400 Subject: [PATCH 04/23] Add physical.ErrValueTooLarge --- physical/raft/raft.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/physical/raft/raft.go b/physical/raft/raft.go index 501d54f9019e..603eede72473 100644 --- a/physical/raft/raft.go +++ b/physical/raft/raft.go @@ -940,7 +940,7 @@ func (b *RaftBackend) Put(ctx context.Context, entry *physical.Entry) error { valueLen := len(entry.Value) if uint64(valueLen) > b.kvMaxValueSize { - return fmt.Errorf("value size too large; got %d bytes, max: %d bytes", valueLen, b.kvMaxValueSize) + return fmt.Errorf("%s; got %d bytes, max: %d bytes", physical.ErrValueTooLarge, valueLen, b.kvMaxValueSize) } command := &LogData{ From 2f101d228d0db60a77dcd1c7b478c63ac458af9a Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 19 May 2020 10:56:49 -0400 Subject: [PATCH 05/23] Add TestRaft_Backend_LargeValue --- physical/raft/raft_test.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/physical/raft/raft_test.go b/physical/raft/raft_test.go index 930e46a40d07..2f72ecc591c3 100644 --- a/physical/raft/raft_test.go +++ b/physical/raft/raft_test.go @@ -7,6 +7,7 @@ import ( fmt "fmt" "io" "io/ioutil" + "math/rand" "os" "path/filepath" "testing" @@ -203,6 +204,22 @@ func TestRaft_Backend(t *testing.T) { physical.ExerciseBackend(t, b) } +func TestRaft_Backend_LargeValue(t *testing.T) { + b, dir := getRaft(t, true, true) + defer os.RemoveAll(dir) + + t.Helper() + + value := make([]byte, defaultKVMaxValueSize+1) + rand.Read(value) + entry := &physical.Entry{Key: "foo", Value: value} + + err := b.Put(context.Background(), entry) + if err == nil { + t.Error("expected error for put entry") + } +} + func TestRaft_Backend_ListPrefix(t *testing.T) { b, dir := getRaft(t, true, true) defer os.RemoveAll(dir) From 808271d8f9f4e2ff169d7f76dc4c659182f2a494 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 19 May 2020 11:01:17 -0400 Subject: [PATCH 06/23] Add case for Get after failed put --- physical/raft/raft_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/physical/raft/raft_test.go b/physical/raft/raft_test.go index 2f72ecc591c3..6c6e749ebb89 100644 --- a/physical/raft/raft_test.go +++ b/physical/raft/raft_test.go @@ -218,6 +218,14 @@ func TestRaft_Backend_LargeValue(t *testing.T) { if err == nil { t.Error("expected error for put entry") } + + out, err := b.Get(context.Background(), entry.Key) + if err != nil { + t.Errorf("unexpected error after failed put: %v", err) + } + if out != nil { + t.Error("expected response entry to be nil after a failed put") + } } func TestRaft_Backend_ListPrefix(t *testing.T) { From 0d6f5134db67fba1f790a084370cab0cb55c9ed2 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 19 May 2020 11:04:15 -0400 Subject: [PATCH 07/23] Add value size check to RaftBackend#applyLog --- physical/raft/raft.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/physical/raft/raft.go b/physical/raft/raft.go index 603eede72473..55214265c44d 100644 --- a/physical/raft/raft.go +++ b/physical/raft/raft.go @@ -934,7 +934,9 @@ func (b *RaftBackend) Get(ctx context.Context, path string) (*physical.Entry, er return b.fsm.Get(ctx, path) } -// Put inserts an entry in the log for the put operation +// Put inserts an entry in the log for the put operation. It will return an +// error if the value exceeds the configured kv_max_value_size or if the call to +// applyLog fails. func (b *RaftBackend) Put(ctx context.Context, entry *physical.Entry) error { defer metrics.MeasureSince([]string{"raft-storage", "put"}, time.Now()) @@ -1016,6 +1018,13 @@ func (b *RaftBackend) applyLog(ctx context.Context, command *LogData) error { return errors.New("raft storage backend is not initialized") } + for _, op := range command.Operations { + valueLen := len(op.Value) + if uint64(valueLen) > b.kvMaxValueSize { + return fmt.Errorf("%s; got %d bytes, max: %d bytes", physical.ErrValueTooLarge, valueLen, b.kvMaxValueSize) + } + } + commandBytes, err := proto.Marshal(command) if err != nil { return err From 366721b9205fb30c6f7e822afce7fbd27c79019d Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 19 May 2020 12:42:47 -0400 Subject: [PATCH 08/23] Implement TestRaft_TransactionalBackend_LargeValue --- physical/raft/raft_test.go | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/physical/raft/raft_test.go b/physical/raft/raft_test.go index 6c6e749ebb89..d54ac8c4c626 100644 --- a/physical/raft/raft_test.go +++ b/physical/raft/raft_test.go @@ -214,8 +214,7 @@ func TestRaft_Backend_LargeValue(t *testing.T) { rand.Read(value) entry := &physical.Entry{Key: "foo", Value: value} - err := b.Put(context.Background(), entry) - if err == nil { + if err := b.Put(context.Background(), entry); err == nil { t.Error("expected error for put entry") } @@ -228,6 +227,38 @@ func TestRaft_Backend_LargeValue(t *testing.T) { } } +func TestRaft_TransactionalBackend_LargeValue(t *testing.T) { + b, dir := getRaft(t, true, true) + defer os.RemoveAll(dir) + + t.Helper() + + value := make([]byte, defaultKVMaxValueSize+1) + rand.Read(value) + + txns := []*physical.TxnEntry{ + &physical.TxnEntry{ + Operation: physical.PutOperation, + Entry: &physical.Entry{ + Key: "foo", + Value: value, + }, + }, + } + + if err := b.Transaction(context.Background(), txns); err == nil { + t.Error("expected error for transactions") + } + + out, err := b.Get(context.Background(), txns[0].Entry.Key) + if err != nil { + t.Errorf("unexpected error after failed put: %v", err) + } + if out != nil { + t.Error("expected response entry to be nil after a failed put") + } +} + func TestRaft_Backend_ListPrefix(t *testing.T) { b, dir := getRaft(t, true, true) defer os.RemoveAll(dir) From c2fd553ae1ffc1cca9ff8579fb7c2ddd0cb44ef2 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 19 May 2020 15:14:10 -0400 Subject: [PATCH 09/23] Add kv_max_value_size section to raft storage doc --- website/pages/docs/configuration/storage/raft.mdx | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/website/pages/docs/configuration/storage/raft.mdx b/website/pages/docs/configuration/storage/raft.mdx index e3bad53b8edb..6f16ad6237fb 100644 --- a/website/pages/docs/configuration/storage/raft.mdx +++ b/website/pages/docs/configuration/storage/raft.mdx @@ -94,6 +94,16 @@ time. To use Raft for HA coordination users must also use Raft for storage. still need to be unsealed manually. See the section below that describes the parameters accepted by the `retry_join` stanza. +- `kv_max_value_size` `(integer: 1572864)` - This configures the maximum number of + bytes for a value in a Raft entry. It applies to both Put operations and transactions. + Any value size in an entry or transaction exceeding this configuration value + will cause the respective operation to fail. Raft has a suggested max size of + data in a raft log entry. This is based on current architecture, default timing, + etc. Integrated storage also uses a chunk size that is the threshold used for + breaking a large value into chunks. By default, the chunk size is the same as + raft's max size log entry. The default value for this configuration is 1572864 + -- three times the chunking size. + ### `retry_join` stanza - `leader_api_addr` `(string: "")` - Address of a possible leader node. From b279a904bf00c162a73245114e5c47301b69217c Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 19 May 2020 15:24:28 -0400 Subject: [PATCH 10/23] Add warn log to Get --- physical/raft/raft.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/physical/raft/raft.go b/physical/raft/raft.go index 55214265c44d..cef50c4492c7 100644 --- a/physical/raft/raft.go +++ b/physical/raft/raft.go @@ -931,7 +931,18 @@ func (b *RaftBackend) Get(ctx context.Context, path string) (*physical.Entry, er b.permitPool.Acquire() defer b.permitPool.Release() - return b.fsm.Get(ctx, path) + entry, err := b.fsm.Get(ctx, path) + if entry != nil { + valueLen := len(entry.Value) + if uint64(valueLen) > b.kvMaxValueSize { + b.logger.Warn( + "retrieved entry value is too large. See https://www.vaultproject.io/docs/configuration/storage/raft#raft-parameters", + "size", valueLen, "suggested", b.kvMaxValueSize, + ) + } + } + + return entry, err } // Put inserts an entry in the log for the put operation. It will return an From 93318b99d7425387d4a4a52aa975afc24e038ad3 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 19 May 2020 16:14:31 -0400 Subject: [PATCH 11/23] update log --- physical/raft/raft.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/physical/raft/raft.go b/physical/raft/raft.go index cef50c4492c7..6d678fcb6a05 100644 --- a/physical/raft/raft.go +++ b/physical/raft/raft.go @@ -936,7 +936,7 @@ func (b *RaftBackend) Get(ctx context.Context, path string) (*physical.Entry, er valueLen := len(entry.Value) if uint64(valueLen) > b.kvMaxValueSize { b.logger.Warn( - "retrieved entry value is too large. See https://www.vaultproject.io/docs/configuration/storage/raft#raft-parameters", + "retrieved entry value is too large; see https://www.vaultproject.io/docs/configuration/storage/raft#raft-parameters", "size", valueLen, "suggested", b.kvMaxValueSize, ) } From 9f4fea4ce55027f9d2f3991a079ece09e8c46b21 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 19 May 2020 16:31:29 -0400 Subject: [PATCH 12/23] Use fatal instead of error --- physical/raft/raft_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/physical/raft/raft_test.go b/physical/raft/raft_test.go index d54ac8c4c626..baa55f2cac43 100644 --- a/physical/raft/raft_test.go +++ b/physical/raft/raft_test.go @@ -215,15 +215,15 @@ func TestRaft_Backend_LargeValue(t *testing.T) { entry := &physical.Entry{Key: "foo", Value: value} if err := b.Put(context.Background(), entry); err == nil { - t.Error("expected error for put entry") + t.Fatal("expected error for put entry") } out, err := b.Get(context.Background(), entry.Key) if err != nil { - t.Errorf("unexpected error after failed put: %v", err) + t.Fatalf("unexpected error after failed put: %v", err) } if out != nil { - t.Error("expected response entry to be nil after a failed put") + t.Fatal("expected response entry to be nil after a failed put") } } @@ -247,15 +247,15 @@ func TestRaft_TransactionalBackend_LargeValue(t *testing.T) { } if err := b.Transaction(context.Background(), txns); err == nil { - t.Error("expected error for transactions") + t.Fatal("expected error for transactions") } out, err := b.Get(context.Background(), txns[0].Entry.Key) if err != nil { - t.Errorf("unexpected error after failed put: %v", err) + t.Fatalf("unexpected error after failed put: %v", err) } if out != nil { - t.Error("expected response entry to be nil after a failed put") + t.Fatal("expected response entry to be nil after a failed put") } } From 77dc11ea3d520533e835bc0101c9e7c63facc55b Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Wed, 20 May 2020 12:08:55 -0400 Subject: [PATCH 13/23] cl++ --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 42ab6c89324f..90ceea5c0abf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ CHANGES: +* storage/raft: The storage configuration now accepts a new `kv_max_value_size` config that will limit + the total size in bytes of any entry commited via raft. It defaults to `"1572864"` -- three times the chunking size. [[GH-9027](https://github.com/hashicorp/vault/pull/9027)] * token: Token creation with custom token ID via `id` will no longer allow periods (`.`) as part of the input string. The final generated token value may contain periods, such as the `s.` prefix for service token indication. [[GH-8646](https://github.com/hashicorp/vault/pull/8646/files)] From 2f5913dc958c1870b3c7ee594a2749b8a8dc9af8 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Wed, 20 May 2020 18:37:26 -0400 Subject: [PATCH 14/23] Change config value --- CHANGELOG.md | 2 +- physical/raft/raft.go | 63 +++++++++---------- physical/raft/raft_test.go | 19 ++++-- .../pages/docs/configuration/storage/raft.mdx | 12 ++-- 4 files changed, 50 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 77d878c0a385..ad4af498da35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ CHANGES: -* storage/raft: The storage configuration now accepts a new `kv_max_value_size` config that will limit +* storage/raft: The storage configuration now accepts a new `max_entry_size` config that will limit the total size in bytes of any entry commited via raft. It defaults to `"1572864"` -- three times the chunking size. [[GH-9027](https://github.com/hashicorp/vault/pull/9027)] * token: Token creation with custom token ID via `id` will no longer allow periods (`.`) as part of the input string. The final generated token value may contain periods, such as the `s.` prefix for service token diff --git a/physical/raft/raft.go b/physical/raft/raft.go index 828a23b799c5..57106bd94513 100644 --- a/physical/raft/raft.go +++ b/physical/raft/raft.go @@ -53,7 +53,7 @@ var ( restoreOpDelayDuration = 5 * time.Second - defaultKVMaxValueSize = uint64(3 * raftchunking.ChunkSize) + defaultMaxEntrySize = uint64(3 * raftchunking.ChunkSize) ) // RaftBackend implements the backend interfaces and uses the raft protocol to @@ -110,9 +110,10 @@ type RaftBackend struct { // permitPool is used to limit the number of concurrent storage calls. permitPool *physical.PermitPool - // kvMaxValueSize imposes a limit on values in individual KV operations. It is - // suggested to use a value of 3x the Raft chunking size for optimal performance. - kvMaxValueSize uint64 + // maxEntrySize imposes a size limit (in bytes) on a raft entry (put or transaction). + // It is suggested to use a value of 3x the Raft chunking size for optimal + // performance. + maxEntrySize uint64 } // LeaderJoinInfo contains information required by a node to join itself as a @@ -310,27 +311,27 @@ func NewRaftBackend(conf map[string]string, logger log.Logger) (physical.Backend } } - kvMaxValueSize := defaultKVMaxValueSize - if kvMaxValueSizeCfg := conf["kv_max_value_size"]; len(kvMaxValueSizeCfg) != 0 { - i, err := strconv.Atoi(kvMaxValueSizeCfg) + maxEntrySize := defaultMaxEntrySize + if maxEntrySizeCfg := conf["max_entry_size"]; len(maxEntrySizeCfg) != 0 { + i, err := strconv.Atoi(maxEntrySizeCfg) if err != nil { - return nil, fmt.Errorf("failed to parse 'kv_max_value_size': %w", err) + return nil, fmt.Errorf("failed to parse 'max_entry_size': %w", err) } - kvMaxValueSize = uint64(i) + maxEntrySize = uint64(i) } return &RaftBackend{ - logger: logger, - fsm: fsm, - conf: conf, - logStore: log, - stableStore: stable, - snapStore: snap, - dataDir: path, - localID: localID, - permitPool: physical.NewPermitPool(physical.DefaultParallelOperations), - kvMaxValueSize: kvMaxValueSize, + logger: logger, + fsm: fsm, + conf: conf, + logStore: log, + stableStore: stable, + snapStore: snap, + dataDir: path, + localID: localID, + permitPool: physical.NewPermitPool(physical.DefaultParallelOperations), + maxEntrySize: maxEntrySize, }, nil } @@ -946,10 +947,10 @@ func (b *RaftBackend) Get(ctx context.Context, path string) (*physical.Entry, er entry, err := b.fsm.Get(ctx, path) if entry != nil { valueLen := len(entry.Value) - if uint64(valueLen) > b.kvMaxValueSize { + if uint64(valueLen) > b.maxEntrySize { b.logger.Warn( "retrieved entry value is too large; see https://www.vaultproject.io/docs/configuration/storage/raft#raft-parameters", - "size", valueLen, "suggested", b.kvMaxValueSize, + "size", valueLen, "suggested", b.maxEntrySize, ) } } @@ -958,16 +959,10 @@ func (b *RaftBackend) Get(ctx context.Context, path string) (*physical.Entry, er } // Put inserts an entry in the log for the put operation. It will return an -// error if the value exceeds the configured kv_max_value_size or if the call to +// error if the value exceeds the configured max_entry_size or if the call to // applyLog fails. func (b *RaftBackend) Put(ctx context.Context, entry *physical.Entry) error { defer metrics.MeasureSince([]string{"raft-storage", "put"}, time.Now()) - - valueLen := len(entry.Value) - if uint64(valueLen) > b.kvMaxValueSize { - return fmt.Errorf("%s; got %d bytes, max: %d bytes", physical.ErrValueTooLarge, valueLen, b.kvMaxValueSize) - } - command := &LogData{ Operations: []*LogOperation{ &LogOperation{ @@ -1041,18 +1036,16 @@ func (b *RaftBackend) applyLog(ctx context.Context, command *LogData) error { return errors.New("raft storage backend is not initialized") } - for _, op := range command.Operations { - valueLen := len(op.Value) - if uint64(valueLen) > b.kvMaxValueSize { - return fmt.Errorf("%s; got %d bytes, max: %d bytes", physical.ErrValueTooLarge, valueLen, b.kvMaxValueSize) - } - } - commandBytes, err := proto.Marshal(command) if err != nil { return err } + cmdSize := len(commandBytes) + if uint64(cmdSize) > b.maxEntrySize { + return fmt.Errorf("%s; got %d bytes, max: %d bytes", physical.ErrValueTooLarge, cmdSize, b.maxEntrySize) + } + var chunked bool var applyFuture raft.ApplyFuture switch { diff --git a/physical/raft/raft_test.go b/physical/raft/raft_test.go index baa55f2cac43..04bdd8a263d1 100644 --- a/physical/raft/raft_test.go +++ b/physical/raft/raft_test.go @@ -10,6 +10,7 @@ import ( "math/rand" "os" "path/filepath" + "strings" "testing" "time" @@ -210,14 +211,19 @@ func TestRaft_Backend_LargeValue(t *testing.T) { t.Helper() - value := make([]byte, defaultKVMaxValueSize+1) + value := make([]byte, defaultMaxEntrySize+1) rand.Read(value) entry := &physical.Entry{Key: "foo", Value: value} - if err := b.Put(context.Background(), entry); err == nil { + err := b.Put(context.Background(), entry) + if err == nil { t.Fatal("expected error for put entry") } + if !strings.Contains(err.Error(), physical.ErrValueTooLarge) { + t.Fatalf("expected %q, got %v", physical.ErrValueTooLarge, err) + } + out, err := b.Get(context.Background(), entry.Key) if err != nil { t.Fatalf("unexpected error after failed put: %v", err) @@ -233,7 +239,7 @@ func TestRaft_TransactionalBackend_LargeValue(t *testing.T) { t.Helper() - value := make([]byte, defaultKVMaxValueSize+1) + value := make([]byte, defaultMaxEntrySize+1) rand.Read(value) txns := []*physical.TxnEntry{ @@ -246,10 +252,15 @@ func TestRaft_TransactionalBackend_LargeValue(t *testing.T) { }, } - if err := b.Transaction(context.Background(), txns); err == nil { + err := b.Transaction(context.Background(), txns) + if err == nil { t.Fatal("expected error for transactions") } + if !strings.Contains(err.Error(), physical.ErrValueTooLarge) { + t.Fatalf("expected %q, got %v", physical.ErrValueTooLarge, err) + } + out, err := b.Get(context.Background(), txns[0].Entry.Key) if err != nil { t.Fatalf("unexpected error after failed put: %v", err) diff --git a/website/pages/docs/configuration/storage/raft.mdx b/website/pages/docs/configuration/storage/raft.mdx index 6f16ad6237fb..fb3519f764e0 100644 --- a/website/pages/docs/configuration/storage/raft.mdx +++ b/website/pages/docs/configuration/storage/raft.mdx @@ -94,12 +94,12 @@ time. To use Raft for HA coordination users must also use Raft for storage. still need to be unsealed manually. See the section below that describes the parameters accepted by the `retry_join` stanza. -- `kv_max_value_size` `(integer: 1572864)` - This configures the maximum number of - bytes for a value in a Raft entry. It applies to both Put operations and transactions. - Any value size in an entry or transaction exceeding this configuration value - will cause the respective operation to fail. Raft has a suggested max size of - data in a raft log entry. This is based on current architecture, default timing, - etc. Integrated storage also uses a chunk size that is the threshold used for +- `max_entry_size` `(integer: 1572864)` - This configures the maximum number of + bytes for a raft entry. It applies to both Put operations and transactions. + Any put or transaction operation exceeding this configuration value will cause + the respective operation to fail. Raft has a suggested max size of data in a + raft log entry. This is based on current architecture, default timing, etc. + Integrated storage also uses a chunk size that is the threshold used for breaking a large value into chunks. By default, the chunk size is the same as raft's max size log entry. The default value for this configuration is 1572864 -- three times the chunking size. From 2d2b9f9cae450ff2d97bb3c5c537d83c6bfdcf0d Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Wed, 20 May 2020 18:41:42 -0400 Subject: [PATCH 15/23] Update godoc --- physical/raft/raft.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/physical/raft/raft.go b/physical/raft/raft.go index 57106bd94513..0e1fb70f9566 100644 --- a/physical/raft/raft.go +++ b/physical/raft/raft.go @@ -959,8 +959,8 @@ func (b *RaftBackend) Get(ctx context.Context, path string) (*physical.Entry, er } // Put inserts an entry in the log for the put operation. It will return an -// error if the value exceeds the configured max_entry_size or if the call to -// applyLog fails. +// error if the resulting entry encoding exceeds the configured max_entry_size +// or if the call to applyLog fails. func (b *RaftBackend) Put(ctx context.Context, entry *physical.Entry) error { defer metrics.MeasureSince([]string{"raft-storage", "put"}, time.Now()) command := &LogData{ From ee7f8d34ba65bfb9d5a2d4c962cdc7e7219818f5 Mon Sep 17 00:00:00 2001 From: Alexander Bezobchuk Date: Tue, 26 May 2020 13:06:56 -0400 Subject: [PATCH 16/23] Update physical/raft/raft_test.go Co-authored-by: Calvin Leung Huang --- physical/raft/raft_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/physical/raft/raft_test.go b/physical/raft/raft_test.go index 04bdd8a263d1..a52563dc8fe3 100644 --- a/physical/raft/raft_test.go +++ b/physical/raft/raft_test.go @@ -209,7 +209,6 @@ func TestRaft_Backend_LargeValue(t *testing.T) { b, dir := getRaft(t, true, true) defer os.RemoveAll(dir) - t.Helper() value := make([]byte, defaultMaxEntrySize+1) rand.Read(value) From bc63c8e3d3e546db63848e707e31dddb01f101d3 Mon Sep 17 00:00:00 2001 From: Alexander Bezobchuk Date: Tue, 26 May 2020 13:07:12 -0400 Subject: [PATCH 17/23] Update physical/raft/raft_test.go Co-authored-by: Calvin Leung Huang --- physical/raft/raft_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/physical/raft/raft_test.go b/physical/raft/raft_test.go index a52563dc8fe3..2a1d1375e2b5 100644 --- a/physical/raft/raft_test.go +++ b/physical/raft/raft_test.go @@ -236,7 +236,6 @@ func TestRaft_TransactionalBackend_LargeValue(t *testing.T) { b, dir := getRaft(t, true, true) defer os.RemoveAll(dir) - t.Helper() value := make([]byte, defaultMaxEntrySize+1) rand.Read(value) From 5c71cfb3cce6e9331c2eabc091a6f6c72713f3fc Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Wed, 27 May 2020 14:52:07 -0400 Subject: [PATCH 18/23] Change default to 2x --- physical/raft/raft.go | 4 ++-- physical/raft/raft_test.go | 2 -- website/pages/docs/configuration/storage/raft.mdx | 6 +++--- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/physical/raft/raft.go b/physical/raft/raft.go index 0e1fb70f9566..d240d13713df 100644 --- a/physical/raft/raft.go +++ b/physical/raft/raft.go @@ -53,7 +53,7 @@ var ( restoreOpDelayDuration = 5 * time.Second - defaultMaxEntrySize = uint64(3 * raftchunking.ChunkSize) + defaultMaxEntrySize = uint64(2 * raftchunking.ChunkSize) ) // RaftBackend implements the backend interfaces and uses the raft protocol to @@ -111,7 +111,7 @@ type RaftBackend struct { permitPool *physical.PermitPool // maxEntrySize imposes a size limit (in bytes) on a raft entry (put or transaction). - // It is suggested to use a value of 3x the Raft chunking size for optimal + // It is suggested to use a value of 2x the Raft chunking size for optimal // performance. maxEntrySize uint64 } diff --git a/physical/raft/raft_test.go b/physical/raft/raft_test.go index 2a1d1375e2b5..3a072d0a7635 100644 --- a/physical/raft/raft_test.go +++ b/physical/raft/raft_test.go @@ -209,7 +209,6 @@ func TestRaft_Backend_LargeValue(t *testing.T) { b, dir := getRaft(t, true, true) defer os.RemoveAll(dir) - value := make([]byte, defaultMaxEntrySize+1) rand.Read(value) entry := &physical.Entry{Key: "foo", Value: value} @@ -236,7 +235,6 @@ func TestRaft_TransactionalBackend_LargeValue(t *testing.T) { b, dir := getRaft(t, true, true) defer os.RemoveAll(dir) - value := make([]byte, defaultMaxEntrySize+1) rand.Read(value) diff --git a/website/pages/docs/configuration/storage/raft.mdx b/website/pages/docs/configuration/storage/raft.mdx index fb3519f764e0..b16b130c6611 100644 --- a/website/pages/docs/configuration/storage/raft.mdx +++ b/website/pages/docs/configuration/storage/raft.mdx @@ -94,15 +94,15 @@ time. To use Raft for HA coordination users must also use Raft for storage. still need to be unsealed manually. See the section below that describes the parameters accepted by the `retry_join` stanza. -- `max_entry_size` `(integer: 1572864)` - This configures the maximum number of +- `max_entry_size` `(integer: 1048576)` - This configures the maximum number of bytes for a raft entry. It applies to both Put operations and transactions. Any put or transaction operation exceeding this configuration value will cause the respective operation to fail. Raft has a suggested max size of data in a raft log entry. This is based on current architecture, default timing, etc. Integrated storage also uses a chunk size that is the threshold used for breaking a large value into chunks. By default, the chunk size is the same as - raft's max size log entry. The default value for this configuration is 1572864 - -- three times the chunking size. + raft's max size log entry. The default value for this configuration is 1048576 + -- two times the chunking size. ### `retry_join` stanza From fcb4201694b70acb044f4c87445a5e571301246e Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Wed, 27 May 2020 15:06:26 -0400 Subject: [PATCH 19/23] Add entry size sample metric --- physical/raft/raft.go | 2 ++ physical/raft/raft_test.go | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/physical/raft/raft.go b/physical/raft/raft.go index d240d13713df..5797bfcca2b7 100644 --- a/physical/raft/raft.go +++ b/physical/raft/raft.go @@ -1046,6 +1046,8 @@ func (b *RaftBackend) applyLog(ctx context.Context, command *LogData) error { return fmt.Errorf("%s; got %d bytes, max: %d bytes", physical.ErrValueTooLarge, cmdSize, b.maxEntrySize) } + defer metrics.AddSample([]string{"raft-storage", "entrySize"}, float32(cmdSize)) + var chunked bool var applyFuture raft.ApplyFuture switch { diff --git a/physical/raft/raft_test.go b/physical/raft/raft_test.go index 3a072d0a7635..b1c85395e0ca 100644 --- a/physical/raft/raft_test.go +++ b/physical/raft/raft_test.go @@ -4,7 +4,7 @@ import ( "context" "crypto/md5" "encoding/base64" - fmt "fmt" + "fmt" "io" "io/ioutil" "math/rand" From f42e2d3b8b61ee6023281c733962efe91ee7e004 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Thu, 28 May 2020 10:17:38 -0400 Subject: [PATCH 20/23] Add metric to docs --- website/pages/docs/internals/telemetry.mdx | 1 + 1 file changed, 1 insertion(+) diff --git a/website/pages/docs/internals/telemetry.mdx b/website/pages/docs/internals/telemetry.mdx index 393022f09323..57c2c441fbb0 100644 --- a/website/pages/docs/internals/telemetry.mdx +++ b/website/pages/docs/internals/telemetry.mdx @@ -374,6 +374,7 @@ These metrics relate to raft based [integrated storage][integrated-storage]. | `vault.raft-storage.put` | Time to insert log entry to persist path. | ms | timer | | `vault.raft-storage.list` | Time to list all entries under the prefix from the FSM. | ms | timer | | `vault.raft-storage.transaction` | Time to insert operations into a single log. | ms | timer | +| `vault.raft-storage.entrySize` | The total size of a Raft entry during log application in bytes. | bytes | sample | ## Integrated Raft Storage Leadership Changes From 80eef73dfaa3d00258cff3274d570b8a46b2b92d Mon Sep 17 00:00:00 2001 From: Alexander Bezobchuk Date: Fri, 29 May 2020 09:36:01 -0400 Subject: [PATCH 21/23] Update physical/raft/raft.go Co-authored-by: Brian Kassouf --- physical/raft/raft.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/physical/raft/raft.go b/physical/raft/raft.go index 5797bfcca2b7..c8535ea7cd23 100644 --- a/physical/raft/raft.go +++ b/physical/raft/raft.go @@ -1046,7 +1046,7 @@ func (b *RaftBackend) applyLog(ctx context.Context, command *LogData) error { return fmt.Errorf("%s; got %d bytes, max: %d bytes", physical.ErrValueTooLarge, cmdSize, b.maxEntrySize) } - defer metrics.AddSample([]string{"raft-storage", "entrySize"}, float32(cmdSize)) + defer metrics.AddSample([]string{"raft-storage", "entry_size"}, float32(cmdSize)) var chunked bool var applyFuture raft.ApplyFuture From f859f3a31e8f8c9ced3e3982e52d97389096c645 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 29 May 2020 09:37:31 -0400 Subject: [PATCH 22/23] Update entry_size references --- website/pages/docs/internals/telemetry.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/pages/docs/internals/telemetry.mdx b/website/pages/docs/internals/telemetry.mdx index 57c2c441fbb0..66d182087801 100644 --- a/website/pages/docs/internals/telemetry.mdx +++ b/website/pages/docs/internals/telemetry.mdx @@ -374,7 +374,7 @@ These metrics relate to raft based [integrated storage][integrated-storage]. | `vault.raft-storage.put` | Time to insert log entry to persist path. | ms | timer | | `vault.raft-storage.list` | Time to list all entries under the prefix from the FSM. | ms | timer | | `vault.raft-storage.transaction` | Time to insert operations into a single log. | ms | timer | -| `vault.raft-storage.entrySize` | The total size of a Raft entry during log application in bytes. | bytes | sample | +| `vault.raft-storage.entry_size` | The total size of a Raft entry during log application in bytes. | bytes | sample | ## Integrated Raft Storage Leadership Changes From a2818368d5957a946c691258a0f28dc7bc9a758d Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 29 May 2020 10:58:03 -0400 Subject: [PATCH 23/23] cl++ --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 69378e50d072..c4ab9cf64280 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ CHANGES: * storage/raft: The storage configuration now accepts a new `max_entry_size` config that will limit - the total size in bytes of any entry commited via raft. It defaults to `"1572864"` -- three times the chunking size. [[GH-9027](https://github.com/hashicorp/vault/pull/9027)] + the total size in bytes of any entry committed via raft. It defaults to `"1048576"` (1MiB). [[GH-9027](https://github.com/hashicorp/vault/pull/9027)] * token: Token creation with custom token ID via `id` will no longer allow periods (`.`) as part of the input string. The final generated token value may contain periods, such as the `s.` prefix for service token indication. [[GH-8646](https://github.com/hashicorp/vault/pull/8646/files)]