From 5f587c3262259c4d6ca5a17e955fc96f8f03e0d5 Mon Sep 17 00:00:00 2001 From: Shlomi Noach Date: Mon, 14 Jan 2019 13:27:44 +0200 Subject: [PATCH 1/4] Adding --force-named-panic option --- go/base/context.go | 1 + go/cmd/gh-ost/main.go | 1 + go/logic/server.go | 9 +++++++++ 3 files changed, 11 insertions(+) diff --git a/go/base/context.go b/go/base/context.go index 0043d07..318fdf9 100644 --- a/go/base/context.go +++ b/go/base/context.go @@ -123,6 +123,7 @@ type MigrationContext struct { CutOverExponentialBackoff bool ExponentialBackoffMaxInterval int64 ForceNamedCutOverCommand bool + ForceNamedPanicCommand bool PanicFlagFile string HooksPath string HooksHintMessage string diff --git a/go/cmd/gh-ost/main.go b/go/cmd/gh-ost/main.go index 3024702..a745c21 100644 --- a/go/cmd/gh-ost/main.go +++ b/go/cmd/gh-ost/main.go @@ -81,6 +81,7 @@ func main() { flag.BoolVar(&migrationContext.TimestampOldTable, "timestamp-old-table", false, "Use a timestamp in old table name. This makes old table names unique and non conflicting cross migrations") cutOver := flag.String("cut-over", "atomic", "choose cut-over type (default|atomic, two-step)") flag.BoolVar(&migrationContext.ForceNamedCutOverCommand, "force-named-cut-over", false, "When true, the 'unpostpone|cut-over' interactive command must name the migrated table") + flag.BoolVar(&migrationContext.ForceNamedPanicCommand, "force-named-panic", false, "When true, the 'panic' interactive command must name the migrated table") flag.BoolVar(&migrationContext.SwitchToRowBinlogFormat, "switch-to-rbr", false, "let this tool automatically switch binary log format to 'ROW' on the replica, if needed. The format will NOT be switched back. I'm too scared to do that, and wish to protect you if you happen to execute another migration while this one is running") flag.BoolVar(&migrationContext.AssumeRBR, "assume-rbr", false, "set to 'true' when you know for certain your server uses 'ROW' binlog_format. gh-ost is unable to tell, event after reading binlog_format, whether the replication process does indeed use 'ROW', and restarts replication to be certain RBR setting is applied. Such operation requires SUPER privileges which you might not have. Setting this flag avoids restarting replication and you can proceed to use gh-ost without SUPER privileges") diff --git a/go/logic/server.go b/go/logic/server.go index 919f178..831fdfa 100644 --- a/go/logic/server.go +++ b/go/logic/server.go @@ -322,6 +322,15 @@ help # This message } case "panic": { + if arg == "" && this.migrationContext.ForceNamedPanicCommand { + err := fmt.Errorf("User commanded 'panic' without specifying table name, but --force-named-panic is set") + return NoPrintStatusRule, err + } + if arg != "" && arg != this.migrationContext.OriginalTableName { + // User explicitly provided table name. This is a courtesy protection mechanism + err := fmt.Errorf("User commanded 'panic' on %s, but migrated table is %s; ignoring request.", arg, this.migrationContext.OriginalTableName) + return NoPrintStatusRule, err + } err := fmt.Errorf("User commanded 'panic'. I will now panic, without cleanup. PANIC!") this.migrationContext.PanicAbort <- err return NoPrintStatusRule, err From 6c5805d844935776a0ebdb4260b91f598f878c81 Mon Sep 17 00:00:00 2001 From: Shlomi Noach Date: Mon, 14 Jan 2019 13:32:43 +0200 Subject: [PATCH 2/4] documenting new flag --- doc/command-line-flags.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/doc/command-line-flags.md b/doc/command-line-flags.md index 804ba58..3d96ce4 100644 --- a/doc/command-line-flags.md +++ b/doc/command-line-flags.md @@ -111,6 +111,14 @@ While the ongoing estimated number of rows is still heuristic, it's almost exact Without this parameter, migration is a _noop_: testing table creation and validity of migration, but not touching data. +### force-named-cut-over + +If given, a `cut-over` command must name the migrated table, or else ignored. + +### force-named-panic + +If given, a `panic` command must name the migrated table, or else ignored. + ### force-table-names Table name prefix to be used on the temporary tables. From b4da7cfbfb0a08a94e13df82403789713da4ed71 Mon Sep 17 00:00:00 2001 From: Rafe Kettler Date: Fri, 22 Feb 2019 10:33:19 -0800 Subject: [PATCH 3/4] Improve SSL support Related issue: https://github.com/github/gh-ost/issues/521 - Add --ssl-cert and --ssl-key options to specify SSL public/private key files - Allow combining --ssl-allow-insecure with other --ssl* flags. `mysql.RegisterTLSConfig` allows combining the corresponding parameters in the `tls.Config` it receives, so gh-ost should allow this. I found being able to pass --ssl-allow-insecure along with --ssl-ca, --ssl-cert, and --ssl-key useful in testing. - Use the same TLS config everywhere. Since the CLI only supports a single set of --ssl* configuration parameters, this should be fine -- `mysql.RegisterTLSConfig` documentation indicates the TLS config given will not be modified, so it can safely be used in many goroutines provided we also do not modify it. The previous implementation did not work when the TLS config was duplicated, which happens when gh-ost walks up the replication chain trying to find the master. This is because, when the config is duplicated, we must call `RegisterTLSConfig` again with the new config. This config is exactly the same, so it's easiest to side-step the issue by registering the TLS config once and using it everywhere. --- doc/command-line-flags.md | 8 +++++++ go/base/context.go | 4 +++- go/cmd/gh-ost/main.go | 8 +++++++ go/mysql/connection.go | 47 +++++++++++++++++++++++-------------- go/mysql/connection_test.go | 2 +- 5 files changed, 49 insertions(+), 20 deletions(-) diff --git a/doc/command-line-flags.md b/doc/command-line-flags.md index c20a5a8..2d9bfd1 100644 --- a/doc/command-line-flags.md +++ b/doc/command-line-flags.md @@ -189,6 +189,14 @@ Allows `gh-ost` to connect to the MySQL servers using encrypted connections, but `--ssl-ca=/path/to/ca-cert.pem`: ca certificate file (in PEM format) to use for server certificate verification. If specified, the default system ca cert pool will not be used for verification, only the ca cert provided here. Requires `--ssl`. +### ssl-cert + +`--ssl-cert=/path/to/ssl-cert.crt`: SSL public key certificate file (in PEM format). + +### ssl-key + +`--ssl-key=/path/to/ssl-key.key`: SSL private key file (in PEM format). + ### test-on-replica Issue the migration on a replica; do not modify data on master. Useful for validating, testing and benchmarking. See [`testing-on-replica`](testing-on-replica.md) diff --git a/go/base/context.go b/go/base/context.go index b2a6c15..8a8f721 100644 --- a/go/base/context.go +++ b/go/base/context.go @@ -102,6 +102,8 @@ type MigrationContext struct { UseTLS bool TLSAllowInsecure bool TLSCACertificate string + TLSCertificate string + TLSKey string CliMasterUser string CliMasterPassword string @@ -702,7 +704,7 @@ func (this *MigrationContext) ApplyCredentials() { func (this *MigrationContext) SetupTLS() error { if this.UseTLS { - return this.InspectorConnectionConfig.UseTLS(this.TLSCACertificate, this.TLSAllowInsecure) + return this.InspectorConnectionConfig.UseTLS(this.TLSCACertificate, this.TLSCertificate, this.TLSKey, this.TLSAllowInsecure) } return nil } diff --git a/go/cmd/gh-ost/main.go b/go/cmd/gh-ost/main.go index a467f07..5c30ffd 100644 --- a/go/cmd/gh-ost/main.go +++ b/go/cmd/gh-ost/main.go @@ -57,6 +57,8 @@ func main() { flag.BoolVar(&migrationContext.UseTLS, "ssl", false, "Enable SSL encrypted connections to MySQL hosts") flag.StringVar(&migrationContext.TLSCACertificate, "ssl-ca", "", "CA certificate in PEM format for TLS connections to MySQL hosts. Requires --ssl") + flag.StringVar(&migrationContext.TLSCertificate, "ssl-cert", "", "Certificate in PEM format for TLS connections to MySQL hosts. Requires --ssl") + flag.StringVar(&migrationContext.TLSKey, "ssl-key", "", "Key in PEM format for TLS connections to MySQL hosts. Requires --ssl") flag.BoolVar(&migrationContext.TLSAllowInsecure, "ssl-allow-insecure", false, "Skips verification of MySQL hosts' certificate chain and host name. Requires --ssl") flag.StringVar(&migrationContext.DatabaseName, "database", "", "database name (mandatory)") @@ -204,6 +206,12 @@ func main() { if migrationContext.TLSCACertificate != "" && !migrationContext.UseTLS { log.Fatalf("--ssl-ca requires --ssl") } + if migrationContext.TLSCertificate != "" && !migrationContext.UseTLS { + log.Fatalf("--ssl-cert requires --ssl") + } + if migrationContext.TLSKey != "" && !migrationContext.UseTLS { + log.Fatalf("--ssl-key requires --ssl") + } if migrationContext.TLSAllowInsecure && !migrationContext.UseTLS { log.Fatalf("--ssl-allow-insecure requires --ssl") } diff --git a/go/mysql/connection.go b/go/mysql/connection.go index d6c7215..654998c 100644 --- a/go/mysql/connection.go +++ b/go/mysql/connection.go @@ -16,6 +16,10 @@ import ( "github.com/go-sql-driver/mysql" ) +const ( + TLS_CONFIG_KEY = "ghost" +) + // ConnectionConfig is the minimal configuration required to connect to a MySQL server type ConnectionConfig struct { Key InstanceKey @@ -57,34 +61,41 @@ func (this *ConnectionConfig) Equals(other *ConnectionConfig) bool { return this.Key.Equals(&other.Key) || this.ImpliedKey.Equals(other.ImpliedKey) } -func (this *ConnectionConfig) UseTLS(caCertificatePath string, allowInsecure bool) error { +func (this *ConnectionConfig) UseTLS(caCertificatePath, clientCertificate, clientKey string, allowInsecure bool) error { var rootCertPool *x509.CertPool + var certs []tls.Certificate var err error - if !allowInsecure { - if caCertificatePath == "" { - rootCertPool, err = x509.SystemCertPool() - if err != nil { - return err - } - } else { - rootCertPool = x509.NewCertPool() - pem, err := ioutil.ReadFile(caCertificatePath) - if err != nil { - return err - } - if ok := rootCertPool.AppendCertsFromPEM(pem); !ok { - return errors.New("could not add ca certificate to cert pool") - } + if caCertificatePath == "" { + rootCertPool, err = x509.SystemCertPool() + if err != nil { + return err } + } else { + rootCertPool = x509.NewCertPool() + pem, err := ioutil.ReadFile(caCertificatePath) + if err != nil { + return err + } + if ok := rootCertPool.AppendCertsFromPEM(pem); !ok { + return errors.New("could not add ca certificate to cert pool") + } + } + if clientCertificate != "" || clientKey != "" { + cert, err := tls.LoadX509KeyPair(clientCertificate, clientKey) + if err != nil { + return err + } + certs = []tls.Certificate{cert} } this.tlsConfig = &tls.Config{ + Certificates: certs, RootCAs: rootCertPool, InsecureSkipVerify: allowInsecure, } - return mysql.RegisterTLSConfig(this.Key.StringCode(), this.tlsConfig) + return mysql.RegisterTLSConfig(TLS_CONFIG_KEY, this.tlsConfig) } func (this *ConnectionConfig) TLSConfig() *tls.Config { @@ -103,7 +114,7 @@ func (this *ConnectionConfig) GetDBUri(databaseName string) string { // simplify construction of the DSN below. tlsOption := "false" if this.tlsConfig != nil { - tlsOption = this.Key.StringCode() + tlsOption = TLS_CONFIG_KEY } return fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?interpolateParams=%t&autocommit=true&charset=utf8mb4,utf8,latin1&tls=%s", this.User, this.Password, hostname, this.Key.Port, databaseName, interpolateParams, tlsOption) } diff --git a/go/mysql/connection_test.go b/go/mysql/connection_test.go index 3a6cf29..6ce85a7 100644 --- a/go/mysql/connection_test.go +++ b/go/mysql/connection_test.go @@ -80,5 +80,5 @@ func TestGetDBUriWithTLSSetup(t *testing.T) { c.tlsConfig = &tls.Config{} uri := c.GetDBUri("test") - test.S(t).ExpectEquals(uri, "gromit:penguin@tcp(myhost:3306)/test?interpolateParams=true&autocommit=true&charset=utf8mb4,utf8,latin1&tls=myhost:3306") + test.S(t).ExpectEquals(uri, "gromit:penguin@tcp(myhost:3306)/test?interpolateParams=true&autocommit=true&charset=utf8mb4,utf8,latin1&tls=ghost") } From 7ead4c4a56dda163fbf50fce6bb22ac6d96b690f Mon Sep 17 00:00:00 2001 From: Shlomi Noach Date: Mon, 25 Feb 2019 14:02:57 +0200 Subject: [PATCH 4/4] named throttle, no-throttle --- go/logic/server.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/go/logic/server.go b/go/logic/server.go index 831fdfa..774c4ab 100644 --- a/go/logic/server.go +++ b/go/logic/server.go @@ -292,12 +292,22 @@ help # This message } case "throttle", "pause", "suspend": { + if arg != "" && arg != this.migrationContext.OriginalTableName { + // User explicitly provided table name. This is a courtesy protection mechanism + err := fmt.Errorf("User commanded 'throttle' on %s, but migrated table is %s; ignoring request.", arg, this.migrationContext.OriginalTableName) + return NoPrintStatusRule, err + } atomic.StoreInt64(&this.migrationContext.ThrottleCommandedByUser, 1) fmt.Fprintf(writer, throttleHint) return ForcePrintStatusAndHintRule, nil } case "no-throttle", "unthrottle", "resume", "continue": { + if arg != "" && arg != this.migrationContext.OriginalTableName { + // User explicitly provided table name. This is a courtesy protection mechanism + err := fmt.Errorf("User commanded 'no-throttle' on %s, but migrated table is %s; ignoring request.", arg, this.migrationContext.OriginalTableName) + return NoPrintStatusRule, err + } atomic.StoreInt64(&this.migrationContext.ThrottleCommandedByUser, 0) return ForcePrintStatusAndHintRule, nil }