-
-
Notifications
You must be signed in to change notification settings - Fork 39
Parameterize targetfile in puppetserver::config::bootstrap #54
base: master
Are you sure you want to change the base?
Changes from all commits
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 |
---|---|---|
@@ -1,5 +1,6 @@ | ||
define puppetserver::config::bootstrap ( | ||
$ensure = 'present', | ||
Optional[Stdlib::Absolutepath] $targetfile = undef, | ||
# For compat with other config types, unused | ||
$value = undef, | ||
) { | ||
|
@@ -19,15 +20,28 @@ | |
} | ||
} | ||
|
||
if versioncmp($::puppetversion, '4.0.0') >= 0 { | ||
$targetfile = '/etc/puppetlabs/puppetserver/bootstrap.cfg' | ||
# puppetserver >= 2.5 changed the path to the bootstrap configuration file [1] [2] [3]. | ||
# $::puppetversion is the version of the Puppet agent on the client and can | ||
# not reliable distinguish the puppetserver version running on the master. | ||
# For puppetserver 2.5 and above you need to set the targetfile to: | ||
# '/etc/puppetlabs/puppetserver/services.d/ca.cfg' | ||
# | ||
# [1] https://github.com/voxpupuli/puppet-puppetserver/issues/52 | ||
# [2] https://puppet.com/docs/puppetserver/2.7/release_notes.html#potential-breaking-issues-when-upgrading-with-a-modified-bootstrapcfg | ||
# [3] https://puppet.com/docs/puppetserver/2.7/release_notes.html#new-feature-flexible-service-bootstrappingca-configuration-file | ||
if $targetfile { | ||
$targetfile_real = $targetfile | ||
} else { | ||
$targetfile = '/etc/puppetserver/bootstrap.cfg' | ||
if versioncmp($::puppetversion, '4.0.0') >= 0 { | ||
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. Shouldn't this be using the version of the puppetserver software and not the agent? I realize you are maintaining the current functionality, though it seems that is not what should happen here. 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. Yes that's exactly what I described in the issue #52. From my knowledge there is no puppetserver version fact available to achieve that. We had the same issue in your ssh module [1] and I found no workaround for puppet master software related version numbers. [1] https://github.com/ghoneycutt/puppet-module-common/blob/master/manifests/mkuser.pp#L113-L117 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. Could you add a small comment to the code and link to your issue? That would really help those who might be debugging this issue as it is possible they have the new agent and slightly older puppetserver. 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. comment added |
||
$targetfile_real = '/etc/puppetlabs/puppetserver/bootstrap.cfg' | ||
} else { | ||
$targetfile_real = '/etc/puppetserver/bootstrap.cfg' | ||
} | ||
} | ||
|
||
augeas { "Set puppetserver bootstrap ${title}": | ||
lens => 'Simplelines.lns', | ||
incl => $targetfile, | ||
incl => $targetfile_real, | ||
changes => $changes, | ||
onlyif => $onlyif, | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
require 'spec_helper' | ||
|
||
describe 'puppetserver::config::bootstrap' do | ||
let(:title) { 'foo' } | ||
|
||
on_supported_os.each do |os, facts| | ||
context "on #{os}" do | ||
context 'without param' do | ||
it do | ||
is_expected.to contain_augeas('Set puppetserver bootstrap foo').with( | ||
lens: 'Simplelines.lns', | ||
incl: '/etc/puppetlabs/puppetserver/bootstrap.cfg', | ||
changes: 'set ./01 \'foo\'', | ||
onlyif: 'match ./*[label()!=\'#comment\' and .=\'foo\'] size == 0' | ||
) | ||
end | ||
end | ||
|
||
context 'when ensure => absent' do | ||
let(:params) { { ensure: 'absent' } } | ||
|
||
it do | ||
is_expected.to contain_augeas('Set puppetserver bootstrap foo').with( | ||
changes: 'rm ./*[label()!=\'#comment\' and .=\'foo\']', | ||
onlyif: 'match ./*[label()!=\'#comment\' and .=\'foo\'] size != 0' | ||
) | ||
end | ||
end | ||
|
||
context 'when fact puppetversion => 3.9.9' do | ||
let(:facts) { facts.merge(puppetversion: '3.9.9') } | ||
|
||
it { is_expected.to contain_augeas('Set puppetserver bootstrap foo').with_incl('/etc/puppetserver/bootstrap.cfg') } | ||
end | ||
|
||
context 'when fact puppetversion => 4.0.0' do | ||
let(:facts) { facts.merge(puppetversion: '4.0.0') } | ||
|
||
it { is_expected.to contain_augeas('Set puppetserver bootstrap foo').with_incl('/etc/puppetlabs/puppetserver/bootstrap.cfg') } | ||
end | ||
|
||
context 'when targetfile => /etc/puppetlabs/puppetserver/services.d/ca.cfg' do | ||
let(:params) { { targetfile: '/etc/puppetlabs/puppetserver/services.d/ca.cfg' } } | ||
|
||
it { is_expected.to contain_augeas('Set puppetserver bootstrap foo').with_incl('/etc/puppetlabs/puppetserver/services.d/ca.cfg') } | ||
end | ||
end | ||
end | ||
|
||
describe 'variable data type and content validations' do | ||
validations = { | ||
'Optional[Stdlib::Absolutepath]' => { | ||
param: %w[targetfile], | ||
valid: %w[/absolute/filepath /absolute/directory/], | ||
invalid: ['../string', %w[array], { 'ha' => 'sh' }, 3, 2.42, false, nil], | ||
message: 'expects a (match for|match for Stdlib::Absolutepath =|Stdlib::Absolutepath =) Variant\[Stdlib::Windowspath.*Stdlib::Unixpath' # Puppet (4.x|5.0 & 5.1|5.x) | ||
}, | ||
'String for ensure' => { | ||
param: %w[ensure], | ||
valid: %w[absent present], | ||
invalid: ['string', %w[array], { 'ha' => 'sh' }, 3, 2.42, false], | ||
message: 'Wrong value for "ensure"' | ||
} | ||
} | ||
|
||
validations.sort.each do |type, var| | ||
mandatory_facts = {} if mandatory_facts.nil? | ||
mandatory_params = {} if mandatory_params.nil? | ||
var[:param].each do |parameter| | ||
var[:facts] = {} if var[:facts].nil? | ||
var[:params] = {} if var[:params].nil? | ||
|
||
var[:valid].each do |valid| | ||
context "when #{parameter} (#{type}) is set to valid #{valid} (as #{valid.class})" do | ||
let(:facts) { [mandatory_facts, var[:facts]].reduce(:merge) } | ||
let(:params) { [mandatory_params, var[:params], { :"#{parameter}" => valid }].reduce(:merge) } | ||
|
||
it { is_expected.to compile } | ||
end | ||
end | ||
|
||
var[:invalid].each do |invalid| | ||
context "when #{parameter} (#{type}) is set to invalid #{invalid} (as #{invalid.class})" do | ||
let(:params) { [mandatory_params, var[:params], { :"#{parameter}" => invalid }].reduce(:merge) } | ||
|
||
it 'fails' do | ||
expect { is_expected.to contain_class(subject) }.to raise_error(Puppet::Error, %r{#{var[:message]}}) | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end | ||
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.
This is a bit confusing as the define actually does this for us, so it is not needed here.
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.
The define is not aware of the used puppetserver version, so it will not be able to choose the correct path. You need to manually set this.
If $puppetserver::version would always contain a version number we could use that, but it is needed to also support the default of 'present' that can be any version.
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.
Technical it would be possible to use
generate()
to get the used puppetserver version.Usually I would prefer to avoid generators for the unneeded additional load on the puppet masters.
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.
Agreed we should not use the generator function.