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

Jump to bottom

fix(azure): Avoid non-primary nics from having routes to DNS CPC-4224 #5180

Closed
wants to merge 1 commit into from
+121 −9
Closed

fix(azure): Avoid non-primary nics from having routes to DNS CPC-4224 #5180

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
39 changes: 34 additions & 5 deletions cloudinit/sources/DataSourceAzure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1482,18 +1482,23 @@ def availability_zone(self):
def _generate_network_config(self):
"""Generate network configuration according to configuration."""
# Use IMDS network metadata, if configured.
fxn = add_dhcp_config_for_primary_and_secondary_nics_to_network_config
if (
self._metadata_imds
and self._metadata_imds != sources.UNSET
and self.ds_cfg.get("apply_network_config")
):
try:
return generate_network_config_from_instance_network_metadata(
self._metadata_imds["network"],
apply_network_config_for_secondary_ips=self.ds_cfg.get(
"apply_network_config_for_secondary_ips"
),
netcfg = (
generate_network_config_from_instance_network_metadata(
self._metadata_imds["network"],
apply_network_config_for_secondary_ips=self.ds_cfg.get(
"apply_network_config_for_secondary_ips"
),
)
)
net_cfg = fxn(netcfg)
return net_cfg
except Exception as e:
LOG.error(
"Failed generating network config "
Expand Down Expand Up @@ -1945,6 +1950,30 @@ def load_azure_ds_dir(source_dir):
return (md, ud, cfg, {"ovf-env.xml": contents})


def add_dhcp_config_for_primary_and_secondary_nics_to_network_config(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this config only applied to NICs with hv_netvsc as driver?

Copy link
Contributor Author

@CalvoM CalvoM May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When a virtual machine (VM) is created in Azure, a synthetic network interface is created for each virtual NIC in its configuration. 
The synthetic interface is a VMbus device and uses the netvsc driver.
 Network packets that use this synthetic interface flow through the virtual switch in the Azure host and onto the datacenter's physical network.

The synthetic interface always has a name in the form eth\<n\>. 
Depending on the Linux distribution, the VF interface might have a name in the form eth\<n\>. 
Or it might have a different name in the form of enP\<n\> because of a udev rule that does renaming

Source: https://learn.microsoft.com/en-us/azure/virtual-network/accelerated-networking-how-it-works

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that:

  1. Synthetic interfaces are identifiable by the hv_netvsc driver.
  2. Synthetic interfaces are the only affected interface kinds affected by the dns problem?

Could please add a breadcrumb pointing to this, as a comment and/or in the commit message?

Copy link
Contributor

Choose a reason for hiding this comment

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

The synthetic interface always has a name in the form eth<n>.
Depending on the Linux distribution, the VF interface might have a name in the form eth<n>.
Or it might have a different name in the form of enP<n> because of a udev rule that does renaming

Your proposed solution only matches against eth* names, should we also match against enP\<n\> ones? Or is there a reason I do not see to restrict this to eth*?

netconfig: dict,
) -> dict:
"""
Configure the ephemeral and hotplug details.
parameters:
netconfig: dict, Network config details with other ethernet devices.
returns the updated network configuration
"""
netconfig["ethernets"]["primary"] = {
"dhcp4": True,
"match": {"driver": "hv_netvsc", "name": "eth0"},
}
netconfig["ethernets"]["secondary"] = {
"dhcp4": True,
"dhcp4-overrides": {"use-dns": False},
"optional": True,
"match": {"driver": "hv_netvsc", "name": "!eth0"},
}

return netconfig


@azure_ds_telemetry_reporter
def generate_network_config_from_instance_network_metadata(
network_metadata: dict,
Expand Down
45 changes: 44 additions & 1 deletion tests/integration_tests/datasources/test_azure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
import datetime

import pytest
import yaml
from pycloudlib.azure.util import AzureCreateParams, AzureParams
from pycloudlib.cloud import ImageType

from tests.integration_tests.clouds import IntegrationCloud
from tests.integration_tests.conftest import get_validated_source
from tests.integration_tests.instances import IntegrationInstance
from tests.integration_tests.integration_settings import PLATFORM
from tests.integration_tests.releases import CURRENT_RELEASE
from tests.integration_tests.releases import BIONIC, CURRENT_RELEASE


def _check_for_eject_errors(
Expand Down Expand Up @@ -45,3 +49,42 @@ def test_azure_eject(session_cloud: IntegrationCloud):
session_cloud.cloud_instance.delete_image(snapshot_id)
else:
_check_for_eject_errors(instance)


@pytest.mark.skipif(PLATFORM != "azure", reason="Test is Azure specific")
@pytest.mark.skipif(
CURRENT_RELEASE < BIONIC, reason="Easier to test on Bionic+"
)
def test_azure_multi_nic_setup(
setup_image, session_cloud: IntegrationCloud
) -> None:
"""Integration test for https://warthogs.atlassian.net/browse/CPC-3999.
Azure should have the primary NIC only route to DNS.
Ensure other NICs do not have route to DNS.
Copy link

Choose a reason for hiding this comment

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

I do not believe this test exercises the behaviour of concern. As described in https://warthogs.atlassian.net/browse/CPC-3999 , there is no route to the DHCP/DNS server through the secondary NIC on Mantic hosts (per ip route), but the DNS resolver has still associated the DNS server with the secondary NIC and thus transmits DNS queries through that NIC. As such, I believe that this test should instead verify that there is no longer a DNS server associated with the secondary NIC in the output of resolvectl dns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Let me see how parse resolvectl dns. Thanks for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bionic does not have resolvectl, so that will be interesting....will figure out how to update the test.

"""
us = datetime.datetime.now().strftime("%f")
rg_params = AzureParams(f"ci-test-multi-nic-setup-{us}", None)
nic_one = AzureCreateParams(f"ci-nic1-test-{us}", rg_params.name, None)
nic_two = AzureCreateParams(f"ci-nic2-test-{us}", rg_params.name, None)
with session_cloud.launch(
launch_kwargs={
"resource_group_params": rg_params,
"network_interfaces_params": [nic_one, nic_two],
}
) as snapshot_instance:
_check_for_eject_errors(snapshot_instance)
if CURRENT_RELEASE.series == "bionic":
aciba90 marked this conversation as resolved.
Show resolved Hide resolved
ret = snapshot_instance.execute("systemd-resolve --status")
assert ret.ok, ret.stderr
assert ret.stdout.count("Current Scopes: DNS") == 1
else:
ret = snapshot_instance.execute("resolvectl dns")
assert ret.ok, ret.stderr
routes = yaml.safe_load(ret.stdout)
Copy link
Contributor

Choose a reason for hiding this comment

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

Per RESOLVECTL(1) the output of this command is not yaml, the --json flag does nothing in the dns subcommand case and the routes can contain : possible invalidating the use of yaml.safe_load.

Not sure if the tests are going to hit this problem, but if we would like to be 100% safe here we should split by the first occurrence of : to correctly parse this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I would try coming up with a logic to parse the output. I think I saw the output was something similar to

Global:
Link 4 (enP13718s1):
Link 3 (eth1): 168.63.129.16
Link 2 (eth0): 168.63.129.16

Thus I thought yaml parsing will be easier.

routes_devices = list(routes.keys())
eth1_dev = [dev for dev in routes_devices if "(eth1)" in dev][0]
assert routes[eth1_dev] is None
aciba90 marked this conversation as resolved.
Show resolved Hide resolved
# check the instance can resolve something
res = snapshot_instance.execute("resolvectl query google.com")
assert res.ok, res.stderr
46 changes: 43 additions & 3 deletions tests/unittests/sources/test_azure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -976,7 +976,17 @@ def test_single_ipv4_nic_configuration(
"dhcp6": False,
"match": {"macaddress": "00:0d:3a:04:75:98"},
"set-name": "eth0",
}
},
"primary": {
"dhcp4": True,
"match": {"driver": "hv_netvsc", "name": "eth0"},
},
"secondary": {
"dhcp4": True,
"dhcp4-overrides": {"use-dns": False},
"optional": True,
"match": {"driver": "hv_netvsc", "name": "!eth0"},
},
},
"version": 2,
}
Expand Down Expand Up @@ -1557,7 +1567,17 @@ def test_network_config_set_from_imds(self):
"dhcp6": False,
"dhcp4": True,
"dhcp4-overrides": {"route-metric": 100},
}
},
"primary": {
"dhcp4": True,
"match": {"driver": "hv_netvsc", "name": "eth0"},
},
"secondary": {
"dhcp4": True,
"dhcp4-overrides": {"use-dns": False},
"optional": True,
"match": {"driver": "hv_netvsc", "name": "!eth0"},
},
},
"version": 2,
}
Expand Down Expand Up @@ -1595,6 +1615,16 @@ def test_network_config_set_from_imds_route_metric_for_secondary_nic(self):
"dhcp4": True,
"dhcp4-overrides": {"route-metric": 300},
},
"primary": {
"dhcp4": True,
"match": {"driver": "hv_netvsc", "name": "eth0"},
},
"secondary": {
"dhcp4": True,
"dhcp4-overrides": {"use-dns": False},
"optional": True,
"match": {"driver": "hv_netvsc", "name": "!eth0"},
},
},
"version": 2,
}
Expand Down Expand Up @@ -1626,7 +1656,17 @@ def test_network_config_set_from_imds_for_secondary_nic_no_ip(self):
"dhcp6": False,
"dhcp4": True,
"dhcp4-overrides": {"route-metric": 100},
}
},
"primary": {
"dhcp4": True,
"match": {"driver": "hv_netvsc", "name": "eth0"},
},
"secondary": {
"dhcp4": True,
"dhcp4-overrides": {"use-dns": False},
"optional": True,
"match": {"driver": "hv_netvsc", "name": "!eth0"},
},
},
"version": 2,
}
Expand Down
Loading