Skip to content

Allow IPv6 allocation post endpoint create via network option#741

Merged
mrjana merged 1 commit intomoby:masterfrom
aboch:b6
Nov 10, 2015
Merged

Allow IPv6 allocation post endpoint create via network option#741
mrjana merged 1 commit intomoby:masterfrom
aboch:b6

Conversation

@aboch
Copy link
Copy Markdown
Contributor

@aboch aboch commented Nov 6, 2015

  • On default bridge network only

Related to moby/moby#17739

Signed-off-by: Alessandro Boch aboch@docker.com

Comment thread drivers/bridge/bridge.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With this option, the IPAM doesn't even know about this range. What happens if the same range is used by some other network and IPAM gives out the same IP address as what was generated here using MAC address.

Would it be better to allocate this generated IP address from IPAM during the SetIPAddress call?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That would be the ideal solution. But we do not have the ipam options for Endpoints.
Second option is that libnetwork does it, but I'd refrain for adding bridge driver specific code into libnetwork.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What I am suggesting is create the pool in IPAM from the caller of libnetwork(docker), have some way of not allocating the IP address for IPv6(in fact we can do it for all networks if mask <= 80) and then allocate the IP address during SetIPAddress when driver calls. WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That would work, but decision whether to operate that way (allocating the address pre or post CreateEndpoint) should not be taken by libnetwork alone and based on mask lengths. The address allocation strategy should be controlled at least by a network driver property.
Also we probably want this behavior (deterministic ipv6 address setting implicitly via --mac-address run option) be preserved only for default bridge network, not for all bridge networks.

@aboch aboch changed the title Allow legacy ipv6 options for bridge Allow IPv6 allocation post endpoint create via network option Nov 10, 2015
@aboch
Copy link
Copy Markdown
Contributor Author

aboch commented Nov 10, 2015

@mrjana Reworked the diffs based on your input. PTAL when you get a chance. Thanks.

Comment thread network.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we change it to something like NetworkOptionIPv6DeferAlloc?

@mrjana
Copy link
Copy Markdown
Contributor

mrjana commented Nov 10, 2015

LGTM

Comment thread drivers/bridge/bridge.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ip6 is set only for the condition of ones <= 80. it is possible that it can be nil in cases when the config.AddressIPv6 doesnt fall under the condition. Should this be checked ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this.
It cannot happen in the docker code, but this condition needs nevertheless to be covered in the libnetwork context.
Will update the diffs shortly.

- Controlled by network option

Signed-off-by: Alessandro Boch <aboch@docker.com>
@aboch
Copy link
Copy Markdown
Contributor Author

aboch commented Nov 10, 2015

Addressed the issue pointed out by @mavenugo
@mavenugo @mrjana PTAL

@mavenugo
Copy link
Copy Markdown
Contributor

LGTM

@mrjana
Copy link
Copy Markdown
Contributor

mrjana commented Nov 10, 2015

LGTM again

mrjana added a commit that referenced this pull request Nov 10, 2015
Allow IPv6 allocation post endpoint create via network option
@mrjana mrjana merged commit e8ebc0b into moby:master Nov 10, 2015
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems to be an unassigned network. I suggest you to use: 2001:db8::/32 as per RFC 3849

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Please open a PR and we'll get it merged.

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.

4 participants