-
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
[VTAdmin API] Replace underscores with dashes in cluster ID #15722
base: main
Are you sure you want to change the base?
[VTAdmin API] Replace underscores with dashes in cluster ID #15722
Conversation
Signed-off-by: notfelineit <notfelineit@gmail.com>
Signed-off-by: notfelineit <notfelineit@gmail.com>
Signed-off-by: notfelineit <notfelineit@gmail.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
go/vt/vtadmin/cluster/config.go
Outdated
@@ -284,6 +288,17 @@ func (cfg Config) Merge(override Config) Config { | |||
return merged | |||
} | |||
|
|||
func formatID(id string) string { | |||
if strings.Contains(id, "_") { |
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.
Are there any other characters that can occur in the input that will fail here as well? Is there any reference also to GRPC docs which describes what characters are valid and which ones are not?
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 couldn't find anything definitive in the gRPC repository, however with some digging:
- gRPC Core defines custom resolver names as following URI syntacx
- Further digging from the modifications section of the above RFC shows that gRPC naming followed ABNF, and specifically the
scheme
portion of the resolver does not seem to allow_
underscores: https://datatracker.ietf.org/doc/html/rfc2396#section-3.1- we use the cluster ID as the scheme, which is why this PR replaces all the underscores in the cluster ID. For example
my_cluster_id://vtctld/
is a custom resolver name generated by vtadmin api
- we use the cluster ID as the scheme, which is why this PR replaces all the underscores in the cluster ID. For example
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 think I'd like to ship this fix, and then spend more time being thoughtful about:
- Testing the format stated in RFC 2396
- Implementing cluster ID validations following the format in the RFC, if it is indeed strict
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.
cluster ID as the scheme
Which characters are allowed as a cluster ID? Should we add a validation on that so it wouldn't even be allowed to use anything that won't also fit into a scheme?
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.
https://datatracker.ietf.org/doc/html/rfc3986#appendix-A has the allowed characters as:
scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
so we should just assert on that i think
Signed-off-by: notfelineit <notfelineit@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15722 +/- ##
==========================================
- Coverage 68.40% 68.40% -0.01%
==========================================
Files 1556 1556
Lines 195121 195288 +167
==========================================
+ Hits 133479 133591 +112
- Misses 61642 61697 +55 ☔ View full report in Codecov by Sentry. |
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.
awesome find! just one minor comment
@@ -237,7 +241,7 @@ func (cfg *Config) MarshalJSON() ([]byte, error) { | |||
// config. Neither the caller or the argument are modified in any way. | |||
func (cfg Config) Merge(override Config) Config { | |||
merged := Config{ | |||
ID: cfg.ID, | |||
ID: formatID(cfg.ID), |
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.
can we do this further up the call stack (when we parse a config) so we don't need to do this operation every time we merge two configs?
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.
Yes! 😄
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 think this fixes the noted issue so don't forget to add the Fixes
keyword. The issue will then be linked and automatically closed with a note.
@@ -284,6 +288,13 @@ func (cfg Config) Merge(override Config) Config { | |||
return merged | |||
} | |||
|
|||
func formatID(id string) string { | |||
// gRPC can't process custom resolver names with underscores | |||
id = strings.Replace(id, "_", "-", -1) |
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.
Since we're here, we could address the wider issue I think just as easily. We could replace any invalid chars with dashes, or remove them as well. You can see the regex to replace any unaccepted chars with dashes here: https://go.dev/play/p/qmHmZ_KSnMI
If you wanted to do that, you probably just want to make the regexp a package variable so that we don't have to compile it every time (although I don't imagine we hit this very often so probably not a practical issue even if we did).
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.
Thought about this a bit more and IMO:
auto-fixing is a breaking change (what happens if I have a deployment with clusters hello_world
and hello-world
?), as well as its alternative of failing loudly on a "bad" name.
but the alternative is less magical, since users are having the names changed out silently beneath them, so we should maybe just do input validation and include an entry in the release notes?
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.
Yeah I agree that it's a breaking change - did you see my latest comment? I agree with input validation!
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.
Yep, that plan looks good to me!
Ok, I'm back working on this! @mattlord and @dbussink , I didn't originally intend to bundle the full fix with this PR, but I will now. The reason being I didn't want to make a decision on "break" or "fix silently" when users pass in an invalid cluster name. However, I think a reasonable first step might be to:
That changes things a bit, because I'm not sure if this PR should still automatically change @ajm188 what do you think? Should we:
|
What about we add warning for now to not immediately break people and then we turn it into an error if you use an invalid name for the next release (v21)? That way we don't have to do anything here trying to interpret what the user intended long term. |
@dbussink Yeah, I think that's a good way to go about it. Including what @ajm188 said too! Here's the plan taken from everyone's input:
And for clarification, we won't plan to auto format any cluster names! |
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
Moving to draft until all changes are complete. |
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
Description
VTAdmin API fails to connect to VTGate and vtctld when there are underscores
_
in the VTGate & vtctld custom name resolvers. The custom name is based on the cluster ID, so this PR switches underscores for dashes-
in the initial cluster ID.This PR should be backported, since it fixes a bug that exists in v17-present.
Related Issue(s)
Fixes #15721
Checklist
Deployment Notes
N/A