Update HanaDb to v5 Database interface#10142
Conversation
tvoran
left a comment
tvoran
left a comment
There was a problem hiding this comment.
Sorry, this is getting caught in the changeover from newdbplugin --> dbplugin/v5.
The other thing I noticed is that updating a user's expiration isn't being tested, though I'm not sure how hard that would be to add.
| if err != nil { | ||
| return err | ||
| if req.Password != nil { | ||
| err = h.updateUserPassword(ctx, tx, req.Username, req.Password) |
There was a problem hiding this comment.
Changing passwords and expirations will share a transaction. Is that intentional? I'm okay with it, I just want to make sure everyone else is good with it.
There was a problem hiding this comment.
I did, in fact, do that intentionally. The way I see it, if a user attempts to do two different actions and one fails, knowing the same request can be used is helpful. I'm not sure how others feel, but that's my preference with most interactions with an API.
There was a problem hiding this comment.
+1, if I update two pieces of configuration in the same API call to Vault, I would find it least surprising as a user if they either both succeed or both fail.
There was a problem hiding this comment.
Awesome! That's the same logic I had in my head, I just wanted to make sure everyone was on the same page.
| func (h *HANA) RenewUser(ctx context.Context, statements dbplugin.Statements, username string, expiration time.Time) error { | ||
| statements = dbutil.StatementCompatibilityHelper(statements) | ||
| func (h *HANA) UpdateUser(ctx context.Context, req newdbplugin.UpdateUserRequest) (newdbplugin.UpdateUserResponse, error) { | ||
| h.Lock() |
There was a problem hiding this comment.
Asking because I have the same question for the Redshift DB migration: why do we need to hold a mutex lock inside the plugin? I can see that Vault needs to serialise password updates so that it knows which is the current password after concurrent rotate requests. However, the state is stored outside of this plugin, so the synchronisation would also need to be outside of the plugin i.e. around the state update in the caller. Am I missing another synchronisation requirement? cc @pcman312
There was a problem hiding this comment.
This was protecting against races when multiple calls are made to the engine at the same time. Specifically: rotate root credentials + any other call since rotate root was changing the plugin instance's configuration. I'm not 100% sure this is needed any longer, but I'd like to keep it for safety until we can confirm that it isn't needed.
Valarissa
force-pushed
the
update_hana_to_v5
branch
from
10f21a6 to
1868f09
Compare
| if err != nil { | ||
| t.Fatalf("err: %s", err) | ||
| db := new() | ||
| initResp := dbtesting.AssertInitialize(t, db, initReq) |
There was a problem hiding this comment.
nit: probably should include a defer dbtesting.AssertClose(t, db)
plugins/database/hana/hana_test.go
Outdated
| userResp := dbtesting.AssertNewUser(t, db, newReq) | ||
| assertCredsExist(t, connURL, userResp.Username, password) | ||
|
|
||
| test.req.Username = userResp.Username |
There was a problem hiding this comment.
Instead of manipulating the test case, what do you think about making the test case have a list of deletion statements rather than the full request object? That way you can construct the request here without having it be a part of the test case.
* Update HanaDb to v5 dbplugin * Add ability to update passwords for HANA db
) (hashicorp#10170) Co-authored-by: lane-wetmore <lane.wetmore@hashicorp.com>
4 participants
Updates the HANA Database plugin to adhere to the v5 Database interface.