[migration] Add support for loading credentials from file provided in command line

[brirams]

Summary

Fixes #421. This PR adds support for loading host, port, username, password, database, tls mode and tls certificate path from a --conf file argument to spirit. It expects the path to exist, as enforced by kong's existingfile type annotation and use go-ini to load credentials from the client section. Adds test to verify basic functionality.

Changes

• Migration now has a Conf field, which is populated by kong using the --conf argument. I chose a name that seemed natural to me but open to any better suggestions. I
• normalizeOptions loads the file using go-ini and checks for a client section and uses the mysql standard host, user, password, database and port names for these arguments . Using tls-* instead of MySQL's ssl-* for tls settings. Args for --host, --username, --password, --database, --tls-ca and --tls-mode take precedence over what's in file
• updated USAGE to explain this functionality

Testing

• new and existing unit tests pass
• built and run with expected results:

❯ ./spirit --help
Usage: spirit

Flags:
  -h, --help                                                           Show context-sensitive help.
      --host=STRING                                                    Hostname
      --username=STRING                                                User
      --password=PASSWORD                                              Password
      --database=STRING                                                Database
      --conf=STRING                                                    MySQL conf file
      --table=STRING                                                   Table
      --alter=STRING                                                   The alter statement to run on the table
      --threads=4                                                      Number of concurrent threads for copy and checksum tasks
      --target-chunk-time=500ms                                        The target copy time for each chunk
      --replica-dsn=STRING                                             A DSN for a replica which (if specified) will be used for lag checking.
      --replica-max-lag=120s                                           The maximum lag allowed on the replica before the migration throttles.
      --lock-wait-timeout=30s                                          The DDL lock_wait_timeout required for checksum and cutover
      --skip-drop-after-cutover                                        Keep old table after completing cutover
      --defer-cutover                                                  Defer cutover (and checksum) until sentinel table is dropped
      --force-kill                                                     Kill long-running transactions in order to acquire metadata lock (MDL) at checksum and cutover time
      --strict                                                         Exit on --alter mismatch when incomplete migration is detected
      --statement=""                                                   The SQL statement to run (replaces --table and --alter)
      --tls-mode=STRING                                                TLS connection mode (case insensitive): DISABLED, PREFERRED (default), REQUIRED, VERIFY_CA, VERIFY_IDENTITY
      --tls-ca=STRING                                                  Path to custom TLS CA certificate file
      --enable-experimental-buffered-copy                              Use the experimental buffered copier/repl applier based on the DBLog algorithm
      --enable-experimental-linting                                    Enable experimental linting checks before running migration
      --enable-experimental-linters=ENABLE-EXPERIMENTAL-LINTERS,...    Experimental linters to enable (default "all")
      --experimental-linter-config=EXPERIMENTAL-LINTER-CONFIG,...      Configuration options for experimental linters in the form linter_name.key=value
      --experimental-lint-only                                         Exit after executing linters
❯ cat my.cnf
[client]
user=some-user
password=super-secret
database=bramos_test
host=localhost
port=3306
❯ ./spirit --conf my.cnf --table testing --alter  'DROP  KEY idx_name_unique'
INFO[0000] Starting spirit migration: concurrency=4 target-chunk-size=500ms table='bramos_test.testing' alter=DROP  KEY idx_name_unique
INFO[0000] attempting to acquire metadata lock
INFO[0000] acquired metadata lock: bramos_test.testing-46cd71b3
INFO[0000] apply complete: instant-ddl=false inplace-ddl=true
INFO[0000] releasing metadata lock: bramos_test.testing-46cd71b3
❯ ./spirit --host localhost:3306 --database test --conf /var/log/nonexistent --table test_table --alter "ADD COLUMN other_json_column_again_5 varchar(100)"
spirit: error: --conf: stat /var/log/nonexistent: no such file or directory

[morgo]

Thank you for working on this. I see from the tests that this parses a MySQL-like ini file (rather than kong file etc). I think that's a great level of integration - it wasn't what I had intended but I think works fine.

But I have the following suggestions:

• Parse [client] only (I think you are doing this), but document as such and give an example.
• It's not just username + password, but we should also consider reading out host + database name. Thus the only arguments required to spirit will be either a --table + --alter or a --statement.
• In terms of precedence, this might be tricky because I'm not sure I've tried to differentiate between a default kong has sent me, or a value. Ideally we can differentiate and then arguments specified take precedence if both exist. This would also make it possible to auto-parse ~/.my.cnf if it exists.

[brirams]

I see from the tests that this parses a MySQL-like ini file (rather than kong file etc). I think that's a great level of integration - it wasn't what I had intended but I think works fine.

ha! i completely missed that kong had the ability to parse an arbitrary config file but yeah -- keeping in line with MySQL ini files seems like a good thing to do for consistency with other tooling in the ecosystem

Parse [client] only (I think you are doing this), but document as such and give an example.

yup -- we're only parsing the client section for these params

It's not just username + password, but we should also consider reading out host + database name. Thus the only arguments required to spirit will be either a --table + --alter or a --statement.

host and database make sense: would it be reasonable, then, to also parse port from the config file, as it seems standard to have it defined in its own field in things like the MySQL CLI. I'd assume yes but want to confirm before going too far upfield

In terms of precedence, this might be tricky because I'm not sure I've tried to differentiate between a default kong has sent me, or a value. Ideally we can differentiate and then arguments specified take precedence if both exist. This would also make it possible to auto-parse ~/.my.cnf if it exists.

so the easy answer here is to make Host, Username, Password and Database be string pointers in the Migration struct and apply defaults for these fields in code instead of relying on kong's default annotation. The downside here is that, without the annotation, you lose the pretty printing of those values in the help output:

current behavior

❯ spirit -h
Usage: spirit

Flags:
  -h, --help                       Show context-sensitive help.
      --host="127.0.0.1:3306"      Hostname
      --username="msandbox"        User
      --password="msandbox"        Password
      --database="test"            Database

without the default annotation

❯ ./spirit -h
Usage: spirit

Flags:
  -h, --help                       Show context-sensitive help.
      --host=HOST                  Hostname
      --username=USERNAME          User
      --password=PASSWORD          Password
      --database=DATABASE          Database

I could also just shove the default in the help text but that also begs the question: do ya'll care for defaults for these four fields? we could just remove them for these fields and fail when they're not provided in the command line or conf file. I believe that's what part of the code actually checks for here and here but these were never true since the defaults injected by kong would also be non-empty.

let me know what you think and i can take another crack 🙇🏽

[morgo]

host and database make sense: would it be reasonable, then, to also parse port from the config file, as it seems standard to have it defined in its own field in things like the MySQL CLI. I'd assume yes but want to confirm before going too far upfield

Yes that's fine.

I could also just shove the default in the help text but that also begs the question: do ya'll care for defaults for these four fields? we could just remove them for these fields and fail when they're not provided in the command line or conf file.

Yes, I agree that's the tradeoff here. The most straight forward way to implement this is to remove kong defaults.

We can later parse and substitute in defaults, but as you said it means they won't appear in help output. But I think that's fine.

[brirams]

took another crack with some changes:

• renamed arg and references --conf to expand its use beyond just creds
• have CLI args for host, username, password and database take precedence over what's provided in conf file
• fallback to existing defaults if none of the above are provided
• update USAGE.md saying as such, including an example of a valid client section

Most of the updates are to the change to pointer types for some of these fields. if you prefer a cleaner PR, i can try and figure out another approach

[brirams]

this would have never been true as a normal course of operation since kong would inject a default at runtime

[morgo]

This covers a use case we have internally. We use spirit as a library/package and don't rely on kong to inject values.

I think it's fine to remove though, we can find another way to handle this.

[brirams]

same goes here: kong would inject the default of test.

[morgo]

I'm sure there's a reason, but can you explain the *string change? Is it to differentiate between not set, and set to empty?

If we can avoid it, that would be ideal of course.

[brirams]

ah -- so this was a carry over of trying to keep the default values within kong's purview which ended up not working out and then eventually convincing myself that it was cleaner to represent a value not being passed in as a nil pointer than an empty string. Happy to keep these as strings to make the PR smaller. lmk!

[morgo]

Yes, I think it's better as strings. If you can make this change back, I think this PR is much smaller/we don't have to consider the side effects here from *string.

[brirams]

question: do you still want to support passing the literal string "" as a password on the CLI and file? I could see that being a legitimate use-case for password test databases. if so, that would necessitate switching just that field to be a pointer since we'd need to make the distinction between "" and not being provided at all.

[morgo]

Good point. Yes, I guess we need to support passing an empty string for the password.

[brirams]

cool -- i'll update to support that

[morgo]

LGTM on first glance - I'm OOO tomorrow, but can take a look next week.

If you get a chance to add support for the new TLS options from #443 , I think this further supports the use case of why config files are handy.

[brirams]

If you get a chance to add support for the new TLS options from #443 , I think this further supports the use case of why config files are handy.

i'll take a look!

[brirams]

an interesting thing to note: the parameters for tls-ca and tls-mode are ssl-ca and ssl-mode in mysql8 so this won't be fully compatible with conf files used by plain vanilla mysql clients

[morgo]

an interesting thing to note: the parameters for tls-ca and tls-mode are ssl-ca and ssl-mode in mysql8 so this won't be fully compatible with conf files used by plain vanilla mysql clients

I think this is fine for now. We could accept the MySQL names in a mysql config file if we decide to. We named these options tls-* intentionally because the MySQL names are not technically correct.

[morgo]

Just a few linter errors if you don't mind fixing. Otherwise LGTM:

 Error: pkg/migration/migration.go:184:1: File is not properly formatted (gofmt)
  	password *string
  ^
  Error: pkg/migration/migration_test.go:631:1: File is not properly formatted (gofmt)
  		Host:     "cli-host:3306",
  ^
  Error: pkg/migration/migration_test.go:663:2: empty: use assert.Empty (testifylint)
  	assert.Equal(t, "", *migration.Password)
  	^
  Error: pkg/migration/migration_test.go:855:2: empty: use assert.Empty (testifylint)
  	assert.Equal(t, "", *migration.Password)

[morgo]

LGTM, thank you again for the contribution.