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

ParsePortSpec: handle IPv6 addresses #17

Closed
wants to merge 1 commit into from

Conversation

stapelberg
Copy link

split out of moby/moby#20315
in order to fix moby/moby#11518

Signed-off-by: Michael Stapelberg stapelberg@google.com

split out of moby/moby#20315
in order to fix moby/moby#11518

Signed-off-by: Michael Stapelberg <stapelberg@google.com>
@lenovouser
Copy link

Can this be merged? This is kind of essential as you can't bind containers to IPv6-Addresses at the moment. @LK4D4 you seem the most active contributor from the Docker team. Can you review this?

parts, err = PartParser(portSpecTemplate, rawPort)
if err != nil {
return nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is an easier way to handle this. PartParser() is an unfortunate abstraction. It's a generic way of escaping : separated strings with no way to escape : within the parts, and which is only used in a single place.

This code effectively introduces a new PartParser() that supports escaping colons using square brackets, but doesn't take advantage of the built-in functions in net and net/url`.

I think we should fix this by deprecating PartParser(), and replacing it with a new function that just parses the port spec into the appropriate parts using the available built-in functions.

@dnephin
Copy link
Contributor

dnephin commented Aug 8, 2016

We should add a test case as well, once we work out the implementation.

@stapelberg
Copy link
Author

@dnephin I don’t have strong opinions on how the code should look like. Would you want to add a test and refactor it so that it corresponds to your ideals? :)

@dnephin
Copy link
Contributor

dnephin commented Aug 8, 2016

I opened #19 with those changes

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.

Docker -p ip:hostPort:containerPort and ip::containerPort not compatible with IPv6 addresses
3 participants