From 32d3e01a7b2c945a0619d974689bc0d0c7e94b70 Mon Sep 17 00:00:00 2001 From: Humair Khan Date: Mon, 19 Feb 2024 21:04:02 -0500 Subject: [PATCH] feat: allow dspo to verify tls secured mysql This change allows the DSPO to perform heatlh checks against a tls secured database. If the database is behind a self-signed cert, end user can provide a ca-bundle either as a global cert (configmap named "odh-trust-bundle" or via the ".caBundle" config option via the DSPA. This change also exposes the "ExtraParams" field for DSP, meaning users can now add ny DSN parameters when configuring a DB connection for DSP. These params are utilized in the same manner by the DSPO when conducting the health check to keep the behavior consistent. Signed-off-by: Humair Khan --- api/v1alpha1/dspipeline_types.go | 13 ++ api/v1alpha1/zz_generated.deepcopy.go | 5 + ...b.io_datasciencepipelinesapplications.yaml | 10 ++ .../internal/apiserver/deployment.yaml.tmpl | 6 + .../apiserver/server-config.yaml.tmpl | 18 +++ controllers/apiserver.go | 1 + controllers/config/defaults.go | 5 +- controllers/database.go | 153 ++++++++++++++++-- controllers/dspipeline_params.go | 21 ++- controllers/suite_test.go | 4 +- .../created/apiserver_deployment.yaml | 8 +- .../created/apiserver_deployment.yaml | 7 + .../created/apiserver_deployment.yaml | 4 + .../created/apiserver_deployment.yaml | 4 + .../created/apiserver_deployment.yaml | 7 + .../created/apiserver_deployment.yaml | 7 + 16 files changed, 257 insertions(+), 16 deletions(-) create mode 100644 config/internal/apiserver/server-config.yaml.tmpl diff --git a/api/v1alpha1/dspipeline_types.go b/api/v1alpha1/dspipeline_types.go index d52f3bdbb..83f809f57 100644 --- a/api/v1alpha1/dspipeline_types.go +++ b/api/v1alpha1/dspipeline_types.go @@ -173,6 +173,19 @@ type MlPipelineUI struct { type Database struct { *MariaDB `json:"mariaDB,omitempty"` *ExternalDB `json:"externalDB,omitempty"` + + // +kubebuilder:validation:Optional + // CustomExtraParams allow users to further customize the sql dsn parameters used by the Pipeline Server + // when opening a connection with the Database. + // ref: https://github.com/go-sql-driver/mysql?tab=readme-ov-file#dsn-data-source-name + // + // Value must be a JSON string. For example, to disable tls for Pipeline Server DB connection + // the user can provide a string: {"tls":"true"} + // + // If updating post DSPA deployment, then a manual restart of the pipeline server pod will be required + // so the new configmap maybe consumed. + CustomExtraParams *string `json:"customExtraParams,omitempty"` + // Default: false // +kubebuilder:default:=false // +kubebuilder:validation:Optional diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 864bddb54..185a264fa 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -230,6 +230,11 @@ func (in *Database) DeepCopyInto(out *Database) { *out = new(ExternalDB) (*in).DeepCopyInto(*out) } + if in.CustomExtraParams != nil { + in, out := &in.CustomExtraParams, &out.CustomExtraParams + *out = new(string) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Database. diff --git a/config/crd/bases/datasciencepipelinesapplications.opendatahub.io_datasciencepipelinesapplications.yaml b/config/crd/bases/datasciencepipelinesapplications.opendatahub.io_datasciencepipelinesapplications.yaml index 9e49832df..51ba1c82d 100644 --- a/config/crd/bases/datasciencepipelinesapplications.opendatahub.io_datasciencepipelinesapplications.yaml +++ b/config/crd/bases/datasciencepipelinesapplications.opendatahub.io_datasciencepipelinesapplications.yaml @@ -181,6 +181,16 @@ spec: DS Pipelines metadata tracking. Specify either the default MariaDB deployment, or configure your own External SQL DB. properties: + customExtraParams: + description: "CustomExtraParams allow users to further customize + the sql dsn parameters used by the Pipeline Server when opening + a connection with the Database. ref: https://github.com/go-sql-driver/mysql?tab=readme-ov-file#dsn-data-source-name + \n Value must be a JSON string. For example, to disable tls + for Pipeline Server DB connection the user can provide a string: + {\"tls\":\"true\"} \n If updating post DSPA deployment, then + a manual restart of the pipeline server pod will be required + so the new configmap maybe consumed." + type: string disableHealthCheck: default: false description: 'Default: false' diff --git a/config/internal/apiserver/deployment.yaml.tmpl b/config/internal/apiserver/deployment.yaml.tmpl index ae0d064f4..3e00ec6aa 100644 --- a/config/internal/apiserver/deployment.yaml.tmpl +++ b/config/internal/apiserver/deployment.yaml.tmpl @@ -155,6 +155,9 @@ spec: {{ end }} {{ if or .APIServer.EnableSamplePipeline .APIServer.CABundle }} volumeMounts: + - name: server-config + mountPath: /config/config.json + subPath: config.json {{ if .APIServer.EnableSamplePipeline }} - name: sample-config mountPath: /config/sample_config.json @@ -220,6 +223,9 @@ spec: - name: proxy-tls secret: secretName: ds-pipelines-proxy-tls-{{.Name}} + - name: server-config + configMap: + name: pipeline-server-config-{{.Name}} {{ if .APIServer.CABundle }} - name: ca-bundle configMap: diff --git a/config/internal/apiserver/server-config.yaml.tmpl b/config/internal/apiserver/server-config.yaml.tmpl new file mode 100644 index 000000000..f90f27f36 --- /dev/null +++ b/config/internal/apiserver/server-config.yaml.tmpl @@ -0,0 +1,18 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: pipeline-server-config-{{.Name}} + namespace: {{.Namespace}} + labels: + app: {{.APIServerDefaultResourceName}} + component: data-science-pipelines +data: + config.json: | + { + "DBConfig": { + "DriverName": "mysql", + "ConMaxLifeTime": "120s", + "ExtraParams": {{ .DBConnection.ExtraParams }} + }, + "InitConnectionTimeout": "6m" + } diff --git a/controllers/apiserver.go b/controllers/apiserver.go index 4e12b3967..b22a3be43 100644 --- a/controllers/apiserver.go +++ b/controllers/apiserver.go @@ -28,6 +28,7 @@ const apiServerDefaultResourceNamePrefix = "ds-pipeline-" var apiServerTemplates = []string{ "apiserver/artifact_script.yaml.tmpl", + "apiserver/server-config.yaml.tmpl", "apiserver/role_ds-pipeline.yaml.tmpl", "apiserver/role_pipeline-runner.yaml.tmpl", "apiserver/role_ds-pipeline-user-access.yaml.tmpl", diff --git a/controllers/config/defaults.go b/controllers/config/defaults.go index 14991db2a..9a16dcf2a 100644 --- a/controllers/config/defaults.go +++ b/controllers/config/defaults.go @@ -41,8 +41,9 @@ const ( ArtifactScriptConfigMapKey = "artifact_script" DSPServicePrefix = "ds-pipeline" - DBSecretNamePrefix = "ds-pipeline-db-" - DBSecretKey = "password" + DBSecretNamePrefix = "ds-pipeline-db-" + DBSecretKey = "password" + DBDefaultExtraParams = "{\"tls\":\"%t\"}" MariaDBName = "mlpipeline" MariaDBHostPrefix = "mariadb" diff --git a/controllers/database.go b/controllers/database.go index 33a083a53..92c7bd0cd 100644 --- a/controllers/database.go +++ b/controllers/database.go @@ -17,12 +17,18 @@ package controllers import ( "context" + cryptoTls "crypto/tls" + "crypto/x509" "database/sql" b64 "encoding/base64" "fmt" + "github.com/go-logr/logr" + "github.com/go-sql-driver/mysql" _ "github.com/go-sql-driver/mysql" dspav1alpha1 "github.com/opendatahub-io/data-science-pipelines-operator/api/v1alpha1" "github.com/opendatahub-io/data-science-pipelines-operator/controllers/config" + "k8s.io/apimachinery/pkg/util/json" + "os" ) const dbSecret = "mariadb/secret.yaml.tmpl" @@ -35,22 +41,115 @@ var dbTemplates = []string{ dbSecret, } -// extract to var for mocking in testing -var ConnectAndQueryDatabase = func(host, port, username, password, dbname string) bool { +func tLSClientConfig(pem []byte) (*cryptoTls.Config, error) { + rootCertPool := x509.NewCertPool() + + if f := os.Getenv("SSL_CERT_FILE"); f != "" { + data, err := os.ReadFile(f) + if err == nil { + rootCertPool.AppendCertsFromPEM(data) + } + } + + if ok := rootCertPool.AppendCertsFromPEM(pem); !ok { + return nil, fmt.Errorf("error parsing CA Certificate, ensure provided certs are in valid PEM format") + } + tlsConfig := &cryptoTls.Config{ + RootCAs: rootCertPool, + } + return tlsConfig, nil +} + +func createMySQLConfig(user, password string, mysqlServiceHost string, + mysqlServicePort string, dbName string, mysqlExtraParams map[string]string) *mysql.Config { + + params := map[string]string{ + "charset": "utf8", + "parseTime": "True", + "loc": "Local", + } + + for k, v := range mysqlExtraParams { + params[k] = v + } + + return &mysql.Config{ + User: user, + Passwd: password, + Net: "tcp", + Addr: fmt.Sprintf("%s:%s", mysqlServiceHost, mysqlServicePort), + Params: params, + DBName: dbName, + AllowNativePasswords: true, + } +} + +var ConnectAndQueryDatabase = func( + host string, + log logr.Logger, + port, username, password, dbname, tls string, + pemCerts []byte, + extraParams map[string]string) (bool, error) { + + mysqlConfig := createMySQLConfig( + username, + password, + host, + port, + "", + extraParams, + ) + // Create a context with a timeout of 1 second ctx, cancel := context.WithTimeout(context.Background(), config.DefaultDBConnectionTimeout) defer cancel() - connectionString := fmt.Sprintf("%s:%s@tcp(%s:%s)/%s", username, password, host, port, dbname) - db, err := sql.Open("mysql", connectionString) + var tlsConfig *cryptoTls.Config + switch tls { + case "false", "": + // don't set anything + case "true": + if len(pemCerts) != 0 { + var err error + tlsConfig, err = tLSClientConfig(pemCerts) + if err != nil { + log.Info(fmt.Sprintf("Encountered error when processing custom ca bundle, Error: %v", err)) + return false, err + } + } + case "skip-verify", "preferred": + tlsConfig = &cryptoTls.Config{InsecureSkipVerify: true} + default: + // Unknown config, default to don't set anything + } + + // Only register tls config in the case of: "true", "skip-verify", "preferred" + if tlsConfig != nil { + err := mysql.RegisterTLSConfig("custom", tlsConfig) + // If ExtraParams{"tls": ".."} is set, that takes precedent over mysqlConfig.TLSConfig + // so we need to make sure we're setting our tls config to be used instead if it exists + if _, ok := mysqlConfig.Params["tls"]; ok { + mysqlConfig.Params["tls"] = "custom" + } + // Just to be safe, we also set it here, fallback from mysqlConfig.Params["tls"] not being set + mysqlConfig.TLSConfig = "custom" + if err != nil { + return false, err + } + } + + db, err := sql.Open("mysql", mysqlConfig.FormatDSN()) if err != nil { - return false + return false, err } defer db.Close() testStatement := "SELECT 1;" _, err = db.QueryContext(ctx, testStatement) - return err == nil + if err != nil { + return false, err + } + return true, nil } func (r *DSPAReconciler) isDatabaseAccessible(ctx context.Context, dsp *dspav1alpha1.DataSciencePipelinesApplication, @@ -72,16 +171,50 @@ func (r *DSPAReconciler) isDatabaseAccessible(ctx context.Context, dsp *dspav1al } decodePass, _ := b64.StdEncoding.DecodeString(params.DBConnection.Password) - dbHealthCheckPassed := ConnectAndQueryDatabase(params.DBConnection.Host, + + var extraParamsJson map[string]string + err := json.Unmarshal([]byte(params.DBConnection.ExtraParams), &extraParamsJson) + if err != nil { + log.Info(fmt.Sprintf("Could not parse tls config in ExtraParams, if setting CustomExtraParams, ensure the JSON string is well-formed. Error: %v", err)) + return false + } + + // tls can be true, false, skip-verify, preferred + // we default to true if it's an externalDB, false otherwise + // (if not specified via CustomExtraParams) + tls := "false" + if usingExternalDB { + tls = "true" + } + + // Override tls with the value in ExtraParams, if specified + // If users have specified a CustomExtraParams field, have to + // check for "tls" existence because users may choose to leave + // out the "tls" param + if val, ok := extraParamsJson["tls"]; ok { + tls = val + } + + dbHealthCheckPassed, err := ConnectAndQueryDatabase( + params.DBConnection.Host, + log, params.DBConnection.Port, params.DBConnection.Username, string(decodePass), - params.DBConnection.DBName) + params.DBConnection.DBName, + tls, + params.APICustomPemCerts, + extraParamsJson, + ) + if err != nil { + log.Info(fmt.Sprintf("Unable to connect to Database: %v", err)) + return false + } + if dbHealthCheckPassed { log.Info("Database Health Check Successful") - } else { - log.Info("Unable to connect to Database") } + return dbHealthCheckPassed } diff --git a/controllers/dspipeline_params.go b/controllers/dspipeline_params.go index 4d49ceba4..d476fa1a8 100644 --- a/controllers/dspipeline_params.go +++ b/controllers/dspipeline_params.go @@ -20,6 +20,7 @@ import ( "context" "encoding/base64" "fmt" + "k8s.io/apimachinery/pkg/util/json" "math/rand" "time" @@ -65,6 +66,7 @@ type DBConnection struct { DBName string CredentialsSecret *dspa.SecretKeyValue Password string + ExtraParams string } type ObjectStorageConnection struct { @@ -87,7 +89,7 @@ func (p *DSPAParams) UsingExternalDB(dsp *dspa.DataSciencePipelinesApplication) return false } -// StorageHealthCheckDisabled will return the value if the Database has disableHealthCheck specified in the CR, otherwise false. +// DatabaseHealthCheckDisabled will return the value if the Database has disableHealthCheck specified in the CR, otherwise false. func (p *DSPAParams) DatabaseHealthCheckDisabled(dsp *dspa.DataSciencePipelinesApplication) bool { if dsp.Spec.Database != nil { return dsp.Spec.Database.DisableHealthCheck @@ -149,6 +151,9 @@ func (p *DSPAParams) SetupDBParams(ctx context.Context, dsp *dspa.DataSciencePip p.DBConnection.Port = dsp.Spec.Database.ExternalDB.Port p.DBConnection.Username = dsp.Spec.Database.ExternalDB.Username p.DBConnection.DBName = dsp.Spec.Database.ExternalDB.DBName + // Assume default external connection is tls enabled + // user can override this via CustomExtraParams field + p.DBConnection.ExtraParams = fmt.Sprintf(config.DBDefaultExtraParams, true) customCreds = dsp.Spec.Database.ExternalDB.PasswordSecret } else { // If no externalDB or mariaDB is specified, DSPO assumes @@ -180,11 +185,25 @@ func (p *DSPAParams) SetupDBParams(ctx context.Context, dsp *dspa.DataSciencePip p.DBConnection.Port = config.MariaDBHostPort p.DBConnection.Username = p.MariaDB.Username p.DBConnection.DBName = p.MariaDB.DBName + // By Default OOB mariadb is not tls enabled + p.DBConnection.ExtraParams = fmt.Sprintf(config.DBDefaultExtraParams, false) if p.MariaDB.PasswordSecret != nil { customCreds = p.MariaDB.PasswordSecret } } + // User specified custom Extra parameters will always take precedence + if dsp.Spec.Database.CustomExtraParams != nil { + // Validate CustomExtraParams is a valid params json + var validParamsJson map[string]string + err := json.Unmarshal([]byte(*dsp.Spec.Database.CustomExtraParams), &validParamsJson) + if err != nil { + log.Info(fmt.Sprintf("Encountered error when validationg CustomExtraParams field in DSPA, please ensure the params are well-formed: Error: %v", err)) + return err + } + p.DBConnection.ExtraParams = *dsp.Spec.Database.CustomExtraParams + } + // Secret where DB credentials reside on cluster var credsSecretName string var credsPasswordKey string diff --git a/controllers/suite_test.go b/controllers/suite_test.go index c8e9c2801..8adfd9d55 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -73,8 +73,8 @@ func TestAPIs(t *testing.T) { var _ = BeforeEach(func() { By("Overriding the Database and Object Store live connection functions with trivial stubs") - ConnectAndQueryDatabase = func(host string, port string, username string, password string, dbname string) bool { - return true + ConnectAndQueryDatabase = func(host string, log logr.Logger, port, username, password, dbname, tls string, pemCerts []byte, extraParams map[string]string) (bool, error) { + return true, nil } ConnectAndQueryObjStore = func(ctx context.Context, log logr.Logger, endpoint, bucket string, accesskey, secretkey []byte, secure bool, pemCerts []byte) bool { return true diff --git a/controllers/testdata/declarative/case_0/expected/created/apiserver_deployment.yaml b/controllers/testdata/declarative/case_0/expected/created/apiserver_deployment.yaml index fa277a796..4be9291a6 100644 --- a/controllers/testdata/declarative/case_0/expected/created/apiserver_deployment.yaml +++ b/controllers/testdata/declarative/case_0/expected/created/apiserver_deployment.yaml @@ -136,6 +136,9 @@ spec: cpu: 500m memory: 1Gi volumeMounts: + - name: server-config + mountPath: /config/config.json + subPath: config.json - mountPath: /config/sample_config.json name: sample-config subPath: sample_config.json @@ -193,6 +196,10 @@ spec: secret: secretName: ds-pipelines-proxy-tls-testdsp0 defaultMode: 420 + - name: server-config + configMap: + defaultMode: 420 + name: pipeline-server-config-testdsp0 - configMap: defaultMode: 420 name: sample-config-testdsp0 @@ -201,5 +208,4 @@ spec: defaultMode: 420 name: sample-pipeline-testdsp0 name: sample-pipeline - serviceAccountName: ds-pipeline-testdsp0 diff --git a/controllers/testdata/declarative/case_2/expected/created/apiserver_deployment.yaml b/controllers/testdata/declarative/case_2/expected/created/apiserver_deployment.yaml index 5c1263828..c2205a899 100644 --- a/controllers/testdata/declarative/case_2/expected/created/apiserver_deployment.yaml +++ b/controllers/testdata/declarative/case_2/expected/created/apiserver_deployment.yaml @@ -136,6 +136,9 @@ spec: cpu: 2522m memory: 5Gi volumeMounts: + - name: server-config + mountPath: /config/config.json + subPath: config.json - mountPath: /config/sample_config.json name: sample-config subPath: sample_config.json @@ -193,6 +196,10 @@ spec: secret: secretName: ds-pipelines-proxy-tls-testdsp2 defaultMode: 420 + - name: server-config + configMap: + defaultMode: 420 + name: pipeline-server-config-testdsp2 - configMap: defaultMode: 420 name: sample-config-testdsp2 diff --git a/controllers/testdata/declarative/case_3/expected/created/apiserver_deployment.yaml b/controllers/testdata/declarative/case_3/expected/created/apiserver_deployment.yaml index 0b617788d..79e78d123 100644 --- a/controllers/testdata/declarative/case_3/expected/created/apiserver_deployment.yaml +++ b/controllers/testdata/declarative/case_3/expected/created/apiserver_deployment.yaml @@ -187,4 +187,8 @@ spec: secret: secretName: ds-pipelines-proxy-tls-testdsp3 defaultMode: 420 + - name: server-config + configMap: + name: pipeline-server-config-testdsp3 + defaultMode: 420 serviceAccountName: ds-pipeline-testdsp3 diff --git a/controllers/testdata/declarative/case_4/expected/created/apiserver_deployment.yaml b/controllers/testdata/declarative/case_4/expected/created/apiserver_deployment.yaml index 94524294c..eabb4b6fe 100644 --- a/controllers/testdata/declarative/case_4/expected/created/apiserver_deployment.yaml +++ b/controllers/testdata/declarative/case_4/expected/created/apiserver_deployment.yaml @@ -187,4 +187,8 @@ spec: secret: secretName: ds-pipelines-proxy-tls-testdsp4 defaultMode: 420 + - name: server-config + configMap: + defaultMode: 420 + name: pipeline-server-config-testdsp4 serviceAccountName: ds-pipeline-testdsp4 diff --git a/controllers/testdata/declarative/case_5/expected/created/apiserver_deployment.yaml b/controllers/testdata/declarative/case_5/expected/created/apiserver_deployment.yaml index 92f6ac5b9..1f59a95e9 100644 --- a/controllers/testdata/declarative/case_5/expected/created/apiserver_deployment.yaml +++ b/controllers/testdata/declarative/case_5/expected/created/apiserver_deployment.yaml @@ -136,6 +136,9 @@ spec: cpu: 500m memory: 1Gi volumeMounts: + - name: server-config + mountPath: /config/config.json + subPath: config.json - mountPath: /config/sample_config.json name: sample-config subPath: sample_config.json @@ -193,6 +196,10 @@ spec: secret: secretName: ds-pipelines-proxy-tls-testdsp5 defaultMode: 420 + - configMap: + defaultMode: 420 + name: pipeline-server-config-testdsp5 + name: server-config - configMap: defaultMode: 420 name: sample-config-testdsp5 diff --git a/controllers/testdata/declarative/case_6/expected/created/apiserver_deployment.yaml b/controllers/testdata/declarative/case_6/expected/created/apiserver_deployment.yaml index 96c5967e8..585b0db80 100644 --- a/controllers/testdata/declarative/case_6/expected/created/apiserver_deployment.yaml +++ b/controllers/testdata/declarative/case_6/expected/created/apiserver_deployment.yaml @@ -142,6 +142,9 @@ spec: cpu: 500m memory: 1Gi volumeMounts: + - name: server-config + mountPath: /config/config.json + subPath: config.json - name: ca-bundle mountPath: /etc/pki/tls/certs - name: oauth-proxy @@ -196,6 +199,10 @@ spec: secret: secretName: ds-pipelines-proxy-tls-testdsp6 defaultMode: 420 + - name: server-config + configMap: + name: pipeline-server-config-testdsp6 + defaultMode: 420 - name: ca-bundle configMap: name: testcabundleconfigmap6