Remove External Connection Secrets from operator responsibilities #118

gmfrasca · 2023-05-15T21:34:49Z

Resolves #151

DSPO should not be responsible for maintaining the connections secrets for an external database or object store. Remove this from the maintained artifacts list.

Description

DSPO should not be responsible for maintaining the connections secrets for an external database or object store. Remove this from the maintained artifacts list.

How Has This Been Tested?

For all tests, assume DSPA is named sample

There are essentially 9 test cases/DSPAs here:

** Default Values Test **

both Default MariaDB and Minio options [expected: will create ds-pipline-db-sample and ds-pipeline-s3-sample secrets]

** Object Storage Tests **

DSPA with spec.objectStorage.minio.s3CredentialsSecret specified but the secret specified in secretName does not exist yet [expected: DSPO will create this secret and generate random credentials for the accesskey and secretkey]
DSPA with spec.objectStorage.minio.s3CredentialsSecret specified and the secret specified in secretName already exists [expected: DSPO will create Minio pod but reuse values found in this secret]
DSPA with spec.objectStorage.externalStorage.s3CredentialsSecret specified but the secret specified in secretName does not exist yet [expected: DSPO will not spin up any pods, and will return an error]
DSPA with spec.objectStorage.externalStorage.s3CredentialsSecret specified and the secret specified in secretName already exists [expected: DSPO will spin up a DSPA and successfully connect to the External S3 Storage]

** Database Tests **

DSPA with spec.database.mariaDB.passwordSecret specified but the secret specified in name does not exist yet [expected: DSPO will create this secret and generate random credentials for the password]
DSPA with spec.database.mariaDB.passwordSecret specified and the secret specified in name already exists [expected: DSPO will create MariaDB pod but reuse the password found in this secret]
DSPA with spec.database.externalDB.passwordSecret specified but the secret specified in name does not exist yet [expected: DSPO will not spin up any pods, and will return an error]
DSPA with spec.database.externalDB.passwordSecret specified and the secret specified in name already exists [expected: DSPO will spin up a DSPA and successfully connect to the External Database]

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

gmfrasca · 2023-05-23T20:06:19Z

Secrets used for testing:

ds-pipeline-db-sample:

apiVersion: v1
kind: Secret
type: Opaque
metadata:
  name: ds-pipeline-db-sample
data:
  password: {{ base64-encoded Database password }}

mlpipeline-minio-artifact:

apiVersion: v1
kind: Secret
type: Opaque
metadata:
  name: mlpipeline-minio-artifact
data:
  accesskey: {{ base64-encoded S3 Access Key }}
  host: {{ base64-encoded S3 Endpoint URL }}
  port: {{ base64-encoded S3 Endpoint Port }}
  secretkey: {{ base64-encoded S3 Secret Key }}
  secure: {{ base64-encoded boolean true/false (true if https, false if http) }}

controllers/database.go

controllers/storage.go

HumairAK

Atm this will not work for db because the deployment for apiserver (and kfp ui) will still get the secret it was previously deploying passed in as env vars, the culprit is here

Also, you may want to run through this code now that we are not deploying our own secret any more, as this may need to reflect that change.

HumairAK · 2023-06-06T23:33:40Z

Similarly for this, you will likely need to update this

dsp-developers · 2023-09-05T21:18:19Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-118

dsp-developers · 2023-09-13T19:26:32Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-118

controllers/dspipeline_params.go

dsp-developers · 2023-09-13T21:25:33Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-118

HumairAK · 2023-09-14T19:48:35Z

currently this doesn't work for s3 storage because we are not passing in OBJECTSTORECONFIG_CREDENTIALSSECRET to the api server, so I think you'll need to do that. right now it's still expecting the default secret so pipelines are failing with Error: secret "mlpipeline-minio-artifact" not found

opendatahub-io/data-science-pipelines@8d49200#diff-47d6ab6bca869e0559465170316707ceda42f65103bd0d4e93519294305e62d1R50

controllers/dspipeline_params.go

dsp-developers · 2023-09-15T19:47:51Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-118

dsp-developers · 2023-10-04T16:41:56Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-118

HumairAK · 2023-10-04T21:44:38Z

I have pretty thoroughly tested this and logged the outcomes:
Check marks indicate the expected outcome, unchecks indicate it not meet the outcome.

The default cases I don't really care that much, they seem like more edge cases.

Verification check list:

Standard Test suite (STS):

Simple DSPA deployed successfully
Run Iris Pipeline Successfully
Check S3 storage for artifacts passing/storage
Retrieve completed run (db test)

For the DSPA + Secret at the same time deploy cases, the Secrets are listed after the DSPA in the same yaml file.

sample.yaml

apiVersion: datasciencepipelinesapplications.opendatahub.io/v1alpha1
kind: DataSciencePipelinesApplication
metadata:
  name: case3
  namespace: dspa
spec:
  apiServer:
    image: quay.io/opendatahub/ds-pipelines-api-server@sha256:650f196f212664c5d4bd9a293fcd5fd0dca40e1d5c3af67f57b2e69a436070b2
  database:
    mariaDB:
      passwordSecret:
        key: case3dbpsw
        name: case3-dbsecret
  objectStorage:
    minio:
      image: 'quay.io/opendatahub/minio:RELEASE.2019-08-14T20-37-41Z-license-compliance'
  mlpipelineUI:
    image: 'quay.io/opendatahub/odh-ml-pipelines-frontend-container:beta-ui'
---
kind: Secret
apiVersion: v1
metadata:
  name: case3-dbsecret
stringData:
  case3dbpsw: 
type: Opaque

Default Connections

Deploy without any secrets specified
- Case: Standard Test suite
- Case: All secrets cleaned up on DSPA delete
Deploy custom DB secret specified, Secret first, then DSPA
- Case: Standard Test suite
- Case: DB secret NOT cleaned up on DSPA delete
- Case: DB Secret is specified in DSPA, but not deployed, should throw error
Deploy custom DB secret specified, DSPA + Secret at the same time
- Case: Standard Test suite
Deploy custom S3 secret specified, Secret first, then DSPA
- Constraints: s3 secret name != mlpipeline-minio-artifact
- Case: Standard Test suite
- Case: S3 secret NOT cleaned up on DSPA delete
- Case: S3 Secret is specified in DSPA, but not deployed, should throw error
Deploy custom S3 secret specified, DSPA + Secret at the same time
- Constraints: s3 secret name != mlpipeline-minio-artifact
- Case: Standard Test suite
Deploy custom S3 AND default DB secret specified, Secret first, then DSPA
- Constraints: s3 secret name != mlpipeline-minio-artifact
- Case: Standard Test suite
- Case: S3 secrets NOT cleaned up on DSPA delete

External Connections

Deploy using S3/DB external with custom secrets, Secret first, then DSPA
- Constraints: s3 secret name != mlpipeline-minio-artifact
- Case: Standard Test suite
- Case: Secrets NOT cleaned up on DSPA delete

Deploy using S3/DB external with custom secrets, DSPA + Secret at the same time

Constraints: s3 secret name != mlpipeline-minio-artifact
Case: Standard Test suite
Case: Secrets NOT cleaned up on DSPA delete

Note this case results in the DSPO operator blocking on a thread for a couple of minutes
It then deploys fine.

2023-10-04T19:26:19Z	DEBUG	Unable to retrieve secret [dbsecret].
2023-10-04T19:26:19Z	INFO	Encountered error when parsing CR: [DB Password from secret [dbsecret] for key [customkey] was not successfully retrieved, ensure that the secret with this key exist.]	{"namespace": "dspa", "dspa_name": "case2"}
2023-10-04T19:26:19Z	DEBUG	DataSciencePipelinesApplication Reconciler called.	{"namespace": "dspa", "dspa_name": "case2"}
2023-10-04T19:26:19Z	DEBUG	Unable to retrieve secret [dbsecret].
2023-10-04T19:26:19Z	INFO	Encountered error when parsing CR: [DB Password from secret [dbsecret] for key [customkey] was not successfully retrieved, ensure that the secret with this key exist.]	{"namespace": "dspa", "dspa_name": "case2"}
2023-10-04T19:28:19Z	DEBUG	DataSciencePipelinesApplication Reconciler called.	{"namespace": "dspa", "dspa_name": "case2"}

Deploy using S3/DB external with no secrets
- Case: Simple DSPA fails to deploy
Note this does block a thread ~2min, but with a maxconcurrency count > 1, this is fine
unless n dspas are blocked, and maxconcurrencycount == n

Upgrade testing:

Steps (with default MariaDB):

Install ODH (v1.8)
Install DSPA Kfdef with ODH-Dashboard & DS Pipelines (main)
Create Data Connection + Pipeline Server
Upgrade Kfdef to leverage custom secrets change in DSPO
Confirm Pipelines run successfully
Reboot API Server to confirm connection still works (repull secrets into pod)
Confirm Pipelines run successfully
Confirm artifact storage works in iris example
Confirm Runs are stored in db
Deleted old minio artifact secret
Restarted API Server pod
Confirm Runs still work, artifacts are stored in s3
Confirm deleting pipeline server does not delete data connection secret

Steps (with External DB):

Note: When doing this via odh dashboard the secret will be deleted, but it's the dashboard that deletes it not dspo, you can confirm this by only deleting the dspa cr and not via dashboard.

HumairAK · 2023-10-05T15:30:21Z

We want this check:

 Confirm deleting pipeline server does not delete db secret (created by dashboard)

@gmfrasca I did not confirm the above for the MariaDB case. I'm not sure if ODH Dashboard creates any secrets for the MariaDB default deployment, please confirm this and if dashboard indeed uses their own secret, then we need to handle this too:

Case: DB secret NOT cleaned up on DSPA delete
Case: DB Secret is specified in DSPA, but not deployed, should throw error (not crash)

…sted

- 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

- Allows DSP APIServer to use credentials found in secrets other than 'mlpipeline-minio-artifact'

dsp-developers · 2023-11-07T16:23:07Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-118

dsp-developers · 2023-11-07T17:11:58Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-118

HumairAK · 2023-11-08T17:55:45Z

restested all cases, and confirmed upgrades with odh v2 operator, the dspa seamlessly switches to using the secret created via odh dashboard, it does not clean up the mlpipeline-minio-artifact secret, but it is no longer used

/lgtm
/approve

openshift-ci · 2023-11-08T17:55:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HumairAK

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [HumairAK]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the do-not-merge/work-in-progress label May 15, 2023

openshift-ci bot requested review from DharmitD and gregsheremeta May 15, 2023 21:34

gmfrasca force-pushed the rm-secret-creation branch from 08d41b9 to 563e58d Compare May 23, 2023 19:04

openshift-merge-robot added the needs-rebase label May 23, 2023

gmfrasca force-pushed the rm-secret-creation branch from 563e58d to ecc93f8 Compare May 23, 2023 19:05

openshift-merge-robot removed the needs-rebase label May 23, 2023

gmfrasca changed the title ~~WIP: Remove External Connection Secrets from operator responsibilities~~ Remove External Connection Secrets from operator responsibilities May 23, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label May 23, 2023

HumairAK reviewed Jun 6, 2023

View reviewed changes

controllers/database.go Show resolved Hide resolved

HumairAK reviewed Jun 6, 2023

View reviewed changes

controllers/storage.go Show resolved Hide resolved

HumairAK reviewed Jun 6, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase label Jun 13, 2023

gmfrasca force-pushed the rm-secret-creation branch from ecc93f8 to 24a957a Compare June 14, 2023 20:03

openshift-merge-robot removed the needs-rebase label Jun 14, 2023

HumairAK added the qe/verify Labels to inform qe on issues to verify. label Jul 24, 2023

openshift-merge-robot added the needs-rebase label Jul 29, 2023

gmfrasca force-pushed the rm-secret-creation branch from 746d4ef to db6ce91 Compare September 5, 2023 21:15

openshift-merge-robot removed the needs-rebase label Sep 5, 2023

gmfrasca force-pushed the rm-secret-creation branch from db6ce91 to 3eb5187 Compare September 13, 2023 19:23

HumairAK requested changes Sep 13, 2023

View reviewed changes

controllers/dspipeline_params.go Outdated Show resolved Hide resolved

controllers/dspipeline_params.go Show resolved Hide resolved

controllers/dspipeline_params.go Outdated Show resolved Hide resolved

openshift-ci bot assigned HumairAK Sep 13, 2023

gmfrasca force-pushed the rm-secret-creation branch from cc7d0b8 to bb6a384 Compare September 13, 2023 21:22

HumairAK reviewed Sep 14, 2023

View reviewed changes

controllers/dspipeline_params.go Show resolved Hide resolved

gmfrasca force-pushed the rm-secret-creation branch from a7d01dd to f3070b6 Compare October 4, 2023 16:38

gmfrasca force-pushed the rm-secret-creation branch from f3070b6 to 06b0bc1 Compare November 6, 2023 21:44

openshift-merge-robot added the needs-rebase label Nov 6, 2023

gmfrasca added 12 commits November 7, 2023 10:48

Remove External Database Secret Creation

fb4dc7b

Remove External Object Storage Secret Creation

2821c46

Create DevTools in DSPA CR, Add Database/Storage Secret Creation

d91acba

DSPO to manage credential secrets if using deployed storage/db

8bef10f

Change DB/Storage Behavior to only Generate creds if deployable reque…

c0b9762

…sted

Include DSPA name on Created ObjectStorage Secret Names

95b5ce7

Centralize Secret value retrieval/creation functionality

c0af336

Remove DevTools CRD item

755700a

- 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

Fix Functional Tests to handle Secret Creation Behavior changes

d8c9e5d

Propogate unhandled errors when fetching Secrets for DB/ObjStore creds

84be90c

Pass ObjStore Credential Metadata EnvVars to APIServer

be46a1b

- Allows DSP APIServer to use credentials found in secrets other than 'mlpipeline-minio-artifact'

Only use managed DB/Storage Secret if not specified in DSPA

c8835b8

gmfrasca force-pushed the rm-secret-creation branch from 06b0bc1 to c8835b8 Compare November 7, 2023 15:50

openshift-merge-robot removed the needs-rebase label Nov 7, 2023

Fix Unit and Functional Tests

4e4c3fa

gmfrasca force-pushed the rm-secret-creation branch from 6fdc5db to 4e4c3fa Compare November 7, 2023 17:07

openshift-ci bot added the lgtm label Nov 8, 2023

openshift-ci bot added the approved label Nov 8, 2023

openshift-merge-bot bot merged commit 40a8930 into opendatahub-io:main Nov 8, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove External Connection Secrets from operator responsibilities #118

Remove External Connection Secrets from operator responsibilities #118

gmfrasca commented May 15, 2023 •

edited

Loading

gmfrasca commented May 23, 2023

HumairAK left a comment

HumairAK commented Jun 6, 2023

dsp-developers commented Sep 5, 2023

dsp-developers commented Sep 13, 2023

dsp-developers commented Sep 13, 2023

HumairAK commented Sep 14, 2023

dsp-developers commented Sep 15, 2023

dsp-developers commented Oct 4, 2023

HumairAK commented Oct 4, 2023 •

edited

Loading

HumairAK commented Oct 5, 2023 •

edited

Loading

dsp-developers commented Nov 7, 2023

dsp-developers commented Nov 7, 2023

HumairAK commented Nov 8, 2023 •

edited

Loading

openshift-ci bot commented Nov 8, 2023

Remove External Connection Secrets from operator responsibilities #118

Remove External Connection Secrets from operator responsibilities #118

Conversation

gmfrasca commented May 15, 2023 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

gmfrasca commented May 23, 2023

HumairAK left a comment

Choose a reason for hiding this comment

HumairAK commented Jun 6, 2023

dsp-developers commented Sep 5, 2023

dsp-developers commented Sep 13, 2023

dsp-developers commented Sep 13, 2023

HumairAK commented Sep 14, 2023

dsp-developers commented Sep 15, 2023

dsp-developers commented Oct 4, 2023

HumairAK commented Oct 4, 2023 • edited Loading

HumairAK commented Oct 5, 2023 • edited Loading

dsp-developers commented Nov 7, 2023

dsp-developers commented Nov 7, 2023

HumairAK commented Nov 8, 2023 • edited Loading

openshift-ci bot commented Nov 8, 2023

gmfrasca commented May 15, 2023 •

edited

Loading

HumairAK commented Oct 4, 2023 •

edited

Loading

HumairAK commented Oct 5, 2023 •

edited

Loading

HumairAK commented Nov 8, 2023 •

edited

Loading