gem: fix soundness issue when uninstalling default gems on Ubuntu #10689
Conversation
|
cc @None @johanwiren |
ansibullbot
added
bug
ansibullbot
removed
the
needs_revision
There was a problem hiding this comment.
Some comments.
Additionally, I would point that the module has been silently ignoring those gems that cannot be uninstalled until now, so popping an error out of the sudden is a breaking change.
I can imagine that some users might not care whether the specific package was uninstalled or not, while others might want to do something about it. I am thinking this might be better with some flag to determine how the module should behave in that case. I can think of 3 scenarios:
- ignore entirely (current behaviour)
- generate warning (module result is success)
- generate error (module result is failure)
| community.general.gem: | ||
| name: json | ||
| state: absent | ||
| when: ansible_distribution == "Ubuntu" |
There was a problem hiding this comment.
This is good enough for now, but I would suppose this happens because that gem must be installed by default in a .deb package and there is some mechanism in place to prevent the removal. I would suppose other distros/OS'es will have something similar, maybe not by default but after the OS package is installed.
One specific gem in one specific OS feels a bit fragile to me. For the future it might be interesting to better understand this issue and how the OS blocks the removal and either:
- ensure blocked-removal gems are installed in different platforms, or
- disregard the OS entirely and make the test block a gem and then try to remove it
There was a problem hiding this comment.
My brain coming back to this: I think the main issue I have with this is that the condition for that situation is very likely NOT ansible_distribution == "Ubuntu", but rather that gem located the json gem installed in, say, /usr/lib/ruby/..../ and does not: a) have permissions or, b) allows removing from that location at all or, c) does not allow removing removing from that location without some kind of --force parameter
felixfontein
added
check-before-release
Co-authored-by: Alexei Znamensky <[email protected]>
Co-authored-by: Alexei Znamensky <[email protected]>
ansibullbot
added
the
needs_revision
ansibullbot
removed
the
needs_revision
| command_output = uninstall(module) | ||
| if command_output is not None and len(command_output) == 3 and exists(module): |
There was a problem hiding this comment.
uninstall() ends with return module.run_command(), so it should always return the tuple. However, in line 227 above, when in check mode, it returns None.
So, arguably, if command_output is not None then it MUST be a tuple of length 3, so no need to test that.
There was a problem hiding this comment.
If I remove the len check then the unit tests break, see here:
https://github.com/ansible-collections/community.general/actions/runs/17037384348/job/48292931828
(CI of commit d5575c5)
There was a problem hiding this comment.
It does, but why? Is this test running in check mode? It doesn't look like. So what is the value of command_output in line 338 for that testcase?
Logic says it should either be None or a tuple of 3. Either my reading of the code is wrong and there is more to it, or something else is broken.
Or, let' s put it this other way, we have 3 cases for command_output:
- None
- 3-tuple
- this X value we are not sure
What should the module do when the result of that command is X? Right now it falls in the common case of the else and declares changed=True and goes to the successful end. Given that it is not clear what is happening, that sounds like a loose-end.
There was a problem hiding this comment.
In the unit tests, run_module() is patched, but it doesn't return anything specific (probably some automatic mock object?). Simplify adjusting the tests so that patch_run_command() returns a mock that returns (0, '', '') when called should fix this.
| if command_output is not None and len(command_output) == 3 and exists(module): | ||
| rc, out, err = command_output | ||
| module.fail_json( | ||
| msg="Gem could not be removed", |
There was a problem hiding this comment.
I also reckon the wording could be a tad more explanatory. Imagine the user running this like they have done before, and now getting a message "Gem could not be removed". But... why? What should they do next? For us, reading this PR, we have some idea of what is going on, but they will not have a clue.
|
friendly ping @gdrosos are you still working on this? |
SUMMARY
Attempt to fix incorrect reporting when uninstalling default gems on Ubuntu.
Previously, the
gemmodule reportedchanged: trueeven though default gems (e.g.json) cannot be removed.On Ubuntu,
gem uninstallexits with code 0 but leaves the gem installed, misleading the module.With this patch, the module now re-checks after uninstall and fails if the gem is still present. I also added integration tests
Fixes #10451
ISSUE TYPE
COMPONENT NAME
gem