Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct user <-> group relationship on removal #9114

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Correct user <-> group relationship on removal #9114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
50d0d5e
Add User autorequire tests
ekohl Sep 29, 2023
d77e622
Shorten user autorequire code using safe navigators
ekohl Sep 29, 2023
65f9ee6
Correct user <-> group relationship on removal
ekohl Sep 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 43 additions & 28 deletions lib/puppet/type/user.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,15 @@ module Puppet
provided in the `gid` attribute) or any group listed in the `groups`
attribute then the user resource will autorequire that group. If Puppet
is managing any role accounts corresponding to the user's roles, the
user resource will autorequire those role accounts."
user resource will autorequire those role accounts.
Only when the resource is ensured to be present.

**Autobefores:** If Puppet is managing the user's primary group (as
provided in the `gid` attribute) or any group listed in the `groups`
attribute then the user resource will autobefore that group. If Puppet
is managing any role accounts corresponding to the user's roles, the
user resource will autorequire those role accounts.
Only when the resource is ensured to be absent."

feature :allows_duplicates,
"The provider supports duplicate users with the same UID."
Expand Down Expand Up @@ -459,40 +467,47 @@ def insync?(current)
end
end

# Autorequire the group, if it's around
# Autorequire groups, if they're around
autorequire(:group) do
autos = []

# autorequire primary group, if managed
obj = @parameters[:gid]
groups = obj.shouldorig if obj
if groups
groups = groups.collect { |group|
if group.is_a?(String) && group =~/^\d+$/
Integer(group)
else
group
end
}
groups.each { |group|
case group
when Integer
resource = catalog.resources.find { |r| r.is_a?(Puppet::Type.type(:group)) && r.should(:gid) == group }
if resource
autos << resource
if self[:ensure] != :absent
if @parameters[:gid]&.shouldorig
groups_in_catalog = catalog.resources.filter { |r| r.is_a?(Puppet::Type.type(:group)) }
autos += @parameters[:gid].shouldorig.filter_map do |group|
if (group.is_a?(String) && group.match?(/^\d+$/)) || group.is_a?(Integer)
gid = Integer(group)
groups_in_catalog.find { |r| r.should(:gid) == gid }
else
group
end
else
autos << group
end
}
end

if @parameters[:groups]&.should
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is .should doing? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's mostly based on what was already there. I'm just refactoring.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

puppet/lib/puppet/property.rb

Line 549 in 7ca202b

@shouldorig = values

autos += @parameters[:groups].should.split(",")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can multiple groups be passed as comma separated string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

puppet/lib/puppet/property.rb

Lines 531 to 535 in 7ca202b

if match_all?
return @should.collect { |val| self.unmunge(val) }
else
return self.unmunge(@should[0])
end

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

newproperty(:members, :array_matching => :all, :required_features => :manages_members) do

end
end

# autorequire groups, excluding primary group, if managed
obj = @parameters[:groups]
if obj
groups = obj.should
if groups
autos += groups.split(",")
autos
end

# Autobefore the primary group, if it's around. Otherwise group removal
# fails when that group is also ensured absent.
autobefore(:group) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

autos = []

if self[:ensure] == :absent
if @parameters[:gid]&.shouldorig
groups_in_catalog = catalog.resources.filter { |r| r.is_a?(Puppet::Type.type(:group)) }
autos += @parameters[:gid].shouldorig.filter_map do |group|
if (group.is_a?(String) && group.match?(/^\d+$/)) || group.is_a?(Integer)
gid = Integer(group)
groups_in_catalog.find { |r| r.should(:gid) == gid }
else
group
end
end
end
end

Expand Down
111 changes: 111 additions & 0 deletions spec/unit/type/user_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,117 @@ def self.instances; []; end
resource.parameter(:gid).sync
end
end

context 'automatic relations' do
let(:testgroup) { Puppet::Type.type(:group).new(:name => 'foo', :gid => 50) }

before do
Puppet::Resource::Catalog.new :testing do |conf|
conf.add_resource(testuser)
conf.add_resource(testgroup)
end
end

context 'when ensure => present' do
context 'when gid specified as :absent' do
let(:testuser) { described_class.new(:name => "testuser", :gid => :absent) }

it "has no autorequire" do
expect(testuser.autorequire).to be_empty
end

it "has no autobefore" do
expect(testuser.autobefore).to be_empty
end
end

context 'when gid specified as number-looking string' do
let(:testuser) { described_class.new(:name => "testuser", :gid => '50') }

it "autorequires the group" do
expect(testuser.autorequire).to match_array([an_object_having_attributes(source: testgroup, target: testuser)])
end

it "has no autobefore" do
expect(testuser.autobefore).to be_empty
end
end

context 'when gid specified as integer' do
let(:testuser) { described_class.new(:name => "testuser", :gid => 50) }

it "autorequires the group" do
expect(testuser.autorequire).to match_array([an_object_having_attributes(source: testgroup, target: testuser)])
end

it "has no autobefore" do
expect(testuser.autobefore).to be_empty
end
end

context 'when gid specified as name' do
let(:testuser) { described_class.new(:name => "testuser", :gid => 'foo') }

it "autorequires the group" do
expect(testuser.autorequire).to match_array([an_object_having_attributes(source: testgroup, target: testuser)])
end

it "has no autobefore" do
expect(testuser.autobefore).to be_empty
end
end
end

context 'when ensure => absent' do
context 'when gid specified as :absent' do
let(:testuser) { described_class.new(:name => "testuser", :ensure => :absent, :gid => :absent) }

it "has no autorequire" do
expect(testuser.autorequire).to be_empty
end

it "has no autobefore" do
expect(testuser.autobefore).to be_empty
end
end

context 'when gid specified as number-looking string' do
let(:testuser) { described_class.new(:name => "testuser", :ensure => :absent, :gid => '50') }

it "has no autorequire" do
expect(testuser.autorequire).to be_empty
end

it "autobefores the group" do
expect(testuser.autobefore).to match_array([an_object_having_attributes(source: testuser, target: testgroup)])
end
end

context 'when gid specified as integer' do
let(:testuser) { described_class.new(:name => "testuser", :ensure => :absent, :gid => 50) }

it "has no autorequire" do
expect(testuser.autorequire).to be_empty
end

it "autobefores the group" do
expect(testuser.autobefore).to match_array([an_object_having_attributes(source: testuser, target: testgroup)])
end
end

context 'when gid specified as name' do
let(:testuser) { described_class.new(:name => "testuser", :ensure => :absent, :gid => 'foo') }

it "has no autorequire" do
expect(testuser.autorequire).to be_empty
end

it "autobefores the group" do
expect(testuser.autobefore).to match_array([an_object_having_attributes(source: testuser, target: testgroup)])
end
end
end
end
end

describe "when managing groups" do
Expand Down
Loading