Repository: ansible/ansible
Pull request: Account for empty strings when splitting the host pattern
##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Filter out empty strings when returning the list of hosts from `split_host_pattern()`.
Fixes #61964
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->

##### 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 -->
`lib/ansible/inventory/manager.py`

Problem statement:
Ansible crashes when `--limit` expression includes superfluous comma
##### SUMMARY
Running ansible with `--limit a,` or `--limit a,,b` or `--limit ,` leads to (when `-vvv` is added)
```
ERROR! Unexpected Exception, this is probably a bug: string index out of range
the full traceback was:

Traceback (most recent call last):
  File ".../ansible/bin/ansible-playbook", line 123, in <module>
    exit_code = cli.run()
  File ".../ansible/lib/ansible/cli/playbook.py", line 116, in run
    CLI.get_host_list(inventory, context.CLIARGS['subset'])
  File ".../ansible/lib/ansible/cli/__init__.py", line 487, in get_host_list
    inventory.subset(subset)
  File ".../ansible/lib/ansible/inventory/manager.py", line 614, in subset
    if x[0] == "@":
IndexError: string index out of range
ERROR! Unexpected Exception, this is probably a bug: string index out of range
to see the full traceback, use -vvv
```

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
lib/ansible/inventory/manager.py

##### ANSIBLE VERSION
```
2.9.0
```
Also happens with `devel`, and potentially older ansible versions.

##### STEPS TO REPRODUCE
Run `ansible-playbook playbook.yml --limit a,` for an existing playbook `playbook.yml` (doesn't matter what it actually does, and also does not matter whether you use `a` or a real host name). Same result with `--limit a,,b` or similar expressions where `.split(',')` will lead to an empty string in the resulting list.

##### EXPECTED RESULTS
Ansible complaining about syntax, or simply ignoring the trailing comma / empty parts of the `.split(',')`.

##### ACTUAL RESULTS
```
ERROR! Unexpected Exception, this is probably a bug: string index out of range
to see the full traceback, use -vvv
```

Hints from issue discussion:
Potential fixes:

1. Change https://github.com/ansible/ansible/blob/devel/lib/ansible/inventory/manager.py#L133 to `return [p.strip() for p in patterns if p.strip()]`. Disadvantage: `--limit ,` is equivalent to no limit, which can be confusing. On the other hand, trailing or repeated commas would be simply ignored, which is kind of user-friendly (in some sense).

2. Raise proper exception when empty string is found (either in https://github.com/ansible/ansible/blob/devel/lib/ansible/inventory/manager.py#L133 or where `split_host_pattern` is used, like in https://github.com/ansible/ansible/blob/devel/lib/ansible/inventory/manager.py#L610). This would be more user-friendly (proper error message instead of crash).

cc @abellotti @cben @evertmulder @gtanzillo @yaacov @zgalor
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
!component =lib/ansible/inventory/manager.py

Commit message: Improve tests

- add more unit test cases
- add specific integration test with more cases

Patch to review:
diff --git a/changelogs/fragments/split-host-pattern-empty-strings.yaml b/changelogs/fragments/split-host-pattern-empty-strings.yaml
new file mode 100644
index 00000000000000..35af0f0e8bde31
--- /dev/null
+++ b/changelogs/fragments/split-host-pattern-empty-strings.yaml
@@ -0,0 +1,2 @@
+bugfixes:
+  - account for empty strings in when splitting the host pattern (https://github.com/ansible/ansible/issues/61964)
diff --git a/lib/ansible/inventory/manager.py b/lib/ansible/inventory/manager.py
index 14e61ba9334aa1..b5e19208661ea5 100644
--- a/lib/ansible/inventory/manager.py
+++ b/lib/ansible/inventory/manager.py
@@ -110,6 +110,7 @@ def split_host_pattern(pattern):
     # comma-separated list of patterns.
     if u',' in pattern:
         patterns = pattern.split(u',')
+        patterns = [p.strip() for p in patterns]
 
     # If it doesn't, it could still be a single pattern. This accounts for
     # non-separator uses of colons: IPv6 addresses and [x:y] host ranges.
@@ -129,8 +130,7 @@ def split_host_pattern(pattern):
                     )+              # occurring once or more
                 '''), pattern, re.X
             )
-
-    return [p.strip() for p in patterns]
+    return [p for p in patterns if p]
 
 
 class InventoryManager(object):
diff --git a/test/integration/targets/inventory_limit/aliases b/test/integration/targets/inventory_limit/aliases
new file mode 100644
index 00000000000000..3005e4b26d029a
--- /dev/null
+++ b/test/integration/targets/inventory_limit/aliases
@@ -0,0 +1 @@
+shippable/posix/group4
diff --git a/test/integration/targets/inventory_limit/hosts.yml b/test/integration/targets/inventory_limit/hosts.yml
new file mode 100644
index 00000000000000..2e1b1927303f67
--- /dev/null
+++ b/test/integration/targets/inventory_limit/hosts.yml
@@ -0,0 +1,5 @@
+all:
+  hosts:
+    host1:
+    host2:
+    host3:
diff --git a/test/integration/targets/inventory_limit/runme.sh b/test/integration/targets/inventory_limit/runme.sh
new file mode 100755
index 00000000000000..05a9b8796d56a7
--- /dev/null
+++ b/test/integration/targets/inventory_limit/runme.sh
@@ -0,0 +1,31 @@
+#!/usr/bin/env bash
+
+set -eux
+
+trap 'echo "Host pattern limit test failed"' ERR
+
+# https://github.com/ansible/ansible/issues/61964
+
+# These tests should return all hosts
+ansible -i hosts.yml all --limit ,, --list-hosts | grep -q 'hosts (3)'
+ansible -i hosts.yml ,, --list-hosts | grep -q 'hosts (3)'
+ansible -i hosts.yml , --list-hosts | grep -q 'hosts (3)'
+ansible -i hosts.yml all --limit , --list-hosts | grep -q 'hosts (3)'
+ansible -i hosts.yml all --limit '' --list-hosts | grep -q 'hosts (3)'
+
+
+# Only one host
+ansible -i hosts.yml all --limit ,,host1 --list-hosts | grep -q 'hosts (1)'
+ansible -i hosts.yml ,,host1 --list-hosts | grep -q 'hosts (1)'
+
+ansible -i hosts.yml all --limit host1,, --list-hosts | grep -q 'hosts (1)'
+ansible -i hosts.yml host1,, --list-hosts | grep -q 'hosts (1)'
+
+
+# Only two hosts
+ansible -i hosts.yml all --limit host1,,host3 --list-hosts | grep -q 'hosts (2)'
+ansible -i hosts.yml host1,,host3 --list-hosts | grep -q 'hosts (2)'
+
+ansible -i hosts.yml all --limit 'host1, ,    ,host3' --list-hosts | grep -q 'hosts (2)'
+ansible -i hosts.yml 'host1, ,    ,host3' --list-hosts | grep -q 'hosts (2)'
+
diff --git a/test/units/plugins/inventory/test_inventory.py b/test/units/plugins/inventory/test_inventory.py
index 48c3f9834a4e4d..66b5ec37874134 100644
--- a/test/units/plugins/inventory/test_inventory.py
+++ b/test/units/plugins/inventory/test_inventory.py
@@ -49,6 +49,10 @@ class TestInventory(unittest.TestCase):
         'a:b': ['a', 'b'],
         ' a : b ': ['a', 'b'],
         'foo:bar:baz[1:2]': ['foo', 'bar', 'baz[1:2]'],
+        'a,,b': ['a', 'b'],
+        'a,  ,b,,c, ,': ['a', 'b', 'c'],
+        ',': [],
+        '': [],
     }
 
     pattern_lists = [


Code review comments:
diff_hunk: @@ -129,8 +130,7 @@ def split_host_pattern(pattern):
                     )+              # occurring once or more
                 '''), pattern, re.X
             )
-
-    return [p.strip() for p in patterns]
+    return [p for p in patterns if p]
path: lib/ansible/inventory/manager.py
text: @user1:
patterns from the regex won't be stripped?

also this traverses the list x2, not sure what is more expensive, strip x2 or list x2

@author:
Doh.

I went back to doing the "double strip" with a comprehension. With 100,000 hosts a regular `for` loop performed the same as doing the comprehension with the "double strip" on both Python 2 and 3.

```
patterns = [' host{} '.format(n) for n in range(1,100000)]

# Python 3
In [29]: %timeit split_host_pattern_loop(patterns)
1.06 s ± 66.6 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

In [30]: %timeit split_host_pattern_ds(patterns)
1.01 s ± 17.2 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)


# Python 2
In [14]: %timeit split_host_pattern_loop(patterns)
1 loop, best of 3: 1.36 s per loop

In [18]: %timeit split_host_pattern_ds(patterns)
1 loop, best of 3: 1.37 s per loop
```

@user1:
you REAAAALLY don't want a `for ` loop ....