fix: splunk_upgrade always evaluates true (list/string version comparison) - #931
Open
tod-uma wants to merge 1 commit into
Open
fix: splunk_upgrade always evaluates true (list/string version comparison)#931tod-uma wants to merge 1 commit into
tod-uma wants to merge 1 commit into
Conversation
`get_facts.yml`'s "Set current version fact" task builds `splunk_current_version`/`splunk_current_build_hash` with `regex_search(regexp, '\1')`. Ansible's `regex_search` filter always returns a list when a group reference is passed (see `plugins/filter/core.py`), so these facts are single-element lists, e.g. `['9.4.8']`. Commit 16fd108 ("change splunk_target_version to string") added `| first` to unwrap `splunk_target_version` for its `install_splunk.yml` consumer, but did not update the "Set current version fact" task to match. The result: `splunk_target_version != splunk_current_version` compares a string to a list and is always True, so `splunk_upgrade` evaluates true on every run against an already-installed host (as long as `build_location` is set) -- regardless of whether the installed version actually differs from the target. This forces an unconditional splunkd stop/start (and, when `splunk.allow_upgrade` is set, a full reinstall-check cycle) on every single converge. Reproduced directly against ansible-core's regex_search filter: before the fix, `'9.4.8' != ['9.4.8']` is True; after unwrapping with `| first`, it's False, as expected when the versions actually match. Same commit also left a stale `| first` on `splunk_target_version` at the "Determine if Splunk has preinstall checks" task, which pre-dated the source-level unwrap and expected `splunk_target_version` to still be a list. Post-16fd108, applying `| first` to the now-scalar string takes its first character ("9" instead of "9.4.8"), which fails the `is version('9.4.0', '>=')` check for every 9.x/10.x target. Removed the now-redundant `| first` there too.
3 tasks
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.
Summary
get_facts.yml's "Set current version fact" task buildssplunk_current_version/splunk_current_build_hashwithregex_search(regexp, '\1'). Ansible'sregex_searchfilter always returns a list when a group reference is passed (seeplugins/filter/core.py), so these facts end up as single-element lists, e.g.['9.4.8'].Commit 16fd108 ("change splunk_target_version to string") added
| firstto unwrapsplunk_target_versionfor itsinstall_splunk.ymlconsumer, but didn't update the "Set current version fact" task to match. The result:splunk_target_version != splunk_current_versioncompares a string to a list and is alwaysTrue, sosplunk_upgradeevaluatestrueon every run against an already-installed host (as long asbuild_locationis set) — regardless of whether the installed version actually differs from the target.Impact: an unconditional splunkd stop/start (and, when
splunk.allow_upgradeis set, a full reinstall-check cycle) on every single converge against an already-installed host — not just on real upgrades.Reproduced directly against ansible-core's
regex_searchfilter, independent of any host: before the fix,'9.4.8' != ['9.4.8']isTrue; after unwrapping with| first, it'sFalse, as expected when the versions actually match.The same commit also left a stale
| firstonsplunk_target_versionat "Determine if Splunk has preinstall checks" — that task pre-dates the source-level unwrap and expectedsplunk_target_versionto still be a list. Post-16fd108, applying| firstto the now-scalar string takes its first character ("9" instead of "9.4.8"), which fails theis version('9.4.0', '>=')check for every 9.x/10.x target. Removed the now-redundant| firstthere too.Test plan
regex_searchfilter directly (list vs. scalar), confirmed the fix resolves itansible-lintclean of new issues (only pre-existing stylistic findings shared by the rest of the file)splunk_upgradenow correctly evaluatesfalseand the previously-unconditional splunkd restart no longer occurs, run after runreconcile_config_map_values's unconditional remove+reapply, filed separately) that the two are unrelated — this fix alone brings a steady-state converge from 4 changed tasks down to 2 (the remaining 2 are the separate bug)