Repository: ansible/ansible
Pull request: Unfrack extra vars
Since now it is possible to re-reference them mid play, we cannot rely on the initial load having the correct relative path.

fixes #75126 

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below -->
args parsing

Problem statement:
Ansible fails to load extra variable file on inventory reload
### Summary

Under specific circumstances, when the playbook executes inventory reload, Ansible fails to load variables from the extras file.

### Issue Type

Bug Report

### Component Name

inventory

### Ansible Version

```console
$ ansible --version
ansible [core 2.11.2] 
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/home/tadej/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible
  ansible collection location = /home/tadej/.ansible/collections:/usr/share/ansible/collections
  executable location = /home/tadej/tmp/111/venv/bin/ansible
  python version = 3.9.5 (default, May 14 2021, 00:00:00) [GCC 10.3.1 20210422 (Red Hat 10.3.1-1)]
  jinja version = 3.0.1
  libyaml = True
```


### Configuration

```console
$ ansible-config dump --only-changed
```


### OS / Environment

Fedora 34

### Steps to Reproduce

This is the script that will set up a minimal environment that still demonstrates the issue:

```bash
cd /tmp
mkdir -p 1/2/3

cat << EOF > 1/vars.yaml
---
my: var
EOF

cat << EOF > 1/2/inventory.yaml
---
plugin: constructed
groups:
  webservers: inventory_hostname.startswith('web')
EOF

cat << EOF > 1/2/3/playbook.yaml
---
- hosts: localhost
  gather_facts: false
  tasks:
    - name: Make sure var is loaded
      debug:
        var: my

    - name: Reload inventory
      meta: refresh_inventory
EOF

cd 1/2
ansible-playbook -vvv -e @../vars.yaml -i inventory.yaml 3/playbook.yaml
```

The relevant warning in the output is this:

```
[WARNING]:  * Failed to parse /tmp/1/2/inventory.yaml with auto plugin: Unable to retrieve file contents
  Could not find or access '/tmp/1/2/vars.yaml' on the Ansible Controller. If you are using a module and
  expect the file to exist on the remote, see the remote_src option
```

Ansible made an error here and somehow constructed a bad path `tmp/1/2/vars.yaml` instead of `tmp/1/vars.yaml`. It gets the path right the first time vars are loaded, but fails if we trigger inventory reload from the playbook. Do note that the bad behavior is not triggered if the playbook is not nested inside a subfolder, which indicates that somewhere path to vars file is interpreted relatively to the playbook's path.

### Expected Results

Ansible should reload the inventory.

### Actual Results

```console
ansible-playbook [core 2.11.2]     
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/home/tadej/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible
  ansible collection location = /home/tadej/.ansible/collections:/usr/share/ansible/collections
  executable location = /home/tadej/tmp/111/venv/bin/ansible-playbook
  python version = 3.9.5 (default, May 14 2021, 00:00:00) [GCC 10.3.1 20210422 (Red Hat 10.3.1-1)]
  jinja version = 3.0.1                   
  libyaml = True                          
Using /etc/ansible/ansible.cfg as config file
host_list declined parsing /tmp/1/2/inventory.yaml as it did not pass its verify_file() method
script declined parsing /tmp/1/2/inventory.yaml as it did not pass its verify_file() method
Parsed /tmp/1/2/inventory.yaml inventory source with auto plugin
[WARNING]: provided hosts list is empty, only localhost is available. Note that the implicit localhost does not match 'all'
Skipping callback 'default', as we already have a stdout callback.
Skipping callback 'minimal', as we already have a stdout callback.
Skipping callback 'oneline', as we already have a stdout callback.

PLAYBOOK: playbook.yaml *******************************************************************************************************************************************************************************************************************************************************************************************************
1 plays in 3/playbook.yaml

PLAY [localhost] **************************************************************************************************************************************************************************************************************************************************************************************************************
META: ran handlers

TASK [Make sure var is loaded] ************************************************************************************************************************************************************************************************************************************************************************************************
task path: /tmp/1/2/3/playbook.yaml:5
ok: [localhost] => {
    "my": "var"
}

TASK [Reload inventory] *******************************************************************************************************************************************************************************************************************************************************************************************************
task path: /tmp/1/2/3/playbook.yaml:9
host_list declined parsing /tmp/1/2/inventory.yaml as it did not pass its verify_file() method
script declined parsing /tmp/1/2/inventory.yaml as it did not pass its verify_file() method
toml declined parsing /tmp/1/2/inventory.yaml as it did not pass its verify_file() method
[WARNING]:  * Failed to parse /tmp/1/2/inventory.yaml with auto plugin: Unable to retrieve file contents Could not find or access '/tmp/1/2/vars.yaml' on the Ansible Controller. If you are using a module and expect the file to exist on the remote, see the remote_src option
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/inventory/manager.py", line 290, in parse_source
    plugin.parse(self._inventory, self._loader, source, cache=cache)
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/plugins/inventory/auto.py", line 58, in parse
    plugin.parse(inventory, loader, path, cache=cache)
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/plugins/inventory/constructed.py", line 140, in parse
    super(InventoryModule, self).parse(inventory, loader, path, cache=cache)
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/plugins/inventory/__init__.py", line 186, in parse
    self._vars = load_extra_vars(loader)
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/utils/vars.py", line 195, in load_extra_vars
    data = loader.load_from_file(extra_vars_opt[1:])
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/parsing/dataloader.py", line 94, in load_from_file
    (b_file_data, show_content) = self._get_file_contents(file_name)
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/parsing/dataloader.py", line 162, in _get_file_contents
    raise AnsibleFileNotFound("Unable to retrieve file contents", file_name=file_name)
[WARNING]:  * Failed to parse /tmp/1/2/inventory.yaml with yaml plugin: Unable to retrieve file contents Could not find or access '/tmp/1/2/vars.yaml' on the Ansible Controller. If you are using a module and expect the file to exist on the remote, see the remote_src option
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/inventory/manager.py", line 290, in parse_source
    plugin.parse(self._inventory, self._loader, source, cache=cache)
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/plugins/inventory/yaml.py", line 99, in parse
    super(InventoryModule, self).parse(inventory, loader, path)
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/plugins/inventory/__init__.py", line 186, in parse
    self._vars = load_extra_vars(loader)
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/utils/vars.py", line 195, in load_extra_vars
    data = loader.load_from_file(extra_vars_opt[1:])
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/parsing/dataloader.py", line 94, in load_from_file
    (b_file_data, show_content) = self._get_file_contents(file_name)
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/parsing/dataloader.py", line 162, in _get_file_contents
    raise AnsibleFileNotFound("Unable to retrieve file contents", file_name=file_name)
[WARNING]:  * Failed to parse /tmp/1/2/inventory.yaml with ini plugin: Unable to retrieve file contents Could not find or access '/tmp/1/2/vars.yaml' on the Ansible Controller. If you are using a module and expect the file to exist on the remote, see the remote_src option
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/inventory/manager.py", line 290, in parse_source
    plugin.parse(self._inventory, self._loader, source, cache=cache)
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/plugins/inventory/ini.py", line 104, in parse
    super(InventoryModule, self).parse(inventory, loader, path)
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/plugins/inventory/__init__.py", line 186, in parse
    self._vars = load_extra_vars(loader)
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/utils/vars.py", line 195, in load_extra_vars
    data = loader.load_from_file(extra_vars_opt[1:])
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/parsing/dataloader.py", line 94, in load_from_file
    (b_file_data, show_content) = self._get_file_contents(file_name)
  File "/home/tadej/tmp/111/venv/lib64/python3.9/site-packages/ansible/parsing/dataloader.py", line 162, in _get_file_contents
    raise AnsibleFileNotFound("Unable to retrieve file contents", file_name=file_name)
[WARNING]: Unable to parse /tmp/1/2/inventory.yaml as an inventory source
[WARNING]: No inventory was parsed, only implicit localhost is available
META: inventory successfully refreshed
META: ran handlers
META: ran handlers

PLAY RECAP ********************************************************************************************************************************************************************************************************************************************************************************************************************
localhost                  : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0
```


### Code of Conduct

- [X] I agree to follow the Ansible Code of Conduct

Hints from issue discussion:
/cc @samdoran As promised, I prepared a sample that replicates the behavior I noticed in one of our integration tests. https://github.com/ansible-collections/servicenow.itsm/blob/48b521fd7e83a22994def9b010b674150c99e8a8/tests/integration/targets/inventory/runme.sh#L39-L43 is the part that started failing with Ansible Core while this worked with Ansible Base and 2.9.
```patch
diff --git a/lib/ansible/cli/arguments/option_helpers.py b/lib/ansible/cli/arguments/option_helpers.py
index 0447d8238a..fed63bf11a 100644
--- a/lib/ansible/cli/arguments/option_helpers.py
+++ b/lib/ansible/cli/arguments/option_helpers.py
@@ -100,6 +100,16 @@ def unfrack_path(pathsep=False):
     return inner


+def maybe_unfrack_path(beacon='@'):
+
+    def inner(value):
+        if value.startswith(beacon)
+            return beacon + unfrackpath(value[1:])
+        else:
+            return value
+    return inner
+
+
 def _git_repo_info(repo_path):
     """ returns a string containing git branch, commit id and commit date """
     result = None
@@ -340,7 +350,7 @@ def add_runas_prompt_options(parser, runas_group=None):

 def add_runtask_options(parser):
     """Add options for commands that run a task"""
-    parser.add_argument('-e', '--extra-vars', dest="extra_vars", action="append",
+    parser.add_argument('-e', '--extra-vars', dest="extra_vars", action="append", type=maybe_unfrack_path(),
                         help="set additional variables as key=value or YAML/JSON, if filename prepend with @", default=[])
```
The issue is a relative path to a file passed in as `extra_vars` gets mangled by inventory. So we'll need to resolve that path correctly before passing it to inventory for parsing (as shown above).
I can confirm that patch posted by @bcoca does indeed resolve the issue for us.
~Sounds good. I'll work on a PR~
Confirmed this was broken in f8aa30765963cbcfdce67c86079bbf1261100a66 (h/t @sivel).
I was curious why this only failed on the _second_ attempt to parse variables. On the first invocation, `self._basedir` of the dataloader object  is `'.'`, so we end up passing `'./../vars.yml'` to `unfrackpath()` which resolves it correctly to `/issues/75126/1/vars.yml` (current working directory is `/issues/75126/1/2/`).

Then, `self._basedir` is set to the playbook basedir:

```
  File "/Users/sdoran/Source/ansible/lib/ansible/executor/playbook_executor.py", line 130, in run
    self._loader.set_basedir(pb._basedir)
```

When the inventory is refreshed, `self._basedir` on the dataloader is now the playbook directory. We then pass `/issues/75126/1/2/3/../vars.yml` to `unfrackpath()` which resolves to `/issues/75126/1/2/vars.yml`. 💥

Commit message: added missing

Patch to review:
diff --git a/changelogs/fragments/extra_vars_unfrack.yml b/changelogs/fragments/extra_vars_unfrack.yml
new file mode 100644
index 00000000000000..5700e1fff7740b
--- /dev/null
+++ b/changelogs/fragments/extra_vars_unfrack.yml
@@ -0,0 +1,2 @@
+bugfixes:
+  - Ensure we get full path for extra vars into cliargs to avoid realpath issues after initial load.
diff --git a/lib/ansible/cli/arguments/option_helpers.py b/lib/ansible/cli/arguments/option_helpers.py
index 0447d8238a50b9..0c50f1f27d3cdd 100644
--- a/lib/ansible/cli/arguments/option_helpers.py
+++ b/lib/ansible/cli/arguments/option_helpers.py
@@ -100,6 +100,16 @@ def inner(value):
     return inner
 
 
+def maybe_unfrack_path(beacon):
+
+    def inner(value):
+        if value.startswith(beacon):
+            return beacon + unfrackpath(value[1:])
+        else:
+            return value
+    return inner
+
+
 def _git_repo_info(repo_path):
     """ returns a string containing git branch, commit id and commit date """
     result = None
@@ -340,7 +350,7 @@ def add_runas_prompt_options(parser, runas_group=None):
 
 def add_runtask_options(parser):
     """Add options for commands that run a task"""
-    parser.add_argument('-e', '--extra-vars', dest="extra_vars", action="append",
+    parser.add_argument('-e', '--extra-vars', dest="extra_vars", action="append", type=maybe_unfrack_path('@'),
                         help="set additional variables as key=value or YAML/JSON, if filename prepend with @", default=[])
 
 
diff --git a/test/integration/targets/inventory/1/2/3/extra_vars_relative.yml b/test/integration/targets/inventory/1/2/3/extra_vars_relative.yml
new file mode 100644
index 00000000000000..98c2634059f6a1
--- /dev/null
+++ b/test/integration/targets/inventory/1/2/3/extra_vars_relative.yml
@@ -0,0 +1,11 @@
+- hosts: localhost
+  gather_facts: false
+  tasks:
+    - name: Make sure var is loaded
+      assert:
+        that:
+          - my is defined
+          - my == 'var'
+
+    - name: Reload inventory, forces extra vars re-eval from diff basedir
+      meta: refresh_inventory
diff --git a/test/integration/targets/inventory/1/2/inventory.yml b/test/integration/targets/inventory/1/2/inventory.yml
new file mode 100644
index 00000000000000..5082cef2f2e6c3
--- /dev/null
+++ b/test/integration/targets/inventory/1/2/inventory.yml
@@ -0,0 +1,3 @@
+plugin: constructed
+groups:
+  webservers: inventory_hostname.startswith('web')
diff --git a/test/integration/targets/inventory/1/vars.yml b/test/integration/targets/inventory/1/vars.yml
new file mode 100644
index 00000000000000..c11458438c4ab2
--- /dev/null
+++ b/test/integration/targets/inventory/1/vars.yml
@@ -0,0 +1 @@
+my: var
diff --git a/test/integration/targets/inventory/runme.sh b/test/integration/targets/inventory/runme.sh
index 7ce16cf54efec8..c5fe322643179f 100755
--- a/test/integration/targets/inventory/runme.sh
+++ b/test/integration/targets/inventory/runme.sh
@@ -88,3 +88,9 @@ fi
 
 # ensure we don't traceback on inventory due to variables with int as key
 ansible-inventory  -i inv_with_int.yml --list "$@"
+
+# test in subshell relative paths work mid play for extra vars in inventory refresh
+{
+	cd 1/2
+	ansible-playbook -e @../vars.yml -i inventory.yml 3/extra_vars_relative.yml "$@"
+}


Code review comments:
diff_hunk: @@ -88,3 +88,9 @@ fi
 
 # ensure we don't traceback on inventory due to variables with int as key
 ansible-inventory  -i inv_with_int.yml --list "$@"
+
+# test in subshell relative paths work mid play for extra vars in inventory refresh
+{
+	cd 1/2
+	ansible-playbook -e @../vars.yml -i inventory.yml 3/extra_vars_relative.yml "$@"
path: test/integration/targets/inventory/runme.sh
text: @user1:
If I understand things correctly, this command will not fail even if the fix is not present because failing to parse an inventory file is not fatal. Edits to the playbook itself I suggested above should rectify this, but we will need a small adjustment here:

```suggestion
	ansible-playbook -e @../vars.yml -i base_inventory.ini -i inventory.yml 3/extra_vars_relative.yml "$@"
```