Prevent creation of KSVC if its namespace is not in the Mesh #181

israel-hdez · 2024-03-13T00:27:54Z

Description

When a KServe InferenceService is created, odh-model-controller enrolls the ISVC namespace to the mesh, if needed. It was found that sometimes the namespace enrollment process may take a little time to be processed.

Since KServe and odh-model-controllers are (to certain extent) independent of each other, sometimes KServe controller and Knative serving controller are faster and the pod from the KSVC may be created faster than the mesh enrollment process. This would lead to the KSVC pod not having an Istio sidecar.

Since the Istio authorization rules are evaluated on the sidecar of target service, the missing sidecar on the model/ksvc pod would mean that the traffic would bypass the authorization rules and this impacts ODH KServe authorization (ref: https://istio.io/latest/docs/ops/best-practices/security/#server-first-tcp-protocols-are-not-supported).

In an effort to prevent that situation, this is adding a new validating webhook that would block creation of the Knative Service until the namespace is acknowledged to be a member of the service mesh.

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

How Has This Been Tested?

Simple sanity check: ensuring a model can be deployed.

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

openshift-ci · 2024-03-13T00:27:59Z

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

controllers/webhook/ksvc_validator.go

bartoszmajsak

With rejecting creation you kind of follow eventual consistency, so I think that is ok per se, especially considering the fact that those two controllers are independent of each other.

I have one thing for you to validate, and a few nits. Other than that the code looks good. Big kudos for spending time on writing tests!

Important

Keep in mind that at some point we will switch from creating SMM (or directly manipulating SMMR which we shouldn't be doing in the first place) to label selectors and at this point we will need to make sure we can handle this case as well. I do not know if SMMR is internally updated if that's the case, but I will figure it out.

Makefile

config/webhook/kustomization.yaml

controllers/webhook/ksvc_validator.go

controllers/utils/init.go

controllers/webhook/ksvc_validator.go

controllers/reconcilers/kserve_istio_smmr_reconciler.go

main.go

bartoszmajsak · 2024-03-18T11:32:30Z

main.go

+			os.Exit(1)
+		}
+	} else {
+		setupLog.Info("Skipping setup of Knative Service validating Webhook.")


If you really have to log such info perhaps it's good to add "why" it is skipped.

I agree. I'll add a reason.

Fixed... That said, I'm no longer sure how much we can skip this.
The conditional is in prevention of RawDeployment setup, where Knative is missing. However, AFAIK, there is no way to introduce equivalent conditionals around the ValidatingWebhookConfiguration resource (i.e. the operator manifests).

So, if the user has Knative for other uses, the ValidatingWebhookConfiguration would take effect and would break the cluster, given the controller would have the logic turned off.

I'll tag @VaishnaviHire and @zdtsw for their advice.

controllers/webhook/ksvc_validator.go

bartoszmajsak · 2024-03-18T11:36:27Z

controllers/webhook/ksvc_validator.go

+	log.Info("Rejecting Knative service because its namespace is not a member of the service mesh")
+	return fmt.Errorf("rejecting creation of Knative service %s on namespace %s because the namespace is not a configured member of the mesh", ksvc.Name, ksvc.Namespace)


Not sure you really need a log here - the error you are returning seems to be sufficient and I assume it will be logged somehow?

In addition the info message is misleading. It can be pending or failing if I'm reading this right.

The error is not written to the controller logs. The error is returned to the client that tried to create the resource. So, the client chooses what to do with the message. Because of this, I chose to add the log message to make it clear what is happening in odh-model-controller side.

About pending or failing, I'm not sure if it is worth to go that deep here :-) Stating it is not configured is enough for me to make the user go and diagnose it, in case it stays on one of those states for longer than expected.

Ok, I thought this is handled by operator-sdk, but if that is not the case logging is helpful here.

About pending or failing, I'm not sure if it is worth to go that deep here :-) Stating it is not configured is enough for me to make the user go and diagnose it, in case it stays on one of those states for longer than expected.

I was coming from the angle that "not configured" is a bit ambiguous here. Does it mean that someone forgot to put the proper label on the namespace so it is not really seen by the mesh OR it cannot be configured by the mesh because... other errors? But I agree it might be going to deep.

Makefile

When a KServe InferenceService is created, odh-model-controller enrolls the ISVC namespace to the mesh, if needed. It was found that sometimes the namespace enrollment process may take a little time to be processed. Since KServe and odh-model-controllers are (to certain extent) independent of each other, sometimes KServe controller and Knative serving controller are faster and the pod from the KSVC may be created faster than the mesh enrollment process. This would lead to the KSVC pod not having an Istio sidecar. Since the Istio authorization rules are evaluated on the sidecar of target service, the missing sidecar on the model/ksvc pod would mean that the traffic would bypass the authorization rules and this impacts ODH KServe authorization (ref: https://istio.io/latest/docs/ops/best-practices/security/#server-first-tcp-protocols-are-not-supported). In an effort to prevent that situation, this is adding a new validating webhook that would block creation of the Knative Service until the namespace is acknowledged to be a member of the service mesh. Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com>

israel-hdez · 2024-03-20T16:29:40Z

I just squashed the commits.
@spolti @VedantMahabaleshwarkar @bartoszmajsak Please, give it a final pass.

bartoszmajsak

LGTM

openshift-ci · 2024-03-21T21:05:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bartoszmajsak, 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:

~~OWNERS~~ [VedantMahabaleshwarkar,israel-hdez]

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

This change hanleds "soft opt-in" for authorization capability handled by Authorino. The logic consists of following steps: * during the controller bootstrap it will check if GroupVersion `authorino.kuadrant.io/v1beta2` is present in the cluster and conditionally add it to scheme. * when defining watches (using builder instance for OpenshiftReconciler) it will conditionally set a watch for `AuthConfig` according to the rules defined in opendatahub-io#181 but additionally if Authorino GroupVersion is present in the schema * it guards AuthConfig reconciler from executing based on the "schema rule". This prevents from constant failures due to AuthConfig not being defined for the cluster. NOTE: As it is the case with original logic brought in opendatahub-io#181, it will require restart of the controller pod to set watch on AuthConfig resource when Authorino is installed in the cluster.

This change handles "soft opt-in" for authorization capability handled by Authorino. The logic consists of following steps: * during the controller bootstrap it will check if GroupVersion `authorino.kuadrant.io/v1beta2` is present in the cluster and conditionally add it to scheme. * when defining watches (using builder instance for OpenshiftReconciler) it will conditionally set a watch for `AuthConfig` according to the rules defined in opendatahub-io#181 but additionally if Authorino GroupVersion is present in the schema * it guards AuthConfig reconciler from executing based on the "schema rule". This prevents from constant failures due to AuthConfig not being defined for the cluster. NOTE: As it is the case with original logic brought in opendatahub-io#181, it will require restart of the controller pod to set watch on AuthConfig resource when Authorino is installed in the cluster. Custom image: quay.io/bmajsak/odh-model-controller:latest

This change handles "soft opt-in" for authorization capability handled by Authorino. The logic consists of following steps: * during the controller bootstrap it will check if GroupVersion `authorino.kuadrant.io/v1beta2` is present in the cluster and conditionally add it to scheme. * when defining watches (using builder instance for OpenshiftReconciler) it will conditionally set a watch for `AuthConfig` according to the rules defined in opendatahub-io#181 but additionally if Authorino GroupVersion is present in the schema * it guards AuthConfig reconciler from executing based on the "schema rule". This prevents from constant failures due to AuthConfig not being defined for the cluster. NOTE: As it is the case with original logic brought in opendatahub-io#181, it will require restart of the controller pod to set watch on AuthConfig resource when Authorino is installed in the cluster. Custom image: quay.io/bmajsak/odh-model-controller:latest Signed-off-by: bartoszmajsak <bartosz.majsak@gmail.com>

This change handles "soft opt-in" for authorization capability handled by Authorino. The solution is based on Conditions exposed in DSCInitialization resource, as provided in opendatahub-io/opendatahub-operator#949. When the condition of type `CapabilityServiceMeshAuthorization` has a reason `MissingOperator` it will assume authorino/authorization module is not present and therefore does not handle AuthConfigs - leaving models not secured as it was for the previous versions. This approach allows to softly opt-in for Authorization without blocking the upgrade. Any other reason means that ODH Operator is in not happy state and will eventually reconcile so that Authorino will start working. The logic consists of the following steps: * during the controller bootstrap it will check separately if ServiceMesh is enabled and Authorino is present (latter by inspecting the status.condition) * when defining watches (using builder instance for OpenshiftReconciler) it will conditionally set a watch for `AuthConfig` * it guards AuthConfig reconciler from executing based on the "authorization capability". This prevents constant failures due to `AuthConfig` not being defined for the cluster. > [!NOTE] > As it is the case with original logic brought in opendatahub-io#181, it will require restart of the controller pod to set a watch on AuthConfig resource when Authorino is installed in the cluster. Signed-off-by: bartoszmajsak <bartosz.majsak@gmail.com>

openshift-ci bot added the do-not-merge/work-in-progress label Mar 13, 2024

openshift-ci bot added the approved label Mar 13, 2024

israel-hdez commented Mar 13, 2024

View reviewed changes

controllers/webhook/ksvc_validator.go Show resolved Hide resolved

israel-hdez marked this pull request as ready for review March 15, 2024 00:20

openshift-ci bot removed the do-not-merge/work-in-progress label Mar 15, 2024

openshift-ci bot requested review from terrytangyuan and VedantMahabaleshwarkar March 15, 2024 00:21

bartoszmajsak requested changes Mar 15, 2024

View reviewed changes

Makefile Show resolved Hide resolved

config/webhook/kustomization.yaml Outdated Show resolved Hide resolved

controllers/webhook/ksvc_validator.go Outdated Show resolved Hide resolved

openshift-ci bot assigned bartoszmajsak Mar 15, 2024

spolti reviewed Mar 15, 2024

View reviewed changes

controllers/utils/init.go Show resolved Hide resolved

spolti reviewed Mar 15, 2024

View reviewed changes

controllers/webhook/ksvc_validator.go Show resolved Hide resolved

VedantMahabaleshwarkar approved these changes Mar 15, 2024

View reviewed changes

openshift-ci bot assigned VedantMahabaleshwarkar Mar 15, 2024

openshift-ci bot added the lgtm label Mar 15, 2024

z103cb reviewed Mar 18, 2024

View reviewed changes

controllers/reconcilers/kserve_istio_smmr_reconciler.go Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

bartoszmajsak reviewed Mar 18, 2024

View reviewed changes

openshift-ci bot removed the lgtm label Mar 19, 2024

israel-hdez force-pushed the jira-2542-fix branch from 623cc58 to 6a1dd19 Compare March 20, 2024 16:25

bartoszmajsak approved these changes Mar 21, 2024

View reviewed changes

openshift-ci bot added the lgtm label Mar 21, 2024

openshift-merge-bot bot merged commit 3904c1d into opendatahub-io:main Mar 21, 2024
5 checks passed

israel-hdez deleted the jira-2542-fix branch March 21, 2024 23:04

bartoszmajsak mentioned this pull request Apr 2, 2024

feat(authz): soft opt-in for authz capability #183

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent creation of KSVC if its namespace is not in the Mesh #181

Prevent creation of KSVC if its namespace is not in the Mesh #181

israel-hdez commented Mar 13, 2024 •

edited

Loading

openshift-ci bot commented Mar 13, 2024

bartoszmajsak left a comment

bartoszmajsak Mar 18, 2024

israel-hdez Mar 19, 2024

israel-hdez Mar 19, 2024 •

edited

Loading

bartoszmajsak Mar 18, 2024

israel-hdez Mar 19, 2024 •

edited

Loading

bartoszmajsak Mar 21, 2024

israel-hdez commented Mar 20, 2024

bartoszmajsak left a comment

openshift-ci bot commented Mar 21, 2024

		log.Info("Rejecting Knative service because its namespace is not a member of the service mesh")
		return fmt.Errorf("rejecting creation of Knative service %s on namespace %s because the namespace is not a configured member of the mesh", ksvc.Name, ksvc.Namespace)

Prevent creation of KSVC if its namespace is not in the Mesh #181

Prevent creation of KSVC if its namespace is not in the Mesh #181

Conversation

israel-hdez commented Mar 13, 2024 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

openshift-ci bot commented Mar 13, 2024

bartoszmajsak left a comment

Choose a reason for hiding this comment

bartoszmajsak Mar 18, 2024

Choose a reason for hiding this comment

israel-hdez Mar 19, 2024

Choose a reason for hiding this comment

israel-hdez Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

bartoszmajsak Mar 18, 2024

Choose a reason for hiding this comment

israel-hdez Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

bartoszmajsak Mar 21, 2024

Choose a reason for hiding this comment

israel-hdez commented Mar 20, 2024

bartoszmajsak left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 21, 2024

israel-hdez commented Mar 13, 2024 •

edited

Loading

israel-hdez Mar 19, 2024 •

edited

Loading

israel-hdez Mar 19, 2024 •

edited

Loading