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

Fix to parse IPv6 inside brackets in the config urls #185

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cgeoffroy
Copy link

Add some tests to verify host parsing behavior and provide some fixes to the parsing function to better handle IPv6 hosts.

The problem is that in URL, IPv6 are surrounded with brackets [] which are not removed after parsing.

Copy link
Collaborator

@raphaelhetzel raphaelhetzel left a comment

Choose a reason for hiding this comment

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

Thanks.
Note that I never tested this with IPv6, so it would be great to learn what works and what needs changing.

let host = match maybe_an_ip {
Some(ip) => ip.to_string(), // WARNING the verbatim IP in `raw` may be outputed in a canonical form
None => {
// FIXME: the val[2] could still be an hostname or dns name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not quite sure if this is actually the behavior desired for listening.

We might need two different functions (one for IPs and one for hosts), or pass the raw string through an additional IP parsing step.

In any case, the API of this is not sufficient.

@ccicconetti
Copy link
Member

@raphaelhetzel @cgeoffroy sorry what are the plans re: this PR?

I understand the changes by themselves may not automatically enable IPv6, but is there any reason why we shouldn't merge this (there's also a new unit test)?

@raphaelhetzel
Copy link
Collaborator

@cgeoffroy can merge this (I believe we just did not communicate that very well).

@ccicconetti
Copy link
Member

@cgeoffroy can merge this (I believe we just did not communicate that very well).

Ah OK. Well, @cgeoffroy can you do a git rebase -i origin/main and fix conflicts and then rebase the PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants