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.

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

Prevent InferenceGraphs to refer to other namespaces #357

Open
wants to merge 1 commit into
base: incubating
Choose a base branch
from

Conversation

israel-hdez
Copy link
Contributor

Description

In ODH we prefer isolation between projects/namespaces. For InferenceGraph CRD, KServe doesn't do any validation on the used hostname when a node/step is using serviceUrl. This can break isolation between projects, because it is allowed to specify a hostname from another namespace.

This adds a validating webhook to prevent the user from using an in-cluster hostname that would be referring to a different namespace from the created/updated InferenceGraph.

Fixes: https://issues.redhat.com/browse/RHOAIENG-17828

How Has This Been Tested?

TODO

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

Copy link
Contributor

openshift-ci bot commented Jan 20, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Contributor

openshift-ci bot commented Jan 20, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: israel-hdez

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

In ODH we prefer isolation between projects/namespaces. For InferenceGraph CRD, KServe doesn't do any validation on the used hostname when a node/step is using `serviceUrl`. This can break isolation between projects, because it is allowed to specify a hostname from another namespace.

This adds a validating webhook to prevent the user from using an in-cluster hostname that would be referring to a different namespace from the created/updated InferenceGraph.

Signed-off-by: Edgar Hernández <[email protected]>
@israel-hdez israel-hdez force-pushed the j17828-ig-validate-serviceUrl branch from 8f2421c to 8fe55be Compare January 21, 2025 20:39
@israel-hdez israel-hdez marked this pull request as ready for review January 21, 2025 20:40
@openshift-ci openshift-ci bot requested review from Jooho and terrytangyuan January 21, 2025 20:40
@israel-hdez israel-hdez requested review from spolti and hdefazio and removed request for Jooho and terrytangyuan January 21, 2025 20:41
// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type InferenceGraph.
func (v *InferenceGraphCustomValidator) ValidateCreate(_ context.Context, obj runtime.Object) (admission.Warnings, error) {
inferencegraph, ok := obj.(*servingv1alpha1.InferenceGraph)
if !ok {
Copy link
Member

Choose a reason for hiding this comment

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

what are the odds of it return a false positive?
is there another InferenceGraph type that might conflict with the KServe one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that this is defensive coding (this is from scaffolding).

The ValidatingWebhookConfiguration is external to the controller. So, users (or devs? 🙂) may misconfigure it, and you may receive here some other Kind (Pods, Deployments, or any other resource). I think the type assertion is for those (rare) cases.

About a conflicting type, I'm not really sure if it is possible. Maybe if it is coming from a different API Group? IDK.... I'm not even sure how controller-runtime will be handling that case.

@@ -30,10 +30,10 @@ import (

istiov1beta1 "istio.io/client-go/pkg/apis/security/v1beta1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I'm following why this file was changed?

Copy link
Member

Choose a reason for hiding this comment

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

goimport pattern, mostly likely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is just my preference of having on the last group of imports only only the ones from "this" package. The k8s.io/apimachinery/pkg/apis/meta/v1 is not a package from this repo, so I moved up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants