Fixes ipahost idempotence issues

[rjeffman]

Fixes: #1296

Summary by Sourcery

Improve idempotence in the ipahost module by normalizing input values uniformly and refactoring host entry processing.

Bug Fixes:

• Fix idempotence issues by normalizing host parameters (case, FQDN, SSH keys) before comparison and ignoring unsupported updatedns settings

Enhancements:

• Refactor ipahost module to use EntryFactory for streamlined parameter conversion and entry iteration
• Add conversion helpers (lowercase, uppercase, unique FQDN, SSH public key normalization) in ansible_freeipa_module
• Update compare_args_ipa to include key context in debug messages and ignore updatedns during modifications
• Refactor ipasudorule to use unique FQDN conversion for host entries

Tests:

• Add integration tests to verify idempotence of mac_address casing and repeated host definitions in ipahost

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

• The new single_host flag logic (single_host=len(entry_factory)>1) no longer matches the original hosts is None semantics—please verify it still produces the intended batching behavior when processing multiple entries.
• There’s still duplicated logic for splitting FQDN into host and domain in the DNS record sections—extracting that into a helper would reduce code repetition and minimize edge‐case bugs.
• Even with the EntryFactory, the main function is extremely large; consider breaking out host command generation for each state/action into helper functions to improve readability and maintainability.

Prompt for AI Agents

Please address the comments from this code review:
## Overall Comments
- The new single_host flag logic (`single_host=len(entry_factory)>1`) no longer matches the original `hosts is None` semantics—please verify it still produces the intended batching behavior when processing multiple entries.
- There’s still duplicated logic for splitting FQDN into host and domain in the DNS record sections—extracting that into a helper would reduce code repetition and minimize edge‐case bugs.
- Even with the EntryFactory, the main function is extremely large; consider breaking out host command generation for each state/action into helper functions to improve readability and maintainability.

## Individual Comments

### Comment 1
<location> `plugins/modules/ipahost.py:658` </location>
<code_context>
                 result["result"]["randompassword"]


+def convert_sshpubkey(value):
+    """Ensure sshpubkey values are correct."""
+    if isinstance(value, list):
+        value = [str(normalize_sshpubkey(key)) for key in value]
+    if isinstance(value, (str, unicode)):  # pylint: disable=E0606
+        value = str(normalize_sshpubkey(value))
+    return value
+
+
</code_context>

<issue_to_address>
convert_sshpubkey may not handle nested lists or None values robustly.

Please update convert_sshpubkey to handle None inputs and nested lists to prevent runtime errors.
</issue_to_address>

### Comment 2
<location> `plugins/module_utils/ansible_freeipa_module.py:540` </location>
<code_context>
     return value


+def convert_param_value_to_uppercase(value):
+    if isinstance(value, list):
+        value = [v.upper() for v in value]
+    if isinstance(value, (str, unicode)):
+        value = value.upper()
+    return value
+
+
</code_context>

<issue_to_address>
convert_param_value_to_uppercase does not handle None values.

Explicitly handle None to ensure consistent behavior and prevent unintended results in downstream code.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[rjeffman]

Dowstream tests are passing with these changes.

[varunmylaraiah]

PR LGTM.
I’ve tested it against the existing upstream and downstream test suites, and all tests are passing.

[t-woerner]

I ignore_list_check is enabled, enabling allow_empty_list_item does not have any impact.
There is no description for ignore_list_check and the use case etc. Also not in the ipahost change later on, where it is used.

[t-woerner]

As invalid is now a variable that is used later on in the code, it should have a better name to make sure that it will not be used for something else.

[DorBreger]

Any news on this?

[DorBreger]

If I can be of any help on this PR, please let me know. I would really love to see this merged

[rjeffman]

@DorBreger this change will cause a huge impact on the way plugins are created, and we are postponing it for a few months. It is close to our radar, but we can't really do it right now as we have more pressure on other fronts.

And there are some "not optimal" design decisions I made on this PR that I feel need to be addressed before it could be merged.

A more realistic time frame (but still a crude estimate) is "second half of 2026". I'll certainly not touch it before May.

[DorBreger]

@DorBreger this change will cause a huge impact on the way plugins are created, and we are postponing it for a few months. It is close to our radar, but we can't really do it right now as we have more pressure on other fronts.

And there are some "not optimal" design decisions I made on this PR that I feel need to be addressed before it could be merged.

A more realistic time frame (but still a crude estimate) is "second half of 2026". I'll certainly not touch it before May.

I understand that this is a big change. We are facing an issue with ipa_dnsrecord not being idempotent, failing if the record already exists. Would you recommend submitting a fix to community.general for now?

[rjeffman]

We don't have any influence in the community.general project. We follow different approaches to managing IPA deployments (specially in the communication with the servers).

Note that ansible-freeipa plugins do not have underscore (_) on their names (this is the easiest way to differentiate them).