Skip to content

ARROW-10475: [C++][FlightRPC] handle IPv6 hosts#8585

Closed
lidavidm wants to merge 1 commit into
apache:masterfrom
lidavidm:arrow-10475
Closed

ARROW-10475: [C++][FlightRPC] handle IPv6 hosts#8585
lidavidm wants to merge 1 commit into
apache:masterfrom
lidavidm:arrow-10475

Conversation

@lidavidm

@lidavidm lidavidm commented Nov 3, 2020

Copy link
Copy Markdown
Member

uriparser has a more detailed hostData struct that exposes whether the host is IPv4, IPv6, or something else, but doesn't provide apparent ways to convert back. There's also already tests for the current behavior (IPv6 host gets returned without brackets). If we don't care about those tests, we can simply check if uriparser thinks the host is IPv6, and if so, add the brackets back, without writing an IPv6/IPv4 address formatter. (Though that's technically wrong, I suppose - the host is really the unbracketed address, and the brackets only come into play when formatted as a URI.)

Otherwise, here I check if the host looks like IPv6 inside Flight and add the brackets back.

@github-actions

github-actions Bot commented Nov 4, 2020

Copy link
Copy Markdown

@lidavidm lidavidm force-pushed the arrow-10475 branch 2 times, most recently from e865b40 to 0ee5af7 Compare November 4, 2020 13:24
@pitrou

pitrou commented Nov 9, 2020

Copy link
Copy Markdown
Member

There's also already tests for the current behavior (IPv6 host gets returned without brackets)

Those tests are correct, but we can add a function to encode back the host (UriEncodeHost perhaps).

@lidavidm

lidavidm commented Nov 11, 2020

Copy link
Copy Markdown
Member Author

I've added a method on the Uri class now to format the host, though the implementation is essentially the naive one still (if ipv6 add brackets else pass through the host).

@pitrou

pitrou commented Nov 12, 2020

Copy link
Copy Markdown
Member

I meant a separate function, not a method. The host doesn't necessarily come from a URI.

@lidavidm

Copy link
Copy Markdown
Member Author

Ah sorry, I think I got what you mean now - updated.

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, thank you @lidavidm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants