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

cli: fix use of IPv6 addresses with RPC client commands #37977

Merged
merged 1 commit into from Jun 3, 2019

Conversation

Projects
None yet
5 participants
@knz
Copy link
Member

commented Jun 3, 2019

Fixes #33008.

(This was actually a regression of my doing, back from #28373. Didn't pick it up back then because we didn't have a test.)

Release note (bug fix): the cockroach command line utilities that
internally use a RPC connection (e.g. cockroach quit, cockroach init, etc) again properly support passing an IPv6 address literal via
the --host argument.

@knz knz requested review from bdarnell and andreimatei Jun 3, 2019

@knz knz requested a review from cockroachdb/cli-prs as a code owner Jun 3, 2019

@knz

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

(will backport)

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

This change is Reviewable

@awoods187

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

@knz thank you for the speedy turnaround! It is much appreciated.

@knz knz force-pushed the knz:20190603-fix-ipv6 branch from dbb491d to a11a719 Jun 3, 2019

@knz

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

@bdarnell do you have any idea how to test this effectively? The TC agents don't support ipv6, so neither a direct test nor a docker test would do (ipv6 support is inherited in docker - no ipv6 outside, not inside either).

@bdarnell

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Make it a roachtest, to swat a fly with a fleet of battleships? The VMs created by roachprod have ipv6 loopback enabled (although they don't have routable ipv6 addresses).

@knz

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

I'm not keen to turn a problem from bad to worse.

@andreimatei
Copy link
Member

left a comment

LGTM

Do y'all know if we could have routable ipv6 in roachprod? Then we could migrate some random roachtest to ipv6 to exercise this fix (right?)

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @bdarnell, and @knz)


pkg/cli/cli_test.go, line 429 at r1 (raw file):

}

func Example_ipv6_client() {

I don't like this "example" very much. It's not really what examples are supposed to be used for. Also particularly the reference to the bug seems click bait. Also I thought this would not work on TC?

How about just writing a unit test for the addrWithDefaultHost() function?

cli: fix use of IPv6 addresses with RPC client commands
Release note (bug fix): the `cockroach` command line utilities that
internally use a RPC connection (e.g. `cockroach quit`, `cockroach
init`, etc) again properly support passing an IPv6 address literal via
the `--host` argument.

@knz knz force-pushed the knz:20190603-fix-ipv6 branch from a11a719 to ab4f77a Jun 3, 2019

@knz
Copy link
Member Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @bdarnell)


pkg/cli/cli_test.go, line 429 at r1 (raw file):

Also I thought this would not work on TC?

Yeah I was trying this out to see how TC would respond. That's how I found TC didn't like it.

How about just writing a unit test for the addrWithDefaultHost() function?

Yep, that's the better idea. Done.

@knz

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

TFYR

bors r+

craig bot pushed a commit that referenced this pull request Jun 3, 2019

Merge #37942 #37977
37942: sql: Adding support for show indexes from database command r=rohany a=rohany

As requested in #37270, support for a show indexes from database command would be helpful. This PR includes support for the command in the parser. Future PR's will implement the functionality of the parsed results.

37977: cli: fix use of IPv6 addresses with RPC client commands r=knz a=knz

Fixes #33008.

(This was actually a regression of my doing, back from #28373. Didn't pick it up back then because we didn't have a test.)

Release note (bug fix): the `cockroach` command line utilities that
internally use a RPC connection (e.g. `cockroach quit`, `cockroach
init`, etc) again properly support passing an IPv6 address literal via
the `--host` argument.

Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
@craig

This comment has been minimized.

Copy link

commented Jun 3, 2019

Build succeeded

@craig craig bot merged commit ab4f77a into cockroachdb:master Jun 3, 2019

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.