Skip to content

(FM-2577) - Change from sqlcmd.exe to win32ole connector #99

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 3 commits into from
May 13, 2015
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
1 change: 0 additions & 1 deletion .fixtures.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,5 @@ fixtures:
forge_modules:
acl: "puppetlabs/acl"
stdlib: "puppetlabs/stdlib"
acl: "puppetlabs/acl"
symlinks:
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting... duplicate eh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think this came from a merge conflict.

Copy link
Contributor

Choose a reason for hiding this comment

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

right on.

sqlserver: "#{source_dir}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this actually work now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Symlinking on Windows that is.

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 does if you have puppetlabs_spec_helper > 0.9 and puppet > 3.5

Copy link
Contributor

Choose a reason for hiding this comment

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

:)

2 changes: 1 addition & 1 deletion lib/puppet/provider/sqlserver.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def try_execute(command, msg = nil)
##
def self.run_authenticated_sqlcmd(query, opts)
b = binding
@sql_instance_config = "C:/Program Files/Microsoft SQL Server/.puppet/.#{opts[:instance_name]}.cfg"
@sql_instance_config = File.join(Puppet[:vardir], "cache/sqlserver/.#{resource[:instance]}.cfg")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

if File.exists?(@sql_instance_config)
@sql_instance_config = native_path(@sql_instance_config)
else
Expand Down
32 changes: 24 additions & 8 deletions lib/puppet/provider/sqlserver_tsql/mssql.rb
Original file line number Diff line number Diff line change
@@ -1,17 +1,33 @@
require File.expand_path(File.join(File.dirname(__FILE__), '..', 'sqlserver'))
require File.expand_path(File.join(File.dirname(__FILE__), '..', '..', '..', 'puppet_x/sqlserver/sql_connection'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Wheeeeee. I've never understood when and why things go in puppet_x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a convention I believe that came out of moving non puppet related code out. @hunner maybe you could give a better history of this.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds great! I would like to understand the history of this.


Puppet::Type::type(:sqlserver_tsql).provide(:mssql, :parent => Puppet::Provider::Sqlserver) do

def run(query, opts)
debug("Running resource #{query} against #{resource[:instance]} with failonfail set to #{opts[:failonfail]}")
opts[:instance_name] = resource[:instance]
result = Puppet::Provider::Sqlserver.run_authenticated_sqlcmd(query, opts)
return result

def run(query)
debug("Running resource #{query} against #{resource[:instance]}")
Copy link
Contributor

Choose a reason for hiding this comment

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

the removal of failonfail isn't mentioned here - why removed?

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 is not an execution of sqlcmd.exe and as a result does not take a failonfail param as is the case with command execution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right on.

config = get_instance_config
sqlconn = PuppetX::Sqlserver::SqlConnection.new

sqlconn.open_and_run_command(query, config)
end

def run_update
result = self.run(resource[:command], {:failonfail => true})
return result
def get_instance_config
config_file = File.join(Puppet[:vardir], "cache/sqlserver/.#{resource[:instance]}.cfg")
Copy link
Contributor

Choose a reason for hiding this comment

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

While I'm thinking about this, what does instance equate to? Could it possible have any non-legit characters like \ or /?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it is not. Instance is the Named Instance. https://msdn.microsoft.com/en-us/library/ms143531.aspx

Embedded spaces or other special characters are not allowed in instance names. The 
backslash (\), comma (,), colon (:), semi-colon (;), single quote ('), ampersand (&), and 
at sign (@) are also not allowed.

if !File.exists? (config_file)
fail('Required config file missing, add the appropriate sqlserver::config and rerun')
end
if !File.readable?(config_file)
fail('Unable to read config file, ensure proper permissions and please try again')
end
JSON.parse(File.read(config_file))
end

def run_check
return self.run(resource[:onlyif])
end

def run_update
return self.run(resource[:command])
end
end
9 changes: 5 additions & 4 deletions lib/puppet/type/sqlserver_tsql.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,7 @@ def self.checks
newcheck(:onlyif, :parent => Puppet::Property::SqlserverTsql) do
#Runs in the event that our TSQL exits with anything other than 0
def check(value)
begin
output = provider.run(value, :failonfail => false)
end
output = provider.run(value)
debug("OnlyIf returned exitstatus of #{output.exitstatus}")
output.exitstatus != 0
end
Expand Down Expand Up @@ -61,7 +59,10 @@ def output

def refresh
if self.check_all_attributes(true)
provider.run_update
result = provider.run_update
if result.has_errors
fail("Unable to apply changes, failed with error message #{result.error_message}")
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to pass along the error if something goes wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct if we are unable to apply the update we raise a failure and give the user the error message have received from sqlserver

end
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

end
end

Expand Down
137 changes: 137 additions & 0 deletions lib/puppet_x/sqlserver/sql_connection.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
module PuppetX
module Sqlserver

CONNECTION_CLOSED = 0

class SqlConnection
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a doc that links back to our original source for this code?

Let's also make sure we follow the licensing policy as well.

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 got this almost entirely from http://rubyonwindows.blogspot.com/2007/03/ruby-ado-and-sqlserver.html, I glanced at the other but it had so much abstraction that it didn't even go down that path.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok - I'm not sure how this works then given this SO answer about snippet licensing - http://meta.stackexchange.com/a/12537

Maybe @stahnma has an opinion on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The amount of code we used from there was so minor, we took the oh yeah, that's how you create a win32ole adodb.connection, all the rest is really stuff I added for error handling.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I know you added a bunch of additional work here. Unfortunately it doesn't really matter how little / much if it was copied from somewhere initially.

I think we're OK, but I just want to make sure we're in legal accordance here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, we need some sort of credit to that post and David Mullet.

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 am all for given credit where credit is due. However how would you go about this?

attr_reader :exception_caught


def initialize
@connection = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.

@data = nil
end

def open_and_run_command(query, config)
begin
open(config)
command(query)
ensure
close
end

result
end

private
def connection
@connection ||= create_connection
end

def open(config)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this move to private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? We might want to use it for other things other than open_and_run_command. We could use it for getting data fields later on.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could, later on, also move it out of private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why this diff hasn't updated but it was moved inside the private call.

connection_string = get_connection_string(config)
connection.Open(connection_string) unless !connection_closed?
end

def get_connection_string(config)
config = {'database' => 'master'}.merge(config)
# Open ADO connection to the SQL Server database
connection_string = "Provider=SQLOLEDB.1;"
connection_string << "Persist Security Info=False;"
connection_string << "User ID=#{config['admin']};"
connection_string << "password=#{config['pass']};"
connection_string << "Initial Catalog=#{config['database']};"
connection_string << "Application Name=Puppet;"
if config['instance'] !~ /^MSSQLSERVER$/
connection_string << "Data Source=localhost\\#{config['instance']};"
else
connection_string << "Data Source=localhost;"
end
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 this? As in why am I seeing localhost here on either side of the config?

Copy link
Contributor

Choose a reason for hiding this comment

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

Data source should not always be localhost unless I'm missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So for example we have a named instance on the localhost you have to have \Instance name, if it is however the default instance of MSSQLSERVER and you add localhost\MSSQLSERVER it will fail the connection.

end

def command(sql)
Copy link
Contributor

Choose a reason for hiding this comment

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

should this move to private then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change method signature so that the block is required.

def command(sql, &block)

reset_instance
begin
r = execute(sql)
yield(r) if block_given?
rescue win32_exception => e
@exception_caught = e
end
nil
end

def result
ResultOutput.new(has_errors, error_message)
end

def has_errors
@exception_caught != nil
end

def error_message
@exception_caught.message unless @exception_caught == nil
end

def close
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this move to private then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all except initialize and open_and_run_command have been moved to private

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

begin
connection.Close unless connection_closed?
rescue win32_exception => e
end
end

def reset_instance
@data = nil
@fields = nil
@exception_caught = nil
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. I think I would call this reset_instance

Copy link
Contributor

Choose a reason for hiding this comment

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

Mostly because it's called the first time and every time, not just after it has previously run.


def connection_closed?
connection.State == CONNECTION_CLOSED
end

def create_connection
require 'win32ole'
WIN32OLE.new('ADODB.Connection')
end

def execute (sql)
connection.Execute(sql, nil, nil)
end

def parse_column_names(result)
result.Fields.extend(Enumerable).map { |column| column.Name }
end

# having as a method instead of hard coded allows us to stub and test outside of Windows
def win32_exception
::WIN32OLERuntimeError
end

def connection=(conn)
@connection = conn
end
end

class ResultOutput
attr_reader :exitstatus, :error_message, :raw_error_message

def initialize(has_errors, error_message)
@exitstatus = has_errors ? 1 : 0
if error_message
@raw_error_message = error_message
@error_message = parse_for_error(error_message)
end
end

def has_errors
@exitstatus != 0
end

private
def parse_for_error(result)
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be private

match = result.match(/SQL Server\n\s+(.*)/i)
match[1] unless match == nil
end
end
end
end
10 changes: 3 additions & 7 deletions manifests/config.pp
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,10 @@
$instance_name = $title,
) {
#possible future parameter if we do end up supporting different install directories
$install_dir ='C:/Program Files/Microsoft SQL Server'
$config_dir = "${install_dir}/.puppet"
$config_dir = "${::puppet_vardir}/cache/sqlserver"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

$config_file = "${config_dir}/.${instance_name}.cfg"
if !defined(File[$config_dir]){
file{ $config_dir:
ensure => directory
}
}
ensure_resource('file', ["${::puppet_vardir}/cache",$config_dir], { 'ensure' => 'directory','recurse' => 'true' })
Copy link
Contributor

Choose a reason for hiding this comment

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

Magic? I think I understand what this is doing...

Copy link
Contributor

Choose a reason for hiding this comment

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

Where does ensure_resource come from?

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 is part of stdlib and will ensure a file resource exists if not already defined. Great helper so we don't double declare the same resource if we have multiple configs being specified. https://github.com/puppetlabs/puppetlabs-stdlib#ensure_resource

Copy link
Contributor

Choose a reason for hiding this comment

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

👍


file{ $config_file:
content => template('sqlserver/instance_config.erb'),
require => File[$config_dir],
Expand Down
12 changes: 6 additions & 6 deletions spec/defines/config_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@
RSpec.describe 'sqlserver::config', :type => :define do
let(:title) { 'MSSQLSERVER' }
let(:params) { {
:instance_name => 'MSSQLSERVER',
:admin_user => 'sa',
:admin_pass => 'Pupp3t1@',
:instance_name => 'MSSQLSERVER',
:admin_user => 'sa',
:admin_pass => 'Pupp3t1@',
} }
let(:facts) { {:osfamily => 'windows', :platform => :windows} }
let(:facts) { {:osfamily => 'windows', :platform => :windows, :puppet_vardir => 'C:/ProgramData/PuppetLabs/puppet/var'} }
describe 'compile' do
it {
should contain_file('C:/Program Files/Microsoft SQL Server/.puppet/.MSSQLSERVER.cfg')
should contain_file('C:/Program Files/Microsoft SQL Server/.puppet')
should contain_file('C:/ProgramData/PuppetLabs/puppet/var/cache/sqlserver/.MSSQLSERVER.cfg')
should contain_file('C:/ProgramData/PuppetLabs/puppet/var/cache/sqlserver')
Copy link
Contributor

Choose a reason for hiding this comment

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

:)

Copy link
Contributor

Choose a reason for hiding this comment

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

Except this might need to also point to the variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have mocked out the variable in the top scope

let(:facts) {{}}

so we shouldn't need it for the test case.

}
end
end
1 change: 0 additions & 1 deletion spec/functions/sqlserver_upcase_spec.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
#! /usr/bin/env ruby -S rspec
require 'spec_helper'
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


describe "the sqlserver_upcase function" do
Expand Down
1 change: 0 additions & 1 deletion spec/functions/sqlserver_validate_hash_uniq_values_spec.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
#! /usr/bin/env ruby -S rspec
require 'spec_helper'

describe "the sqlserver_validate_hash_uniq_values" do
Expand Down
85 changes: 85 additions & 0 deletions spec/unit/puppet_x/sql_connection_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
require 'rspec'
require 'spec_helper'
require File.expand_path(File.join(File.dirname(__FILE__), '..', '..', '..', 'lib/puppet_x/sqlserver/sql_connection'))

RSpec.describe PuppetX::Sqlserver::SqlConnection do
let(:subject) { PuppetX::Sqlserver::SqlConnection.new }
let(:config) { {'admin' => 'sa', 'pass' => 'Pupp3t1@', 'instance' => 'MSSQLSERVER'} }

def stub_connection
@connection = mock()
subject.stubs(:create_connection).returns(@connection)
subject.stubs(:win32_exception).returns(Exception)
end

def stub_no_errors
subject.stubs(:has_errors).returns(false)
subject.stubs(:error_message).returns(nil)
end

describe 'open_and_run_command' do
context 'command' do
before :each do
stub_connection
@connection.stubs(:State).returns(0)
@connection.stubs(:Open).with('Provider=SQLOLEDB.1;Persist Security Info=False;User ID=sa;password=Pupp3t1@;Initial Catalog=master;Application Name=Puppet;Data Source=localhost;')
end
it 'should not raise an error but populate has_errors with message' do
subject.stubs(:win32_exception).returns(Exception)
subject.stubs(:execute).raises(Exception.new("SQL Server\n error has happened"))
expect {
result = subject.open_and_run_command('whacka whacka whacka', config)
expect(result.exitstatus).to eq(1)
expect(result.error_message).to eq('error has happened')
}.to_not raise_error(Exception)

end
it 'should yield when passed a block' do
subject.stubs(:execute).returns('results')
subject.open_and_run_command('myquery', config) do |r|
expect(r).to eq('results')
end
end
end
context 'closed connection' do
before :each do
stub_connection
stub_no_errors
@connection.stubs(:State).returns(0)
end
it 'should not add MSSQLSERVER to connection string' do
@connection.stubs(:Open).with('Provider=SQLOLEDB.1;Persist Security Info=False;User ID=sa;password=Pupp3t1@;Initial Catalog=master;Application Name=Puppet;Data Source=localhost;')
subject.open_and_run_command('query', {'admin' => 'sa', 'pass' => 'Pupp3t1@', 'instance' => 'MSSQLSERVER'})
end
it 'should add a non default instance to connection string' do
@connection.stubs(:Open).with('Provider=SQLOLEDB.1;Persist Security Info=False;User ID=superuser;password=puppetTested;Initial Catalog=master;Application Name=Puppet;Data Source=localhost\LOGGING;')
subject.open_and_run_command('query', {'admin' => 'superuser', 'pass' => 'puppetTested', 'instance' => 'LOGGING'})
end
end
context 'open connection' do
it 'should not reopen an existing connection' do
stub_connection
@connection.expects(:open).never
@connection.stubs(:State).returns(1)
@connection.expects(:Execute).with('query', nil, nil)
subject.open_and_run_command('query', {'admin' => 'sa', 'pass' => 'Pupp3t1@', 'instance' => 'MSSQLSERVER'})
end
end
context 'return result with errors' do
it {
subject.expects(:open).with({'admin' => 'sa', 'pass' => 'Pupp3t1@', 'instance' => 'MSSQLSERVER'})
subject.expects(:command).with('SELECT * FROM sys.databases')
subject.expects(:close).once
subject.stubs(:has_errors).returns(:true)
subject.stubs(:error_message).returns(
'SQL Server
invalid syntax provider')
result =
subject.open_and_run_command('SELECT * FROM sys.databases',
{'instance' => 'MSSQLSERVER', 'admin' => 'sa', 'pass' => 'Pupp3t1@'})
expect(result.exitstatus).to eq(1)
expect(result.error_message).to eq('invalid syntax provider')
}
end
end
end
1 change: 0 additions & 1 deletion templates/create/login.sql.erb
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,3 @@ BEGIN
<% end -%>
END

Copy link
Contributor

Choose a reason for hiding this comment

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

Line ending noise


2 changes: 1 addition & 1 deletion templates/instance_config.erb
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{ 'instance': '<%= @instance_name %>','admin':'<%= @admin_user %>','pass':'<%= @admin_pass %>' }
{ "instance": "<%= @instance_name %>","admin":"<%= @admin_user %>","pass":"<%= @admin_pass %>" }
Copy link
Contributor

Choose a reason for hiding this comment

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

So we don't support integrated security at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be clear we are talking about passing the current users credentials from a command window, through ruby to an adodb. I don't see this as an optimal pattern. Accepting a windows login might be more likely however.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, integrated security is just telling SQL server to go get the Windows login context and use it. It's just "Integrated Security=SSPI" and part of the connection 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.

What would be the benefit of it? Does it cost anything to add it to the connection string?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not worry about the integrated security bit here, but let's file another ticket. I have a feeling there will be a few more touch points to support that (including at initial SQL install time) if we want to do it. There are some tradeoffs

Pros

  • No need to stash security credentials in a sidecar config file on disk anywhere (given it's not encrypted, I think this is a bad idea)
  • Simpler config

Cons

  • Additional installation config
  • If the end-user turns it off, the module stops working (I'm not too concerned here since this is user error)

Copy link
Contributor

Choose a reason for hiding this comment

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

Would totally love to see that file have some sort of encryption, even if we figure out dpapi. ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

@Iristyle the additional installation config is already there.