Fix error 400 when updating the mac address of a network interface #370

alxiord · 2018-06-21T10:26:12Z

Updating a network interface via PUT with a new MAC resulted in error 400 because the implementation attempted a second take() on the inner (unchanged) tap device.

Changes

reworked the PUT logic to support updates; the interfaces are now stored in a Vec instead of a LinkedList because, when updating the name of an interface, the tap device changes, meaning that the old NetworkInterfaceConfig needs to be removed and the new one added in its stead
added unit tests
added integration tests for all the update use cases
changed the default network interface configuration in the integration test to start with the ID 1 instead of 0 in order to avoid future confusion

Testing done

Build & unit tests

cargo fmt --all
cargo build
cargo build --release
sudo env PATH=$PATH cargo test --all
sudo env PATH=$PATH cargo kcov --all

File	Coverage
Total	%
device_config/net.rs	75.7%

Integration tests

sudo env PATH=$PATH python3 -m pytest

Manual tests

# add 1 network interface
curl --unix-socket /tmp/firecracker.socket -i\
      -X PUT "http://localhost/network-interfaces/1"\
      -H "accept: application/json"\
      -H "Content-Type: application/json"\
      -d "{ 
            \"iface_id\": \"1\", 
            \"host_dev_name\": \"vmtap33\", 
            \"guest_mac\": \"06:00:00:00:00:01\", 
            \"state\": \"Attached\" 
        }"
HTTP/1.1 201 Created
Content-Length: 0
Date: Mon, 18 Jun 2018 15:09:53 GMT

# try to add 2nd network interface with the same mac
curl --unix-socket /tmp/firecracker.socket -i\
     -X PUT "http://localhost/network-interfaces/2"\
      -H "accept: application/json"\
      -H "Content-Type: application/json"\
      -d "{ 
            \"iface_id\": \"2\", 
            \"host_dev_name\": \"ivmtap33\", 
            \"guest_mac\": \"06:00:00:00:00:01\", 
            \"state\": \"Attached\" 
        }"
HTTP/1.1 400 Bad Request
Content-Type: application/json
Transfer-Encoding: chunked
Date: Mon, 18 Jun 2018 15:11:08 GMT

{
  "fault_message": "The specified guest MAC address is already in use."
}

# add 2nd interface with different mac
curl --unix-socket /tmp/firecracker.socket -i\
     -X PUT "http://localhost/network-interfaces/2"\
      -H "accept: application/json"\
      -H "Content-Type: application/json"\
      -d "{ 
            \"iface_id\": \"2\", 
            \"host_dev_name\": \"ivmtap33\", 
            \"guest_mac\": \"06:00:00:00:00:02\", 
            \"state\": \"Attached\" 
        }"
HTTP/1.1 201 Created
Content-Length: 0
Date: Mon, 18 Jun 2018 15:11:56 GMT


# try to update 1st interface with the 2nd's mac
curl --unix-socket /tmp/firecracker.socket -i\
     -X PUT "http://localhost/network-interfaces/1"\
      -H "accept: application/json"\
      -H "Content-Type: application/json"\
      -d "{ 
            \"iface_id\": \"1\", 
            \"host_dev_name\": \"vmtap33\", 
            \"guest_mac\": \"06:00:00:00:00:02\", 
            \"state\": \"Attached\" 
        }"
HTTP/1.1 400 Bad Request
Content-Type: application/json
Transfer-Encoding: chunked
Date: Mon, 18 Jun 2018 15:12:46 GMT

{
  "fault_message": "The specified guest MAC address is already in use."
}

# update 1st interface with another mac
curl --unix-socket /tmp/firecracker.socket -i\
     -X PUT "http://localhost/network-interfaces/1"\
      -H "accept: application/json"\
      -H "Content-Type: application/json"\
      -d "{ 
            \"iface_id\": \"1\", 
            \"host_dev_name\": \"vmtap33\", 
            \"guest_mac\": \"06:00:00:00:00:03\", 
            \"state\": \"Attached\" 
        }"
HTTP/1.1 204 No Content
Date: Mon, 18 Jun 2018 15:13:14 GMT

# try to update 1st interface with the 2nd one's name
curl --unix-socket /tmp/firecracker.socket -i\
     -X PUT "http://localhost/network-interfaces/1"\
      -H "accept: application/json"\
      -H "Content-Type: application/json"\
      -d "{ 
            \"iface_id\": \"1\", 
            \"host_dev_name\": \"ivmtap33\", 
            \"guest_mac\": \"06:00:00:00:00:03\", 
            \"state\": \"Attached\" 
        }"
HTTP/1.1 400 Bad Request
Content-Type: application/json
Transfer-Encoding: chunked
Date: Mon, 18 Jun 2018 15:15:07 GMT

{
  "fault_message": "Could not open TAP device."
}

# update 1st interface with another name and mac
curl --unix-socket /tmp/firecracker.socket -i\
     -X PUT "http://localhost/network-interfaces/1"\
      -H "accept: application/json"\
      -H "Content-Type: application/json"\
      -d "{ 
            \"iface_id\": \"1\", 
            \"host_dev_name\": \"vmtap34\", 
            \"guest_mac\": \"06:00:00:00:00:04\", 
            \"state\": \"Attached\" 
        }"
HTTP/1.1 204 No Content
Date: Mon, 18 Jun 2018 15:14:13 GMT

dianpopa · 2018-06-26T16:59:20Z

vmm/src/device_config/net.rs

+    }
+
+    fn push(&mut self, body: NetworkInterfaceBody) -> Result<(), SyncError> {
+        self.validate_unique_mac(&body.guest_mac)?;


This check is valid for all scenarios (create, update). Why not use this only once right after pub fn put function before any add/update logic starts happening?

There's a corner case:

update scenario (ID stays the same)

tap device name changes (new device)

mac address stays the same

This can be addressed either by taking the ID into account in validate_unique_mac or by calling validate_unique_mac before every push / update where it's needed. I chose the latter because I didn't want to bring in the ID in a validation function.

I get it now.

dianpopa · 2018-06-26T17:12:19Z

vmm/src/device_config/net.rs

-            }
-        }
-        self.if_list.push_back(cfg);
+    fn add(&mut self, body: NetworkInterfaceBody) -> result::Result<SyncOkStatus, SyncError> {


I would rename the add function to create given that the outcomes when calling put are two: Ok(SyncOkStatus::Updated) and Ok(SyncOkStatus::Created) respectively. It is confusing when you read the code to see: add, create and push altogether. I would move in this function all the code from the push function (i.e. try_from_body and adding it to the vector of configs) and delete the push function alltogether. Inside the update you would remove the old config, create a new one calling create and the function would still return Ok(SyncOkStatus::Updated). i think this would make the code easier to read.

dianpopa · 2018-06-26T17:12:57Z

vmm/src/device_config/net.rs

+        assert!(netif_configs.put(netif_body).is_ok());
+        assert_eq!(netif_configs.if_list.len(), 1);
+
+        // Try to add another interface with the same MAC


nit: full stop for cosmetic purposes

dianpopa · 2018-06-26T17:13:01Z

vmm/src/device_config/net.rs

+        assert!(netif_configs.put(other_netif_body.clone()).is_err());
+        assert_eq!(netif_configs.if_list.len(), 1);
+
+        // Add another interface


nit: full stop

dianpopa · 2018-06-26T17:13:06Z

vmm/src/device_config/net.rs

+            .push(make_netif_cfg(other_netif_body.clone(), "foo"));
+        assert_eq!(netif_configs.if_list.len(), 2);
+
+        // Try to update with an unavailable name


nit: full stop

dianpopa · 2018-06-26T17:13:10Z

vmm/src/device_config/net.rs

+        assert!(netif_configs.put(other_netif_body.clone()).is_err());
+        assert_eq!(netif_configs.if_list.len(), 2);
+
+        // Try to update with an unavailable MAC


nit: full stop

alxiord · 2018-06-27T14:25:18Z

FYI this doesn fix the problem that appears when issuing InstanceStart after InstanceHalt because the tap object is taken during InstanceStart and never put back.

Changing the MAC address resulted in error 400 because the inner tap object was already taken. Added support to leave the tap in place and update the other fields, unless the interface's name changes. Fixes #354 Signed-off-by: Alexandra Iordache <[email protected]>

The network interfaces update test case only covered interface name update, which worked because a new tap device was created. See #354 Signed-off-by: Alexandra Iordache <[email protected]>

alxiord mentioned this pull request Jun 21, 2018

Added some more vmm unit tests #367

Merged

alxiord self-assigned this Jun 21, 2018

acatangiu changed the title ~~Fix error 400 when updating the mac address of a netork interface~~ Fix error 400 when updating the mac address of a network interface Jun 22, 2018

alxiord added the Type: Bug Indicates an unexpected problem or unintended behavior label Jun 22, 2018

alxiord added this to the Customer 2 Production milestone Jun 22, 2018

dianpopa requested a review from a team June 26, 2018 16:13

dianpopa reviewed Jun 26, 2018

View reviewed changes

dianpopa previously approved these changes Jun 27, 2018

View reviewed changes

alxiord dismissed dianpopa’s stale review via 360876e June 28, 2018 09:29

dianpopa previously approved these changes Jun 28, 2018

View reviewed changes

acatangiu dismissed dianpopa’s stale review via 4b2d745 June 28, 2018 15:26

dianpopa approved these changes Jun 28, 2018

View reviewed changes

Alexandra Iordache added 2 commits June 28, 2018 18:28

Integration tests for all network update use cases

4b2d745

The network interfaces update test case only covered interface name update, which worked because a new tap device was created. See #354 Signed-off-by: Alexandra Iordache <[email protected]>

acatangiu approved these changes Jun 28, 2018

View reviewed changes

acatangiu merged commit 7207fde into firecracker-microvm:master Jun 28, 2018

alxiord deleted the mac branch September 5, 2018 09:15

hyandell unassigned alxiord Nov 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix error 400 when updating the mac address of a network interface #370

Fix error 400 when updating the mac address of a network interface #370

Uh oh!

alxiord commented Jun 21, 2018

Uh oh!

dianpopa Jun 26, 2018

Uh oh!

alxiord Jun 27, 2018

Uh oh!

dianpopa Jun 27, 2018

Uh oh!

dianpopa Jun 26, 2018

Uh oh!

alxiord Jun 27, 2018

Uh oh!

dianpopa Jun 26, 2018

Uh oh!

alxiord Jun 27, 2018

Uh oh!

dianpopa Jun 26, 2018

Uh oh!

alxiord Jun 27, 2018

Uh oh!

dianpopa Jun 26, 2018

Uh oh!

alxiord Jun 27, 2018

Uh oh!

dianpopa Jun 26, 2018

Uh oh!

alxiord Jun 27, 2018

Uh oh!

alxiord commented Jun 27, 2018

Uh oh!

Uh oh!

Fix error 400 when updating the mac address of a network interface #370

Fix error 400 when updating the mac address of a network interface #370

Uh oh!

Conversation

alxiord commented Jun 21, 2018

Changes

Testing done

Build & unit tests

Integration tests

Manual tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alxiord commented Jun 27, 2018

Uh oh!

Uh oh!