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

secret/database: ensure plugins are closed if they cannot be initialized #3768

Merged
merged 1 commit into from
Jan 9, 2018
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
13 changes: 13 additions & 0 deletions builtin/logical/database/dbplugin/client.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package dbplugin

import (
"context"
"errors"
"sync"

Expand All @@ -18,13 +19,25 @@ type DatabasePluginClient struct {
Database
}

// This wraps the Close call and ensures we both close the database connection
// and kill the plugin.
func (dc *DatabasePluginClient) Close() error {
err := dc.Database.Close()
dc.client.Kill()

return err
}

// This wraps the Initialize call and ensures we close the plugin on error.
func (dc *DatabasePluginClient) Initialize(ctx context.Context, config map[string]interface{}, verifyConnection bool) error {
err := dc.Database.Initialize(ctx, config, verifyConnection)
if err != nil {
dc.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

The database object in here might be nil if verify_connection was set to false (thus never calling Connection() to instantiate the object). dc.Close() -> dc.Database.Close() would be called on a nil db if the Initialize error comes from something else (e.g. https://github.com/hashicorp/vault/blob/master/plugins/helper/database/connutil/sql.go#L58).

Copy link
Member

Choose a reason for hiding this comment

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

Isn't that something that should be handled by the Close() implementation in the database though?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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.

Ah, missed that. It's all good here!

}

return err
}

// newPluginClient returns a databaseRPCClient with a connection to a running
// plugin. The client is wrapped in a DatabasePluginClient object to ensure the
// plugin is killed on call of Close().
Expand Down
1 change: 0 additions & 1 deletion builtin/logical/database/path_config_connection.go
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,6 @@ func (b *databaseBackend) connectionWriteHandler() framework.OperationFunc {

err = db.Initialize(ctx, config.ConnectionDetails, verifyConnection)
if err != nil {
db.Close()
return logical.ErrorResponse(fmt.Sprintf("error creating database object: %s", err)), nil
}

Expand Down