diff --git a/manifests/v1beta1/components/controller/controller.yaml b/manifests/v1beta1/components/controller/controller.yaml index 0500be09145..c6f97b5f189 100644 --- a/manifests/v1beta1/components/controller/controller.yaml +++ b/manifests/v1beta1/components/controller/controller.yaml @@ -51,11 +51,18 @@ spec: fieldRef: fieldPath: metadata.namespace volumeMounts: + - mountPath: /tmp/cert + name: cert + readOnly: true - mountPath: /katib-config.yaml name: katib-config subPath: katib-config.yaml readOnly: true volumes: + - name: cert + secret: + defaultMode: 420 + secretName: katib-webhook-cert - name: katib-config configMap: name: katib-config diff --git a/manifests/v1beta1/components/controller/rbac.yaml b/manifests/v1beta1/components/controller/rbac.yaml index f96f0e60c90..8ddebc191d2 100644 --- a/manifests/v1beta1/components/controller/rbac.yaml +++ b/manifests/v1beta1/components/controller/rbac.yaml @@ -57,7 +57,7 @@ rules: - "get" - "list" - "watch" - - "create" + - "update" - "delete" - apiGroups: - apps diff --git a/manifests/v1beta1/components/webhook/kustomization.yaml b/manifests/v1beta1/components/webhook/kustomization.yaml index 36d137d8db2..09b4fd9f707 100644 --- a/manifests/v1beta1/components/webhook/kustomization.yaml +++ b/manifests/v1beta1/components/webhook/kustomization.yaml @@ -4,3 +4,4 @@ kind: Kustomization resources: - webhooks.yaml + - secret.yaml diff --git a/manifests/v1beta1/components/webhook/secret.yaml b/manifests/v1beta1/components/webhook/secret.yaml new file mode 100644 index 00000000000..346c185f2ba --- /dev/null +++ b/manifests/v1beta1/components/webhook/secret.yaml @@ -0,0 +1,4 @@ +apiVersion: v1 +kind: Secret +metadata: + name: katib-webhook-cert diff --git a/pkg/apis/config/v1beta1/defaults.go b/pkg/apis/config/v1beta1/defaults.go index 2ff00a0b717..936c1b03238 100644 --- a/pkg/apis/config/v1beta1/defaults.go +++ b/pkg/apis/config/v1beta1/defaults.go @@ -38,8 +38,8 @@ const ( DefaultDiskRequest = "500Mi" // DefaultWebhookServiceName is the default service name for the admission webhooks. DefaultWebhookServiceName = "katib-controller" - // DefaultControllerName is the default katib-controller deployment name. - DefaultControllerName = "katib-controller" + // DefaultWebhookSecretName is the default secret name to save the certs for the admission webhooks. + DefaultWebhookSecretName = "katib-webhook-cert" ) var ( @@ -103,14 +103,14 @@ func setControllerConfig(controllerConfig *ControllerConfig) { } func setCertGeneratorConfig(certGeneratorConfig *CertGeneratorConfig) { - if len(certGeneratorConfig.ServiceName) != 0 { + if len(certGeneratorConfig.ServiceName) != 0 || len(certGeneratorConfig.WebhookSecretName) != 0 { certGeneratorConfig.Enable = true } if certGeneratorConfig.Enable && len(certGeneratorConfig.ServiceName) == 0 { certGeneratorConfig.ServiceName = DefaultWebhookServiceName } - if certGeneratorConfig.Enable && len(certGeneratorConfig.ControllerName) == 0 { - certGeneratorConfig.ControllerName = DefaultControllerName + if certGeneratorConfig.Enable && len(certGeneratorConfig.WebhookSecretName) == 0 { + certGeneratorConfig.WebhookSecretName = DefaultWebhookSecretName } } diff --git a/pkg/apis/config/v1beta1/defaults_test.go b/pkg/apis/config/v1beta1/defaults_test.go index 587c12db159..ef593e732bc 100644 --- a/pkg/apis/config/v1beta1/defaults_test.go +++ b/pkg/apis/config/v1beta1/defaults_test.go @@ -274,14 +274,14 @@ func TestSetCertGeneratorConfig(t *testing.T) { }{ "All parameters correctly are specified": { config: CertGeneratorConfig{ - Enable: true, - ServiceName: "test", - ControllerName: "katib-test", + Enable: true, + ServiceName: "test", + WebhookSecretName: "katib-test", }, wantConfig: CertGeneratorConfig{ - Enable: true, - ServiceName: "test", - ControllerName: "katib-test", + Enable: true, + ServiceName: "test", + WebhookSecretName: "katib-test", }, }, "CertGeneratorConfig is empty": { @@ -293,9 +293,9 @@ func TestSetCertGeneratorConfig(t *testing.T) { Enable: true, }, wantConfig: CertGeneratorConfig{ - Enable: true, - ServiceName: DefaultWebhookServiceName, - ControllerName: DefaultControllerName, + Enable: true, + ServiceName: DefaultWebhookServiceName, + WebhookSecretName: DefaultWebhookSecretName, }, }, } diff --git a/pkg/apis/config/v1beta1/types.go b/pkg/apis/config/v1beta1/types.go index f9795cd3d87..83c230ce67a 100644 --- a/pkg/apis/config/v1beta1/types.go +++ b/pkg/apis/config/v1beta1/types.go @@ -82,17 +82,19 @@ type ControllerConfig struct { LeaderElectionID string `json:"leaderElectionID,omitempty"` } +// CertGeneratorConfig is the certGenerator structure in Katib config. type CertGeneratorConfig struct { // Enable indicates the internal cert-generator is enabled. // Defaults to 'false'. Enable bool `json:"enable,omitempty"` // ServiceName indicates which service is used for the admission webhook. + // If it is set, the cert-generator forcefully is enabled even if the '.init.certGenerator.enable' is false. // Defaults to 'katib-controller'. ServiceName string `json:"serviceName,omitempty"` - // ControllerName indicates the katib-controller deployment name. - // It is used as an owner for secret embedded certs. - // Defaults to 'katib-controller'. - ControllerName string `json:"controllerName,omitempty"` + // WebhookSecretName indicates which secrets is used to save the certs for the admission webhook. + // If it is set, the cert-generator forcefully is enabled even if the '.init.certGenerator.enable' is false. + // Defaults to 'katib-webhook-cert'. + WebhookSecretName string `json:"webhookSecretName,omitempty"` } // SuggestionConfig is the suggestion structure in Katib config. diff --git a/pkg/cert-generator/v1beta1/const.go b/pkg/cert-generator/v1beta1/const.go index 31b9c18a479..cda52c72df2 100644 --- a/pkg/cert-generator/v1beta1/const.go +++ b/pkg/cert-generator/v1beta1/const.go @@ -17,7 +17,6 @@ limitations under the License. package v1beta1 const ( - Secret = "katib-webhook-cert" Webhook = "katib.kubeflow.org" serverKeyName = "tls.key" serverCertName = "tls.crt" diff --git a/pkg/cert-generator/v1beta1/generator.go b/pkg/cert-generator/v1beta1/generator.go index 3f275cec07a..ceade59c911 100644 --- a/pkg/cert-generator/v1beta1/generator.go +++ b/pkg/cert-generator/v1beta1/generator.go @@ -26,16 +26,11 @@ import ( "errors" "fmt" "math/big" - "os" - "path" "strings" "time" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" - appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/klog" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -49,17 +44,16 @@ var ( errCertCheckFail = errors.New("failed to check if certs already exist") errCreateCertFail = errors.New("failed to create certs") errCreateCertSecretFail = errors.New("failed to create secret embedded certs") - errSaveCertOnLocal = errors.New("failed to save certs on local") errInjectCertError = errors.New("failed to inject certs into WebhookConfigurations") ) // CertGenerator is the manager to generate certs. type CertGenerator struct { - namespace string - serviceName string - controllerName string - kubeClient client.Client - certsReady chan struct{} + namespace string + serviceName string + secretName string + kubeClient client.Client + certsReady chan struct{} certs *certificates fullServiceDomain string @@ -84,18 +78,18 @@ func (c *CertGenerator) NeedLeaderElection() bool { // AddToManager adds the cert-generator to the manager. func AddToManager(mgr manager.Manager, config configv1beta1.CertGeneratorConfig, certsReady chan struct{}) error { return mgr.Add(&CertGenerator{ - namespace: consts.DefaultKatibNamespace, - serviceName: config.ServiceName, - controllerName: config.ControllerName, - kubeClient: mgr.GetClient(), - certsReady: certsReady, + namespace: consts.DefaultKatibNamespace, + serviceName: config.ServiceName, + secretName: config.WebhookSecretName, + kubeClient: mgr.GetClient(), + certsReady: certsReady, }) } // generate generates certificates for the admission webhooks. func (c *CertGenerator) generate(ctx context.Context) error { controllerService := &corev1.Service{} - if err := c.kubeClient.Get(ctx, client.ObjectKey{Namespace: c.namespace, Name: c.serviceName}, controllerService); err != nil { + if err := c.kubeClient.Get(ctx, client.ObjectKey{Name: c.serviceName, Namespace: c.namespace}, controllerService); err != nil { return fmt.Errorf("%w: %v", errServiceNotFound, err) } @@ -109,13 +103,10 @@ func (c *CertGenerator) generate(ctx context.Context) error { if err = c.createCert(); err != nil { return fmt.Errorf("%w: %v", errCreateCertFail, err) } - if err = c.createCertSecret(ctx); err != nil { + if err = c.updateCertSecret(ctx); err != nil { return fmt.Errorf("%w: %v", errCreateCertSecretFail, err) } } - if err = c.saveCertOnLocal(); err != nil { - return fmt.Errorf("%w: %v", errSaveCertOnLocal, err) - } if err = c.injectCert(ctx); err != nil { return fmt.Errorf("%w: %v", errInjectCertError, err) } @@ -127,10 +118,7 @@ func (c *CertGenerator) generate(ctx context.Context) error { // since another controller pod will create the secret. func (c *CertGenerator) isCertExist(ctx context.Context) (bool, error) { secret := &corev1.Secret{} - if err := c.kubeClient.Get(ctx, client.ObjectKey{Name: Secret, Namespace: c.namespace}, secret); err != nil { - if apierrors.IsNotFound(err) { - return false, nil - } + if err := c.kubeClient.Get(ctx, client.ObjectKey{Name: c.secretName, Namespace: c.namespace}, secret); err != nil { return false, err } key := secret.Data[serverKeyName] @@ -145,26 +133,6 @@ func (c *CertGenerator) isCertExist(ctx context.Context) (bool, error) { return false, nil } -// saveCertOnLocal saves the certs on local. -func (c *CertGenerator) saveCertOnLocal() error { - if err := os.MkdirAll(consts.CertDir, 0760); err != nil { - return err - } - f, err := os.Create(path.Join(consts.CertDir, serverKeyName)) - if err != nil { - return err - } - if _, err = f.Write(c.certs.keyPem); err != nil { - return err - } - f, err = os.Create(path.Join(consts.CertDir, serverCertName)) - if err != nil { - return err - } - _, err = f.Write(c.certs.certPem) - return err -} - // createCert creates the self-signed certificate and private key. func (c *CertGenerator) createCert() error { now := time.Now() @@ -198,51 +166,18 @@ func (c *CertGenerator) createCert() error { return nil } -// createCertSecret creates Secret embedded tls.key and tls.crt. -func (c *CertGenerator) createCertSecret(ctx context.Context) error { - controller := &appsv1.Deployment{} - if err := c.kubeClient.Get(ctx, client.ObjectKey{Name: c.controllerName, Namespace: c.namespace}, controller); err != nil { - return err - } - - // Create secret with CA cert and server cert/key. - // Add ownerReferences to clean-up secret with controller Pod. - webhookCertSecret := &corev1.Secret{ - TypeMeta: metav1.TypeMeta{ - Kind: "Secret", - APIVersion: corev1.SchemeGroupVersion.String(), - }, - ObjectMeta: metav1.ObjectMeta{ - Name: Secret, - Namespace: c.namespace, - OwnerReferences: []metav1.OwnerReference{ - *metav1.NewControllerRef(controller, appsv1.SchemeGroupVersion.WithKind("Deployment")), - }, - }, - Type: corev1.SecretTypeTLS, - Data: map[string][]byte{ - serverKeyName: c.certs.keyPem, - serverCertName: c.certs.certPem, - }, - } - - oldSecret := &corev1.Secret{} - err := c.kubeClient.Get(ctx, client.ObjectKey{Namespace: c.namespace, Name: Secret}, oldSecret) - if client.IgnoreNotFound(err) != nil { +// updateCertSecret updates Secret embedded tls.key and tls.crt. +func (c *CertGenerator) updateCertSecret(ctx context.Context) error { + secret := &corev1.Secret{} + if err := c.kubeClient.Get(ctx, client.ObjectKey{Name: c.secretName, Namespace: c.namespace}, secret); err != nil { return err } - if err == nil { - klog.Warning("Previous secret was found and removed.") - if err = c.kubeClient.Delete(ctx, oldSecret); err != nil { - return err - } + if len(secret.Data) == 0 { + secret.Data = make(map[string][]byte, 2) } - - klog.Infof("Creating Secret: %q", Secret) - if err = c.kubeClient.Create(ctx, webhookCertSecret); err != nil { - return err - } - return nil + secret.Data[serverKeyName] = c.certs.keyPem + secret.Data[serverCertName] = c.certs.certPem + return c.kubeClient.Update(ctx, secret) } // injectCert applies patch to ValidatingWebhookConfiguration and MutatingWebhookConfiguration. diff --git a/pkg/cert-generator/v1beta1/generator_test.go b/pkg/cert-generator/v1beta1/generator_test.go index 320cb154a5a..dc7464f1f90 100644 --- a/pkg/cert-generator/v1beta1/generator_test.go +++ b/pkg/cert-generator/v1beta1/generator_test.go @@ -18,15 +18,12 @@ package v1beta1 import ( "context" - "os" - "path/filepath" "strings" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" admissionregistration "k8s.io/api/admissionregistration/v1" - appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/scheme" @@ -34,23 +31,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" configv1beta1 "github.com/kubeflow/katib/pkg/apis/config/v1beta1" - "github.com/kubeflow/katib/pkg/controller.v1beta1/consts" ) func TestGenerate(t *testing.T) { const testNamespace = "test" - controllerDeployment := &appsv1.Deployment{ - TypeMeta: metav1.TypeMeta{ - Kind: "Deployment", - APIVersion: appsv1.SchemeGroupVersion.String(), - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "katib-controller", - Namespace: testNamespace, - UID: "test", - }, - } emptyVWebhookConfig := &admissionregistration.ValidatingWebhookConfiguration{ TypeMeta: metav1.TypeMeta{ APIVersion: admissionregistration.SchemeGroupVersion.String(), @@ -85,13 +70,13 @@ func TestGenerate(t *testing.T) { }, }, } - controllerSecret := &corev1.Secret{ + webhookSecret := &corev1.Secret{ TypeMeta: metav1.TypeMeta{ Kind: "Secret", APIVersion: corev1.SchemeGroupVersion.String(), }, ObjectMeta: metav1.ObjectMeta{ - Name: Secret, + Name: "katib-test-secret", Namespace: testNamespace, }, } @@ -113,90 +98,72 @@ func TestGenerate(t *testing.T) { }{ "Generate successfully": { opts: &CertGenerator{ - namespace: testNamespace, - serviceName: "katib-controller", - controllerName: "katib-controller", + namespace: testNamespace, + serviceName: "katib-controller", + secretName: "katib-test-secret", }, objects: []client.Object{ - controllerDeployment, emptyVWebhookConfig, emptyMWebhookConfig, controllerService, + webhookSecret, }, }, - "There is an old Secret, katib-webhook-cert": { + "There is not ValidatingWebhookConfiguration": { opts: &CertGenerator{ - namespace: testNamespace, - serviceName: "katib-controller", - controllerName: "katib-controller", + namespace: testNamespace, + serviceName: "katib-controller", + secretName: "katib-test-secret", }, objects: []client.Object{ - controllerDeployment, - emptyVWebhookConfig, emptyMWebhookConfig, controllerService, - controllerSecret, + webhookSecret, }, + wantError: errInjectCertError, }, - "There is not Deployment, katib-controller": { + "There is not MutatingWebhookConfiguration": { opts: &CertGenerator{ - namespace: testNamespace, - serviceName: "katib-controller", - controllerName: "katib-controller", + namespace: testNamespace, + serviceName: "katib-controller", + secretName: "katib-test-secret", }, objects: []client.Object{ emptyVWebhookConfig, - emptyMWebhookConfig, - controllerService, - }, - wantError: errCreateCertSecretFail, - }, - "There is not ValidatingWebhookConfiguration": { - opts: &CertGenerator{ - namespace: testNamespace, - serviceName: "katib-controller", - controllerName: "katib-controller", - }, - objects: []client.Object{ - controllerDeployment, - emptyMWebhookConfig, controllerService, + webhookSecret, }, wantError: errInjectCertError, }, - "There is not MutatingWebhookConfiguration": { + "There is not Service katib-controller": { opts: &CertGenerator{ - namespace: testNamespace, - serviceName: "katib-controller", - controllerName: "katib-controller", + namespace: testNamespace, + serviceName: "katib-controller", + secretName: "katib-test-secret", }, objects: []client.Object{ - controllerDeployment, emptyVWebhookConfig, - controllerService, + emptyMWebhookConfig, + webhookSecret, }, - wantError: errInjectCertError, + wantError: errServiceNotFound, }, - "There is no Service katib-controller": { + "There is not Secret katib-webhook-cert": { opts: &CertGenerator{ - namespace: testNamespace, - serviceName: "katib-controller", - controllerName: "katib-controller", + namespace: testNamespace, + serviceName: "katib-controller", + secretName: "katib-test-secret", }, objects: []client.Object{ - controllerDeployment, emptyVWebhookConfig, emptyMWebhookConfig, + controllerService, }, - wantError: errServiceNotFound, + wantError: errCertCheckFail, }, } for name, tc := range tests { t.Run(name, func(t *testing.T) { - if err := os.RemoveAll(consts.CertDir); err != nil { - t.Fatalf("Failed to clean up cert dir: %v", err) - } - kc := buildFakeClient(tc.objects) tc.opts.kubeClient = kc err := tc.opts.generate(context.Background()) @@ -206,11 +173,8 @@ func TestGenerate(t *testing.T) { if tc.wantError == nil { secret := &corev1.Secret{} - if err = kc.Get(context.Background(), client.ObjectKey{Name: Secret, Namespace: testNamespace}, secret); err != nil { - t.Fatalf("Failed to get a controllerSecret: %v", err) - } - if !metav1.IsControlledBy(secret, controllerDeployment) { - t.Errorf("Unexpected owner for the secret: %v", secret.OwnerReferences) + if err = kc.Get(context.Background(), client.ObjectKey{Name: tc.opts.secretName, Namespace: testNamespace}, secret); err != nil { + t.Fatalf("Failed to get a webhookSecret: %v", err) } if len(secret.Data[serverKeyName]) == 0 { t.Errorf("Unexpected tls.key embedded in secret: %v", secret.Data) @@ -219,13 +183,6 @@ func TestGenerate(t *testing.T) { t.Errorf("Unexpected tls.crt embedded in secret: %v", secret.Data) } - if _, err = os.Stat(filepath.Join(consts.CertDir, serverKeyName)); err != nil { - t.Errorf("Failed to find tls.key: %v", err) - } - if _, err = os.Stat(filepath.Join(consts.CertDir, serverCertName)); err != nil { - t.Errorf("Failed to find tls.crt: %v", err) - } - vConfig := &admissionregistration.ValidatingWebhookConfiguration{} if err = kc.Get(context.Background(), client.ObjectKey{Name: Webhook}, vConfig); err != nil { t.Fatalf("Failed to get a ValidatingWebhookConfiguration: %v", err) diff --git a/pkg/util/v1beta1/katibconfig/config_test.go b/pkg/util/v1beta1/katibconfig/config_test.go index a6f5c631734..bc480e9aaa2 100644 --- a/pkg/util/v1beta1/katibconfig/config_test.go +++ b/pkg/util/v1beta1/katibconfig/config_test.go @@ -388,7 +388,7 @@ init: certGenerator: enable: true serviceName: katib-test - controllerName: katib-test-ctrl + webhookSecretName: katib-test-secret controller: experimentSuggestionName: test metricsAddr: :8081 @@ -440,9 +440,9 @@ runtime: katibConfigFile: fullInitConfig, wantInitConfigData: configv1beta1.InitConfig{ CertGeneratorConfig: configv1beta1.CertGeneratorConfig{ - Enable: true, - ServiceName: "katib-test", - ControllerName: "katib-test-ctrl", + Enable: true, + ServiceName: "katib-test", + WebhookSecretName: "katib-test-secret", }, ControllerConfig: configv1beta1.ControllerConfig{ ExperimentSuggestionName: "test",