Correct user <-> group relationship on removal #9114
Conversation
This refactors the code using safe navigators and modern Ruby functions.
When a group needs to be removed, it can't be the primary group of a user. So if a user is ensured absent, it needs to happen before the group is removed.
|
Can one of the admins verify this patch? |
| } | ||
| end | ||
|
|
||
| if @parameters[:groups]&.should |
There was a problem hiding this comment.
what is .should doing? 🤔
There was a problem hiding this comment.
It's mostly based on what was already there. I'm just refactoring.
There was a problem hiding this comment.
The .should is the desired value specified in the catalog, which has been canonicalized by the property's munge method. Whereas .shouldorig is the desired value before munging:
Line 549 in 7ca202b
| end | ||
|
|
||
| if @parameters[:groups]&.should | ||
| autos += @parameters[:groups].should.split(",") |
There was a problem hiding this comment.
can multiple groups be passed as comma separated string?
There was a problem hiding this comment.
Good question, was just refactoring. Now I question why it was there. I did notice in the other example that shouldorig was an array while should wasn't. So perhaps that's the problem?
There was a problem hiding this comment.
Internally puppet stores the desired values as an array (both should and shouldorig) for example https://github.com/puppetlabs/puppet/blob/7ca202bd2faf876e1904a8a2e891c10c2cee6ffc/lib/puppet/property.rb#L552C1-L552C1
However, the should reader will either return a single value (default case) or an array depending on whether the property is single or multi-valued:
Lines 531 to 535 in 7ca202b
The behavior is determined based on the array_matching value specified when the property is defined using newproperty. For example, members of the group is multi-valued:
puppet/lib/puppet/type/group.rb
Line 84 in 7ca202b
|
|
||
| # Autobefore the primary group, if it's around. Otherwise group removal | ||
| # fails when that group is also ensured absent. | ||
| autobefore(:group) do |
There was a problem hiding this comment.
There's a long standing bug in puppet that auto* relationships and ensure => absent don't work well together. I filed a redmine ticket a long time ago, which was exported to https://puppet.atlassian.net/browse/PUP-2451. Ideally I think puppet would know to reverse the direction for any auto* relationship when ensuring absent. Worse, if you try to set an explicit relationship, it will create a cycle due to the auto relationship.
There was a problem hiding this comment.
I have written Puppet code where I had a lot of conditionals based on absent/present. I'd love for Puppet to be smarter here. But reading https://github.com/voxpupuli/puppet-systemd/blob/dbaa56f690e36f9f164fee464bf7b0e8a9a6d37c/manifests/unit_file.pp#L131-L142 I really don't know how it could be designed in a way that's not more confusing than the status quo.
Overall few people write modules with (full) support to ensure things are absent.
When a group needs to be removed, it can't be the primary group of a user. So if a user is ensured absent, it needs to happen before the group is removed.
This PR first adds tests to the existing code, before refactoring it. Then in the last commit it adds autobefore and makes sure autorequires is empty when ensure is absent.