Skip to content

Conversation

@luminitavoicu
Copy link
Contributor

@luminitavoicu luminitavoicu commented Jan 20, 2022

Reason for This PR

Provide a cleaner API for allowing MMDS requests on network interfaces.
Also fixes #2174

Description of Changes

  • Add a network_interfaces field that takes a list of the network interface IDs capable of forwarding packets to the MMDS. Network interface IDs mentioned must be valid at the time of this request. The net device model will reply to HTTP GET requests sent to the MMDS address via the interfaces mentioned. In this case, both ARP requests and TCP segments heading to ipv4_address are intercepted by the device model, and do not reach the associated TAP device.

  • Remove allow_mmds_requests from the body of the request that creates network interfaces.

  • This functionality can be added in rust-vmm.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The issue which led to this PR has a clear conclusion.
  • This PR follows the solution outlined in the related issue.
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any newly added unsafe code is properly documented.
  • Any API changes follow the Runbook for Firecracker API changes.
  • Any user-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

Enhance `/mmds/config` to contain a list of the network interface
IDs capable of forwarding packets to MMDS. Network interface IDs
mentioned must be valid at the time of this request. The net
device model will reply to HTTP GET requests sent to the MMDS
address via the interfaces mentioned. In this case, both ARP
requests and TCP segments heading to `ipv4_address` are
intercepted by the device model, and do not reach the associated
TAP device.

This implementation makes the `allow-mmds-requests` field set
when creating a net device redundant. Will remove the
`allow-mmds-requests` field in a subsequent commit.

Signed-off-by: Luminita Voicu <lumivo@amazon.com>
@luminitavoicu luminitavoicu self-assigned this Jan 20, 2022
Remove `allow_mmds_requests` from the API request that attaches
network interfaces. Interfaces that allow forwarding requests
to MMDS are now specified through the `network_interfaces`
field inside PUT requests to `/mmds/config`.

Signed-off-by: Luminita Voicu <lumivo@amazon.com>
Signed-off-by: Luminita Voicu <lumivo@amazon.com>
@luminitavoicu luminitavoicu force-pushed the net_ifaces_in_mmds_config branch from a778394 to 9b22233 Compare January 20, 2022 12:03
@luminitavoicu luminitavoicu changed the title Specify network interfaces that alo Specify network interfaces that accept MMDS requests through /mmds/config API Jan 20, 2022
@luminitavoicu luminitavoicu added NextRelease Status: Awaiting review Indicates that a pull request is ready to be reviewed labels Jan 20, 2022
Copy link
Contributor

@dianpopa dianpopa left a comment

Choose a reason for hiding this comment

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

There are some passages from the mmds doc that need updating, for example this one: https://github.com/firecracker-microvm/firecracker/blob/main/docs/mmds/mmds-user-guide.md#example.

Comment on lines +304 to +305
let _ = self.net_builder.build(body)?;
Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
let _ = self.net_builder.build(body)?;
Ok(())
let _ = self.net_builder.build(body)

@luminitavoicu
Copy link
Contributor Author

There are some passages from the mmds doc that need updating, for example this one: https://github.com/firecracker-microvm/firecracker/blob/main/docs/mmds/mmds-user-guide.md#example.

Indeed, there are many updates needed to the docs. I would prefer to include them in the PR that updates MMDS's behavior, since these changes are tightly coupled with MMDS. WDYT?

assert test_microvm.api_session.is_status_no_content(response.status_code)


def test_api_mmds_config(test_microvm_with_api):
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when an interface is configured via /network-interfaces, mmds is enabled on it, and then the interface deleted from /network-interfaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

        // Create `MmdsNetworkStack` and configure the IPv4 address for
        // existing built network devices whose names are defined in the
        // network interface ID list.
        for net_device in self.net_builder.iter_mut() {
            let mut net_device_lock = net_device.lock().expect("Poisoned lock");
            if network_interfaces.contains(net_device_lock.id()) {
                net_device_lock.configure_mmds_network_stack(ipv4_addr);
            } else {
                net_device_lock.disable_mmds_network_stack();
            }
        }

This piece of code from set_mmds_config() handles this case. We iterate through all network devices attached and if the ID of the net iface is present in the network_interfaces list provided through MmdsConfig, then we configure it to allow MMDS requests. Otherwise, we disable the MMDS Network Stack from that interface. This ensures that only the interfaces specified in the list accept forwarding requests to MMDS.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Radu was referring to the case where an user appends an interface and then deletes it. We do not offer a delete api endpoint in Firecracker. Also, the patch request on network interface allows updating of the rate limiters only.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Diana for clarifying! In this case simple is better

@luminitavoicu luminitavoicu merged commit f35f324 into firecracker-microvm:main Jan 21, 2022
@luminitavoicu luminitavoicu deleted the net_ifaces_in_mmds_config branch January 21, 2022 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Awaiting review Indicates that a pull request is ready to be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Align MMDS configuration and MMDS operation device requirements

4 participants