Migrate elasticsearch plugin to v5 database plugin interface #19
Conversation
In DeleteUser, always try deleting the role, since the role name == user name, and we don't get the creation_statement in DeleteUser anymore.
delete the user before the role it uses
Added local_dev.sh and run_acceptance.sh scripts.
elasticsearch.go
Outdated
| if err := client.ChangePassword(ctx, req.Username, req.Password.NewPassword); err != nil { | ||
| return newdbplugin.UpdateUserResponse{}, fmt.Errorf("unable to change password: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: can you add a comment explicitly saying the expiration change is a no-op?
| return nil, dbutil.ErrEmptyCreationStatement | ||
| } | ||
| stmt := &creationStatement{} | ||
| if err := json.Unmarshal([]byte(statements.Creation[0]), stmt); err != nil { | ||
| return nil, errwrap.Wrapf(fmt.Sprintf("unable to unmarshal %s: {{err}}", []byte(statements.Creation[0])), err) | ||
| if err := json.Unmarshal([]byte(statements.Commands[0]), stmt); err != nil { |
There was a problem hiding this comment.
If I'm reading this correctly, this function will ignore any commands that aren't in the first index. If so, can you put an error if the length of the commands is >1 so the user doesn't get confused?
elasticsearch_test.go
Outdated
| if len(configToStore) != len(config) { | ||
| t.Fatalf("expected %s, received %s", config, configToStore) | ||
| resp := dbtesting.AssertInitialize(t, e.Elasticsearch, req) | ||
| if len(resp.Config) != len(req.Config) { |
There was a problem hiding this comment.
Since this isn't going through a translation layer (like gRPC), the req.Config could be manipulated in place, making these assertions useless. I recommend making an expected response object with a copy of the request config then comparing the two.
| } | ||
| dbtesting.AssertUpdateUser(t, e.Elasticsearch, req) |
There was a problem hiding this comment.
Also assert that you can log in with the new credentials?
There was a problem hiding this comment.
That is already being done in the integration tests:
vault-plugin-database-elasticsearch/integration_test.go
Lines 152 to 170 in 9cde79b
Seems like that's a better spot since these tests are using a mock elasticsearch.
scripts/local_dev.sh
Outdated
|
|
||
| echo "--> Vault server" | ||
| echo " Writing config" | ||
| tee "$SCRATCH/vault.hcl" > /dev/null <<EOF |
There was a problem hiding this comment.
nit: Why not use -dev-plugin-dir?
There was a problem hiding this comment.
Good idea, changed in 507cc53
| -dev-ha -dev-transactional -dev-root-token-id=root \ | ||
| & | ||
| sleep 2 | ||
| VAULT_PID=$! |
There was a problem hiding this comment.
Shouldn't this be one line up? Or will it ignore a PID from sleep 2?
There was a problem hiding this comment.
Yeah, $! is the pid of the last background process, so it ignores the sleep 2. Doesn't hurt to move it up though.
| @@ -13,7 +13,7 @@ func main() { | |||
| flags := apiClientMeta.FlagSet() | |||
| flags.Parse(os.Args[1:]) | |||
|
|
|||
| if err := elasticsearch.Run(apiClientMeta.GetTLSConfig()); err != nil { | |||
| if err := elasticsearch.Run(); err != nil { | |||
There was a problem hiding this comment.
Since we're punting on AutoMTLS for the time being, this line should be reverted
Co-authored-by: Michael Golowka <[email protected]>
Overview
Migrating this plugin to the new database plugin interface.
Design of Change
Then changing the functions to satisfy the newdbplugin Database interface.
Also adds testing docs and scripts.
Related Issues/Pull Requests
Contributor Checklist
My Docs PR Link
Example
Unit and integration tests
Acceptance tests with real elasticsearch