[topo] Disallow the slash character in shard names (vitessio#12843)

* Disallow the slash character in shard names Fixes vitessio#12842. Signed-off-by: Andrew Mason <[email protected]> * add release note Signed-off-by: Andrew Mason <[email protected]> * Update go/vt/topo/shard.go Co-authored-by: Deepthi Sigireddi <[email protected]> Signed-off-by: Andrew Mason <[email protected]> * Update changelog/17.0/17.0.0/summary.md Co-authored-by: Deepthi Sigireddi <[email protected]> Signed-off-by: Andrew Mason <[email protected]> --------- Signed-off-by: Andrew Mason <[email protected]> Signed-off-by: Andrew Mason <[email protected]> Co-authored-by: Deepthi Sigireddi <[email protected]>
planetscale · Apr 6, 2023 · 26cf001 · 26cf001
1 parent d6c92cb
commit 26cf001
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 2 deletions.
diff --git 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
@@ -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)
+		})
+	}
+}