DNS Resolver: Adding hosts should not delete what already exists #153

stratus-ss · 2025-01-22T17:53:49Z

Is your feature request related to a problem? Please describe.
Currently when you use this module to add host entries, all previous entries are deleted and replaced with only the entries in the calling playbook. This behaviour is not ideal. You should have the option to retain current entries.

Describe the solution you'd like
There should be a clear delineated option to either overwrite or append to the host list.

Additional context
I believe this can be accomplished by modifying the _params_to_obj() method in pfsense_dns_resolver.py and adding a helper function to determine what currently exists in the target pfsense host. The same can be done for domain overrides.

I have hacked around on this and my working prototype looks like this

     def _params_to_obj(self):
         params = self.params
 
-        obj = dict()
+        # Initialize with existing configuration
+        obj = {}
+        if self.root_elt is not None:
+            # Preserve existing hosts
+            existing_hosts = []
+            for host_elt in self.root_elt.findall("hosts"):
+                host_entry = {}
+                for child in host_elt:
+                    host_entry[child.tag] = child.text
+                existing_hosts.append(host_entry)
+
+            # Preserve existing domain overrides
+            existing_overrides = []
+            for override_elt in self.root_elt.findall("domainoverrides"):
+                override_entry = {}
+                for child in override_elt:
+                    override_entry[child.tag] = child.text
+                existing_overrides.append(override_entry)
+
+            obj["hosts"] = existing_hosts
+            obj["domainoverrides"] = existing_overrides
-            self._get_ansible_param(obj, "hosts")
-            self._get_ansible_param(obj, "domainoverrides")
+
+            # Append new hosts if provided
+            if params.get("hosts"):
+                if "hosts" not in obj:
+                    obj["hosts"] = []
+                for host in params["hosts"]:
+                    if host.get("aliases"):
+                        host["aliases"] = {"item": host["aliases"]}
+                    else:
+                        host["aliases"] = "\n\t\t\t"
+                obj["hosts"].extend(params["hosts"])
+
+            # Append new domain overrides if provided
+            if params.get("domainoverrides"):
+                if "domainoverrides" not in obj:
+                    obj["domainoverrides"] = []
+                obj["domainoverrides"].extend(params["domainoverrides"])
-            # wrap <item> to all hosts.alias
-            for host in obj["hosts"]:
-                if host["aliases"]:
-                    tmp_aliases = host["aliases"]
-                    host["aliases"] = {
-                        "item": tmp_aliases
-                    }
-                else:
-                    # Default is an empty element
-                    host["aliases"] = "\n\t\t\t"
-

+    def _merge_param(self, obj, params, param_name, transform=None):
+        """Helper method to merge parameters only if they are provided"""
+        if params.get(param_name) is not None:
+            value = params[param_name]
+            if transform:
+                value = transform(value)
+            obj[param_name] = value
+
+    def _get_current_config(self):
+        """Get the current configuration from the target"""
+        if self.root_elt is None:
+            return None
+
+        current = {}
+
+        # Extract existing configuration into a dict
+        for child in self.root_elt:
+            if child.tag == "hosts":
+                if "hosts" not in current:
+                    current["hosts"] = []
+                host_entry = {}
+                for host_child in child:
+                    host_entry[host_child.tag] = host_child.text
+                current["hosts"].append(host_entry)
+
+            elif child.tag == "domainoverrides":
+                if "domainoverrides" not in current:
+                    current["domainoverrides"] = []
+                override_entry = {}
+                for override_child in child:
+                    override_entry[override_child.tag] = override_child.text
+                current["domainoverrides"].append(override_entry)
+
+            else:
+                current[child.tag] = child.text
+
+        return current
+

I'm happy to work a PR through, but I am not sure if this meets any requirements the project has.
#worksOnMyMachine

I have a commit in my fork which does the following:

leaves the current host over rides intact
Updates a record if the host and the domain exist in the host override
leaves the custom options intact

The text was updated successfully, but these errors were encountered:

opoplawski · 2025-01-27T05:45:19Z

How do you remove hosts or domainoverrides then? I think we need an extra parameter to determine what the intent is - merge or strict definition.

opoplawski · 2025-01-27T05:45:44Z

A PR would be great to work through this.

stratus-ss · 2025-01-27T17:49:33Z

The libvirt community module uses "flags" for some options. That might be reasonable to implement here.

    - name: remove and undefine vm
      community.libvirt.virt:
        command: undefine
        name: "{{ vm }}"
        flags: managed_save,snapshots_metadata,delete_volumes

I'm not sure if this is a reasonable approach or too complex. It might be too complex or out of style for this project however.

Before I start working on a PR, I would love some input as to the way the author(s) think might be a reasonable approach. Should it be a completely different option? Should there be 2 options? What do you think

opoplawski · 2025-01-31T04:56:16Z

I think we'll go with the suggestion from here: https://forum.ansible.com/t/how-best-to-express-desired-merged-or-replaced-state-for-module-that-accepts-a-list-of-items/39895

Let's have purge_domainoverrides and purge_hosts which default to true. If false, then the items are added/updated only.

stratus-ss · 2025-01-31T13:19:43Z

Thanks, I'll work on this and get back to you

Tanchwa · 2025-03-04T15:32:52Z

Surely we can just use the state: exists or state: removed attributes?

stratus-ss · 2025-03-04T15:34:29Z

I haven't forgotten about this, i just had #life happen
:-)

Tanchwa · 2025-03-04T15:44:10Z

no worries... I've been trying to get into writing more python... do you have a working branch that I could take a crack at on your fork?

stratus-ss · 2025-03-04T15:46:09Z

my fork is working yes. I have just been using my fork. However, my fork doesn't have the option requested. I changed the default behavior

Tanchwa · 2025-03-04T15:48:11Z

haha sorry by working branch I mean feature branch

stratus-ss · 2025-03-04T15:55:03Z

https://github.com/stratus-ss/pfsensible/tree/preserve_unbound_hosts

opoplawski · 2025-03-15T03:16:10Z

@stratus-ss Can you rebase and file a PR?

stratus-ss · 2025-03-15T14:21:49Z

@opoplawski I can but I haven't had time to work in your requested changes. Work has been very intense

stratus-ss · 2025-03-15T15:40:54Z

i'll see about writing some unit tests for this as well

stratus-ss · 2025-03-16T20:20:09Z

so i started to pick through the unit tests but really wasn't clear how they should work. Is there any documentation on this so I don't have to essentially reverse-engineer testing? @opoplawski

opoplawski · 2025-03-18T02:51:03Z

I'm afraid there isn't any documentation at the moment. Hopefully looking at the tests for a simple module would be a good starting point. We generally have a separate initial simple xml config file for each - but it isn't even really necessary.

stratus-ss · 2025-03-22T20:30:22Z

I have updated my fork & branch, I added a new option similar to state called preserve

  preserve:
    description: Preserve the current DNS entries instead of overriding them.
    required: false
    default: false
    type: bool

It is confirmed working with the rebase. I will write some tests. They may not be in line with the current test style. We can adjust that after we have a test to base off of

stratus-ss linked a pull request Mar 24, 2025 that will close this issue

feat(pfsense_dns_resolver.py): Option to preserve unbound hosts #179

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DNS Resolver: Adding hosts should not delete what already exists #153

DNS Resolver: Adding hosts should not delete what already exists #153

DNS Resolver: Adding hosts should not delete what already exists #153

DNS Resolver: Adding hosts should not delete what already exists #153

Comments