New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Updated mariadb to serve over tls #696

Merged

openshift-merge-bot merged 1 commit into opendatahub-io:main from VaniHaripriya:RHOAIENG-4972

Sep 20, 2024

Contributor

VaniHaripriya commented Aug 30, 2024 •

edited

Loading

The issue resolved by this Pull Request:

Resolves #RHOAIENG-4972

Description of your changes:

Updated MariaDB config to server over TLS.
Added the oc annotation to generate the certs.
Added a config map with the ssl info.

Testing instructions

Deploy DSPO and the dspa using the below yaml file for two scenarios. After deployment, verify that the health
checks are successful.
Create a pipeline run and ensure that it completes successfully.
Execute the following command inside the MariaDB pod terminal to verify that the TLS setup is correctly configured.

mysql -u $MYSQL_USER -p$MYSQL_PASSWORD -h 127.0.0.1 -D $MYSQL_DATABASE
   
MariaDB [mlpipeline]> SHOW GLOBAL VARIABLES LIKE 'have_ssl';
+---------------+-------+
| Variable_name | Value |
+---------------+-------+
| have_ssl      | YES   |
+---------------+-------+
1 row in set (0.001 sec)

TLS Enabled

Deploy the following DSPA

apiVersion: datasciencepipelinesapplications.opendatahub.io/v1alpha1
kind: DataSciencePipelinesApplication
metadata:
  name: sample
spec:
  podToPodTLS: true
  dspVersion: v2
  apiServer:
    enableSamplePipeline: true
    cABundle:
      configMapKey: ca.crt
      configMapName: kube-root-ca.crt
  objectStorage:   
    enableExternalRoute: true
    minio:
      deploy: true
      image: 'quay.io/opendatahub/minio:RELEASE.2019-08-14T20-37-41Z-license-compliance'  
  mlpipelineUI:
    image: quay.io/opendatahub/ds-pipelines-frontend:latest

TLS Disabled

Deploy the following DSPA

apiVersion: datasciencepipelinesapplications.opendatahub.io/v1alpha1
kind: DataSciencePipelinesApplication
metadata:
  name: sample
spec:
  podToPodTLS: false
  dspVersion: v2
  apiServer:
    enableSamplePipeline: true
    cABundle:
      configMapKey: ca.crt
      configMapName: kube-root-ca.crt
  objectStorage:  
    enableExternalRoute: true
    minio:
      deploy: true
      image: 'quay.io/opendatahub/minio:RELEASE.2019-08-14T20-37-41Z-license-compliance'  
  mlpipelineUI:
    image: quay.io/opendatahub/ds-pipelines-frontend:latest

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

openshift-ci bot requested review from DharmitD and rimolive

August 30, 2024 15:37

VaniHaripriya force-pushed the RHOAIENG-4972 branch from f3a81ee to 852db9c Compare

August 30, 2024 15:39

Contributor

dsp-developers commented Aug 30, 2024

A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-696
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 [email protected]:opendatahub-io/data-science-pipelines-operator.git
cd data-science-pipelines-operator/
git fetch origin pull/696/head
git checkout -b pullrequest f3a81ee0eba65788cceed4f723428d6946594abf
oc new-project opendatahub
make deploy IMG="quay.io/opendatahub/data-science-pipelines-operator:pr-696"

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

Contributor

dsp-developers commented Aug 30, 2024

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

VaniHaripriya force-pushed the RHOAIENG-4972 branch from 852db9c to 4108d12 Compare

August 30, 2024 15:42

Contributor

dsp-developers commented Aug 30, 2024

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

3 similar comments

Contributor

dsp-developers commented Sep 3, 2024

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

Contributor

dsp-developers commented Sep 3, 2024

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

Contributor

dsp-developers commented Sep 4, 2024

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

VaniHaripriya force-pushed the RHOAIENG-4972 branch from bed850b to 09c222f Compare

September 4, 2024 03:45

Contributor

dsp-developers commented Sep 4, 2024

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

VaniHaripriya force-pushed the RHOAIENG-4972 branch from 241acca to da5de41 Compare

September 4, 2024 03:54

Contributor

dsp-developers commented Sep 4, 2024

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

1 similar comment

Contributor

dsp-developers commented Sep 4, 2024

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

hbelmiro reviewed

View reviewed changes

controllers/dspipeline_params.go Outdated

-              	PodToPodTLS bool
+              	PodToPodTLS             bool
+              	CustomExtraParams       string

Contributor

hbelmiro Sep 4, 2024

It's not 100% clear to me, but I believe TLS should be enabled when PodToPodTLS is true. Can you confirm @HumairAK?

Contributor

gregsheremeta Sep 6, 2024

agreed.

so instead of doing this:

  database:
    customExtraParams: '{"tls":"true"}'

just make that happen automatically when PodToPodTLS is true

Contributor

gregsheremeta Sep 6, 2024

also it doesn't look like the value of customExtraParams is actually used anywhere, which is a bit puzzling

Contributor Author

VaniHaripriya Sep 6, 2024

@HumairAK could you confirm that TLS should be enabled when PodToPodTLS is true. I remember that we discussed to use CustomExtraParams instead of PodToPodTLS in this case, so just want to make sure of that.

Contributor

HumairAK Sep 18, 2024

sorry for the delayed response, yes that's correct, we only want to enable mariadb when podtopodtls is set to true, in this scenario we should probably configure extra params automatically as needed to make this work

Contributor

gregsheremeta Sep 18, 2024

in this scenario we should probably configure extra params automatically as needed to make this work

so we don't need the customExtraParams at all. Cool. Thanks for confirming.

Contributor

hbelmiro commented Sep 4, 2024

/verified

gregsheremeta reviewed

View reviewed changes

config/internal/mariadb/default/service.yaml.tmpl Outdated Show resolved Hide resolved

Member

anishasthana commented Sep 9, 2024

/retest

VaniHaripriya force-pushed the RHOAIENG-4972 branch from 004043e to 90ed8a9 Compare

September 19, 2024 02:24

Contributor

gregsheremeta commented Sep 19, 2024

some nitpicks, but
/lgtm

openshift-ci bot assigned gregsheremeta

openshift-ci bot added the lgtm label

gregsheremeta reviewed

View reviewed changes

controllers/database.go Outdated Show resolved Hide resolved

controllers/dspipeline_params.go Outdated Show resolved Hide resolved

openshift-ci bot removed the lgtm label


          Updated mariadb to serve over tls

92a451f

VaniHaripriya force-pushed the RHOAIENG-4972 branch from 5f2be8d to 92a451f Compare

September 19, 2024 17:15

Contributor

gregsheremeta commented Sep 20, 2024

/lgtm

nice work!

openshift-ci bot added the lgtm label

Contributor

HumairAK commented Sep 20, 2024

awesome thanks folks!

/approve

Contributor

openshift-ci bot commented Sep 20, 2024

[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 approved label

openshift-merge-bot bot merged commit 7270713 into opendatahub-io:main

7 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

hbelmiro hbelmiro left review comments

HumairAK HumairAK left review comments

gregsheremeta gregsheremeta left review comments

DharmitD Awaiting requested review from DharmitD

rimolive Awaiting requested review from rimolive

Labels