Repository navigation
Fix connection option handling for delegated driver - #2988
Merged
Merged
Conversation
The documentation and the code tends to suggest that in order to configure the ansible connection plugin, the option to use is 'connection' but the test handling this option is testing 'ansible_connection', so fix that. Signed-off-by: Arnaud Patard <apatard@hupstream.com>
ssbarnea
requested changes
Nov 22, 2020
Member
There was a problem hiding this comment.
This is exactly the opposite, we should always follow Ansible and adapt to its changes, so we avoid confusing users. Based on https://docs.ansible.com/ansible/latest/user_guide/intro_inventory.html we need to assure we use ansible_connection and not just connection.
OK to update the docs, ok even if it breaks some users config, we will mark it as major and explain it that we follow ansible.
Contributor
Author
|
hm. not sure to follow. The change is only for the case of managed instances and here, the options are already not following ansible. See https://github.com/ansible-community/molecule/blob/master/src/molecule/driver/delegated.py#L185. If the idea is to get ansible variables by adding 'ansible_' as prefix, then my change is fine (and the identity option has possibly to be renamed). If not, then all options has to be renamed. |
ssbarnea
approved these changes
Nov 25, 2020
9 tasks done
shvenkat-rh
pushed a commit
that referenced
this pull request
Sep 17, 2026
Fixes #4696. ## Problem On the default driver with Molecule managing the instances, every host with an instance-config entry gets `ansible_connection: smart` in the generated inventory unless the create playbook recorded a `connection`. If the create playbook recorded no `user` or `port`, the host also gets `ansible_user: null` and `ansible_port: null`. A host variable beats the same variable set at group or all level, so a user's own `ansible_connection`, `ansible_user` or `ansible_port` in the scenario's inventory is silently discarded. Scenario, with a create playbook that records only `instance` and `address`: ```yaml # molecule/default/molecule.yml driver: name: default platforms: - name: probe groups: - probes provisioner: name: ansible inventory: group_vars: probes: ansible_connection: local ansible_user: groupuser ansible_port: 2222 ``` On `main` (`20b71bda`), the host entry Molecule generates and the effective host vars: ```yaml # <ephemeral directory>/inventory/ansible_inventory.yml probe: &id001 ansible_connection: smart ansible_host: 127.0.0.1 ansible_port: null ansible_ssh_common_args: -o UserKnownHostsFile=/dev/null -o ControlMaster=auto -o ControlPersist=60s -o ForwardX11=no -o LogLevel=ERROR -o IdentitiesOnly=yes -o StrictHostKeyChecking=no ansible_user: null ``` ```json // ansible-inventory --host probe "ansible_connection 9B78 ": "smart", "ansible_port": null, "ansible_user": null, ``` ## What changes - Molecule writes `ansible_connection`, `ansible_user`, `ansible_host` and `ansible_port` only when the create playbook recorded the matching value. No `smart` fallback, no `null`. - A scenario that sets no transport anywhere connects as before on every supported core. - A scenario that set its own value now gets it. The same scenario on this branch (`fb42ac7d`). The `ansible_connection: smart`, `ansible_port: null` and `ansible_user: null` lines are gone from the host entry, and the group values apply: ```yaml # <ephemeral directory>/inventory/ansible_inventory.yml probe: &id001 ansible_host: 127.0.0.1 ansible_ssh_common_args: -o UserKnownHostsFile=/dev/null -o ControlMaster=auto -o ControlPersist=60s -o ForwardX11=no -o LogLevel=ERROR -o IdentitiesOnly=yes -o StrictHostKeyChecking=no ``` ```json // ansible-inventory --host probe "ansible_connection": "local", "ansible_port": 2222, "ansible_user": "groupuser", ``` The reporter's scenario records no `connection`, so it now gets no `ansible_connection` and connects over `ssh` without the `connection: ssh` workaround. `driver.options.ansible_connection_options` still applies only with `managed: False`, and `provisioner.connection_options` is the override for the generated host vars. Both are unchanged. `ansible_ssh_common_args` is still written for every managed host. ## Risk / Impact A managed scenario that set `ansible_connection`, `ansible_user` or `ansible_port` in its own inventory and depended on Molecule's host-level `smart` or `null` winning now gets its own value. ## Root cause `Delegated.ansible_connection_options()` in `src/molecule/driver/delegated.py` builds the managed host vars from `instance_config.yml`. The `instance_params` table's last entry is `("connection", "smart")` under a truthy guard, so the fallback is emitted on every host unless the instance config carries its own `connection`. Below the table, `ansible_user`, `ansible_host` and `ansible_port` are assigned unconditionally from `d.get(...)`, so an unrecorded key lands as `None` and is dumped as `null`. The `smart` fallback is a regression. #1674 (`0e07804b`, 2019) introduced it. #2811 (`4efdb48a`, 2020-09) and #2988 (`11d09c81`, 2020-11) put it behind a guard, so `ansible_connection` was written only when recorded. #3932 (`5b750ff4`, 2023-06) folded the keys into the `instance_params` table and reintroduced an unconditional write, as `null`. #3957 (`99a95f87`, 2023-07) made it `smart` because `ansible_connection: null` crashed `meta: reset_connection`. This PR leaves the key absent rather than `null`, so that crash does not return. The `null` writes for user, host and port also date from #1674, so dropping them is new behavior. Ansible's own default transport has been `ssh` since ansible-core 2.16, and `smart` resolved to `ssh` on any controller with a ControlPersist-capable OpenSSH, so removing the host var does not change the plugin for a scenario that sets nothing. The fix changes the table entry to `("connection", None)` and drops `None` values from the result in one comprehension at the return. ## Changes - The default driver writes the four host vars only when the create playbook recorded a value for them. - `docs/configuration.md`, one paragraph after the instance-config API block: the four host vars are written only when recorded, where a managed scenario sets its own values, and that `driver.options.ansible_connection_options` supplies host vars only with `managed: False`. ## Tests `tests/unit/driver/test_delegated.py`: - `test_ansible_connection_options_when_managed`, ssh case: the `"ansible_connection": "smart"` expectation is removed and `assert "ansible_connection" not in ssh_result` is added. The winrm case, which records `connection: winrm`, is unchanged and still expects it. - New `test_ansible_connection_options_omits_unrecorded_keys_when_managed`: the instance config records `instance`, `address` and `user: null`. Asserts `ansible_host` is present, the other three are absent, and no value is `None`, which is the shape #3957 fixed. ## Test plan - [x] `tox -e lint` passes - [x] `tox -e pkg` passes - [x] `tox -e py` passes, 910 passed, 7 skipped, same skips as `main` - [x] Coverage unchanged, 90% total, `delegated.py` 93% - [x] New test fails on `main` (2 failed, 44 passed), passes on this branch (46 passed) - [x] Scenario above run on `main` and this branch, `ansible_inventory.yml` and `ansible-inventory --host` captured, outputs are the blocks in this body - [x] Scenario recording `address`, `user`, `port` and no `connection`, only the `ansible_connection: smart` line differs - [x] Scenario with no `instance_config.yml`, host entry `{}` on both sides - [x] Docs updated <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Delegated driver inventory no longer forces the `smart` connection type when no connection is recorded. - Connection settings from inventory, configuration files, environment variables, and other supported options now apply correctly. - Unrecorded connection values no longer override existing host or inventory settings. - SSH common arguments continue to be applied for both managed and unmanaged instances. - **Documentation** - Updated delegated driver documentation to clarify connection and inventory behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Jeff Pullen <9343691+jeffcpullen@users.noreply.github.com> Co-authored-by: Jeff Pullen <9343691+jeffcpullen@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The documentation and the code tends to suggest that in order to configure the ansible connection plugin, the option to use is 'connection' but the test handling this option is testing 'ansible_connection', so fix that.