UPSTREAM: <carry>: Added support for TLS to MLMD GRPC Server #683

hbelmiro · 2024-08-09T17:55:32Z

The issue resolved by this Pull Request:

Resolves https://issues.redhat.com/browse/RHOAIENG-4971

This PR depends on:

UPSTREAM: <carry>: Added support for TLS to MLMD GRPC Server data-science-pipelines#72

Description of the changes:

Added support for TLS to MLMD GRPC Server

Testing instructions

There are 2 scenarios for testing:

TLS Enabled

Deploy the following DSPA

apiVersion: datasciencepipelinesapplications.opendatahub.io/v1alpha1
kind: DataSciencePipelinesApplication
metadata:
  name: dspa
spec:
  dspVersion: v2
  podToPodTLS: true
  apiServer:
    image: "quay.io/opendatahub/ds-pipelines-api-server:pr-72"
    argoDriverImage: "quay.io/opendatahub/ds-pipelines-driver:pr-72"
    argoLauncherImage: "quay.io/opendatahub/ds-pipelines-launcher:pr-72"
    enableSamplePipeline: true
  persistenceAgent:
    image: "quay.io/opendatahub/ds-pipelines-persistenceagent:pr-72"
  scheduledWorkflow:
    image: "quay.io/opendatahub/ds-pipelines-scheduledworkflow:pr-72"
  mlmd:  
    deploy: true  # Optional component
    grpc:
      image: "quay.io/opendatahub/mlmd-grpc-server:latest"
    envoy:
      image: "registry.redhat.io/openshift-service-mesh/proxyv2-rhel8:2.3.9-2"
  mlpipelineUI:
    deploy: true  # Optional component 
    image: "quay.io/opendatahub/ds-pipelines-frontend:pr-72"
  objectStorage:
    minio:
      deploy: true
      image: 'quay.io/opendatahub/minio:RELEASE.2019-08-14T20-37-41Z-license-compliance'

Run the sample pipeline
The run must complete successfully

TLS Disabled

Deploy the following DSPA

apiVersion: datasciencepipelinesapplications.opendatahub.io/v1alpha1
kind: DataSciencePipelinesApplication
metadata:
  name: dspa
spec:
  dspVersion: v2
  podToPodTLS: false
  apiServer:
    image: "quay.io/opendatahub/ds-pipelines-api-server:pr-72"
    argoDriverImage: "quay.io/opendatahub/ds-pipelines-driver:pr-72"
    argoLauncherImage: "quay.io/opendatahub/ds-pipelines-launcher:pr-72"
    enableSamplePipeline: true
  persistenceAgent:
    image: "quay.io/opendatahub/ds-pipelines-persistenceagent:pr-72"
  scheduledWorkflow:
    image: "quay.io/opendatahub/ds-pipelines-scheduledworkflow:pr-72"
  mlmd:  
    deploy: true  # Optional component
    grpc:
      image: "quay.io/opendatahub/mlmd-grpc-server:latest"
    envoy:
      image: "registry.redhat.io/openshift-service-mesh/proxyv2-rhel8:2.3.9-2"
  mlpipelineUI:
    deploy: true  # Optional component 
    image: "quay.io/opendatahub/ds-pipelines-frontend:pr-72"
  objectStorage:
    minio:
      deploy: true
      image: 'quay.io/opendatahub/minio:RELEASE.2019-08-14T20-37-41Z-license-compliance'

Run the sample pipeline
The run must complete successfully

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-08-09T17:57:51Z

A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-683
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/683/head
git checkout -b pullrequest 85a6ca9ebb69f28e05780de54509b0d3855cb533
oc new-project opendatahub
make deploy IMG="quay.io/opendatahub/data-science-pipelines-operator:pr-683"

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

Signed-off-by: hbelmiro <helber.belmiro@gmail.com>

dsp-developers · 2024-08-30T19:13: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-683

VaniHaripriya · 2024-09-04T16:32:39Z

/verified
/lgtm
Deployed DSPO and verified the two scenarios mentioned in the testing instructions. Created pipeline runs and they completed successfully.

gregsheremeta · 2024-09-06T13:26:42Z

api/v1alpha1/dspipeline_types.go

+	// +kubebuilder:validation:Optional
+	CertificateContents string `json:"certificateContents,omitempty"`
+	// +kubebuilder:validation:Optional
+	PrivateKeyContents string `json:"privateKeyContents,omitempty"`


I don't love that the user needs to paste these in here. For apiserver, we are able to just use the paths to the OpenShift-provded certs like so:

data-science-pipelines-operator/config/internal/apiserver/default/deployment.yaml.tmpl

Lines 180 to 191 in e101b0f

name: ds-pipeline-api-server

command: ['/bin/apiserver']

args:

- --config=/config

- -logtostderr=true

{{ if .APIServer.EnableSamplePipeline }}

- --sampleconfig=/config/sample_config.json

{{ end }}

{{ if .PodToPodTLS }}

- --tlsCertPath=/etc/tls/private/tls.crt

- --tlsCertKeyPath=/etc/tls/private/tls.key

{{ end }}

could we not do something similar for here? Could we not read those file contents instead of having them be manually pasted here?

This is automatically set by the operator and is not for users to use (technically they can). I had to add these fields to be able to set the contents in the template.
Regarding setting the contents, I also don't like it, but the MLMD config requires that.

re: having to set the contents -- sure, no way around that.

I had to add these fields to be able to set the contents in the template.

Ok, I understand the reason. I really don't like adding fields to the API for this though. Hmm. How sure are we that there's no other way to feed things to the manifestival templating? That doesn't sound right to me.

yeah I think you could just add two strings roughly here

data-science-pipelines-operator/controllers/dspipeline_params.go

Line 70 in 20dd747

CustomKfpLauncherConfigMapData string

and then populate them

and then use those to feed the templating engine (example:

data-science-pipelines-operator/config/internal/apiserver/default/kfp_launcher_config.yaml.tmpl

Line 4 in e101b0f

{{.CustomKfpLauncherConfigMapData}}

)

and avoid changing the DSPA API

HumairAK · 2024-09-06T14:23:41Z

controllers/dspipeline_params.go

+func (p *DSPAParams) getMlmdGrpcCertificatesSecret(ctx context.Context, client client.Client) (v1.Secret, error) {
+	secretName := types.NamespacedName{
+		Namespace: p.Namespace,
+		Name:      "ds-pipeline-metadata-grpc-tls-certs-" + p.Name,
+	}
+	secret := v1.Secret{}
+	err := client.Get(ctx, secretName, &secret)
+	return secret, err
+}


this can be a static helper method like we do for configmaps

HumairAK · 2024-09-06T14:36:23Z

controllers/dspipeline_params.go

+		if apierrs.IsNotFound(err) && ignoreSecretNotFound {
+			logger.Info("Ignoring secret not found due to DSPO_MLMD_IGNORE_AUTO_GENERATED_SECRET_NOT_FOUND_ERROR = true")


As a project admin, I think I might find this log message confusing.

Maybe something more user friendly like "Did not detect an MLMD GRPC Service-CA provisioned TLS kubernetes secret, continuing (DSPA is configured to ignore)."

As a more broader point, I actually think we might want to report an error here instead, because we enter here at: if params.PodToPodTLS == true, in this case we expect the service-ca provided tls secret to exist, if it doesn't, something has gone wrong.

See #683 (comment).

HumairAK · 2024-09-06T14:52:15Z

config/internal/ml-metadata/metadata-grpc-tls-config-secret.yaml.tmpl

+    ssl_config {
+      server_cert: "{{.MLMD.GRPC.CertificateContents}}"
+      server_key: "{{.MLMD.GRPC.PrivateKeyContents}}"
+      client_verify: false


to confirm, this is for mutual tls right? i.e. server also verifying client tls certs? I'm thinking maybe we should drop a comment here mentioning that? wdyt? in proto files comments can be added via //

HumairAK · 2024-09-06T14:54:48Z

controllers/dspipeline_params.go

@@ -893,3 +898,29 @@ func (p *DSPAParams) ExtractParams(ctx context.Context, dsp *dspa.DataSciencePip

 	return nil
 }
+
+func (p *DSPAParams) LoadMlmdCertificates(ctx context.Context, client client.Client, logger logr.Logger) error {


nit: can we keep ExtractParams() at the bottom?

HumairAK · 2024-09-06T14:56:01Z

controllers/config/defaults.go

+	DBConnectionTimeoutConfigName              = "DSPO.HealthCheck.Database.ConnectionTimeout"
+	RequeueTimeConfigName                      = "DSPO.RequeueTime"
+	ApiServerIncludeOwnerReferenceConfigName   = "DSPO.ApiServer.IncludeOwnerReference"
+	MlmdIgnoreAutoGeneratedSecretNotFoundError = "DSPO.Mlmd.IgnoreAutoGeneratedSecretNotFoundError"


can you elaborate on why this is needed? when do we want to ignore the secret?

We'll ignore the secret on tests. Since tests don't run on OpenShift, the secret is never created.
Should I add a comment?

I feel like this might be a hack to get around our existing tests. ATM I'm thinking we should just not be doing PodtoPodTLS testing that is is not an live openshift environment. See: https://issues.redhat.com/browse/RHOAIENG-11646, Can we just set this to false, and not require the creation of such secrets to begin with in this mode?

What I'd rather see is we add openshift ci, and add integration tests for when podtopodtls == true
Since this feature inherently requires a working live openshift ci environment, the tests for this should be integration tests in nature.

We have tests that have TLS enabled, for example

data-science-pipelines-operator/controllers/testdata/declarative/case_6/deploy/04_cr.yaml

Line 15 in cf1bd60

podToPodTLS: true

HumairAK · 2024-09-06T15:09:46Z

controllers/mlmd.go

+		err := r.ApplyDir(dsp, params, mlmdTemplatesDir+"/"+mlmdGrpcService)
+		if err != nil {
+			return err
+		}
+
+		if params.PodToPodTLS {
+			err = params.LoadMlmdCertificates(ctx, r.Client, r.Log)
+			if err != nil {
+				return err
+			}
+		}


Interesting, okay so I think you're doing this because we need the tls/key pair generated to submit into the protofile. One issue I see with this is that it could result in potential failures initially, because service-ca is creating these secrets asynchronously, and may not create it in time before we attempt to fetch it.

I'm wondering if it might be better to do this in stages:

first reconcile and create the service

ensure dspo triggers a reconcile when the service-ca tls secret is created

in the new reconcile, detect that we are podtopodtls == true && secretCreated == true, then proceed with r.ApplyDir(mlmddir)

Interested to hear thoughts, @gregsheremeta / @hbelmiro

@HumairAK we would wait for the new reconcile indefinitely and never know if the secret was never created.
What about trying to LoadMlmdCertificates and repeat until a timeout (maybe 30s?)? It would be simpler and we can log an error if the certificates are not found.

openshift-ci · 2024-09-06T15:09:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from humairak. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

Signed-off-by: hbelmiro <helber.belmiro@gmail.com>

dsp-developers · 2024-09-06T20:33:38Z

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

This reverts commit 692aad2.

dsp-developers · 2024-09-06T21:27:44Z

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

Signed-off-by: hbelmiro <helber.belmiro@gmail.com>

dsp-developers · 2024-09-06T22:09: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-683

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

hbelmiro force-pushed the RHOAIENG-4971 branch from 85a6ca9 to e66051d Compare August 9, 2024 18:06

hbelmiro force-pushed the RHOAIENG-4971 branch from c2cfd2e to 488ff25 Compare August 20, 2024 21:17

hbelmiro force-pushed the RHOAIENG-4971 branch from 620fd03 to 777e81c Compare August 30, 2024 15:34

hbelmiro mentioned this pull request Aug 30, 2024

UPSTREAM: <carry>: Added support for TLS to MLMD GRPC Server opendatahub-io/data-science-pipelines#72

Open

1 task

UPSTREAM: <carry>: Added support for TLS to MLMD GRPC Server

20dd747

Signed-off-by: hbelmiro <helber.belmiro@gmail.com>

hbelmiro force-pushed the RHOAIENG-4971 branch from 5adfe93 to 20dd747 Compare August 30, 2024 19:10

opendatahub-io deleted a comment from dsp-developers Aug 30, 2024

hbelmiro changed the title ~~RHOAIENG-4971 WIP~~ UPSTREAM: <carry>: Added support for TLS to MLMD GRPC Server Aug 30, 2024

hbelmiro marked this pull request as ready for review August 30, 2024 19:27

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

openshift-ci bot requested review from gregsheremeta and HumairAK August 30, 2024 19:27

openshift-ci bot assigned VaniHaripriya Sep 4, 2024

openshift-ci bot added the lgtm label Sep 4, 2024

gregsheremeta reviewed Sep 6, 2024

View reviewed changes

HumairAK requested changes Sep 6, 2024

View reviewed changes

openshift-ci bot assigned HumairAK Sep 6, 2024

openshift-ci bot removed the lgtm label Sep 6, 2024

HumairAK requested changes Sep 6, 2024

View reviewed changes

hbelmiro added 5 commits September 6, 2024 12:12

Turned getMlmdGrpcCertificatesSecret into a static function

c4d6845

Signed-off-by: hbelmiro <helber.belmiro@gmail.com>

Moved ExtractParams to the bottom

3f9cc66

Signed-off-by: hbelmiro <helber.belmiro@gmail.com>

Added comment for mTLS on MLMD config

b7c5bae

Signed-off-by: hbelmiro <helber.belmiro@gmail.com>

Used METADATA_TLS_ENABLED flag

3b05a84

Signed-off-by: hbelmiro <helber.belmiro@gmail.com>

Made --mlPipelineServiceTLSEnabled mandatory

692aad2

Signed-off-by: hbelmiro <helber.belmiro@gmail.com>

Revert "Made --mlPipelineServiceTLSEnabled mandatory"

3166b1d

This reverts commit 692aad2.

hbelmiro added 2 commits September 6, 2024 18:30

Reverted incorrect change

a03b049

Signed-off-by: hbelmiro <helber.belmiro@gmail.com>

Made METADATA_TLS_ENABLED optional

20406ba

Signed-off-by: hbelmiro <helber.belmiro@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UPSTREAM: <carry>: Added support for TLS to MLMD GRPC Server #683

UPSTREAM: <carry>: Added support for TLS to MLMD GRPC Server #683

hbelmiro commented Aug 9, 2024 •

edited

Loading

dsp-developers commented Aug 9, 2024

dsp-developers commented Aug 30, 2024

VaniHaripriya commented Sep 4, 2024 •

edited

Loading

gregsheremeta Sep 6, 2024 •

edited

Loading

hbelmiro Sep 6, 2024

gregsheremeta Sep 6, 2024

gregsheremeta Sep 6, 2024 •

edited

Loading

HumairAK Sep 6, 2024

hbelmiro Sep 6, 2024

HumairAK Sep 6, 2024

HumairAK Sep 6, 2024

hbelmiro Sep 6, 2024

HumairAK Sep 6, 2024

hbelmiro Sep 6, 2024

HumairAK Sep 6, 2024

hbelmiro Sep 6, 2024

HumairAK Sep 6, 2024 •

edited

Loading

hbelmiro Sep 6, 2024

HumairAK Sep 6, 2024

hbelmiro Sep 6, 2024

HumairAK Sep 6, 2024

hbelmiro Sep 6, 2024

openshift-ci bot commented Sep 6, 2024

dsp-developers commented Sep 6, 2024

dsp-developers commented Sep 6, 2024

dsp-developers commented Sep 6, 2024

	name: ds-pipeline-api-server
	command: ['/bin/apiserver']
	args:
	- --config=/config
	- -logtostderr=true
	{{ if .APIServer.EnableSamplePipeline }}
	- --sampleconfig=/config/sample_config.json
	{{ end }}
	{{ if .PodToPodTLS }}
	- --tlsCertPath=/etc/tls/private/tls.crt
	- --tlsCertKeyPath=/etc/tls/private/tls.key
	{{ end }}

		if apierrs.IsNotFound(err) && ignoreSecretNotFound {
		logger.Info("Ignoring secret not found due to DSPO_MLMD_IGNORE_AUTO_GENERATED_SECRET_NOT_FOUND_ERROR = true")

UPSTREAM: <carry>: Added support for TLS to MLMD GRPC Server #683

Are you sure you want to change the base?

UPSTREAM: <carry>: Added support for TLS to MLMD GRPC Server #683

Conversation

hbelmiro commented Aug 9, 2024 • edited Loading

The issue resolved by this Pull Request:

Description of the changes:

Testing instructions

Checklist

dsp-developers commented Aug 9, 2024

dsp-developers commented Aug 30, 2024

VaniHaripriya commented Sep 4, 2024 • edited Loading

gregsheremeta Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregsheremeta Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HumairAK Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 6, 2024

dsp-developers commented Sep 6, 2024

dsp-developers commented Sep 6, 2024

dsp-developers commented Sep 6, 2024

hbelmiro commented Aug 9, 2024 •

edited

Loading

VaniHaripriya commented Sep 4, 2024 •

edited

Loading

gregsheremeta Sep 6, 2024 •

edited

Loading

gregsheremeta Sep 6, 2024 •

edited

Loading

HumairAK Sep 6, 2024 •

edited

Loading