Skip to content

fix(netconf_config.py): type conversion issue for confirm_timeout #689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jclaerhout
Copy link

SUMMARY

This pull request fixes a type conversion issue in the netconf_config module of the ansible.netcommon collection.

When using the confirm parameter, Ansible throws an error due to a type mismatch:

Argument must be bytes or unicode, got 'int'

This occurs because confirm_timeout, which is expected to be an integer, is not properly converted before being passed to the NETCONF commit operation.

The fix ensures confirm_timeout is explicitly converted to a string using to_text() with errors='surrogate_or_strict' before passing it to the commit operation.

Fixes #551


ISSUE TYPE
  • Bugfix Pull Request

COMPONENT NAME

ansible.netcommon.netconf_config


ADDITIONAL INFORMATION

The issue occurs when attempting to use confirm in ansible.netcommon.netconf_config, leading to a failure due to incorrect type handling.

Code Change (Line 669 of netconf_config.py):

- confirmed=confirmed_commit, timeout=confirm_timeout
+ confirmed=confirmed_commit, timeout=to_text(confirm_timeout, errors='surrogate_or_strict')

This fix ensures proper type handling and allows users to correctly perform confirmed commits with rollback timers.

References:

Let me know if any further modifications are required! 🚀

Voici un message de commit clair et concis pour ton pull request :  

Ensure confirm_timeout is correctly converted to a string using `to_text()` to prevent type mismatches.  
This resolves errors like `Argument must be bytes or unicode, got 'int'` when using commit confirmation.  

Change applied in line 669:  
- `confirmed=confirmed_commit, timeout=confirm_timeout`  
+ `confirmed=confirmed_commit, timeout=to_text(confirm_timeout, errors='surrogate_or_strict')`
@Dalle55
Copy link

Dalle55 commented May 6, 2025

Hi,
We've been testing it in a dedicated virtualenv on Cisco IOS XE devices, and would like to report a few important issues that surfaced — along with a working patch and reproduction steps.

Problems observed with the current PR implementation

1. to_text() called with unsupported keyword error

This line is problematic:

confirm = to_text(module.params["confirm"], error="surrogate_or_strict")
  • to_text() from ansible.module_utils._text does not support the error keyword.
  • It causes immediate runtime failure:
TypeError: to_text() got an unexpected keyword argument 'error'

Fix used in our local patch:

confirm = to_text(module.params["confirm"])

2. Confirm value not properly cast

If confirm is passed as an integer from the playbook (as in confirm: 10), Ansible will fail with:

Argument must be bytes or unicode, got 'int'

Fix applied:

We explicitly convert confirm to string before using it in any NETCONF RPC or XML operation:

confirm = str(to_text(module.params["confirm"]))

3. Confirmed commit not functioning properly on Cisco IOS XE

  • Even after fixing type handling, the module logic assumes commit and confirm can be bundled into a single RPC.
  • However, Cisco IOS XE does not support this behavior reliably.
  • Sending <commit confirmed="true" timeout="10"/> does not work. Cisco expects a full <commit><confirmed/><confirm-timeout>10</confirm-timeout></commit> RPC sent separately.

Temporary workaround:

We split the commit logic into two steps using netconf_rpc manually after pushing config via netconf_config.

@Dalle55
Copy link

Dalle55 commented May 6, 2025

Correction regarding commit confirmed in Ansible's netconf_config module

RFC 6241, Section 8.4 (Confirmed Commit)

According to RFC 6241, section 8.4, the correct format for a confirmed commit operation in NETCONF is to use child elements, not attributes. Example:

<commit>
  <confirmed/>
  <confirm-timeout>10</confirm-timeout>
</commit>

However, the netconf_config module currently generates the following structure:

<commit confirmed="true" timeout="10"/>

This is not compliant with RFC 6241 and leads to errors on compliant NETCONF servers.

Real-World Observation

Cisco IOS XE correctly implements the confirmed commit functionality using the expected structure from the RFC. It rejects the attribute-based form because it is not valid NETCONF syntax.

This means:

  • Cisco is acting correctly.
  • The Ansible module is generating invalid XML and must be fixed.

Recommendation

Update the module to generate proper RFC-compliant commit RPCs using child elements for confirmed and confirm-timeout.

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.

netconf_config false argument when using confirm_commit
2 participants