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.

Sign up for GitHub

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

Support NDP proxying on bridge networks #1316

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support NDP proxying on bridge networks #1316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions drivers/bridge/bridge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ type networkConfiguration struct {
BridgeName string
EnableIPv6 bool
EnableIPMasquerade bool
NDPProxyInterface string
EnableICC bool
Mtu int
DefaultBindingIP net.IP
Expand Down Expand Up @@ -215,6 +216,8 @@ func (c *networkConfiguration) fromLabels(labels map[string]string) error {
if c.EnableIPMasquerade, err = strconv.ParseBool(value); err != nil {
return parseErr(label, value, err.Error())
}
case NDPProxyInterface:
c.NDPProxyInterface = value
case EnableICC:
if c.EnableICC, err = strconv.ParseBool(value); err != nil {
return parseErr(label, value, err.Error())
Expand Down Expand Up @@ -680,6 +683,7 @@ func (d *driver) createNetwork(config *networkConfiguration) error {
bridgeSetup.queueStep(setupBridgeIPv4)

enableIPv6Forwarding := d.config.EnableIPForwarding && config.AddressIPv6 != nil
enableNDPProxying := config.NDPProxyInterface != "" && config.AddressIPv6 != nil

// Conditionally queue setup steps depending on configuration values.
for _, step := range []struct {
Expand All @@ -699,6 +703,9 @@ func (d *driver) createNetwork(config *networkConfiguration) error {
// Enable IPv6 Forwarding
{enableIPv6Forwarding, setupIPv6Forwarding},

// Enable NDP Proxying
{enableNDPProxying, setupNDPProxying},

// Setup Loopback Adresses Routing
{!d.config.EnableUserlandProxy, setupLoopbackAdressesRouting},

Expand Down Expand Up @@ -1019,6 +1026,27 @@ func (d *driver) CreateEndpoint(nid, eid string, ifInfo driverapi.InterfaceInfo,
}
}

// Add a neighbor proxy if using NDP proxying
if config.NDPProxyInterface != "" && config.EnableIPv6 {
link, err := d.nlh.LinkByName(config.NDPProxyInterface)
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

Please format and return a more expressive error, otherwise caller will receive a simple link not found, which does not help much.

Copy link

Choose a reason for hiding this comment

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

I'm not familiar with go;
Is this what you need:

return fmt.Errorf("Link not found: %v", err)

Copy link
Contributor

Choose a reason for hiding this comment

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

Simply return fmt.Errorf("could not find link for ndp proxy interface %q: %v", config.NDPProxyInterface, err)

}
neighbor := netlink.Neigh{
LinkIndex: link.Attrs().Index,
Family: netlink.FAMILY_V6,
State: netlink.NUD_PERMANENT,
Type: netlink.NDA_UNSPEC,
Flags: netlink.NTF_PROXY,
IP: endpoint.addrv6.IP,
HardwareAddr: endpoint.macAddress,
}
if err := d.nlh.NeighAdd(&neighbor); err != nil {
logrus.Warnf("could not add the neighbor proxy: %v", err)
return err
}
}

if err = d.storeUpdate(endpoint); err != nil {
return fmt.Errorf("failed to save bridge endpoint %s to store: %v", endpoint.id[0:7], err)
}
Expand Down Expand Up @@ -1077,6 +1105,24 @@ func (d *driver) DeleteEndpoint(nid, eid string) error {
}
}()

// Try removal of neighbor proxy. Discard error: it is a best effort.
// Also make sure defer does not see this error either.
if n.config.NDPProxyInterface != "" && n.config.EnableIPv6 {
link, err := d.nlh.LinkByName(n.config.NDPProxyInterface)
if err == nil {
Copy link
Contributor

@aboch aboch Aug 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with the best effort approach. But it would be useful to log a warning in case the link is not found (specifying this happened during neigh proxy entry removal)

Copy link

Choose a reason for hiding this comment

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

logrus.Warnf("couldn't remove neighbour: %v", err)

?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Does this makes sense ?

logrus.Warnf("could not remove ndp neighbour, because could not find link for ndp proxy interface %q: %v", config.NDPProxyInterface, err)

neighbor := netlink.Neigh{
LinkIndex: link.Attrs().Index,
Family: netlink.FAMILY_V6,
State: netlink.NUD_PERMANENT,
Type: netlink.NDA_UNSPEC,
Flags: netlink.NTF_PROXY,
IP: ep.addrv6.IP,
HardwareAddr: ep.macAddress,
}
d.nlh.NeighDel(&neighbor)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too, can you please log a warning message. Thanks

}
}

// Try removal of link. Discard error: it is a best effort.
// Also make sure defer does not see this error either.
if link, err := d.nlh.LinkByName(ep.srcName); err == nil {
Expand Down
4 changes: 4 additions & 0 deletions drivers/bridge/bridge_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ func (ncfg *networkConfiguration) MarshalJSON() ([]byte, error) {
nMap["BridgeName"] = ncfg.BridgeName
nMap["EnableIPv6"] = ncfg.EnableIPv6
nMap["EnableIPMasquerade"] = ncfg.EnableIPMasquerade
nMap["NDPProxyInterface"] = ncfg.NDPProxyInterface
nMap["EnableICC"] = ncfg.EnableICC
nMap["Mtu"] = ncfg.Mtu
nMap["Internal"] = ncfg.Internal
Expand Down Expand Up @@ -185,6 +186,9 @@ func (ncfg *networkConfiguration) UnmarshalJSON(b []byte) error {
ncfg.BridgeName = nMap["BridgeName"].(string)
ncfg.EnableIPv6 = nMap["EnableIPv6"].(bool)
ncfg.EnableIPMasquerade = nMap["EnableIPMasquerade"].(bool)
if v, ok := nMap["NDPProxyInterface"]; ok {
ncfg.NDPProxyInterface = v.(string)
}
ncfg.EnableICC = nMap["EnableICC"].(bool)
ncfg.Mtu = int(nMap["Mtu"].(float64))
if v, ok := nMap["Internal"]; ok {
Expand Down
23 changes: 23 additions & 0 deletions drivers/bridge/bridge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/docker/libnetwork/options"
"github.com/docker/libnetwork/testutils"
"github.com/docker/libnetwork/types"
"github.com/vishvananda/netlink"
)

func init() {
Expand Down Expand Up @@ -269,13 +270,15 @@ func TestCreateFullOptionsLabels(t *testing.T) {
gwV6s := "2001:db8:2600:2700:2800::25/80"
nwV6, _ := types.ParseCIDR(nwV6s)
gwV6, _ := types.ParseCIDR(gwV6s)
ndpPxyIface := "lo"

labels := map[string]string{
BridgeName: DefaultBridgeName,
DefaultBridge: "true",
EnableICC: "true",
EnableIPMasquerade: "true",
DefaultBindingIP: bndIPs,
NDPProxyInterface: ndpPxyIface,
}

netOption := make(map[string]interface{})
Expand Down Expand Up @@ -318,6 +321,10 @@ func TestCreateFullOptionsLabels(t *testing.T) {
t.Fatalf("incongruent EnableIPMasquerade in bridge network")
}

if nw.config.NDPProxyInterface != ndpPxyIface {
t.Fatalf("incongruend NDPProxyInterface in bridge network")
}

bndIP := net.ParseIP(bndIPs)
if !bndIP.Equal(nw.config.DefaultBindingIP) {
t.Fatalf("Unexpected: %v", nw.config.DefaultBindingIP)
Expand Down Expand Up @@ -347,6 +354,22 @@ func TestCreateFullOptionsLabels(t *testing.T) {
if te.Interface().AddressIPv6().IP.String() != "2001:db8:2600:2700:2800:aabb:ccdd:eeff" {
t.Fatalf("Unexpected endpoint IPv6 address: %v", te.Interface().AddressIPv6().IP)
}

// Check that the neighbor proxy was created by trying to delete it,
// because netlink.NeighList currently can't list proxies.
link, _ := d.nlh.LinkByName(ndpPxyIface)
err = d.nlh.NeighDel(&netlink.Neigh{
LinkIndex: link.Attrs().Index,
Family: netlink.FAMILY_V6,
State: netlink.NUD_PERMANENT,
Type: netlink.NDA_UNSPEC,
Flags: netlink.NTF_PROXY,
IP: te.Interface().AddressIPv6().IP,
HardwareAddr: te.Interface().MacAddress(),
})
if err != nil {
t.Fatalf("Cannot delete neighbor proxy, suggesting it wasn't created: %v", err)
}
}

func TestCreate(t *testing.T) {
Expand Down
3 changes: 3 additions & 0 deletions drivers/bridge/labels.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ const (
// EnableIPMasquerade label for bridge driver
EnableIPMasquerade = "com.docker.network.bridge.enable_ip_masquerade"

// NDPProxyInterface label for bridge driver
NDPProxyInterface = "com.docker.network.bridge.ndp_proxy_interface"

// EnableICC label
EnableICC = "com.docker.network.bridge.enable_icc"

Expand Down
31 changes: 31 additions & 0 deletions drivers/bridge/setup_ipv6.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ const (
ipv6ForwardConfPerm = 0644
ipv6ForwardConfDefault = "/proc/sys/net/ipv6/conf/default/forwarding"
ipv6ForwardConfAll = "/proc/sys/net/ipv6/conf/all/forwarding"
ndpProxyConfPerm = 0644
ndpProxyConfDefault = "/proc/sys/net/ipv6/conf/default/proxy_ndp"
ndpProxyConfAll = "/proc/sys/net/ipv6/conf/all/proxy_ndp"
)

func init() {
Expand Down Expand Up @@ -117,3 +120,31 @@ func setupIPv6Forwarding(config *networkConfiguration, i *bridgeInterface) error

return nil
}

func setupNDPProxying(config *networkConfiguration, i *bridgeInterface) error {
// Get current NDP default proxying setup
ndpProxyDataDefault, err := ioutil.ReadFile(ndpProxyConfDefault)
if err != nil {
return fmt.Errorf("Cannot read NDP default proxying setup: %v", err)
}
// Enable NDP default proxying only if it is not already enabled
if ndpProxyDataDefault[0] != '1' {
if err := ioutil.WriteFile(ndpProxyConfDefault, []byte{'1', '\n'}, ndpProxyConfPerm); err != nil {
logrus.Warnf("Unable to enable NDP default proxying: %v", err)
}
}

// Get current NDP all proxying setup
ndpProxyDataAll, err := ioutil.ReadFile(ndpProxyConfAll)
if err != nil {
return fmt.Errorf("Cannot read NDP all proxying setup: %v", err)
}
// Enable NDP all proxying only if it is not already enabled
if ndpProxyDataAll[0] != '1' {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it needed to enbale the proxying on all interfaces ? Would it work if we only enable it on the interface specified by the network driver option com.docker.network.bridge.ndp_proxy_interface=<iface> ?

Copy link

Choose a reason for hiding this comment

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

If I understood correctly, neighbours are announced/answered on the configured interface. Request made on other interfaces won't work.
Hence, as Docker doesn't know where the requests are coming from, it may be enabled on all interfaces or on those specified by a daemon param, isnt it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @mostolog for taking over this.
This is actually the most important comment. Would it work if we enable the ndp proxy only on the host interface specified in the driver option, instead of doing it for all the host interfaces ?

Copy link
Contributor

Choose a reason for hiding this comment

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

[We posted at the same time]

I think the host interface passed via driver options is the one that will advertise outside of the host that it provides reachability to the IPv6 segment assigned to the docker bridge network. Therefore I'd expect external IPv6 requests to the containers coming into the host via that same interface.

Copy link

Choose a reason for hiding this comment

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

IIUC, it's looping through networks in https://github.com/mostolog/libnetwork/blob/patch-1/drivers/bridge/bridge.go#L594 and setting ndp in https://github.com/mostolog/libnetwork/blob/patch-1/drivers/bridge/bridge.go#L686, right?

Would an if (network == com.docker.network.bridge.ndp_proxy_interface) to set enableNDPProxying work?

Copy link
Contributor

@aboch aboch Mar 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

No, the first loop you pointed out is only to detect if the to-be-created network's configuration conflicts with any existing network. And it ends at https://github.com/mostolog/libnetwork/blob/patch-1/drivers/bridge/bridge.go#L615

Then, the second line you linked

enableNDPProxying := config.NDPProxyInterface != "" && config.AddressIPv6 != nil

it's where the driver detects whether it does or does not need to program the ndp proxying for this network. This is just fine and does not need any changes.

The only one I was asking for is to see if it is enough for setupNDPProxying() to only enable proxying for the interface passed in the driver options, instead of enabling it for all the interfaces in the host.

The setup function has access to the ndp interface name because it is passed the network configuration:
func setupNDPProxying(config *networkConfiguration, i *bridgeInterface) error {

So I was asking that instead of etting ndpProxyConfAll, we should set

ndpProxyConfPath := fmt.Sprintf("/proc/sys/net/ipv6/conf/%s/proxy_ndp", config.NDPProxyInterface)
if err := ioutil.WriteFile(ndpProxyConfPath, []byte{'1', '\n'}, ndpProxyConfPerm); err != nil {
		logrus.Warnf("Unable to enable NDP proxying for interface %s: %v", config.NDPProxyInterface,err)
	}

Copy link

Choose a reason for hiding this comment

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

Committed on my patch-1 branch, however I'm not sure if it's ok. Please review and let me know if ready for PR

if err := ioutil.WriteFile(ndpProxyConfAll, []byte{'1', '\n'}, ndpProxyConfPerm); err != nil {
logrus.Warnf("Unable to enable NDP all proxying: %v", err)
}
}

return nil
}