diff --git a/changelog/16.0/16.0.2/summary.md b/changelog/16.0/16.0.2/summary.md new file mode 100644 index 00000000000..99b0c998396 --- /dev/null +++ b/changelog/16.0/16.0.2/summary.md @@ -0,0 +1,7 @@ +## Summary + +### Shard name validation in TopoServer + +Prior to v16.0.2, 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. \ No newline at end of file diff --git a/go/vt/topo/shard.go b/go/vt/topo/shard.go index 7f03bf13364..2599c7de962 100644 --- a/go/vt/topo/shard.go +++ b/go/vt/topo/shard.go @@ -120,6 +120,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) + }) + } +}