add mssql physical backend #2546

sebastianhabaing · 2017-03-30T11:01:48Z

No description provided.

chrishoffman

Thanks for the contribution! Overall this look pretty good. There are just a few things relating to required arguments and handling of schemas.

Also, can you please add documentation for configuring this plugin. You can add the documentation here website/source/docs/configuration/storage

chrishoffman · 2017-03-30T23:44:11Z

physical/mssql.go

+		return nil, fmt.Errorf("failed to create mssql database: %v", err)
+	}
+
+	dbTable := database + ".." + table


This is assuming the default schema, presumably dbo. Can you please add a configuration option for the schema with a default value of dbo?

Ok, I add schema option.

This statement will no longer be necessary if you remove the conditional schema logic.

chrishoffman · 2017-03-30T23:49:47Z

physical/mssql.go

+	connectionString := "server=" + server + ";app name" + appname + ";connection timeout=" + connectionTimeout + ";log=" + logLevel
+	if username != "" {
+		connectionString += ";user id=" + username
+	}


I can't think of case where there is no username or password. I think it should fail if this is not provided.

Without username and password, driver is using SSO.
From driver documentation:

"user id" - enter the SQL Server Authentication user id or the Windows Authentication user id in the DOMAIN\User format. On Windows, if user id is empty or missing Single-Sign-On is used.

I think the most likely usage would be providing a username and password to this backend. Are you expecting to use the Single Sign-On features of the driver? I would be curious to see how this works in practice.

You can provide username and password. SSO is only option of mssql go driver so I added it as a feature to this provider.
Yes, In our environment we use sso. For example when I am logged to my workstation I can connect to mssql database on different machine. (It's works same as Windows Auth in this example)

After some further internal discussions, we believe this is fine. Even though SSO makes a lot of sense in a user workstation context it could also work in this context. This is especially true since the same person configuring the Vault installation will also be in charge of the physical backend configuration.

chrishoffman · 2017-03-30T23:50:06Z

physical/mssql.go

+	}
+
+	if password != "" {
+		connectionString += ";password=" + password


See above comment.

See above reply ;)

chrishoffman · 2017-03-31T00:05:11Z

physical/mssql.go

+		return nil, fmt.Errorf("failed to connect to mssql: %v", err)
+	}
+
+	if _, err := db.Exec("IF NOT EXISTS(SELECT * FROM sys.databases WHERE name = '" + database + "') CREATE DATABASE " + database); err != nil {


I think this should work without specifying the database. To be more explicit, it may make sense to add the master database to this query.

I you don't include database name, default name 'Vault' will be used. It's working same as mysql and postgres provider.

I thought that sys.databases was a special table that resided in master but I don't actually think that is the case, so I think this is fine.

chrishoffman · 2017-03-31T00:15:32Z

physical/mssql.go

+	}
+
+	dbTable := database + ".." + table
+	createQuery := "IF NOT EXISTS(SELECT 1 FROM " + database + ".INFORMATION_SCHEMA.TABLES WHERE TABLE_TYPE='BASE TABLE' AND TABLE_NAME='" + table + "') CREATE TABLE " + dbTable +


Based on adding schema above, TABLE_SCHEMA will need to be added to the criteria for this query.

This statement will no longer be necessary if you remove the conditional schema logic.

chrishoffman

Thanks for adding the docs. A few minor edits to the docs and a couple of changes to handling schema.

chrishoffman · 2017-04-05T14:23:46Z

physical/mssql.go

+		schema = "dbo"
+	}
+
+	connectionString := "server=" + server + ";app name" + appname + ";connection timeout=" + connectionTimeout + ";log=" + logLevel


There is a missing = after app name. Would you mind converting this to a fmt.Sprintf which may make it easier to spot these issues?

ok changed.

chrishoffman · 2017-04-05T14:27:09Z

physical/mssql.go

+	createQuery := "IF NOT EXISTS(SELECT 1 FROM " + database + ".INFORMATION_SCHEMA.TABLES WHERE TABLE_TYPE='BASE TABLE' AND TABLE_NAME='" + table + "') CREATE TABLE " + dbTable +
+		" (Path VARCHAR(512) PRIMARY KEY, Value VARBINARY(MAX))"
+
+	if schema != "dbo" {


I'm not sure this guard is necessary. It is likely the dbo schema is going to be available but it probably still makes sense to do the same check for dbo.

I must check if user want different schema than dbo and create it when it does not exists. You can't drop schema dbo and it always exists. MSDN

Thanks for the docs. I thought dbo was just a default schema and could itself be dropped.

chrishoffman · 2017-04-05T14:46:54Z