feat: DSPO handle db tls connections and configs #575

HumairAK · 2024-02-20T14:27:32Z

The issue resolved by this Pull Request:

Resolves for v1.6:

Description of your changes:

This change allows the DSPO to perform health checks against a tls
secured database. If the database is behind a self-signed cert, end user
can provide a ca-bundle either as a global cert (configmap named
"odh-trust-bundle" or via the ".caBundle" config option via the DSPA.

This change also exposes the "ExtraParams" field for DSP, meaning users
can now add ny DSN parameters when configuring a DB connection for DSP.
These params are utilized in the same manner by the DSPO when conducting
the health check to keep the behavior consistent.

NOTE that before external connections made a connection via tls:false, now it's enforced by default. To disable it, users will need to set customExtraParams to: {"tls":"true"} string.

Testing 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"}

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

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

This change allows the DSPO to perform heatlh checks against a tls secured database. If the database is behind a self-signed cert, end user can provide a ca-bundle either as a global cert (configmap named "odh-trust-bundle" or via the ".caBundle" config option via the DSPA. This change also exposes the "ExtraParams" field for DSP, meaning users can now add ny DSN parameters when configuring a DB connection for DSP. These params are utilized in the same manner by the DSPO when conducting the health check to keep the behavior consistent. Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com>

dsp-developers · 2024-02-20T14:30:46Z

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

amadhusu

I was able to test and verify the first two test cases but not the last one with the new field "customExtraParams".

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.

amadhusu

Tested the customExtraParams after Humair's suggestion about the format. It works as expected.

gregsheremeta

i have some thoughts and nitpicks ... all of which we can deal with in a follow up

/lgtm

controllers/util/util.go

gregsheremeta · 2024-02-22T19:55:12Z

config/overlays/make-deploy/kustomization.yaml

@@ -1,6 +1,6 @@
 apiVersion: kustomize.config.k8s.io/v1beta1
 kind: Kustomization
-namespace: odh-applications
+namespace: opendatahub


nitpick, not related, should extract

config/internal/apiserver/server-config.yaml.tmpl

.../bases/datasciencepipelinesapplications.opendatahub.io_datasciencepipelinesapplications.yaml

gregsheremeta · 2024-02-22T20:01:19Z

controllers/dspipeline_controller.go

+		// Watch for global ca bundle, if one is added to this namespace
+		// we need to reconcile on all the dspa's in this namespace
+		// so they may mount this cert in the appropriate containers
+		Watches(&source.Kind{Type: &corev1.ConfigMap{}},


watching ConfigMap gobbles up memory doesn't it? Do we need to check for that and mitigate, possibly by increasing limits and or requests?

We added a similar watcher for a pod, and didn't notice a major uptick (a lot of this is internally cached and heavily optimized in k8s I believe), and pod events work queue is likely to be insanely longer than the configmap events

buut if it does cause issues, we will probably catch it in our perf testing and adjust accordingly

gregsheremeta · 2024-02-22T20:05:29Z

controllers/dspipeline_params.go

@@ -149,6 +151,9 @@ func (p *DSPAParams) SetupDBParams(ctx context.Context, dsp *dspa.DataSciencePip
 		p.DBConnection.Port = dsp.Spec.Database.ExternalDB.Port
 		p.DBConnection.Username = dsp.Spec.Database.ExternalDB.Username
 		p.DBConnection.DBName = dsp.Spec.Database.ExternalDB.DBName
+		// Assume default external connection is tls enabled
+		// user can override this via CustomExtraParams field
+		p.DBConnection.ExtraParams = fmt.Sprintf(config.DBDefaultExtraParams, true)


The extra params should be a struct that gets serialized to json when it needs to be. I guess it needs to by dynamic though, because you don't want to chase the mysql API. Hmm. Sprintf'ing the 'true' or 'false' specifically for tls feels weird when the field is named "DBDefaultExtraParams" and not "tlsEnabled". Not sure what the solution is but wanted to call out that it feels weird to me.

I actually did it that way at first, i.e. Extraparams was of type map[string]string but in our configmap template we need to render it as a string, the thing is we need to pass it in as a string anyways to render it in our go template configmap, I was torn on it, this method resulted in a lot less lines of code so I did it this way instead

gregsheremeta · 2024-02-22T20:07:36Z

controllers/dspipeline_params.go

+			log.Info(fmt.Sprintf("Encountered error when attempting to fetch ConfigMap: [%s], Error: %v", globalCABundleCFGMapKey, err))
+			return err
+		} else {
+			// Found a global cert, consume this cert, takes precedence over "cABundle" provided via DSPA


local usually overrides global ... pinged you in slack about it

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

gregsheremeta · 2024-02-22T20:43:29Z

/lgtm

gregsheremeta · 2024-02-22T20:45:00Z

/approve

openshift-ci · 2024-02-22T20:45:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amadhusu, gregsheremeta

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

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

dsp-developers · 2024-02-22T20:51:47Z

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

openshift-ci bot requested review from amadhusu and gregsheremeta February 20, 2024 14:27

HumairAK added 6 commits February 20, 2024 09:27

chore: update deployment ns to opendatahub.

d0aca8c

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

feat: dspo to reconcile on odh ca events.

b400dff

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

feat: use odh global config when provided.

574cd85

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

chore: clean up x509 minio error msg.

8b6ed3f

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

feat: correct logger messages in dspa params

673a04a

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

HumairAK force-pushed the v1.6.x-RHOAIENG-1690-and-2385-2 branch from 703f618 to 32d3e01 Compare February 20, 2024 14:28

HumairAK changed the title ~~V1.6.x rhoaieng 1690 and 2385 2~~ Feat: DSPO handls db tls connections and configs Feb 20, 2024

HumairAK changed the title ~~Feat: DSPO handls db tls connections and configs~~ feat: DSPO handls db tls connections and configs Feb 20, 2024

gregsheremeta changed the title ~~feat: DSPO handls db tls connections and configs~~ feat: DSPO handle db tls connections and configs Feb 20, 2024

amadhusu reviewed Feb 21, 2024

View reviewed changes

amadhusu approved these changes Feb 22, 2024

View reviewed changes

openshift-ci bot assigned amadhusu Feb 22, 2024

openshift-ci bot added the lgtm label Feb 22, 2024

gregsheremeta reviewed Feb 22, 2024

View reviewed changes

openshift-ci bot assigned gregsheremeta Feb 22, 2024

chore: fix typos

167ba85

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

openshift-ci bot removed the lgtm label Feb 22, 2024

openshift-ci bot added the lgtm label Feb 22, 2024

openshift-ci bot added the approved label Feb 22, 2024

HumairAK merged commit 782777c into opendatahub-io:v1.6.x Feb 22, 2024
5 of 7 checks passed

HumairAK mentioned this pull request Feb 26, 2024

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

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: DSPO handle db tls connections and configs #575

feat: DSPO handle db tls connections and configs #575

HumairAK commented Feb 20, 2024 •

edited

dsp-developers commented Feb 20, 2024

amadhusu left a comment

amadhusu left a comment

gregsheremeta left a comment

gregsheremeta Feb 22, 2024

gregsheremeta Feb 22, 2024

HumairAK Feb 22, 2024

HumairAK Feb 22, 2024

gregsheremeta Feb 22, 2024

HumairAK Feb 22, 2024 •

edited

gregsheremeta Feb 22, 2024

gregsheremeta commented Feb 22, 2024

gregsheremeta commented Feb 22, 2024

openshift-ci bot commented Feb 22, 2024

dsp-developers commented Feb 22, 2024

feat: DSPO handle db tls connections and configs #575

feat: DSPO handle db tls connections and configs #575

Conversation

HumairAK commented Feb 20, 2024 • edited

The issue resolved by this Pull Request:

Description of your changes:

Testing instructions

Checklist

dsp-developers commented Feb 20, 2024

amadhusu left a comment

Choose a reason for hiding this comment

amadhusu left a comment

Choose a reason for hiding this comment

gregsheremeta left a comment

Choose a reason for hiding this comment

gregsheremeta Feb 22, 2024

Choose a reason for hiding this comment

gregsheremeta Feb 22, 2024

Choose a reason for hiding this comment

HumairAK Feb 22, 2024

Choose a reason for hiding this comment

HumairAK Feb 22, 2024

Choose a reason for hiding this comment

gregsheremeta Feb 22, 2024

Choose a reason for hiding this comment

HumairAK Feb 22, 2024 • edited

Choose a reason for hiding this comment

gregsheremeta Feb 22, 2024

Choose a reason for hiding this comment

gregsheremeta commented Feb 22, 2024

gregsheremeta commented Feb 22, 2024

openshift-ci bot commented Feb 22, 2024

dsp-developers commented Feb 22, 2024

HumairAK commented Feb 20, 2024 •

edited

HumairAK Feb 22, 2024 •

edited