DBPW - Migrate MySQL db to v5 database engine #10110

Valarissa · 2020-10-08T04:28:57Z

Overview

Converts plugin to use the Database v5 interface introduced in #9641

Design of Change

Converted each of the functions over to their equivalent in the new interface.
Init -> Initialize
CreateUser -> NewUser
RenewUser, SetCredentials, & RotateRootCredentials -> UpdateUser
RevokeUser -> DeleteUser

Test Output

``` === RUN TestMySQL_Initialize --- PASS: TestMySQL_Initialize (13.38s) === RUN TestMySQL_CreateUser --- PASS: TestMySQL_CreateUser (28.95s) === RUN TestMySQL_CreateUser/missing_creation_statements --- PASS: TestMySQL_CreateUser/missing_creation_statements (0.00s) === RUN TestMySQL_CreateUser/non-legacy --- PASS: TestMySQL_CreateUser/non-legacy (12.39s) === RUN TestMySQL_CreateUser/non-legacy/prepared_statement_name --- PASS: TestMySQL_CreateUser/non-legacy/prepared_statement_name (0.02s) === RUN TestMySQL_CreateUser/non-legacy/prepared_statement_username --- PASS: TestMySQL_CreateUser/non-legacy/prepared_statement_username (0.02s) === RUN TestMySQL_CreateUser/non-legacy/create_name --- PASS: TestMySQL_CreateUser/non-legacy/create_name (0.02s) === RUN TestMySQL_CreateUser/non-legacy/create_username --- PASS: TestMySQL_CreateUser/non-legacy/create_username (0.02s) === RUN TestMySQL_CreateUser/legacy --- PASS: TestMySQL_CreateUser/legacy (16.56s) === RUN TestMySQL_CreateUser/legacy/prepared_statement_name --- PASS: TestMySQL_CreateUser/legacy/prepared_statement_name (0.02s) === RUN TestMySQL_CreateUser/legacy/prepared_statement_username --- PASS: TestMySQL_CreateUser/legacy/prepared_statement_username (0.02s) === RUN TestMySQL_CreateUser/legacy/create_name --- PASS: TestMySQL_CreateUser/legacy/create_name (0.02s) === RUN TestMySQL_CreateUser/legacy/create_username --- PASS: TestMySQL_CreateUser/legacy/create_username (0.02s) === RUN TestMySQL_RotateRootCredentials --- PASS: TestMySQL_RotateRootCredentials (37.78s) === RUN TestMySQL_RotateRootCredentials/empty_statements --- PASS: TestMySQL_RotateRootCredentials/empty_statements (12.21s) === RUN TestMySQL_RotateRootCredentials/default_username --- PASS: TestMySQL_RotateRootCredentials/default_username (13.46s) === RUN TestMySQL_RotateRootCredentials/default_name --- PASS: TestMySQL_RotateRootCredentials/default_name (12.10s) === RUN TestMySQL_DeleteUser --- PASS: TestMySQL_DeleteUser (11.97s) === RUN TestMySQL_DeleteUser/empty_statements --- PASS: TestMySQL_DeleteUser/empty_statements (0.02s) === RUN TestMySQL_DeleteUser/default_name --- PASS: TestMySQL_DeleteUser/default_name (0.02s) === RUN TestMySQL_DeleteUser/default_username --- PASS: TestMySQL_DeleteUser/default_username (0.02s) === RUN TestMySQL_UpdateUser --- PASS: TestMySQL_UpdateUser (39.02s) === RUN TestMySQL_UpdateUser/custom_statement_username --- PASS: TestMySQL_UpdateUser/custom_statement_username (12.35s) === RUN TestMySQL_UpdateUser/empty_statements --- PASS: TestMySQL_UpdateUser/empty_statements (13.81s) === RUN TestMySQL_UpdateUser/custom_statement_name --- PASS: TestMySQL_UpdateUser/custom_statement_name (12.86s) === RUN TestMySQL_Initialize_ReservedChars --- PASS: TestMySQL_Initialize_ReservedChars (13.83s) PASS

Process finished with exit code 0

</details>

# Contributor Checklist
- [x] Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
  - Upcoming PR for the conversion to the new Database PR
- [x] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
- [ ] Backwards compatible
  - Not backwards compatible with old versions of Vault
- [x] https://github.com/hashicorp/vault/pull/10050 Merged & updated vendoring in this repo

pcman312 · 2020-10-08T17:05:04Z

plugins/database/mysql/mysql.go

-	"time"
-
-	stdmysql "github.com/go-sql-driver/mysql"
+	"fmt"


I recommend running goimports on this

pcman312 · 2020-10-08T19:19:32Z

plugins/database/mysql/mysql.go

 	}

+	password := req.Password
+
+	expirationStr := req.Expiration.String()


This needs to be:

Suggested change

expirationStr := req.Expiration.String()

expirationStr := req.Expiration.Format("2006-01-02 15:04:05-0700")

otherwise the formatting will be dramatically different.

pcman312 · 2020-10-08T19:23:21Z

plugins/database/mysql/mysql.go

+	var dispName, roleName, maxLen credsutil.UsernameOpt
+
+	if m.legacy {
+		dispName = credsutil.DisplayName(req.UsernameConfig.DisplayName, LegacyUsernameLen)
+		roleName = credsutil.RoleName(req.UsernameConfig.RoleName, LegacyMetadataLen)
+		maxLen = credsutil.MaxLength(LegacyUsernameLen)
+	} else {
+		dispName = credsutil.DisplayName(req.UsernameConfig.DisplayName, UsernameLen)
+		roleName = credsutil.RoleName(req.UsernameConfig.RoleName, MetadataLen)
+		maxLen = credsutil.MaxLength(UsernameLen)
+	}
+
+	username, err := credsutil.GenerateUsername(dispName, roleName, maxLen)


nit: this could be simpler:

Suggested change

var dispName, roleName, maxLen credsutil.UsernameOpt

if m.legacy {

dispName = credsutil.DisplayName(req.UsernameConfig.DisplayName, LegacyUsernameLen)

roleName = credsutil.RoleName(req.UsernameConfig.RoleName, LegacyMetadataLen)

maxLen = credsutil.MaxLength(LegacyUsernameLen)

} else {

dispName = credsutil.DisplayName(req.UsernameConfig.DisplayName, UsernameLen)

roleName = credsutil.RoleName(req.UsernameConfig.RoleName, MetadataLen)

maxLen = credsutil.MaxLength(UsernameLen)

}

username, err := credsutil.GenerateUsername(dispName, roleName, maxLen)

var dispNameLen, roleNameLen, maxLen int

if m.legacy {

dispName = LegacyUsernameLen

roleName = LegacyMetadataLen

maxLen = LegacyUsernameLen

} else {

dispName = UsernameLen

roleName = MetadataLen

maxLen = UsernameLen

}

username, err := credsutil.GenerateUsername(

credsutil.DisplayName(req.UsernameConfig.DisplayName, dispNameLen),

credsutil.RoleName(req.UsernameConfig.RoleName),

credsutil.MaxLength(maxLen),

)

That's fair, I wasn't sure which would be the better approach. Your suggestion isolates the differences more succinctly.

pcman312 · 2020-10-08T19:29:43Z

plugins/database/mysql/mysql.go

-	if err := tx.Commit(); err != nil {
-		return nil, err
-	}
+	return newdbplugin.UpdateUserResponse{}, nil


Can you put a comment saying that expiration changes are a no-op?

pcman312 · 2020-10-08T19:32:18Z

plugins/database/mysql/mysql_test.go

+	}
+
+	db = new(false)
+	_, err = db.Initialize(context.Background(), initReq)


If you want, you can use the sdk/database/newdbplugin/testing/ package for some of these. It has assertion functions for each of the DB functions such as AssertInitialize which takes a *testing.T object, a DB instance, and a request and does all of the boilerplate around running it (creating a context and asserting that the function did not error)

I'll keep that in mind for the next DB I do and utilize it then. If we come back to it, I'll redo this one. Thanks for the tip though!

Migrate MySQL db to v5 database engine

c6c7c46

Valarissa requested a review from pcman312 October 8, 2020 04:28

pcman312 reviewed Oct 8, 2020

View reviewed changes

Fix some nits and fix up imports

85284d3

Valarissa requested a review from pcman312 October 8, 2020 21:49

pcman312 approved these changes Oct 9, 2020

View reviewed changes

Merge branch 'master' into update_mysql_to_db_v5

532a012

Valarissa merged commit ae27bf5 into master Oct 9, 2020

pcman312 added the secret/database label Oct 19, 2020

pcman312 changed the title ~~Migrate MySQL db to v5 database engine~~ DBPW - Migrate MySQL db to v5 database engine Oct 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DBPW - Migrate MySQL db to v5 database engine #10110

DBPW - Migrate MySQL db to v5 database engine #10110

Valarissa commented Oct 8, 2020

pcman312 Oct 8, 2020

pcman312 Oct 8, 2020

pcman312 Oct 8, 2020

Valarissa Oct 8, 2020

pcman312 Oct 8, 2020

pcman312 Oct 8, 2020

Valarissa Oct 8, 2020

	expirationStr := req.Expiration.String()
	expirationStr := req.Expiration.Format("2006-01-02 15:04:05-0700")

DBPW - Migrate MySQL db to v5 database engine #10110

DBPW - Migrate MySQL db to v5 database engine #10110

Conversation

Valarissa commented Oct 8, 2020

Overview

Design of Change

Test Output

pcman312 Oct 8, 2020

Choose a reason for hiding this comment

pcman312 Oct 8, 2020

Choose a reason for hiding this comment

pcman312 Oct 8, 2020

Choose a reason for hiding this comment

Valarissa Oct 8, 2020

Choose a reason for hiding this comment

pcman312 Oct 8, 2020

Choose a reason for hiding this comment

pcman312 Oct 8, 2020

Choose a reason for hiding this comment

Valarissa Oct 8, 2020

Choose a reason for hiding this comment