Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: DSPO handle db tls connections and configs (v2) #577

Merged
merged 8 commits into from
Mar 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions api/v1alpha1/dspipeline_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,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 may be consumed.
CustomExtraParams *string `json:"customExtraParams,omitempty"`

// Default: false
// +kubebuilder:default:=false
// +kubebuilder:validation:Optional
Expand Down
5 changes: 5 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion config/base/params.env
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ IMAGESV2_ARGO_APISERVER=quay.io/opendatahub/ds-pipelines-api-server:latest
IMAGESV2_ARGO_PERSISTENCEAGENT=quay.io/opendatahub/ds-pipelines-persistenceagent:latest
IMAGESV2_ARGO_SCHEDULEDWORKFLOW=quay.io/opendatahub/ds-pipelines-scheduledworkflow:latest
IMAGESV2_ARGO_MLMDENVOY=registry.redhat.io/openshift-service-mesh/proxyv2-rhel8:2.3.9-2
IMAGESV2_ARGO_MLMDGRPC=quay.io/opendatahub/ds-pipelines-metadata-grpc:latest
IMAGESV2_ARGO_MLMDGRPC=quay.io/opendatahub/mlmd-grpc-server:latest
IMAGESV2_ARGO_MLMDWRITER=quay.io/opendatahub/ds-pipelines-metadata-writer:latest
IMAGESV2_ARGO_WORKFLOWCONTROLLER=quay.io/opendatahub/ds-pipelines-argo-workflowcontroller:3.3.10-upstream
IMAGESV2_ARGO_ARGOEXEC=quay.io/opendatahub/ds-pipelines-argo-argoexec:3.3.10-upstream
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,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 may be consumed."
type: string
disableHealthCheck:
default: false
description: 'Default: false'
Expand Down
4 changes: 2 additions & 2 deletions config/internal/apiserver/default/artifact_script.yaml.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ data:
artifact_name=$(basename $2)

aws_cp() {
{{ if .APIServer.CABundle }}
aws s3 --endpoint {{.ObjectStorageConnection.Endpoint}} --ca-bundle {{ .PiplinesCABundleMountPath }}/{{ .APIServer.CABundle.ConfigMapKey }} cp $1.tgz s3://{{.ObjectStorageConnection.Bucket}}/artifacts/$PIPELINERUN/$PIPELINETASK/$1.tgz
{{ if .CustomCABundle }}
aws s3 --endpoint {{.ObjectStorageConnection.Endpoint}} --ca-bundle {{ .PiplinesCABundleMountPath }} cp $1.tgz s3://{{.ObjectStorageConnection.Bucket}}/artifacts/$PIPELINERUN/$PIPELINETASK/$1.tgz
{{ else }}
aws s3 --endpoint {{.ObjectStorageConnection.Endpoint}} cp $1.tgz s3://{{.ObjectStorageConnection.Bucket}}/artifacts/$PIPELINERUN/$PIPELINETASK/$1.tgz
{{ end }}
Expand Down
31 changes: 19 additions & 12 deletions config/internal/apiserver/default/deployment.yaml.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,17 @@ spec:
value: "{{.DBConnection.Host}}"
- name: DBCONFIG_PORT
value: "{{.DBConnection.Port}}"
{{ if .APIServer.CABundle }}
{{ if .CustomCABundle }}
- name: ARTIFACT_COPY_STEP_CABUNDLE_CONFIGMAP_NAME
value: "{{.APIServer.CABundle.ConfigMapName}}"
value: "{{.CustomCABundle.ConfigMapName}}"
- name: ARTIFACT_COPY_STEP_CABUNDLE_CONFIGMAP_KEY
value: "{{.APIServer.CABundle.ConfigMapKey}}"
value: "{{.CustomCABundle.ConfigMapKey}}"
- name: ARTIFACT_COPY_STEP_CABUNDLE_MOUNTPATH
value: {{ .PiplinesCABundleMountPath }}
value: {{ .CustomCABundleRootMountPath }}
{{ end }}
{{ if .CustomSSLCertDir }}
- name: SSL_CERT_DIR
value: {{.CustomSSLCertDir}}
{{ end }}
- name: AUTO_UPDATE_PIPELINE_DEFAULT_VERSION
value: "{{.APIServer.AutoUpdatePipelineDefaultVersion}}"
Expand Down Expand Up @@ -206,17 +210,20 @@ spec:
memory: {{.APIServer.Resources.Limits.Memory}}
{{ end }}
{{ end }}
{{ if or .APIServer.EnableSamplePipeline .APIServer.CABundle }}
{{ if or .APIServer.EnableSamplePipeline .CustomCABundle }}
volumeMounts:
- name: server-config
mountPath: /config/config.json
subPath: config.json
{{ if .APIServer.EnableSamplePipeline }}
- name: sample-config
mountPath: /config/sample_config.json
subPath: sample_config.json
- name: sample-pipeline
mountPath: /samples/
{{ end }}
{{ if .APIServer.CABundle }}
- mountPath: {{ .APIServerPiplinesCABundleMountPath }}
{{ if .CustomCABundle }}
- mountPath: {{ .CustomCABundleRootMountPath }}
name: ca-bundle
{{ end }}
{{ end }}
Expand Down Expand Up @@ -273,13 +280,13 @@ spec:
- name: proxy-tls
secret:
secretName: ds-pipelines-proxy-tls-{{.Name}}
{{ if .APIServer.CABundle }}
- name: server-config
configMap:
name: pipeline-server-config-{{.Name}}
{{ if .CustomCABundle }}
- name: ca-bundle
configMap:
name: {{ .APIServer.CABundle.ConfigMapName }}
items:
- key: {{ .APIServer.CABundle.ConfigMapKey }}
path: {{ .APIServer.CABundle.ConfigMapKey }}
name: {{ .CustomCABundle.ConfigMapName }}
{{ end }}
{{ if .APIServer.EnableSamplePipeline }}
- name: sample-config
Expand Down
33 changes: 33 additions & 0 deletions config/internal/apiserver/default/server-config.yaml.tmpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
apiVersion: v1
kind: ConfigMap
metadata:
name: pipeline-server-config-{{.Name}}
namespace: {{.Namespace}}
labels:
app: {{.APIServerDefaultResourceName}}
component: data-science-pipelines
data:
config.json: |
{{ if eq .DSPVersion "v2" }}
{
"DBConfig": {
"MySQLConfig": {
"ExtraParams": {{ .DBConnection.ExtraParams }},
"GroupConcatMaxLen": "4194304"
},
"PostgreSQLConfig": {},
"ConMaxLifeTime": "120s"
},
"DBDriverName": "mysql",
"InitConnectionTimeout": "6m"
}
{{ else }}
{
"DBConfig": {
"DriverName": "mysql",
"ConMaxLifeTime": "120s",
"ExtraParams": {{ .DBConnection.ExtraParams }}
},
"InitConnectionTimeout": "6m"
}
{{ end }}
14 changes: 14 additions & 0 deletions config/internal/ml-metadata/metadata-grpc.deployment.yaml.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ spec:
- --mysql_config_user=$(DBCONFIG_USER)
- --mysql_config_password=$(DBCONFIG_PASSWORD)
- --enable_database_upgrade=true
{{ if .CustomCABundle }}
- --mysql_config_sslrootcert={{ .PiplinesCABundleMountPath }}
{{ end }}
command:
- /bin/metadata_store_server
env:
Expand Down Expand Up @@ -82,4 +85,15 @@ spec:
memory: {{.MLMD.GRPC.Resources.Limits.Memory}}
{{ end }}
{{ end }}
volumeMounts:
{{ if .CustomCABundle }}
- mountPath: {{ .CustomCABundleRootMountPath }}
name: ca-bundle
{{ end }}
serviceAccountName: ds-pipeline-metadata-grpc-{{.Name}}
volumes:
{{ if .CustomCABundle }}
- name: ca-bundle
configMap:
name: {{ .CustomCABundle.ConfigMapName }}
{{ end }}
33 changes: 26 additions & 7 deletions controllers/config/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package config

import (
"fmt"
"time"

dspav1alpha1 "github.com/opendatahub-io/data-science-pipelines-operator/api/v1alpha1"
Expand All @@ -25,16 +26,25 @@ import (
)

const (
DefaultImageValue = "MustSetInConfig"
APIServerPiplinesCABundleMountPath = "/etc/pki/tls/certs"
PiplinesCABundleMountPath = "/etc/pki/tls/certs"
MLPipelineUIConfigMapPrefix = "ds-pipeline-ui-configmap-"
ArtifactScriptConfigMapNamePrefix = "ds-pipeline-artifact-script-"
ArtifactScriptConfigMapKey = "artifact_script"
DSPServicePrefix = "ds-pipeline"
DefaultImageValue = "MustSetInConfig"

CustomCABundleRootMountPath = "/dsp-custom-certs"

// GlobalODHCaBundleConfigMapName key and label values are a contract with
// ODH Platform https://github.com/opendatahub-io/architecture-decision-records/pull/28
GlobalODHCaBundleConfigMapName = "odh-trusted-ca-bundle"

CustomDSPTrustedCAConfigMapNamePrefix = "dsp-trusted-ca"
CustomDSPTrustedCAConfigMapKey = "dsp-ca.crt"

MLPipelineUIConfigMapPrefix = "ds-pipeline-ui-configmap-"
ArtifactScriptConfigMapNamePrefix = "ds-pipeline-artifact-script-"
ArtifactScriptConfigMapKey = "artifact_script"
DSPServicePrefix = "ds-pipeline"

DefaultDBSecretNamePrefix = "ds-pipeline-db-"
DefaultDBSecretKey = "password"
DBDefaultExtraParams = "{\"tls\":\"%t\"}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as in the v1 patch -- I would prefer a type safe way of doing this / not using string templating. This seems like not idiomatic go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can open a "would be nice to refactor this" ticket for this and add it to our backlog. Not really a big deal / definitely no need to block merge.

GeneratedDBPasswordLength = 12

MariaDBName = "mlpipeline"
Expand Down Expand Up @@ -195,3 +205,12 @@ func GetDurationConfigWithDefault(configName string, value time.Duration) time.D
}
return viper.GetDuration(configName)
}

// GetCABundleFileMountPath provides the location in pipeline step-copy-artifact step where the
// ca bundle is mounted for aws cli to connect to s3 store.
// Since pipeline step-copy-artifact step uses aws cli, and there are issues surrounding
// passing a path to aws cli (see: https://github.com/aws/aws-cli/issues/3425#issuecomment-402289636)
// as such for pipelines, we concatenate the certs into a single cert bundle and use a separate configmap for this
func GetCABundleFileMountPath() string {
return fmt.Sprintf("%s/%s", CustomCABundleRootMountPath, CustomDSPTrustedCAConfigMapKey)
}
Loading
Loading