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

HumairAK · 2024-02-26T21:58:24Z

The issue resolved by this Pull Request:

Resolves

RHOAIENG-16900
RHOAIENG-2385

Description of your changes:

Same as:

Notes:

New server config parameters (we currently only support mysql/mariadb configurations, postgres config doesn't support tls right now in kfp)
MLMD pod is switched to use Model Registry's MLMD grpc image, so we can utilize their tls flags and mount the odh/user provided ca bundle in that pod.

Testing instructions

Same as: #575 & #579
But for v2 with adjustments to work with v2:

Full instructions:

Deploy a secure/tls enabled s3/mariadb behind a self-signed cert (for example in a self-signed ocp cluster), provide these configs to the DSAP as external configs. Ensure they only accept tls based connections.

Deploy a cabundle as a configmap in the DSPA namespace:

kind: ConfigMap
apiVersion: v1
metadata:
  name: odh-trusted-ca-bundle
data:
  ca-bundle.crt: | 
      <your-bundle>

Deploy DSPA configured to leverage these connections via external db/object store connections.

Confirm DSPO is able conduct successfull health checks (if it deploys the DSPA pods then health checks worked), if it didn't, log the error here.

Confirm DSPA comes up successfully, in this change DSPO willl udpate the DSP server's configs to enable "tls" for db connections. Confirm this behavior by running a successful pipeline.

Confirm DSPA "simple" v1 still works (no external db/s3 configured).

Confirm setting "customExtraParams" in the DSPA's ".spec.database.customExtraParams". Provide wrong ca bundle, confirm it fails to deploy dspa, then set ".spec.database.customExtraParams" to {"tls":"true"}, it should work. Because the value is a json string, you should use | or >- to format the yaml value, example:

spec:
  dspVersion: v2
  database:
    customExtraParams: |                           
      {"tls": "skip-verify"}

Confirm that when you include an odh-trusted-ca-bundle configmap, and another cabundle in the DSPA via .spec.cABundle, the result is a configmap named dsp-trusted-ca-* that includes both bundles, which is mounted in the DSP Apiserver, as well as the user launcher pod.

Checklist

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

dsp-developers · 2024-02-26T22:44:38Z

A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-577
An OCP cluster where you are logged in as cluster admin is required.

To use this image run the following:

cd $(mktemp -d)
git clone git@github.com:opendatahub-io/data-science-pipelines-operator.git
cd data-science-pipelines-operator/
git fetch origin pull/577/head
git checkout -b pullrequest 6709929741e3daa52672b39e40876a2869d53e5c
oc new-project opendatahub
make deploy IMG="quay.io/opendatahub/data-science-pipelines-operator:pr-577"

More instructions here on how to deploy and test a Data Science Pipelines Application.

dsp-developers · 2024-02-27T14:46:06Z

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-577

HumairAK · 2024-02-27T17:55:58Z

/hold

dsp-developers · 2024-02-29T21:37:25Z

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-577

dsp-developers · 2024-02-29T21: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-577

HumairAK · 2024-03-05T20:53:23Z

/unhold

dsp-developers · 2024-03-05T20:55:41Z

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-577

dsp-developers · 2024-03-05T21:05:27Z

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-577

dsp-developers · 2024-03-06T03:21:31Z

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-577

dsp-developers · 2024-03-06T03:58:15Z

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-577

dsp-developers · 2024-03-07T21:39:20Z

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-577

dsp-developers · 2024-03-07T22:04: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-577

dsp-developers · 2024-03-08T15:57:12Z

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-577

Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com> (cherry picked from commit b400dff)

Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com> (cherry picked from commit 574cd85)

Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com> (cherry picked from commit 673a04a)

Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com>

When a configmap trust ca store is provided, whether it's from a user provisioned configmap with a ca bundle via the DSPA's caBundle field, or whether it's a configmap name "odh-trusted-ca-bundle" (provided by odh or user), then this change will create an amalgmation of all of the certs from these configmaps and create a single dspo managed configmap. The resulting configmap has a single cert that is the accumulation of all afore mentioned certs, and can be passed as a single file to the dsp server, or pipeline pods to utilize. This helps work around issues and overhead work required when mounting to a ca path with multiple certs, because such a path requires that it first be re_hashed. Furthermore this allows us to work around issues related to aws cli being unable to utilize ca paths with it's AWS_CA_BUNDLE env variable when passing artifacts during the step-copy-artifacts step. Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com>

Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com>

dsp-developers · 2024-03-11T16:45:27Z

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-577

gregsheremeta

nothing i've commented on should be considered a blocker to merging. I'm happy to tag lgtm now if you're rather merge now and fixup later (or not make any changes at all, that's fine too)

controllers/dspipeline_params.go

gregsheremeta · 2024-03-11T19:04:16Z

controllers/config/defaults.go


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


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

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

controllers/dspipeline_params.go

Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com>

dsp-developers · 2024-03-11T19:22:15Z

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-577

gregsheremeta · 2024-03-11T19:25:02Z

/lgtm

rimolive · 2024-03-11T19:59:44Z

/lgtm
/approve

openshift-ci · 2024-03-11T19:59:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rimolive

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~~ [rimolive]

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 requested review from rimolive and VaniHaripriya February 26, 2024 21:58

HumairAK changed the title ~~V2 rhoaieng 1690 and 2385~~ feat: DSPO handle db tls connections and configs Feb 26, 2024

HumairAK changed the title ~~feat: DSPO handle db tls connections and configs~~ feat: DSPO handle db tls connections and configs (v2) Feb 26, 2024

HumairAK force-pushed the v2-RHOAIENG-1690-and-2385 branch from 6709929 to 76b8cf7 Compare February 27, 2024 14:43

openshift-ci bot added the do-not-merge/hold label Feb 27, 2024

HumairAK changed the title ~~feat: DSPO handle db tls connections and configs (v2)~~ wip: feat: DSPO handle db tls connections and configs (v2) Feb 28, 2024

openshift-ci bot added the do-not-merge/work-in-progress label Feb 28, 2024

HumairAK force-pushed the v2-RHOAIENG-1690-and-2385 branch 3 times, most recently from dffc3ee to 2628402 Compare February 29, 2024 21:34

HumairAK force-pushed the v2-RHOAIENG-1690-and-2385 branch from 2628402 to d72b805 Compare February 29, 2024 21:38

HumairAK changed the title ~~wip: feat: DSPO handle db tls connections and configs (v2)~~ feat: DSPO handle db tls connections and configs (v2) Mar 1, 2024

openshift-ci bot removed the do-not-merge/work-in-progress label Mar 1, 2024

openshift-ci bot removed the do-not-merge/hold label Mar 5, 2024

HumairAK force-pushed the v2-RHOAIENG-1690-and-2385 branch from 9fc7115 to 472db5a Compare March 5, 2024 21:03

HumairAK force-pushed the v2-RHOAIENG-1690-and-2385 branch from 70a9bf4 to 9ee3dac Compare March 6, 2024 03:56

HumairAK force-pushed the v2-RHOAIENG-1690-and-2385 branch from 9ee3dac to 7376a5c Compare March 7, 2024 21:37

HumairAK force-pushed the v2-RHOAIENG-1690-and-2385 branch from 7376a5c to 477e865 Compare March 7, 2024 22:02

HumairAK force-pushed the v2-RHOAIENG-1690-and-2385 branch from 477e865 to 5f6a984 Compare March 8, 2024 15:54

HumairAK removed request for rimolive and VaniHaripriya March 8, 2024 18:04

HumairAK added 7 commits March 11, 2024 12:42

feat: dspo to reconcile on odh ca events.

55fdadd

Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com> (cherry picked from commit b400dff)

feat: use odh global config when provided.

fdb1804

Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com> (cherry picked from commit 574cd85)

feat: correct logger messages in dspa params

6433d51

Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com> (cherry picked from commit 673a04a)

feat: allow dspo to verify tls secured mysql

2600ac9

Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com>

chore: mount ca bundle to mlmd

531c957

Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com>

chore: add more cert tests.

7ade9ee

Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com>

HumairAK force-pushed the v2-RHOAIENG-1690-and-2385 branch from 5f6a984 to 7ade9ee Compare March 11, 2024 16:43

HumairAK requested a review from rimolive March 11, 2024 17:32

gregsheremeta reviewed Mar 11, 2024

View reviewed changes

chore: clarify cabundle comments and error descriptions

1a30d8d

Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com>

openshift-ci bot assigned gregsheremeta Mar 11, 2024

openshift-ci bot added the lgtm label Mar 11, 2024

openshift-ci bot assigned rimolive Mar 11, 2024

openshift-ci bot added the approved label Mar 11, 2024

HumairAK merged commit 8d9f36a into opendatahub-io:main Mar 11, 2024
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

HumairAK commented Feb 26, 2024 •

edited

Loading

dsp-developers commented Feb 26, 2024

dsp-developers commented Feb 27, 2024

HumairAK commented Feb 27, 2024

dsp-developers commented Feb 29, 2024

dsp-developers commented Feb 29, 2024

HumairAK commented Mar 5, 2024

dsp-developers commented Mar 5, 2024

dsp-developers commented Mar 5, 2024

dsp-developers commented Mar 6, 2024

dsp-developers commented Mar 6, 2024

dsp-developers commented Mar 7, 2024

dsp-developers commented Mar 7, 2024

dsp-developers commented Mar 8, 2024

dsp-developers commented Mar 11, 2024

gregsheremeta left a comment

gregsheremeta Mar 11, 2024

gregsheremeta Mar 11, 2024

dsp-developers commented Mar 11, 2024

gregsheremeta commented Mar 11, 2024

rimolive commented Mar 11, 2024

openshift-ci bot commented Mar 11, 2024

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

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

Conversation

HumairAK commented Feb 26, 2024 • edited Loading

The issue resolved by this Pull Request:

Description of your changes:

Testing instructions

Checklist

dsp-developers commented Feb 26, 2024

dsp-developers commented Feb 27, 2024

HumairAK commented Feb 27, 2024

dsp-developers commented Feb 29, 2024

dsp-developers commented Feb 29, 2024

HumairAK commented Mar 5, 2024

dsp-developers commented Mar 5, 2024

dsp-developers commented Mar 5, 2024

dsp-developers commented Mar 6, 2024

dsp-developers commented Mar 6, 2024

dsp-developers commented Mar 7, 2024

dsp-developers commented Mar 7, 2024

dsp-developers commented Mar 8, 2024

dsp-developers commented Mar 11, 2024

gregsheremeta left a comment

Choose a reason for hiding this comment

gregsheremeta Mar 11, 2024

Choose a reason for hiding this comment

gregsheremeta Mar 11, 2024

Choose a reason for hiding this comment

dsp-developers commented Mar 11, 2024

gregsheremeta commented Mar 11, 2024

rimolive commented Mar 11, 2024

openshift-ci bot commented Mar 11, 2024

HumairAK commented Feb 26, 2024 •

edited

Loading