Enabled the multiple domains support on an inference service #3615

houshengbo · 2024-04-19T20:13:02Z

What this PR does / why we need it:
Currently KServe supports setting only a single external host for inference services. After enabling the multi-host support, we can facilitate the use cases like migrating the inference services from one to another. This PR enables this feature on a global level with the field additionalIngressDomains in the configMap inferenceservice-config.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2747

Type of changes
Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Feature/Issue validation/testing:

Please describe the tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.

Test A
Test B
Logs

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Checklist:

Have you added unit/e2e tests that prove your fix is effective or that this feature works?
Has code been commented, particularly in hard-to-understand areas?
Have you made corresponding changes to the documentation?

Release note:

Enable the multiple domains support on an inference service.

houshengbo · 2024-04-22T14:37:18Z

/retest

spolti · 2024-04-22T14:47:00Z

pkg/apis/serving/v1beta1/configmap_test.go

+				"ingressService": "test-destination",
+				"localGateway": "knative-serving/knative-local-gateway",
+				"localGatewayService": "knative-local-gateway.istio-system.svc.cluster.local",
+                "urlScheme": "https",


missing indentation

spolti · 2024-04-22T14:50:51Z

pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler.go

@@ -432,6 +432,11 @@ func createIngress(isvc *v1beta1.InferenceService, useDefault bool, config *v1be
 		})
 		// Include ingressDomain to the domains (both internal and external) derived by KNative
 		hosts = append(hosts, url.Host)
+
+		// Include additional ingressDomain to the domains (both internal and external)
+		if config.AdditionalIngressDomains != nil && len(*config.AdditionalIngressDomains) > 0 {


Guess we need to do some validation here before append.
We could relay in the k8s validator api, or, does istio validates it?

This one should work the same as ingressConfig.IngressDomain. We just read the values from the CM and set them in this config. I did not find any validation for other fields. Do you see validation for other fields?

We will create or update the ingress returned by createIngress. If there is validation issue, the creation or update will fail. That's my assumption, but need to verify.

would help to verify if the values passed in additionalIngressDomain are valid URLs. This includes their length and presence of special characters

spolti · 2024-04-23T17:51:00Z

pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler.go

+		for _, domain := range *config.AdditionalIngressDomains {
+			host := fmt.Sprintf("%s.%s", prefix, domain)
+			if !IsValidHostURL(fmt.Sprintf("%s://%s", config.UrlScheme, host)) {
+				log.Error(fmt.Errorf("The domain name %s in the additionalIngressDomains is not a valid domain name.", domain),


Suggested change

log.Error(fmt.Errorf("The domain name %s in the additionalIngressDomains is not a valid domain name.", domain),

log.Error(fmt.Errorf("The domain name %s in the additionalIngressDomains is not valid", domain),

spolti · 2024-04-23T17:58:30Z

pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/domain.go

@@ -62,3 +63,8 @@ func GenerateDomainName(name string, obj metav1.ObjectMeta, ingressConfig *v1bet

 	return buf.String(), nil
 }
+
+func IsValidHostURL(hostURL string) bool {
+	_, err := url.ParseRequestURI(hostURL)


there is a package called validation in the apimachinery tool that could help with it.

IIRC:

validation.IsDNS1123Subdomain()

That might be useful here, as we are validating domains for ingress controller.

wdyt @israel-hdez @yuzisun ?

I change the way for us to validate the domain or subdomain.
The tricky part is how to get the subdomain. The safest way is to get the domain name for the existing hosts that knative serving generated, remove it and append the new additional ingress domain to get the new hosts.

Signed-off-by: Vincent Hou <shou73@bloomberg.net>

* Added the validation for the URLs Signed-off-by: Vincent Hou <shou73@bloomberg.net>

Signed-off-by: Vincent Hou <shou73@bloomberg.net>

israel-hdez · 2024-04-30T19:36:03Z

I quickly scanned the code and looks like it would work.

However, I'm wondering if we could try to rely less on Istio implementation and prefer using Knative resources? Knative already has some features to manage and customize domains.

I'd like to write a proposal for this. If it would be accepted, this PR code may become obsolete.

houshengbo · 2024-04-30T21:52:43Z

Kserve leverages istio as the default ingress, and knative serving is just one of options provisioning the services. It may not be enough if you only think of knative.

yuzisun · 2024-04-30T23:14:12Z

I quickly scanned the code and looks like it would work.

However, I'm wondering if we could try to rely less on Istio implementation and prefer using Knative resources? Knative already has some features to manage and customize domains.

I'd like to write a proposal for this. If it would be accepted, this PR code may become obsolete.

I think we still want an optional routing layer for KServe specific needs, we have implemented features like path based routing and logic for routing to different inference components. The virtual service can be disabled if not needed and in future we can migrate to gateway API so it does not rely on istio.

yuzisun · 2024-04-30T23:23:38Z

Nice work on the first major kserve PR! @houshengbo
/lgtm
/approve

oss-prow-bot · 2024-04-30T23:23:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexagriffith, houshengbo, yuzisun

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

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

israel-hdez · 2024-04-30T23:32:50Z

we have implemented features like path based routing

Hmm... OK, I hadn't this feature in mind.
Sorry for the noise.

…3615) * Added the field AdditionalIngressDomains into the struct IngressConfig Signed-off-by: Vincent Hou <shou73@bloomberg.net> * Added the additional ingress domains into the hosts Signed-off-by: Vincent Hou <shou73@bloomberg.net> * Fixed the indentation Signed-off-by: Vincent Hou <shou73@bloomberg.net> * Added isvc name and namespace into the domain name * Added the validation for the URLs Signed-off-by: Vincent Hou <shou73@bloomberg.net> * Validate the domain in the additionalIngressDomains Signed-off-by: Vincent Hou <shou73@bloomberg.net> * Create the hosts from the list of additionalIngressDomains Signed-off-by: Vincent Hou <shou73@bloomberg.net> * Change the way to validate the host Signed-off-by: Vincent Hou <shou73@bloomberg.net> * Change the validation error message Signed-off-by: Vincent Hou <shou73@bloomberg.net> * Revert the name to url Signed-off-by: Vincent Hou <shou73@bloomberg.net> * Get all the available domain list Signed-off-by: Vincent Hou <shou73@bloomberg.net> * gofmt -s -w the file Signed-off-by: Vincent Hou <shou73@bloomberg.net> * Add additionalIngressDomains into the charts Signed-off-by: Vincent Hou <shou73@bloomberg.net> * Added the comments and refactor the tests Signed-off-by: Vincent Hou <shou73@bloomberg.net> * Regenerate the manifests Signed-off-by: Vincent Hou <shou73@bloomberg.net> * Modify createHTTPMatchRequest, the charts and the test cases Signed-off-by: Vincent Hou <shou73@bloomberg.net> * Run make generate Signed-off-by: Vincent Hou <shou73@bloomberg.net> --------- Signed-off-by: Vincent Hou <shou73@bloomberg.net> Signed-off-by: asd981256 <asd981256@gmail.com>

oss-prow-bot bot added the do-not-merge/work-in-progress label Apr 19, 2024

oss-prow-bot bot requested review from rachitchauhan43 and terrytangyuan April 19, 2024 20:13

houshengbo force-pushed the enable-multiple-domain branch 3 times, most recently from 3833e49 to f28620c Compare April 19, 2024 20:17

houshengbo changed the title ~~WIP: Added the field AdditionalIngressDomains into the struct IngressConfig~~ WIP: Enabled the multiple domains support on an inference service Apr 19, 2024

houshengbo force-pushed the enable-multiple-domain branch 6 times, most recently from 9d7d588 to 8504c99 Compare April 22, 2024 14:27

houshengbo changed the title ~~WIP: Enabled the multiple domains support on an inference service~~ Enabled the multiple domains support on an inference service Apr 22, 2024

oss-prow-bot bot removed the do-not-merge/work-in-progress label Apr 22, 2024

houshengbo force-pushed the enable-multiple-domain branch from 8504c99 to a25510c Compare April 22, 2024 14:27

spolti reviewed Apr 22, 2024

View reviewed changes

houshengbo force-pushed the enable-multiple-domain branch 6 times, most recently from 7c99b8e to 98d5657 Compare April 23, 2024 17:06

spolti reviewed Apr 23, 2024

View reviewed changes

houshengbo force-pushed the enable-multiple-domain branch 3 times, most recently from a9d1feb to 9401b99 Compare April 24, 2024 16:29

houshengbo force-pushed the enable-multiple-domain branch from caa0dea to c840f59 Compare April 29, 2024 16:19

houshengbo added 15 commits April 29, 2024 12:42

Added the field AdditionalIngressDomains into the struct IngressConfig

13caf74

Signed-off-by: Vincent Hou <shou73@bloomberg.net>

Added the additional ingress domains into the hosts

5bf9055

Signed-off-by: Vincent Hou <shou73@bloomberg.net>

Fixed the indentation

1460d70

Signed-off-by: Vincent Hou <shou73@bloomberg.net>

Added isvc name and namespace into the domain name

41af4a7

* Added the validation for the URLs Signed-off-by: Vincent Hou <shou73@bloomberg.net>

Validate the domain in the additionalIngressDomains

f412b96

Signed-off-by: Vincent Hou <shou73@bloomberg.net>

Create the hosts from the list of additionalIngressDomains

aa1e05e

Signed-off-by: Vincent Hou <shou73@bloomberg.net>

Change the way to validate the host

4259679

Signed-off-by: Vincent Hou <shou73@bloomberg.net>

Change the validation error message

4839230

Signed-off-by: Vincent Hou <shou73@bloomberg.net>

Revert the name to url

5536719

Signed-off-by: Vincent Hou <shou73@bloomberg.net>

Get all the available domain list

1144116

Signed-off-by: Vincent Hou <shou73@bloomberg.net>

gofmt -s -w the file

39d427e

Signed-off-by: Vincent Hou <shou73@bloomberg.net>

Add additionalIngressDomains into the charts

fe25da9

Signed-off-by: Vincent Hou <shou73@bloomberg.net>

Added the comments and refactor the tests

b56f4c6

Signed-off-by: Vincent Hou <shou73@bloomberg.net>

Regenerate the manifests

33b303b

Signed-off-by: Vincent Hou <shou73@bloomberg.net>

Modify createHTTPMatchRequest, the charts and the test cases

3036579

Signed-off-by: Vincent Hou <shou73@bloomberg.net>

houshengbo force-pushed the enable-multiple-domain branch from c840f59 to 56d3cb5 Compare April 29, 2024 16:53

Run make generate

f589c58

Signed-off-by: Vincent Hou <shou73@bloomberg.net>

houshengbo force-pushed the enable-multiple-domain branch from 56d3cb5 to f589c58 Compare April 29, 2024 17:25

oss-prow-bot bot assigned yuzisun Apr 30, 2024

oss-prow-bot bot added the lgtm label Apr 30, 2024

oss-prow-bot bot added the approved label Apr 30, 2024

yuzisun merged commit 36a3e6d into kserve:master Apr 30, 2024
57 of 58 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabled the multiple domains support on an inference service #3615

Enabled the multiple domains support on an inference service #3615

houshengbo commented Apr 19, 2024 •

edited

houshengbo commented Apr 22, 2024

spolti Apr 22, 2024

spolti Apr 22, 2024

houshengbo Apr 22, 2024

houshengbo Apr 22, 2024

sukumargaonkar Apr 22, 2024

spolti Apr 23, 2024

spolti Apr 23, 2024

houshengbo Apr 24, 2024

israel-hdez commented Apr 30, 2024

houshengbo commented Apr 30, 2024

yuzisun commented Apr 30, 2024

yuzisun commented Apr 30, 2024

oss-prow-bot bot commented Apr 30, 2024

israel-hdez commented Apr 30, 2024

	log.Error(fmt.Errorf("The domain name %s in the additionalIngressDomains is not a valid domain name.", domain),
	log.Error(fmt.Errorf("The domain name %s in the additionalIngressDomains is not valid", domain),

Enabled the multiple domains support on an inference service #3615

Enabled the multiple domains support on an inference service #3615

Conversation

houshengbo commented Apr 19, 2024 • edited

houshengbo commented Apr 22, 2024

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

israel-hdez commented Apr 30, 2024

houshengbo commented Apr 30, 2024

yuzisun commented Apr 30, 2024

yuzisun commented Apr 30, 2024

oss-prow-bot bot commented Apr 30, 2024

israel-hdez commented Apr 30, 2024

houshengbo commented Apr 19, 2024 •

edited