Add internal-cert-controller disable flag #2426
Conversation
Signed-off-by: Garvit-77 <[email protected]>
| controllerv2 "github.com/kubeflow/training-operator/pkg/controller.v2" | ||
| runtime "github.com/kubeflow/training-operator/pkg/runtime.v2" | ||
| runtimecore "github.com/kubeflow/training-operator/pkg/runtime.v2/core" | ||
| webhookv2 "github.com/kubeflow/training-operator/pkg/webhook.v2" | ||
| ) | ||
|
|
||
| const ( | ||
| webhookConfigurationName = "validator.training-operator-v2.kubeflow.org" |
There was a problem hiding this comment.
Just a small confusion should we use webhookConfigurationName instead of namespace(as of V1) ?
| WebhookConfigurationName: webhookConfigurationName, | ||
| }, certsReady); err != nil { | ||
| setupLog.Error(err, "unable to set up cert rotation") | ||
| if err := createCertificate(mgr.GetClient(), namespace, webhookSecretName, webhookServiceName); err != nil { |
There was a problem hiding this comment.
Do we want this to be configurable, so cert-manager is an option, not a requirement?
My understanding is we still want it possible to install Kubeflow trainer without cert-manager.
There was a problem hiding this comment.
The cert-manager is optional. We should not create resources automatically in manager.
We just add Certifications by manifests, not by automatic mechanism like current impl.
There was a problem hiding this comment.
@tenzen-y can you please brief if any changes are required in current PR ?
There was a problem hiding this comment.
- Prepare the flag whether or not the controller manager launch internal cert controller.
- Add CertManager installation manifests like https://github.com/kubeflow/katib/tree/master/manifests/v1beta1/installs/katib-cert-manager
Signed-off-by: Garvit-77 <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@tenzen-y |
| WebhookConfigurationName: webhookConfigurationName, | ||
| }, certsReady); err != nil { | ||
| setupLog.Error(err, "unable to set up cert rotation") | ||
| if err := createCertificate(mgr.GetClient(), namespace, webhookSecretName, webhookServiceName); err != nil { |
There was a problem hiding this comment.
As I mentioned the previous comment, we should not remove and replace the existing CertManagement mechanism. We just provide the command flag whether or not manager uses internal-certmanagement mechanism.
- Add a flag
--enable-internal-cert (default true) - If the flag is false, manager does not perform
cert.ManageCerts. - Add installation manifests with CertManager
There was a problem hiding this comment.
@tenzen-y yes i will revert the code back to cert generation by controller
| // Cert generation Using cert-manager | ||
| func createCertificate(client client.Client, namespace, secretName, serviceName string) error { | ||
| cert := &cmapi.Certificate{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: secretName, | ||
| Namespace: namespace, | ||
| }, | ||
| Spec: cmapi.CertificateSpec{ | ||
| SecretName: secretName, | ||
| IssuerRef: cmmeta.ObjectReference{ | ||
| Name: "letsencrypt-prod", | ||
| Kind: "ClusterIssuer", | ||
| }, | ||
| CommonName: serviceName, | ||
| DNSNames: []string{serviceName}, | ||
| Usages: []cmapi.KeyUsage{ | ||
| cmapi.UsageServerAuth, | ||
| cmapi.UsageClientAuth, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| if err := client.Create(context.Background(), cert); err != nil { | ||
| return fmt.Errorf("failed to create certificate: %v", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
| // Cert generation Using cert-manager | |
| func createCertificate(client client.Client, namespace, secretName, serviceName string) error { | |
| cert := &cmapi.Certificate{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: secretName, | |
| Namespace: namespace, | |
| }, | |
| Spec: cmapi.CertificateSpec{ | |
| SecretName: secretName, | |
| IssuerRef: cmmeta.ObjectReference{ | |
| Name: "letsencrypt-prod", | |
| Kind: "ClusterIssuer", | |
| }, | |
| CommonName: serviceName, | |
| DNSNames: []string{serviceName}, | |
| Usages: []cmapi.KeyUsage{ | |
| cmapi.UsageServerAuth, | |
| cmapi.UsageClientAuth, | |
| }, | |
| }, | |
| } | |
| if err := client.Create(context.Background(), cert); err != nil { | |
| return fmt.Errorf("failed to create certificate: %v", err) | |
| } | |
| return nil | |
| } |
| WebhookConfigurationName: webhookConfigurationName, | ||
| }, certsReady); err != nil { | ||
| setupLog.Error(err, "unable to set up cert rotation") | ||
| if err := createCertificate(mgr.GetClient(), namespace, webhookSecretName, webhookServiceName); err != nil { |
There was a problem hiding this comment.
| if err := createCertificate(mgr.GetClient(), namespace, webhookSecretName, webhookServiceName); err != nil { |
There was a problem hiding this comment.
I am wondering do we really need to have cert-manager installation ?
Can we re-use OPA cert-manager rotator to give user flexibility to control certificate or cert-manager will give us additional features ?
cc @kubeflow/release-team @kubeflow/wg-manifests-leads
I do not have a strong request for CertManager support. One thing is productionizing trainer since they sometimes want to use the certified certifications (OPA internal-certs generates self-signed certs). As an alternative and minimized solution, we could consider supporting only disabling the OPA internal-cert manager and then giving them to specify arbitrary certifications. |
Does OPA support only self-signed certs @tenzen-y ?
Yeah, this is what I was thinking as well. Like do we really need to explain cluster admins on how to configure cert manager to generate certs for Kubeflow Trainer webhook? |
|
FYI, as I can see even with Cert Manager right now we use self-signed certs by default: |
Yes, that is OPA internal-certs objective.
Documentation might be better. @astefanutti Do you see any use cases where customers want to use certified certifications for admission webhook controllers? |
No, I haven't personally seen any customer requests / requirements to have full-fledged certificate management / PKI for admission webhooks. cert-manager is more driven by the need to integrate with trusted certificate authority like let's encrypt for external ingress / gateway, not for in-cluster communication between control plane components. Now I can understand that if cert-manager is already deployed, it can be used to manage internal certificates as well. |
|
cc @kubeflow/release-team @kubeflow/kubeflow-steering-committee @juliusvonkohout @franciscojavierarceo |
Thank you for getting back feedback! In that case, it would be better to add only flag to disable internal cert controller and then enhance our documentations how to use arbitrary certificates for the admission webhook controllers. |
|
Let me summarize for @Garvit-77: we decided not to support CertManger. So, if you are ok, could you convert this PR to just add a flag to disable and enable the internal cert controller? |
There is also KFP and maybe others. But yes, we can remove it as soon as no one is using it anymore. |
|
@tenzen-y Surely , i did understood the conversation except about the OPA |
If KFP uses it in the same way that other Kubeflow projects is using, we can easily remove this dependency. |
Signed certificates for webhooks are not a bad thing. When to Use cert-manager:
|
|
/retitle add internal-cert-controller disable flag |
google-oss-prow
bot
changed the title
|
/retitle Add internal-cert-controller disable flag |
google-oss-prow
bot
changed the title
|
@juliusvonkohout @andreyvelich Should we prepare the dedicated issue to discuss CertManager entirely KF? |
|
I agree with @tenzen-y, I will create dedicated issue in |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Fixes #2049