Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5fb3fcc
Replace underscores in ID with dashes
notfelineit Apr 15, 2024
93125b1
Log old id
notfelineit Apr 15, 2024
3690dd3
Always replace underscores with ids
notfelineit Apr 15, 2024
d62404e
Remove log line
notfelineit Apr 15, 2024
3dce263
Remove conditional
notfelineit Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 13 additions & 2 deletions go/vt/vtadmin/cluster/config.go
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@ import (
"fmt"
"io"
"strconv"
"strings"
"time"

"github.com/spf13/viper"
Expand Down Expand Up @@ -149,6 +150,8 @@ func LoadConfig(r io.Reader, configType string) (cfg *Config, id string, err err
return nil, "", ErrNoConfigID
}

id = formatID(id)

tmp := map[string]string{}
if err := v.Unmarshal(&tmp); err != nil {
return nil, id, err
Expand All @@ -164,6 +167,7 @@ func LoadConfig(r io.Reader, configType string) (cfg *Config, id string, err err
if err := cfg.unmarshalMap(tmp); err != nil {
return nil, id, err
}
cfg.ID = id

return cfg, id, nil
}
Expand Down Expand Up @@ -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),
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! 😄

Name: cfg.Name,
DiscoveryImpl: cfg.DiscoveryImpl,
DiscoveryFlagsByImpl: map[string]map[string]string{},
Expand All @@ -255,7 +259,7 @@ func (cfg Config) Merge(override Config) Config {
}

if override.ID != "" {
merged.ID = override.ID
merged.ID = formatID(override.ID)
}

if override.Name != "" {
Expand Down Expand Up @@ -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)
Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor Author

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!

Copy link
Contributor

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!


return id
}

func mergeStringMap(base map[string]string, override map[string]string) {
for k, v := range override {
base[k] = v
Expand Down
18 changes: 18 additions & 0 deletions go/vt/vtadmin/cluster/config_test.go
View file
Open in desktop
Expand Up @@ -49,6 +49,24 @@ func TestMergeConfig(t *testing.T) {
VtctldFlags: map[string]string{},
},
},
{
name: "id with underscores",
base: Config{
ID: "my_cluster",
Name: "mycluster",
},
override: Config{
DiscoveryImpl: "consul",
},
expected: Config{
ID: "my-cluster",
Name: "mycluster",
DiscoveryImpl: "consul",
DiscoveryFlagsByImpl: FlagsByImpl{},
VtSQLFlags: map[string]string{},
VtctldFlags: map[string]string{},
},
},
{
name: "merging discovery flags",
base: Config{
Expand Down
13 changes: 11 additions & 2 deletions go/vt/vtadmin/cluster/dynamic/cluster_test.go
View file
Open in desktop
Expand Up @@ -23,11 +23,11 @@ func TestClusterFromString(t *testing.T) {
{
name: "ok",
s: `{
"id": "dynamic_cluster",
"id": "dynamic-cluster",
"discovery": "dynamic",
"discovery-dynamic-discovery": "{\"vtctlds\": [ { \"host\": { \"fqdn\": \"localhost:15000\", \"hostname\": \"localhost:15999\" } } ], \"vtgates\": [ { \"host\": {\"hostname\": \"localhost:15991\" } } ] }"
}`,
expectedID: "dynamic_cluster",
expectedID: "dynamic-cluster",
},
{
name: "empty id",
Expand All @@ -50,6 +50,15 @@ func TestClusterFromString(t *testing.T) {
s: `{`,
shouldErr: true,
},
{
name: "id with underscores",
s: `{
"id": "id_with_underscores",
"discovery": "dynamic",
"discovery-dynamic-discovery": "{\"vtctlds\": [ { \"host\": { \"fqdn\": \"localhost:15000\", \"hostname\": \"localhost:15999\" } } ], \"vtgates\": [ { \"host\": {\"hostname\": \"localhost:15991\" } } ] }"
}`,
expectedID: "id-with-underscores",
},
}

for _, tt := range tests {
Expand Down