Skip to content

Conversation

@rjeffman
Copy link
Member

@rjeffman rjeffman commented Aug 26, 2025

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

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

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 ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@rjeffman rjeffman force-pushed the host_dns_record_idempotence branch 5 times, most recently from 1ccd618 to 35ea5d7 Compare August 27, 2025 00:28
@rjeffman rjeffman requested a review from t-woerner August 27, 2025 00:30
@rjeffman rjeffman force-pushed the host_dns_record_idempotence branch 5 times, most recently from 0b4a6a0 to b246dff Compare August 27, 2025 20:42
@rjeffman rjeffman added the Candidate Good candidate for next minor release. label Aug 29, 2025
@rjeffman
Copy link
Member Author

rjeffman commented Sep 4, 2025

Dowstream tests are passing with these changes.

When comparing an argument dict to an IPA dict, the log messages were
inconsistent and did not provide enough information for debugging.

This change make the messages consistent and display the dict key being
evaluated if the key result in tagging the dicts as different.

Signed-off-by: Rafael Guterres Jeffman <[email protected]>
This patch moves the function used to ensure that hostnames used in
ipasurule are FQDN and unique to module_utils, so it can be used by
other modules than require the same behavior for a hostaname parameter.

Signed-off-by: Rafael Guterres Jeffman <[email protected]>
When using ipahost with the 'hosts' parameters several parameter
conversion routines were not being performed causing the module to try
to execute tasks it was not suposed to.

This issues were fixed by adapting the module to use EntryFactory.

Another issue was happening when 'update_dns: true' is used with some
parameters, like 'description', 'location' or 'mac_address', where, even
if no change was to be performed the plugin would evaluate that the
arguments were different, and tried to modify the object.

The fix for this issue is to ignore 'updatedns' when comparing the
parameters provided with the existing IPA object.

This modification also fix the case where duplicate entries used with
the 'name' parameter in case 'state: absent' would fail without
indicating the use of duplicate entries.

Fixes: freeipa#1296
Fixes: https://issues.redhat.com/browse/RHEL-111063

Signed-off-by: Rafael Guterres Jeffman <[email protected]>
@rjeffman rjeffman force-pushed the host_dns_record_idempotence branch from b246dff to e7819b1 Compare November 11, 2025 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Candidate Good candidate for next minor release.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ipa_dnsrecord no modifications to be performed when record already exists.

1 participant