Create common CRD validate and mutating webhook for all operator

[wackxu]

If the spec of tfjob is invalid, we should reject the request when creating and also set default when create tfjob. we can use the k8s webhook to do this. And also we can replace the unstructured informer code.

[issue-label-bot]

Issue-Label Bot is automatically applying the label improvement/enhancement to this issue, with a confidence of 0.86. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

[wackxu]

cc @gaocegege @johnugeorge @richardsliu

[gaocegege]

It is a good idea. I think we need it.

[gaocegege]

Personally, I think we should use validation webhook instead of the unstructured informer in the code.

[wackxu]

@gaocegege Yes. agree with that. I think we can use one webhook server for all the operator to do validate or set default value, WDYT?

[gaocegege]

Personally, SGTM.

WDYT @johnugeorge @richardsliu

[johnugeorge]

LGTM. However, we should not take this up in this release as we are late.

[gaocegege]

Yeah, I think so

[wackxu]

@gaocegege @johnugeorge @richardsliu So we need create a new repository for the webhook, then I can start the webhook work.

[gaocegege]

We need a detailed proposal to persuade the community to open a new repo.

[johnugeorge]

Just wondering why do we need a separate repo for webhook?

[gaocegege]

I think we need a proposal first, then we can discuss it and decide if we need a repo for it.

@wackxu Are you interested in the proposal? I can co-work with you if you are busy.

[wackxu]

Just wondering why do we need a separate repo for webhook?

We may want to add validate webhook for all operators and we have about six operators, so I think use one operator for all operators and we can use some switch to let user to choose which webhook to use.

[wackxu]

I think we need a proposal first, then we can discuss it and decide if we need a repo for it.

@wackxu Are you interested in the proposal? I can co-work with you if you are busy.

@gaocegege I am busy with some internal work recently. You can start write the proposal and I can help review and implement the code.

[gaocegege]

@wackxu Maybe we could have a meeting later to make sure that we are on the same page.

[wackxu]

@gaocegege yes, we can talk more details in the slack.

[johnugeorge]

@wackxu Are you planning to take this up?

[johnugeorge]

Related: #1026

[wackxu]

@johnugeorge Sorry for the delay, Yes I will but we need a new repo for it first.

[richardsliu]

@johnugeorge Should we take this on for 0.7?

[johnugeorge]

@wackxu Why do you need a separate repo for it?

[wackxu]

@johnugeorge I think we should add webhook for all operators. If we do not put them in a single repo, then we should add webhook for each operator separately. It is not convenient for user that use multiple operator to deploy and maintain.

[wackxu]

We can use switch to control which webhook is turned on. If user only use one operator such as tf-operator, then he can only turn on the tf webhook switch. It is convenient for user to manage these operator webhook.

[gaocegege]

@wackxu @johnugeorge Is there any update? I think it is an important feature.

[johnugeorge]

@gaocegege This is not yet taken up

[sarahmaddox]

/area gsoc

[gaocegege]

/cc @carmark @johnugeorge @ChanYiLin

I think we can reuse this repo to keep the webhook if needed. https://github.com/kubeflow/crd-validation

[jlewi]

Do we still need to create our validation webhook? It looks like Kubernetes custom resources now allow schemas to be specified using OpenAPI in which case valdiation will happen automatically.
https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#specifying-a-structural-schema

Related: kubeflow/katib#1170

[lalithvaka]

Do we still need to create our validation webhook? It looks like Kubernetes custom resources now allow schemas to be specified using OpenAPI in which case valdiation will happen automatically.
https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#specifying-a-structural-schema

Related: kubeflow/katib#1170

This seems like a good idea. Only thing to watch for is this feature is supported only K8S Cluster version 1.16 and above!

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[issue-label-bot]

Issue-Label Bot is automatically applying the labels:

Label	Probability
feature	0.92

Please mark this comment with 👍 or 👎 to give our bot feedback!
Links: app homepage, dashboard and code for this bot.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.