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

Disable dynamic webhook creation in the Katib controller #1405

Closed
andreyvelich opened this issue Dec 1, 2020 · 12 comments · Fixed by #1450
Closed

Disable dynamic webhook creation in the Katib controller #1405

andreyvelich opened this issue Dec 1, 2020 · 12 comments · Fixed by #1450

Comments

@andreyvelich
Copy link
Member

andreyvelich commented Dec 1, 2020

/kind feature

Currently, Katib controller automatically creates admission webhooks.
As a result, we have several issues:

We should disable the webhook creation completely and prepare appropriate manifests.
As well, we should implement the process for the certificate creation without cert-manager, if it is possible.

Let's think about the design here.

/cc @gaocegege @johnugeorge @knkski @jlewi
/priority p1

@gaocegege
Copy link
Member

SGTM. One question here: How to setup a dev environment easily without this feature?

@andreyvelich
Copy link
Member Author

SGTM. One question here: How to setup a dev environment easily without this feature?

Do you mean without cert-manager ?

@yanniszark
Copy link
Contributor

@andreyvelich is there any remaining work to figure out before moving certificate creation out of the Katib controller? Regarding:

As well, we should implement the process for the certificate creation without cert-manager, if it is possible.

I don't think Katib should care about this. Katib should IMO provide a way to do this with cert-manager (as this is pretty standard and kubebuilder does it as well) and allow the consumer of the manifests to specify another way if required. This could be done by packaging the cert-manager functionality in a Kustomize component, which allows creating re-usable and composable kustomizations.

So besides the manifests part, what is the work that needs to be done in order to disable webhook creation in the Katib code?

@andreyvelich
Copy link
Member Author

andreyvelich commented Jan 13, 2021

@yanniszark In that case all Katib users should install cert-manager on their clusters. Thus, we should maintain some version of this CRD and be consistent with other Kubeflow apps, e.g. KFServing.

So besides the manifests part, what is the work that needs to be done in order to disable webhook creation in the Katib code?

Once we add webhooks to the manifests we should refactor controller to not create webhook during controller start: https://github.com/kubeflow/katib/blob/master/pkg/webhook/v1beta1/webhook.go.

@andreyvelich
Copy link
Member Author

Any thoughts @gaocegege @johnugeorge ?

@gaocegege
Copy link
Member

I think we should have such a configuration to allow users to remove webhook logic.

@andreyvelich
Copy link
Member Author

andreyvelich commented Jan 14, 2021

@gaocegege Do you mean we should leave dynamic webhook creation in the controller for some cases ?
I think after we add manifests for the Webhooks, we don't need to leave functionality for webhook creation in the controller, so we have only problem with the certificate.

What do you think about cert-manager ?

@gaocegege
Copy link
Member

Oh SGTM.

we don't need to leave functionality for webhook creation in the controller, so we have only problem with the certificate.

@kuikuikuizzZ
Copy link

@gaocegege @andreyvelich Is possible to use a job to take over the creation/remove of the certificate like spark-operator did ? ref: https://github.com/GoogleCloudPlatform/spark-on-k8s-operator/blob/master/charts/spark-operator-chart/templates/webhook-init-job.yaml#L29 ?

@knkski
Copy link
Contributor

knkski commented Jan 27, 2021

I opened #1424, which addresses the issue of allowing someone to disable the dynamic webhook creation. It doesn't directly add a replacement method, as that's handled separately in the operators here.

@andreyvelich
Copy link
Member Author

@gaocegege @andreyvelich Is possible to use a job to take over the creation/remove of the certificate like spark-operator did ? ref: https://github.com/GoogleCloudPlatform/spark-on-k8s-operator/blob/master/charts/spark-operator-chart/templates/webhook-init-job.yaml#L29 ?

Do you know, when they start this Job, once they install Spark Operator on the K8s cluster?

@kuikuikuizzZ
Copy link

kuikuikuizzZ commented Jan 30, 2021

@andreyvelich

@gaocegege @andreyvelich Is possible to use a job to take over the creation/remove of the certificate like spark-operator did ? ref: https://github.com/GoogleCloudPlatform/spark-on-k8s-operator/blob/master/charts/spark-operator-chart/templates/webhook-init-job.yaml#L29 ?

Do you know, when they start this Job, once they install Spark Operator on the K8s cluster?

Yes, this job will setup when "helm install", and it has another job to cleanup the configmap when "helm uninstall" and "helm upgrade“.
https://github.com/GoogleCloudPlatform/spark-on-k8s-operator/blob/master/charts/spark-operator-chart/templates/webhook-cleanup-job.yaml#L7

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

Successfully merging a pull request may close this issue.

6 participants