From e646928281d8bf0c9b571656dbc9672fd3d4ac92 Mon Sep 17 00:00:00 2001 From: Ming Wang Date: Tue, 24 Oct 2023 13:57:53 -0400 Subject: [PATCH 1/7] copy secret in each namespace --- internal/controllers/certmanager.go | 29 ++++++++++++++++++- .../clustercryostat_controller_test.go | 13 +++++++++ .../resource_definitions/certificates.go | 4 +-- internal/controllers/reconciler_test.go | 14 +++++++-- internal/test/clients.go | 2 +- internal/test/resources.go | 4 +-- 6 files changed, 58 insertions(+), 8 deletions(-) diff --git a/internal/controllers/certmanager.go b/internal/controllers/certmanager.go index 93bdf044..7f86ce51 100644 --- a/internal/controllers/certmanager.go +++ b/internal/controllers/certmanager.go @@ -56,7 +56,7 @@ func (r *Reconciler) setupTLS(ctx context.Context, cr *model.CryostatInstance) ( } // Create CA certificate for Cryostat using the self-signed issuer - caCert := resources.NewCryostatCACert(cr) + caCert := resources.NewCryostatCACert(cr, cr.InstallNamespace) err = r.createOrUpdateCertificate(ctx, caCert, cr.Object) if err != nil { return nil, err @@ -122,6 +122,33 @@ func (r *Reconciler) setupTLS(ctx context.Context, cr *model.CryostatInstance) ( return nil, err } + // Copy Cryostat CA secret in each target namespace + for _, ns := range cr.TargetNamespaces { + secret, err := r.GetCertificateSecret(ctx, caCert) + if err != nil { + return nil, err + } + namespaceSecret := secret.DeepCopy() + namespaceSecret.Namespace = ns + } + // Delete any Cryostat CA secrets in target namespaces that are no longer requested + for _, ns := range toDelete(cr) { + secret, err := r.GetCertificateSecret(ctx, caCert) + if err != nil { + return nil, err + } + namespaceSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secret.Name, + Namespace: ns, + }, + } + err = r.deleteSecret(ctx, namespaceSecret) + if err != nil { + return nil, err + } + } + // Get the Cryostat CA certificate bytes from certificate secret caBytes, err := r.getCertficateBytes(ctx, caCert) if err != nil { diff --git a/internal/controllers/clustercryostat_controller_test.go b/internal/controllers/clustercryostat_controller_test.go index 4c93df77..2aa989e9 100644 --- a/internal/controllers/clustercryostat_controller_test.go +++ b/internal/controllers/clustercryostat_controller_test.go @@ -66,6 +66,10 @@ var _ = Describe("ClusterCryostatController", func() { t.expectRBAC() }) + It("should create Cryostat CA Certificate in each namespace", func() { + t.expectCertificates() + }) + It("should update the target namespaces in Status", func() { t.expectTargetNamespaces() }) @@ -94,6 +98,15 @@ var _ = Describe("ClusterCryostatController", func() { Expect(err).ToNot(BeNil()) Expect(errors.IsNotFound(err)).To(BeTrue()) }) + It("leave Cryostat CA Certificate for the first namespace", func() { + t.expectCertificates() + }) + It("should remove Cryostat CA Certificate from the second namespace", func() { + certificate := t.NewCACert(targetNamespaces[1]) + err := t.Client.Get(context.Background(), types.NamespacedName{Name: certificate.Name, Namespace: certificate.Namespace}, certificate) + Expect(err).ToNot(BeNil()) + Expect(errors.IsNotFound(err)).To(BeTrue()) + }) It("should update the target namespaces in Status", func() { t.expectTargetNamespaces() }) diff --git a/internal/controllers/common/resource_definitions/certificates.go b/internal/controllers/common/resource_definitions/certificates.go index 3d3dd076..e104b4b1 100644 --- a/internal/controllers/common/resource_definitions/certificates.go +++ b/internal/controllers/common/resource_definitions/certificates.go @@ -54,11 +54,11 @@ func NewCryostatCAIssuer(cr *model.CryostatInstance) *certv1.Issuer { } } -func NewCryostatCACert(cr *model.CryostatInstance) *certv1.Certificate { +func NewCryostatCACert(cr *model.CryostatInstance, namespace string) *certv1.Certificate { return &certv1.Certificate{ ObjectMeta: metav1.ObjectMeta{ Name: cr.Name + "-ca", - Namespace: cr.InstallNamespace, + Namespace: namespace, }, Spec: certv1.CertificateSpec{ CommonName: fmt.Sprintf("ca.%s.cert-manager", cr.Name), diff --git a/internal/controllers/reconciler_test.go b/internal/controllers/reconciler_test.go index b02b8d3d..bb3b2967 100644 --- a/internal/controllers/reconciler_test.go +++ b/internal/controllers/reconciler_test.go @@ -1865,7 +1865,7 @@ func (c *controllerTest) commonTests() { }) It("should fail to reconcile", func() { - t.expectAlreadyOwnedError(reconcileErr, "RoleBinding", t.NewRoleBinding(nsInput.Namespace), otherInput) + t.expectAlreadyOwnedError(reconcileErr, "Certificate", t.NewCACert(nsInput.Namespace), otherInput) }) It("should emit a CryostatNameConflict event", func() { @@ -2304,7 +2304,7 @@ func (t *cryostatTestInput) expectWaitingForCertificate() { func (t *cryostatTestInput) expectCertificates() { // Check certificates - certs := []*certv1.Certificate{t.NewCryostatCert(), t.NewCACert(), t.NewReportsCert()} + certs := []*certv1.Certificate{t.NewCryostatCert(), t.NewCACert(t.Namespace), t.NewReportsCert()} if !t.Minimal { certs = append(certs, t.NewGrafanaCert()) } else { @@ -2321,6 +2321,16 @@ func (t *cryostatTestInput) expectCertificates() { t.checkMetadata(actual, expected) Expect(actual.Spec).To(Equal(expected.Spec)) } + // Check for Cryostat CA Certificate in each target namespace + Expect(t.TargetNamespaces).ToNot(BeEmpty()) // Sanity check for tests + for _, ns := range t.TargetNamespaces { + actual := &certv1.Certificate{} + expected := t.NewCACert(ns) + err := t.Client.Get(context.Background(), types.NamespacedName{Name: expected.Name, Namespace: expected.Namespace}, actual) + Expect(err).ToNot(HaveOccurred()) + t.checkMetadata(actual, expected) + Expect(actual.Spec).To(Equal(expected.Spec)) + } // Check issuers as well issuers := []*certv1.Issuer{t.NewSelfSignedIssuer(), t.NewCryostatCAIssuer()} for _, expected := range issuers { diff --git a/internal/test/clients.go b/internal/test/clients.go index 221a414f..95d64c86 100644 --- a/internal/test/clients.go +++ b/internal/test/clients.go @@ -70,7 +70,7 @@ func (c *testClient) makeCertificatesReady(ctx context.Context, obj runtime.Obje // If this object is one of the operator-managed certificates, mock the behaviour // of cert-manager processing those certificates cert, ok := obj.(*certv1.Certificate) - if ok && c.matchesName(cert, c.NewCryostatCert(), c.NewCACert(), c.NewGrafanaCert(), c.NewReportsCert()) && + if ok && c.matchesName(cert, c.NewCryostatCert(), c.NewCACert(c.Namespace), c.NewGrafanaCert(), c.NewReportsCert()) && len(cert.Status.Conditions) == 0 { // Create certificate secret c.createCertSecret(ctx, cert) diff --git a/internal/test/resources.go b/internal/test/resources.go index 79c95726..d1cf75f6 100644 --- a/internal/test/resources.go +++ b/internal/test/resources.go @@ -1071,11 +1071,11 @@ func (r *TestResources) NewReportsCert() *certv1.Certificate { } } -func (r *TestResources) NewCACert() *certv1.Certificate { +func (r *TestResources) NewCACert(ns string) *certv1.Certificate { return &certv1.Certificate{ ObjectMeta: metav1.ObjectMeta{ Name: r.Name + "-ca", - Namespace: r.Namespace, + Namespace: ns, }, Spec: certv1.CertificateSpec{ CommonName: fmt.Sprintf("ca.%s.cert-manager", r.Name), From bfc0aaa5cf9473667b42edb53fe83d6565a162ba Mon Sep 17 00:00:00 2001 From: Ming Wang Date: Thu, 2 Nov 2023 16:32:45 -0400 Subject: [PATCH 2/7] revert changed tests --- internal/controllers/certmanager.go | 2 +- .../controllers/clustercryostat_controller_test.go | 13 ------------- .../common/resource_definitions/certificates.go | 4 ++-- internal/controllers/reconciler_test.go | 14 ++------------ internal/test/clients.go | 2 +- internal/test/resources.go | 4 ++-- 6 files changed, 8 insertions(+), 31 deletions(-) diff --git a/internal/controllers/certmanager.go b/internal/controllers/certmanager.go index 7f86ce51..8a7eef45 100644 --- a/internal/controllers/certmanager.go +++ b/internal/controllers/certmanager.go @@ -56,7 +56,7 @@ func (r *Reconciler) setupTLS(ctx context.Context, cr *model.CryostatInstance) ( } // Create CA certificate for Cryostat using the self-signed issuer - caCert := resources.NewCryostatCACert(cr, cr.InstallNamespace) + caCert := resources.NewCryostatCACert(cr) err = r.createOrUpdateCertificate(ctx, caCert, cr.Object) if err != nil { return nil, err diff --git a/internal/controllers/clustercryostat_controller_test.go b/internal/controllers/clustercryostat_controller_test.go index 2aa989e9..4c93df77 100644 --- a/internal/controllers/clustercryostat_controller_test.go +++ b/internal/controllers/clustercryostat_controller_test.go @@ -66,10 +66,6 @@ var _ = Describe("ClusterCryostatController", func() { t.expectRBAC() }) - It("should create Cryostat CA Certificate in each namespace", func() { - t.expectCertificates() - }) - It("should update the target namespaces in Status", func() { t.expectTargetNamespaces() }) @@ -98,15 +94,6 @@ var _ = Describe("ClusterCryostatController", func() { Expect(err).ToNot(BeNil()) Expect(errors.IsNotFound(err)).To(BeTrue()) }) - It("leave Cryostat CA Certificate for the first namespace", func() { - t.expectCertificates() - }) - It("should remove Cryostat CA Certificate from the second namespace", func() { - certificate := t.NewCACert(targetNamespaces[1]) - err := t.Client.Get(context.Background(), types.NamespacedName{Name: certificate.Name, Namespace: certificate.Namespace}, certificate) - Expect(err).ToNot(BeNil()) - Expect(errors.IsNotFound(err)).To(BeTrue()) - }) It("should update the target namespaces in Status", func() { t.expectTargetNamespaces() }) diff --git a/internal/controllers/common/resource_definitions/certificates.go b/internal/controllers/common/resource_definitions/certificates.go index e104b4b1..3d3dd076 100644 --- a/internal/controllers/common/resource_definitions/certificates.go +++ b/internal/controllers/common/resource_definitions/certificates.go @@ -54,11 +54,11 @@ func NewCryostatCAIssuer(cr *model.CryostatInstance) *certv1.Issuer { } } -func NewCryostatCACert(cr *model.CryostatInstance, namespace string) *certv1.Certificate { +func NewCryostatCACert(cr *model.CryostatInstance) *certv1.Certificate { return &certv1.Certificate{ ObjectMeta: metav1.ObjectMeta{ Name: cr.Name + "-ca", - Namespace: namespace, + Namespace: cr.InstallNamespace, }, Spec: certv1.CertificateSpec{ CommonName: fmt.Sprintf("ca.%s.cert-manager", cr.Name), diff --git a/internal/controllers/reconciler_test.go b/internal/controllers/reconciler_test.go index bb3b2967..b02b8d3d 100644 --- a/internal/controllers/reconciler_test.go +++ b/internal/controllers/reconciler_test.go @@ -1865,7 +1865,7 @@ func (c *controllerTest) commonTests() { }) It("should fail to reconcile", func() { - t.expectAlreadyOwnedError(reconcileErr, "Certificate", t.NewCACert(nsInput.Namespace), otherInput) + t.expectAlreadyOwnedError(reconcileErr, "RoleBinding", t.NewRoleBinding(nsInput.Namespace), otherInput) }) It("should emit a CryostatNameConflict event", func() { @@ -2304,7 +2304,7 @@ func (t *cryostatTestInput) expectWaitingForCertificate() { func (t *cryostatTestInput) expectCertificates() { // Check certificates - certs := []*certv1.Certificate{t.NewCryostatCert(), t.NewCACert(t.Namespace), t.NewReportsCert()} + certs := []*certv1.Certificate{t.NewCryostatCert(), t.NewCACert(), t.NewReportsCert()} if !t.Minimal { certs = append(certs, t.NewGrafanaCert()) } else { @@ -2321,16 +2321,6 @@ func (t *cryostatTestInput) expectCertificates() { t.checkMetadata(actual, expected) Expect(actual.Spec).To(Equal(expected.Spec)) } - // Check for Cryostat CA Certificate in each target namespace - Expect(t.TargetNamespaces).ToNot(BeEmpty()) // Sanity check for tests - for _, ns := range t.TargetNamespaces { - actual := &certv1.Certificate{} - expected := t.NewCACert(ns) - err := t.Client.Get(context.Background(), types.NamespacedName{Name: expected.Name, Namespace: expected.Namespace}, actual) - Expect(err).ToNot(HaveOccurred()) - t.checkMetadata(actual, expected) - Expect(actual.Spec).To(Equal(expected.Spec)) - } // Check issuers as well issuers := []*certv1.Issuer{t.NewSelfSignedIssuer(), t.NewCryostatCAIssuer()} for _, expected := range issuers { diff --git a/internal/test/clients.go b/internal/test/clients.go index 95d64c86..221a414f 100644 --- a/internal/test/clients.go +++ b/internal/test/clients.go @@ -70,7 +70,7 @@ func (c *testClient) makeCertificatesReady(ctx context.Context, obj runtime.Obje // If this object is one of the operator-managed certificates, mock the behaviour // of cert-manager processing those certificates cert, ok := obj.(*certv1.Certificate) - if ok && c.matchesName(cert, c.NewCryostatCert(), c.NewCACert(c.Namespace), c.NewGrafanaCert(), c.NewReportsCert()) && + if ok && c.matchesName(cert, c.NewCryostatCert(), c.NewCACert(), c.NewGrafanaCert(), c.NewReportsCert()) && len(cert.Status.Conditions) == 0 { // Create certificate secret c.createCertSecret(ctx, cert) diff --git a/internal/test/resources.go b/internal/test/resources.go index d1cf75f6..79c95726 100644 --- a/internal/test/resources.go +++ b/internal/test/resources.go @@ -1071,11 +1071,11 @@ func (r *TestResources) NewReportsCert() *certv1.Certificate { } } -func (r *TestResources) NewCACert(ns string) *certv1.Certificate { +func (r *TestResources) NewCACert() *certv1.Certificate { return &certv1.Certificate{ ObjectMeta: metav1.ObjectMeta{ Name: r.Name + "-ca", - Namespace: ns, + Namespace: r.Namespace, }, Spec: certv1.CertificateSpec{ CommonName: fmt.Sprintf("ca.%s.cert-manager", r.Name), From 8ea8eea2e5e195b17347fa8e3e687a014b77c95f Mon Sep 17 00:00:00 2001 From: Ming Wang Date: Mon, 6 Nov 2023 15:28:55 -0500 Subject: [PATCH 3/7] update deletion --- internal/controllers/certmanager.go | 37 +++++++++++++++++++++++++++++ internal/controllers/reconciler.go | 5 ++++ 2 files changed, 42 insertions(+) diff --git a/internal/controllers/certmanager.go b/internal/controllers/certmanager.go index 8a7eef45..1e7213c2 100644 --- a/internal/controllers/certmanager.go +++ b/internal/controllers/certmanager.go @@ -130,6 +130,12 @@ func (r *Reconciler) setupTLS(ctx context.Context, cr *model.CryostatInstance) ( } namespaceSecret := secret.DeepCopy() namespaceSecret.Namespace = ns + err = r.createOrUpdateSecret(ctx, namespaceSecret, cr.Object, func() error { + if secret.StringData == nil { + secret.StringData = map[string]string{} + } + return nil + }) } // Delete any Cryostat CA secrets in target namespaces that are no longer requested for _, ns := range toDelete(cr) { @@ -158,6 +164,37 @@ func (r *Reconciler) setupTLS(ctx context.Context, cr *model.CryostatInstance) ( return tlsConfig, nil } +func (r *Reconciler) finalizeTLS(ctx context.Context, cr *model.CryostatInstance) error { + caCert := resources.NewCryostatCACert(cr) + err := r.createOrUpdateCertificate(ctx, caCert, cr.Object) + if err != nil { + return err + } + for _, ns := range cr.TargetNamespaces { + secret, err := r.GetCertificateSecret(ctx, caCert) + if err != nil { + return err + } + namespaceSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secret.Name, + Namespace: ns, + }, + } + err = r.deleteSecret(ctx, namespaceSecret) + if err != nil { + if kerrors.IsNotFound(err) { + r.Log.Info("secret not found, proceeding with deletion", "name", namespaceSecret.Name, "namespace", namespaceSecret.Namespace) + return nil + } + r.Log.Error(err, "failed to delete secret", "name", namespaceSecret.Name, "namespace", namespaceSecret.Namespace) + return err + } + r.Log.Info("deleted secret", "name", namespaceSecret.Name, "namespace", namespaceSecret.Namespace) + } + return nil +} + func (r *Reconciler) setCertSecretOwner(ctx context.Context, owner metav1.Object, certs ...*certv1.Certificate) error { // Make Cryostat CR controller of secrets created by cert-manager for _, cert := range certs { diff --git a/internal/controllers/reconciler.go b/internal/controllers/reconciler.go index 30b8f4b3..7cff16ef 100644 --- a/internal/controllers/reconciler.go +++ b/internal/controllers/reconciler.go @@ -161,6 +161,11 @@ func (r *Reconciler) reconcile(ctx context.Context, cr *model.CryostatInstance) return reconcile.Result{}, err } + err = r.finalizeTLS(ctx, cr) + if err != nil { + return reconcile.Result{}, err + } + err = common.RemoveFinalizer(ctx, r.Client, cr.Object, cryostatFinalizer) if err != nil { return reconcile.Result{}, err From 6e302105439788a1b21a3787f39ec7cf1448726c Mon Sep 17 00:00:00 2001 From: Ming Wang Date: Wed, 8 Nov 2023 14:23:28 -0500 Subject: [PATCH 4/7] clean up --- internal/controllers/certmanager.go | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/internal/controllers/certmanager.go b/internal/controllers/certmanager.go index 1e7213c2..f5be8429 100644 --- a/internal/controllers/certmanager.go +++ b/internal/controllers/certmanager.go @@ -139,13 +139,9 @@ func (r *Reconciler) setupTLS(ctx context.Context, cr *model.CryostatInstance) ( } // Delete any Cryostat CA secrets in target namespaces that are no longer requested for _, ns := range toDelete(cr) { - secret, err := r.GetCertificateSecret(ctx, caCert) - if err != nil { - return nil, err - } namespaceSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: secret.Name, + Name: caCert.Spec.SecretName, Namespace: ns, }, } @@ -166,22 +162,14 @@ func (r *Reconciler) setupTLS(ctx context.Context, cr *model.CryostatInstance) ( func (r *Reconciler) finalizeTLS(ctx context.Context, cr *model.CryostatInstance) error { caCert := resources.NewCryostatCACert(cr) - err := r.createOrUpdateCertificate(ctx, caCert, cr.Object) - if err != nil { - return err - } for _, ns := range cr.TargetNamespaces { - secret, err := r.GetCertificateSecret(ctx, caCert) - if err != nil { - return err - } namespaceSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: secret.Name, + Name: caCert.Spec.SecretName, Namespace: ns, }, } - err = r.deleteSecret(ctx, namespaceSecret) + err := r.deleteSecret(ctx, namespaceSecret) if err != nil { if kerrors.IsNotFound(err) { r.Log.Info("secret not found, proceeding with deletion", "name", namespaceSecret.Name, "namespace", namespaceSecret.Namespace) From 864a7495b773e911c9c9f12703825c5cedec8f82 Mon Sep 17 00:00:00 2001 From: Ming Wang Date: Thu, 9 Nov 2023 15:02:55 -0500 Subject: [PATCH 5/7] add tests --- internal/controllers/certmanager.go | 74 ++++++++++--------- .../clustercryostat_controller_test.go | 19 ++++- .../controllers/common/finalizer_utils.go | 4 +- internal/controllers/reconciler.go | 14 ++-- internal/controllers/reconciler_test.go | 13 +++- internal/test/resources.go | 12 +++ 6 files changed, 92 insertions(+), 44 deletions(-) diff --git a/internal/controllers/certmanager.go b/internal/controllers/certmanager.go index f5be8429..f2a0bda5 100644 --- a/internal/controllers/certmanager.go +++ b/internal/controllers/certmanager.go @@ -122,32 +122,42 @@ func (r *Reconciler) setupTLS(ctx context.Context, cr *model.CryostatInstance) ( return nil, err } + secret, err := r.GetCertificateSecret(ctx, caCert) + if err != nil { + return nil, err + } // Copy Cryostat CA secret in each target namespace for _, ns := range cr.TargetNamespaces { - secret, err := r.GetCertificateSecret(ctx, caCert) - if err != nil { - return nil, err - } - namespaceSecret := secret.DeepCopy() - namespaceSecret.Namespace = ns - err = r.createOrUpdateSecret(ctx, namespaceSecret, cr.Object, func() error { - if secret.StringData == nil { - secret.StringData = map[string]string{} + if ns != cr.InstallNamespace { + namespaceSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: caCert.Spec.SecretName, + Namespace: ns, + }, + Type: secret.Type, } - return nil - }) + err = r.createOrUpdateSecret(ctx, namespaceSecret, cr.Object, func() error { + namespaceSecret.Data = secret.Data + return nil + }) + if err != nil { + return nil, err + } + } } // Delete any Cryostat CA secrets in target namespaces that are no longer requested for _, ns := range toDelete(cr) { - namespaceSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: caCert.Spec.SecretName, - Namespace: ns, - }, - } - err = r.deleteSecret(ctx, namespaceSecret) - if err != nil { - return nil, err + if ns != cr.InstallNamespace { + namespaceSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: caCert.Spec.SecretName, + Namespace: ns, + }, + } + err = r.deleteSecret(ctx, namespaceSecret) + if err != nil { + return nil, err + } } } @@ -163,22 +173,18 @@ func (r *Reconciler) setupTLS(ctx context.Context, cr *model.CryostatInstance) ( func (r *Reconciler) finalizeTLS(ctx context.Context, cr *model.CryostatInstance) error { caCert := resources.NewCryostatCACert(cr) for _, ns := range cr.TargetNamespaces { - namespaceSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: caCert.Spec.SecretName, - Namespace: ns, - }, - } - err := r.deleteSecret(ctx, namespaceSecret) - if err != nil { - if kerrors.IsNotFound(err) { - r.Log.Info("secret not found, proceeding with deletion", "name", namespaceSecret.Name, "namespace", namespaceSecret.Namespace) - return nil + if ns != cr.InstallNamespace { + namespaceSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: caCert.Spec.SecretName, + Namespace: ns, + }, + } + err := r.deleteSecret(ctx, namespaceSecret) + if err != nil { + return err } - r.Log.Error(err, "failed to delete secret", "name", namespaceSecret.Name, "namespace", namespaceSecret.Namespace) - return err } - r.Log.Info("deleted secret", "name", namespaceSecret.Name, "namespace", namespaceSecret.Namespace) } return nil } diff --git a/internal/controllers/clustercryostat_controller_test.go b/internal/controllers/clustercryostat_controller_test.go index 4c93df77..a5227600 100644 --- a/internal/controllers/clustercryostat_controller_test.go +++ b/internal/controllers/clustercryostat_controller_test.go @@ -20,6 +20,7 @@ import ( "github.com/cryostatio/cryostat-operator/internal/controllers" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" ) @@ -62,6 +63,10 @@ var _ = Describe("ClusterCryostatController", func() { t.expectMainDeployment() }) + It("should create CA Cert secret in each namespace", func() { + t.expectCertificates() + }) + It("should create RBAC in each namespace", func() { t.expectRBAC() }) @@ -80,7 +85,9 @@ var _ = Describe("ClusterCryostatController", func() { *cr.TargetNamespaceStatus = targetNamespaces t.objs = append(t.objs, cr.Object, t.NewRoleBinding(targetNamespaces[0]), - t.NewRoleBinding(targetNamespaces[1])) + t.NewRoleBinding(targetNamespaces[1]), + t.NewCACertSecret(targetNamespaces[0]), + t.NewCACertSecret(targetNamespaces[1])) }) It("should create the expected main deployment", func() { t.expectMainDeployment() @@ -94,6 +101,16 @@ var _ = Describe("ClusterCryostatController", func() { Expect(err).ToNot(BeNil()) Expect(errors.IsNotFound(err)).To(BeTrue()) }) + It("leave CA Cert secret for the first namespace", func() { + t.expectCertificates() + }) + It("should remove CA Cert secret from the second namespace", func() { + caCert := t.NewCACert() + secret := &corev1.Secret{} + err := t.Client.Get(context.Background(), types.NamespacedName{Name: caCert.Name, Namespace: targetNamespaces[1]}, secret) + Expect(err).ToNot(BeNil()) + Expect(errors.IsNotFound(err)).To(BeTrue()) + }) It("should update the target namespaces in Status", func() { t.expectTargetNamespaces() }) diff --git a/internal/controllers/common/finalizer_utils.go b/internal/controllers/common/finalizer_utils.go index dc36890f..02bd1d67 100644 --- a/internal/controllers/common/finalizer_utils.go +++ b/internal/controllers/common/finalizer_utils.go @@ -22,7 +22,7 @@ import ( ) // AddFinalizer adds the provided finalizer to the object and updates it in the cluster -func AddFinalizer(ctx context.Context, client client.Client, obj controllerutil.Object, finalizer string) error { +func AddFinalizer(ctx context.Context, client client.Client, obj client.Object, finalizer string) error { log.Info("adding finalizer to object", "namespace", obj.GetNamespace(), "name", obj.GetName()) controllerutil.AddFinalizer(obj, finalizer) err := client.Update(ctx, obj) @@ -35,7 +35,7 @@ func AddFinalizer(ctx context.Context, client client.Client, obj controllerutil. } // RemoveFinalizer removes the provided finalizer from the object and updates it in the cluster -func RemoveFinalizer(ctx context.Context, client client.Client, obj controllerutil.Object, finalizer string) error { +func RemoveFinalizer(ctx context.Context, client client.Client, obj client.Object, finalizer string) error { log.Info("removing finalizer from object", "namespace", obj.GetNamespace(), "name", obj.GetName()) controllerutil.RemoveFinalizer(obj, finalizer) err := client.Update(ctx, obj) diff --git a/internal/controllers/reconciler.go b/internal/controllers/reconciler.go index 7cff16ef..be5d4605 100644 --- a/internal/controllers/reconciler.go +++ b/internal/controllers/reconciler.go @@ -161,6 +161,7 @@ func (r *Reconciler) reconcile(ctx context.Context, cr *model.CryostatInstance) return reconcile.Result{}, err } + // Finalizer for CA Cert secrets err = r.finalizeTLS(ctx, cr) if err != nil { return reconcile.Result{}, err @@ -201,8 +202,15 @@ func (r *Reconciler) reconcile(ctx context.Context, cr *model.CryostatInstance) return reconcile.Result{}, err } + // Reconcile RBAC resources for Cryostat + err = r.reconcileRBAC(ctx, cr) + if err != nil { + return reconcile.Result{}, err + } + // Set up TLS using cert-manager, if available var tlsConfig *resources.TLSConfig + if r.IsCertManagerEnabled(cr) { tlsConfig, err = r.setupTLS(ctx, cr) if err != nil { @@ -235,12 +243,6 @@ func (r *Reconciler) reconcile(ctx context.Context, cr *model.CryostatInstance) } } - // Reconcile RBAC resources for Cryostat - err = r.reconcileRBAC(ctx, cr) - if err != nil { - return reconcile.Result{}, err - } - serviceSpecs := &resources.ServiceSpecs{ InsightsURL: r.InsightsProxy, } diff --git a/internal/controllers/reconciler_test.go b/internal/controllers/reconciler_test.go index b02b8d3d..c40f6822 100644 --- a/internal/controllers/reconciler_test.go +++ b/internal/controllers/reconciler_test.go @@ -2336,7 +2336,18 @@ func (t *cryostatTestInput) expectCertificates() { err := t.Client.Get(context.Background(), types.NamespacedName{Name: expectedSecret.Name, Namespace: expectedSecret.Namespace}, secret) Expect(err).ToNot(HaveOccurred()) t.checkMetadata(secret, expectedSecret) - Expect(secret.StringData).To(Equal(secret.StringData)) + Expect(secret.StringData).To(Equal(expectedSecret.StringData)) + + // Check CA Cert secrets in each target namespace + Expect(t.TargetNamespaces).ToNot(BeEmpty()) + for _, ns := range t.TargetNamespaces { + namespaceSecret := t.NewCACertSecret(ns) + secret := &corev1.Secret{} + err := t.Client.Get(context.Background(), types.NamespacedName{Name: namespaceSecret.Name, Namespace: ns}, secret) + Expect(err).ToNot(HaveOccurred()) + t.checkMetadata(secret, namespaceSecret) + Expect(secret.Data).To(Equal(namespaceSecret.Data)) + } } func (t *cryostatTestInput) expectRBAC() { diff --git a/internal/test/resources.go b/internal/test/resources.go index 79c95726..6ef0ada2 100644 --- a/internal/test/resources.go +++ b/internal/test/resources.go @@ -880,6 +880,18 @@ func (r *TestResources) NewTestService() *corev1.Service { } } +func (r *TestResources) NewCACertSecret(ns string) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: r.Name + "-ca", + Namespace: ns, + }, + Data: map[string][]byte{ + corev1.TLSCertKey: []byte(r.Name + "-ca-bytes"), + }, + } +} + func (r *TestResources) NewGrafanaSecret() *corev1.Secret { return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ From 64acdbb14ca1eddd856a4903f37c21f4dacfc23c Mon Sep 17 00:00:00 2001 From: Ming Wang Date: Mon, 20 Nov 2023 12:15:05 -0500 Subject: [PATCH 6/7] only copy over TLSkey --- internal/controllers/certmanager.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/controllers/certmanager.go b/internal/controllers/certmanager.go index f2a0bda5..c36429c9 100644 --- a/internal/controllers/certmanager.go +++ b/internal/controllers/certmanager.go @@ -137,7 +137,10 @@ func (r *Reconciler) setupTLS(ctx context.Context, cr *model.CryostatInstance) ( Type: secret.Type, } err = r.createOrUpdateSecret(ctx, namespaceSecret, cr.Object, func() error { - namespaceSecret.Data = secret.Data + if namespaceSecret.Data == nil { + namespaceSecret.Data = map[string][]byte{} + } + namespaceSecret.Data[corev1.TLSCertKey] = secret.Data[corev1.TLSCertKey] return nil }) if err != nil { From 16caf4e785cf973dfd30e879a4efbfde4b2615fb Mon Sep 17 00:00:00 2001 From: Ming Wang Date: Wed, 29 Nov 2023 15:31:10 -0500 Subject: [PATCH 7/7] updates --- internal/controllers/certmanager.go | 2 +- internal/controllers/reconciler_test.go | 15 +++++++++------ internal/test/clients.go | 3 ++- internal/test/resources.go | 1 + 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/internal/controllers/certmanager.go b/internal/controllers/certmanager.go index c36429c9..ae9fa875 100644 --- a/internal/controllers/certmanager.go +++ b/internal/controllers/certmanager.go @@ -134,7 +134,7 @@ func (r *Reconciler) setupTLS(ctx context.Context, cr *model.CryostatInstance) ( Name: caCert.Spec.SecretName, Namespace: ns, }, - Type: secret.Type, + Type: corev1.SecretTypeOpaque, } err = r.createOrUpdateSecret(ctx, namespaceSecret, cr.Object, func() error { if namespaceSecret.Data == nil { diff --git a/internal/controllers/reconciler_test.go b/internal/controllers/reconciler_test.go index c40f6822..156209b2 100644 --- a/internal/controllers/reconciler_test.go +++ b/internal/controllers/reconciler_test.go @@ -2341,12 +2341,15 @@ func (t *cryostatTestInput) expectCertificates() { // Check CA Cert secrets in each target namespace Expect(t.TargetNamespaces).ToNot(BeEmpty()) for _, ns := range t.TargetNamespaces { - namespaceSecret := t.NewCACertSecret(ns) - secret := &corev1.Secret{} - err := t.Client.Get(context.Background(), types.NamespacedName{Name: namespaceSecret.Name, Namespace: ns}, secret) - Expect(err).ToNot(HaveOccurred()) - t.checkMetadata(secret, namespaceSecret) - Expect(secret.Data).To(Equal(namespaceSecret.Data)) + if ns != t.Namespace { + namespaceSecret := t.NewCACertSecret(ns) + secret := &corev1.Secret{} + err := t.Client.Get(context.Background(), types.NamespacedName{Name: namespaceSecret.Name, Namespace: ns}, secret) + Expect(err).ToNot(HaveOccurred()) + t.checkMetadata(secret, namespaceSecret) + Expect(secret.Data).To(Equal(namespaceSecret.Data)) + Expect(secret.Type).To(Equal(namespaceSecret.Type)) + } } } diff --git a/internal/test/clients.go b/internal/test/clients.go index 221a414f..05632c1a 100644 --- a/internal/test/clients.go +++ b/internal/test/clients.go @@ -92,7 +92,8 @@ func (c *testClient) createCertSecret(ctx context.Context, cert *certv1.Certific Namespace: cert.Namespace, }, Data: map[string][]byte{ - corev1.TLSCertKey: []byte(cert.Name + "-bytes"), + corev1.TLSCertKey: []byte(cert.Name + "-bytes"), + corev1.TLSPrivateKeyKey: []byte(cert.Name + "-key"), }, } err := c.Create(ctx, secret) diff --git a/internal/test/resources.go b/internal/test/resources.go index 6ef0ada2..c9dd11e5 100644 --- a/internal/test/resources.go +++ b/internal/test/resources.go @@ -886,6 +886,7 @@ func (r *TestResources) NewCACertSecret(ns string) *corev1.Secret { Name: r.Name + "-ca", Namespace: ns, }, + Type: corev1.SecretTypeOpaque, Data: map[string][]byte{ corev1.TLSCertKey: []byte(r.Name + "-ca-bytes"), },