-
Notifications
You must be signed in to change notification settings - Fork 29
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
add oauth-proxy to rawdeployments if odh auth label is present #419
add oauth-proxy to rawdeployments if odh auth label is present #419
Conversation
Skipping CI for Draft Pull Request. |
pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/kube_ingress_reconciler.go
Fixed
Show fixed
Hide fixed
360560d
to
67041cc
Compare
tests passing locally, the failures look like test infra failures /retest-required |
67041cc
to
2cce5bc
Compare
44ba660
to
da8dc3a
Compare
/retest-required |
not sure what's happening with the tests, they are passing locally :( |
pkg/controller/v1beta1/inferenceservice/reconcilers/deployment/deployment_reconciler.go
Outdated
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/service/service_reconciler.go
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/kube_ingress_reconciler.go
Outdated
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/deployment/deployment_reconciler.go
Outdated
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/deployment/deployment_reconciler.go
Outdated
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/kube_ingress_reconciler.go
Outdated
Show resolved
Hide resolved
the e2e raw test failure is because https://github.com/opendatahub-io/kserve/blob/master/test/scripts/openshift-ci/run-e2e-tests.sh needs to be updated with the new behavior. Imo it can be ignored now can will be fixed later in https://issues.redhat.com/browse/RHOAIENG-14604 |
pkg/controller/v1beta1/inferenceservice/reconcilers/deployment/deployment_reconciler.go
Outdated
Show resolved
Hide resolved
// Check if the route is admitted | ||
for _, ingress := range route.Status.Ingress { | ||
for _, condition := range ingress.Conditions { | ||
if condition.Type == "Admitted" && condition.Status == "True" { |
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.
There might be a constant from netv1 for "Admitted"
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.
is there a constant as Yuan suggested?
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.
updates on this?
pkg/controller/v1beta1/inferenceservice/reconcilers/deployment/deployment_reconciler.go
Outdated
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/deployment/deployment_reconciler.go
Outdated
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/kube_ingress_reconciler.go
Outdated
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/kube_ingress_reconciler.go
Outdated
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/service/service_reconciler.go
Outdated
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/service/service_reconciler.go
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/deployment/deployment_reconciler.go
Show resolved
Hide resolved
@@ -1264,6 +1270,109 @@ var _ = Describe("v1beta1 inference service controller", func() { | |||
Eventually(func() error { return k8sClient.Get(context.TODO(), predictorHPAKey, actualHPA) }, timeout). | |||
Should(HaveOccurred()) | |||
}) | |||
It("Should have no ingress created if labeled as cluster-local", func() { |
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.
Perhaps, contribute this one to upstream?
pkg/controller/v1beta1/inferenceservice/rawkube_controller_test.go
Outdated
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/service/service_reconciler.go
Show resolved
Hide resolved
Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>
Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>
pkg/controller/v1beta1/inferenceservice/reconcilers/deployment/deployment_reconciler.go
Show resolved
Hide resolved
} | ||
if val, ok := componentMeta.Labels[constants.ODHKserveRawAuth]; ok && val == "true" { | ||
switch { | ||
case componentExt != nil && componentExt.Batcher != nil: |
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.
something like:
- --openshift-service-account=oauth-proxy
- --client-id=system:serviceaccount:my-namespace:oauth-proxy
- --client-secret-file=/var/run/secrets/kubernetes.io/serviceaccount/token
pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/kube_ingress_reconciler.go
Outdated
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/raw/raw_kube_reconciler.go
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/service/service_reconciler.go
Outdated
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/kube_ingress_reconciler.go
Outdated
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/deployment/deployment_reconciler.go
Outdated
Show resolved
Hide resolved
a690bb3
to
d9b067e
Compare
Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>
d9b067e
to
8a3c76b
Compare
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.
Quite fine. I'll do some testings on my Tuesday,
pkg/controller/v1beta1/inferenceservice/reconcilers/deployment/deployment_reconciler.go
Outdated
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/deployment/deployment_reconciler_test.go
Outdated
Show resolved
Hide resolved
@@ -457,7 +461,7 @@ func TestCreateDefaultDeployment(t *testing.T) { | |||
ttExpected := getDefaultExpectedDeployment() | |||
|
|||
// update objectMeta using modify func | |||
got := createRawDeployment(ttArgs.objectMeta, ttArgs.workerObjectMeta, ttArgs.componentExt, tt.modifyArgs(ttArgs).podSpec, tt.modifyArgs(ttArgs).workerPodSpec) | |||
got, _ := createRawDeployment(clientset, ttArgs.objectMeta, ttArgs.workerObjectMeta, ttArgs.componentExt, tt.modifyArgs(ttArgs).podSpec, tt.modifyArgs(ttArgs).workerPodSpec) |
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.
this one is the same.
@@ -760,7 +764,7 @@ func TestCreateDefaultDeployment(t *testing.T) { | |||
ttExpected := getDefaultExpectedDeployment() | |||
|
|||
// update objectMeta using modify func | |||
got := createRawDeployment(tt.modifyObjectMetaArgs(ttArgs).objectMeta, tt.modifyWorkerObjectMetaArgs(ttArgs).workerObjectMeta, ttArgs.componentExt, tt.modifyPodSpecArgs(ttArgs).podSpec, tt.modifyWorkerPodSpecArgs(ttArgs).workerPodSpec) | |||
got, _ := createRawDeployment(clientset, tt.modifyObjectMetaArgs(ttArgs).objectMeta, tt.modifyWorkerObjectMetaArgs(ttArgs).workerObjectMeta, ttArgs.componentExt, tt.modifyPodSpecArgs(ttArgs).podSpec, tt.modifyWorkerPodSpecArgs(ttArgs).workerPodSpec) |
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.
same here
pkg/controller/v1beta1/inferenceservice/reconcilers/raw/raw_kube_reconciler.go
Show resolved
Hide resolved
pkg/controller/v1beta1/inferenceservice/reconcilers/deployment/deployment_reconciler.go
Show resolved
Hide resolved
Manual testingSince controller functionality is the concern, all tests are using this gist as a reference, which is using a SKLearn runtime with a basic model. Despite the SKLearn runtime is not an ODH-supported one, we still should be able to verify correctness of controller functionality. Testing is done by deploying kserve-controller from this PR, and also odh-model-controller from PR opendatahub-io/odh-model-controller#274. ODH setup is done with a custom build of odh-operator. The setup is a standard Serverless setup. For the testings, the deploymentMode annotation is used to switch the mode to Raw. Quick regression testing for Serverless mode🟢 OK Follow the mentioned gist as is: https://gist.github.com/israel-hdez/af374562ef9e5b9d80890aa6f0bce20d Deploy a slim InferenceService in Raw deployment mode🔴 Fails Follow the gist, but use these annotations in the InferenceService: annotations:
serving.kserve.io/deploymentMode: "RawDeployment" 🟢 The model seems to deploy fine oc get isvc sklearn-v2-iris-test1
NAME URL READY PREV LATEST PREVROLLEDOUTREVISION LATESTREADYREVISION AGE
sklearn-v2-iris-test1 https://sklearn-v2-iris-test1-kserve-raw-tests.example.com True 10m
# ---
oc get isvc sklearn-v2-iris-test1 -o yaml | yq '{"address": .status.address, "components": .status.components, "url": .status.url}'
address:
url: https://sklearn-v2-iris-test1-predictor.kserve-raw-tests.svc.cluster.local # OK
components:
predictor:
url: http://sklearn-v2-iris-test1-predictor-kserve-raw-tests.example.com # Wrong
url: https://sklearn-v2-iris-test1-kserve-raw-tests.example.com # Wrong Deploy a Raw InferenceService with explicit private label🔴 Fails Follow the gist, but use these metadata in the InferenceService: annotations:
serving.kserve.io/deploymentMode: "RawDeployment"
labels:
networking.kserve.io/visibility: "cluster-local" 🟢 The model seems to deploy fine oc get isvc sklearn-v2-iris-test3
NAME URL READY PREV LATEST PREVROLLEDOUTREVISION LATESTREADYREVISION AGE
sklearn-v2-iris-test3 https://sklearn-v2-iris-test3-kserve-raw-tests.example.com True 73s
# ---
oc get isvc sklearn-v2-iris-test3 -o yaml | yq '{"address": .status.address, "components": .status.components, "url": .status.url}'
address:
url: https://sklearn-v2-iris-test3-predictor.kserve-raw-tests.svc.cluster.local # OK
components:
predictor:
url: http://sklearn-v2-iris-test3-predictor-kserve-raw-tests.example.com # Wrong
url: https://sklearn-v2-iris-test3-kserve-raw-tests.example.com # Wrong Deploy a Raw InferenceService with explicit exposed label🔴 Fails Follow the gist, but use these metadata in the InferenceService: annotations:
serving.kserve.io/deploymentMode: "RawDeployment"
labels:
networking.kserve.io/visibility: "exposed" 🟢 The model seems to deploy fine 🔴 Reconcile error in odh-model-controller:
🔴 Reconcile error in kserve-controller (surely a consequence of the previous):
Deploy a slim Raw InferenceService with auth enabled🔴 Fails Follow the gist, but use these metadata in the InferenceService: annotations:
serving.kserve.io/deploymentMode: "RawDeployment"
labels:
networking.kserve.io/visibility: "exposed" 🔴 The model does not deploy; kserve-controller reconcile error:
Deploy a Raw InferenceService with auth enabled and explicit private label🔴 Fails Follow the gist, but use these metadata in the InferenceService: annotations:
serving.kserve.io/deploymentMode: "RawDeployment"
labels:
networking.kserve.io/visibility: "cluster-local"
security.opendatahub.io/enable-auth: "true" 🔴 The model does not deploy; kserve-controller reconcile error:
Deploy a Raw InferenceService with auth enabled and route enabled🔴 Fails Follow the gist, but use these metadata in the InferenceService: annotations:
serving.kserve.io/deploymentMode: "RawDeployment"
labels:
networking.kserve.io/visibility: "exposed"
security.opendatahub.io/enable-auth: "true" 🔴 The model does not deploy; kserve-controller reconcile error:
Other notes
|
Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>
6289cae
to
0aa3eee
Compare
/test images |
Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>
@VedantMahabaleshwarkar: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Manual testing - round 2(See previous test round for background) Quick regression testing for Serverless mode🟢 OK Follow the mentioned gist as is: https://gist.github.com/israel-hdez/af374562ef9e5b9d80890aa6f0bce20d Deploy a slim InferenceService in Raw deployment mode🟡 OK, needs follow-up PR for fixes Follow the gist, but use these annotations in the InferenceService: annotations:
serving.kserve.io/deploymentMode: "RawDeployment" 🟢 The model seems to deploys fine oc get isvc sklearn-v2-iris-test1 -o yaml | yq '{"address": .status.address, "components": .status.components, "url": .status.url}'
address:
url: https://sklearn-v2-iris-test1-predictor.kserve-raw-tests.svc.cluster.local # Schema should be plain-text HTTP
components:
predictor:
url: http://sklearn-v2-iris-test1-predictor-kserve-raw-tests.example.com # Wrong
url: http://sklearn-v2-iris-test1-predictor.kserve-raw-tests.svc.cluster.local # OK Deploy a Raw InferenceService with explicit private label🟡 OK, needs follow-up PR for fixes Follow the gist, but use these metadata in the InferenceService: annotations:
serving.kserve.io/deploymentMode: "RawDeployment"
labels:
networking.kserve.io/visibility: "cluster-local" 🟢 The model seems to deploys fine oc get isvc sklearn-v2-iris-test2 -o yaml | yq '{"address": .status.address, "components": .status.components, "url": .status.url}'
address:
url: https://sklearn-v2-iris-test2-predictor.kserve-raw-tests.svc.cluster.local # Schema should be HTTP
components:
predictor:
url: http://sklearn-v2-iris-test2-predictor-kserve-raw-tests.example.com # Wrong
url: http://sklearn-v2-iris-test2-predictor.kserve-raw-tests.svc.cluster.local # OK Deploy a Raw InferenceService with explicit exposed label🔴 Fails Follow the gist, but use these metadata in the InferenceService: annotations:
serving.kserve.io/deploymentMode: "RawDeployment"
labels:
networking.kserve.io/visibility: "exposed" 🟢 The model seems to deploys fine
Deploy a slim Raw InferenceService with auth enabled🟡 OK, needs follow-up PR for fixes Follow the gist, but use these metadata in the InferenceService: annotations:
serving.kserve.io/deploymentMode: "RawDeployment"
labels:
security.opendatahub.io/enable-auth: "true" 🟢 The model deploys fine oc get isvc sklearn-v2-iris-test4 -o yaml | yq '{"address": .status.address, "components": .status.components, "url": .status.url}'
address:
url: https://sklearn-v2-iris-test4-predictor.kserve-raw-tests.svc.cluster.local:8443 # OK
components:
predictor:
url: http://sklearn-v2-iris-test4-predictor-kserve-raw-tests.example.com # Wrong
url: http://sklearn-v2-iris-test4-predictor.kserve-raw-tests.svc.cluster.local:8443 # Schema should be HTTPS. Deploy a Raw InferenceService with auth enabled and explicit private label🟡 OK, needs follow-up PR for fixes Follow the gist, but use these metadata in the InferenceService: annotations:
serving.kserve.io/deploymentMode: "RawDeployment"
labels:
networking.kserve.io/visibility: "cluster-local"
security.opendatahub.io/enable-auth: "true" 🟡 Same results as previous. Deploy a Raw InferenceService with auth enabled and route enabled🟡 OK, needs follow-up PR for fixes Follow the gist, but use these metadata in the InferenceService: annotations:
serving.kserve.io/deploymentMode: "RawDeployment"
labels:
networking.kserve.io/visibility: "exposed"
security.opendatahub.io/enable-auth: "true" 🟢 The model deploys fine oc get isvc sklearn-v2-iris-test6 -o yaml | yq '{"address": .status.address, "components": .status.components, "url": .status.url}'
address:
url: https://sklearn-v2-iris-test6-predictor.kserve-raw-tests.svc.cluster.local:8443 # OK
components:
predictor:
url: http://sklearn-v2-iris-test6-predictor-kserve-raw-tests.example.com # Wrong
url: https://sklearn-v2-iris-test6-kserve-raw-tests.apps-crc.testing # OK Other notes
|
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.
/lgtm
Approving, but needs follow-up work.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: israel-hdez, VedantMahabaleshwarkar 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 |
…atahub-io#419) * add oauth-proxy to rawdeployments if odh auth label is present * remove ingress modifications * bug fix * consume oauth proxy params from configmap * fix oauth proxy sar and minor bugs * revert some unneeded changes * add oauth proxy flag to prevent login page redirect on invalid request * address feedback * update to newer oauth proxy image * minor fix * fix unit test * more feedback * cookie secret * test and other fixes * fix lint issues * address latest feedback * missed import sort * address more feedback * bug fix * fix lint error (cherry picked from commit d987799) Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>
* add oauth-proxy to rawdeployments if odh auth label is present (#419) * add oauth-proxy to rawdeployments if odh auth label is present * remove ingress modifications * bug fix * consume oauth proxy params from configmap * fix oauth proxy sar and minor bugs * revert some unneeded changes * add oauth proxy flag to prevent login page redirect on invalid request * address feedback * update to newer oauth proxy image * minor fix * fix unit test * more feedback * cookie secret * test and other fixes * fix lint issues * address latest feedback * missed import sort * address more feedback * bug fix * fix lint error (cherry picked from commit d987799) Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> * introduce service configuration at configmap level (kserve#3672) (cherry picked from commit 23c0396) Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> * [RHOAIENG-17229] - Routing and Headless Service Support in KServe Raw Mode Deployment chore: Follow up: remove the hardcoded clsuterIP setting and add the service configuration. Signed-off-by: Spolti <[email protected]> (cherry picked from commit 33b1600) * [RHOAIENG-16851] - Rawdeployment bug fixes (#462) * [RHOAIENG-16851] fix scheme bugs in status.url and status.address.url for rawdeployment Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> * [RHOAIENG-16851] Remove component url temporarily Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> * [RHOAIENG-16851] Use transformer spec to set upstream port in oauth-proxy if a transformer-container is present Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> * [RHOAIENG-16851] address feedback Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> --------- Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> (cherry picked from commit 13b5166) * go.mod fixes Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> --------- Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> Co-authored-by: Filippe Spolti <[email protected]>
What this PR does / why we need it:
This PR adds the following :
"security.opendatahub.io/enable-auth" = "true"
-- an oauth proxy container is added to the deployment
-- http port is replaced by https port in the service
--
"service.beta.openshift.io/serving-cert-secret-name"
is added to the service to allow creation of the tls secretWhich issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes # https://issues.redhat.com/browse/RHOAIENG-10291, https://issues.redhat.com/browse/RHOAIENG-13444
TEST WITH: opendatahub-io/odh-model-controller#274
Type of changes
Please delete options that are not relevant.
Feature/Issue validation/testing:
To test enabling route for a particular ISVC
-- Create any kserve isvc+SR
-- isvc should have annotation :
"serving.kserve.io/deploymentMode" : RawDeployment
-- isvc should have label :
"networking.kserve.io/visibility": "enable-route"
To test route with auth:
-- Create any kserve isvc+SR
-- isvc should have annotation :
"serving.kserve.io/deploymentMode" : RawDeployment
-- isvc should have label:
"security.opendatahub.io/enable-auth" = "true"
-- isvc should have label:
"networking.kserve.io/visibility": "enable-route"
To test Inference without auth:
-- remove isvc label
"security.opendatahub.io/enable-auth" = "true"
Now inference should work without token
Checklist:
Release note:
Re-running failed tests
/rerun-all
- rerun all failed workflows./rerun-workflow <workflow name>
- rerun a specific failed workflow. Only one workflow name can be specified. Multiple /rerun-workflow commands are allowed per comment.