Skip to content

Fix DNS in NetworkState (SC-133)#923

Merged
blackboxsw merged 2 commits into
canonical:mainfrom
TheRealFalcon:nameserver-interface
Jun 17, 2021
Merged

Fix DNS in NetworkState (SC-133)#923
blackboxsw merged 2 commits into
canonical:mainfrom
TheRealFalcon:nameserver-interface

Conversation

@TheRealFalcon
Copy link
Copy Markdown
Contributor

Proposed Commit Message

Fix DNS in NetworkState

v1 network config currently has no concept of interface-specific DNS,
which is required for certain renderers. To fix this, added an
optional 'interface' key on the v1 nameserver definition. If
specified, it makes the DNS settings specific to the interface.
Otherwise, it will be defined as global DNS as it always has.

Additionally, DNS for v2 wasn't being recognized correctly. For DNS
defined on a particular interface, these settings now also go into the
global DNS settings as they were intended.

Additional Context

These changes would help enable a systemd-networkd renderer.

Test Steps

Added new unit tests. This can't be integration tested yet as a renderer would need to pick up these changes.

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

@TheRealFalcon TheRealFalcon requested a review from blackboxsw June 16, 2021 21:59
@TheRealFalcon TheRealFalcon changed the title Fix DNS in NetworkState Fix DNS in NetworkState (SC-133) Jun 16, 2021
@blackboxsw blackboxsw self-assigned this Jun 17, 2021
Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

Thanks @TheRealFalcon minor rename needed for _handle_individual_nameserver I think but beyond that +1.

Maybe worth expanding the doc example to use the interface key?
diff --git a/doc/rtd/topics/network-config-format-v1.rst b/doc/rtd/topics/network-config-format-v1.rst
index d379b437..71d5f000 100644
--- a/doc/rtd/topics/network-config-format-v1.rst
+++ b/doc/rtd/topics/network-config-format-v1.rst
@@ -356,6 +356,7 @@ the following keys:
address:
- 192.168.23.2
- 8.8.8.8

  •    interface: interface0  # Ties nameserver to interface0 only
       search:
         - exemplary
    

Comment thread cloudinit/net/network_state.py Outdated
dns['search'].extend(search)

@ensure_command_keys(['address'])
def handle_individual_nameserver(self, command, iface):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We might want to rename this because of ugly magic of CommandHandlerMeta which will attempt to map any 'handle_' method of NetworkStateInterpreter as a viable "command"
Note we've done the same with _handle_bond_bridge to avoid "registering" and handler for the top-level network config key "bond_bridge"

Suggested change
def handle_individual_nameserver(self, command, iface):
def _handle_individual_nameserver(self, command, iface):

version: 1
config:
- type: nameserver
interface: {}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we specifically name this format variable or give it an index so it's more clear that we will probably be using this as a format string. Initial glance at the string made me think we were expecting a dict type in tests.

Suggested change
interface: {}
interface: {0} # or {int_name}

v1 network config currently has no concept of interface-specific DNS,
which is required for certain renderers. To fix this, added an
optional 'interface' key on the v1 nameserver definition. If
specified, it makes the DNS settings specific to the interface.
Otherwise, it will be defined as global DNS as it always has.

Additionally, DNS for v2 wasn't being recognized correctly. For DNS
defined on a particular interface, these settings now also go into the
global DNS settings as they were intended.
Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

LGTM!

@blackboxsw blackboxsw merged commit abd2da5 into canonical:main Jun 17, 2021
@TheRealFalcon TheRealFalcon deleted the nameserver-interface branch June 17, 2021 21:55
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