-
Notifications
You must be signed in to change notification settings - Fork 582
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.
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
Addition of stdlib::getpwnam to return /etc/passwd
entry.
#1419
Changes from all commits
3d833e7
241a191
5e8f4b9
bf9a9af
a890db1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
# frozen_string_literal: true | ||
|
||
# @summary | ||
# Given a username returns the user's entry from the `/etc/passwd` file. | ||
# | ||
# >* Note:* The `stdlib::getpwnam` function will work only on platforms that support | ||
# `getpwnam`. Typically that is on POSIX like OSes and not Windows. | ||
# | ||
Puppet::Functions.create_function(:'stdlib::getpwnam') do | ||
# @param user | ||
# The username to fetch password record for. | ||
# | ||
# @example Get a password entry for user steve as a hash. | ||
# $passwd_entry = stdlib::getpwnam('steve') | ||
# | ||
# @example Get the UID of user steve | ||
# $uid = stdlib::getpwnam('steve')["uid"] | ||
# | ||
# @raise ArgumentError if no password entry can be found for the specified user. | ||
# | ||
# @return | ||
# [Hash] For example `{"name"=>"root", "passwd"=>"x", "uid"=>0, "gid"=>0, "gecos"=>"root", "dir"=>"/root", "shell"=>"/bin/bash"}` | ||
dispatch :getpwnam do | ||
param 'String', :user | ||
return_type 'Hash' | ||
end | ||
|
||
def getpwnam(user) | ||
Etc.getpwnam(user).to_h.first(7).map { |k, v| { k.to_s => v } }.reduce(:merge) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,109 @@ | ||||||
# frozen_string_literal: true | ||||||
|
||||||
require 'spec_helper_acceptance' | ||||||
|
||||||
describe 'function stdlib::getpwnam' do | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better make the whole test conditional?
Suggested change
|
||||||
context 'finding the UID of the root user' do | ||||||
let(:pp) do | ||||||
<<-MANIFEST | ||||||
$_password_entry = stdlib::getpwnam('root') | ||||||
file{ '/tmp/roots_uid.txt': | ||||||
ensure => file, | ||||||
content => "${_password_entry['uid']}\n", | ||||||
} | ||||||
|
||||||
# Test the use case for systemd module | ||||||
user{ 'testu': | ||||||
ensure => present, | ||||||
uid => 250, | ||||||
} | ||||||
MANIFEST | ||||||
end | ||||||
|
||||||
it 'works idempotently with no errors' do | ||||||
apply_manifest(pp, catch_failures: true) unless os[:family] == 'windows' | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of repeating the condition ( |
||||||
apply_manifest(pp, catch_changes: true) unless os[:family] == 'windows' | ||||||
end | ||||||
|
||||||
describe file('/tmp/roots_uid.txt') do | ||||||
it { is_expected.to contain '0' } unless os[:family] == 'windows' | ||||||
end | ||||||
end | ||||||
|
||||||
context 'put the uid into environment of an exec ' do | ||||||
let(:pp) do | ||||||
<<-MANIFEST | ||||||
exec{ 'env_to_file': | ||||||
user => 'testu', | ||||||
environment => Deferred( | ||||||
'inline_epp', | ||||||
[ | ||||||
'MY_XDG_RUNTIME_DIR=/run/user/<%= $pwval["uid"] %>', | ||||||
{ 'pwval' => Deferred('stdlib::getpwnam',['testu'])} | ||||||
] | ||||||
), | ||||||
command => 'echo $MY_XDG_RUNTIME_DIR > /tmp/test-xdg-dir', | ||||||
path => $facts['path'], | ||||||
creates => '/tmp/test-xdg-dir', | ||||||
} | ||||||
MANIFEST | ||||||
end | ||||||
|
||||||
it 'works idempotently with no errors' do | ||||||
apply_manifest(pp, catch_failures: true) unless os[:family] == 'windows' | ||||||
apply_manifest(pp, catch_changes: true) unless os[:family] == 'windows' | ||||||
end | ||||||
|
||||||
describe file('/tmp/test-xdg-dir') do | ||||||
it { is_expected.to contain '/run/user/250' } unless os[:family] == 'windows' | ||||||
it { is_expected.to be_owned_by 'testu' } unless os[:family] == 'windows' | ||||||
end | ||||||
end | ||||||
|
||||||
# This puppet fails on Puppet7 | ||||||
# It fails with " Error: can't find user for testx" | ||||||
# This is | ||||||
# https://puppet.atlassian.net/browse/PUP-11526 | ||||||
# Code below should be fine but the fact function is not defined | ||||||
# which I don't understand so I can't mark as pending on Puppet7. | ||||||
# context 'put the uid into environment of an exec' do | ||||||
# let(:pp) do | ||||||
# <<-MANIFEST | ||||||
# user{ 'testx': | ||||||
# ensure => present, | ||||||
# uid => 251, | ||||||
# } | ||||||
# exec{ 'env_to_file': | ||||||
# user => 'testx', | ||||||
# environment => Deferred( | ||||||
# 'inline_epp', | ||||||
# [ | ||||||
# 'MY_XDG_RUNTIME_DIR=/run/user/<%= $pwval["uid"] %>', | ||||||
# { 'pwval' => Deferred('stdlib::getpwnam',['testx'])} | ||||||
# ] | ||||||
# ), | ||||||
# command => 'echo $MY_XDG_RUNTIME_DIR > /tmp/testx-xdg-dir', | ||||||
# path => $facts['path'], | ||||||
# creates => '/tmp/testx-xdg-dir', | ||||||
# require => User['testx'], | ||||||
# } | ||||||
# MANIFEST | ||||||
# end | ||||||
# | ||||||
# it 'works idempotently with no errors' do | ||||||
# pending('puppet7 having https://puppet.atlassian.net/browse/PUP-11526') if fact('puppetversion').to_f < 8 && (os[:family] != 'windows') | ||||||
# apply_manifest(pp, catch_failures: true) unless os[:family] == 'windows' | ||||||
# apply_manifest(pp, catch_changes: true) unless os[:family] == 'windows' | ||||||
# end | ||||||
# | ||||||
# describe file('/tmp/testx-xdg-dir') do | ||||||
# it { | ||||||
# pending('puppet7 having https://puppet.atlassian.net/browse/PUP-11526') if fact('puppetversion').to_f < 8 && (os[:family] != 'windows') | ||||||
# is_expected.to contain '/run/user/251' unless os[:family] == 'windows' | ||||||
# } | ||||||
# it { | ||||||
# pending('puppet7 having https://puppet.atlassian.net/browse/PUP-11526') if fact('puppetversion').to_f < 8 && (os[:family] != 'windows') | ||||||
# is_expected.to be_owned_by 'testx' unless os[:family] == 'windows' | ||||||
# } | ||||||
# end | ||||||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'spec_helper' | ||
|
||
describe 'stdlib::getpwnam' do | ||
it { is_expected.not_to be_nil } | ||
it { is_expected.to run.with_params.and_raise_error(ArgumentError, %r{expects 1 argument, got none}i) } | ||
|
||
it { | ||
passwd_entry = Etc::Passwd.new( | ||
'steve', | ||
'x', | ||
1000, | ||
1001, | ||
'Steve', | ||
'/home/steve', | ||
'/bin/fish', | ||
) | ||
allow(Etc).to receive(:getpwnam).with(anything, anything).and_call_original | ||
allow(Etc).to receive(:getpwnam).with('steve').and_return(passwd_entry) | ||
is_expected.to run.with_params('steve').and_return( | ||
{ | ||
'name' => 'steve', | ||
'passwd' => 'x', | ||
'uid' => 1000, | ||
'gid' => 1001, | ||
'gecos' => 'Steve', | ||
'dir' => '/home/steve', | ||
'shell' => '/bin/fish' | ||
}, | ||
) | ||
Comment on lines
+10
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if mocking is the best strategy. I would only have a unit test (and skip it on windows) that get info for a user we are sure will exist (either we get the name of the current user or we look for |
||
} | ||
|
||
it { is_expected.to run.with_params('notexisting').and_raise_error(ArgumentError, %r{can't find user for notexisting}i) } | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user isn't found this will probably end up being
nil.to_h
. Also, why do you even need to do the map? Isn't this sufficient?I'd expect Puppet to translate the symbols to strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With just
Etc.getpwnam(user).to_h
you get a Tuple rather than a Hash back - changing the return to Tupleshows this:
which does not seem much good.
For the no such user it fails in Etc.getpwnam itself.
which seemed reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is odd, perhaps Ruby version dependent?
Sounds fair to me. Is there a
@raises
tag in puppet-strings to document this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the
@raises
With
Etc.getpwnam(user).to_h
:I get
for both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, I checked on a bunch of versions of ruby and
Etc.getpwnam('romain').to_h
return a Hash for all of them (2.5, 2.7, 3.1 and 3.2). What you get in your last message seems to be the output ofto_a
applied on a Hash 🤔.Also why only keep the 7 first entries? (also
#first
on a Hash return an Array 😉) All OS do not return the same information, so I think it make sense to keep it unchanged and allow the end-user to consume what they want from the hash?The suggested change by @ekohl seems to really be what I would expect, can you please double-check this and if you reproduce the issue show us the failing code so that we can better understand what is going on?