Browse code

libnet/d/bridge: don't parse the MacAddress netlabel

Libnet's method `(*Network).createEndpoint()` is already parsing this
netlabel to set the field `ep.iface.mac`. Later on, this same method
invoke the driver's method `CreateEndpoint` with an `InterfaceInfo` arg
and an `options` arg (an opaque map of driver otps).

The `InterfaceInfo` interface contains a `MacAddress()` method that
returns `ep.iface.mac`. And the opaque map may contain the key
`netlabel.MacAddress`.

Prior to this change, the bridge driver was calling `MacAddress()`. If
no value was returned, it'd fall back to the option set in the `options`
map, or generate a MAC address based on the IP address.

However, the expected type of the `options` value is a `net.HardwareAddr`.
This is what's set by the daemon when handing over the endpoint config
to libnet controller. If the value is a string, as is the case if the
MAC address is provided through `EndpointsSettings.DriverOpts`, it
produces an error.

As such, the opaque option and the `MacAddress()` are necessarily the
same -- either nothing or a `net.HardwareAddr`. No need to keep both.

Moreover, the struct `endpointConfiguration` was only used to store that
netlabel value. Drop it too.

Signed-off-by: Albin Kerouanton <albinker@gmail.com>

Albin Kerouanton authored on 2024/03/06 18:46:58
Showing 4 changed files
... ...
@@ -91,11 +91,6 @@ const (
91 91
 	ifaceCreatedByUser
92 92
 )
93 93
 
94
-// endpointConfiguration represents the user specified configuration for the sandbox endpoint
95
-type endpointConfiguration struct {
96
-	MacAddress net.HardwareAddr
97
-}
98
-
99 94
 // containerConfiguration represents the user specified configuration for a container
100 95
 type containerConfiguration struct {
101 96
 	ParentEndpoints []string
... ...
@@ -115,7 +110,6 @@ type bridgeEndpoint struct {
115 115
 	addr            *net.IPNet
116 116
 	addrv6          *net.IPNet
117 117
 	macAddress      net.HardwareAddr
118
-	config          *endpointConfiguration // User specified parameters
119 118
 	containerConfig *containerConfiguration
120 119
 	extConnConfig   *connectivityConfiguration
121 120
 	portMapping     []types.PortBinding // Operation port bindings
... ...
@@ -926,7 +920,7 @@ func setHairpinMode(nlh *netlink.Handle, link netlink.Link, enable bool) error {
926 926
 	return nil
927 927
 }
928 928
 
929
-func (d *driver) CreateEndpoint(nid, eid string, ifInfo driverapi.InterfaceInfo, epOptions map[string]interface{}) error {
929
+func (d *driver) CreateEndpoint(nid, eid string, ifInfo driverapi.InterfaceInfo, _ map[string]interface{}) error {
930 930
 	if ifInfo == nil {
931 931
 		return errors.New("invalid interface info passed")
932 932
 	}
... ...
@@ -963,15 +957,9 @@ func (d *driver) CreateEndpoint(nid, eid string, ifInfo driverapi.InterfaceInfo,
963 963
 		return driverapi.ErrEndpointExists(eid)
964 964
 	}
965 965
 
966
-	// Try to convert the options to endpoint configuration
967
-	epConfig, err := parseEndpointOptions(epOptions)
968
-	if err != nil {
969
-		return err
970
-	}
971
-
972 966
 	// Create and add the endpoint
973 967
 	n.Lock()
974
-	endpoint := &bridgeEndpoint{id: eid, nid: nid, config: epConfig}
968
+	endpoint := &bridgeEndpoint{id: eid, nid: nid}
975 969
 	n.endpoints[eid] = endpoint
976 970
 	n.Unlock()
977 971
 
... ...
@@ -1065,10 +1053,12 @@ func (d *driver) CreateEndpoint(nid, eid string, ifInfo driverapi.InterfaceInfo,
1065 1065
 	endpoint.addr = ifInfo.Address()
1066 1066
 	endpoint.addrv6 = ifInfo.AddressIPv6()
1067 1067
 
1068
-	// Set the sbox's MAC if not provided. If specified, use the one configured by user, otherwise generate one based on IP.
1068
+	// We assign a default MAC address derived from the IP address to make sure
1069
+	// that if a container is disconnected and reconnected in a short timeframe,
1070
+	// stale ARP entries will still point to the right container.
1069 1071
 	if endpoint.macAddress == nil {
1070
-		endpoint.macAddress = electMacAddress(epConfig, endpoint.addr.IP)
1071
-		if err = ifInfo.SetMacAddress(endpoint.macAddress); err != nil {
1072
+		endpoint.macAddress = netutils.GenerateMACFromIP(endpoint.addr.IP)
1073
+		if err := ifInfo.SetMacAddress(endpoint.macAddress); err != nil {
1072 1074
 			return err
1073 1075
 		}
1074 1076
 	}
... ...
@@ -1463,24 +1453,6 @@ func (d *driver) IsBuiltIn() bool {
1463 1463
 	return true
1464 1464
 }
1465 1465
 
1466
-func parseEndpointOptions(epOptions map[string]interface{}) (*endpointConfiguration, error) {
1467
-	if epOptions == nil {
1468
-		return nil, nil
1469
-	}
1470
-
1471
-	ec := &endpointConfiguration{}
1472
-
1473
-	if opt, ok := epOptions[netlabel.MacAddress]; ok {
1474
-		if mac, ok := opt.(net.HardwareAddr); ok {
1475
-			ec.MacAddress = mac
1476
-		} else {
1477
-			return nil, &ErrInvalidEndpointConfig{}
1478
-		}
1479
-	}
1480
-
1481
-	return ec, nil
1482
-}
1483
-
1484 1466
 func parseContainerOptions(cOptions map[string]interface{}) (*containerConfiguration, error) {
1485 1467
 	if cOptions == nil {
1486 1468
 		return nil, nil
... ...
@@ -1528,10 +1500,3 @@ func parseConnectivityOptions(cOptions map[string]interface{}) (*connectivityCon
1528 1528
 
1529 1529
 	return cc, nil
1530 1530
 }
1531
-
1532
-func electMacAddress(epConfig *endpointConfiguration, ip net.IP) net.HardwareAddr {
1533
-	if epConfig != nil && epConfig.MacAddress != nil {
1534
-		return epConfig.MacAddress
1535
-	}
1536
-	return netutils.GenerateMACFromIP(ip)
1537
-}
... ...
@@ -35,7 +35,6 @@ func TestEndpointMarshalling(t *testing.T) {
35 35
 		addrv6:     ip2,
36 36
 		macAddress: mac,
37 37
 		srcName:    "veth123456",
38
-		config:     &endpointConfiguration{MacAddress: mac},
39 38
 		containerConfig: &containerConfiguration{
40 39
 			ParentEndpoints: []string{"one", "due", "three"},
41 40
 			ChildEndpoints:  []string{"four", "five", "six"},
... ...
@@ -90,7 +89,6 @@ func TestEndpointMarshalling(t *testing.T) {
90 90
 
91 91
 	if e.id != ee.id || e.nid != ee.nid || e.srcName != ee.srcName || !bytes.Equal(e.macAddress, ee.macAddress) ||
92 92
 		!types.CompareIPNet(e.addr, ee.addr) || !types.CompareIPNet(e.addrv6, ee.addrv6) ||
93
-		!compareEpConfig(e.config, ee.config) ||
94 93
 		!compareContainerConfig(e.containerConfig, ee.containerConfig) ||
95 94
 		!compareConnConfig(e.extConnConfig, ee.extConnConfig) ||
96 95
 		!compareBindings(e.portMapping, ee.portMapping) {
... ...
@@ -98,16 +96,6 @@ func TestEndpointMarshalling(t *testing.T) {
98 98
 	}
99 99
 }
100 100
 
101
-func compareEpConfig(a, b *endpointConfiguration) bool {
102
-	if a == b {
103
-		return true
104
-	}
105
-	if a == nil || b == nil {
106
-		return false
107
-	}
108
-	return bytes.Equal(a.MacAddress, b.MacAddress)
109
-}
110
-
111 101
 func compareContainerConfig(a, b *containerConfiguration) bool {
112 102
 	if a == b {
113 103
 		return true
... ...
@@ -385,10 +373,9 @@ func TestCreateFullOptionsLabels(t *testing.T) {
385 385
 
386 386
 	// In short here we are testing --fixed-cidr-v6 daemon option
387 387
 	// plus --mac-address run option
388
-	mac, _ := net.ParseMAC("aa:bb:cc:dd:ee:ff")
389
-	epOptions := map[string]interface{}{netlabel.MacAddress: mac}
390 388
 	te := newTestEndpoint(ipdList[0].Pool, 20)
391
-	err = d.CreateEndpoint("dummy", "ep1", te.Interface(), epOptions)
389
+	te.iface.mac = netutils.MustParseMAC("aa:bb:cc:dd:ee:ff")
390
+	err = d.CreateEndpoint("dummy", "ep1", te.Interface(), map[string]interface{}{})
392 391
 	if err != nil {
393 392
 		t.Fatal(err)
394 393
 	}
... ...
@@ -264,7 +264,6 @@ func (ep *bridgeEndpoint) MarshalJSON() ([]byte, error) {
264 264
 	if ep.addrv6 != nil {
265 265
 		epMap["Addrv6"] = ep.addrv6.String()
266 266
 	}
267
-	epMap["Config"] = ep.config
268 267
 	epMap["ContainerConfig"] = ep.containerConfig
269 268
 	epMap["ExternalConnConfig"] = ep.extConnConfig
270 269
 	epMap["PortMapping"] = ep.portMapping
... ...
@@ -300,11 +299,7 @@ func (ep *bridgeEndpoint) UnmarshalJSON(b []byte) error {
300 300
 	ep.id = epMap["id"].(string)
301 301
 	ep.nid = epMap["nid"].(string)
302 302
 	ep.srcName = epMap["SrcName"].(string)
303
-	d, _ := json.Marshal(epMap["Config"])
304
-	if err := json.Unmarshal(d, &ep.config); err != nil {
305
-		log.G(context.TODO()).Warnf("Failed to decode endpoint config %v", err)
306
-	}
307
-	d, _ = json.Marshal(epMap["ContainerConfig"])
303
+	d, _ := json.Marshal(epMap["ContainerConfig"])
308 304
 	if err := json.Unmarshal(d, &ep.containerConfig); err != nil {
309 305
 		log.G(context.TODO()).Warnf("Failed to decode endpoint container config %v", err)
310 306
 	}
... ...
@@ -149,3 +149,12 @@ func IsV6Listenable() bool {
149 149
 	})
150 150
 	return v6ListenableCached
151 151
 }
152
+
153
+// MustParseMAC returns a net.HardwareAddr or panic.
154
+func MustParseMAC(s string) net.HardwareAddr {
155
+	mac, err := net.ParseMAC(s)
156
+	if err != nil {
157
+		panic(err)
158
+	}
159
+	return mac
160
+}