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

CalvoM · 2024-04-17T10:50:48Z

Proposed Commit Message

fix(azure): Avoid non-primary nics from having routes to DNS

From jammy, we have routes to dhcp by all NICS,since  RoutesToDns=true 
is the default in networkd.

Additional Context

From jammy, RouteToDns=true is the default for systemd-networkd.
This causes slow resolution of urls and failures in journalctl.
We need to limit the route to DHCP to the primary NIC.

Test Steps

CLOUD_INIT_PLATFORM=azure CLOUD_INIT_OS_IMAGE=jammy CLOUD_INIT_CLOUD_INIT_SOURCE=cloud-init-bddeb.deb tox -e integration-tests -- tests/integration_tests/datasources/test_azure.py::test_azure_multi_nic_setup

Checklist

My code follows the process laid out in the documentation
I have updated or added any unit tests accordingly
I have updated or added any documentation accordingly

Merge type

Squash merge using "Proposed Commit Message"
Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

aciba90 · 2024-04-18T14:34:29Z

Hey, @cjp256, could you also take a look at this one? Thanks.

danpdraper · 2024-04-18T18:40:09Z

cloudinit/sources/DataSourceAzure.py

+        "match": {"driver": "hv_netvsc", "name": "!eth0"},
+    }
+
+    netconfig["ethernets"]["hotpluggedeth0"] = {


How about naming the ethernets in the configuration primary and secondary instead of hotpluggedeth0 and ephemeral? I believe that the former better expresses the relationship between the two. That said, I am open to other suggestions.

Yeah. Agreed.

danpdraper · 2024-04-18T18:49:09Z

tests/integration_tests/datasources/test_azure.py

+    """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.


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.

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

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

danpdraper · 2024-04-18T18:54:28Z

@CalvoM Please update the commit message. As described in https://warthogs.atlassian.net/browse/CPC-3999 , the association of the DNS server with the secondary NIC is a problem as of at least Focal. I would also suggest that you launch Bionic and Xenial VMs to see how far back the fix needs to be ported.

cjp256 · 2024-04-18T20:00:50Z

I think I've only noticed RoutesToDns=True default in 22.04+. Will test this out.

Overall, I think maybe if this is an Azure-specific change, would it work to just conditionally add the use-dns flag here for all but the first nic (that is, idx != 0)?
https://github.com/CalvoM/cloud-init/blob/e41c2bca2535216336d09ba263e4130cbd13878a/cloudinit/sources/DataSourceAzure.py#L1993

Thanks for finding a solution!

aciba90

Functionally, the approach looks good to me, thanks.

SF: 00380708 says that the networkd change of defauts starts in jammy and onwards, do we need to patch this new config out for focal, or is this a noop change for focal and back? @CalvoM, @danpdraper.

tests/integration_tests/datasources/test_azure.py

Use the primary nic ip address when creating an VM with multi nics. Needed for canonical/cloud-init#5180

danpdraper · 2024-04-19T17:36:56Z

I think I've only noticed RoutesToDns=True default in 22.04+. Will test this out.

Overall, I think maybe if this is an Azure-specific change, would it work to just conditionally add the use-dns flag here for all but the first nic (that is, idx != 0)? https://github.com/CalvoM/cloud-init/blob/e41c2bca2535216336d09ba263e4130cbd13878a/cloudinit/sources/DataSourceAzure.py#L1993

Thanks for finding a solution!

@cjp256 Our investigation revealed that the association of the DNS server with the second NIC by systemd-resolved pre-dates Jammy (try running resolvectl dns on a Focal host). That said, DNS resolution latency only appears to be affected on Jammy+ hosts; the cloud-init logs on my Focal host revealed that the first instance of resolution of azure.archive.ubuntu.com was a sub-second operation. The systemd-resolved.service(8) man pages in both Focal and Jammy contain language to the following effect:

The lookup requests that systemd-resolved.service receives are routed to the available DNS servers, LLMNR, and MulticastDNS interfaces according to the following rules:
...

Queries for multi-label names are routed via unicast DNS on local interfaces that have a DNS server configured, plus the globally configured DNS servers if there are any. Which interfaces are used is determined by the routing logic based on search and route-only domains, described below.

...

If lookups are routed to multiple interfaces, the first successful response is returned...
...
The following query routing logic applies for unicast DNS lookups initiated by systemd-resolved.service:
...

If a query does not match any configured routing domain (either per-link or global), it is sent to all DNS servers that are configured on links with the DefaultRoute= option set, as well as the globally configured DNS server.

Assuming the foregoing is consistent with the implementation, DNS requests should be transmitted through both NICs on both Focal and Jammy hosts (DefaultRoute=Yes is set for both NICs on both hosts). This suggests that resolution latency should be affected on both hosts. That said, the statement "[i]f lookups are routed to multiple interfaces, the first successful response is returned" is ambiguous. Does it mean that systemd-resolved will forward the first successful response to the client as soon as it arrives, or wait for all responses to come back (or the requests to time-out) before returning the first successful response? I wonder if the former is true in Focal, and the latter is true in Jammy+?

In any case, @CalvoM 's solution will remove the secondary NIC from the DNS resolution path and thus will address any associated resolution latency.

danpdraper · 2024-04-19T17:44:04Z

Functionally, the approach looks good to me, thanks.

SF: 00380708 says that the networkd change of defauts starts in jammy and onwards, do we need to patch this new config out for focal, or is this a noop change for focal and back? @CalvoM, @danpdraper.

Good question. Even though resolution latency does not appear to be affected on hosts running pre-Jammy releases, we know that DNS resolution through the second NIC is not supported, so a suggestion in the systemd-resolved configuration that resolution through that NIC is supported would be a misrepresentation. As such, I believe we should back-port the change to Focal, as well as Bionic and Xenial if necessary (@CalvoM please confirm whether Bionic and Xenial are affected).

tests/integration_tests/datasources/test_azure.py

danpdraper · 2024-04-22T20:53:28Z

cloudinit/sources/DataSourceAzure.py

@@ -1945,6 +1948,28 @@ def load_azure_ds_dir(source_dir):
    return (md, ud, cfg, {"ovf-env.xml": contents})


+def configure_ephemeral_and_hotplug_device(netconfig: dict) -> dict:


We should probably rename this function to something like add_dhcp_config_for_primary_and_secondary_nics_to_network_config. The doc string should be updated accordingly.

CalvoM · 2024-05-06T08:55:45Z

We had a discussion with @danpdraper and we see Xenial is not affected by this issue, so we will not backport it to xenial.

aciba90

Thanks, @CalvoM, for fixing this. I have added some inline comments/requests.

tests/integration_tests/datasources/test_azure.py

aciba90 · 2024-05-06T15:59:58Z

tests/integration_tests/datasources/test_azure.py

+        else:
+            ret = snapshot_instance.execute("resolvectl dns")
+            assert ret.ok
+            routes = yaml.safe_load(ret.stdout)


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.

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.

tests/integration_tests/datasources/test_azure.py

aciba90 · 2024-05-06T16:31:43Z

cloudinit/sources/DataSourceAzure.py

@@ -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(


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

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

I understand that:

Synthetic interfaces are identifiable by the hv_netvsc driver.

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?

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*?

…hem. As from Bionic, systemd has associated DNS server with secondary NICs, this causes delay in the DNS resolution from Jammy and forwards. We see it evidently in the cloud-init logs where resolving the url takes way more seconds.

aciba90 · 2024-05-21T14:02:40Z

@cjp256:

Overall, I think maybe if this is an Azure-specific change, would it work to just conditionally add the use-dns flag here for all but the first nic (that is, idx != 0)? https://github.com/CalvoM/cloud-init/blob/e41c2bca2535216336d09ba263e4130cbd13878a/cloudinit/sources/DataSourceAzure.py#L1993

That would simplify the fix, but per:

The nics returned by the network call are not guaranteed to be in order.

in https://learn.microsoft.com/en-us/azure/virtual-machines/instance-metadata-service?tabs=linux#response-1

It looks like cloud-init cannot rely on positions in the IMDS response to determine which NIC is the primary one. Or is that ensured?

cjp256 · 2024-05-21T15:25:03Z

@cjp256:

Overall, I think maybe if this is an Azure-specific change, would it work to just conditionally add the use-dns flag here for all but the first nic (that is, idx != 0)? https://github.com/CalvoM/cloud-init/blob/e41c2bca2535216336d09ba263e4130cbd13878a/cloudinit/sources/DataSourceAzure.py#L1993

That would simplify the fix, but per:

The nics returned by the network call are not guaranteed to be in order.

in https://learn.microsoft.com/en-us/azure/virtual-machines/instance-metadata-service?tabs=linux#response-1

It looks like cloud-init cannot rely on positions in the IMDS response to determine which NIC is the primary one. Or is that ensured?

The primary nic will always be first in the response. The ordering of secondary NICs are not guaranteed today (but someday...). That why I think all we need to do is simplify the change to just add the use-dns flag for idx=0:
https://github.com/CalvoM/cloud-init/blob/e41c2bca2535216336d09ba263e4130cbd13878a/cloudinit/sources/DataSourceAzure.py#L1993

The driver/synthetic management is already handled in that method too.

aciba90 · 2024-06-04T14:49:18Z

Closing this as #5314 superseded.

CalvoM marked this pull request as ready for review April 17, 2024 11:38

CalvoM force-pushed the multi_nic_netplan_azure branch 4 times, most recently from d1e179d to 57ce7ba Compare April 17, 2024 15:22

CalvoM changed the title ~~fix(azure): Avoid non-primary nics from having routes to DNS~~ fix(azure): Avoid non-primary nics from having routes to DNS CPC-4224 Apr 17, 2024

CalvoM force-pushed the multi_nic_netplan_azure branch from 57ce7ba to b81625e Compare April 17, 2024 15:25

CalvoM mentioned this pull request Apr 18, 2024

fix(azure): Use correct ip when creating with multiple nics canonical/pycloudlib#362

Merged

CalvoM force-pushed the multi_nic_netplan_azure branch from b81625e to e41c2bc Compare April 18, 2024 09:47

aciba90 self-requested a review April 18, 2024 14:32

danpdraper suggested changes Apr 18, 2024

View reviewed changes

aciba90 requested changes Apr 19, 2024

View reviewed changes

tests/integration_tests/datasources/test_azure.py Outdated Show resolved Hide resolved

aciba90 pushed a commit to canonical/pycloudlib that referenced this pull request Apr 19, 2024

fix(azure): Use correct ip when creating with multiple nics (#362)

679f5fc

Use the primary nic ip address when creating an VM with multi nics. Needed for canonical/cloud-init#5180

aciba90 self-assigned this Apr 19, 2024

CalvoM force-pushed the multi_nic_netplan_azure branch 2 times, most recently from 903fd44 to f93dc8e Compare April 22, 2024 20:44

danpdraper suggested changes Apr 22, 2024

View reviewed changes

CalvoM force-pushed the multi_nic_netplan_azure branch from f93dc8e to 872674b Compare April 24, 2024 09:28

CalvoM force-pushed the multi_nic_netplan_azure branch from 872674b to 0e9748d Compare May 6, 2024 08:54

CalvoM requested review from aciba90 and danpdraper May 6, 2024 08:55

aciba90 requested changes May 6, 2024

View reviewed changes

CalvoM force-pushed the multi_nic_netplan_azure branch from 0e9748d to 0a06fb5 Compare May 8, 2024 08:48

aciba90 marked this pull request as draft May 21, 2024 15:43

aciba90 mentioned this pull request May 22, 2024

fix(azure): disable use-dns for secondary nics #5314

Merged

4 tasks

aciba90 closed this Jun 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

CalvoM commented Apr 17, 2024 •

edited

Loading

aciba90 commented Apr 18, 2024

danpdraper Apr 18, 2024

CalvoM Apr 22, 2024

danpdraper Apr 18, 2024

CalvoM Apr 19, 2024

CalvoM Apr 22, 2024

danpdraper commented Apr 18, 2024

cjp256 commented Apr 18, 2024 •

edited

Loading

aciba90 left a comment

danpdraper commented Apr 19, 2024 •

edited

Loading

danpdraper commented Apr 19, 2024

danpdraper Apr 22, 2024

CalvoM commented May 6, 2024

aciba90 left a comment

aciba90 May 6, 2024

CalvoM May 8, 2024

aciba90 May 6, 2024

CalvoM May 8, 2024 •

edited

Loading

aciba90 May 8, 2024

aciba90 May 8, 2024

aciba90 commented May 21, 2024

cjp256 commented May 21, 2024 •

edited

Loading

aciba90 commented Jun 4, 2024

		@@ -1945,6 +1948,28 @@ def load_azure_ds_dir(source_dir):
		return (md, ud, cfg, {"ovf-env.xml": contents})


		def configure_ephemeral_and_hotplug_device(netconfig: dict) -> dict:

		@@ -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(

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

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

Conversation

CalvoM commented Apr 17, 2024 • edited Loading

Proposed Commit Message

Additional Context

Test Steps

Checklist

Merge type

aciba90 commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpdraper commented Apr 18, 2024

cjp256 commented Apr 18, 2024 • edited Loading

aciba90 left a comment

Choose a reason for hiding this comment

danpdraper commented Apr 19, 2024 • edited Loading

danpdraper commented Apr 19, 2024

Choose a reason for hiding this comment

CalvoM commented May 6, 2024

aciba90 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CalvoM May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aciba90 commented May 21, 2024

cjp256 commented May 21, 2024 • edited Loading

aciba90 commented Jun 4, 2024

CalvoM commented Apr 17, 2024 •

edited

Loading

cjp256 commented Apr 18, 2024 •

edited

Loading

danpdraper commented Apr 19, 2024 •

edited

Loading

CalvoM May 8, 2024 •

edited

Loading

cjp256 commented May 21, 2024 •

edited

Loading