Assigning Address driver options #707

rmb938 · 2015-10-24T16:13:59Z

Adds the ability to give driver options when assigning an address.

Also when assigning the ipam gateway address pass the options bellow

{
  "gateway": "true"
}

This will allow ipam drivers to detect whether an address belongs to a gateway or not.

Currently under CreateEndpoint options are set to nil. This should be changed once EndpointOptions contains driver options for the endpoint it self and driver options for its address.

Signed-off-by: Ryan Belgrave rmb1993@gmail.com

rmb938 · 2015-10-24T17:07:23Z

Can someone poke CI? It timed out

aboch · 2015-10-24T17:45:50Z

endpoint.go

@@ -690,7 +690,7 @@ func (ep *endpoint) DataScope() string {
 	return ep.getNetwork().DataScope()
 }

-func (ep *endpoint) assignAddress() error {
+func (ep *endpoint) assignAddress(adOptions map[string]string) error {


I'd prefer to have an EndpointCreateOptionIpam() setter for CreateEndpioint() logic which would set a preferred address and a map of options for this endpoint. Similarly to NetworkOptionIpam() for network create.

Then this function could directly access it as ipamAllocateVersion() does for network.

rmb938 · 2015-10-24T18:30:04Z

@aboch How does that look?

aboch · 2015-10-24T19:22:50Z

endpoint.go

@@ -637,6 +639,14 @@ func EndpointOptionGeneric(generic map[string]interface{}) EndpointOption {
 	}
 }

+// CreateOptionIpam function returns an option setter for the ipam configuration for this endpoint
+func CreateOptionIpam(prefAddress net.IP, ipamOptions map[string]string) EndpointOption {


I was thinking of an InterfaceIpamConf structure so that it is easier to add more fields in future. But I cannot think of any other fields, besides the address and options.
Therefore I think It is better your way, less boilerplate code.

Yea that makes sense. I have no problem changing it to that if others feel that it is needed now.

it seems that only one IP address can be passed. Would it be possible to have multiple per end point ?

Thanks for the comment. Currently we do not support that, but I think only few changes would make it possible.
Just curious about the use case, is this for IP aliasing ?

@aboch one of the use case is if you want to both have a VIP and a local address. This could be for anycast or implementing direct server return (https://www.nanog.org/meetings/nanog51/presentations/Monday/NANOG51.Talk45.nanog51-Schaumann.pdf). The second IP can be on the loopback or any interface.

@jc-m we do envision the support for properly defined Services and yes, we will have Virtual IPs, Load balancers and all the whole 9 yards. It is just that we are not there yet.

But you did bring up an important point that if you have a custom IPAM driver, then this should be a case of the IPAM driver handling it & libnetwork should not worry about that restriction. But still, even if we make the current PR handle multiple-ip, it stops short at the IPAM layer. The endpoint isnt assigned to multiple IPs when it ends up in the network Driver.
Wont you feel better to have a full e2e support for multiple IPs from the User to IPAM driver and ends up finally in the endpoint as seen by the network driver ?

Again, as @aboch suggested, we are fully supportive of multiple IP assignment for the endpoints for all the use-cases that you suggested. Just that we dont have it covered e2e.

@mavenugo I obviously care about the e2e support. I see how supporting multiple IPs per endpoints can be a problem with the current model. Another way would have been to pass the preferred IPs in libnetwork.EndpointOptionGeneric , and this would have provided more flexibility. Some drivers could simply ignore them, others could use them.

@jc-m am glad that you brought up these points so that we can get it resolved soon. Will you mind opening an issue/proposal using which we can design it together to support multiple IPs e2e ?

My suggestion would be to let this PR work for a single IP which is supported e2e and work on a proposal to support multiple IPs end-to-end ?

I agree with getting this one go with 1 IP for now. We will provide a proposal for the multiple IPs building on this PR.

created #798

aboch · 2015-10-24T19:24:29Z

LGTM
ping @mrjana @mavenugo

rmb938 · 2015-10-28T23:29:40Z

any updates on getting this merged?

mavenugo · 2015-10-28T23:31:43Z

@rmb938 unfortunately, this has to wait for the next docker release.
We have to think and discuss more on the impact of Network driver vs IPAM driver for options.

rmb938 · 2015-10-28T23:32:51Z

Alright thanks.

mavenugo · 2015-12-01T22:20:40Z

network.go

@@ -944,7 +945,10 @@ func (n *network) ipamAllocateVersion(ipVer int, ipam ipamapi.Ipam) error {
 		// irrespective of whether ipam driver returned a gateway already.
 		// If none of the above is true, libnetwork will allocate one.
 		if cfg.Gateway != "" || d.Gateway == nil {
-			if d.Gateway, _, err = ipam.RequestAddress(d.PoolID, net.ParseIP(cfg.Gateway), nil); err != nil {
+			var gatewayOpts = map[string]string{
+				netlabel.Gateway: "",


I would personally avoid using an empty string as a way to replace a bool=true. This is not readable.
Shall we use a more explicit Key and its value can be netlabel.Gateway ?

@mavenugo I take the blame on this. See #707 (comment)

I am fine to change the value back to "true". This is actually how @rmb938 first wrote it.

@mavenugo @aboch I have no problem with changing it back. I'll also fix any merge conflicts in the next few days.

@rmb938 i was thinking more like adding a new constant/label in ipamapi package RequestAddressType as the key and the value can be netlabel.Gateway :). Can you please make this change and push it ?
We like to get this in soon before the next Vendor into docker.

@mavenugo Go is not my native language so would this be the correct way to do it?

ipamapi/contract.go

In the const declaration add

// RequestAddressType represents the Address Type used when requesting an address RequestAddressType = "RequestAddressType"

and in network.go

var gatewayOpts = map[string]string{ ipamapi.RequestAddressType: netlabel.Gateway, }

@rmb938 yes. that works.

mavenugo · 2015-12-04T00:55:31Z

LGTM.

cc @aboch

aboch · 2015-12-04T01:38:48Z

@rmb938 Something weird with the commits. Please check.

rmb938 · 2015-12-04T02:46:25Z

@aboch how is that?

options. When requesting a gateway address send a gateway label in the options. Signed-off-by: Ryan Belgrave <rmb1993@gmail.com>

aboch · 2015-12-04T18:49:32Z

@rmb938 Thanks for fixing the multiple commit issue.
LGTM

Assigning Address driver options

rmb938 mentioned this pull request Oct 24, 2015

assigning address driver options #705

Closed

rmb938 force-pushed the ipam_allocate_options branch from 2a932a4 to 5d2db71 Compare October 24, 2015 16:42

aboch reviewed Oct 24, 2015
View reviewed changes

rmb938 force-pushed the ipam_allocate_options branch from 527e83a to e3c3479 Compare October 24, 2015 18:53

aboch reviewed Oct 24, 2015
View reviewed changes

aboch added the status/1-needs-design-review label Oct 24, 2015

aboch mentioned this pull request Nov 30, 2015

Add a network option to disable default gateway #778

Merged

mavenugo reviewed Dec 1, 2015
View reviewed changes

aboch added status/2-needs-code-review and removed status/1-needs-design-review labels Dec 2, 2015

rmb938 force-pushed the ipam_allocate_options branch from fafec47 to 3dcf051 Compare December 4, 2015 02:08

rmb938 force-pushed the ipam_allocate_options branch from 3dcf051 to 64e7616 Compare December 4, 2015 03:38

When assigning an address for an endpoint set preferred address and

35f1a1f

options. When requesting a gateway address send a gateway label in the options. Signed-off-by: Ryan Belgrave <rmb1993@gmail.com>

rmb938 force-pushed the ipam_allocate_options branch from b57a937 to 35f1a1f Compare December 4, 2015 05:04

aboch added a commit that referenced this pull request Dec 4, 2015

Merge pull request #707 from rmb938/ipam_allocate_options

077b076

Assigning Address driver options

aboch merged commit 077b076 into moby:master Dec 4, 2015

allanhung mentioned this pull request Dec 23, 2015

lvs (linux virtual server) in libnetwork projectcalico/libnetwork-plugin#33

Closed

rmb938 deleted the ipam_allocate_options branch August 30, 2017 00:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assigning Address driver options #707

Assigning Address driver options #707

rmb938 commented Oct 24, 2015

rmb938 commented Oct 24, 2015

aboch Oct 24, 2015

rmb938 commented Oct 24, 2015

aboch Oct 24, 2015

rmb938 Oct 24, 2015

jc-m Dec 1, 2015

aboch Dec 1, 2015

jc-m Dec 1, 2015

mavenugo Dec 2, 2015

jc-m Dec 3, 2015

mavenugo Dec 3, 2015

jc-m Dec 3, 2015

jc-m Dec 3, 2015

aboch commented Oct 24, 2015

rmb938 commented Oct 28, 2015

mavenugo commented Oct 28, 2015

rmb938 commented Oct 28, 2015

mavenugo Dec 1, 2015

aboch Dec 1, 2015

rmb938 Dec 2, 2015

mavenugo Dec 3, 2015

rmb938 Dec 4, 2015

mavenugo Dec 4, 2015

mavenugo commented Dec 4, 2015

aboch commented Dec 4, 2015

rmb938 commented Dec 4, 2015

aboch commented Dec 4, 2015

Assigning Address driver options #707

Assigning Address driver options #707

Conversation

rmb938 commented Oct 24, 2015

rmb938 commented Oct 24, 2015

Choose a reason for hiding this comment

rmb938 commented Oct 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboch commented Oct 24, 2015

rmb938 commented Oct 28, 2015

mavenugo commented Oct 28, 2015

rmb938 commented Oct 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavenugo commented Dec 4, 2015

aboch commented Dec 4, 2015

rmb938 commented Dec 4, 2015

aboch commented Dec 4, 2015