Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add oauth-proxy to rawdeployments if odh auth label is present #419

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

add oauth-proxy to rawdeployments if odh auth label is present #419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
da8dc3a
add oauth-proxy to rawdeployments if odh auth label is present
VedantMahabaleshwarkar Oct 9, 2024
ae2a13a
remove ingress modifications
VedantMahabaleshwarkar Oct 15, 2024
a89f2bd
bug fix
VedantMahabaleshwarkar Oct 16, 2024
05ef572
consume oauth proxy params from configmap
VedantMahabaleshwarkar Oct 16, 2024
a94bdb6
fix oauth proxy sar and minor bugs
VedantMahabaleshwarkar Oct 16, 2024
25fd6aa
revert some unneeded changes
VedantMahabaleshwarkar Oct 16, 2024
363289e
minor fix for linter
VedantMahabaleshwarkar Oct 16, 2024
693027b
add oauth proxy flag to prevent login page redirect on invalid request
VedantMahabaleshwarkar Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 29 additions & 29 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,35 +139,35 @@ manifests: controller-gen
$(CONTROLLER_GEN) object:headerFile="hack/boilerplate.go.txt" paths=./pkg/apis/serving/v1alpha1
$(CONTROLLER_GEN) object:headerFile="hack/boilerplate.go.txt" paths=./pkg/apis/serving/v1beta1

#TODO Remove this until new controller-tools is released
perl -pi -e 's/storedVersions: null/storedVersions: []/g' config/crd/full/serving.kserve.io_inferenceservices.yaml
perl -pi -e 's/conditions: null/conditions: []/g' config/crd/full/serving.kserve.io_inferenceservices.yaml
perl -pi -e 's/Any/string/g' config/crd/full/serving.kserve.io_inferenceservices.yaml
perl -pi -e 's/storedVersions: null/storedVersions: []/g' config/crd/full/serving.kserve.io_trainedmodels.yaml
perl -pi -e 's/conditions: null/conditions: []/g' config/crd/full/serving.kserve.io_trainedmodels.yaml
perl -pi -e 's/Any/string/g' config/crd/full/serving.kserve.io_trainedmodels.yaml
perl -pi -e 's/storedVersions: null/storedVersions: []/g' config/crd/full/serving.kserve.io_inferencegraphs.yaml
perl -pi -e 's/conditions: null/conditions: []/g' config/crd/full/serving.kserve.io_inferencegraphs.yaml
perl -pi -e 's/Any/string/g' config/crd/full/serving.kserve.io_inferencegraphs.yaml
#remove the required property on framework as name field needs to be optional
yq 'del(.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.*.properties.*.required)' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
#remove ephemeralContainers properties for compress crd size https://github.com/kubeflow/kfserving/pull/1141#issuecomment-714170602
yq 'del(.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.*.properties.ephemeralContainers)' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
#knative does not allow setting port on liveness or readiness probe
yq 'del(.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.*.properties.*.properties.readinessProbe.properties.httpGet.required)' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
yq 'del(.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.*.properties.*.properties.livenessProbe.properties.httpGet.required)' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
yq 'del(.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.*.properties.*.properties.readinessProbe.properties.tcpSocket.required)' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
yq 'del(.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.*.properties.*.properties.livenessProbe.properties.tcpSocket.required)' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
yq 'del(.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.*.properties.containers.items.properties.livenessProbe.properties.httpGet.required)' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
yq 'del(.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.*.properties.containers.items.properties.readinessProbe.properties.httpGet.required)' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
#With v1 and newer kubernetes protocol requires default
yq '.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties | .. | select(has("protocol")) | path' config/crd/full/serving.kserve.io_inferenceservices.yaml -o j | jq -r '. | map(select(numbers)="["+tostring+"]") | join(".")' | awk '{print "."$$0".protocol.default"}' | xargs -n1 -I{} yq '{} = "TCP"' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
yq '.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties | .. | select(has("protocol")) | path' config/crd/full/serving.kserve.io_clusterservingruntimes.yaml -o j | jq -r '. | map(select(numbers)="["+tostring+"]") | join(".")' | awk '{print "."$$0".protocol.default"}' | xargs -n1 -I{} yq '{} = "TCP"' -i config/crd/full/serving.kserve.io_clusterservingruntimes.yaml
yq '.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties | .. | select(has("protocol")) | path' config/crd/full/serving.kserve.io_servingruntimes.yaml -o j | jq -r '. | map(select(numbers)="["+tostring+"]") | join(".")' | awk '{print "."$$0".protocol.default"}' | xargs -n1 -I{} yq '{} = "TCP"' -i config/crd/full/serving.kserve.io_servingruntimes.yaml
./hack/minimal-crdgen.sh
kubectl kustomize config/crd/full > test/crds/serving.kserve.io_inferenceservices.yaml
cp config/crd/minimal/* charts/kserve-crd-minimal/templates/
rm charts/kserve-crd-minimal/templates/kustomization.yaml
# #TODO Remove this until new controller-tools is released
# perl -pi -e 's/storedVersions: null/storedVersions: []/g' config/crd/full/serving.kserve.io_inferenceservices.yaml
# perl -pi -e 's/conditions: null/conditions: []/g' config/crd/full/serving.kserve.io_inferenceservices.yaml
# perl -pi -e 's/Any/string/g' config/crd/full/serving.kserve.io_inferenceservices.yaml
# perl -pi -e 's/storedVersions: null/storedVersions: []/g' config/crd/full/serving.kserve.io_trainedmodels.yaml
# perl -pi -e 's/conditions: null/conditions: []/g' config/crd/full/serving.kserve.io_trainedmodels.yaml
# perl -pi -e 's/Any/string/g' config/crd/full/serving.kserve.io_trainedmodels.yaml
# perl -pi -e 's/storedVersions: null/storedVersions: []/g' config/crd/full/serving.kserve.io_inferencegraphs.yaml
# perl -pi -e 's/conditions: null/conditions: []/g' config/crd/full/serving.kserve.io_inferencegraphs.yaml
# perl -pi -e 's/Any/string/g' config/crd/full/serving.kserve.io_inferencegraphs.yaml
# #remove the required property on framework as name field needs to be optional
# yq 'del(.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.*.properties.*.required)' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
# #remove ephemeralContainers properties for compress crd size https://github.com/kubeflow/kfserving/pull/1141#issuecomment-714170602
# yq 'del(.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.*.properties.ephemeralContainers)' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
Comment on lines +142 to +155
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is commenting these out intentional?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in update

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are still commented out?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm still seeing comments here...

# #knative does not allow setting port on liveness or readiness probe
# yq 'del(.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.*.properties.*.properties.readinessProbe.properties.httpGet.required)' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
# yq 'del(.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.*.properties.*.properties.livenessProbe.properties.httpGet.required)' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
# yq 'del(.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.*.properties.*.properties.readinessProbe.properties.tcpSocket.required)' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
# yq 'del(.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.*.properties.*.properties.livenessProbe.properties.tcpSocket.required)' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
# yq 'del(.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.*.properties.containers.items.properties.livenessProbe.properties.httpGet.required)' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
# yq 'del(.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties.*.properties.containers.items.properties.readinessProbe.properties.httpGet.required)' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
# #With v1 and newer kubernetes protocol requires default
# yq '.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties | .. | select(has("protocol")) | path' config/crd/full/serving.kserve.io_inferenceservices.yaml -o j | jq -r '. | map(select(numbers)="["+tostring+"]") | join(".")' | awk '{print "."$$0".protocol.default"}' | xargs -n1 -I{} yq '{} = "TCP"' -i config/crd/full/serving.kserve.io_inferenceservices.yaml
# yq '.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties | .. | select(has("protocol")) | path' config/crd/full/serving.kserve.io_clusterservingruntimes.yaml -o j | jq -r '. | map(select(numbers)="["+tostring+"]") | join(".")' | awk '{print "."$$0".protocol.default"}' | xargs -n1 -I{} yq '{} = "TCP"' -i config/crd/full/serving.kserve.io_clusterservingruntimes.yaml
# yq '.spec.versions[0].schema.openAPIV3Schema.properties.spec.properties | .. | select(has("protocol")) | path' config/crd/full/serving.kserve.io_servingruntimes.yaml -o j | jq -r '. | map(select(numbers)="["+tostring+"]") | join(".")' | awk '{print "."$$0".protocol.default"}' | xargs -n1 -I{} yq '{} = "TCP"' -i config/crd/full/serving.kserve.io_servingruntimes.yaml
# ./hack/minimal-crdgen.sh
# kubectl kustomize config/crd/full > test/crds/serving.kserve.io_inferenceservices.yaml
# cp config/crd/minimal/* charts/kserve-crd-minimal/templates/
# rm charts/kserve-crd-minimal/templates/kustomization.yaml

# Run go fmt against code
fmt:
Expand Down
6 changes: 5 additions & 1 deletion cmd/manager/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import (
v1beta1controller "github.com/kserve/kserve/pkg/controller/v1beta1/inferenceservice"
"github.com/kserve/kserve/pkg/webhook/admission/pod"
"github.com/kserve/kserve/pkg/webhook/admission/servingruntime"
routev1 "github.com/openshift/api/route/v1"
)

var (
Expand Down Expand Up @@ -187,7 +188,10 @@ func main() {
}
}
}

if err = routev1.AddToScheme(mgr.GetScheme()); err != nil {
setupLog.Error(err, "unable to add routev1 APIs to scheme")
os.Exit(1)
}
setupLog.Info("Setting up core scheme")
if err := v1.AddToScheme(mgr.GetScheme()); err != nil {
setupLog.Error(err, "unable to add Core APIs to scheme")
Expand Down
8 changes: 8 additions & 0 deletions config/overlays/odh/inferenceservice-config-patch.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,14 @@ metadata:
namespace: kserve
data:
explainers: "{}"
oauthProxy: |-
{
"image" : "$(oauth-proxy)",
"memoryRequest": "256Mi",
"memoryLimit": "256Mi",
"cpuRequest": "100m",
"cpuLimit": "100m",
}
storageInitializer: |-
{
"image" : "$(kserve-storage-initializer)",
Expand Down
7 changes: 7 additions & 0 deletions config/overlays/odh/kustomization.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,13 @@ vars:
apiVersion: v1
kind: ConfigMap
name: kserve-parameters
- fieldref:
fieldpath: data.oauth-proxy
name: oauth-proxy
objref:
apiVersion: v1
kind: ConfigMap
name: kserve-parameters

configurations:
- params.yaml
1 change: 1 addition & 0 deletions config/overlays/odh/params.env
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ kserve-controller=quay.io/opendatahub/kserve-controller:latest
kserve-agent=quay.io/opendatahub/kserve-agent:latest
kserve-router=quay.io/opendatahub/kserve-router:latest
kserve-storage-initializer=quay.io/opendatahub/kserve-storage-initializer:latest
oauth-proxy=registry.redhat.io/openshift4/ose-oauth-proxy@sha256:4bef31eb993feb6f1096b51b4876c65a6fb1f4401fee97fa4f4542b6b7c9bc46
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This image is extremely old, and with F health: https://catalog.redhat.com/software/containers/openshift4/ose-oauth-proxy/5cdb2133bed8bd5717d5ae64?image=6306f12280cc9b3291272668&architecture=amd64

Please, use a newer one.

8 changes: 8 additions & 0 deletions config/rbac/role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,14 @@ rules:
- patch
- update
- watch
- apiGroups:
- route.openshift.io
resources:
- routes
verbs:
- get
- list
- watch
- apiGroups:
- serving.knative.dev
resources:
Expand Down
3 changes: 2 additions & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ require (
github.com/kelseyhightower/envconfig v1.4.0
github.com/onsi/ginkgo/v2 v2.13.0
github.com/onsi/gomega v1.30.0
github.com/openshift/api v3.9.0+incompatible
github.com/pkg/errors v0.9.1
github.com/spf13/cobra v1.8.0
github.com/spf13/pflag v1.0.5
Expand All @@ -34,6 +35,7 @@ require (
k8s.io/code-generator v0.28.4
k8s.io/klog v1.0.0
k8s.io/kube-openapi v0.0.0-20231113174909-778a5567bc1e
k8s.io/utils v0.0.0-20230726121419-3b25d923346b
knative.dev/networking v0.0.0-20231115015815-3af9769712cd
knative.dev/pkg v0.0.0-20231115001034-97c7258e3a98
knative.dev/serving v0.39.3
Expand Down Expand Up @@ -128,7 +130,6 @@ require (
k8s.io/component-base v0.28.4 // indirect
k8s.io/gengo v0.0.0-20230829151522-9cce18d56c01 // indirect
k8s.io/klog/v2 v2.110.1 // indirect
k8s.io/utils v0.0.0-20230726121419-3b25d923346b // indirect
sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect
sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect
)
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,8 @@ github.com/onsi/gomega v1.30.0 h1:hvMK7xYz4D3HapigLTeGdId/NcfQx1VHMJc60ew99+8=
github.com/onsi/gomega v1.30.0/go.mod h1:9sxs+SwGrKI0+PWe4Fxa9tFQQBG5xSsSbMXOI8PPpoQ=
github.com/opencontainers/go-digest v1.0.0 h1:apOUWs51W5PlhuyGyz9FCeeBIOUDA/6nW8Oi/yOhh5U=
github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM=
github.com/openshift/api v3.9.0+incompatible h1:fJ/KsefYuZAjmrr3+5U9yZIZbTOpVkDDLDLFresAeYs=
github.com/openshift/api v3.9.0+incompatible/go.mod h1:dh9o4Fs58gpFXGSYfnVxGR9PnV53I8TW84pQaJDdGiY=
github.com/perimeterx/marshmallow v1.1.5 h1:a2LALqQ1BlHM8PZblsDdidgv1mWi1DgC2UmX50IvK2s=
github.com/perimeterx/marshmallow v1.1.5/go.mod h1:dsXbUu8CRzfYP5a87xpp0xq9S3u0Vchtcl8we9tYaXw=
github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
Expand Down
14 changes: 0 additions & 14 deletions pkg/apis/serving/v1beta1/openapi_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions pkg/constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ var (
KnativeServingAPIGroupName = KnativeServingAPIGroupNamePrefix + ".dev"
KServeNamespace = getEnvOrDefault("POD_NAMESPACE", "kserve")
KServeDefaultVersion = "v0.5.0"
KserveServiceAccountName = "kserve-sa"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take into consideration my comment from the other PR: opendatahub-io/odh-model-controller#274 (comment)

)

// InferenceService Constants
Expand Down Expand Up @@ -122,6 +123,8 @@ const (
NetworkVisibility = "networking.kserve.io/visibility"
ClusterLocalVisibility = "cluster-local"
ClusterLocalDomain = "svc.cluster.local"
ODHKserveRawAuth = "security.opendatahub.io/enable-auth"
ODHRouteEnabled = "enable-route"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe change to exposed which term more broadly used in OpenShift ecosystem? e.g. oc expose command of the CLI.

Suggested change
ODHRouteEnabled = "enable-route"
ODHRouteEnabled = "exposed"

The label would then read as networking.kserve.io/visibility: exposed

)

// StorageSpec Constants
Expand Down Expand Up @@ -414,6 +417,16 @@ const (
SupportedModelMLFlow = "mlflow"
)

// opendatahub rawDeployment Auth
const (
OauthProxyImage = "registry.redhat.io/openshift4/ose-oauth-proxy@sha256:4bef31eb993feb6f1096b51b4876c65a6fb1f4401fee97fa4f4542b6b7c9bc46"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to expose this config? e.g. any updates in the image would require code change in KServe

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't update this oauth proxy image very frequently at all, but do you think I should add it to the configmap and try to consume it from there?

Copy link

@israel-hdez israel-hdez Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better exposing the config, as it allow users quickly moving to another image in case this one becomes vulnerable.

Also, all the other Oauth related constants should be exposed. These should be defaults and users should be capable of customizing, since we don't know how much load there will be which would determine the amount of memory and CPU to be assigned.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, kept these constants as fallback values but PR includes changes to the kserve manifests where the oauth proxy params are defined

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my other comment: https://github.com/opendatahub-io/kserve/pull/419/files#r1805239200

OauthProxyPort = 8443
OauthProxyResourceMemoryLimit = "256Mi"
OauthProxyResourceCPULimit = "100m"
OauthProxyResourceMemoryRequest = "256Mi"
OauthProxyResourceCPURequest = "100m"
)

type ProtocolVersion int

const (
Expand Down
1 change: 1 addition & 0 deletions pkg/controller/v1beta1/inferenceservice/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ import (
// +kubebuilder:rbac:groups=core,resources=namespaces,verbs=get;list;watch
// +kubebuilder:rbac:groups=core,resources=events,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch
// +kubebuilder:rbac:groups=route.openshift.io,resources=routes,verbs=get;list;watch

// InferenceServiceState describes the Readiness of the InferenceService
type InferenceServiceState string
Expand Down
Loading
Loading