Add internal-cert-controller disable flag #2426
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,18 +17,24 @@ limitations under the License. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package main | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "context" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "crypto/tls" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "errors" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "flag" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "net/http" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "os" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| zaplog "go.uber.org/zap" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "go.uber.org/zap/zapcore" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| apiruntime "k8s.io/apimachinery/pkg/runtime" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| utilruntime "k8s.io/apimachinery/pkg/util/runtime" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clientgoscheme "k8s.io/client-go/kubernetes/scheme" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ctrl "sigs.k8s.io/controller-runtime" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "sigs.k8s.io/controller-runtime/pkg/controller" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "sigs.k8s.io/controller-runtime/pkg/healthz" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "sigs.k8s.io/controller-runtime/pkg/log/zap" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -38,17 +44,12 @@ import ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| schedulerpluginsv1alpha1 "sigs.k8s.io/scheduler-plugins/apis/scheduling/v1alpha1" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| kubeflowv2 "github.com/kubeflow/training-operator/pkg/apis/kubeflow.org/v2alpha1" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| scheme = apiruntime.NewScheme() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setupLog = ctrl.Log.WithName("setup") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -67,10 +68,12 @@ func main() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var probeAddr string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var secureMetrics bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var enableHTTP2 bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var namespace string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var webhookServerPort int | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var webhookServiceName string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var webhookSecretName string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var tlsOpts []func(*tls.Config) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var enableCertController bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flag.StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metrics endpoint binds to. "+ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "Use :8443 for HTTPS or :8080 for HTTP, or leave as 0 to disable the metrics service.") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -82,6 +85,7 @@ func main() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "If set, the metrics endpoint is served securely via HTTPS. Use --metrics-secure=false to use HTTP instead.") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flag.BoolVar(&enableHTTP2, "enable-http2", false, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "If set, HTTP/2 will be enabled for the metrics and webhook servers") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flag.BoolVar(&enableCertController, "enable-cert-controller", false, "Enable the internal certificate controller.") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Cert generation flags | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| flag.IntVar(&webhookServerPort, "webhook-server-port", 9443, "Endpoint port for the webhook server.") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -128,12 +132,8 @@ func main() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| certsReady := make(chan struct{}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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. 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.
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.
There was a problem hiding this comment. @tenzen-y yes i will revert the code back to cert generation by controller There was a problem hiding this comment.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setupLog.Error(err, "Unable to create CertManager certificate") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| os.Exit(1) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -155,6 +155,34 @@ func main() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+158
to
+185
There was a problem hiding this comment.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func setupControllers(mgr ctrl.Manager, runtimes map[string]runtime.Runtime, certsReady <-chan struct{}) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setupLog.Info("Waiting for certificate generation to complete") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <-certsReady | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| apiVersion: cert-manager.io/v1 | ||
| kind: Certificate | ||
| metadata: | ||
| name: training-operator-webhook-cert | ||
| namespace: kubeflow | ||
| spec: | ||
| isCA: true | ||
| secretName: training-operator-webhook-cert | ||
| issuerRef: | ||
| kind: Issuer | ||
| name: trainer-cert-issuer | ||
| commonName: ${TRAINER_SERVICE_NAME}.${TRAINER_NAMESPACE}.svc | ||
| dnsNames: | ||
| - ${TRAINER_SERVICE_NAME}.${TRAINER_NAMESPACE}.svc | ||
| - ${TRAINER_SERVICE_NAME}.${TRAINER_NAMESPACE}.svc.cluster.local | ||
| --- | ||
| apiVersion: cert-manager.io/v1 | ||
| kind: issuer | ||
| metadata: | ||
| name: trainer-cert-issuer | ||
| spec: | ||
| selfSigned: {} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| --- | ||
| varReference: | ||
| - path: spec/commanName | ||
| kind: Certificate | ||
| - path: spec/dnsNames | ||
| kind: Certificate | ||
| - path: spec/issuerRef/name | ||
| kind: Certificate | ||
| - path: metadata/annotations | ||
| kind: validatingwebhookconfiguration | ||
| nameReference: | ||
| - kind: issuer | ||
| group: cert-manager.io | ||
| fieldSpecs: | ||
| - kind: Certificate | ||
| group: cert-manager.io | ||
| path: spec/issuerRef/name |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| apiVersion: kustomize.config.k8s.io/v1beta1 | ||
| kind: Kustomization | ||
| namespace: kubeflow-system | ||
| resources: | ||
| - ../../base/crds/ | ||
| - ../../base/manager/ | ||
| - ../../base/webhook/ | ||
| - ../../base/rbac/ | ||
| - ../../base/runtimes/ | ||
| - certificate.yaml | ||
|
|
||
| images: | ||
| - name: kubeflow/training-operator-v2 | ||
| newName: kubeflow/training-operator-v2 | ||
| newTag: latest | ||
|
|
||
| patchesStrategicMerge: | ||
| - patch.yaml | ||
|
|
||
| vars: | ||
| - name: TRAINER_NAMESPACE | ||
| objref: | ||
| name: trainer-controller-manager | ||
| kind: Service | ||
| apiVersion: v2alpha1 | ||
| fieldref: | ||
| fieldpath: metadata.namespace | ||
| - name: TRAINER_SERVICE_NAME | ||
| objref: | ||
| name: trainer-controller-manager | ||
| kind: Service | ||
| apiVersion: v2alpha1 | ||
| fieldref: | ||
| fieldpath: metadata.name | ||
| - name: TRAINER_CERT_NAME | ||
| objref: | ||
| name: trainer-webhook-cert | ||
| kind: Certificate | ||
| apiVersion: cert-manager.io/v1 | ||
| fieldref: | ||
| fieldpath: metadata.name | ||
|
|
||
| configurations: | ||
| - config.yaml | ||
|
|
||
| secretGenerator: | ||
| - name: training-operator-v2-webhook-cert | ||
| namespace: kubeflow-system | ||
| options: | ||
| disableNameSuffixHash: true |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| apiVersion: admissionregistration.k8s.io/v1 | ||
| kind: validatingwebhookconfiguration | ||
| metadata: | ||
| name: trainer.kubeflow.org | ||
| annotations: | ||
| cert-manager.io/inject-ca-from: $(TRAINER_NAMESPACE)/$(TRAINER_CERT_NAME) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small confusion should we use webhookConfigurationName instead of namespace(as of V1) ?