Add redshift database plugin

[malnick]

Adds a database secrets engine for Redshift (postgres 8 compatible).

Because there are no local mocking methods available for Redshift, this plugin leverages a live Redshift cluster and enables test methods using the VAULT_ACC pattern. See comments in header to redshift_test.go for a full disclosure of caveats and setup instructions to run these acceptance tests.

Running these tests locally:

➜  vault/plugins/database/redshift malnick/redshift ✓ VAULT_ACC=1 REDSHIFT_URL=<redacted>:5439/<redacted> REDSHIFT_USER=<redacted> REDSHIFT_PASSWORD=<redacted> TEST_ROTATE_ROOT=1 go test -v
=== RUN   TestPostgreSQL_Initialize
--- PASS: TestPostgreSQL_Initialize (1.41s)
=== RUN   TestPostgreSQL_CreateUser
--- PASS: TestPostgreSQL_CreateUser (5.78s)
=== RUN   TestPostgreSQL_RenewUser
--- PASS: TestPostgreSQL_RenewUser (10.36s)
=== RUN   TestPostgreSQL_RevokeUser
--- PASS: TestPostgreSQL_RevokeUser (8.76s)
=== RUN   TestPostgresSQL_SetCredentials
--- PASS: TestPostgresSQL_SetCredentials (4.82s)
=== RUN   TestPostgreSQL_RotateRootCredentials
rotated root credentials, new user/pass:
username: <redacted>
password: <redacted>
--- PASS: TestPostgreSQL_RotateRootCredentials (1.29s)
PASS
ok      github.com/hashicorp/vault/plugins/database/redshift    33.138s

[calvn]

Looks like the var is misspelled, s/rotateStatents/rotateStatements/.

[malnick]

A transient artifact no less https://github.com/hashicorp/vault/blob/master/plugins/database/postgresql/postgresql.go#L452

I'll fix this and PR a fix in the original codebase too. Good catch, thanks!

[malnick]

#8306

[tyrannosaurus-becks]

Looks great so far! Thanks for working on this!

[tyrannosaurus-becks]

Suggested change

	_ = tx.Rollback()
	tx.Rollback()

Might as well just quietly drop the error, we do everywhere else. It would be super nice if we could log these, but alas, no logger!

[tyrannosaurus-becks]

Also, I have a question about these Rollback statements. I see Rollback is called this way in the mysql plugin. Is the idea basically that if we succeeded doing the transaction, then Rollback just won't work? I'm wondering if maybe we should add a guard that's like "success" that is false by default, and then it gets set to true if the whole transaction succeeded. Then inside the deferred function, it would be like "if !success, rollback" (pseudocode).

[tyrannosaurus-becks]

Might be nice to use go-multierror here to return more information about what did and didn't succeed. Commonly used in Vault.

[tyrannosaurus-becks]

I'm also wondering about an edge case here. I do believe that revocations get retried if they return an error. So, if we got part way through here, returned an error below, and then a minute later restarted the logic from the top, could we get all the way down? I'm not sure we need to do somersaults to make it work - I notice the other plugins use simplistic logic, just something to think about.

[malnick]

Do you have a good example of how this is implemented elsewhere? This plugin was largely inspired (and ripped off from) the PG secrets engine. If there's a simpler implementation you had in mind, would love to check it out.

[tyrannosaurus-becks]

Sure, I think this is a nice example of it: https://github.com/hashicorp/vault/blob/master/plugins/database/influxdb/influxdb.go#L192

[tyrannosaurus-becks]

You can also treat it like a regular error. So, for instance, in the example above, if right after the loop you wanted to do something like:

if result != nil {
    return
}
... do more stuff

It would totally work like normal.

[malnick]

Fixed

[malnick]

Thanks @tyrannosaurus-becks - I got to everything but the final question you had in here. I force pushed over the existing commit, here's the results of testing with the latest changes:

=== RUN   TestPostgreSQL_Initialize
--- PASS: TestPostgreSQL_Initialize (1.53s)
=== RUN   TestPostgreSQL_CreateUser
--- PASS: TestPostgreSQL_CreateUser (6.12s)
=== RUN   TestPostgreSQL_RenewUser
--- PASS: TestPostgreSQL_RenewUser (11.38s)
=== RUN   TestPostgreSQL_RevokeUser
--- PASS: TestPostgreSQL_RevokeUser (9.11s)
=== RUN   TestPostgresSQL_SetCredentials
--- PASS: TestPostgresSQL_SetCredentials (5.28s)
=== RUN   TestPostgreSQL_RotateRootCredentials
--- SKIP: TestPostgreSQL_RotateRootCredentials (0.00s)
PASS
ok      github.com/hashicorp/vault/plugins/database/redshift    33.818s```

[tyrannosaurus-becks]

Thanks for all your work on this so far!

Would you be willing to merge in master so we can also get a clean look at the build before merging? Also, I notice there are some test failures related to TestPredict_Plugins we need to resolve. Right here, we need to add "redshift-database-plugin".

[malnick]

Awesome, thanks @tyrannosaurus-becks - added the plugin to the plugin predict test file and rebased on master.

[malnick]

Thanks for the second pass @tyrannosaurus-becks !

I added the missing db.Close() statements and fixed the remaining feedback. Rebased on master just now, but it'll probably need to happen one more time soon because we're merging code quickly right now.

Latest test results with fixes and rebase:

=== RUN   TestPostgreSQL_Initialize
--- PASS: TestPostgreSQL_Initialize (1.46s)
=== RUN   TestPostgreSQL_CreateUser
--- PASS: TestPostgreSQL_CreateUser (7.50s)
=== RUN   TestPostgreSQL_RenewUser
--- PASS: TestPostgreSQL_RenewUser (11.83s)
=== RUN   TestPostgreSQL_RevokeUser
--- PASS: TestPostgreSQL_RevokeUser (10.41s)
=== RUN   TestPostgresSQL_SetCredentials
--- PASS: TestPostgresSQL_SetCredentials (5.29s)
=== RUN   TestPostgreSQL_RotateRootCredentials
--- SKIP: TestPostgreSQL_RotateRootCredentials (0.00s)
PASS
ok      github.com/hashicorp/vault/plugins/database/redshift    38.058s

[tyrannosaurus-becks]

Nice! Thank you!