Create webhook for NIM enablement #288

xieshenzh · 2024-11-05T19:20:49Z

Create the validation webhook for NIM Account.

Description

Created the webhook
Updated the manifests to include the webhook
Check the DSC flag to decide whether to enable the webhook

How Has This Been Tested?

Deploy the controller in Openshift
Create an Account CR.
2.1 Check if the Account is created when the api key secret is not created
2.2 Check if the Account is created when there is already an Account CR in the namespace
2.3 Check if the Account is created when the api key secret is created and there is no other Account in the namespace
Update an Account CR
3.1 Check if the Account is updated when updating the api key secret and the new api key secret is not created
3.2 Check if the Account is updated when updating the api key secret and the new api key secret is created

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-11-05T19:21:00Z

Hi @xieshenzh. Thanks for your PR.

I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

main.go

Jooho · 2024-11-06T21:12:31Z

Could you please write tests for this webhook?

xieshenzh · 2024-11-06T21:14:59Z

Could you please write tests for this webhook?

I did: https://github.com/opendatahub-io/odh-model-controller/pull/288/files#diff-a276b8cfa9be48d35d7eb0c0ed9d8a2d4dea589741656fb6bac43c03dd2652a2

Jooho · 2024-11-06T21:19:22Z

oh I missed it. thanks for the tests. I will review this.

Jooho · 2024-11-06T21:35:12Z

controllers/webhook/nim_account_webhook.go

+			return field.Invalid(field.NewPath("spec").Child("apiKeySecret").Child("name"), newAccount.Spec.APIKeySecret.Name, msg)
+		}
+		name := newAccount.Spec.APIKeySecret.Name
+		namespace := newAccount.Spec.APIKeySecret.Namespace


namespace can be different from where it is created?

Technically, the namespace of the Secret can be different from the namespace of the Account.
If there is a requirement to disallow this scenario, this webhook should reject Account creation or update in this case.
But I have not received such requirement so far.

odh-model-controller can access to all namespaces and also so can get secret but it is for internal usage for odh-model-controller. But if it is available to access all secrets in the cluster by this cr, I think it is not a good idea for security reasons. @etirelli @terrytangyuan @israel-hdez wdyt?

@Jooho The webhook is updated. It won't check the Secret anymore.
If you still have security concerns on the usage of the Secret.
Please add your comments and requests in PR #289 instead.
Thanks.

controllers/webhook/nim_account_webhook.go

controllers/utils/utils.go

controllers/webhook/nim_account_webhook.go

zdtsw · 2024-11-07T08:19:26Z

controllers/suite_test.go

@@ -116,7 +116,8 @@ var _ = BeforeSuite(func() {
 	By("Bootstrapping test environment")
 	envTest = &envtest.Environment{
 		CRDInstallOptions: envtest.CRDInstallOptions{
-			Paths:              []string{filepath.Join("..", "config", "crd", "external")},
+			Paths: []string{filepath.Join("..", "config", "crd", "external"),
+				filepath.Join("..", "config", "crd", "bases")},


tbh, i would think this new CRD to be shipped out by kserve as an overlay not by odh-model-controller.
say, kserve is disabled, modelmesh is enabled, CRD would still be installed in the cluster, should it?

or not include "crd/bases" from "base/kustomization.yaml".
instead, in Operator to create it on kserve demand.

@etirelli @israel-hdez @lburgazzoli @TomerFi @Jooho @mpaulgreen
Any thoughts on the suggestions?

Here are some legacy comments that can drive the decision. Please feel free to override me. https://issues.redhat.com/projects/NVPE/issues/NVPE-4?filter=allissues and from a slack thread - https://redhat-internal.slack.com/archives/C07CZDA6DE1/p1730391034630149?thread_ts=1730389624.036899&cid=C07CZDA6DE1

@zdtsw I believe this was discussed before, and the overall agreement was that odh-model-controller is the best choice at the moment. I think you are correct on the model-mesh case, but at the same time, I believe the idea is to eventually remove model-mesh, so it would be a non-issue. But I defer to the model serving team if they think otherwise.

i do not have objection to place CRD in such a way

No strong preference on this. I think it's fine keep it as is.

I'm OK with having the CRD in this repo.

main.go

terrytangyuan · 2024-11-08T21:08:20Z

controllers/suite_test.go

@@ -116,7 +116,8 @@ var _ = BeforeSuite(func() {
 	By("Bootstrapping test environment")
 	envTest = &envtest.Environment{
 		CRDInstallOptions: envtest.CRDInstallOptions{
-			Paths:              []string{filepath.Join("..", "config", "crd", "external")},
+			Paths: []string{filepath.Join("..", "config", "crd", "external"),
+				filepath.Join("..", "config", "crd", "bases")},


No strong preference on this. I think it's fine keep it as is.

controllers/webhook/nim_account_webhook.go

main.go

Signed-off-by: Xieshen Zhang <[email protected]>

xieshenzh · 2024-11-12T18:38:52Z

@Jooho I don't think there is any outstanding concerns on this PR.
Could you please add /ok-to-test?

israel-hdez · 2024-11-13T05:58:07Z

/ok-to-test

israel-hdez

/lgtm

openshift-ci · 2024-11-14T17:02:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: israel-hdez, TomerFi, xieshenzh

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~~ [israel-hdez]

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

openshift-ci bot added do-not-merge/work-in-progress needs-ok-to-test labels Nov 5, 2024

xieshenzh force-pushed the nim-account-webhook branch 3 times, most recently from 09288d1 to 6a12973 Compare November 6, 2024 20:28

xieshenzh marked this pull request as ready for review November 6, 2024 20:30

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

openshift-ci bot requested review from spolti and terrytangyuan November 6, 2024 20:30

terrytangyuan reviewed Nov 6, 2024

View reviewed changes

main.go Outdated Show resolved Hide resolved

Jooho reviewed Nov 7, 2024

View reviewed changes

zdtsw reviewed Nov 7, 2024

View reviewed changes

xieshenzh commented Nov 7, 2024

View reviewed changes

main.go Outdated Show resolved Hide resolved

xieshenzh force-pushed the nim-account-webhook branch 2 times, most recently from 90473a2 to 77f564c Compare November 8, 2024 04:29

terrytangyuan reviewed Nov 8, 2024

View reviewed changes

TomerFi suggested changes Nov 10, 2024

View reviewed changes

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

TomerFi mentioned this pull request Nov 10, 2024

Task/NVPE-10: Create NIM Account controller #289

Merged

3 tasks

TomerFi reviewed Nov 10, 2024

View reviewed changes

main.go Outdated Show resolved Hide resolved

xieshenzh marked this pull request as draft November 11, 2024 17:24

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

Create webhook for NIM enablement

8f8c62c

Signed-off-by: Xieshen Zhang <[email protected]>

xieshenzh force-pushed the nim-account-webhook branch from 77f564c to 8f8c62c Compare November 12, 2024 18:11

xieshenzh marked this pull request as ready for review November 12, 2024 18:14

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

openshift-ci bot requested a review from mwaykole November 12, 2024 18:15

openshift-ci bot requested a review from rnetser November 12, 2024 18:15

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Nov 13, 2024

TomerFi approved these changes Nov 13, 2024

View reviewed changes

israel-hdez approved these changes Nov 14, 2024

View reviewed changes

openshift-ci bot assigned israel-hdez Nov 14, 2024

openshift-ci bot added the lgtm label Nov 14, 2024

openshift-ci bot added the approved label Nov 14, 2024

openshift-merge-bot bot merged commit ee5f3df into opendatahub-io:main Nov 14, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create webhook for NIM enablement #288

Create webhook for NIM enablement #288

xieshenzh commented Nov 5, 2024 •

edited

Loading

openshift-ci bot commented Nov 5, 2024

Jooho commented Nov 6, 2024

xieshenzh commented Nov 6, 2024

Jooho commented Nov 6, 2024

Jooho Nov 6, 2024

xieshenzh Nov 7, 2024

Jooho Nov 8, 2024

xieshenzh Nov 12, 2024

zdtsw Nov 7, 2024

xieshenzh Nov 7, 2024

mpaulgreen Nov 7, 2024

etirelli Nov 8, 2024

zdtsw Nov 8, 2024

terrytangyuan Nov 8, 2024

israel-hdez Nov 13, 2024

terrytangyuan Nov 8, 2024

xieshenzh commented Nov 12, 2024

israel-hdez commented Nov 13, 2024

israel-hdez left a comment

openshift-ci bot commented Nov 14, 2024

Create webhook for NIM enablement #288

Create webhook for NIM enablement #288

Conversation

xieshenzh commented Nov 5, 2024 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

openshift-ci bot commented Nov 5, 2024

Jooho commented Nov 6, 2024

xieshenzh commented Nov 6, 2024

Jooho commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xieshenzh commented Nov 12, 2024

israel-hdez commented Nov 13, 2024

israel-hdez left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 14, 2024

xieshenzh commented Nov 5, 2024 •

edited

Loading