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

Sql Server (mssql) secret backend #998

Merged
merged 8 commits into from Mar 11, 2016
Merged

Conversation

chrishoffman
Copy link
Contributor

This request adds support for a MSSQL secret backend. It is modeled after the MySQL backend.
#374

@jefferai
Copy link
Member

jefferai commented Mar 2, 2016

@chrishoffman Could you sync this with master? At this point it wants to bring in a lot of Godeps stuff that no longer exists. In addition in the last month there have been some significant changes in the MySQL backend and it'd be good to bring these changes in. After that's done I'll be ready to review.

Thanks!

@chrishoffman
Copy link
Contributor Author

Sure thing.

@jefferai
Copy link
Member

jefferai commented Mar 2, 2016

Great!

@chrishoffman
Copy link
Contributor Author

Everything should be ready to go now. I had most of the changes in the mysql backend already and just added the verify_connection option to the connection config. I had some trouble with the Godeps stuff but I think that is squared away.

"max_open_connections": &framework.FieldSchema{
Type: framework.TypeInt,
Description: "Maximum number of open connections to database",
},
Copy link
Member

Choose a reason for hiding this comment

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

You mentioned about verify_connection in the comments but the changes are not here. Did you miss out on pushing the changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it looks like I missed this. I'll add it back.

@vishalnayak
Copy link
Member

@chrishoffman Thank you for this!
I have left a few comments.

@chrishoffman
Copy link
Contributor Author

The requested updates have been made and I tested the docs and they look good. This is ready again for review.

@@ -1,9 +1,6 @@
{
"ImportPath": "github.com/hashicorp/vault",
"GoVersion": "go1.6",
"Packages": [
Copy link
Member

Choose a reason for hiding this comment

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

These lines need to be here -- can you just manually add them back in?


func (b *backend) pathRoleDelete(
req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
err := req.Storage.Delete("role/" + data.Get("name").(string))
Copy link
Member

Choose a reason for hiding this comment

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

You should ensure that "name" isn't empty before calling Delete or you'll be issuing a delete on the higher level path.

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 could be more explicit here with requiring non-empty roles but the router will not allow any name to be passed here without at least 3 characters. Would you rather me be more explicit here?

Copy link
Member

Choose a reason for hiding this comment

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

Nope, missed the requirement in the regex. It's all good.

@chrishoffman
Copy link
Contributor Author

I made the requested modifications with the exception of the outstanding roles question. Let me know if the router regular expression is adequate for preventing empty name parameters.

@vishalnayak
Copy link
Member

@chrishoffman Thanks for acting on the comments quickly and for the PR in general!
Merging this.

vishalnayak added a commit that referenced this pull request Mar 11, 2016
Sql Server (mssql) secret backend
@vishalnayak vishalnayak merged commit 0b2477d into hashicorp:master Mar 11, 2016
@chrishoffman chrishoffman deleted the mssql branch March 11, 2016 15:48
rajanadar referenced this pull request in rajanadar/VaultSharp Aug 28, 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