From 69a57cedcde4aefd1a45423259a439ca3953fa87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ege=20G=C3=BCne=C5=9F?= Date: Wed, 1 Jun 2022 10:28:28 +0300 Subject: [PATCH 1/2] K8SPXC-1030: Don't delete cert-manager certs by default We shouldn't set owner references to cert-manager objects if we don't want to delete secrets too. This way, after the PXC cluster is deleted issuers, certificates and their secrets will remain intact in the cluster. If users want to cleanup objects created for SSL, we introduce a new finalizer: `delete-ssl`. If this finalizer is set, the operator will delete secrets, certificates and issuers. Unfortunately, cert-manager doesn't set owner reference to the secret it created and this behaviour can only configured by command line flag in the controller. Since we can't control how users deploy cert-manager to their clusters, we shouldn't rely on this feature and cleanup certificates and secrets altogether. Hopefully, https://github.com/cert-manager/cert-manager/pull/5158 will merged and we can configure this behaviour on certificate level. --- deploy/cr.yaml | 1 + pkg/controller/pxc/controller.go | 64 ++++++++++++++++++++++++++++++++ pkg/controller/pxc/tls.go | 37 +++++++----------- 3 files changed, 78 insertions(+), 24 deletions(-) diff --git a/deploy/cr.yaml b/deploy/cr.yaml index 9193e1d60d..2d74e5c20a 100644 --- a/deploy/cr.yaml +++ b/deploy/cr.yaml @@ -4,6 +4,7 @@ metadata: name: cluster1 finalizers: - delete-pxc-pods-in-order +# - delete-ssl # - delete-proxysql-pvc # - delete-pxc-pvc # annotations: diff --git a/pkg/controller/pxc/controller.go b/pkg/controller/pxc/controller.go index 99b2a720c1..4ebe0740fb 100644 --- a/pkg/controller/pxc/controller.go +++ b/pkg/controller/pxc/controller.go @@ -10,6 +10,7 @@ import ( "sync/atomic" "time" + cm "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" "github.com/go-logr/logr" "github.com/go-logr/zapr" "github.com/pkg/errors" @@ -228,6 +229,8 @@ func (r *ReconcilePerconaXtraDBCluster) Reconcile(_ context.Context, request rec for _, fnlz := range o.GetFinalizers() { var sfs api.StatefulApp switch fnlz { + case "delete-ssl": + err = r.deleteCerts(o) case "delete-proxysql-pvc": sfs = statefulset.NewProxy(o) // deletePVC is always true on this stage @@ -1172,6 +1175,67 @@ func (r *ReconcilePerconaXtraDBCluster) deleteSecrets(cr *api.PerconaXtraDBClust return nil } +func (r *ReconcilePerconaXtraDBCluster) deleteCerts(cr *api.PerconaXtraDBCluster) error { + issuers := []string{ + cr.Name + "-pxc-ca-issuer", + cr.Name + "-pxc-issuer", + } + for _, issuerName := range issuers { + issuer := &cm.Issuer{} + err := r.client.Get(context.TODO(), types.NamespacedName{ + Namespace: cr.Namespace, + Name: issuerName, + }, issuer) + if err != nil { + continue + } + + err = r.client.Delete(context.TODO(), issuer, &client.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &issuer.UID}}) + if err != nil { + return errors.Wrapf(err, "delete issuer %s", issuerName) + } + } + + certs := []string{ + cr.Name + "-ssl", + cr.Name + "-ssl-internal", + cr.Name + "-ca-cert", + } + for _, certName := range certs { + secret := &corev1.Secret{} + err := r.client.Get(context.TODO(), types.NamespacedName{ + Namespace: cr.Namespace, + Name: certName, + }, secret) + if client.IgnoreNotFound(err) != nil { + continue + } + + err = r.client.Delete(context.TODO(), secret, + &client.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &secret.UID}}) + if client.IgnoreNotFound(err) != nil { + return errors.Wrapf(err, "delete secret %s", certName) + } + + cert := &cm.Certificate{} + err = r.client.Get(context.TODO(), types.NamespacedName{ + Namespace: cr.Namespace, + Name: certName, + }, cert) + if err != nil { + continue + } + + err = r.client.Delete(context.TODO(), cert, + &client.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &cert.UID}}) + if err != nil { + return errors.Wrapf(err, "delete certificate %s", certName) + } + } + + return nil +} + func setControllerReference(ro runtime.Object, obj metav1.Object, scheme *runtime.Scheme) error { ownerRef, err := OwnerRef(ro, scheme) if err != nil { diff --git a/pkg/controller/pxc/tls.go b/pkg/controller/pxc/tls.go index ec7cc1bdba..bddde99c6a 100644 --- a/pkg/controller/pxc/tls.go +++ b/pkg/controller/pxc/tls.go @@ -62,12 +62,6 @@ func (r *ReconcilePerconaXtraDBCluster) reconcileSSL(cr *api.PerconaXtraDBCluste } func (r *ReconcilePerconaXtraDBCluster) createSSLByCertManager(cr *api.PerconaXtraDBCluster) error { - owner, err := OwnerRef(cr, r.scheme) - if err != nil { - return err - } - ownerReferences := []metav1.OwnerReference{owner} - issuerName := cr.Name + "-pxc-issuer" caIssuerName := cr.Name + "-pxc-ca-issuer" issuerKind := "Issuer" @@ -77,15 +71,14 @@ func (r *ReconcilePerconaXtraDBCluster) createSSLByCertManager(cr *api.PerconaXt issuerName = cr.Spec.TLS.IssuerConf.Name issuerGroup = cr.Spec.TLS.IssuerConf.Group } else { - if err := r.createIssuer(ownerReferences, cr.Namespace, caIssuerName, ""); err != nil { + if err := r.createIssuer(cr.Namespace, caIssuerName, ""); err != nil { return err } caCert := &cm.Certificate{ ObjectMeta: metav1.ObjectMeta{ - Name: cr.Name + "-ca-cert", - Namespace: cr.Namespace, - OwnerReferences: ownerReferences, + Name: cr.Name + "-ca-cert", + Namespace: cr.Namespace, }, Spec: cm.CertificateSpec{ SecretName: cr.Name + "-ca-cert", @@ -101,7 +94,7 @@ func (r *ReconcilePerconaXtraDBCluster) createSSLByCertManager(cr *api.PerconaXt }, } - err = r.client.Create(context.TODO(), caCert) + err := r.client.Create(context.TODO(), caCert) if err != nil && !k8serr.IsAlreadyExists(err) { return fmt.Errorf("create CA certificate: %v", err) } @@ -110,16 +103,15 @@ func (r *ReconcilePerconaXtraDBCluster) createSSLByCertManager(cr *api.PerconaXt return err } - if err := r.createIssuer(ownerReferences, cr.Namespace, issuerName, caCert.Spec.SecretName); err != nil { + if err := r.createIssuer(cr.Namespace, issuerName, caCert.Spec.SecretName); err != nil { return err } } kubeCert := &cm.Certificate{ ObjectMeta: metav1.ObjectMeta{ - Name: cr.Name + "-ssl", - Namespace: cr.Namespace, - OwnerReferences: ownerReferences, + Name: cr.Name + "-ssl", + Namespace: cr.Namespace, }, Spec: cm.CertificateSpec{ SecretName: cr.Spec.PXC.SSLSecretName, @@ -143,7 +135,7 @@ func (r *ReconcilePerconaXtraDBCluster) createSSLByCertManager(cr *api.PerconaXt kubeCert.Spec.DNSNames = append(kubeCert.Spec.DNSNames, cr.Spec.TLS.SANs...) } - err = r.client.Create(context.TODO(), kubeCert) + err := r.client.Create(context.TODO(), kubeCert) if err != nil && !k8serr.IsAlreadyExists(err) { return fmt.Errorf("create certificate: %v", err) } @@ -154,9 +146,8 @@ func (r *ReconcilePerconaXtraDBCluster) createSSLByCertManager(cr *api.PerconaXt kubeCert = &cm.Certificate{ ObjectMeta: metav1.ObjectMeta{ - Name: cr.Name + "-ssl-internal", - Namespace: cr.Namespace, - OwnerReferences: ownerReferences, + Name: cr.Name + "-ssl-internal", + Namespace: cr.Namespace, }, Spec: cm.CertificateSpec{ SecretName: cr.Spec.PXC.SSLInternalSecretName, @@ -220,8 +211,7 @@ func (r *ReconcilePerconaXtraDBCluster) waitForCerts(namespace string, secretsLi } } -func (r *ReconcilePerconaXtraDBCluster) createIssuer(ownRef []metav1.OwnerReference, namespace, issuer string, caCertSecret string) error { - +func (r *ReconcilePerconaXtraDBCluster) createIssuer(namespace, issuer string, caCertSecret string) error { spec := cm.IssuerSpec{} if caCertSecret == "" { @@ -240,9 +230,8 @@ func (r *ReconcilePerconaXtraDBCluster) createIssuer(ownRef []metav1.OwnerRefere err := r.client.Create(context.TODO(), &cm.Issuer{ ObjectMeta: metav1.ObjectMeta{ - Name: issuer, - Namespace: namespace, - OwnerReferences: ownRef, + Name: issuer, + Namespace: namespace, }, Spec: spec, }) From 658e73dfc00be8cb1428da3a883ca27c0d2fc53f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ege=20G=C3=BCne=C5=9F?= Date: Fri, 15 Jul 2022 14:36:50 +0300 Subject: [PATCH 2/2] fix tests --- .../compare/certificate_no-proxysql-ssl-internal.yml | 4 ---- e2e-tests/init-deploy/compare/certificate_no-proxysql-ssl.yml | 4 ---- .../init-deploy/compare/issuer_no-proxysql-pxc-issuer.yml | 4 ---- .../compare/certificate_some-name-tls-issueref-ssl.yml | 4 ---- .../compare/certificate_some-name-tls-issue-ssl.yml | 4 ---- .../compare/issuer_some-name-tls-issue-pxc-ca-issuer.yml | 4 ---- .../compare/issuer_some-name-tls-issue-pxc-issuer.yml | 4 ---- 7 files changed, 28 deletions(-) diff --git a/e2e-tests/init-deploy/compare/certificate_no-proxysql-ssl-internal.yml b/e2e-tests/init-deploy/compare/certificate_no-proxysql-ssl-internal.yml index 7fa8592273..27532b245c 100644 --- a/e2e-tests/init-deploy/compare/certificate_no-proxysql-ssl-internal.yml +++ b/e2e-tests/init-deploy/compare/certificate_no-proxysql-ssl-internal.yml @@ -3,10 +3,6 @@ kind: Certificate metadata: generation: 1 name: no-proxysql-ssl-internal - ownerReferences: - - controller: true - kind: PerconaXtraDBCluster - name: no-proxysql spec: commonName: no-proxysql-pxc dnsNames: diff --git a/e2e-tests/init-deploy/compare/certificate_no-proxysql-ssl.yml b/e2e-tests/init-deploy/compare/certificate_no-proxysql-ssl.yml index 369ad6ee63..34595bb07b 100644 --- a/e2e-tests/init-deploy/compare/certificate_no-proxysql-ssl.yml +++ b/e2e-tests/init-deploy/compare/certificate_no-proxysql-ssl.yml @@ -3,10 +3,6 @@ kind: Certificate metadata: generation: 1 name: no-proxysql-ssl - ownerReferences: - - controller: true - kind: PerconaXtraDBCluster - name: no-proxysql spec: commonName: no-proxysql-proxysql dnsNames: diff --git a/e2e-tests/init-deploy/compare/issuer_no-proxysql-pxc-issuer.yml b/e2e-tests/init-deploy/compare/issuer_no-proxysql-pxc-issuer.yml index d90ab9741e..5f697fea1d 100644 --- a/e2e-tests/init-deploy/compare/issuer_no-proxysql-pxc-issuer.yml +++ b/e2e-tests/init-deploy/compare/issuer_no-proxysql-pxc-issuer.yml @@ -3,10 +3,6 @@ kind: Issuer metadata: generation: 1 name: no-proxysql-pxc-issuer - ownerReferences: - - controller: true - kind: PerconaXtraDBCluster - name: no-proxysql spec: ca: secretName: no-proxysql-ca-cert diff --git a/e2e-tests/tls-issue-cert-manager-ref/compare/certificate_some-name-tls-issueref-ssl.yml b/e2e-tests/tls-issue-cert-manager-ref/compare/certificate_some-name-tls-issueref-ssl.yml index 5b43d0b405..4dda03f504 100644 --- a/e2e-tests/tls-issue-cert-manager-ref/compare/certificate_some-name-tls-issueref-ssl.yml +++ b/e2e-tests/tls-issue-cert-manager-ref/compare/certificate_some-name-tls-issueref-ssl.yml @@ -3,10 +3,6 @@ kind: Certificate metadata: generation: 1 name: some-name-tls-issueref-ssl - ownerReferences: - - controller: true - kind: PerconaXtraDBCluster - name: some-name-tls-issueref spec: commonName: some-name-tls-issueref-proxysql dnsNames: diff --git a/e2e-tests/tls-issue-cert-manager/compare/certificate_some-name-tls-issue-ssl.yml b/e2e-tests/tls-issue-cert-manager/compare/certificate_some-name-tls-issue-ssl.yml index c03a1f8efa..04fda21de7 100644 --- a/e2e-tests/tls-issue-cert-manager/compare/certificate_some-name-tls-issue-ssl.yml +++ b/e2e-tests/tls-issue-cert-manager/compare/certificate_some-name-tls-issue-ssl.yml @@ -3,10 +3,6 @@ kind: Certificate metadata: generation: 1 name: some-name-tls-issue-ssl - ownerReferences: - - controller: true - kind: PerconaXtraDBCluster - name: some-name-tls-issue spec: commonName: some-name-tls-issue-proxysql dnsNames: diff --git a/e2e-tests/tls-issue-cert-manager/compare/issuer_some-name-tls-issue-pxc-ca-issuer.yml b/e2e-tests/tls-issue-cert-manager/compare/issuer_some-name-tls-issue-pxc-ca-issuer.yml index f623226af3..e59fa131fd 100644 --- a/e2e-tests/tls-issue-cert-manager/compare/issuer_some-name-tls-issue-pxc-ca-issuer.yml +++ b/e2e-tests/tls-issue-cert-manager/compare/issuer_some-name-tls-issue-pxc-ca-issuer.yml @@ -3,9 +3,5 @@ kind: Issuer metadata: generation: 1 name: some-name-tls-issue-pxc-ca-issuer - ownerReferences: - - controller: true - kind: PerconaXtraDBCluster - name: some-name-tls-issue spec: selfSigned: {} diff --git a/e2e-tests/tls-issue-cert-manager/compare/issuer_some-name-tls-issue-pxc-issuer.yml b/e2e-tests/tls-issue-cert-manager/compare/issuer_some-name-tls-issue-pxc-issuer.yml index feaca92177..745ae1263c 100644 --- a/e2e-tests/tls-issue-cert-manager/compare/issuer_some-name-tls-issue-pxc-issuer.yml +++ b/e2e-tests/tls-issue-cert-manager/compare/issuer_some-name-tls-issue-pxc-issuer.yml @@ -3,10 +3,6 @@ kind: Issuer metadata: generation: 1 name: some-name-tls-issue-pxc-issuer - ownerReferences: - - controller: true - kind: PerconaXtraDBCluster - name: some-name-tls-issue spec: ca: secretName: some-name-tls-issue-ca-cert