Fix configuration of DNS servers via OpenStack #5384

jcmoore3 · 2024-06-06T19:48:22Z

Ensure DNS server addresses are parsed from the proper location of network_data.json

Proposed Commit Message

fix: Ensure proper configuration of DNS servers via OpenStack json network config

Ensure DNS server addresses are parsed from the proper location
of network_data.json and that the services entry is not propogated

Additional Context

Test Steps

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

github-actions · 2024-06-25T00:21:51Z

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

jcmoore3 · 2024-06-25T12:49:56Z

Apologies that the CLA is taking so long. This needs to go through my company's internal open source approval process. I do not anticipate any issues, it just takes time. I'm hoping to have final internal approval this week.

jcmoore3 · 2024-06-27T16:00:08Z

CLA signed, please let me know if anything else is required to get this moving. Change to github-cla-signers is in PR #5369.

aciba90

Thanks, @jcmoore3, for fixing this.

The following patch contains your fixes + unittest + resolutions of the inline comments (please discuss them if you think I am wrong) after a rebase of main:

commit 4e9e760b5d0a1e965fe756f19dcf3dfb618264c7
Author: Curt Moore <[email protected]>
Date:   Wed Jun 5 18:13:32 2024 -0500

    Fix configuration of DNS servers via OpenStack
    
    Ensure DNS server addresses are parsed from the proper location
    of network_data.json
    
    Fixes GH-5386
    
    Co-authored-by: Alberto Contreras <[email protected]>

diff --git a/cloudinit/sources/helpers/openstack.py b/cloudinit/sources/helpers/openstack.py
index 70998dda2..977b13c0d 100644
--- a/cloudinit/sources/helpers/openstack.py
+++ b/cloudinit/sources/helpers/openstack.py
@@ -578,7 +578,13 @@ def convert_net_json(network_json=None, known_macs=None):
             "scope",
             "dns_nameservers",
             "dns_search",
-            "routes",
+        ],
+        "routes": [
+            "network",
+            "destination",
+            "netmask",
+            "gateway",
+            "metric"
         ],
     }
 
@@ -620,6 +626,21 @@ def convert_net_json(network_json=None, known_macs=None):
                 (k, v) for k, v in network.items() if k in valid_keys["subnet"]
             )
 
+            # Filter the route entries as they may contain extra elements such
+            # as DNS which are required elsewhere by the cloudinit schema
+            if network.get("routes"):
+                subnet.update(
+                    {
+                        "routes": [
+                            dict(
+                                (k, v) for k, v in route.items()
+                                if k in valid_keys["routes"]
+                            )
+                            for route in network["routes"]
+                        ]
+                    }
+                )
+
             if network["type"] == "ipv4_dhcp":
                 subnet.update({"type": "dhcp4"})
             elif network["type"] == "ipv6_dhcp":
@@ -650,7 +671,13 @@ def convert_net_json(network_json=None, known_macs=None):
                 service["address"]
                 for service in network.get("services", [])
                 if service.get("type") == "dns"
+            ] + [
+                service["address"]
+                for route in network.get("routes", [])
+                for service in route.get("services", [])
+                if service.get("type") == "dns"
             ]
+
             if dns_nameservers:
                 subnet["dns_nameservers"] = dns_nameservers
 
diff --git a/tests/unittests/sources/helpers/test_openstack.py b/tests/unittests/sources/helpers/test_openstack.py
index 7ae164140..6ec0bd75b 100644
--- a/tests/unittests/sources/helpers/test_openstack.py
+++ b/tests/unittests/sources/helpers/test_openstack.py
@@ -231,3 +231,129 @@ class TestConvertNetJson:
         assert expected == openstack.convert_net_json(
             network_json=network_json, known_macs=macs
         )
+
+    def test_dns_servers(self):
+        """
+        Verify additional properties under subnet.routes are not rendered
+        """
+        network_json = {
+            "links": [
+                {
+                    "id": "ens1f0np0",
+                    "name": "ens1f0np0",
+                    "type": "phy",
+                    "ethernet_mac_address": "xx:xx:xx:xx:xx:00",
+                    "mtu": 9000,
+                },
+                {
+                    "id": "ens1f1np1",
+                    "name": "ens1f1np1",
+                    "type": "phy",
+                    "ethernet_mac_address": "xx:xx:xx:xx:xx:01",
+                    "mtu": 9000,
+                },
+                {
+                    "id": "bond0",
+                    "name": "bond0",
+                    "type": "bond",
+                    "bond_links": ["ens1f0np0", "ens1f1np1"],
+                    "mtu": 9000,
+                    "ethernet_mac_address": "xx:xx:xx:xx:xx:00",
+                    "bond_mode": "802.3ad",
+                    "bond_xmit_hash_policy": "layer3+4",
+                    "bond_miimon": 100,
+                },
+                {
+                    "id": "bond0.123",
+                    "name": "bond0.123",
+                    "type": "vlan",
+                    "vlan_link": "bond0",
+                    "vlan_id": 123,
+                    "vlan_mac_address": "xx:xx:xx:xx:xx:00",
+                },
+            ],
+            "networks": [
+                {
+                    "id": "publicnet-ipv4",
+                    "type": "ipv4",
+                    "link": "bond0.123",
+                    "ip_address": "x.x.x.x",
+                    "netmask": "255.255.255.0",
+                    "routes": [
+                        {
+                            "network": "0.0.0.0",
+                            "netmask": "0.0.0.0",
+                            "gateway": "x.x.x.1",
+                            "services": [
+                                {"type": "dns", "address": "1.1.1.1"},
+                                {"type": "dns", "address": "8.8.8.8"},
+                            ],
+                        }
+                    ],
+                    "network_id": "00000000-0000-0000-0000-000000000000",
+                }
+            ],
+            "services": [],
+        }
+        expected = {
+            "version": 1,
+            "config": [
+                {
+                    "name": "ens1f0np0",
+                    "type": "physical",
+                    "mtu": 9000,
+                    "subnets": [],
+                    "mac_address": "xx:xx:xx:xx:xx:00",
+                },
+                {
+                    "name": "ens1f1np1",
+                    "type": "physical",
+                    "mtu": 9000,
+                    "subnets": [],
+                    "mac_address": "xx:xx:xx:xx:xx:01",
+                },
+                {
+                    "name": "bond0",
+                    "type": "bond",
+                    "mtu": 9000,
+                    "subnets": [],
+                    "mac_address": "xx:xx:xx:xx:xx:00",
+                    "params": {
+                        "bond-mode": "802.3ad",
+                        "bond-xmit_hash_policy": "layer3+4",
+                        "bond-miimon": 100,
+                    },
+                    "bond_interfaces": ["ens1f0np0", "ens1f1np1"],
+                },
+                {
+                    "name": "bond0.123",
+                    "type": "vlan",
+                    "subnets": [
+                        {
+                            "type": "static",
+                            "netmask": "255.255.255.0",
+                            "routes": [
+                                {
+                                    "network": "0.0.0.0",
+                                    "netmask": "0.0.0.0",
+                                    "gateway": "x.x.x.1",
+                                }
+                            ],
+                            "address": "x.x.x.x",
+                            "dns_nameservers": ["1.1.1.1", "8.8.8.8"],
+                            "ipv4": True,
+                        }
+                    ],
+                    "vlan_id": 123,
+                    "vlan_link": "bond0",
+                },
+            ],
+        }
+        macs = {
+            "xx:xx:xx:xx:xx:00": "ens1f0np0",
+            "xx:xx:xx:xx:xx:01": "ens1f1np1",
+        }
+        netcfg = openstack.convert_net_json(
+            network_json=network_json, known_macs=macs
+        )
+        assert expected == netcfg

cloudinit/sources/helpers/openstack.py

Ensure DNS server addresses are parsed from the proper location of network_data.json

aciba90 · 2024-07-10T11:29:01Z

I have pushed rebasing main and adding a unit test.

aciba90

Thanks, @jcmoore3, for addressing the previous comments. I have an inline proposal.

aciba90 · 2024-07-10T14:00:39Z

cloudinit/sources/helpers/openstack.py

-                for service in network.get("services", [])
-                if service.get("type") == "dns"
-            ]
+            # Look for either subnet or network specific DNS servers


As we are not going to deal with tons of dns nameservers, we can probably keep it simple using lists, maintaining the order in which the dns nameserves are defined in network_json by doing something in the lines of the following patch. WDYT?

diff --git a/cloudinit/sources/helpers/openstack.py b/cloudinit/sources/helpers/openstack.py index 558f024e4..9b46a22c3 100644 --- a/cloudinit/sources/helpers/openstack.py +++ b/cloudinit/sources/helpers/openstack.py @@ -661,27 +661,23 @@ def convert_net_json(network_json=None, known_macs=None): ) # Look for either subnet or network specific DNS servers - # and add them as subnet level DNS entries. Use a set to - # for accumulation to eliminate duplicates. + # and add them as subnet level DNS entries. # Subnet specific nameservers - dns_nameservers = set( - [ - service["address"] - for route in network.get("routes", []) - for service in route.get("services", []) - if service.get("type") == "dns" - ] - ) + dns_nameservers = [ + service["address"] + for route in network.get("routes", []) + for service in route.get("services", []) + if service.get("type") == "dns" + ] # Network specific nameservers - dns_nameservers.update( - [ - service["address"] - for service in network.get("services", []) - if service.get("type") == "dns" - ] - ) + for service in network.get("services", []): + if service.get("type") != "dns": + continue + if service["address"] in dns_nameservers: + continue + dns_nameservers.append(service["address"]) if dns_nameservers: - subnet["dns_nameservers"] = list(dns_nameservers) + subnet["dns_nameservers"] = dns_nameservers # Enable accept_ra for stateful and legacy ipv6_dhcp types if network["type"] in ["ipv6_dhcpv6-stateful", "ipv6_dhcp"]: diff --git a/tests/unittests/sources/helpers/test_openstack.py b/tests/unittests/sources/helpers/test_openstack.py index ec85cbeec..6ec0bd75b 100644 --- a/tests/unittests/sources/helpers/test_openstack.py +++ b/tests/unittests/sources/helpers/test_openstack.py @@ -340,7 +340,7 @@ class TestConvertNetJson: } ], "address": "x.x.x.x", - "dns_nameservers": ["8.8.8.8", "1.1.1.1"], + "dns_nameservers": ["1.1.1.1", "8.8.8.8"], "ipv4": True, } ],

Thanks for adding the test case, I had intended to do that but overlooked it when I was updating the other test cases.

I agree, simpler is better. I've pushed a new PR with the modifications you suggested.

aciba90

LGTM, thanks!

Ensure DNS server addresses are parsed from the proper location of network_data.json Fixes canonical#5386 Co-authored-by: Alberto Contreras <[email protected]>

Ensure DNS server addresses are parsed from the proper location of network_data.json Fixes #5386 Co-authored-by: Alberto Contreras <[email protected]>

holmanb added the CLA not signed The submitter of the PR has not (yet) signed the CLA label Jun 10, 2024

github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Jun 25, 2024

holmanb removed the stale-pr Pull request is stale; will be auto-closed soon label Jun 25, 2024

holmanb added CLA signed The submitter of the PR has signed the CLA and removed CLA not signed The submitter of the PR has not (yet) signed the CLA labels Jun 27, 2024

aciba90 requested changes Jun 28, 2024

View reviewed changes

cloudinit/sources/helpers/openstack.py Outdated Show resolved Hide resolved

cloudinit/sources/helpers/openstack.py Show resolved Hide resolved

aciba90 self-assigned this Jul 1, 2024

jcmoore3 force-pushed the fix-dns-config branch from 3448bcb to 9ef43a4 Compare July 9, 2024 21:22

jcmoore3 and others added 3 commits July 10, 2024 13:18

Fix configuration of DNS servers via OpenStack

bc27a09

Ensure DNS server addresses are parsed from the proper location of network_data.json

Update parsing logic

b27caab

add unittest

bef09f4

aciba90 force-pushed the fix-dns-config branch from 9ef43a4 to bef09f4 Compare July 10, 2024 11:28

aciba90 reviewed Jul 10, 2024

View reviewed changes

Simplify DNS server detection logic

a6c9709

aciba90 approved these changes Jul 11, 2024

View reviewed changes

aciba90 merged commit e0e6a42 into canonical:main Jul 11, 2024
23 checks passed

holmanb pushed a commit that referenced this pull request Aug 6, 2024

Fix configuration of DNS servers via OpenStack (#5384)

e4c6604

Ensure DNS server addresses are parsed from the proper location of network_data.json Fixes #5386 Co-authored-by: Alberto Contreras <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix configuration of DNS servers via OpenStack #5384

Fix configuration of DNS servers via OpenStack #5384

jcmoore3 commented Jun 6, 2024 •

edited

Loading

github-actions bot commented Jun 25, 2024

jcmoore3 commented Jun 25, 2024

jcmoore3 commented Jun 27, 2024

aciba90 left a comment

aciba90 commented Jul 10, 2024

aciba90 left a comment

aciba90 Jul 10, 2024

jcmoore3 Jul 10, 2024

aciba90 left a comment

Fix configuration of DNS servers via OpenStack #5384

Fix configuration of DNS servers via OpenStack #5384

Conversation

jcmoore3 commented Jun 6, 2024 • edited Loading

Proposed Commit Message

Additional Context

Test Steps

Checklist

Merge type

github-actions bot commented Jun 25, 2024

jcmoore3 commented Jun 25, 2024

jcmoore3 commented Jun 27, 2024

aciba90 left a comment

Choose a reason for hiding this comment

aciba90 commented Jul 10, 2024

aciba90 left a comment

Choose a reason for hiding this comment

aciba90 Jul 10, 2024

Choose a reason for hiding this comment

jcmoore3 Jul 10, 2024

Choose a reason for hiding this comment

aciba90 left a comment

Choose a reason for hiding this comment

jcmoore3 commented Jun 6, 2024 •

edited

Loading