Add brackets around IPv6 addrs in e2e test IP:port endpoints #52748

leblancd · 2017-09-19T21:41:05Z

There are several locations in the e2e tests where endpoints of the
form IPv6:port use IPv6 addresses directly, without surrounding brackets.
Brackets are required around IPv6 addresses in this case, in order to
distinguish the colons in the IPv6 address from the colon immediately
preceding the port.

Also, wherever the curl command might be used with an IPv6 address
surrounded in brackets, the "-g" argument is added to the curl
command line arguments so that the brackets can be interpreted
correctly.

fixes #52746

What this PR does / why we need it:
This PR adds brackets around IPv6 addresses when they appear as part of an IPv6-addr:port endpoint
in the e2e tests. This is needed because any connections that attempt to use IPv6-addr:port
endpoint without brackets surrounding the IPv6-addr will fail.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #52746

Special notes for your reviewer:

Release note:

NONE

xiangpengzhao · 2017-09-20T03:33:58Z

/ok-to-test

leblancd · 2017-09-25T01:15:16Z

/test pull-kubernetes-unit

sttts · 2017-09-25T10:04:52Z

/cc @frobware for IPv6

k8s-ci-robot · 2017-09-25T10:04:54Z

@sttts: GitHub didn't allow me to request PR reviews from the following users: frobware, for, IPv6.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @frobware for IPv6

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.

frobware

I'll assume that given the switch to --globoff that we know of no other meta-character uses in the remainder of the URL.

frobware · 2017-09-25T10:15:26Z

test/e2e/framework/networking_utils.go

-		containerIP,
-		containerHttpPort,
+	ipPort := net.JoinHostPort(containerIP, strconv.Itoa(containerHttpPort))
+	cmd := fmt.Sprintf("curl -g -q -s 'http://%s/dial?request=hostName&protocol=%s&host=%s&port=%d&tries=1'",


In scripts (and command lines like these) I think it is better to be explicit, so --globoff instead of -g. It may save you one lookup to the man page as you later try to recall what -g does.

frobware · 2017-09-25T10:25:31Z

test/images/nettest/nettest.go

@@ -278,7 +280,8 @@ func getWebserverEndpoints(client clientset.Interface) sets.String {
 	for _, ss := range endpoints.Subsets {
 		for _, a := range ss.Addresses {
 			for _, p := range ss.Ports {
-				eps.Insert(fmt.Sprintf("http://%s:%d", a.IP, p.Port))
+				ipPort := net.JoinHostPort(a.IP, strconv.Itoa(int(p.Port)))


As we already do int(p.Port) just pass this directly to net.JoinHostPort(). The strconv.Itoa() is not necessary.

p.Port in this case is an int32. The int(p.Port) is an intermediate conversion from int32 to int, since strconv.Itoa() won't take int32 directly. I could use fmt.Sprint(p.Port), if the double conversion is misleading.

frobware · 2017-09-25T10:27:03Z

test/integration/federation/framework/api.go

@@ -70,7 +72,9 @@ func (f *FederationAPIFixture) SetUpWithRunOptions(t *testing.T, runOptions *opt
 		t.Fatal(err)
 	}

-	f.Host = fmt.Sprintf("http://%s:%d", runOptions.InsecureServing.BindAddress, runOptions.InsecureServing.BindPort)
+	port := strconv.Itoa(runOptions.InsecureServing.BindPort)
+	hostPort := net.JoinHostPort(runOptions.InsecureServing.BindAddress, port)


Consistency: It seems that the other changes refer to this as ipPort - why not here too?

leblancd

@frobware : The 2 places where --globoff is being added do not have any square brackets or curly braces besides the possible IPv6 address.

leblancd · 2017-09-25T13:44:05Z

test/images/nettest/nettest.go

@@ -278,7 +280,8 @@ func getWebserverEndpoints(client clientset.Interface) sets.String {
 	for _, ss := range endpoints.Subsets {
 		for _, a := range ss.Addresses {
 			for _, p := range ss.Ports {
-				eps.Insert(fmt.Sprintf("http://%s:%d", a.IP, p.Port))
+				ipPort := net.JoinHostPort(a.IP, strconv.Itoa(int(p.Port)))


p.Port in this case is an int32. The int(p.Port) is an intermediate conversion from int32 to int, since strconv.Itoa() won't take int32 directly. I could use fmt.Sprint(p.Port), if the double conversion is misleading.

leblancd · 2017-09-25T13:51:05Z

test/e2e/framework/networking_utils.go

-		containerIP,
-		containerHttpPort,
+	ipPort := net.JoinHostPort(containerIP, strconv.Itoa(containerHttpPort))
+	cmd := fmt.Sprintf("curl -g -q -s 'http://%s/dial?request=hostName&protocol=%s&host=%s&port=%d&tries=1'",


frobware · 2017-09-25T14:00:12Z

p.Port in this case is an int32. The int(p.Port) is an intermediate conversion from int32 to int, since strconv.Itoa() won't take int32 directly. I could use fmt.Sprint(p.Port), if the double conversion is misleading

Of course! My mistake. As you suggest the sprintf is cleaner.

danehans · 2017-10-09T15:58:17Z

/area ipv6
/sig network

ncdc · 2017-10-12T13:35:15Z

/unassign
/assign @danwinship (please reassign to someone else in sig-network as you see fit)

danwinship · 2017-10-12T16:13:11Z

--globoff has not been needed for IPv6 IP literals since curl 7.37.0 (May 21 2014)

... which is newer than what's in RHEL7. Sigh. OK. Well, anyway, neither the old nor new man pages mention anything about IPv6 in the documentation for --globoff, so I think that whether you say --globoff or -g, it's going to be confusing, since there's no apparent globbing in the URLs to worry about. I feel like it would be better to stick with -g (for brevity and for consistency with the other options), but put a comment explaining why it's needed.

Also, git grep '://%s:%' turns up a bunch more places that might need fixing. (Some are false positives but some look like they aren't.)

There are several locations in the e2e tests where endpoints of the form IP:port use IPv6 addresses directly, without surrounding brackets. Brackets are required around IPv6 addresses in this case, in order to distinguish the colons in the IPv6 address from the colon immediately preceding the port. Also, wherever the curl command might be used with an IPv6 address surrounded in brackets, the "-g" argument is added to the curl command line arguments so that the brackets can be interpreted correctly. fixes kubernetes#52746

danehans · 2017-11-14T17:47:05Z

@bowei @thockin anything else needed to get this PR merged?

danehans · 2017-11-22T17:59:27Z

/assign @thockin

thockin · 2017-11-22T18:25:07Z

/lgtm
/approve

I want to thank the IPv6 team for consistently producing small, easy to review PRs.

k8s-github-robot · 2017-11-22T18:25:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leblancd, pmichali, thockin

Associated issue: 52746

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

Needs approval from an approver in each of these OWNERS Files:

~~test/OWNERS~~ [thockin]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

danehans · 2017-11-24T21:34:59Z

@thockin thank you! I see this PR has not merged b/c the submit queue states "Milestone is for a future release and cannot be merged". Correct me if I'm wring, but I assume this PR will not get merged until the 1.10 cycle.

fejta-bot · 2017-11-30T02:19:53Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

bowei · 2017-12-04T20:34:51Z

/status approved-for-milestone

bowei · 2017-12-04T20:35:02Z

/priority critical-urgent

k8s-github-robot · 2017-12-04T22:09:36Z

/test all [submit-queue is verifying that this PR is safe to merge]

leblancd · 2017-12-04T22:18:26Z

/kind feature

k8s-github-robot · 2017-12-04T22:19:06Z

[MILESTONENOTIFIER] Milestone Pull Request Needs Attention

@danwinship @leblancd @pmichali @sttts @thockin @kubernetes/sig-network-misc

Action required: During code freeze, pull requests in the milestone should be in progress.
If this pull request is not being actively worked on, please remove it from the milestone.
If it is being worked on, please add the status/in-progress label so it can be tracked with other in-flight pull requests.

Note: This pull request is marked as priority/critical-urgent, and must be updated every 1 day during code freeze.

Example update:

ACK.  In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required

Pull Request Labels

sig/network: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/feature: New functionality.

Help

k8s-github-robot · 2017-12-04T22:22:10Z

Automatic merge from submit-queue (batch tested with PRs 52748, 56623). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot · 2017-12-04T22:52:59Z

@leblancd: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-verify	`2827b7f`	link	`/test pull-kubernetes-verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 19, 2017

k8s-github-robot assigned sttts and ncdc Sep 19, 2017

k8s-github-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Sep 19, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 20, 2017

leblancd force-pushed the v6_e2e_host_port branch 2 times, most recently from cacd5c1 to 4fe9816 Compare September 22, 2017 19:08

frobware reviewed Sep 25, 2017

View reviewed changes

leblancd commented Sep 25, 2017

View reviewed changes

leblancd force-pushed the v6_e2e_host_port branch from 4fe9816 to 3afd48b Compare September 25, 2017 13:59

leblancd force-pushed the v6_e2e_host_port branch 2 times, most recently from ed2d6fb to 5c5b712 Compare September 25, 2017 14:07

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2017

leblancd mentioned this pull request Oct 6, 2017

IPv6 e2e Tests #47666

Closed

k8s-ci-robot added area/ipv6 sig/network Categorizes an issue or PR as relevant to SIG Network. labels Oct 9, 2017

leblancd force-pushed the v6_e2e_host_port branch from 5c5b712 to 01c65ff Compare October 9, 2017 18:58

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2017

k8s-ci-robot unassigned ncdc Oct 12, 2017

ncdc assigned danwinship Oct 12, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2017

leblancd force-pushed the v6_e2e_host_port branch from bf7f760 to 2827b7f Compare November 14, 2017 16:10

k8s-github-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 Nov 14, 2017

k8s-ci-robot assigned thockin Nov 22, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2017

k8s-ci-robot added the status/approved-for-milestone label Dec 4, 2017

k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Dec 4, 2017

freehan added this to the v1.9 milestone Dec 4, 2017

k8s-github-robot added the milestone/incomplete-labels label Dec 4, 2017

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 4, 2017

k8s-github-robot added milestone/needs-attention and removed milestone/incomplete-labels labels Dec 4, 2017

k8s-github-robot merged commit f9f8dc3 into kubernetes:master Dec 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add brackets around IPv6 addrs in e2e test IP:port endpoints #52748

Add brackets around IPv6 addrs in e2e test IP:port endpoints #52748

leblancd commented Sep 19, 2017 •

edited

Loading

xiangpengzhao commented Sep 20, 2017

leblancd commented Sep 25, 2017

sttts commented Sep 25, 2017

k8s-ci-robot commented Sep 25, 2017

frobware left a comment

frobware Sep 25, 2017

leblancd Sep 25, 2017

frobware Sep 25, 2017

leblancd Sep 25, 2017

frobware Sep 25, 2017

leblancd left a comment

leblancd Sep 25, 2017

leblancd Sep 25, 2017

frobware commented Sep 25, 2017

danehans commented Oct 9, 2017

ncdc commented Oct 12, 2017

danwinship commented Oct 12, 2017

danehans commented Nov 14, 2017

danehans commented Nov 22, 2017

thockin commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

danehans commented Nov 24, 2017

fejta-bot commented Nov 30, 2017

bowei commented Dec 4, 2017

bowei commented Dec 4, 2017

k8s-github-robot commented Dec 4, 2017

leblancd commented Dec 4, 2017

k8s-github-robot commented Dec 4, 2017

k8s-github-robot commented Dec 4, 2017

k8s-ci-robot commented Dec 4, 2017

Add brackets around IPv6 addrs in e2e test IP:port endpoints #52748

Add brackets around IPv6 addrs in e2e test IP:port endpoints #52748

Conversation

leblancd commented Sep 19, 2017 • edited Loading

xiangpengzhao commented Sep 20, 2017

leblancd commented Sep 25, 2017

sttts commented Sep 25, 2017

k8s-ci-robot commented Sep 25, 2017

frobware left a comment

Choose a reason for hiding this comment

frobware Sep 25, 2017

Choose a reason for hiding this comment

leblancd Sep 25, 2017

Choose a reason for hiding this comment

frobware Sep 25, 2017

Choose a reason for hiding this comment

leblancd Sep 25, 2017

Choose a reason for hiding this comment

frobware Sep 25, 2017

Choose a reason for hiding this comment

leblancd left a comment

Choose a reason for hiding this comment

leblancd Sep 25, 2017

Choose a reason for hiding this comment

leblancd Sep 25, 2017

Choose a reason for hiding this comment

frobware commented Sep 25, 2017

danehans commented Oct 9, 2017

ncdc commented Oct 12, 2017

danwinship commented Oct 12, 2017

danehans commented Nov 14, 2017

danehans commented Nov 22, 2017

thockin commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

danehans commented Nov 24, 2017

fejta-bot commented Nov 30, 2017

bowei commented Dec 4, 2017

bowei commented Dec 4, 2017

k8s-github-robot commented Dec 4, 2017

leblancd commented Dec 4, 2017

k8s-github-robot commented Dec 4, 2017

k8s-github-robot commented Dec 4, 2017

k8s-ci-robot commented Dec 4, 2017

leblancd commented Sep 19, 2017 •

edited

Loading