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.

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

replace fmt.Sprintf("%s:%d") with net.JoinHostPort #380

Closed
kevinburkesegment opened this issue Sep 15, 2023 · 1 comment · Fixed by #388
Closed

replace fmt.Sprintf("%s:%d") with net.JoinHostPort #380

kevinburkesegment opened this issue Sep 15, 2023 · 1 comment · Fixed by #388
Labels
good first issue Good for newcomers

Comments

@kevinburkesegment
Copy link
Contributor

Several places in the codebase use fmt.Sprintf to join a host and a port, for example:

httpgrpc/server/server.go:118:		address := fmt.Sprintf("kubernetes:///%s%s:%s", service, domain, port)
crypto/tls/test/tls_integration_test.go:109:	httpURL := fmt.Sprintf("https://localhost:%d/hello", httpPort)
crypto/tls/test/tls_integration_test.go:110:	grpcHost := fmt.Sprintf("localhost:%d", grpcPort)
kv/memberlist/memberlist_client_test.go:853:					cfg.JoinMembers = []string{fmt.Sprintf("%slocalhost:%d", testData.queryType, ports[1])}
kv/memberlist/memberlist_client_test.go:933:	cfg.JoinMembers = []string{fmt.Sprintf("127.0.0.1:%d", ports[0])}
kv/memberlist/memberlist_client_test.go:999:				_, err := kv.kv.JoinMembers([]string{fmt.Sprintf("127.0.0.1:%d", portToConnect)})
kv/memberlist/memberlist_client_test.go:1158:	_, err = mkv2.JoinMembers([]string{fmt.Sprintf("127.0.0.1:%d", mkv1.GetListeningPort())})
kv/memberlist/memberlist_client_test.go:1202:	cfg2.JoinMembers = []string{fmt.Sprintf("localhost:%d", ports[0])}
kv/memberlist/memberlist_client_test.go:1433:	cfg.JoinMembers = []string{fmt.Sprintf("127.0.0.1:%d", mkv1.GetListeningPort())}
server/server_tracing_test.go:92:	helloRouteURLRaw := fmt.Sprintf("http://%s:%d/hello", httpAddress, httpPort)
server/server_tracing_test.go:97:	helloPathParamRouteURLRaw := fmt.Sprintf("http://%s:%d/hello/world", httpAddress, httpPort)
server/server.go:251:	httpListener, err := net.Listen(network, fmt.Sprintf("%s:%d", cfg.HTTPListenAddress, cfg.HTTPListenPort))
server/server.go:275:	grpcListener, err := net.Listen(network, fmt.Sprintf("%s:%d", cfg.GRPCListenAddress, cfg.GRPCListenPort))
ring/lifecycler_test.go:515:	assert.Equal(t, fmt.Sprintf("%s:%d", lifecyclerCfg.Addr, lifecyclerCfg.Port), desc.GetAddr())

Because fmt.Sprintf is a general purpose parser, it will be slower than invoking the fit-for-purpose net.JoinHostPort. In addition, net.JoinHostPort correctly handles ipv6 addresses, which can include colons.

I'd be happy to replace all of these just wanted to point this out.

@charleskorn
Copy link
Contributor

Thanks for pointing this out @kevinburkesegment.

I'd be happy to replace all of these

If you'd like to open a PR to fix this, that would be great.

@charleskorn charleskorn added the good first issue Good for newcomers label Sep 17, 2023
kevinburkesegment added a commit to kevinburkesegment/dskit that referenced this issue Sep 26, 2023
This will ensure ipv6 compatibility for provided hosts, and this
method should also be faster than the general purpose fmt.Sprintf,
which must first parse the input string.

Fixes grafana#380.
kevinburkesegment added a commit to kevinburkesegment/dskit that referenced this issue Oct 5, 2023
This will ensure ipv6 compatibility for provided hosts, and this
method should also be faster than the general purpose fmt.Sprintf,
which must first parse the input string.

Fixes grafana#380.
kevinburkesegment added a commit to kevinburkesegment/dskit that referenced this issue Oct 5, 2023
This will ensure ipv6 compatibility for provided hosts, and this
method should also be faster than the general purpose fmt.Sprintf,
which must first parse the input string.

Fixes grafana#380.
kevinburkesegment added a commit to kevinburkesegment/dskit that referenced this issue Oct 9, 2023
This will ensure ipv6 compatibility for provided hosts, and this
method should also be faster than the general purpose fmt.Sprintf,
which must first parse the input string.

Fixes grafana#380.
kevinburkesegment added a commit to kevinburkesegment/dskit that referenced this issue Oct 9, 2023
This will ensure ipv6 compatibility for provided hosts, and this
method should also be faster than the general purpose fmt.Sprintf,
which must first parse the input string.

Fixes grafana#380.
kevinburkesegment added a commit to kevinburkesegment/dskit that referenced this issue Oct 9, 2023
This will ensure ipv6 compatibility for provided hosts, and this
method should also be faster than the general purpose fmt.Sprintf,
which must first parse the input string.

Fixes grafana#380.
charleskorn pushed a commit that referenced this issue Oct 10, 2023
This will ensure ipv6 compatibility for provided hosts, and this
method should also be faster than the general purpose fmt.Sprintf,
which must first parse the input string.

Fixes #380.
ying-jeanne pushed a commit that referenced this issue Nov 2, 2023
This will ensure ipv6 compatibility for provided hosts, and this
method should also be faster than the general purpose fmt.Sprintf,
which must first parse the input string.

Fixes #380.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants