diff --git a/config/internal/mariadb/secret.yaml.tmpl b/config/internal/mariadb/secret.yaml.tmpl index 62a9b4d2b..3293bc703 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 17192f27f..e0cf555c1 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 fdf841b9d..8e8773d78 100644 --- a/controllers/config/defaults.go +++ b/controllers/config/defaults.go @@ -30,8 +30,8 @@ const ( ArtifactScriptConfigMapKey = "artifact_script" DSPServicePrefix = "ds-pipeline" - DBSecretNamePrefix = "ds-pipeline-db-" - DBSecretKey = "password" + DefaultDBSecretNamePrefix = "ds-pipeline-db-" + DefaultDBSecretKey = "password" MariaDBName = "mlpipeline" MariaDBHostPrefix = "mariadb" @@ -45,9 +45,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 bf47afc8c..37a736c0e 100644 --- a/controllers/database.go +++ b/controllers/database.go @@ -21,12 +21,14 @@ import ( dspav1alpha1 "github.com/opendatahub-io/data-science-pipelines-operator/api/v1alpha1" ) +const dbSecret = "mariadb/secret.yaml.tmpl" + var mariadbTemplates = []string{ "mariadb/deployment.yaml.tmpl", "mariadb/pvc.yaml.tmpl", "mariadb/sa.yaml.tmpl", "mariadb/service.yaml.tmpl", - "mariadb/secret.yaml.tmpl", + dbSecret, } func (r *DSPAReconciler) ReconcileDatabase(ctx context.Context, dsp *dspav1alpha1.DataSciencePipelinesApplication, diff --git a/controllers/developer_tools.go b/controllers/developer_tools.go index 4031b83be..b74c4ab13 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 349c3ef84..b21a1df84 100644 --- a/controllers/dspipeline_params.go +++ b/controllers/dspipeline_params.go @@ -106,28 +106,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. @@ -141,6 +192,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 == "" { @@ -158,59 +210,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 } @@ -221,16 +238,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 @@ -239,7 +246,13 @@ func (p *DSPAParams) SetupObjectParams(ctx context.Context, dsp *dspa.DataScienc p.ObjectStorageConnection.Secure = dsp.Spec.ObjectStorage.ExternalStorage.Secure // 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] " + @@ -267,7 +280,17 @@ func (p *DSPAParams) SetupObjectParams(ctx context.Context, dsp *dspa.DataScienc p.ObjectStorageConnection.Port = config.MinioPort p.ObjectStorageConnection.Scheme = config.MinioScheme 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 } } @@ -287,62 +310,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 2ae811579..ef3e257ee 100644 --- a/controllers/storage.go +++ b/controllers/storage.go @@ -22,11 +22,14 @@ import ( dspav1alpha1 "github.com/opendatahub-io/data-science-pipelines-operator/api/v1alpha1" ) +const storageSecret = "minio/secret.yaml.tmpl" + var minioTemplates = []string{ "minio/deployment.yaml.tmpl", "minio/pvc.yaml.tmpl", "minio/service.yaml.tmpl", "minio/secret.yaml.tmpl", + storageSecret, } // ReconcileStorage will set up Storage Connection. diff --git a/controllers/testdata/declarative/case_3/expected/created/apiserver_deployment.yaml b/controllers/testdata/declarative/case_3/expected/created/apiserver_deployment.yaml index 0b617788d..25fafd5fb 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 9f2630148..000000000 --- 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 000000000..ec3365d45 --- /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 36d77f689..a1baf725b 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