Skip to content

[PE-17491] Do not fail on install when a restart exit code is returned #200

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

Merged
merged 1 commit into from
Mar 3, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 7 additions & 5 deletions lib/puppet/provider/sqlserver.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,19 @@ class Puppet::Provider::Sqlserver < Puppet::Provider
'powershell.exe'
end

def try_execute(command, msg = nil, obfuscate_strings = nil)
begin
execute(command.compact)
rescue Puppet::ExecutionFailure => error
def try_execute(command, msg = nil, obfuscate_strings = nil, acceptable_exit_codes = [0])
res = execute(command.compact, failonfail: false)
Copy link
Contributor

Choose a reason for hiding this comment

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

This has changed behaviour. Previously this was in a begin rescue block

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 know this has changed behaviour, however the error which is raised and surfaced in the rescue block does not contain the exit code to be able to handle this there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@glennsarti are you happy with this solution? I have tried to replicate the error message which is raised by the execute function when a non-zero exit code is returned, see here: [link].(https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/execution.rb#L235)

I know this is a change in how the try_execute method functions, however the behaviour to the user is exactly the same and the error messages surfaced are exactly the same, except for allowing non-zero exit codes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Without the begin rescue block, the obfuscate strings later on never gets called. This will regress the Password leaking issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wait...I see you have failonfail set to false


unless acceptable_exit_codes.include?(res.exitstatus)
msg = "Failure occured when trying to install SQL Server #{@resource[:name]}" if msg.nil?
msg += " \n #{error}"
msg += " \n Execution of '#{command}' returned #{res.exitstatus}: #{res.strip}"

obfuscate_strings.each {|str| msg.gsub!(str, '**HIDDEN VALUE**') } unless obfuscate_strings.nil?

raise Puppet::Error, msg
end

res
end

private
Expand Down
5 changes: 4 additions & 1 deletion lib/puppet/provider/sqlserver_features/mssql.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,10 @@ def modify_features(action, features)
begin
config_file = create_temp_for_install_switch unless action == 'uninstall'
cmd_args << "/ConfigurationFile=\"#{config_file.path}\"" unless config_file.nil?
try_execute(cmd_args, "Unable to #{action} features (#{features.join(', ')})")
res = try_execute(cmd_args, "Unable to #{action} features (#{features.join(', ')})", nil, [0, 1641, 3010])

warn("#{action} of features (#{features.join(', ')} returned exit code 3010 - reboot required") if res.exitstatus == 3010
warn("#{action} of features (#{features.join(', ')} returned exit code 1641 - reboot initiated") if res.exitstatus == 1641
ensure
if config_file
config_file.close
Expand Down
11 changes: 9 additions & 2 deletions lib/puppet/provider/sqlserver_instance/mssql.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ def modify_features(features, action)
begin
config_file = create_temp_for_install_switch unless action == 'uninstall'
cmd_args << "/ConfigurationFile=\"#{config_file.path}\"" unless config_file.nil?
try_execute(cmd_args, "Error trying to #{action} features (#{features.join(', ')}", obfuscated_strings)
res = try_execute(cmd_args, "Error trying to #{action} features (#{features.join(', ')}", obfuscated_strings, [0, 1641, 3010])

warn("#{action} of features (#{features.join(', ')}) returned exit code 3010 - reboot required") if res.exitstatus == 3010
warn("#{action} of features (#{features.join(', ')}) returned exit code 1641 - reboot initiated") if res.exitstatus == 1641
ensure
if config_file
config_file.close
Expand Down Expand Up @@ -161,7 +164,11 @@ def format_cmd_args_array(switch, arr, cmd_args, use_discrete = false)

def destroy
cmd_args = basic_cmd_args(current_installed_features, 'uninstall')
try_execute(cmd_args, "Unable to uninstall instance #{@resource[:name]}")
res = try_execute(cmd_args, "Unable to uninstall instance #{@resource[:name]}", nil, [0, 1641, 3010])

warn("Uninstall of instance #{@resource[:name]} returned exit code 3010 - reboot required") if res.exitstatus == 3010
warn("Uninstall of instance #{@resource[:name]} returned exit code 1641 - reboot initiated") if res.exitstatus == 1641

@property_hash.clear
exists? ? (return false) : (return true)
end
Expand Down
68 changes: 62 additions & 6 deletions spec/unit/puppet/provider/sqlserver__instance_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,16 @@
subject { provider_class }
let(:additional_install_switches) { [] }

def stub_uninstall(args, installed_features)
def stub_uninstall(args, installed_features, exit_code = 0)
cmd_args = ["#{args[:source]}/setup.exe",
"/ACTION=uninstall",
'/Q',
'/IACCEPTSQLSERVERLICENSETERMS',
"/INSTANCENAME=#{args[:name]}",
"/FEATURES=#{installed_features.join(',')}",]
Puppet::Util::Execution.stubs(:execute).with(cmd_args.compact).returns(0)

result = Puppet::Util::Execution::ProcessOutput.new('', exit_code)
Puppet::Util::Execution.stubs(:execute).with(cmd_args.compact, failonfail: false).returns(result)
end

shared_examples 'run' do |args, munged_values = {}|
Expand Down Expand Up @@ -46,7 +48,7 @@ def stub_uninstall(args, installed_features)
@provider.create
}
end
shared_examples 'create' do
shared_examples 'create' do |exit_code, warning_matcher|
it {
execute_args = args.merge(munged_values)
@resource = Puppet::Type::Sqlserver_instance.new(args)
Expand Down Expand Up @@ -77,20 +79,27 @@ def stub_uninstall(args, installed_features)
additional_install_switches.each do |switch|
cmd_args << switch
end
Puppet::Util::Execution.stubs(:execute).with(cmd_args.compact).returns(0)

# If warning_matcher supplied ensure warnings raised match, otherwise no warnings raised
@provider.stubs(:warn).with(regexp_matches(warning_matcher)).returns(nil).times(1) if warning_matcher
@provider.stubs(:warn).with(anything).times(0) unless warning_matcher

result = Puppet::Util::Execution::ProcessOutput.new('', exit_code || 0)
Puppet::Util::Execution.stubs(:execute).with(cmd_args.compact, failonfail: false).returns(result)
@provider.create
}
end


shared_examples 'destroy' do
shared_examples 'destroy' do |exit_code, warning_matcher|
it {
@resource = Puppet::Type::Sqlserver_instance.new(args)
@provider = provider_class.new(@resource)

stub_source_which_call args[:source]
@provider.expects(:current_installed_features).returns(installed_features)
stub_uninstall args, installed_features
stub_uninstall args, installed_features, exit_code || 0
@provider.stubs(:warn).with(regexp_matches(warning_matcher)).returns(nil).times(1) if warning_matcher
@provider.destroy
}
end
Expand Down Expand Up @@ -118,6 +127,30 @@ def stub_uninstall(args, installed_features)
end
end

describe 'it should raise warning on install when 1641 exit code returned' do
it_behaves_like 'create', 1641, /reboot initiated/i do
args = get_basic_args
let(:args) { args }
munged = {:features => Array.new(args[:features])}
munged[:features].delete('SQL')
munged[:features] += %w(DQ FullText Replication SQLEngine)
munged[:features].sort!
let(:munged_values) { munged }
end
end

describe 'it should raise warning on install when 3010 exit code returned' do
it_behaves_like 'create', 3010, /reboot required/i do
args = get_basic_args
let(:args) { args }
munged = {:features => Array.new(args[:features])}
munged[:features].delete('SQL')
munged[:features] += %w(DQ FullText Replication SQLEngine)
munged[:features].sort!
let(:munged_values) { munged }
end
end

describe 'empty array should' do
it_behaves_like 'destroy on create' do
let(:installed_features) { %w(SQLEngine Replication) }
Expand All @@ -140,6 +173,29 @@ def stub_uninstall(args, installed_features)
end

end

describe 'it should raise warning on uninstall when 1641 exit code returned' do
it_behaves_like 'destroy', 1641, /reboot initiated/i do
let(:args) { {
:name => 'MYSQLSERVER',
:source => 'C:\myinstallexecs',
:features => []
} }
let(:installed_features) { %w(SQLEngine Replication) }
end
end

describe 'it should raise warning on uninstall when 3010 exit code returned' do
it_behaves_like 'destroy', 3010, /reboot required/i do
let(:args) { {
:name => 'MYSQLSERVER',
:source => 'C:\myinstallexecs',
:features => []
} }
let(:installed_features) { %w(SQLEngine Replication) }
end
end

describe 'installed features even if provided features' do
it_behaves_like 'destroy' do
let(:args) { {
Expand Down
31 changes: 25 additions & 6 deletions spec/unit/puppet/provider/sqlserver_features_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
let(:additional_params) { {} }
let(:munged_args) { {} }
let(:additional_switches) { [] }
shared_examples 'create' do
shared_examples 'create' do |exit_code, warning_matcher|
it {
params.merge!(additional_params)
@resource = Puppet::Type::Sqlserver_features.new(params)
Expand All @@ -24,7 +24,8 @@
stub_powershell_call(subject)

executed_args = params.merge(munged_args)
stub_add_features(executed_args, executed_args[:features], additional_switches)
stub_add_features(executed_args, executed_args[:features], additional_switches, exit_code || 0)
@provider.stubs(:warn).with(regexp_matches(warning_matcher)).returns(nil).times(1) if warning_matcher
@provider.create
}
end
Expand Down Expand Up @@ -65,20 +66,23 @@
it_should_behave_like 'create'
end

shared_examples 'features=' do |args|
shared_examples 'features=' do |args, exit_code, warning_matcher|
it {
@resource = Puppet::Type::Sqlserver_features.new(args)
@provider = provider_class.new(@resource)

stub_powershell_call(subject)
stub_source_which_call args
if !feature_remove.empty?
stub_remove_features(args, feature_remove)
stub_remove_features(args, feature_remove, exit_code || 0)
end
if !feature_add.empty?
stub_add_features(args, feature_add)
stub_add_features(args, feature_add, [], exit_code || 0)
end

# If warning_matcher supplied ensure warnings raised match, otherwise no warnings raised
@provider.stubs(:warn).with(regexp_matches(warning_matcher)).returns(nil).times(1) if warning_matcher
@provider.stubs(:warn).with(anything).times(0) unless warning_matcher
@provider.create
}
end
Expand Down Expand Up @@ -106,6 +110,20 @@
it_should_behave_like 'features=', @feature_params
end

context 'it should raise warning on feature install when 1641 exit code returned' do
include_context 'features'
@feature_params[:features] = %w(SSMS)
let(:feature_add) { %w(SSMS) }
it_should_behave_like 'features=', @feature_params, 1641, /reboot initiated/i
end

context 'it should raise warning on feature install when 3010 exit code returned' do
include_context 'features'
@feature_params[:features] = %w(SSMS)
let(:feature_add) { %w(SSMS) }
it_should_behave_like 'features=', @feature_params, 3010, /reboot required/i
end

context 'it should install the expanded tools set' do
include_context 'features'
@feature_params[:features] = %w(Tools)
Expand All @@ -131,13 +149,14 @@
@provider = provider_class.new(@resource)
@provider.stubs(:current_installed_features).returns(%w(SSMS ADV_SSMS Conn))
Puppet::Util.stubs(:which).with("#{feature_params[:source]}/setup.exe").returns("#{feature_params[:source]}/setup.exe")
result = Puppet::Util::Execution::ProcessOutput.new('', 0)
Puppet::Util::Execution.expects(:execute).with(
["#{feature_params[:source]}/setup.exe",
"/ACTION=uninstall",
'/Q',
'/IACCEPTSQLSERVERLICENSETERMS',
"/FEATURES=#{%w(SSMS ADV_SSMS Conn).join(',')}",
]).returns(0)
], failonfail: false).returns(result)
@provider.create
}
end
Expand Down
15 changes: 9 additions & 6 deletions spec/unit/puppet/sqlserver_spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ def stub_powershell_call(subject)
Puppet::Provider::Sqlserver.stubs(:run_install_dot_net).returns()
end

def stub_add_features(args, features, additional_switches = [])
stub_modify_features('install', args, features, additional_switches)
def stub_add_features(args, features, additional_switches = [], exit_code = 0)
stub_modify_features('install', args, features, additional_switches, exit_code)
end

def stub_remove_features(args, features)
stub_modify_features('uninstall', args, features)
def stub_remove_features(args, features, exit_code = 0)
stub_modify_features('uninstall', args, features, [], exit_code)
end

def stub_modify_features(action, args, features, additional_switches = [])
def stub_modify_features(action, args, features, additional_switches = [], exit_code = 0)
cmds = ["#{args[:source]}/setup.exe",
"/ACTION=#{action}",
'/Q',
Expand All @@ -31,5 +31,8 @@ def stub_modify_features(action, args, features, additional_switches = [])
additional_switches.each do |switch|
cmds << switch
end
Puppet::Util::Execution.stubs(:execute).with(cmds).returns(0)

result = Puppet::Util::Execution::ProcessOutput.new('', exit_code)

Puppet::Util::Execution.stubs(:execute).with(cmds, failonfail: false).returns(result)
end