-
Notifications
You must be signed in to change notification settings - Fork 873
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.
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
Adds kfserving ingressgateway for Istio 1.1.6 #949
Adds kfserving ingressgateway for Istio 1.1.6 #949
Conversation
Modifies KFServing and KNative Serving config-maps to use this gateway
/hold cancel /assign @yuzisun @animeshsingh |
Note that PR for Istio 1.3.1 will be raised separately. This has been verfied in kfctl_k8s_istio and kfctl_gcp_iap configurations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @krishnadurai
cc @Tomcli
/lgtm
port: 15030 | ||
targetPort: 15030 | ||
- name: http2-grafana | ||
port: 15031 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these ports for kiali/grafana/prometheus always constant? do we deploy them with kubeflow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@animeshsingh yes they are associated with the deployment config and are constant. We do deploy these services with Istio in Kubeflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thaks - great to know. What's the total infra cost of these 3 services - believe a container each? Are any customized Kubeflow metrics configured to flow to them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@animeshsingh These require a container each. Prometheus, Graphana etc do come with the default istio install in Kubeflow.
Kubeflow does report Prometheus metrics through various operators or services like tfjob-operator or profile-controller. Likewise, the gateway container istio-proxy also reports metrics.
These ports in question are ports which the istio-proxy container exposes. The container istio-proxy is a part of the deployment servicing kfserving-gateway. These ports host https end-points for each of these services like Prometheus to poll these (gateway) services for metrics.
app: kfserving-ingressgateway | ||
kfserving: ingressgateway | ||
spec: | ||
type: LoadBalancer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks the kubeflow istio ingressgateway is using NodePort https:/kubeflow/manifests/blob/master/istio/istio-install/base/istio-noauth.yaml#L14047
Would this work for on prem setup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yuzisun The reason for istio-ingressgateway being NodePort is because of Google's IAP needing it to be NodePort.
Would this work for on prem setup?
If an on-premise setup has some component to provision a LoadBalancer, then LoadBalancer will get its IP. If not, the NodePorts are still accessible though the IP field for LoadBalancer will be in state.
NAME TYPE CLUSTER-IP EXTERNAL-IP PORT(S) AGE
istio-ingressgateway LoadBalancer 10.103.151.45 <pending> 15020:30047/TCP,80:31380/TCP,443:31390/TCP,31400:31400/TCP,15029:30370/TCP,15030:31766/TCP,15031:32168/TCP,15032:31581/TCP,15443:30268/TCP 2d12h
IMO, This should be acceptable. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are using this ingress gateway in GCP IAP setup, so that's ok ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In istio-ingressgateway for GCP IAP, the ingress gateway service is exposed to an Identity Aware Proxy (which has auth checking) by the NodePort and the intention is to disallow access through any other mechanism other than this proxy.
Since we don't want to have auth checks, LoadBalancer should be alright.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, thanks for the explanation! btw we also need to also add the HPA for this ingress gateway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion @yuzisun. I'll include the definition for HPA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yuzisun I've included an HPA.
Thanks @krishnadurai! This is awesome and removes the biggest road block for kubeflow integration! /lgtm |
Thanks @yuzisun! You have been super helpful throughout. @animeshsingh could you please take another look to get this in soon? |
@animeshsingh can you PTAL at this again? |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: animeshsingh 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:
Approvers can indicate their approval by writing |
/retest |
1 similar comment
/retest |
…erry-pick-of-kubeflow#949-upstream-v1.0-branch
…o 1.1.6 Modifies Cherry pick of #949 on v1.0-branch. #949: Adds kfserving ingressgateway for Istio 1.1.6 Modifies (#1027) * Adds kfserving ingressgateway for Istio 1.1.6 Modifies KFServing and KNative Serving config-maps to use this gateway * Updates configs for Istio 1.1.6 * Updates tests for istio 1.1.6 * Adding kfserving gateway to kfdefs having istio-1-1-6 * Gateway name correction for kfserving config * Adds HPA config to gateway Co-authored-by: krishnadurai <[email protected]>
…gateway for Istio 1.1.6 Modifies Cherry pick of kubeflow#949 on v1.0-branch. kubeflow#949: Adds kfserving ingressgateway for Istio 1.1.6 Modifies (kubeflow#1027)" This reverts commit 6cb83f8.
* Adds kfserving ingressgateway for Istio 1.1.6 Modifies KFServing and KNative Serving config-maps to use this gateway * Updates configs for Istio 1.1.6 * Updates tests for istio 1.1.6 * Adding kfserving gateway to kfdefs having istio-1-1-6 * Gateway name correction for kfserving config * Adds HPA config to gateway
* Adds kfserving ingressgateway for Istio 1.1.6 (#949) * Adds kfserving ingressgateway for Istio 1.1.6 Modifies KFServing and KNative Serving config-maps to use this gateway * Updates configs for Istio 1.1.6 * Updates tests for istio 1.1.6 * Adding kfserving gateway to kfdefs having istio-1-1-6 * Gateway name correction for kfserving config * Adds HPA config to gateway * revert changes to kfdef Co-authored-by: Krishna Durai <[email protected]>
Modifies KFServing and KNative Serving config-maps to use this gateway
Which issue is resolved by this Pull Request:
Addresses: #924
Description of your changes:
This implementation introduces an alternative Kubeflow ingress-gateway for KFServing. This route was chosen because the existing Kubeflow gateway is protected by Auth in several configurations. The Knative service readiness prober requests fail when they try to probe a KFServing InferenceService since those prober requests are not backed with Authentication credentials.
This implementation is a temporary solution until KNative introduces a solution for probing services behind an authenticated gateway as mentioned in this issue: knative/serving#6829
Related issues:
kserve/kserve#668
/cc @yuzisun @animeshsingh
/hold for testing
Checklist:
cd manifests/tests
make generate-changed-only
make test
This change is