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.

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

fix: Update DNS behavior for NetworkManager interfaces #5496

Merged
merged 3 commits into from
Jul 18, 2024

Conversation

jcmoore3
Copy link
Contributor

@jcmoore3 jcmoore3 commented Jul 8, 2024

If DNS information is added to a NetworkManager managed interface where the given protocol family is disabled, NetworkManager will be unable to activate the interface.

Proposed Commit Message

fix: Update DNS behavior for NetworkManager interfaces

If DNS information is added to a NetworkManager managed interface where
the given protocol family is disabled, NetworkManager will be unable to
activate the interface.

Additional Context

Test Steps

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>)

Copy link
Contributor

@aciba90 aciba90 left a comment

Choose a reason for hiding this comment

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

Thanks, @jcmoore3, for addressing this.

I don't see anything preventing having ipv{4,6}.dns configured when ipv{4,6}.method == disabled, see [1] and [2]. I did a quick search on the NetworkManager issue tracker and did not find any issue related to this. Before potentially fixing a non-issue, could we create an issue upstream for discussion and another one in cloud-init with steps to reproduce it, logs, etc, please?

[1] https://www.networkmanager.dev/docs/api/latest/settings-ipv4.html
[2] https://www.networkmanager.dev/docs/api/latest/settings-ipv6.html

@aciba90 aciba90 self-assigned this Jul 11, 2024
@jcmoore3
Copy link
Contributor Author

Looking at the NetworkManager docs, I agree that nothing there indicates that ipv{4,6}.dns is disallowed when ipv{4,6}.method=disabled.

However, here is the NetworkManager log output noting that the ipv{4,6}.dns entry is invalid when the connection's ipv{4,6}.method=disabled:

keyfile: load: "/etc/NetworkManager/system-connections/cloud-init-bond0.nmconnection": failed to load connection: invalid connection: ipv4.dns: this property is not allowed for 'method=disabled'

This is running on Rocky 9 with NetworkManager-1.46.0-8.el9_4

Given this seems like intentional behavior based on the NetworkManager log message, should we still open an upstream NetworkManager issue?

Copy link
Contributor

@aciba90 aciba90 left a comment

Choose a reason for hiding this comment

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

Thanks for pointing that out.

I see it in the code: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/main/src/libnm-core-impl/nm-setting-ip6-config.c?ref_type=heads#L297.

should we still open an upstream NetworkManager issue?

We could do it requesting to clarify the documentation, but that is independent of this PR.

Could we minimally add a unit-test covering this change?

@aciba90
Copy link
Contributor

aciba90 commented Jul 12, 2024

Could you please add these unit tests? Feel free to modify them to better represent your use-case, but they cover the changes you are adding:

Patch
commit a5a5a8899f6703391c8a3fd10d2bf2c053978df9
Author: Alberto Contreras <[email protected]>
Date:   Fri Jul 12 17:24:29 2024 +0200

    add unit-test

diff --git a/tests/unittests/net/test_network_manager.py b/tests/unittests/net/test_network_manager.py
new file mode 100644
index 000000000..739302e82
--- /dev/null
+++ b/tests/unittests/net/test_network_manager.py
@@ -0,0 +1,317 @@
+# This file is part of cloud-init. See LICENSE file for license information.
+
+import textwrap
+from unittest import mock
+
+import yaml
+
+from cloudinit.net import network_manager, network_state
+from tests.unittests.helpers import dir2dict
+
+
+def assert_equal_dict(expected_d, found_d):
+    for p, c in expected_d.items():
+        if p not in found_d:
+            continue
+        assert c == found_d[p]
+
+
+class TestNetworkManagerRenderNetworkState:
+    def _parse_network_state_from_config(self, config):
+        with mock.patch("cloudinit.net.network_state.get_interfaces_by_mac"):
+            config = yaml.safe_load(config)
+            return network_state.parse_net_config_data(config)
+
+    def test_bond_dns_baseline(self, tmpdir):
+
+        config = textwrap.dedent(
+            """\
+            version: 1
+            config:
+              - mac_address: 'xx:xx:xx:xx:xx:00'
+                mtu: 9000
+                name: ens1f0np0
+                subnets: []
+                type: physical
+              - mac_address: 'xx:xx:xx:xx:xx:01'
+                mtu: 9000
+                name: ens1f1np1
+                subnets: []
+                type: physical
+              - bond_interfaces:
+                  - ens1f0np0
+                  - ens1f1np1
+                mac_address: 'xx:xx:xx:xx:xx:00'
+                mtu: 9000
+                name: bond0
+                params:
+                  bond-miimon: 100
+                  bond-mode: 802.3ad
+                  bond-xmit_hash_policy: layer3+4
+                subnets: []
+                type: bond
+              - name: bond0.123
+                subnets:
+                  - address: 0.0.0.0
+                    ipv4: true
+                    netmask: 255.255.255.0
+                    prefix: 24
+                    routes:
+                      - gateway: 0.0.0.1
+                        netmask: 0.0.0.0
+                        network: 0.0.0.0
+                    type: static
+                type: vlan
+                vlan_id: 123
+                vlan_link: bond0
+              - address: 1.1.1.1
+                search: hostname1
+                type: nameserver
+            """
+        )
+
+        expected_config = {
+            "/etc/NetworkManager/system-connections/cloud-init-ens1f0np0.nmconnection": textwrap.dedent(  # noqa: E501
+                """\
+                # Generated by cloud-init. Changes will be lost.
+
+                [connection]
+                id=cloud-init ens1f0np0
+                uuid=99c4bf6c-1691-53c4-bfe8-abdcb90b278a
+                autoconnect-priority=120
+                type=ethernet
+                slave-type=bond
+                master=54317911-f840-516b-a10d-82cb4c1f075c
+
+                [user]
+                org.freedesktop.NetworkManager.origin=cloud-init
+
+                [ethernet]
+                mtu=9000
+                mac-address=XX:XX:XX:XX:XX:00
+
+                """
+            ),
+            "/etc/NetworkManager/system-connections/cloud-init-ens1f1np1.nmconnection": textwrap.dedent(  # noqa: E501
+                """\
+                # Generated by cloud-init. Changes will be lost.
+
+                [connection]
+                id=cloud-init ens1f1np1
+                uuid=2685ec2b-1c26-583d-a660-0ab24201fef3
+                autoconnect-priority=120
+                type=ethernet
+                slave-type=bond
+                master=54317911-f840-516b-a10d-82cb4c1f075c
+
+                [user]
+                org.freedesktop.NetworkManager.origin=cloud-init
+
+                [ethernet]
+                mtu=9000
+                mac-address=XX:XX:XX:XX:XX:01
+
+                """
+            ),
+            "/etc/NetworkManager/system-connections/cloud-init-bond0.nmconnection": textwrap.dedent(  # noqa: E501
+                """\
+                # Generated by cloud-init. Changes will be lost.
+
+                [connection]
+                id=cloud-init bond0
+                uuid=54317911-f840-516b-a10d-82cb4c1f075c
+                autoconnect-priority=120
+                type=bond
+                interface-name=bond0
+
+                [user]
+                org.freedesktop.NetworkManager.origin=cloud-init
+
+                [bond]
+                mode=802.3ad
+
+                """
+            ),
+            "/etc/NetworkManager/system-connections/cloud-init-bond0.123.nmconnection": textwrap.dedent(  # noqa: E501
+                """\
+                # Generated by cloud-init. Changes will be lost.
+
+                [connection]
+                id=cloud-init bond0.123
+                uuid=7541e7a5-450b-570b-b3e8-a7f9eced114a
+                autoconnect-priority=120
+                type=vlan
+                interface-name=bond0.123
+
+                [user]
+                org.freedesktop.NetworkManager.origin=cloud-init
+
+                [vlan]
+                id=123
+                parent=54317911-f840-516b-a10d-82cb4c1f075c
+
+                [ipv4]
+                method=manual
+                may-fail=false
+                address1=0.0.0.0/24
+                route1=0.0.0.0/0,0.0.0.1
+                dns=1.1.1.1;
+                dns-search=hostname1;
+
+                """
+            ),
+        }
+        with mock.patch("cloudinit.net.get_interfaces_by_mac"):
+            ns = self._parse_network_state_from_config(config)
+            target = str(tmpdir)
+            network_manager.Renderer().render_network_state(ns, target=target)
+            rendered_content = dir2dict(target)
+            assert_equal_dict(expected_config, rendered_content)
+
+    def test_bond_dns_redacted_with_method_disabled(self, tmpdir):
+
+        config = textwrap.dedent(
+            """\
+            version: 1
+            config:
+              - mac_address: 'xx:xx:xx:xx:xx:00'
+                mtu: 9000
+                name: ens1f0np0
+                subnets: []
+                type: physical
+              - mac_address: 'xx:xx:xx:xx:xx:01'
+                mtu: 9000
+                name: ens1f1np1
+                subnets: []
+                type: physical
+              - bond_interfaces:
+                  - ens1f0np0
+                  - ens1f1np1
+                mac_address: 'xx:xx:xx:xx:xx:00'
+                mtu: 9000
+                name: bond0
+                params:
+                  bond-miimon: 100
+                  bond-mode: 802.3ad
+                  bond-xmit_hash_policy: layer3+4
+                subnets: []
+                type: bond
+              - name: bond0.123
+                subnets:
+                  - address: 0.0.0.0
+                    ipv4: true
+                    netmask: 255.255.255.0
+                    prefix: 24
+                    routes:
+                      - gateway: 0.0.0.1
+                        netmask: 0.0.0.0
+                        network: 0.0.0.0
+                    type: ipv6_slaac  # !! to force ipvx.method to be disabled
+                type: vlan
+                vlan_id: 123
+                vlan_link: bond0
+              - address: 1.1.1.1
+                search: hostname1
+                type: nameserver
+            """
+        )
+
+        expected_config = {
+            "/etc/NetworkManager/system-connections/cloud-init-ens1f0np0.nmconnection": textwrap.dedent(  # noqa: E501
+                """\
+                # Generated by cloud-init. Changes will be lost.
+
+                [connection]
+                id=cloud-init ens1f0np0
+                uuid=99c4bf6c-1691-53c4-bfe8-abdcb90b278a
+                autoconnect-priority=120
+                type=ethernet
+                slave-type=bond
+                master=54317911-f840-516b-a10d-82cb4c1f075c
+
+                [user]
+                org.freedesktop.NetworkManager.origin=cloud-init
+
+                [ethernet]
+                mtu=9000
+                mac-address=XX:XX:XX:XX:XX:00
+
+                """
+            ),
+            "/etc/NetworkManager/system-connections/cloud-init-ens1f1np1.nmconnection": textwrap.dedent(  # noqa: E501
+                """\
+                # Generated by cloud-init. Changes will be lost.
+
+                [connection]
+                id=cloud-init ens1f1np1
+                uuid=2685ec2b-1c26-583d-a660-0ab24201fef3
+                autoconnect-priority=120
+                type=ethernet
+                slave-type=bond
+                master=54317911-f840-516b-a10d-82cb4c1f075c
+
+                [user]
+                org.freedesktop.NetworkManager.origin=cloud-init
+
+                [ethernet]
+                mtu=9000
+                mac-address=XX:XX:XX:XX:XX:01
+
+                """
+            ),
+            "/etc/NetworkManager/system-connections/cloud-init-bond0.nmconnection": textwrap.dedent(  # noqa: E501
+                """\
+                # Generated by cloud-init. Changes will be lost.
+
+                [connection]
+                id=cloud-init bond0
+                uuid=54317911-f840-516b-a10d-82cb4c1f075c
+                autoconnect-priority=120
+                type=bond
+                interface-name=bond0
+
+                [user]
+                org.freedesktop.NetworkManager.origin=cloud-init
+
+                [bond]
+                mode=802.3ad
+
+                """
+            ),
+            "/etc/NetworkManager/system-connections/cloud-init-bond0.123.nmconnection": textwrap.dedent(  # noqa: E501
+                """\
+                # Generated by cloud-init. Changes will be lost.
+
+                [connection]
+                id=cloud-init bond0.123
+                uuid=7541e7a5-450b-570b-b3e8-a7f9eced114a
+                autoconnect-priority=120
+                type=vlan
+                interface-name=bond0.123
+
+                [user]
+                org.freedesktop.NetworkManager.origin=cloud-init
+
+                [vlan]
+                id=123
+                parent=54317911-f840-516b-a10d-82cb4c1f075c
+
+                [ipv6]
+                method=auto
+                may-fail=false
+                address1=0.0.0.0/24
+                dns-search=hostname1;
+
+                [ipv4]
+                method=disabled
+                route1=0.0.0.0/0,0.0.0.1
+
+                """
+            ),
+        }
+        with mock.patch("cloudinit.net.get_interfaces_by_mac"):
+            ns = self._parse_network_state_from_config(config)
+            target = str(tmpdir)
+            network_manager.Renderer().render_network_state(ns, target=target)
+            rendered_content = dir2dict(target)
+            assert_equal_dict(expected_config, rendered_content)

jcmoore3 added a commit to jcmoore3/cloud-init that referenced this pull request Jul 15, 2024
@jcmoore3 jcmoore3 force-pushed the fix-dns-for-disabled-interface branch from 4003b88 to aaf4f7b Compare July 15, 2024 13:29
If DNS information is added to a NetworkManager managed interface where
the given protocol family is disabled, NetworkManager will be unable to
activate the interface.
@jcmoore3 jcmoore3 force-pushed the fix-dns-for-disabled-interface branch from aaf4f7b to fab11f1 Compare July 15, 2024 13:37
@jcmoore3
Copy link
Contributor Author

@aciba90 I have added the tests suggested and they run/pass locally but seem to be having an issue when running as a part of the Github integration checks. I'm not able to immediately spot the issue, do you have any insight?

@aciba90
Copy link
Contributor

aciba90 commented Jul 15, 2024

Thanks, @jcmoore3. The issue is that your local branch does not contain #5495 . CI merges your branch with main and performs the tests. Could please rebase main and adapt the tests?

@jcmoore3 jcmoore3 requested a review from aciba90 July 17, 2024 19:05
Copy link
Contributor

@aciba90 aciba90 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@aciba90 aciba90 merged commit 311f723 into canonical:main Jul 18, 2024
23 checks passed
holmanb pushed a commit to holmanb/cloud-init that referenced this pull request Aug 2, 2024
If DNS information is added to a NetworkManager managed interface where
the given protocol family is disabled, NetworkManager will be unable to
activate the interface.

canonical#5387
holmanb pushed a commit that referenced this pull request Aug 6, 2024
If DNS information is added to a NetworkManager managed interface where
the given protocol family is disabled, NetworkManager will be unable to
activate the interface.

#5387
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants