Replace use of Sprintf with net.JoinHostPort #109344

stbenjam · 2022-04-06T22:39:05Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

On IPv6 clusters, one of the most frequent problems I encounter is
assumptions that one can build a URL with a host and port simply by
using Sprintf, like this:

fmt.Sprintf("http://%s:%d/foo", host, port)

When host is an IPv6 address, this produces an invalid URL as it must
be bracketed, like this:

http://[2001:4860:4860::8888]:9443

This change fixes the occurrences of joining a host and port with the
purpose built net.JoinHostPort function.

I encounter this problem often enough that I started to write a linter
for it. I don't
think the linter is quite ready for wide use yet, but I did run it
against the Kube codebase and found these. While the host portion in
some of these changes may always be an FQDN or IPv4 IP today, it's an
easy thing that can break later on.

Which issue(s) this PR fixes:

N/A

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2022-04-06T22:39:13Z

Hi @stbenjam. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ehashman

/ok-to-test
/priority backlog
/triage accepted

endocrimes · 2022-04-07T17:19:21Z

/test pull-kubernetes-node-e2e-containerd
/test pull-kubernetes-e2e-gce-ubuntu-containerd

stbenjam · 2022-04-08T01:10:18Z

/test pull-kubernetes-node-e2e-containerd
/test pull-kubernetes-e2e-gce-ubuntu-containerd

aojea · 2022-04-08T13:33:44Z

/retest
/lgtm
/approve

we should have a linter for these things 😄 I can't remember the number of bugs we had because of this 🤷

dims · 2022-04-08T13:35:33Z

/approve

k8s-ci-robot · 2022-04-08T13:36:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, dims, stbenjam

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/volume/portworx/OWNERS~~ [dims]
~~test/e2e/storage/OWNERS~~ [aojea,dims]
~~test/e2e/upgrades/apps/OWNERS~~ [aojea,dims]
~~test/e2e/windows/OWNERS~~ [aojea,dims]
~~test/e2e_node/OWNERS~~ [aojea,dims]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

stbenjam · 2022-04-08T22:10:14Z

/retest /lgtm /approve

we should have a linter for these things 😄 I can't remember the number of bugs we had because of this 🤷

working on it :) golangci/golangci-lint#2749

SergeyKanzhelev · 2022-04-13T17:46:16Z

test/e2e_node/util.go

@@ -82,7 +85,7 @@ func getNodeSummary() (*stats.Summary, error) {
 	if err != nil {
 		return nil, fmt.Errorf("failed to get current kubelet config")
 	}
-	req, err := http.NewRequest("GET", fmt.Sprintf("http://%s:%d/stats/summary", kubeletConfig.Address, kubeletConfig.ReadOnlyPort), nil)
+	req, err := http.NewRequest("GET", fmt.Sprintf("http://%s/stats/summary", net.JoinHostPort(kubeletConfig.Address, strconv.Itoa(int(kubeletConfig.ReadOnlyPort)))), nil)


why so many conversions from int32 to string? It looks like JoinHostPort can take int32 as an argument and kubeletConfig.ReadOnlyPort is int32 already

You're looking at a version of JoinHostPort that is not part of the standard library.

This way of doing things is already common in this codebase (and many others):

$ grep -r JoinHostPort . | grep strconv | wc -l 89

got it. Indeed my google search failed me.

On IPv6 clusters, one of the most frequent problems I encounter is assumptions that one can build a URL with a host and port simply by using Sprintf, like this: ```go fmt.Sprintf("http://%s:%d/foo", host, port) ``` When `host` is an IPv6 address, this produces an invalid URL as it must be bracketed, like this: ``` http://[2001:4860:4860::8888]:9443 ``` This change fixes the occurences of joining a host and port with the purpose built `net.JoinHostPort` function. I encounter this problem often enough that I started to [write a linter for it](https://github.com/stbenjam/go-sprintf-host-port). I don't think the linter is quite ready for wide use yet, but I did run it against the Kube codebase and found these. While the host portion in some of these changes may always be an FQDN or IPv4 IP today, it's an easy thing that can break later on.

aojea · 2022-05-04T10:51:34Z

/lgtm

k8s-triage-robot · 2022-05-04T14:42:10Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-triage-robot · 2022-05-04T19:13:10Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/test labels Apr 6, 2022

k8s-ci-robot requested a review from feiskyer April 6, 2022 22:39

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Apr 6, 2022

k8s-ci-robot requested a review from humblec April 6, 2022 22:39

ehashman added this to Triage in SIG Node PR Triage Apr 7, 2022

ehashman reviewed Apr 7, 2022

View reviewed changes

ehashman moved this from Triage to Needs Reviewer in SIG Node PR Triage Apr 7, 2022

SergeyKanzhelev added this to Triage in SIG Node CI/Test Board Apr 7, 2022

stbenjam mentioned this pull request Apr 8, 2022

router: fix all uses of Sprintf to build host:port for a URL openshift/origin#26983

Merged

k8s-ci-robot assigned aojea Apr 8, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 8, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2022

ehashman moved this from Needs Reviewer to Needs Approver in SIG Node PR Triage Apr 12, 2022

SergeyKanzhelev reviewed Apr 13, 2022

View reviewed changes

SergeyKanzhelev moved this from Triage to PRs Waiting on Author in SIG Node CI/Test Board Apr 13, 2022

SergeyKanzhelev moved this from PRs Waiting on Author to Done in SIG Node CI/Test Board Apr 13, 2022

aojea mentioned this pull request Apr 18, 2022

E2e node summary #109529

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 4, 2022

stbenjam force-pushed the host-port-fixes branch from b1cde3e to b351745 Compare May 4, 2022 10:38

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 4, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2022

k8s-ci-robot merged commit 6445efb into kubernetes:master May 4, 2022

SIG Node PR Triage automation moved this from Needs Approver to Done May 4, 2022

k8s-ci-robot added this to the v1.25 milestone May 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace use of Sprintf with net.JoinHostPort #109344

Replace use of Sprintf with net.JoinHostPort #109344

stbenjam commented Apr 6, 2022

k8s-ci-robot commented Apr 6, 2022

ehashman left a comment

endocrimes commented Apr 7, 2022

stbenjam commented Apr 8, 2022

aojea commented Apr 8, 2022

dims commented Apr 8, 2022

k8s-ci-robot commented Apr 8, 2022

stbenjam commented Apr 8, 2022

SergeyKanzhelev Apr 13, 2022

stbenjam Apr 13, 2022

SergeyKanzhelev Apr 13, 2022

aojea commented May 4, 2022

k8s-triage-robot commented May 4, 2022

k8s-triage-robot commented May 4, 2022

Replace use of Sprintf with net.JoinHostPort #109344

Replace use of Sprintf with net.JoinHostPort #109344

Conversation

stbenjam commented Apr 6, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Apr 6, 2022

ehashman left a comment

Choose a reason for hiding this comment

endocrimes commented Apr 7, 2022

stbenjam commented Apr 8, 2022

aojea commented Apr 8, 2022

dims commented Apr 8, 2022

k8s-ci-robot commented Apr 8, 2022

stbenjam commented Apr 8, 2022

SergeyKanzhelev Apr 13, 2022

Choose a reason for hiding this comment

stbenjam Apr 13, 2022

Choose a reason for hiding this comment

SergeyKanzhelev Apr 13, 2022

Choose a reason for hiding this comment

aojea commented May 4, 2022

k8s-triage-robot commented May 4, 2022

k8s-triage-robot commented May 4, 2022