Add support for default Ingress Cert #1022

VaishnaviHire · 2024-05-22T19:43:21Z

Jira Issue: https://issues.redhat.com/browse/RHOAIENG-4221

Description

How Has This Been Tested?

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

zdtsw · 2024-05-23T01:17:05Z

apis/infrastructure/v1/cert_types.go

-	// +kubebuilder:default=SelfSigned
+	// * DefaultIngress: Default ingress certificate configured for OpenShift
+	// +kubebuilder:validation:Enum=SelfSigned;Provided;DefaultIngress
+	// +kubebuilder:default=DefaultIngress


if we change the default value, it can cause problem for these users did not set it in old release.

I think this should be okay. By default users will use default certs. I guess we need to document this change. @israel-hdez Can you confirm?

yes defaultIngress would be the default one for users. If we document it, it would be enough.

for users who already installed kserve, from my understanding, the dci is not updated automatically when they upgraded, right? If so, we can provide a way how to use DefaultIngress.

Yep, if there is an existing DSCI/DSC, it won't be updated. Currently this has been introduced in dsc

Existent installs will have this field populated with the previous default (we are not using pointers). Thus, a new default here would not hurt.

(I suspect that's the reason why the sample has it?

opendatahub-operator/config/samples/datasciencecluster_v1_datasciencecluster.yaml

Line 24 in df469bb

type: SelfSigned

)

pkg/feature/cert.go

pkg/feature/serverless/resources.go

Jooho · 2024-05-29T13:47:16Z

/lgtm

VaishnaviHire · 2024-05-29T14:12:54Z

@Jooho @israel-hdez Is there any changes that we need in odh-model-controller?

VaishnaviHire · 2024-05-29T15:18:42Z

/hold until Edgar's reviews

bartoszmajsak

I shared a few minor comments in the code itself, but there's one big thing we should also address - I don't see a reason why new cert/secret logic should be coupled with Feature struct. I provided a patch you could apply, as this is certainly way too much to comment (even with suggestions).

PTAL at this patch.

Happy to explain the details. The original code (CreateSelfSignedCertificate) was clearly a missed opportunity in the original PR so I felt responsible for cleaning it up :)

Besides that, a couple of tests would really help in gaining confidence that all of it will work as intended. WDYT about adding a few e2e tests covering this as part of this PR?

bartoszmajsak · 2024-05-29T15:29:37Z

apis/infrastructure/v1/cert_types.go

-	Provided   CertType = "Provided"
+	SelfSigned     CertType = "SelfSigned"
+	Provided       CertType = "Provided"
+	DefaultIngress CertType = "DefaultIngress"


IIRC this is specific for openshift ingress, so perhaps it's better to give a more accurate name such as OpenshiftDefaultIngress? WDYT?

bartoszmajsak · 2024-05-29T15:33:07Z

controllers/datasciencecluster/datasciencecluster_controller.go

+	if a.GetObjectKind().GroupVersionKind().Kind == "CustomResourceDefinition" || a.GetName() == "ArgoWorkflowCRD" {
+		return []reconcile.Request{{
+			NamespacedName: types.NamespacedName{Name: requestName},
+		}}
 	}


I am not sure I grok how this is related. Should it deserve separate commit/PR if it's fixing something for a reason?

It is also used in the predicate a bit further down with datasciencepipelines.ArgoWorkflowCRD const

Yeah I just wanted to reuse code for request name so added a function r.getRequestName()

israel-hdez

This almost worked.
The missing part I identify is to set the right secret name in the Knative gateway. As it is, if the user does not specify the SecretName, a default name will be set. The name is set here:

opendatahub-operator/pkg/feature/serverless/loaders.go

Line 19 in df469bb

f.Spec.KnativeCertificateSecret = certificateSecretName

...related template:

opendatahub-operator/components/kserve/resources/servicemesh/routing/istio-ingress-gateway.tmpl.yaml

Line 17 in df469bb

credentialName: {{ .KnativeCertificateSecret }}

I can think two ways of doing it:

Modify that ServingDefaultValues so that it will use the name of the copied secret..... Although, looks like ServingDefaultValues func runs before the secret is copied, so it is hard to know what's going to be its name. So... IDK.
When copying the secret, use f.Spec.KnativeCertificateSecret contents as the name of the copied secret.

Without that change, models are failing to start on my trials. But once the right secret is configured in the Gateway, it is possible to validate these changes by:

Installing the operator.
Installing KServe using the DSC.
Deploying any sample model.
Get/download the public certificate that the URL of the deployed model is exposing (hint: https://stackoverflow.com/a/7886248).
Get/download the public certificate of some other Route (e.g. the OpenShift Console URL).
Compare both downloaded certificates and check that they are the same.

israel-hdez · 2024-05-29T18:24:58Z

apis/infrastructure/v1/cert_types.go

-	// +kubebuilder:default=SelfSigned
+	// * DefaultIngress: Default ingress certificate configured for OpenShift
+	// +kubebuilder:validation:Enum=SelfSigned;Provided;DefaultIngress
+	// +kubebuilder:default=DefaultIngress


Existent installs will have this field populated with the previous default (we are not using pointers). Thus, a new default here would not hurt.

(I suspect that's the reason why the sample has it?

opendatahub-operator/config/samples/datasciencecluster_v1_datasciencecluster.yaml

Line 24 in df469bb

type: SelfSigned

)

israel-hdez · 2024-05-29T19:03:20Z

controllers/datasciencecluster/datasciencecluster_controller.go

@@ -436,26 +437,17 @@ func (r *DataScienceClusterReconciler) SetupWithManager(mgr ctrl.Manager) error
 		Watches(&source.Kind{Type: &corev1.ConfigMap{}}, handler.EnqueueRequestsFromMapFunc(r.watchDataScienceClusterResources), builder.WithPredicates(configMapPredicates)).
 		Watches(&source.Kind{Type: &apiextensionsv1.CustomResourceDefinition{}}, handler.EnqueueRequestsFromMapFunc(r.watchDataScienceClusterResources),
 			builder.WithPredicates(argoWorkflowCRDPredicates)).
+		Watches(&source.Kind{Type: &corev1.Secret{}}, handler.EnqueueRequestsFromMapFunc(r.watchDefaultIngressSecret), builder.WithPredicates(defaultIngressCertSecretPredicates)).


I'm not fully sure how Watches work. But it is worth to check that you won't fall under the issue described here: https://issues.redhat.com/browse/RHOAIENG-1006

Yep this watch was added to reconcile operator when a certificate is expired and secret gets deleted

You just need to be careful. We've been seeing OOMKills with clusters with many secrets.

israel-hdez · 2024-05-29T19:20:59Z

config/rbac/role.yaml

+  - delete
+  - get
+  - list
+  - patch
+  - watch


Can't we omit delete and patch?

israel-hdez · 2024-05-29T19:27:32Z

pkg/feature/cert.go

+		client.InNamespace("openshift-ingress-operator"),
+	}
+
+	err := cli.List(context.TODO(), defaultIngressCtrlList, listOpts...)


Isn't it better to directly get the default one? (https://docs.openshift.com/container-platform/4.15/networking/ingress-operator.html#nw-ingress-view_configuring-ingress)

I think we want to always use the default one. Getting a list of the default ones may not be deterministic, and this may choose a different one if the user has created their own ingresses.

Yeah I was not sure if there would be an edge case where the default is named as something else

it does not have to be part of Feature struct/methods Signed-off-by: bartoszmajsak <[email protected]>

VaishnaviHire · 2024-06-17T15:38:42Z

@zdtsw @israel-hdez @bartoszmajsak I have added e2e tests and also addressed all the comments.

VaishnaviHire · 2024-06-18T11:12:51Z

/test opendatahub-operator-e2e

zdtsw · 2024-06-19T08:52:24Z

/test opendatahub-operator-e2e

zdtsw · 2024-06-19T11:20:24Z

/lgtm

* Add support for default ingress cert * Refactor watches * Add Watch for Ingress cert secret * Update manifests * Fix linters and api docs * Update scheme and roles * Address comments * Update tests to match default values * chore: refactors cert creation it does not have to be part of Feature struct/methods Signed-off-by: bartoszmajsak <[email protected]> * Change default value to OpenshiftDefaultIngress * Update default secret * Add e2e tests * Update tests for Managed Serving * Update serverless tests * Sync manifests

PR opendatahub-io#1022 promoted knative default secret name as the default for entire platform. This can be confusing, therefore this change makes it local to kserve again.

PR #1022 promoted knative default secret name as the default for entire platform. This can be confusing, therefore this change makes it local to kserve again.

…#1067) PR opendatahub-io#1022 promoted knative default secret name as the default for entire platform. This can be confusing, therefore this change makes it local to kserve again.

* Add support for default ingress cert * Refactor watches * Add Watch for Ingress cert secret * Update manifests * Fix linters and api docs * Update scheme and roles * Address comments * Update tests to match default values * chore: refactors cert creation it does not have to be part of Feature struct/methods Signed-off-by: bartoszmajsak <[email protected]> * Change default value to OpenshiftDefaultIngress * Update default secret * Add e2e tests * Update tests for Managed Serving * Update serverless tests * Sync manifests

…#1067) PR opendatahub-io#1022 promoted knative default secret name as the default for entire platform. This can be confusing, therefore this change makes it local to kserve again.

VaishnaviHire requested review from bartoszmajsak, zdtsw and israel-hdez May 22, 2024 19:43

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

openshift-ci bot requested review from cam-garrison and LaVLaS May 22, 2024 19:43

VaishnaviHire removed request for LaVLaS and cam-garrison May 22, 2024 19:45

VaishnaviHire force-pushed the add_cert_type branch from 04859e7 to 2386ca3 Compare May 22, 2024 20:45

zdtsw reviewed May 23, 2024

View reviewed changes

VaishnaviHire changed the title ~~[WIP] Add support for default Ingress Cert~~ Add support for default Ingress Cert May 23, 2024

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

VaishnaviHire force-pushed the add_cert_type branch 2 times, most recently from 766cd78 to b827fcc Compare May 23, 2024 18:26

VaishnaviHire requested a review from Jooho May 28, 2024 16:45

openshift-ci bot assigned Jooho May 29, 2024

openshift-ci bot added the lgtm label May 29, 2024

Jooho removed their assignment May 29, 2024

zdtsw approved these changes May 29, 2024

View reviewed changes

openshift-ci bot assigned zdtsw May 29, 2024

openshift-ci bot added the approved label May 29, 2024

openshift-ci bot added the do-not-merge/hold label May 29, 2024

bartoszmajsak requested changes May 29, 2024

View reviewed changes

openshift-ci bot assigned bartoszmajsak May 29, 2024

openshift-ci bot removed the lgtm label May 29, 2024

israel-hdez reviewed May 29, 2024

View reviewed changes

VaishnaviHire force-pushed the add_cert_type branch from b827fcc to 968a0ad Compare May 30, 2024 19:46

VaishnaviHire added 9 commits June 17, 2024 10:38

Update scheme and roles

6524780

Address comments

fbcdc3f

Update tests to match default values

44277dd

chore: refactors cert creation

7029821

it does not have to be part of Feature struct/methods Signed-off-by: bartoszmajsak <[email protected]>

Change default value to OpenshiftDefaultIngress

ba53b70

Update default secret

478c11a

Add e2e tests

3a74faf

Update tests for Managed Serving

0e4ce94

Update serverless tests

6a6c7a7

VaishnaviHire force-pushed the add_cert_type branch from 08ba557 to 6a6c7a7 Compare June 17, 2024 15:26

openshift-ci bot removed the lgtm label Jun 17, 2024

openshift-merge-robot removed the needs-rebase label Jun 17, 2024

VaishnaviHire removed the do-not-merge/hold label Jun 17, 2024

Sync manifests

22bfea9

VaishnaviHire added the rhoai-2.11 label Jun 18, 2024

zdtsw mentioned this pull request Jun 19, 2024

[sync]: odh to rhoai for 2.11 red-hat-data-services/rhods-operator#288

Merged

7 tasks

openshift-ci bot added the lgtm label Jun 19, 2024

openshift-merge-bot bot merged commit ea81e86 into opendatahub-io:incubation Jun 19, 2024
8 checks passed

bartoszmajsak mentioned this pull request Jun 20, 2024

fix: makes default knative secret local for serverless #1067

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for default Ingress Cert #1022

Add support for default Ingress Cert #1022

VaishnaviHire commented May 22, 2024 •

edited

Loading

zdtsw May 23, 2024

VaishnaviHire May 23, 2024

Jooho May 28, 2024

Jooho May 28, 2024

VaishnaviHire May 28, 2024

israel-hdez May 29, 2024

Jooho commented May 29, 2024

VaishnaviHire commented May 29, 2024

VaishnaviHire commented May 29, 2024

bartoszmajsak left a comment •

edited

Loading

bartoszmajsak May 29, 2024

bartoszmajsak May 29, 2024

bartoszmajsak May 29, 2024

VaishnaviHire May 30, 2024

israel-hdez left a comment

israel-hdez May 29, 2024

israel-hdez May 29, 2024

VaishnaviHire May 30, 2024

israel-hdez Jun 13, 2024

israel-hdez May 29, 2024

israel-hdez May 29, 2024

VaishnaviHire May 30, 2024

VaishnaviHire commented Jun 17, 2024

VaishnaviHire commented Jun 18, 2024

zdtsw commented Jun 19, 2024

zdtsw commented Jun 19, 2024

Add support for default Ingress Cert #1022

Add support for default Ingress Cert #1022

Conversation

VaishnaviHire commented May 22, 2024 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

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

Jooho commented May 29, 2024

VaishnaviHire commented May 29, 2024

VaishnaviHire commented May 29, 2024

bartoszmajsak left a comment • 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

israel-hdez left a 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

VaishnaviHire commented Jun 17, 2024

VaishnaviHire commented Jun 18, 2024

zdtsw commented Jun 19, 2024

zdtsw commented Jun 19, 2024

VaishnaviHire commented May 22, 2024 •

edited

Loading

bartoszmajsak left a comment •

edited

Loading