opuppet/module-concat
4
0
Fork 0
Code Issues Pull requests Releases Wiki Activity

remove default owner/user and group values

Browse source 
The use of $::id to set the default user/owner and group has caused
multiple bugs in the past, is incorrectly used to infer the egid,
introduces a dependency on the `id` fact, and provides no functionally
that can't be accomplished by passing `undef` or not setting the
respective params on the file & exec types.

A possible alternative would be to introduce a dep on the $::gid fact
but that would mean the entire module would depend on a version of
facter than hasn't shipped yet (unworkable) or to add a gid/egid fact
into this module (ugly).
This commit is contained in:
Joshua Hoblitt 2013-10-24 08:48:37 -07:00
parent 1ee9c5b98d
commit aa0180d69a
6 changed files with 29 additions and 95 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

9
manifests/fragment.pp
View file

@ -32,7 +32,7 @@ define concat::fragment(
$order = 10,
$ensure = 'present',
$mode = '0644',
$owner = $::id,
$owner = undef,
$group = undef
) {
validate_absolute_path($target)
@ -51,11 +51,6 @@ define concat::fragment(
$concatdir = $concat::setup::concatdir
$fragdir = "${concatdir}/${safe_target_name}"
$safe_group = $group ? {
undef => $concat::setup::root_group,
default => $group,
}
# if content is passed, use that, else if source is passed use that
# if neither passed, but $ensure is in symlink form, make a symlink
case $ensure {
@ -73,7 +68,7 @@ define concat::fragment(
ensure => $ensure,
mode => $mode,
owner => $owner,
group => $safe_group,
group => $group,
source => $source,
content => $content,
backup => false,

21
manifests/init.pp
View file

@ -55,7 +55,7 @@
define concat(
$ensure = 'present',
$path = $name,
$owner = $::id,
$owner = undef,
$group = undef,
$mode = '0644',
$warn = false,
@ -87,11 +87,6 @@ define concat(
$concat_name = 'fragments.concat.out'
$default_warn_message = '# This file is managed by Puppet. DO NOT EDIT.'
$safe_group = $group ? {
undef => $concat::setup::root_group,
default => $group,
}
if $warn == true {
$use_warn_message = $warn_message ? {
undef => $default_warn_message,
@ -124,7 +119,7 @@ define concat(
File {
owner => $owner,
group => $safe_group,
group => $group,
mode => $mode,
replace => $replace,
backup => false,
@ -166,6 +161,8 @@ define concat(
exec { "concat_${name}":
alias => "concat_${fragdir}",
command => $command,
user => $owner,
group => $group,
notify => File[$name],
subscribe => File[$fragdir],
unless => "${command} -t",
@ -175,15 +172,7 @@ define concat(
File["${fragdir}/fragments.concat"],
],
}
if $::id == 'root' {
Exec["concat_${name}"] {
user => root,
group => $safe_group,
}
}
}
else {
} else {
file { [
$fragdir,
"${fragdir}/fragments",

12
manifests/setup.pp
View file

@ -14,12 +14,6 @@ class concat::setup {
fail("Use of private class ${name} by ${caller_module_name}")
}
$owner = $::id
$root_group = $owner ? {
root => 0,
default => $owner
}
if $::concat_basedir {
$concatdir = $::concat_basedir
} else {
@ -31,17 +25,13 @@ class concat::setup {
}
file { "${concatdir}/bin/concatfragments.sh":
owner => $owner,
group => $root_group,
mode => '0755',
source => 'puppet:///modules/concat/concatfragments.sh',
}
file { [ $concatdir, "${concatdir}/bin" ]:
ensure => directory,
owner => $owner,
group => $root_group,
mode => '0750',
mode => '0755',
}
## Old versions of this module used a different path.

26
spec/unit/classes/concat_setup_spec.rb
View file

@ -2,16 +2,13 @@ require 'spec_helper'
describe 'concat::setup', :type => :class do
shared_examples 'setup' do |id, concatdir|
id = 'root' if id.nil?
shared_examples 'setup' do |concatdir|
concatdir = '/foo' if concatdir.nil?
let(:facts) {{ :id => id, :concat_basedir => concatdir }}
let(:facts) {{ :concat_basedir => concatdir }}
it do
should contain_file("#{concatdir}/bin/concatfragments.sh").with({
:owner => id,
:group => id == 'root' ? 0 : id,
:mode => '0755',
:source => 'puppet:///modules/concat/concatfragments.sh',
:backup => false,
@ -22,9 +19,7 @@ describe 'concat::setup', :type => :class do
it do
should contain_file(file).with({
:ensure => 'directory',
:owner => id,
:group => id == 'root' ? 0 : id,
:mode => '0750',
:mode => '0755',
:backup => false,
})
end
@ -45,23 +40,12 @@ describe 'concat::setup', :type => :class do
"$caller_module_name = 'concat'"
end
context 'id =>' do
context 'root' do
it_behaves_like 'setup', 'root'
end
context 'apenny' do
it_behaves_like 'setup', 'apenny'
end
end
context 'concat_basedir =>' do
context '/foo' do
it_behaves_like 'setup', 'root', '/foo'
it_behaves_like 'setup', '/foo'
end
context 'undef' do
let(:facts) {{ :id => 'root' }}
it 'should fail' do
expect { should }.to raise_error(Puppet::Error, /#{Regexp.escape('$concat_basedir not defined. Try running again with pluginsync=true on the [master] and/or [main] section of your node\'s \'/etc/puppet/puppet.conf\'.')}/)
end
@ -71,7 +55,7 @@ describe 'concat::setup', :type => :class do
end # facts
context 'called from another module namespace' do
let(:facts) {{ :id => 'root', :concat_basedir => '/foo' }}
let(:facts) {{ :concat_basedir => '/foo' }}
it 'should fail' do
expect { should }.to raise_error(Puppet::Error, /Use of private class concat::setup/)
end

34
spec/unit/defines/concat_fragment_spec.rb
View file

@ -5,15 +5,13 @@ describe 'concat::fragment', :type => :define do
shared_examples 'fragment' do |title, params|
params = {} if params.nil?
id = 'root'
p = {
:content => nil,
:source => nil,
:order => 10,
:ensure => 'present',
:mode => '0644',
:owner => id,
:owner => nil,
:group => nil,
:backup => 'puppet',
}.merge(params)
@ -22,19 +20,9 @@ describe 'concat::fragment', :type => :define do
safe_target_name = p[:target].gsub(/[\/\n]/, '_')
concatdir = '/var/lib/puppet/concat'
fragdir = "#{concatdir}/#{safe_target_name}"
if p[:group].nil?
safe_group = id == 'root' ? 0 : id
else
safe_group = p[:group]
end
let(:title) { title }
let(:facts) do
{
:concat_basedir => concatdir,
:id => id,
}
end
let(:facts) {{ :concat_basedir => concatdir }}
let(:params) { params }
let(:pre_condition) do
"concat{ '#{p[:target]}': }"
@ -47,7 +35,7 @@ describe 'concat::fragment', :type => :define do
:ensure => p[:ensure],
:mode => p[:mode],
:owner => p[:owner],
:group => safe_group,
:group => p[:group],
:source => p[:source],
:content => p[:content],
:alias => "concat_fragment_#{title}",
@ -66,7 +54,7 @@ describe 'concat::fragment', :type => :define do
['./etc/motd', 'etc/motd', false].each do |target|
context target do
let(:title) { 'motd_header' }
let(:facts) {{ :concat_basedir => '/tmp', :id => 'root' }}
let(:facts) {{ :concat_basedir => '/tmp' }}
let(:params) {{ :target => target }}
it 'should fail' do
@ -88,7 +76,7 @@ describe 'concat::fragment', :type => :define do
context 'invalid' do
let(:title) { 'motd_header' }
let(:facts) {{ :concat_basedir => '/tmp', :id => 'root' }}
let(:facts) {{ :concat_basedir => '/tmp' }}
let(:params) {{ :ensure => 'invalid', :target => '/etc/motd' }}
it 'should fail' do
@ -109,7 +97,7 @@ describe 'concat::fragment', :type => :define do
context 'false' do
let(:title) { 'motd_header' }
let(:facts) {{ :concat_basedir => '/tmp', :id => 'root' }}
let(:facts) {{ :concat_basedir => '/tmp' }}
let(:params) {{ :content => false, :target => '/etc/motd' }}
it 'should fail' do
@ -130,7 +118,7 @@ describe 'concat::fragment', :type => :define do
context 'false' do
let(:title) { 'motd_header' }
let(:facts) {{ :concat_basedir => '/tmp', :id => 'root' }}
let(:facts) {{ :concat_basedir => '/tmp' }}
let(:params) {{ :source => false, :target => '/etc/motd' }}
it 'should fail' do
@ -151,7 +139,7 @@ describe 'concat::fragment', :type => :define do
context 'false' do
let(:title) { 'motd_header' }
let(:facts) {{ :concat_basedir => '/tmp', :id => 'root' }}
let(:facts) {{ :concat_basedir => '/tmp' }}
let(:params) {{ :order => false, :target => '/etc/motd' }}
it 'should fail' do
@ -170,7 +158,7 @@ describe 'concat::fragment', :type => :define do
context 'false' do
let(:title) { 'motd_header' }
let(:facts) {{ :concat_basedir => '/tmp', :id => 'root' }}
let(:facts) {{ :concat_basedir => '/tmp' }}
let(:params) {{ :mode => false, :target => '/etc/motd' }}
it 'should fail' do
@ -189,7 +177,7 @@ describe 'concat::fragment', :type => :define do
context 'false' do
let(:title) { 'motd_header' }
let(:facts) {{ :concat_basedir => '/tmp', :id => 'root' }}
let(:facts) {{ :concat_basedir => '/tmp' }}
let(:params) {{ :owner => false, :target => '/etc/motd' }}
it 'should fail' do
@ -208,7 +196,7 @@ describe 'concat::fragment', :type => :define do
context 'false' do
let(:title) { 'motd_header' }
let(:facts) {{ :concat_basedir => '/tmp', :id => 'root' }}
let(:facts) {{ :concat_basedir => '/tmp' }}
let(:params) {{ :group => false, :target => '/etc/motd' }}
it 'should fail' do

22
spec/unit/defines/concat_spec.rb
View file

@ -5,14 +5,12 @@ describe 'concat', :type => :define do
shared_examples 'concat' do |title, params|
params = {} if params.nil?
id = 'root'
# default param values
p = {
:ensure => 'present',
:path => title,
:owner => id,
:group => '0',
:owner => nil,
:group => nil,
:mode => '0644',
:warn => false,
:warn_message => nil,
@ -39,12 +37,7 @@ describe 'concat', :type => :define do
let(:title) { title }
let(:params) { params }
let(:facts) do
{
:concat_basedir => concatdir,
:id => id,
}
end
let(:facts) {{ :concat_basedir => concatdir }}
if p[:ensure] == 'present'
it do
@ -102,15 +95,10 @@ describe 'concat', :type => :define do
should contain_exec("concat_#{title}").with({
:alias => "concat_#{fragdir}",
:command => cmd,
:user => p[:owner],
:group => p[:group],
:unless => "#{cmd} -t",
})
if id == 'root'
should contain_exec("concat_#{title}").with({
:user => 'root',
:group => p[:group],
})
end
end
else
[