Backport: [topo] Disallow the slash character in shard names #12843 (#…

…12858) * [topo] Disallow the slash character in shard names (#12843) * Disallow the slash character in shard names Fixes #12842. Signed-off-by: Andrew Mason <andrew@planetscale.com> * add release note Signed-off-by: Andrew Mason <andrew@planetscale.com> * Update go/vt/topo/shard.go Co-authored-by: Deepthi Sigireddi <deepthi.sigireddi@gmail.com> Signed-off-by: Andrew Mason <amason@hey.com> * Update changelog/17.0/17.0.0/summary.md Co-authored-by: Deepthi Sigireddi <deepthi.sigireddi@gmail.com> Signed-off-by: Andrew Mason <amason@hey.com> --------- Signed-off-by: Andrew Mason <andrew@planetscale.com> Signed-off-by: Andrew Mason <amason@hey.com> Co-authored-by: Deepthi Sigireddi <deepthi.sigireddi@gmail.com> * add changelog stub Signed-off-by: Andrew Mason <andrew@planetscale.com> * fix version name Signed-off-by: Andrew Mason <andrew@planetscale.com> --------- Signed-off-by: Andrew Mason <andrew@planetscale.com> Signed-off-by: Andrew Mason <amason@hey.com> Co-authored-by: Deepthi Sigireddi <deepthi.sigireddi@gmail.com>
vitessio · Apr 12, 2023 · 9dcbd7d · 9dcbd7d
1 parent b04c38d
commit 9dcbd7d
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 2 deletions.
diff --git a/changelog/16.0/16.0.2/summary.md 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.
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)
+		})
+	}
+}