From fb4dc7bd309d9a3009c653b0fede172702c386c3 Mon Sep 17 00:00:00 2001 From: Giulio Frasca Date: Mon, 15 May 2023 17:21:03 -0400 Subject: [PATCH 01/13] Remove External Database Secret Creation --- controllers/database.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/controllers/database.go b/controllers/database.go index 33a083a5..0c454ddf 100644 --- a/controllers/database.go +++ b/controllers/database.go @@ -102,13 +102,7 @@ func (r *DSPAReconciler) ReconcileDatabase(ctx context.Context, dsp *dspav1alpha // If external db is specified, it takes precedence if externalDBSpecified { - log.Info("Deploying external db secret.") - // If using external DB, we just need to create the secret - // for apiserver - err := r.Apply(dsp, params, dbSecret) - if err != nil { - return err - } + log.Info("Using externalDB, bypassing database deployment.") } else if deployMariaDB || deployDefaultDB { log.Info("Applying mariaDB resources.") for _, template := range dbTemplates { From 2821c465f8ac77a146ccd56385434ea8d3df6818 Mon Sep 17 00:00:00 2001 From: Giulio Frasca Date: Mon, 15 May 2023 17:30:06 -0400 Subject: [PATCH 02/13] Remove External Object Storage Secret Creation --- controllers/storage.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/controllers/storage.go b/controllers/storage.go index fedc3a2b..23da6bca 100644 --- a/controllers/storage.go +++ b/controllers/storage.go @@ -197,13 +197,7 @@ func (r *DSPAReconciler) ReconcileStorage(ctx context.Context, dsp *dspav1alpha1 // If external storage is specified, it takes precedence if externalStorageSpecified { - log.Info("Deploying external storage secret.") - // If using external storage, we just need to create the secret - // for apiserver - err := r.Apply(dsp, params, storageSecret) - if err != nil { - return err - } + log.Info("Using externalStorage, bypassing object storage deployment.") } else if deployMinio { log.Info("Applying object storage resources.") for _, template := range storageTemplates { @@ -224,7 +218,7 @@ func (r *DSPAReconciler) ReconcileStorage(ctx context.Context, dsp *dspav1alpha1 } } } else { - log.Info("No externalstorage detected, and minio disabled. " + + log.Info("No externalStorage detected, and minio disabled. " + "skipping application of storage Resources") return nil } From d91acbae51365526afea65557375c8f48f878d96 Mon Sep 17 00:00:00 2001 From: Giulio Frasca Date: Tue, 23 May 2023 03:19:40 -0400 Subject: [PATCH 03/13] Create DevTools in DSPA CR, Add Database/Storage Secret Creation --- api/v1alpha1/dspipeline_types.go | 12 +++++ api/v1alpha1/zz_generated.deepcopy.go | 20 +++++++ ...b.io_datasciencepipelinesapplications.yaml | 11 ++++ .../database.secret.yaml.tmpl} | 0 .../storage.secret.yaml.tmpl} | 0 controllers/database.go | 2 - controllers/developer_tools.go | 53 +++++++++++++++++++ controllers/dspipeline_controller.go | 5 ++ controllers/dspipeline_params.go | 2 + controllers/storage.go | 2 - .../declarative/case_3/deploy/02_cr.yaml | 3 ++ 11 files changed, 106 insertions(+), 4 deletions(-) rename config/internal/{mariadb/secret.yaml.tmpl => devtools/database.secret.yaml.tmpl} (100%) rename config/internal/{minio/secret.yaml.tmpl => devtools/storage.secret.yaml.tmpl} (100%) create mode 100644 controllers/developer_tools.go diff --git a/api/v1alpha1/dspipeline_types.go b/api/v1alpha1/dspipeline_types.go index d8922956..d9a24286 100644 --- a/api/v1alpha1/dspipeline_types.go +++ b/api/v1alpha1/dspipeline_types.go @@ -43,6 +43,9 @@ type DSPASpec struct { // +kubebuilder:validation:Optional // +kubebuilder:default:={deploy: false} *MLMD `json:"mlmd"` + // DevTools enables non-production tools and utilities for Data Science Pipelines Operator developers + // +kubebuilder:validation:Optional + *DevTools `json:"devtools"` } type APIServer struct { @@ -267,6 +270,15 @@ type Writer struct { Image string `json:"image"` } +type DevTools struct { + // +kubebuilder:default:=false + // +kubebuilder:validation:Optional + EnableDatabaseSecret bool `json:"enableDatabaseSecret"` + // +kubebuilder:default:=false + // +kubebuilder:validation:Optional + EnableStorageSecret bool `json:"enableStorageSecret"` +} + // ResourceRequirements structures compute resource requirements. // Replaces ResourceRequirements from corev1 which also includes optional storage field. // We handle storage field separately, and should not include it as a subfield for Resources. diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 864bddb5..a0c003b7 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -124,6 +124,11 @@ func (in *DSPASpec) DeepCopyInto(out *DSPASpec) { *out = new(MLMD) (*in).DeepCopyInto(*out) } + if in.DevTools != nil { + in, out := &in.DevTools, &out.DevTools + *out = new(DevTools) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DSPASpec. @@ -242,6 +247,21 @@ func (in *Database) DeepCopy() *Database { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *DevTools) DeepCopyInto(out *DevTools) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DevTools. +func (in *DevTools) DeepCopy() *DevTools { + if in == nil { + return nil + } + out := new(DevTools) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Envoy) DeepCopyInto(out *Envoy) { *out = *in diff --git a/config/crd/bases/datasciencepipelinesapplications.opendatahub.io_datasciencepipelinesapplications.yaml b/config/crd/bases/datasciencepipelinesapplications.opendatahub.io_datasciencepipelinesapplications.yaml index 5bbb80be..2a6c521e 100644 --- a/config/crd/bases/datasciencepipelinesapplications.opendatahub.io_datasciencepipelinesapplications.yaml +++ b/config/crd/bases/datasciencepipelinesapplications.opendatahub.io_datasciencepipelinesapplications.yaml @@ -287,6 +287,17 @@ spec: type: string type: object type: object + devtools: + description: DevTools enables non-production tools and utilities for + Data Science Pipelines Operator developers + properties: + enableDatabaseSecret: + default: false + type: boolean + enableStorageSecret: + default: false + type: boolean + type: object mlmd: default: deploy: false diff --git a/config/internal/mariadb/secret.yaml.tmpl b/config/internal/devtools/database.secret.yaml.tmpl similarity index 100% rename from config/internal/mariadb/secret.yaml.tmpl rename to config/internal/devtools/database.secret.yaml.tmpl diff --git a/config/internal/minio/secret.yaml.tmpl b/config/internal/devtools/storage.secret.yaml.tmpl similarity index 100% rename from config/internal/minio/secret.yaml.tmpl rename to config/internal/devtools/storage.secret.yaml.tmpl diff --git a/controllers/database.go b/controllers/database.go index 0c454ddf..5d2bb651 100644 --- a/controllers/database.go +++ b/controllers/database.go @@ -25,8 +25,6 @@ import ( "github.com/opendatahub-io/data-science-pipelines-operator/controllers/config" ) -const dbSecret = "mariadb/secret.yaml.tmpl" - var dbTemplates = []string{ "mariadb/deployment.yaml.tmpl", "mariadb/pvc.yaml.tmpl", diff --git a/controllers/developer_tools.go b/controllers/developer_tools.go new file mode 100644 index 00000000..4031b83b --- /dev/null +++ b/controllers/developer_tools.go @@ -0,0 +1,53 @@ +/* + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controllers + +import ( + dspav1alpha1 "github.com/opendatahub-io/data-science-pipelines-operator/api/v1alpha1" +) + +const dbSecret = "devtools/database.secret.yaml.tmpl" +const storageSecret = "devtools/storage.secret.yaml.tmpl" + +func (r *DSPAReconciler) ReconcileDevtools(dsp *dspav1alpha1.DataSciencePipelinesApplication, params *DSPAParams) error { + + log := r.Log.WithValues("namespace", dsp.Namespace).WithValues("dspa_name", dsp.Name) + + if dsp.Spec.DevTools != nil { + log.Info("Applying DevTool Resources") + + dbSecretEnabled := dsp.Spec.DevTools.EnableDatabaseSecret + storageSecretEnabled := dsp.Spec.DevTools.EnableStorageSecret + + if dbSecretEnabled { + log.Info("Database secret creation requested") + err := r.Apply(dsp, params, dbSecret) + if err != nil { + return err + } + } + + if storageSecretEnabled { + log.Info("Object Storage secret creation requested") + err := r.Apply(dsp, params, storageSecret) + if err != nil { + return err + } + } + log.Info("Finished applying DevTool Resources") + } + return nil +} diff --git a/controllers/dspipeline_controller.go b/controllers/dspipeline_controller.go index d92275ab..2cdd8116 100644 --- a/controllers/dspipeline_controller.go +++ b/controllers/dspipeline_controller.go @@ -261,6 +261,11 @@ func (r *DSPAReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. } } + err = r.ReconcileDevtools(dspa, params) + if err != nil { + return ctrl.Result{}, err + } + log.Info("Updating CR status") // Refresh DSPA before updating err = r.Get(ctx, req.NamespacedName, dspa) diff --git a/controllers/dspipeline_params.go b/controllers/dspipeline_params.go index 1d8d8d46..c1e7bcd8 100644 --- a/controllers/dspipeline_params.go +++ b/controllers/dspipeline_params.go @@ -54,6 +54,7 @@ type DSPAParams struct { MariaDB *dspa.MariaDB Minio *dspa.Minio MLMD *dspa.MLMD + DevTools *dspa.DevTools DBConnection ObjectStorageConnection } @@ -442,6 +443,7 @@ func (p *DSPAParams) ExtractParams(ctx context.Context, dsp *dspa.DataSciencePip p.MlPipelineUI = dsp.Spec.MlPipelineUI.DeepCopy() p.MariaDB = dsp.Spec.Database.MariaDB.DeepCopy() p.Minio = dsp.Spec.ObjectStorage.Minio.DeepCopy() + p.DevTools = dsp.Spec.DevTools.DeepCopy() p.OAuthProxy = config.GetStringConfigWithDefault(config.OAuthProxyImagePath, config.DefaultImageValue) p.MLMD = dsp.Spec.MLMD.DeepCopy() p.APIServerPiplinesCABundleMountPath = config.APIServerPiplinesCABundleMountPath diff --git a/controllers/storage.go b/controllers/storage.go index 23da6bca..afe1d284 100644 --- a/controllers/storage.go +++ b/controllers/storage.go @@ -31,8 +31,6 @@ import ( "net/http" ) -const storageSecret = "minio/secret.yaml.tmpl" - var storageTemplates = []string{ "minio/deployment.yaml.tmpl", "minio/pvc.yaml.tmpl", diff --git a/controllers/testdata/declarative/case_3/deploy/02_cr.yaml b/controllers/testdata/declarative/case_3/deploy/02_cr.yaml index 0b63880e..4122613f 100644 --- a/controllers/testdata/declarative/case_3/deploy/02_cr.yaml +++ b/controllers/testdata/declarative/case_3/deploy/02_cr.yaml @@ -34,3 +34,6 @@ spec: mlpipelineUI: deploy: false image: frontend:test3 + devtools: + enableDatabaseSecret: true + enableStorageSecret: true From 8bef10fe6c86dfc8fe8c09b692aa72ead8bea589 Mon Sep 17 00:00:00 2001 From: Giulio Frasca Date: Mon, 12 Jun 2023 15:48:36 -0400 Subject: [PATCH 04/13] DSPO to manage credential secrets if using deployed storage/db --- config/internal/mariadb/secret.yaml.tmpl | 10 ++++++++++ config/internal/minio/secret.yaml.tmpl | 15 +++++++++++++++ controllers/database.go | 8 ++++++-- controllers/storage.go | 4 ++-- 4 files changed, 33 insertions(+), 4 deletions(-) create mode 100644 config/internal/mariadb/secret.yaml.tmpl create mode 100644 config/internal/minio/secret.yaml.tmpl diff --git a/config/internal/mariadb/secret.yaml.tmpl b/config/internal/mariadb/secret.yaml.tmpl new file mode 100644 index 00000000..62a9b4d2 --- /dev/null +++ b/config/internal/mariadb/secret.yaml.tmpl @@ -0,0 +1,10 @@ +apiVersion: v1 +kind: Secret +metadata: + name: "{{.DBConnection.CredentialsSecret.Name}}" + namespace: {{.Namespace}} + labels: + app: mariadb-{{.Name}} + component: data-science-pipelines +data: + password: {{.DBConnection.Password}} diff --git a/config/internal/minio/secret.yaml.tmpl b/config/internal/minio/secret.yaml.tmpl new file mode 100644 index 00000000..17192f27 --- /dev/null +++ b/config/internal/minio/secret.yaml.tmpl @@ -0,0 +1,15 @@ +apiVersion: v1 +kind: Secret +metadata: + name: "{{.ObjectStorageConnection.CredentialsSecret.SecretName}}" + namespace: {{.Namespace}} + labels: + app: minio-{{.Name}} + component: data-science-pipelines +stringData: + host: "{{.ObjectStorageConnection.Host}}" + port: "{{.ObjectStorageConnection.Port}}" + secure: "{{.ObjectStorageConnection.Secure}}" +data: + accesskey: "{{.ObjectStorageConnection.AccessKeyID}}" + secretkey: "{{.ObjectStorageConnection.SecretAccessKey}}" diff --git a/controllers/database.go b/controllers/database.go index 5d2bb651..178cb9b3 100644 --- a/controllers/database.go +++ b/controllers/database.go @@ -25,10 +25,11 @@ import ( "github.com/opendatahub-io/data-science-pipelines-operator/controllers/config" ) -var dbTemplates = []string{ +var mariadbTemplates = []string{ "mariadb/deployment.yaml.tmpl", "mariadb/pvc.yaml.tmpl", "mariadb/service.yaml.tmpl", +<<<<<<< HEAD "mariadb/mariadb-sa.yaml.tmpl", dbSecret, } @@ -81,6 +82,9 @@ func (r *DSPAReconciler) isDatabaseAccessible(ctx context.Context, dsp *dspav1al log.Info("Unable to connect to Database") } return dbHealthCheckPassed +======= + "mariadb/secret.yaml.tmpl", +>>>>>>> 24a957a (DSPO to manage credential secrets if using deployed storage/db) } func (r *DSPAReconciler) ReconcileDatabase(ctx context.Context, dsp *dspav1alpha1.DataSciencePipelinesApplication, @@ -103,7 +107,7 @@ func (r *DSPAReconciler) ReconcileDatabase(ctx context.Context, dsp *dspav1alpha log.Info("Using externalDB, bypassing database deployment.") } else if deployMariaDB || deployDefaultDB { log.Info("Applying mariaDB resources.") - for _, template := range dbTemplates { + for _, template := range mariadbTemplates { err := r.Apply(dsp, params, template) if err != nil { return err diff --git a/controllers/storage.go b/controllers/storage.go index afe1d284..c2814806 100644 --- a/controllers/storage.go +++ b/controllers/storage.go @@ -31,7 +31,7 @@ import ( "net/http" ) -var storageTemplates = []string{ +var minioTemplates = []string{ "minio/deployment.yaml.tmpl", "minio/pvc.yaml.tmpl", "minio/service.yaml.tmpl", @@ -198,7 +198,7 @@ func (r *DSPAReconciler) ReconcileStorage(ctx context.Context, dsp *dspav1alpha1 log.Info("Using externalStorage, bypassing object storage deployment.") } else if deployMinio { log.Info("Applying object storage resources.") - for _, template := range storageTemplates { + for _, template := range minioTemplates { err := r.Apply(dsp, params, template) if err != nil { return err From c0b976281142009838c4cd6ac8fd3fe67a201d2c Mon Sep 17 00:00:00 2001 From: Giulio Frasca Date: Wed, 21 Jun 2023 01:50:49 -0400 Subject: [PATCH 05/13] Change DB/Storage Behavior to only Generate creds if deployable requested --- config/internal/mariadb/secret.yaml.tmpl | 2 +- config/internal/minio/secret.yaml.tmpl | 4 +- controllers/config/defaults.go | 10 +- controllers/database.go | 6 +- controllers/developer_tools.go | 8 +- controllers/dspipeline_params.go | 215 ++++++++---------- controllers/storage.go | 2 + .../created/apiserver_deployment.yaml | 12 +- .../expected/created/database_secret.yaml | 11 - .../expected/not_created/database_secret.yaml | 12 + .../storage_secret.yaml | 5 +- 11 files changed, 131 insertions(+), 156 deletions(-) delete mode 100644 controllers/testdata/declarative/case_3/expected/created/database_secret.yaml create mode 100644 controllers/testdata/declarative/case_3/expected/not_created/database_secret.yaml rename controllers/testdata/declarative/case_3/expected/{created => not_created}/storage_secret.yaml (60%) diff --git a/config/internal/mariadb/secret.yaml.tmpl b/config/internal/mariadb/secret.yaml.tmpl index 62a9b4d2..3293bc70 100644 --- a/config/internal/mariadb/secret.yaml.tmpl +++ b/config/internal/mariadb/secret.yaml.tmpl @@ -7,4 +7,4 @@ metadata: app: mariadb-{{.Name}} component: data-science-pipelines data: - password: {{.DBConnection.Password}} + {{.DBConnection.CredentialsSecret.Key}}: "{{.DBConnection.Password}}" diff --git a/config/internal/minio/secret.yaml.tmpl b/config/internal/minio/secret.yaml.tmpl index 17192f27..e0cf555c 100644 --- a/config/internal/minio/secret.yaml.tmpl +++ b/config/internal/minio/secret.yaml.tmpl @@ -11,5 +11,5 @@ stringData: port: "{{.ObjectStorageConnection.Port}}" secure: "{{.ObjectStorageConnection.Secure}}" data: - accesskey: "{{.ObjectStorageConnection.AccessKeyID}}" - secretkey: "{{.ObjectStorageConnection.SecretAccessKey}}" + {{.ObjectStorageConnection.CredentialsSecret.AccessKey}}: "{{.ObjectStorageConnection.AccessKeyID}}" + {{.ObjectStorageConnection.CredentialsSecret.SecretKey}}: "{{.ObjectStorageConnection.SecretAccessKey}}" diff --git a/controllers/config/defaults.go b/controllers/config/defaults.go index b13ef3c7..a5c3e1dc 100644 --- a/controllers/config/defaults.go +++ b/controllers/config/defaults.go @@ -32,8 +32,8 @@ const ( ArtifactScriptConfigMapKey = "artifact_script" DSPServicePrefix = "ds-pipeline" - DBSecretNamePrefix = "ds-pipeline-db-" - DBSecretKey = "password" + DefaultDBSecretNamePrefix = "ds-pipeline-db-" + DefaultDBSecretKey = "password" MariaDBName = "mlpipeline" MariaDBHostPrefix = "mariadb" @@ -47,9 +47,9 @@ const ( MinioDefaultBucket = "mlpipeline" MinioPVCSize = "10Gi" - ObjectStorageSecretName = "mlpipeline-minio-artifact" // hardcoded in kfp-tekton - ObjectStorageAccessKey = "accesskey" - ObjectStorageSecretKey = "secretkey" + DefaultObjectStorageSecretName = "mlpipeline-minio-artifact" // hardcoded in kfp-tekton + DefaultObjectStorageAccessKey = "accesskey" + DefaultObjectStorageSecretKey = "secretkey" MlmdGrpcPort = "8080" ) diff --git a/controllers/database.go b/controllers/database.go index 178cb9b3..b95b7367 100644 --- a/controllers/database.go +++ b/controllers/database.go @@ -25,11 +25,12 @@ import ( "github.com/opendatahub-io/data-science-pipelines-operator/controllers/config" ) +const dbSecret = "mariadb/secret.yaml.tmpl" + var mariadbTemplates = []string{ "mariadb/deployment.yaml.tmpl", "mariadb/pvc.yaml.tmpl", "mariadb/service.yaml.tmpl", -<<<<<<< HEAD "mariadb/mariadb-sa.yaml.tmpl", dbSecret, } @@ -82,9 +83,6 @@ func (r *DSPAReconciler) isDatabaseAccessible(ctx context.Context, dsp *dspav1al log.Info("Unable to connect to Database") } return dbHealthCheckPassed -======= - "mariadb/secret.yaml.tmpl", ->>>>>>> 24a957a (DSPO to manage credential secrets if using deployed storage/db) } func (r *DSPAReconciler) ReconcileDatabase(ctx context.Context, dsp *dspav1alpha1.DataSciencePipelinesApplication, diff --git a/controllers/developer_tools.go b/controllers/developer_tools.go index 4031b83b..b74c4ab1 100644 --- a/controllers/developer_tools.go +++ b/controllers/developer_tools.go @@ -19,8 +19,8 @@ import ( dspav1alpha1 "github.com/opendatahub-io/data-science-pipelines-operator/api/v1alpha1" ) -const dbSecret = "devtools/database.secret.yaml.tmpl" -const storageSecret = "devtools/storage.secret.yaml.tmpl" +const devDBSecret = "devtools/database.secret.yaml.tmpl" +const devStorageSecret = "devtools/storage.secret.yaml.tmpl" func (r *DSPAReconciler) ReconcileDevtools(dsp *dspav1alpha1.DataSciencePipelinesApplication, params *DSPAParams) error { @@ -34,7 +34,7 @@ func (r *DSPAReconciler) ReconcileDevtools(dsp *dspav1alpha1.DataSciencePipeline if dbSecretEnabled { log.Info("Database secret creation requested") - err := r.Apply(dsp, params, dbSecret) + err := r.Apply(dsp, params, devDBSecret) if err != nil { return err } @@ -42,7 +42,7 @@ func (r *DSPAReconciler) ReconcileDevtools(dsp *dspav1alpha1.DataSciencePipeline if storageSecretEnabled { log.Info("Object Storage secret creation requested") - err := r.Apply(dsp, params, storageSecret) + err := r.Apply(dsp, params, devStorageSecret) if err != nil { return err } diff --git a/controllers/dspipeline_params.go b/controllers/dspipeline_params.go index c1e7bcd8..6e7cafbb 100644 --- a/controllers/dspipeline_params.go +++ b/controllers/dspipeline_params.go @@ -129,28 +129,79 @@ func passwordGen(n int) string { return string(b) } +func (p *DSPAParams) RetrieveSecret(ctx context.Context, client client.Client, secretName, secretKey string, log logr.Logger) (string, error) { + secret := &v1.Secret{} + namespacedName := types.NamespacedName{ + Name: secretName, + Namespace: p.Namespace, + } + err := client.Get(ctx, namespacedName, secret) + if err != nil { + log.V(1).Info(fmt.Sprintf("Unable to retrieve secret [%s].", secretName)) + return "", err + } + return base64.StdEncoding.EncodeToString(secret.Data[secretKey]), nil +} + +func (p *DSPAParams) GenerateDBSecret(ctx context.Context, client client.Client, secret *dspa.SecretKeyValue, log logr.Logger) error { + val, err := p.RetrieveSecret(ctx, client, secret.Name, secret.Key, log) + if err != nil && apierrs.IsNotFound(err) { + generatedPass := passwordGen(12) + p.DBConnection.Password = base64.StdEncoding.EncodeToString([]byte(generatedPass)) //TODO: it's ugly setting this class value here, just return a generic val instead + } else if err != nil { + log.Error(err, "Unable to create DB secret...") + return err + } else { + log.Info(fmt.Sprintf("Secret [%s] already exists, using stored value.", secret.Name)) + p.DBConnection.Password = val + } + return nil +} + +func (p *DSPAParams) GenerateObjectStoreSecret(ctx context.Context, client client.Client, secret *dspa.S3CredentialSecret, log logr.Logger) error { + val, err := p.RetrieveSecret(ctx, client, secret.SecretName, secret.AccessKey, log) + if err != nil && apierrs.IsNotFound(err) { + generatedPass := passwordGen(16) + p.ObjectStorageConnection.AccessKeyID = base64.StdEncoding.EncodeToString([]byte(generatedPass)) //TODO: it's ugly setting this class value here, just return a generic val instead + } else if err != nil { + log.Error(err, "Unable to retrieve or create ObjectStore secret...") + return err + } else { + log.Info(fmt.Sprintf("Secret [%s] already exists, using stored value.", secret.SecretName)) + p.ObjectStorageConnection.AccessKeyID = val + } + + val, err = p.RetrieveSecret(ctx, client, secret.SecretName, secret.SecretKey, log) + if err != nil && apierrs.IsNotFound(err) { + generatedPass := passwordGen(24) + p.ObjectStorageConnection.SecretAccessKey = base64.StdEncoding.EncodeToString([]byte(generatedPass)) //TODO: it's ugly setting this class value here, just return a generic val instead + } else if err != nil { + log.Error(err, "Unable to retrieve or create ObjectStore secret...") + return err + } else { + log.Info(fmt.Sprintf("Secret [%s] already exists, using stored value.", secret.SecretName)) + p.ObjectStorageConnection.SecretAccessKey = val + } + return nil +} + // SetupDBParams Populates the DB connection Parameters. // If an external secret is specified, SetupDBParams will retrieve DB credentials from it. // If DSPO is managing a dynamically created secret, then SetupDBParams generates the creds. func (p *DSPAParams) SetupDBParams(ctx context.Context, dsp *dspa.DataSciencePipelinesApplication, client client.Client, log logr.Logger) error { usingExternalDB := p.UsingExternalDB(dsp) - - var customCreds *dspa.SecretKeyValue - - // Even if a secret is specified DSPO will deploy its own secret owned by DSPO - p.DBConnection.CredentialsSecret = &dspa.SecretKeyValue{ - Name: config.DBSecretNamePrefix + p.Name, - Key: config.DBSecretKey, - } - if usingExternalDB { // Assume validation for CR ensures these values exist p.DBConnection.Host = dsp.Spec.Database.ExternalDB.Host p.DBConnection.Port = dsp.Spec.Database.ExternalDB.Port p.DBConnection.Username = dsp.Spec.Database.ExternalDB.Username p.DBConnection.DBName = dsp.Spec.Database.ExternalDB.DBName - customCreds = dsp.Spec.Database.ExternalDB.PasswordSecret + p.DBConnection.CredentialsSecret = dsp.Spec.Database.ExternalDB.PasswordSecret + + // Retreive DB Password from specified secret. Handle errors (empty password) later + password, _ := p.RetrieveSecret(ctx, client, p.DBConnection.CredentialsSecret.Name, p.DBConnection.CredentialsSecret.Key, log) + p.DBConnection.Password = password } else { // If no externalDB or mariaDB is specified, DSPO assumes // MariaDB deployment with defaults. @@ -164,6 +215,7 @@ func (p *DSPAParams) SetupDBParams(ctx context.Context, dsp *dspa.DataSciencePip PVCSize: resource.MustParse(config.MariaDBNamePVCSize), } } + // If MariaDB was specified, ensure missing fields are // populated with defaults. if p.MariaDB.Image == "" { @@ -181,59 +233,24 @@ 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 - if p.MariaDB.PasswordSecret != nil { - customCreds = p.MariaDB.PasswordSecret - } - } - - // Secret where DB credentials reside on cluster - var credsSecretName string - var credsPasswordKey string - customCredentialsSpecified := customCreds != nil - if customCredentialsSpecified { - credsSecretName = customCreds.Name - credsPasswordKey = customCreds.Key - } else { - credsSecretName = p.DBConnection.CredentialsSecret.Name - credsPasswordKey = p.DBConnection.CredentialsSecret.Key - } - - dbSecret := &v1.Secret{} - namespacedName := types.NamespacedName{ - Name: credsSecretName, - Namespace: p.Namespace, - } - - createNewSecret := false - - // Attempt to fetch the specified DB secret - err := client.Get(ctx, namespacedName, dbSecret) - if err != nil && apierrs.IsNotFound(err) { - if !customCredentialsSpecified { - generatedPass := passwordGen(12) - p.DBConnection.Password = base64.StdEncoding.EncodeToString([]byte(generatedPass)) - createNewSecret = true + // If custom DB Secret provided, use its values. Otherwise generate a default + if p.MariaDB.PasswordSecret != nil { + p.DBConnection.CredentialsSecret = p.MariaDB.PasswordSecret } else { - log.Error(err, fmt.Sprintf("DB secret [%s] was specified in CR but does not exist.", - credsSecretName)) + p.DBConnection.CredentialsSecret = &dspa.SecretKeyValue{ + Name: config.DefaultDBSecretNamePrefix + p.Name, + Key: config.DefaultDBSecretKey, + } + } + err := p.GenerateDBSecret(ctx, client, p.DBConnection.CredentialsSecret, log) + if err != nil { return err } - } else if err != nil { - log.Error(err, "Unable to fetch DB secret...") - return err - } - - // Password was dynamically generated, no need to retrieve it from fetched secret - if createNewSecret { - return nil } - - p.DBConnection.Password = base64.StdEncoding.EncodeToString(dbSecret.Data[credsPasswordKey]) - if p.DBConnection.Password == "" { return fmt.Errorf(fmt.Sprintf("DB Password from secret [%s] for key [%s] was not successfully retrieved, "+ - "ensure that the secret with this key exist.", credsSecretName, credsPasswordKey)) + "ensure that the secret with this key exist.", p.DBConnection.CredentialsSecret.Name, p.DBConnection.CredentialsSecret.Key)) } return nil } @@ -244,16 +261,6 @@ func (p *DSPAParams) SetupDBParams(ctx context.Context, dsp *dspa.DataSciencePip func (p *DSPAParams) SetupObjectParams(ctx context.Context, dsp *dspa.DataSciencePipelinesApplication, client client.Client, log logr.Logger) error { usingExternalObjectStorage := p.UsingExternalStorage(dsp) - - var customCreds *dspa.S3CredentialSecret - - // Even if a secret is specified DSPO will deploy its own secret owned by DSPO - p.ObjectStorageConnection.CredentialsSecret = &dspa.S3CredentialSecret{ - SecretName: config.ObjectStorageSecretName, - AccessKey: config.ObjectStorageAccessKey, - SecretKey: config.ObjectStorageSecretKey, - } - if usingExternalObjectStorage { // Assume validation for CR ensures these values exist p.ObjectStorageConnection.Bucket = dsp.Spec.ObjectStorage.ExternalStorage.Bucket @@ -272,7 +279,13 @@ func (p *DSPAParams) SetupObjectParams(ctx context.Context, dsp *dspa.DataScienc // Port can be empty, which is fine. p.ObjectStorageConnection.Port = dsp.Spec.ObjectStorage.ExternalStorage.Port - customCreds = dsp.Spec.ObjectStorage.ExternalStorage.S3CredentialSecret + p.ObjectStorageConnection.CredentialsSecret = dsp.Spec.ObjectStorage.ExternalStorage.S3CredentialSecret + + // Retrieve ObjStore Creds from specified secret. Handle issues (empty creds) later. + accesskey, _ := p.RetrieveSecret(ctx, client, p.ObjectStorageConnection.CredentialsSecret.SecretName, p.ObjectStorageConnection.CredentialsSecret.AccessKey, log) + secretkey, _ := p.RetrieveSecret(ctx, client, p.ObjectStorageConnection.CredentialsSecret.SecretName, p.ObjectStorageConnection.CredentialsSecret.SecretKey, log) + p.ObjectStorageConnection.AccessKeyID = accesskey + p.ObjectStorageConnection.SecretAccessKey = secretkey } else { if p.Minio == nil { return fmt.Errorf("either [spec.objectStorage.minio] or [spec.objectStorage.externalStorage] " + @@ -302,7 +315,17 @@ func (p *DSPAParams) SetupObjectParams(ctx context.Context, dsp *dspa.DataScienc p.ObjectStorageConnection.Secure = util.BoolPointer(false) if p.Minio.S3CredentialSecret != nil { - customCreds = p.Minio.S3CredentialSecret + p.ObjectStorageConnection.CredentialsSecret = p.Minio.S3CredentialSecret + } else { + p.ObjectStorageConnection.CredentialsSecret = &dspa.S3CredentialSecret{ + SecretName: config.DefaultObjectStorageSecretName, + AccessKey: config.DefaultObjectStorageAccessKey, + SecretKey: config.DefaultObjectStorageSecretKey, + } + } + err := p.GenerateObjectStoreSecret(ctx, client, p.ObjectStorageConnection.CredentialsSecret, log) + if err != nil { + return err } } @@ -322,62 +345,12 @@ func (p *DSPAParams) SetupObjectParams(ctx context.Context, dsp *dspa.DataScienc p.ObjectStorageConnection.Endpoint = endpoint - // Secret where credentials reside on cluster - var credsSecretName string - var credsAccessKey string - var credsSecretKey string - - customCredentialsSpecified := customCreds != nil - if customCredentialsSpecified { - credsSecretName = customCreds.SecretName - credsAccessKey = customCreds.AccessKey - credsSecretKey = customCreds.SecretKey - } else { - credsSecretName = p.ObjectStorageConnection.CredentialsSecret.SecretName - credsAccessKey = p.ObjectStorageConnection.CredentialsSecret.AccessKey - credsSecretKey = p.ObjectStorageConnection.CredentialsSecret.SecretKey - } - - storageSecret := &v1.Secret{} - namespacedName := types.NamespacedName{ - Name: credsSecretName, - Namespace: p.Namespace, - } - - createNewSecret := false - - // Attempt to fetch the specified storage secret - err := client.Get(ctx, namespacedName, storageSecret) - if err != nil && apierrs.IsNotFound(err) { - if !customCredentialsSpecified { - generatedPass := passwordGen(16) - p.ObjectStorageConnection.AccessKeyID = base64.StdEncoding.EncodeToString([]byte(generatedPass)) - generatedPass = passwordGen(24) - p.ObjectStorageConnection.SecretAccessKey = base64.StdEncoding.EncodeToString([]byte(generatedPass)) - createNewSecret = true - } else { - log.Error(err, fmt.Sprintf("Storage secret [%s] was specified in CR but does not exist.", - credsSecretName)) - return err - } - } else if err != nil { - log.Error(err, "Unable to fetch Storage secret...") - return err - } - - // Password was dynamically generated, no need to retrieve it from fetched secret - if createNewSecret { - return nil - } - - p.ObjectStorageConnection.AccessKeyID = base64.StdEncoding.EncodeToString(storageSecret.Data[credsAccessKey]) - p.ObjectStorageConnection.SecretAccessKey = base64.StdEncoding.EncodeToString(storageSecret.Data[credsSecretKey]) - if p.ObjectStorageConnection.AccessKeyID == "" || p.ObjectStorageConnection.SecretAccessKey == "" { return fmt.Errorf(fmt.Sprintf("Object Storage Password from secret [%s] for keys [%s, %s] was not "+ - "successfully retrieved, ensure that the secret with this key exist.", credsSecretName, credsAccessKey, credsSecretKey)) + "successfully retrieved, ensure that the secret with this key exist.", + p.ObjectStorageConnection.CredentialsSecret.SecretName, + p.ObjectStorageConnection.CredentialsSecret.AccessKey, p.ObjectStorageConnection.CredentialsSecret.SecretKey)) } - return nil } diff --git a/controllers/storage.go b/controllers/storage.go index c2814806..a3ceba70 100644 --- a/controllers/storage.go +++ b/controllers/storage.go @@ -31,6 +31,8 @@ import ( "net/http" ) +const storageSecret = "minio/secret.yaml.tmpl" + var minioTemplates = []string{ "minio/deployment.yaml.tmpl", "minio/pvc.yaml.tmpl", 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 0b617788..25fafd5f 100644 --- a/controllers/testdata/declarative/case_3/expected/created/apiserver_deployment.yaml +++ b/controllers/testdata/declarative/case_3/expected/created/apiserver_deployment.yaml @@ -29,8 +29,8 @@ spec: - name: DBCONFIG_PASSWORD valueFrom: secretKeyRef: - key: "password" - name: "ds-pipeline-db-testdsp3" + key: "testpswkey3" + name: "testdbpswsecretname3" - name: DBCONFIG_DBNAME value: "testdbname3" - name: DBCONFIG_HOST @@ -77,13 +77,13 @@ spec: - name: OBJECTSTORECONFIG_ACCESSKEY valueFrom: secretKeyRef: - key: "accesskey" - name: "mlpipeline-minio-artifact" + key: "testaccesskey3" + name: "teststoragesecretname3" - name: OBJECTSTORECONFIG_SECRETACCESSKEY valueFrom: secretKeyRef: - key: "secretkey" - name: "mlpipeline-minio-artifact" + key: "testsecretkey3" + name: "teststoragesecretname3" - name: OBJECTSTORECONFIG_SECURE value: "true" - name: MINIO_SERVICE_SERVICE_HOST diff --git a/controllers/testdata/declarative/case_3/expected/created/database_secret.yaml b/controllers/testdata/declarative/case_3/expected/created/database_secret.yaml deleted file mode 100644 index 9f263014..00000000 --- a/controllers/testdata/declarative/case_3/expected/created/database_secret.yaml +++ /dev/null @@ -1,11 +0,0 @@ -kind: Secret -apiVersion: v1 -metadata: - name: ds-pipeline-db-testdsp3 - namespace: default - labels: - app: mariadb-extrenal-storage - component: data-science-pipelines -data: - password: dGVzdGRic2VjcmV0cHN3dmFsdWUz -type: Opaque diff --git a/controllers/testdata/declarative/case_3/expected/not_created/database_secret.yaml b/controllers/testdata/declarative/case_3/expected/not_created/database_secret.yaml new file mode 100644 index 00000000..ec3365d4 --- /dev/null +++ b/controllers/testdata/declarative/case_3/expected/not_created/database_secret.yaml @@ -0,0 +1,12 @@ +kind: Secret +apiVersion: v1 +metadata: + # todo: remove todo- from name this should actually be checked for but causes failures because previous tests don't clean up properly + name: todo-ds-pipeline-db-testdsp3 + namespace: namespace3 + labels: + app: mariadb-extrenal-storage + component: data-science-pipelines +data: + password: dGVzdGRic2VjcmV0cHN3dmFsdWUz +type: Opaque diff --git a/controllers/testdata/declarative/case_3/expected/created/storage_secret.yaml b/controllers/testdata/declarative/case_3/expected/not_created/storage_secret.yaml similarity index 60% rename from controllers/testdata/declarative/case_3/expected/created/storage_secret.yaml rename to controllers/testdata/declarative/case_3/expected/not_created/storage_secret.yaml index 36d77f68..a1baf725 100644 --- a/controllers/testdata/declarative/case_3/expected/created/storage_secret.yaml +++ b/controllers/testdata/declarative/case_3/expected/not_created/storage_secret.yaml @@ -1,8 +1,9 @@ kind: Secret apiVersion: v1 metadata: - name: mlpipeline-minio-artifact - namespace: default + # todo: remove todo- this should actually be checked for but causes failures because previous tests don't clean up properly + name: todo-mlpipeline-minio-artifact + namespace: namespace3 labels: app: minio-extrenal-storage component: data-science-pipelines From 95b5ce73c20a9326966017b095e8276b084ac1f5 Mon Sep 17 00:00:00 2001 From: Giulio Frasca Date: Wed, 30 Aug 2023 17:32:33 -0400 Subject: [PATCH 06/13] Include DSPA name on Created ObjectStorage Secret Names --- controllers/config/defaults.go | 6 +++--- controllers/dspipeline_params.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/controllers/config/defaults.go b/controllers/config/defaults.go index a5c3e1dc..87ad09dd 100644 --- a/controllers/config/defaults.go +++ b/controllers/config/defaults.go @@ -47,9 +47,9 @@ const ( MinioDefaultBucket = "mlpipeline" MinioPVCSize = "10Gi" - DefaultObjectStorageSecretName = "mlpipeline-minio-artifact" // hardcoded in kfp-tekton - DefaultObjectStorageAccessKey = "accesskey" - DefaultObjectStorageSecretKey = "secretkey" + DefaultObjectStorageSecretNamePrefix = "ds-pipeline-s3-" + DefaultObjectStorageAccessKey = "accesskey" + DefaultObjectStorageSecretKey = "secretkey" MlmdGrpcPort = "8080" ) diff --git a/controllers/dspipeline_params.go b/controllers/dspipeline_params.go index 6e7cafbb..39b4d904 100644 --- a/controllers/dspipeline_params.go +++ b/controllers/dspipeline_params.go @@ -318,7 +318,7 @@ func (p *DSPAParams) SetupObjectParams(ctx context.Context, dsp *dspa.DataScienc p.ObjectStorageConnection.CredentialsSecret = p.Minio.S3CredentialSecret } else { p.ObjectStorageConnection.CredentialsSecret = &dspa.S3CredentialSecret{ - SecretName: config.DefaultObjectStorageSecretName, + SecretName: config.DefaultObjectStorageSecretNamePrefix + p.Name, AccessKey: config.DefaultObjectStorageAccessKey, SecretKey: config.DefaultObjectStorageSecretKey, } From c0af336e6c016df997640b97dcfc5fb98edf3df6 Mon Sep 17 00:00:00 2001 From: Giulio Frasca Date: Wed, 30 Aug 2023 18:03:42 -0400 Subject: [PATCH 07/13] Centralize Secret value retrieval/creation functionality --- controllers/config/defaults.go | 9 +++-- controllers/dspipeline_params.go | 61 +++++++++++++++----------------- 2 files changed, 34 insertions(+), 36 deletions(-) diff --git a/controllers/config/defaults.go b/controllers/config/defaults.go index 87ad09dd..2b0c38f6 100644 --- a/controllers/config/defaults.go +++ b/controllers/config/defaults.go @@ -34,6 +34,7 @@ const ( DefaultDBSecretNamePrefix = "ds-pipeline-db-" DefaultDBSecretKey = "password" + GeneratedDBPasswordLength = 12 MariaDBName = "mlpipeline" MariaDBHostPrefix = "mariadb" @@ -47,9 +48,11 @@ const ( MinioDefaultBucket = "mlpipeline" MinioPVCSize = "10Gi" - DefaultObjectStorageSecretNamePrefix = "ds-pipeline-s3-" - DefaultObjectStorageAccessKey = "accesskey" - DefaultObjectStorageSecretKey = "secretkey" + DefaultObjectStorageSecretNamePrefix = "ds-pipeline-s3-" + DefaultObjectStorageAccessKey = "accesskey" + DefaultObjectStorageSecretKey = "secretkey" + GeneratedObjectStorageAccessKeyLength = 16 + GeneratedObjectStorageSecretKeyLength = 24 MlmdGrpcPort = "8080" ) diff --git a/controllers/dspipeline_params.go b/controllers/dspipeline_params.go index 39b4d904..8739cc77 100644 --- a/controllers/dspipeline_params.go +++ b/controllers/dspipeline_params.go @@ -143,46 +143,38 @@ func (p *DSPAParams) RetrieveSecret(ctx context.Context, client client.Client, s return base64.StdEncoding.EncodeToString(secret.Data[secretKey]), nil } -func (p *DSPAParams) GenerateDBSecret(ctx context.Context, client client.Client, secret *dspa.SecretKeyValue, log logr.Logger) error { - val, err := p.RetrieveSecret(ctx, client, secret.Name, secret.Key, log) +func (p *DSPAParams) RetrieveOrCreateSecret(ctx context.Context, client client.Client, secretName, secretKey string, generatedPasswordLength int, log logr.Logger) (string, error) { + val, err := p.RetrieveSecret(ctx, client, secretName, secretKey, log) if err != nil && apierrs.IsNotFound(err) { - generatedPass := passwordGen(12) - p.DBConnection.Password = base64.StdEncoding.EncodeToString([]byte(generatedPass)) //TODO: it's ugly setting this class value here, just return a generic val instead + generatedPass := passwordGen(generatedPasswordLength) + return base64.StdEncoding.EncodeToString([]byte(generatedPass)), nil } else if err != nil { log.Error(err, "Unable to create DB secret...") - return err - } else { - log.Info(fmt.Sprintf("Secret [%s] already exists, using stored value.", secret.Name)) - p.DBConnection.Password = val + return "", err } - return nil + log.Info(fmt.Sprintf("Secret [%s] already exists, using stored value.", secretName)) + return val, nil } -func (p *DSPAParams) GenerateObjectStoreSecret(ctx context.Context, client client.Client, secret *dspa.S3CredentialSecret, log logr.Logger) error { - val, err := p.RetrieveSecret(ctx, client, secret.SecretName, secret.AccessKey, log) - if err != nil && apierrs.IsNotFound(err) { - generatedPass := passwordGen(16) - p.ObjectStorageConnection.AccessKeyID = base64.StdEncoding.EncodeToString([]byte(generatedPass)) //TODO: it's ugly setting this class value here, just return a generic val instead - } else if err != nil { - log.Error(err, "Unable to retrieve or create ObjectStore secret...") - return err - } else { - log.Info(fmt.Sprintf("Secret [%s] already exists, using stored value.", secret.SecretName)) - p.ObjectStorageConnection.AccessKeyID = val +func (p *DSPAParams) RetrieveOrCreateDBSecret(ctx context.Context, client client.Client, secret *dspa.SecretKeyValue, log logr.Logger) (string, error) { + dbPassword, err := p.RetrieveOrCreateSecret(ctx, client, secret.Name, secret.Key, config.GeneratedDBPasswordLength, log) + if err != nil { + return "", err } + return dbPassword, nil - val, err = p.RetrieveSecret(ctx, client, secret.SecretName, secret.SecretKey, log) - if err != nil && apierrs.IsNotFound(err) { - generatedPass := passwordGen(24) - p.ObjectStorageConnection.SecretAccessKey = base64.StdEncoding.EncodeToString([]byte(generatedPass)) //TODO: it's ugly setting this class value here, just return a generic val instead - } else if err != nil { - log.Error(err, "Unable to retrieve or create ObjectStore secret...") - return err - } else { - log.Info(fmt.Sprintf("Secret [%s] already exists, using stored value.", secret.SecretName)) - p.ObjectStorageConnection.SecretAccessKey = val +} + +func (p *DSPAParams) RetrieveOrCreateObjectStoreSecret(ctx context.Context, client client.Client, secret *dspa.S3CredentialSecret, log logr.Logger) (string, string, error) { + accessKey, err := p.RetrieveOrCreateSecret(ctx, client, secret.SecretName, secret.AccessKey, config.GeneratedObjectStorageAccessKeyLength, log) + if err != nil { + return "", "", err } - return nil + secretKey, err := p.RetrieveOrCreateSecret(ctx, client, secret.SecretName, secret.SecretKey, config.GeneratedObjectStorageSecretKeyLength, log) + if err != nil { + return "", "", err + } + return accessKey, secretKey, nil } // SetupDBParams Populates the DB connection Parameters. @@ -243,10 +235,11 @@ func (p *DSPAParams) SetupDBParams(ctx context.Context, dsp *dspa.DataSciencePip Key: config.DefaultDBSecretKey, } } - err := p.GenerateDBSecret(ctx, client, p.DBConnection.CredentialsSecret, log) + dbPassword, err := p.RetrieveOrCreateDBSecret(ctx, client, p.DBConnection.CredentialsSecret, log) if err != nil { return err } + p.DBConnection.Password = dbPassword } if p.DBConnection.Password == "" { return fmt.Errorf(fmt.Sprintf("DB Password from secret [%s] for key [%s] was not successfully retrieved, "+ @@ -323,10 +316,12 @@ func (p *DSPAParams) SetupObjectParams(ctx context.Context, dsp *dspa.DataScienc SecretKey: config.DefaultObjectStorageSecretKey, } } - err := p.GenerateObjectStoreSecret(ctx, client, p.ObjectStorageConnection.CredentialsSecret, log) + accessKey, secretKey, err := p.RetrieveOrCreateObjectStoreSecret(ctx, client, p.ObjectStorageConnection.CredentialsSecret, log) if err != nil { return err } + p.ObjectStorageConnection.AccessKeyID = accessKey + p.ObjectStorageConnection.SecretAccessKey = secretKey } endpoint := fmt.Sprintf( From 755700ae4a4eb314120fb56c95bfe8744332a551 Mon Sep 17 00:00:00 2001 From: Giulio Frasca Date: Tue, 5 Sep 2023 16:51:19 -0400 Subject: [PATCH 08/13] Remove DevTools CRD item - Not useful as Minio/MariaDB params will now generate their secrets automatically if they are missing, so removing before the option hits production and needs to be accounted for in the API --- api/v1alpha1/dspipeline_types.go | 12 ----- api/v1alpha1/zz_generated.deepcopy.go | 20 ------- ...b.io_datasciencepipelinesapplications.yaml | 11 ---- controllers/developer_tools.go | 53 ------------------- controllers/dspipeline_controller.go | 5 -- controllers/dspipeline_params.go | 2 - .../declarative/case_3/deploy/02_cr.yaml | 3 -- 7 files changed, 106 deletions(-) delete mode 100644 controllers/developer_tools.go diff --git a/api/v1alpha1/dspipeline_types.go b/api/v1alpha1/dspipeline_types.go index d9a24286..d8922956 100644 --- a/api/v1alpha1/dspipeline_types.go +++ b/api/v1alpha1/dspipeline_types.go @@ -43,9 +43,6 @@ type DSPASpec struct { // +kubebuilder:validation:Optional // +kubebuilder:default:={deploy: false} *MLMD `json:"mlmd"` - // DevTools enables non-production tools and utilities for Data Science Pipelines Operator developers - // +kubebuilder:validation:Optional - *DevTools `json:"devtools"` } type APIServer struct { @@ -270,15 +267,6 @@ type Writer struct { Image string `json:"image"` } -type DevTools struct { - // +kubebuilder:default:=false - // +kubebuilder:validation:Optional - EnableDatabaseSecret bool `json:"enableDatabaseSecret"` - // +kubebuilder:default:=false - // +kubebuilder:validation:Optional - EnableStorageSecret bool `json:"enableStorageSecret"` -} - // ResourceRequirements structures compute resource requirements. // Replaces ResourceRequirements from corev1 which also includes optional storage field. // We handle storage field separately, and should not include it as a subfield for Resources. diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index a0c003b7..864bddb5 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -124,11 +124,6 @@ func (in *DSPASpec) DeepCopyInto(out *DSPASpec) { *out = new(MLMD) (*in).DeepCopyInto(*out) } - if in.DevTools != nil { - in, out := &in.DevTools, &out.DevTools - *out = new(DevTools) - **out = **in - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DSPASpec. @@ -247,21 +242,6 @@ func (in *Database) DeepCopy() *Database { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *DevTools) DeepCopyInto(out *DevTools) { - *out = *in -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DevTools. -func (in *DevTools) DeepCopy() *DevTools { - if in == nil { - return nil - } - out := new(DevTools) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Envoy) DeepCopyInto(out *Envoy) { *out = *in diff --git a/config/crd/bases/datasciencepipelinesapplications.opendatahub.io_datasciencepipelinesapplications.yaml b/config/crd/bases/datasciencepipelinesapplications.opendatahub.io_datasciencepipelinesapplications.yaml index 2a6c521e..5bbb80be 100644 --- a/config/crd/bases/datasciencepipelinesapplications.opendatahub.io_datasciencepipelinesapplications.yaml +++ b/config/crd/bases/datasciencepipelinesapplications.opendatahub.io_datasciencepipelinesapplications.yaml @@ -287,17 +287,6 @@ spec: type: string type: object type: object - devtools: - description: DevTools enables non-production tools and utilities for - Data Science Pipelines Operator developers - properties: - enableDatabaseSecret: - default: false - type: boolean - enableStorageSecret: - default: false - type: boolean - type: object mlmd: default: deploy: false diff --git a/controllers/developer_tools.go b/controllers/developer_tools.go deleted file mode 100644 index b74c4ab1..00000000 --- a/controllers/developer_tools.go +++ /dev/null @@ -1,53 +0,0 @@ -/* - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package controllers - -import ( - dspav1alpha1 "github.com/opendatahub-io/data-science-pipelines-operator/api/v1alpha1" -) - -const devDBSecret = "devtools/database.secret.yaml.tmpl" -const devStorageSecret = "devtools/storage.secret.yaml.tmpl" - -func (r *DSPAReconciler) ReconcileDevtools(dsp *dspav1alpha1.DataSciencePipelinesApplication, params *DSPAParams) error { - - log := r.Log.WithValues("namespace", dsp.Namespace).WithValues("dspa_name", dsp.Name) - - if dsp.Spec.DevTools != nil { - log.Info("Applying DevTool Resources") - - dbSecretEnabled := dsp.Spec.DevTools.EnableDatabaseSecret - storageSecretEnabled := dsp.Spec.DevTools.EnableStorageSecret - - if dbSecretEnabled { - log.Info("Database secret creation requested") - err := r.Apply(dsp, params, devDBSecret) - if err != nil { - return err - } - } - - if storageSecretEnabled { - log.Info("Object Storage secret creation requested") - err := r.Apply(dsp, params, devStorageSecret) - if err != nil { - return err - } - } - log.Info("Finished applying DevTool Resources") - } - return nil -} diff --git a/controllers/dspipeline_controller.go b/controllers/dspipeline_controller.go index 2cdd8116..d92275ab 100644 --- a/controllers/dspipeline_controller.go +++ b/controllers/dspipeline_controller.go @@ -261,11 +261,6 @@ func (r *DSPAReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. } } - err = r.ReconcileDevtools(dspa, params) - if err != nil { - return ctrl.Result{}, err - } - log.Info("Updating CR status") // Refresh DSPA before updating err = r.Get(ctx, req.NamespacedName, dspa) diff --git a/controllers/dspipeline_params.go b/controllers/dspipeline_params.go index 8739cc77..a4180888 100644 --- a/controllers/dspipeline_params.go +++ b/controllers/dspipeline_params.go @@ -54,7 +54,6 @@ type DSPAParams struct { MariaDB *dspa.MariaDB Minio *dspa.Minio MLMD *dspa.MLMD - DevTools *dspa.DevTools DBConnection ObjectStorageConnection } @@ -411,7 +410,6 @@ func (p *DSPAParams) ExtractParams(ctx context.Context, dsp *dspa.DataSciencePip p.MlPipelineUI = dsp.Spec.MlPipelineUI.DeepCopy() p.MariaDB = dsp.Spec.Database.MariaDB.DeepCopy() p.Minio = dsp.Spec.ObjectStorage.Minio.DeepCopy() - p.DevTools = dsp.Spec.DevTools.DeepCopy() p.OAuthProxy = config.GetStringConfigWithDefault(config.OAuthProxyImagePath, config.DefaultImageValue) p.MLMD = dsp.Spec.MLMD.DeepCopy() p.APIServerPiplinesCABundleMountPath = config.APIServerPiplinesCABundleMountPath diff --git a/controllers/testdata/declarative/case_3/deploy/02_cr.yaml b/controllers/testdata/declarative/case_3/deploy/02_cr.yaml index 4122613f..0b63880e 100644 --- a/controllers/testdata/declarative/case_3/deploy/02_cr.yaml +++ b/controllers/testdata/declarative/case_3/deploy/02_cr.yaml @@ -34,6 +34,3 @@ spec: mlpipelineUI: deploy: false image: frontend:test3 - devtools: - enableDatabaseSecret: true - enableStorageSecret: true From d8c9e5d7e891ae7273b66d24cc340841e5efa311 Mon Sep 17 00:00:00 2001 From: Giulio Frasca Date: Tue, 5 Sep 2023 17:14:22 -0400 Subject: [PATCH 09/13] Fix Functional Tests to handle Secret Creation Behavior changes --- .../case_0/expected/created/apiserver_deployment.yaml | 4 ++-- .../case_2/expected/created/apiserver_deployment.yaml | 4 ++-- .../declarative/case_2/expected/created/minio_deployment.yaml | 4 ++-- .../case_2/expected/created/mlpipelines-ui_deployment.yaml | 4 ++-- .../case_3/expected/not_created/storage_secret.yaml | 2 +- .../case_4/expected/created/apiserver_deployment.yaml | 4 ++-- .../declarative/case_4/expected/created/minio_deployment.yaml | 4 ++-- .../case_4/expected/created/mlpipelines-ui_deployment.yaml | 4 ++-- .../case_5/expected/created/apiserver_deployment.yaml | 4 ++-- .../case_5/expected/created/mlpipelines-ui_deployment.yaml | 4 ++-- 10 files changed, 19 insertions(+), 19 deletions(-) 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 fa277a79..dfa69e0b 100644 --- a/controllers/testdata/declarative/case_0/expected/created/apiserver_deployment.yaml +++ b/controllers/testdata/declarative/case_0/expected/created/apiserver_deployment.yaml @@ -78,12 +78,12 @@ spec: valueFrom: secretKeyRef: key: "accesskey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp0" - name: OBJECTSTORECONFIG_SECRETACCESSKEY valueFrom: secretKeyRef: key: "secretkey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp0" - name: OBJECTSTORECONFIG_SECURE value: "false" - name: MINIO_SERVICE_SERVICE_HOST 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 5c126382..95aa95ce 100644 --- a/controllers/testdata/declarative/case_2/expected/created/apiserver_deployment.yaml +++ b/controllers/testdata/declarative/case_2/expected/created/apiserver_deployment.yaml @@ -78,12 +78,12 @@ spec: valueFrom: secretKeyRef: key: "accesskey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp2" - name: OBJECTSTORECONFIG_SECRETACCESSKEY valueFrom: secretKeyRef: key: "secretkey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp2" - name: OBJECTSTORECONFIG_SECURE value: "false" - name: MINIO_SERVICE_SERVICE_HOST diff --git a/controllers/testdata/declarative/case_2/expected/created/minio_deployment.yaml b/controllers/testdata/declarative/case_2/expected/created/minio_deployment.yaml index 8bf87f74..c3150158 100644 --- a/controllers/testdata/declarative/case_2/expected/created/minio_deployment.yaml +++ b/controllers/testdata/declarative/case_2/expected/created/minio_deployment.yaml @@ -31,12 +31,12 @@ spec: valueFrom: secretKeyRef: key: "accesskey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp2" - name: MINIO_SECRET_KEY valueFrom: secretKeyRef: key: "secretkey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp2" image: minio:test2 name: minio ports: diff --git a/controllers/testdata/declarative/case_2/expected/created/mlpipelines-ui_deployment.yaml b/controllers/testdata/declarative/case_2/expected/created/mlpipelines-ui_deployment.yaml index 4430c650..2b3e5d1c 100644 --- a/controllers/testdata/declarative/case_2/expected/created/mlpipelines-ui_deployment.yaml +++ b/controllers/testdata/declarative/case_2/expected/created/mlpipelines-ui_deployment.yaml @@ -35,12 +35,12 @@ spec: valueFrom: secretKeyRef: key: "accesskey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp2" - name: MINIO_SECRET_KEY valueFrom: secretKeyRef: key: "secretkey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp2" - name: ALLOW_CUSTOM_VISUALIZATIONS value: "true" - name: ARGO_ARCHIVE_LOGS diff --git a/controllers/testdata/declarative/case_3/expected/not_created/storage_secret.yaml b/controllers/testdata/declarative/case_3/expected/not_created/storage_secret.yaml index a1baf725..6cc5af7a 100644 --- a/controllers/testdata/declarative/case_3/expected/not_created/storage_secret.yaml +++ b/controllers/testdata/declarative/case_3/expected/not_created/storage_secret.yaml @@ -2,7 +2,7 @@ kind: Secret apiVersion: v1 metadata: # todo: remove todo- this should actually be checked for but causes failures because previous tests don't clean up properly - name: todo-mlpipeline-minio-artifact + name: todo-ds-pipeline-s3-testdsp3 namespace: namespace3 labels: app: minio-extrenal-storage 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 94524294..f91af974 100644 --- a/controllers/testdata/declarative/case_4/expected/created/apiserver_deployment.yaml +++ b/controllers/testdata/declarative/case_4/expected/created/apiserver_deployment.yaml @@ -78,12 +78,12 @@ spec: valueFrom: secretKeyRef: key: "accesskey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp4" - name: OBJECTSTORECONFIG_SECRETACCESSKEY valueFrom: secretKeyRef: key: "secretkey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp4" - name: OBJECTSTORECONFIG_SECURE value: "false" - name: MINIO_SERVICE_SERVICE_HOST diff --git a/controllers/testdata/declarative/case_4/expected/created/minio_deployment.yaml b/controllers/testdata/declarative/case_4/expected/created/minio_deployment.yaml index df569957..0ea9304b 100644 --- a/controllers/testdata/declarative/case_4/expected/created/minio_deployment.yaml +++ b/controllers/testdata/declarative/case_4/expected/created/minio_deployment.yaml @@ -31,12 +31,12 @@ spec: valueFrom: secretKeyRef: key: "accesskey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp4" - name: MINIO_SECRET_KEY valueFrom: secretKeyRef: key: "secretkey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp4" image: this-minio-image-from-cr-should-be-used:test4 name: minio ports: diff --git a/controllers/testdata/declarative/case_4/expected/created/mlpipelines-ui_deployment.yaml b/controllers/testdata/declarative/case_4/expected/created/mlpipelines-ui_deployment.yaml index 131b3cca..15a5bdb5 100644 --- a/controllers/testdata/declarative/case_4/expected/created/mlpipelines-ui_deployment.yaml +++ b/controllers/testdata/declarative/case_4/expected/created/mlpipelines-ui_deployment.yaml @@ -35,12 +35,12 @@ spec: valueFrom: secretKeyRef: key: "accesskey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp4" - name: MINIO_SECRET_KEY valueFrom: secretKeyRef: key: "secretkey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp4" - name: ALLOW_CUSTOM_VISUALIZATIONS value: "true" - name: ARGO_ARCHIVE_LOGS 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 92f6ac5b..a34471f7 100644 --- a/controllers/testdata/declarative/case_5/expected/created/apiserver_deployment.yaml +++ b/controllers/testdata/declarative/case_5/expected/created/apiserver_deployment.yaml @@ -78,12 +78,12 @@ spec: valueFrom: secretKeyRef: key: "accesskey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp5" - name: OBJECTSTORECONFIG_SECRETACCESSKEY valueFrom: secretKeyRef: key: "secretkey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp5" - name: OBJECTSTORECONFIG_SECURE value: "false" - name: MINIO_SERVICE_SERVICE_HOST diff --git a/controllers/testdata/declarative/case_5/expected/created/mlpipelines-ui_deployment.yaml b/controllers/testdata/declarative/case_5/expected/created/mlpipelines-ui_deployment.yaml index da690063..5d8c14e0 100644 --- a/controllers/testdata/declarative/case_5/expected/created/mlpipelines-ui_deployment.yaml +++ b/controllers/testdata/declarative/case_5/expected/created/mlpipelines-ui_deployment.yaml @@ -35,12 +35,12 @@ spec: valueFrom: secretKeyRef: key: "accesskey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp5" - name: MINIO_SECRET_KEY valueFrom: secretKeyRef: key: "secretkey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp5" - name: ALLOW_CUSTOM_VISUALIZATIONS value: "true" - name: ARGO_ARCHIVE_LOGS From 84be90ceaf5e5702803d5d728f021c0abfe75fce Mon Sep 17 00:00:00 2001 From: Giulio Frasca Date: Wed, 13 Sep 2023 17:19:45 -0400 Subject: [PATCH 10/13] Propogate unhandled errors when fetching Secrets for DB/ObjStore creds --- controllers/dspipeline_params.go | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/controllers/dspipeline_params.go b/controllers/dspipeline_params.go index a4180888..ab197d20 100644 --- a/controllers/dspipeline_params.go +++ b/controllers/dspipeline_params.go @@ -190,8 +190,12 @@ func (p *DSPAParams) SetupDBParams(ctx context.Context, dsp *dspa.DataSciencePip p.DBConnection.DBName = dsp.Spec.Database.ExternalDB.DBName p.DBConnection.CredentialsSecret = dsp.Spec.Database.ExternalDB.PasswordSecret - // Retreive DB Password from specified secret. Handle errors (empty password) later - password, _ := p.RetrieveSecret(ctx, client, p.DBConnection.CredentialsSecret.Name, p.DBConnection.CredentialsSecret.Key, log) + // Retreive DB Password from specified secret. Ignore error if the secret simply doesn't exist (will be created later) + password, err := p.RetrieveSecret(ctx, client, p.DBConnection.CredentialsSecret.Name, p.DBConnection.CredentialsSecret.Key, log) + if err != nil && !apierrs.IsNotFound(err) { + log.Error(err, "Unexpected error encountered while fetching Database Secret") + return err + } p.DBConnection.Password = password } else { // If no externalDB or mariaDB is specified, DSPO assumes @@ -273,9 +277,17 @@ func (p *DSPAParams) SetupObjectParams(ctx context.Context, dsp *dspa.DataScienc p.ObjectStorageConnection.Port = dsp.Spec.ObjectStorage.ExternalStorage.Port p.ObjectStorageConnection.CredentialsSecret = dsp.Spec.ObjectStorage.ExternalStorage.S3CredentialSecret - // Retrieve ObjStore Creds from specified secret. Handle issues (empty creds) later. - accesskey, _ := p.RetrieveSecret(ctx, client, p.ObjectStorageConnection.CredentialsSecret.SecretName, p.ObjectStorageConnection.CredentialsSecret.AccessKey, log) - secretkey, _ := p.RetrieveSecret(ctx, client, p.ObjectStorageConnection.CredentialsSecret.SecretName, p.ObjectStorageConnection.CredentialsSecret.SecretKey, log) + // Retrieve ObjStore Creds from specified secret. Ignore error if the secret simply doesn't exist (will be created later) + accesskey, err := p.RetrieveSecret(ctx, client, p.ObjectStorageConnection.CredentialsSecret.SecretName, p.ObjectStorageConnection.CredentialsSecret.AccessKey, log) + if err != nil && !apierrs.IsNotFound(err) { + log.Error(err, "Unexpected error encountered while fetching Object Storage Secret") + return err + } + secretkey, err := p.RetrieveSecret(ctx, client, p.ObjectStorageConnection.CredentialsSecret.SecretName, p.ObjectStorageConnection.CredentialsSecret.SecretKey, log) + if err != nil && !apierrs.IsNotFound(err) { + log.Error(err, "Unexpected error encountered while fetching Object Storage Secret") + return err + } p.ObjectStorageConnection.AccessKeyID = accesskey p.ObjectStorageConnection.SecretAccessKey = secretkey } else { From be46a1bf9f83950a51acfccd0b1de9600a058479 Mon Sep 17 00:00:00 2001 From: Giulio Frasca Date: Fri, 15 Sep 2023 15:43:17 -0400 Subject: [PATCH 11/13] Pass ObjStore Credential Metadata EnvVars to APIServer - Allows DSP APIServer to use credentials found in secrets other than 'mlpipeline-minio-artifact' --- config/internal/apiserver/deployment.yaml.tmpl | 6 ++++++ .../case_0/expected/created/apiserver_deployment.yaml | 6 ++++++ .../case_2/expected/created/apiserver_deployment.yaml | 6 ++++++ .../case_3/expected/created/apiserver_deployment.yaml | 6 ++++++ .../case_4/expected/created/apiserver_deployment.yaml | 6 ++++++ .../case_5/expected/created/apiserver_deployment.yaml | 6 ++++++ 6 files changed, 36 insertions(+) diff --git a/config/internal/apiserver/deployment.yaml.tmpl b/config/internal/apiserver/deployment.yaml.tmpl index ae0d064f..47fe06b7 100644 --- a/config/internal/apiserver/deployment.yaml.tmpl +++ b/config/internal/apiserver/deployment.yaml.tmpl @@ -80,6 +80,12 @@ spec: value: "ds-pipeline-visualizationserver" - name: ML_PIPELINE_VISUALIZATIONSERVER_SERVICE_PORT value: "8888" + - name: OBJECTSTORECONFIG_CREDENTIALSSECRET + value: "{{.ObjectStorageConnection.CredentialsSecret.SecretName}}" + - name: OBJECTSTORECONFIG_CREDENTIALSACCESSKEYKEY + value: "{{.ObjectStorageConnection.CredentialsSecret.AccessKey}}" + - name: OBJECTSTORECONFIG_CREDENTIALSSECRETKEYKEY + value: "{{.ObjectStorageConnection.CredentialsSecret.SecretKey}}" - name: OBJECTSTORECONFIG_BUCKETNAME value: "{{.ObjectStorageConnection.Bucket}}" - name: OBJECTSTORECONFIG_ACCESSKEY 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 dfa69e0b..23d162fc 100644 --- a/controllers/testdata/declarative/case_0/expected/created/apiserver_deployment.yaml +++ b/controllers/testdata/declarative/case_0/expected/created/apiserver_deployment.yaml @@ -72,6 +72,12 @@ spec: value: "ds-pipeline-visualizationserver" - name: ML_PIPELINE_VISUALIZATIONSERVER_SERVICE_PORT value: "8888" + - name: OBJECTSTORECONFIG_CREDENTIALSSECRET + value: "ds-pipeline-s3-testdsp0" + - name: OBJECTSTORECONFIG_CREDENTIALSACCESSKEYKEY + value: "accesskey" + - name: OBJECTSTORECONFIG_CREDENTIALSSECRETKEYKEY + value: "secretkey" - name: OBJECTSTORECONFIG_BUCKETNAME value: "mlpipeline" - name: OBJECTSTORECONFIG_ACCESSKEY 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 95aa95ce..3376a4c2 100644 --- a/controllers/testdata/declarative/case_2/expected/created/apiserver_deployment.yaml +++ b/controllers/testdata/declarative/case_2/expected/created/apiserver_deployment.yaml @@ -72,6 +72,12 @@ spec: value: "ds-pipeline-visualizationserver" - name: ML_PIPELINE_VISUALIZATIONSERVER_SERVICE_PORT value: "8888" + - name: OBJECTSTORECONFIG_CREDENTIALSSECRET + value: "ds-pipeline-s3-testdsp2" + - name: OBJECTSTORECONFIG_CREDENTIALSACCESSKEYKEY + value: "accesskey" + - name: OBJECTSTORECONFIG_CREDENTIALSSECRETKEYKEY + value: "secretkey" - name: OBJECTSTORECONFIG_BUCKETNAME value: "mlpipeline" - name: OBJECTSTORECONFIG_ACCESSKEY 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 25fafd5f..e87aa42e 100644 --- a/controllers/testdata/declarative/case_3/expected/created/apiserver_deployment.yaml +++ b/controllers/testdata/declarative/case_3/expected/created/apiserver_deployment.yaml @@ -72,6 +72,12 @@ spec: value: "ds-pipeline-visualizationserver" - name: ML_PIPELINE_VISUALIZATIONSERVER_SERVICE_PORT value: "8888" + - name: OBJECTSTORECONFIG_CREDENTIALSSECRET + value: "teststoragesecretname3" + - name: OBJECTSTORECONFIG_CREDENTIALSACCESSKEYKEY + value: "testaccesskey3" + - name: OBJECTSTORECONFIG_CREDENTIALSSECRETKEYKEY + value: "testsecretkey3" - name: OBJECTSTORECONFIG_BUCKETNAME value: "testbucket3" - name: OBJECTSTORECONFIG_ACCESSKEY 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 f91af974..04e0af2e 100644 --- a/controllers/testdata/declarative/case_4/expected/created/apiserver_deployment.yaml +++ b/controllers/testdata/declarative/case_4/expected/created/apiserver_deployment.yaml @@ -72,6 +72,12 @@ spec: value: "ds-pipeline-visualizationserver" - name: ML_PIPELINE_VISUALIZATIONSERVER_SERVICE_PORT value: "8888" + - name: OBJECTSTORECONFIG_CREDENTIALSSECRET + value: "ds-pipeline-s3-testdsp4" + - name: OBJECTSTORECONFIG_CREDENTIALSACCESSKEYKEY + value: "accesskey" + - name: OBJECTSTORECONFIG_CREDENTIALSSECRETKEYKEY + value: "secretkey" - name: OBJECTSTORECONFIG_BUCKETNAME value: "mlpipeline" - name: OBJECTSTORECONFIG_ACCESSKEY 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 a34471f7..90c68bdb 100644 --- a/controllers/testdata/declarative/case_5/expected/created/apiserver_deployment.yaml +++ b/controllers/testdata/declarative/case_5/expected/created/apiserver_deployment.yaml @@ -72,6 +72,12 @@ spec: value: "ds-pipeline-visualizationserver" - name: ML_PIPELINE_VISUALIZATIONSERVER_SERVICE_PORT value: "8888" + - name: OBJECTSTORECONFIG_CREDENTIALSSECRET + value: "ds-pipeline-s3-testdsp5" + - name: OBJECTSTORECONFIG_CREDENTIALSACCESSKEYKEY + value: "accesskey" + - name: OBJECTSTORECONFIG_CREDENTIALSSECRETKEYKEY + value: "secretkey" - name: OBJECTSTORECONFIG_BUCKETNAME value: "mlpipeline" - name: OBJECTSTORECONFIG_ACCESSKEY From c8835b87ee8669e65142f9adb9182abb8cc72a63 Mon Sep 17 00:00:00 2001 From: Giulio Frasca Date: Fri, 3 Nov 2023 18:41:50 -0400 Subject: [PATCH 12/13] Only use managed DB/Storage Secret if not specified in DSPA --- controllers/database.go | 12 +++++++++++- controllers/storage.go | 14 +++++++++++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/controllers/database.go b/controllers/database.go index b95b7367..5ba313f4 100644 --- a/controllers/database.go +++ b/controllers/database.go @@ -20,6 +20,7 @@ import ( "database/sql" b64 "encoding/base64" "fmt" + _ "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" @@ -32,7 +33,6 @@ var mariadbTemplates = []string{ "mariadb/pvc.yaml.tmpl", "mariadb/service.yaml.tmpl", "mariadb/mariadb-sa.yaml.tmpl", - dbSecret, } // extract to var for mocking in testing @@ -100,10 +100,20 @@ func (r *DSPAReconciler) ReconcileDatabase(ctx context.Context, dsp *dspav1alpha // Default DB is currently MariaDB as well, but storing these bools seperately in case that changes deployDefaultDB := !databaseSpecified || defaultDBRequired + externalDBCredentialsProvided := externalDBSpecified && (dsp.Spec.Database.ExternalDB.PasswordSecret != nil) + mariaDBCredentialsProvided := mariaDBSpecified && (dsp.Spec.Database.MariaDB.PasswordSecret != nil) + databaseCredentialsProvided := externalDBCredentialsProvided || mariaDBCredentialsProvided + // If external db is specified, it takes precedence if externalDBSpecified { log.Info("Using externalDB, bypassing database deployment.") } else if deployMariaDB || deployDefaultDB { + if !databaseCredentialsProvided { + err := r.Apply(dsp, params, dbSecret) + if err != nil { + return err + } + } log.Info("Applying mariaDB resources.") for _, template := range mariadbTemplates { err := r.Apply(dsp, params, template) diff --git a/controllers/storage.go b/controllers/storage.go index a3ceba70..69b38c35 100644 --- a/controllers/storage.go +++ b/controllers/storage.go @@ -22,6 +22,8 @@ import ( "encoding/base64" "errors" "fmt" + "net/http" + "github.com/go-logr/logr" "github.com/minio/minio-go/v7" "github.com/minio/minio-go/v7/pkg/credentials" @@ -38,7 +40,6 @@ var minioTemplates = []string{ "minio/pvc.yaml.tmpl", "minio/service.yaml.tmpl", "minio/minio-sa.yaml.tmpl", - storageSecret, } func joinHostPort(host, port string) (string, error) { @@ -195,10 +196,21 @@ func (r *DSPAReconciler) ReconcileStorage(ctx context.Context, dsp *dspav1alpha1 minioSpecified := !storageSpecified || dsp.Spec.ObjectStorage.Minio != nil deployMinio := !storageSpecified || (minioSpecified && dsp.Spec.ObjectStorage.Minio.Deploy) + externalStorageCredentialsProvided := externalStorageSpecified && (dsp.Spec.ObjectStorage.ExternalStorage.S3CredentialSecret != nil) + minioCredentialsProvided := minioSpecified && (dsp.Spec.ObjectStorage.Minio.S3CredentialSecret != nil) + storageCredentialsProvided := externalStorageCredentialsProvided || minioCredentialsProvided + // If external storage is specified, it takes precedence if externalStorageSpecified { log.Info("Using externalStorage, bypassing object storage deployment.") } else if deployMinio { + log.Info("No S3 storage credential reference provided, so using managed secret") + if !storageCredentialsProvided { + err := r.Apply(dsp, params, storageSecret) + if err != nil { + return err + } + } log.Info("Applying object storage resources.") for _, template := range minioTemplates { err := r.Apply(dsp, params, template) From 4e4c3fade775ba7eff539e43aceab7b1d4d71f02 Mon Sep 17 00:00:00 2001 From: Giulio Frasca Date: Tue, 7 Nov 2023 11:19:22 -0500 Subject: [PATCH 13/13] Fix Unit and Functional Tests --- controllers/storage.go | 1 - .../case_6/expected/created/apiserver_deployment.yaml | 10 ++++++++-- .../expected/created/configmap_artifact_script.yaml | 2 +- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/controllers/storage.go b/controllers/storage.go index 69b38c35..a03b3e06 100644 --- a/controllers/storage.go +++ b/controllers/storage.go @@ -30,7 +30,6 @@ import ( dspav1alpha1 "github.com/opendatahub-io/data-science-pipelines-operator/api/v1alpha1" "github.com/opendatahub-io/data-science-pipelines-operator/controllers/config" "github.com/opendatahub-io/data-science-pipelines-operator/controllers/util" - "net/http" ) const storageSecret = "minio/secret.yaml.tmpl" 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 96c5967e..897a3a3b 100644 --- a/controllers/testdata/declarative/case_6/expected/created/apiserver_deployment.yaml +++ b/controllers/testdata/declarative/case_6/expected/created/apiserver_deployment.yaml @@ -78,18 +78,24 @@ spec: value: "ds-pipeline-visualizationserver" - name: ML_PIPELINE_VISUALIZATIONSERVER_SERVICE_PORT value: "8888" + - name: OBJECTSTORECONFIG_CREDENTIALSSECRET + value: "ds-pipeline-s3-testdsp6" + - name: OBJECTSTORECONFIG_CREDENTIALSACCESSKEYKEY + value: "accesskey" + - name: OBJECTSTORECONFIG_CREDENTIALSSECRETKEYKEY + value: "secretkey" - name: OBJECTSTORECONFIG_BUCKETNAME value: "mlpipeline" - name: OBJECTSTORECONFIG_ACCESSKEY valueFrom: secretKeyRef: key: "accesskey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp6" - name: OBJECTSTORECONFIG_SECRETACCESSKEY valueFrom: secretKeyRef: key: "secretkey" - name: "mlpipeline-minio-artifact" + name: "ds-pipeline-s3-testdsp6" - name: OBJECTSTORECONFIG_SECURE value: "false" - name: MINIO_SERVICE_SERVICE_HOST diff --git a/controllers/testdata/declarative/case_6/expected/created/configmap_artifact_script.yaml b/controllers/testdata/declarative/case_6/expected/created/configmap_artifact_script.yaml index 2cbb8402..60baaf58 100644 --- a/controllers/testdata/declarative/case_6/expected/created/configmap_artifact_script.yaml +++ b/controllers/testdata/declarative/case_6/expected/created/configmap_artifact_script.yaml @@ -38,5 +38,5 @@ metadata: name: ds-pipeline-artifact-script-testdsp6 namespace: default labels: - app: ds-pipeline-testdsp5 + app: ds-pipeline-testdsp6 component: data-science-pipelines