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 <HumairAK@users.noreply.github.com>

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

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'

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:

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"
+      }

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",

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"

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
 }
 

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

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