-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[topo] Disallow the slash character in shard names #12843
[topo] Disallow the slash character in shard names #12843
Conversation
Fixes vitessio#12842. Signed-off-by: Andrew Mason <andrew@planetscale.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
If a new flag is being introduced:
If a workflow is added or modified:
Bug fixes
Non-trivial changes
New/Existing features
Backward compatibility
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳
Signed-off-by: Andrew Mason <andrew@planetscale.com>
go/vt/topo/shard.go
Outdated
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that the error code here is the correct one - INVALID_ARGUMENT
. Why does the client display Unknown
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's the vtadmin-web level of reporting, it's INVALID_ARGUMENT all the way through to vtadmin-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we convert all RPC errors to Unknown errors? It just seems misleading to be claiming that it is an unknown rpc error
when it is not.
Or is this deliberate obfuscation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it depends (NOT_FOUND gets converted to 404, for example), but that's something we can address separately if you feel it's an issue
Co-authored-by: Deepthi Sigireddi <deepthi.sigireddi@gmail.com> Signed-off-by: Andrew Mason <amason@hey.com>
Co-authored-by: Deepthi Sigireddi <deepthi.sigireddi@gmail.com> Signed-off-by: Andrew Mason <amason@hey.com>
I was unable to backport this Pull Request to the following branches: |
* Disallow the slash character in shard names Fixes vitessio#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>
…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>
Description
Gives echos of #12732
"demo" of this erroring out in vtadmin:
Related Issue(s)
Fixes #12842.
Checklist
Deployment Notes