Skip to content
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

(PE-16132) Add Windows Based Authentication for sqlserver::config #174

Merged
merged 1 commit into from Jun 15, 2016
Merged

(PE-16132) Add Windows Based Authentication for sqlserver::config #174

merged 1 commit into from Jun 15, 2016

Conversation

glennsarti
Copy link
Contributor

@glennsarti glennsarti commented Jun 9, 2016

Previously to use classes such as sqlserver::login, it would use the
sqlserver::config resource which would contain the credentials to conect to the
database. However this resource could only authenticate using SQL Server based
authenctiation. This means that databases that only used Windows based
authentication could not be managed.

This commit modifies the sqlserver::config class with an additional property
called login_type which can be either SQL_LOGIN or WINDOWS_LOGIN, with a default
of SQL_LOGIN. The login type also determines the validity of the admin_user and
admin_pass properties.

The config resource properties are consumed by the get_connection_string method
in sql_connection. The login_type property then determines whether
'Integrated Security=SSPI' is appended the connection string for connecting to
the database.

Additionally the spec and define tests are modified for the new properties and
default values.

@jpogran
Copy link
Contributor

jpogran commented Jun 14, 2016

Verified that this worked by installing a new single SQL server installation, and ran the following code:

sqlserver::database{ 'minviable':
    instance => $sql_instance_name,
}
sqlserver::config{$sql_instance_name:
  login_type => 'WINDOWS_LOGIN'
}
sqlserver::login{ 'win2012r2\vagrant':
  instance   => $sql_instance_name,
  login_type => 'WINDOWS_LOGIN',
  svrroles   => { 'sysadmin' => 1 },
}

This executed without an error code. I then verified the user account was given sysadmin access.

@jpogran
Copy link
Contributor

jpogran commented Jun 14, 2016

👍

admin_user = config[:admin_user] || ''
admin_pass = config[:admin_pass] || ''

if (config[:login_type] == 'WINDOWS_LOGIN')
Copy link
Contributor

@Iristyle Iristyle Jun 14, 2016

Choose a reason for hiding this comment

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

Shouldn't WINDOWS_LOGIN be mentioned in the README?

Ohh, weird - it is already? But it just wasn't effective?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, that is a good point. Initially I did not include that in sqlserver::config and didn't connect that I needed that there

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, so it looks like login_type is mentioned for sqlserver::login (which is a different thing), but not for sqlserver::config

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 appears I forgot to add that to the README.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you feel that it's confusing to have the same parameter name, perhaps I should use admin_login_type in the sql::config class?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not confused, but I think users might be now that you mention it. The intent is different between the two, mostly because login uses it as a property... whereas for config its more used as a parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
Renamed the login_type property to admin_login_type
Updated the readme.

Previously to use classes such as sqlserver::login, it would use the
sqlserver::config resource which would contain the credentials to conect to the
database.  However this resource could only authenticate using SQL Server based
authenctiation.  This means that databases that only used Windows based
authentication could not be managed.

This commit modifies the sqlserver::config class with an additional property
called login_type which can be either SQL_LOGIN or WINDOWS_LOGIN, with a default
 of SQL_LOGIN.  The login type also determines the validity of the admin_user and
 admin_pass properties.

The config resource properties are consumed by the get_connection_string method
in sql_connection.  The login_type property then determines whether
'Integrated Security=SSPI' is appended the connection string for connecting to
the database.

Additional the spec, define and acceptance tests are modified for the new
properties and default values.
@jpogran jpogran merged commit aeae83e into puppetlabs:master Jun 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants