From 783cdfcbba34ed8d067bf5f52c97d7b6f3b4916e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wilson=20J=C3=BAnior?= Date: Wed, 4 Sep 2024 14:57:33 -0300 Subject: [PATCH] Avoid eventual consistency reading certificates, read first from Rpaasinstance CRD, after that, read from the near real-time resource: nginx CRD --- api/v1alpha1/rpaasinstance_types.go | 10 ++ .../controllers/certificates/cert_manager.go | 13 +- .../certificates/cert_manager_test.go | 6 +- internal/pkg/rpaas/fake/manager.go | 4 +- internal/pkg/rpaas/k8s.go | 122 ++++++++++-------- internal/pkg/rpaas/k8s_test.go | 47 +++---- internal/pkg/rpaas/manager.go | 11 +- 7 files changed, 104 insertions(+), 109 deletions(-) diff --git a/api/v1alpha1/rpaasinstance_types.go b/api/v1alpha1/rpaasinstance_types.go index 73872362..984cc20f 100644 --- a/api/v1alpha1/rpaasinstance_types.go +++ b/api/v1alpha1/rpaasinstance_types.go @@ -5,6 +5,9 @@ package v1alpha1 import ( + "fmt" + "strings" + kedav1alpha1 "github.com/kedacore/keda/v2/apis/keda/v1alpha1" nginxv1alpha1 "github.com/tsuru/nginx-operator/api/v1alpha1" corev1 "k8s.io/api/core/v1" @@ -175,6 +178,13 @@ type CertManager struct { DNSNamesDefault bool `json:"dnsNamesDefault,omitempty"` } +func (r *CertManager) RequiredName() string { + if r.Name != "" { + return r.Name + } + return fmt.Sprintf("cert-manager-%s", strings.ToLower(strings.ReplaceAll(r.Issuer, ".", "-"))) +} + type AllowedUpstream struct { Host string `json:"host,omitempty"` Port int `json:"port,omitempty"` diff --git a/internal/controllers/certificates/cert_manager.go b/internal/controllers/certificates/cert_manager.go index 9f946f48..1d4ca5b7 100644 --- a/internal/controllers/certificates/cert_manager.go +++ b/internal/controllers/certificates/cert_manager.go @@ -200,7 +200,7 @@ func newCertificate(instance *v1alpha1.RpaasInstance, issuer *cmmeta.ObjectRefer labels[k] = v } - labels["rpaas.extensions.tsuru.io/certificate-name"] = cmCertificateName(req) + labels["rpaas.extensions.tsuru.io/certificate-name"] = req.RequiredName() labels["rpaas.extensions.tsuru.io/instance-name"] = instance.Name return &cmv1.Certificate{ @@ -223,7 +223,7 @@ func newCertificate(instance *v1alpha1.RpaasInstance, issuer *cmmeta.ObjectRefer SecretName: CertManagerCertificateNameForInstance(instance.Name, req), SecretTemplate: &cmv1.CertificateSecretTemplate{ Labels: map[string]string{ - "rpaas.extensions.tsuru.io/certificate-name": cmCertificateName(req), + "rpaas.extensions.tsuru.io/certificate-name": req.RequiredName(), "rpaas.extensions.tsuru.io/instance-name": instance.Name, }, }, @@ -321,13 +321,6 @@ func isCertificateReady(cert *cmv1.Certificate) bool { return false } -func cmCertificateName(r v1alpha1.CertManager) string { - if r.Name != "" { - return r.Name - } - return fmt.Sprintf("%s-%s", CertManagerCertificateName, strings.ToLower(strings.ReplaceAll(r.Issuer, ".", "-"))) -} - func CertManagerCertificateNameForInstance(instanceName string, r v1alpha1.CertManager) string { - return fmt.Sprintf("%s-%s", instanceName, cmCertificateName(r)) + return fmt.Sprintf("%s-%s", instanceName, r.RequiredName()) } diff --git a/internal/controllers/certificates/cert_manager_test.go b/internal/controllers/certificates/cert_manager_test.go index 595ced7a..807c0486 100644 --- a/internal/controllers/certificates/cert_manager_test.go +++ b/internal/controllers/certificates/cert_manager_test.go @@ -715,11 +715,15 @@ func Test_CertManagerCertificateName(t *testing.T) { request: v1alpha1.CertManager{Issuer: "my-custom.ClusterIssuer.example.com"}, expected: "cert-manager-my-custom-clusterissuer-example-com", }, + { + request: v1alpha1.CertManager{Issuer: "my-custom.ClusterIssuer.example.com", Name: "cert01"}, + expected: "cert01", + }, } for _, tt := range tests { t.Run(fmt.Sprintf("%v == %q", tt.request, tt.expected), func(t *testing.T) { - got := cmCertificateName(tt.request) + got := tt.request.RequiredName() assert.Equal(t, tt.expected, got) }) } diff --git a/internal/pkg/rpaas/fake/manager.go b/internal/pkg/rpaas/fake/manager.go index 826ab1a8..f4c7738d 100644 --- a/internal/pkg/rpaas/fake/manager.go +++ b/internal/pkg/rpaas/fake/manager.go @@ -20,7 +20,7 @@ var _ rpaas.RpaasManager = &RpaasManager{} type RpaasManager struct { FakeUpdateCertificate func(instance, name string, cert tls.Certificate) error - FakeGetCertificates func(instanceName string) ([]rpaas.CertificateData, []clientTypes.Event, error) + FakeGetCertificates func(instanceName string) ([]clientTypes.CertificateInfo, []clientTypes.Event, error) FakeDeleteCertificate func(instance, name string) error FakeCreateInstance func(args rpaas.CreateArgs) error FakeDeleteInstance func(instanceName string) error @@ -81,7 +81,7 @@ func (m *RpaasManager) GetInstanceInfo(ctx context.Context, instanceName string) return nil, nil } -func (m *RpaasManager) GetCertificates(ctx context.Context, instanceName string) ([]rpaas.CertificateData, []clientTypes.Event, error) { +func (m *RpaasManager) GetCertificates(ctx context.Context, instanceName string) ([]clientTypes.CertificateInfo, []clientTypes.Event, error) { if m.FakeGetCertificates != nil { return m.FakeGetCertificates(instanceName) } diff --git a/internal/pkg/rpaas/k8s.go b/internal/pkg/rpaas/k8s.go index bdcef602..43e8140f 100644 --- a/internal/pkg/rpaas/k8s.go +++ b/internal/pkg/rpaas/k8s.go @@ -636,49 +636,84 @@ func (m *k8sRpaasManager) Stop(ctx context.Context, instanceName string) error { return m.patchInstance(ctx, originalInstance, instance) } -func (m *k8sRpaasManager) GetCertificates(ctx context.Context, instanceName string) ([]CertificateData, []clientTypes.Event, error) { +func (m *k8sRpaasManager) GetCertificates(ctx context.Context, instanceName string) ([]clientTypes.CertificateInfo, []clientTypes.Event, error) { instance, err := m.GetInstance(ctx, instanceName) if err != nil { return nil, nil, err } + secretList := &corev1.SecretList{} + if err = m.cli.List(ctx, secretList, &client.ListOptions{ + Namespace: instance.Namespace, + LabelSelector: labels.SelectorFromSet(map[string]string{ + "rpaas.extensions.tsuru.io/instance-name": instance.Name, + }), + }); err != nil { + return nil, nil, err + } + secretMap := make(map[string]corev1.Secret) + for _, secret := range secretList.Items { + secretMap[secret.Name] = secret + } + nginx, err := m.getNginx(ctx, instance) - if err != nil && k8sErrors.IsNotFound(err) { - return nil, nil, nil + if err != nil { + if !k8sErrors.IsNotFound(err) { + return nil, nil, err + } + nginx = &nginxv1alpha1.Nginx{} } - var certList []CertificateData - for _, tls := range nginx.Spec.TLS { - var s corev1.Secret - if err = m.cli.Get(ctx, types.NamespacedName{Name: tls.SecretName, Namespace: instance.Namespace}, &s); err != nil { + var certMap = make(map[string]clientTypes.CertificateInfo) + allTLS := append([]nginxv1alpha1.NginxTLS{}, instance.Spec.TLS...) + allTLS = append(allTLS, nginx.Spec.TLS...) + + for _, tls := range allTLS { + s, ok := secretMap[tls.SecretName] + if !ok { + continue + } + + x509Certs, err := pki.DecodeX509CertificateChainBytes(s.Data[corev1.TLSCertKey]) + if err != nil { return nil, nil, err } - certData := CertificateData{ - Name: s.Labels[certificates.CertificateNameLabel], - Certificate: string(s.Data[corev1.TLSCertKey]), - Key: string(s.Data[corev1.TLSPrivateKeyKey]), + if len(x509Certs) == 0 { + return nil, nil, fmt.Errorf("no certificates found in pem file") + } + + leaf := x509Certs[0] + + certInfo := clientTypes.CertificateInfo{ + Name: s.Labels[certificates.CertificateNameLabel], + DNSNames: leaf.DNSNames, + ValidFrom: leaf.NotBefore, + ValidUntil: leaf.NotAfter, + PublicKeyAlgorithm: leaf.PublicKeyAlgorithm.String(), + PublicKeyBitSize: publicKeySize(leaf.PublicKey), } issuerGroup := s.Annotations["cert-manager.io/issuer-group"] if issuerGroup == "cert-manager.io" { - certData.CertManagerIssuer = s.Annotations["cert-manager.io/issuer-name"] - certData.IsManagedByCertManager = true + certInfo.CertManagerIssuer = s.Annotations["cert-manager.io/issuer-name"] + certInfo.IsManagedByCertManager = true } else if issuerGroup != "" { - certData.IsManagedByCertManager = true - certData.CertManagerIssuer = fmt.Sprintf("%s.%s.%s", + certInfo.IsManagedByCertManager = true + certInfo.CertManagerIssuer = fmt.Sprintf("%s.%s.%s", s.Annotations["cert-manager.io/issuer-name"], s.Annotations["cert-manager.io/issuer-kind"], issuerGroup, ) } - certList = append(certList, certData) + certMap[certInfo.Name] = certInfo } events := make([]clientTypes.Event, 0) for _, certManagerRequest := range instance.CertManagerRequests() { + certName := certManagerRequest.RequiredName() certManagerCertificateName := certificates.CertManagerCertificateNameForInstance(instance.Name, certManagerRequest) certificateEvents, err := m.eventsForObjectName(ctx, instance.Namespace, "Certificate", certManagerCertificateName) if err != nil { @@ -692,16 +727,30 @@ func (m *k8sRpaasManager) GetCertificates(ctx context.Context, instanceName stri Count: evt.Count, Type: evt.Type, Reason: evt.Reason, - Message: fmt.Sprintf("certificate %q, %s", certManagerCertificateName, evt.Message), + Message: fmt.Sprintf("certificate %q, %s", certName, evt.Message), }) } + if _, ok := certMap[certName]; !ok { + certMap[certName] = clientTypes.CertificateInfo{ + Name: certName, + DNSNames: certManagerRequest.DNSNames, + IsManagedByCertManager: true, + CertManagerIssuer: certManagerRequest.Issuer, + } + } + } sort.SliceStable(events, func(i, j int) bool { return events[i].Last.Before(events[j].Last) // ascending order by last event occurrence }) + var certList []clientTypes.CertificateInfo + for _, cert := range certMap { + certList = append(certList, cert) + } + sort.Slice(certList, func(i, j int) bool { return certList[i].Name < certList[j].Name }) @@ -749,7 +798,7 @@ func (m *k8sRpaasManager) UpdateCertificate(ctx context.Context, instanceName, n return err } - certsInfo, _, err := m.getCertificatesInfo(ctx, instance) + certsInfo, _, err := m.GetCertificates(ctx, instance.Name) if err != nil { return err } @@ -1673,7 +1722,7 @@ func (m *k8sRpaasManager) GetInstanceInfo(ctx context.Context, instanceName stri } var certificateEvents []clientTypes.Event - info.Certificates, certificateEvents, err = m.getCertificatesInfo(ctx, instance) + info.Certificates, certificateEvents, err = m.GetCertificates(ctx, instance.Name) if err != nil { return nil, err } @@ -2106,41 +2155,6 @@ func (m *k8sRpaasManager) newPodStatus(ctx context.Context, pod *corev1.Pod) (cl }, nil } -func (m *k8sRpaasManager) getCertificatesInfo(ctx context.Context, instance *v1alpha1.RpaasInstance) ([]clientTypes.CertificateInfo, []clientTypes.Event, error) { - certs, events, err := m.GetCertificates(ctx, instance.Name) - if err != nil { - return nil, nil, err - } - - var certsInfo []clientTypes.CertificateInfo - for _, cert := range certs { - certs, err := pki.DecodeX509CertificateChainBytes([]byte(cert.Certificate)) - if err != nil { - return nil, nil, err - } - - if len(certs) == 0 { - return nil, nil, fmt.Errorf("no certificates found in pem file") - } - - leaf := certs[0] - - certsInfo = append(certsInfo, clientTypes.CertificateInfo{ - Name: cert.Name, - DNSNames: leaf.DNSNames, - ValidFrom: leaf.NotBefore, - ValidUntil: leaf.NotAfter, - PublicKeyAlgorithm: leaf.PublicKeyAlgorithm.String(), - PublicKeyBitSize: publicKeySize(leaf.PublicKey), - - IsManagedByCertManager: cert.IsManagedByCertManager, - CertManagerIssuer: cert.CertManagerIssuer, - }) - } - - return certsInfo, events, nil -} - func getPortsForPod(pod *corev1.Pod) []clientTypes.PodPort { var ports []clientTypes.PodPort for _, container := range pod.Spec.Containers { diff --git a/internal/pkg/rpaas/k8s_test.go b/internal/pkg/rpaas/k8s_test.go index 985c7e40..dfeac26d 100644 --- a/internal/pkg/rpaas/k8s_test.go +++ b/internal/pkg/rpaas/k8s_test.go @@ -352,7 +352,8 @@ func Test_k8sRpaasManager_GetCertificates(t *testing.T) { Name: "my-instance-certs-abc123", Namespace: "rpaasv2", Labels: map[string]string{ - certificates.CertificateNameLabel: "default", + certificates.CertificateNameLabel: "default", + "rpaas.extensions.tsuru.io/instance-name": "my-instance-1", }, }, Data: map[string][]byte{ @@ -428,11 +429,14 @@ sM5FaDCEIJVbWjPDluxUGbVOQlFHsJs+pZv0Anf9DPwU }, } + notBefore, _ := time.Parse("Jan 2 15:04:05 2006 MST", "Mar 26 20:21:39 2019 GMT") + notAfter, _ := time.Parse("Jan 2 15:04:05 2006 MST", "Mar 25 20:21:39 2020 GMT") + tests := map[string]struct { name string instanceName string expectedError string - expected []CertificateData + expected []clientTypes.CertificateInfo }{ "instance not found": { instanceName: "instance-not-found", @@ -441,37 +445,16 @@ sM5FaDCEIJVbWjPDluxUGbVOQlFHsJs+pZv0Anf9DPwU "getting an existing certificate": { instanceName: "my-instance-1", - expected: []CertificateData{ + expected: []clientTypes.CertificateInfo{ { - Name: "default", - Certificate: `-----BEGIN CERTIFICATE----- -MIIB9TCCAV6gAwIBAgIRAIpoagB8BUn8x36iyvafmC0wDQYJKoZIhvcNAQELBQAw -EjEQMA4GA1UEChMHQWNtZSBDbzAeFw0xOTAzMjYyMDIxMzlaFw0yMDAzMjUyMDIx -MzlaMBIxEDAOBgNVBAoTB0FjbWUgQ28wgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJ -AoGBAOIsM9LhHqI3oBhHDCGZkGKgiI72ghnLr5UpaA3I9U7np/LPzt/JpWRG4wjF -5Var2IRPGoNwLcdybFW0YTqvw1wNY88q9BcpwS5PeV7uWyZqWafdSxxveaG6VeCH -YFMqopOKri4kJ4sZB9WS3xMlGZXK6zHPwA4xPtuVEND+LI17AgMBAAGjSzBJMA4G -A1UdDwEB/wQEAwIFoDATBgNVHSUEDDAKBggrBgEFBQcDATAMBgNVHRMBAf8EAjAA -MBQGA1UdEQQNMAuCCWxvY2FsaG9zdDANBgkqhkiG9w0BAQsFAAOBgQCaF9zDYoPh -4KmqxFI3KB+cl8Z/0y0txxH4vqlnByBBiCLpPzivcCRFlT1bGPVJOLsyd/BdOset -yTcvMUPbnEPXZMR4Dsbzzjco1JxMSvZgkhm85gAlwNGjFZrMXqO8G5R/gpWN3UUc -7likRQOu7q61DlicQAZXRnOh6BbKaq1clg== ------END CERTIFICATE-----`, - Key: `-----BEGIN RSA PRIVATE KEY----- -MIICXQIBAAKBgQDiLDPS4R6iN6AYRwwhmZBioIiO9oIZy6+VKWgNyPVO56fyz87f -yaVkRuMIxeVWq9iETxqDcC3HcmxVtGE6r8NcDWPPKvQXKcEuT3le7lsmalmn3Usc -b3mhulXgh2BTKqKTiq4uJCeLGQfVkt8TJRmVyusxz8AOMT7blRDQ/iyNewIDAQAB -AoGBAI05gJqayyALj8HZCzAnzUpoZxytvAsTbm27TyfcZaCBchNhwxFlvgphYP5n -Y468+xOSuUF9WHiDcDYLzfJxMZAqmuS+D/IREYDkcrGVT1MXfSCkNaFVqG52+hLZ -GmGsy8+KsJnDJ1HYmwfSnaTj3L8+Bf2Hg291Yb1caRH9+5vBAkEA7P5N3cSN73Fa -HwaWzqkaY75mCR4TpRi27YWGA3wdQek2G71HiSbCOxrWOymvgoNRi6M/sdrP5PTt -JAFxC+pd8QJBAPRPvS0Tm/0lMIZ0q7jxyoW/gKDzokmSszopdlvSU53lN06vaYdK -XyTvqOO95nJx0DjkdM26QojJlSueMTitJisCQDuxNfWku0dTGqrz4uo8p5v16gdj -3vjXh8O9vOqFyWy/i9Ri0XDXJVbzxH/0WPObld+BB9sJTRHTKyPFhS7GIlECQDZ8 -chxTez6BxMi3zHR6uEgL5Yv/yfnOldoq1RK1XaChNix+QnLBy2ZZbLkd6P8tEtsd -WE9pct0+193ace/J7fECQQDAhwHBpJjhM+k97D92akneKXIUBo+Egr5E5qF9/g5I -sM5FaDCEIJVbWjPDluxUGbVOQlFHsJs+pZv0Anf9DPwU ------END RSA PRIVATE KEY-----`, + Name: "default", + PublicKeyAlgorithm: "RSA", + PublicKeyBitSize: 1024, + ValidFrom: notBefore.UTC(), + ValidUntil: notAfter.UTC(), + DNSNames: []string{ + "localhost", + }, }, }, }, diff --git a/internal/pkg/rpaas/manager.go b/internal/pkg/rpaas/manager.go index a9d7e27a..da5cd5e6 100644 --- a/internal/pkg/rpaas/manager.go +++ b/internal/pkg/rpaas/manager.go @@ -256,7 +256,7 @@ type RpaasManager interface { UpdateCertificate(ctx context.Context, instance, name string, cert tls.Certificate) error DeleteCertificate(ctx context.Context, instance, name string) error - GetCertificates(ctx context.Context, instanceName string) ([]CertificateData, []clientTypes.Event, error) + GetCertificates(ctx context.Context, instanceName string) ([]clientTypes.CertificateInfo, []clientTypes.Event, error) CreateInstance(ctx context.Context, args CreateArgs) error DeleteInstance(ctx context.Context, name string) error UpdateInstance(ctx context.Context, name string, args UpdateInstanceArgs) error @@ -290,15 +290,6 @@ type RpaasManager interface { UnsetMetadata(ctx context.Context, instanceName string, metadata *clientTypes.Metadata) error } -type CertificateData struct { - Name string `json:"name"` - Certificate string `json:"certificate"` - Key string `json:"key"` - - IsManagedByCertManager bool `json:"isManagedByCertManager,omitempty"` - CertManagerIssuer string `json:"certManagerIssuer,omitempty"` -} - func getAnnotations(params map[string]interface{}) (map[string]string, error) { p, found := params["annotations"] if !found {