[VTAdmin API] Replace underscores with dashes in cluster ID #15722

notfelineit · 2024-04-15T17:57:17Z

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

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

N/A

Signed-off-by: notfelineit <notfelineit@gmail.com>

vitess-bot · 2024-04-15T17:57:19Z

go/vt/vtadmin/cluster/config.go

dbussink · 2024-04-15T18:29:11Z

go/vt/vtadmin/cluster/config.go

@@ -284,6 +288,17 @@ func (cfg Config) Merge(override Config) Config {
 	return merged
 }

+func formatID(id string) string {
+	if strings.Contains(id, "_") {


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?

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

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

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?

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 · 2024-04-15T20:42:44Z

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 68.40%. Comparing base (f118ba2) to head (3dce263).
Report is 6 commits behind head on main.

Files	Patch %	Lines
go/vt/vtadmin/cluster/config.go	71.42%	2 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

ajm188

awesome find! just one minor comment

ajm188 · 2024-04-16T15:40:39Z

go/vt/vtadmin/cluster/config.go

@@ -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),


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?

mattlord

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.

mattlord · 2024-04-16T21:40:18Z

go/vt/vtadmin/cluster/config.go

@@ -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)


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).

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?

Yeah I agree that it's a breaking change - did you see my latest comment? I agree with input validation!

Yep, that plan looks good to me!

notfelineit · 2024-04-19T16:34:21Z

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:

Maintain old behavior on "bad" cluster names (still spin up VTAdmin, vtctld and vtgate won't work) but add a log/notification letting the user know they should use a different cluster name

That changes things a bit, because I'm not sure if this PR should still automatically change _ to - then, because we should probably treat all invalid characters in the same way.

@ajm188 what do you think? Should we:

autofix the names
allow bad names and just notify the user
combination of the two: correct _ and warn on the rest

dbussink · 2024-04-22T07:22:11Z

Maintain old behavior on "bad" cluster names (still spin up VTAdmin, vtctld and vtgate won't work) but add a log/notification letting the user know they should use a different cluster name

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.

notfelineit · 2024-04-22T16:38:29Z

@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:

v20: Add warning (log & warning in the UI) on invalid cluster name
v21: Turn warning into an error on vtadmin-api boot up

And for clarification, we won't plan to auto format any cluster names!

notfelineit added 3 commits April 15, 2024 08:52

Replace underscores in ID with dashes

5fb3fcc

Signed-off-by: notfelineit <notfelineit@gmail.com>

Log old id

93125b1

Signed-off-by: notfelineit <notfelineit@gmail.com>

Always replace underscores with ids

3690dd3

Signed-off-by: notfelineit <notfelineit@gmail.com>

github-actions bot added this to the v20.0.0 milestone Apr 15, 2024

notfelineit marked this pull request as ready for review April 15, 2024 17:58

notfelineit requested review from ajm188 and rohit-nayak-ps as code owners April 15, 2024 17:58

dbussink reviewed Apr 15, 2024

View reviewed changes

notfelineit added 2 commits April 15, 2024 12:24

Remove log line

d62404e

Remove conditional

3dce263

Signed-off-by: notfelineit <notfelineit@gmail.com>

ajm188 approved these changes Apr 16, 2024

View reviewed changes

mattlord approved these changes Apr 16, 2024

View reviewed changes

This was referenced Apr 29, 2024

Release of v19.0.4 #15810

Closed

Release of v18.0.5 #15811

Closed

Release of v17.0.7 #15812

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VTAdmin API] Replace underscores with dashes in cluster ID #15722

[VTAdmin API] Replace underscores with dashes in cluster ID #15722

notfelineit commented Apr 15, 2024 •

edited by deepthi

vitess-bot bot commented Apr 15, 2024

dbussink Apr 15, 2024

notfelineit Apr 15, 2024 •

edited

notfelineit Apr 15, 2024

dbussink Apr 16, 2024

ajm188 Apr 16, 2024

codecov bot commented Apr 15, 2024

ajm188 left a comment

ajm188 Apr 16, 2024

notfelineit Apr 19, 2024

mattlord left a comment

mattlord Apr 16, 2024

ajm188 Apr 19, 2024

notfelineit Apr 22, 2024

ajm188 Apr 22, 2024

notfelineit commented Apr 19, 2024 •

edited

dbussink commented Apr 22, 2024

notfelineit commented Apr 22, 2024 •

edited

[VTAdmin API] Replace underscores with dashes in cluster ID #15722

Are you sure you want to change the base?

[VTAdmin API] Replace underscores with dashes in cluster ID #15722

Conversation

notfelineit commented Apr 15, 2024 • edited by deepthi

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Apr 15, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

Choose a reason for hiding this comment

notfelineit Apr 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 15, 2024

Codecov Report

ajm188 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattlord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notfelineit commented Apr 19, 2024 • edited

dbussink commented Apr 22, 2024

notfelineit commented Apr 22, 2024 • edited

notfelineit commented Apr 15, 2024 •

edited by deepthi

notfelineit Apr 15, 2024 •

edited

notfelineit commented Apr 19, 2024 •

edited

notfelineit commented Apr 22, 2024 •

edited