inventory plugins: make wrapping variables as unsafe smarter to avoid triggering an AWX bug

[felixfontein]

SUMMARY

Ref: #8212.

Closes #8212.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

inventory plugins

[ansibullbot]

cc @Ajpantuso @BongoEADGC6 @Charliekenney23 @InTheCloudDan @LBGarber @Thulium-Drake @abarbare @conloos @ddelnano @decentral1se @displague @feldsam @ilijamt @joshainglis @karmab @krauthosting @morph027 @opoplawski @remyleone @rmcintosh @shayrybak @shinuza
click here for bot help

[evgeni]

I was tempted to say "bleh, regex, use something in value", but a quick timeit says regex is actually faster:

import re
import timeit

binary_type = bytes
text_type = str

_RE_TEMPLATE_CHARS = re.compile(u'[{}]')
_RE_TEMPLATE_CHARS_BYTES = re.compile(b'[{}]')

def _make_unsafe(value):
    return value

def make_unsafe(value):
    if isinstance(value, binary_type):
        if _RE_TEMPLATE_CHARS_BYTES.search(value):
            value = _make_unsafe(value)
        return value
    elif isinstance(value, text_type):
        if _RE_TEMPLATE_CHARS.search(value):
            value = _make_unsafe(value)
        return value
    return value

def make_unsafe_in(value):
    if isinstance(value, binary_type):
        if any(check in value for check in [b'{{', b'}}']):
            value = _make_unsafe(value)
        return value
    if isinstance(value, text_type):
        if any(check in value for check in ['{{', '}}']):
            value = _make_unsafe(value)
        return value
    return value

print(timeit.timeit('make_unsafe("adsdfsdf{{sdfsdfsf}}aaaaa")', globals=globals()))
print(timeit.timeit('make_unsafe(b"adsdfsdf{{sdfsdfsf}}aaaaa")', globals=globals()))
print(timeit.timeit('make_unsafe_in("adsdfsdf{{sdfsdfsf}}aaaaa")', globals=globals()))
print(timeit.timeit('make_unsafe_in(b"adsdfsdf{{sdfsdfsf}}aaaaa")', globals=globals()))

% python3.12 /tmp/bench.py
0.27897425496485084
0.2632201029919088
0.6106972509878688
0.8229789500473998

of course, PyPy is a magnitude faster, but still regex wins:

% pypy3.9 /tmp/bench.py
0.026400349976029247
0.018924177973531187
0.05259729997487739
0.047663011006079614

[evgeni]

But once I switch from any(…) to an explicit '{{' in value or '}}' in value, things get faster (which, in retrospect, makes sense, if a or b short-cuts when a is true, while any([a, b]) does execute BOTH, and then takes any()):

% python3.12 /tmp/bench.py
0.29455614904873073
0.2644505600328557
0.10402327799238265
0.28959875099826604

% pypy3.9 /tmp/bench.py  
0.023167914012447
0.022700645960867405
0.0018618669710122049
0.0015594420256093144

[evgeni]

The above is btw with:

% rpm -q python3 pypy3.9
python3-3.12.2-2.fc39.x86_64
pypy3.9-7.3.15-2.3.9.fc39.x86_64

[evgeni]

I've been thinking about those character groups.
On the one hand, to be "real" Jinja, we need {{, {# or {% (and their counterparts), so the groups here also match on non-Jinja things. But then again, I wouldn't really care.
And we also do not really need to care for closing (the group above matches on either anyway).
So this could become

Suggested change

	_RE_TEMPLATE_CHARS = re.compile(u'[{}]')
	_RE_TEMPLATE_CHARS_BYTES = re.compile(b'[{}]')
	_RE_TEMPLATE_CHARS = re.compile(u'{')
	_RE_TEMPLATE_CHARS_BYTES = re.compile(b'{')

at IMHO no (significant) downside, and with the upside that the matching is faster:

% python3.12 /tmp/bench.py
0.19322030898183584
0.17506323801353574
0.09858699102187529
0.28751170000759885

[felixfontein]

Yeah, the idea was to avoid matching things that obviously aren't templates (and can't concatenated to become templates), like IP addresses or hostnames, so that ansible_host and friends are only marked as unsafe if they contain potentially problematic things.

Whether the code should also check for } or not is 🤷. I'll probably stick to the lazy option and not change it ;-) If it turns out to be too slow we can still reduce it to a simple '{' in value check.

[evgeni]

Overall, I think this is fine.

[patchback]

Backport to stable-6: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-6/7fd37ea247ba351b541d472cbedefc60fb98473f/pr-8225

Backported as #8244

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[patchback]

Backport to stable-7: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-7/7fd37ea247ba351b541d472cbedefc60fb98473f/pr-8225

Backported as #8245

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[felixfontein]

@evgeni thanks a lot for taking a look at this!

[patchback]

Backport to stable-8: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-8/7fd37ea247ba351b541d472cbedefc60fb98473f/pr-8225

Backported as #8246

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.