diff --git a/changelog/17.0/17.0.0/summary.md b/changelog/17.0/17.0.0/summary.md index 9b761967c5b..690f0678c92 100644 --- a/changelog/17.0/17.0.0/summary.md +++ b/changelog/17.0/17.0.0/summary.md @@ -6,6 +6,7 @@ - **[Breaking Changes](#breaking-changes)** - [Dedicated stats for VTGate Prepare operations](#dedicated-vtgate-prepare-stats) - [Keyspace name validation in TopoServer](#keyspace-name-validation) + - [Shard name validation in TopoServer](#shard-name-validation) - **[New command line flags and behavior](#new-flag)** - [Builtin backup: read buffering flags](#builtin-backup-read-buffering-flags) - **[New stats](#new-stats)** @@ -58,6 +59,12 @@ Prior to v17, it was possible to create a keyspace with invalid characters, whic Keyspace names may no longer contain the forward slash ("/") character, and TopoServer's `GetKeyspace` and `CreateKeyspace` methods return an error if given such a name. +#### Shard name validation in TopoServer + +Prior to v17, it was possible to create a shard name with invalid characters, which would then be inaccessible to various cluster management operations. + +Shard names may no longer contain the forward slash ("/") character, and TopoServer's `CreateShard` method returns an error if given such a name. + ### New command line flags and behavior #### Backup --builtinbackup-file-read-buffer-size and --builtinbackup-file-write-buffer-size diff --git a/go/vt/topo/shard.go b/go/vt/topo/shard.go index c6aca7b67c1..df692a6c3c0 100644 --- a/go/vt/topo/shard.go +++ b/go/vt/topo/shard.go @@ -121,6 +121,10 @@ func IsShardUsingRangeBasedSharding(shard string) bool { // ValidateShardName takes a shard name and sanitizes it, and also returns // the KeyRange. func ValidateShardName(shard string) (string, *topodatapb.KeyRange, error) { + if strings.Contains(shard, "/") { + return "", nil, vterrors.Errorf(vtrpc.Code_INVALID_ARGUMENT, "invalid shardId, may not contain '/': %v", shard) + } + if !IsShardUsingRangeBasedSharding(shard) { return shard, nil, nil } diff --git a/go/vt/topo/shard_test.go b/go/vt/topo/shard_test.go index 4c0088f00ee..d0ec08f94ea 100644 --- a/go/vt/topo/shard_test.go +++ b/go/vt/topo/shard_test.go @@ -17,13 +17,14 @@ limitations under the License. package topo import ( + "context" "reflect" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "context" - + "vitess.io/vitess/go/test/utils" topodatapb "vitess.io/vitess/go/vt/proto/topodata" ) @@ -222,3 +223,58 @@ func TestUpdateSourceDeniedTables(t *testing.T) { t.Fatalf("one cell removal from all failed: %v", si) } } + +func TestValidateShardName(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + expectedRange *topodatapb.KeyRange + valid bool + }{ + { + name: "0", + valid: true, + }, + { + name: "-80", + expectedRange: &topodatapb.KeyRange{ + Start: nil, + End: []byte{0x80}, + }, + valid: true, + }, + { + name: "40-80", + expectedRange: &topodatapb.KeyRange{ + Start: []byte{0x40}, + End: []byte{0x80}, + }, + valid: true, + }, + { + name: "foo-bar", + valid: false, + }, + { + name: "a/b", + valid: false, + }, + } + + for _, tcase := range cases { + tcase := tcase + t.Run(tcase.name, func(t *testing.T) { + t.Parallel() + + _, kr, err := ValidateShardName(tcase.name) + if !tcase.valid { + assert.Error(t, err, "expected %q to be an invalid shard name", tcase.name) + return + } + + require.NoError(t, err, "expected %q to be a valid shard name, got error: %v", tcase.name, err) + utils.MustMatch(t, tcase.expectedRange, kr) + }) + } +}