From 2fe6a899de80d97355917c82cee49ad28630955e Mon Sep 17 00:00:00 2001 From: Martin Schuppert Date: Thu, 5 Sep 2024 09:59:05 +0200 Subject: [PATCH] [tls] ValidateCertSecret to not return ctrlResult ValidateCertSecrets miss information on which cert secret is missing. Lets not return a crtlResult in case of a cert secret is missing, instead return a NotFound error and let the caller decide what to do. Jira: OSPRH-9991 Signed-off-by: Martin Schuppert --- modules/common/test/functional/tls_test.go | 25 +++------ modules/common/tls/tls.go | 62 +++++++++++----------- 2 files changed, 39 insertions(+), 48 deletions(-) diff --git a/modules/common/test/functional/tls_test.go b/modules/common/test/functional/tls_test.go index ff849c39..313b43c9 100644 --- a/modules/common/test/functional/tls_test.go +++ b/modules/common/test/functional/tls_test.go @@ -22,7 +22,6 @@ import ( "github.com/openstack-k8s-operators/lib-common/modules/common/service" "github.com/openstack-k8s-operators/lib-common/modules/common/tls" "k8s.io/apimachinery/pkg/types" - ctrl "sigs.k8s.io/controller-runtime" ) var _ = Describe("tls package", func() { @@ -48,15 +47,13 @@ var _ = Describe("tls package", func() { th.CreateEmptySecret(sname) // validate bad ca cert secret - _, ctrlResult, err := tls.ValidateCACertSecret(th.Ctx, cClient, sname) + _, err := tls.ValidateCACertSecret(th.Ctx, cClient, sname) Expect(err).To(HaveOccurred()) - Expect(ctrlResult).To(BeIdenticalTo(ctrl.Result{})) // update ca cert secret with good data th.UpdateSecret(sname, tls.CABundleKey, []byte("foo")) - hash, ctrlResult, err := tls.ValidateCACertSecret(th.Ctx, cClient, sname) + hash, err := tls.ValidateCACertSecret(th.Ctx, cClient, sname) Expect(err).ShouldNot(HaveOccurred()) - Expect(ctrlResult).To(BeIdenticalTo(ctrl.Result{})) Expect(hash).To(BeIdenticalTo("n56fh645hfbh687hc9h678h87h64bh598h577hch5d6h5c9h5d4h74h84h5f4hfch6dh678h547h9bhbchb6h89h5c4h68dhc9h664h557h595h5c5q")) }) @@ -73,24 +70,21 @@ var _ = Describe("tls package", func() { s := &tls.Service{ SecretName: sname.Name, } - _, ctrlResult, err := s.ValidateCertSecret(th.Ctx, h, namespace) + _, err := s.ValidateCertSecret(th.Ctx, h, namespace) Expect(err).To(HaveOccurred()) - Expect(ctrlResult).To(BeIdenticalTo(ctrl.Result{})) // update cert secret with cert, still key missing th.UpdateSecret(sname, tls.CertKey, []byte("cert")) - _, ctrlResult, err = s.ValidateCertSecret(th.Ctx, h, namespace) + _, err = s.ValidateCertSecret(th.Ctx, h, namespace) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("field tls.key not found in Secret")) - Expect(ctrlResult).To(BeIdenticalTo(ctrl.Result{})) // update cert secret with key to be a good cert secret th.UpdateSecret(sname, tls.PrivateKey, []byte("key")) // validate good cert secret - hash, ctrlResult, err := s.ValidateCertSecret(th.Ctx, h, namespace) + hash, err := s.ValidateCertSecret(th.Ctx, h, namespace) Expect(err).ShouldNot(HaveOccurred()) - Expect(ctrlResult).To(BeIdenticalTo(ctrl.Result{})) Expect(hash).To(BeIdenticalTo("n547h97h5cfh587h56ch594h79hd4h96h5cfh565h587h569h688h666h685h67ch7fhfbh664h5f9h694h564h9ch645h675h665h78h7h87h566hb6q")) }) @@ -107,9 +101,8 @@ var _ = Describe("tls package", func() { endpointCfgs := map[service.Endpoint]tls.Service{} // validate empty service map - _, ctrlResult, err := tls.ValidateEndpointCerts(th.Ctx, h, namespace, endpointCfgs) + _, err := tls.ValidateEndpointCerts(th.Ctx, h, namespace, endpointCfgs) Expect(err).ToNot(HaveOccurred()) - Expect(ctrlResult).To(BeIdenticalTo(ctrl.Result{})) endpointCfgs[service.EndpointInternal] = tls.Service{ SecretName: sname.Name, @@ -119,18 +112,16 @@ var _ = Describe("tls package", func() { } // validate service map with bad cert secret - _, ctrlResult, err = tls.ValidateEndpointCerts(th.Ctx, h, namespace, endpointCfgs) + _, err = tls.ValidateEndpointCerts(th.Ctx, h, namespace, endpointCfgs) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("field tls.crt not found in Secret")) - Expect(ctrlResult).To(BeIdenticalTo(ctrl.Result{})) // update cert secret to have missing private key th.UpdateSecret(sname, tls.CertKey, []byte("cert")) // validate service map with good cert secret - hash, ctrlResult, err := tls.ValidateEndpointCerts(th.Ctx, h, namespace, endpointCfgs) + hash, err := tls.ValidateEndpointCerts(th.Ctx, h, namespace, endpointCfgs) Expect(err).ShouldNot(HaveOccurred()) - Expect(ctrlResult).To(BeIdenticalTo(ctrl.Result{})) Expect(hash).To(BeIdenticalTo("n5d7h65dh5d5h569hffh66ch568h95h686h58fhcfh586h5b8hc6hd7h65bh56bh55bh656hfh5f7h84h54bh65dh5c9h8ch64bh64bhdfh8ch589h54bq")) }) }) diff --git a/modules/common/tls/tls.go b/modules/common/tls/tls.go index 9d56f59f..d5a6fc18 100644 --- a/modules/common/tls/tls.go +++ b/modules/common/tls/tls.go @@ -29,12 +29,13 @@ import ( "github.com/openstack-k8s-operators/lib-common/modules/common/secret" "github.com/openstack-k8s-operators/lib-common/modules/common/service" "github.com/openstack-k8s-operators/lib-common/modules/common/util" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + k8s_errors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/reconcile" ) const ( @@ -167,7 +168,7 @@ func (a *APIService) ValidateCertSecrets( ctx context.Context, h *helper.Helper, namespace string, -) (string, ctrl.Result, error) { +) (string, error) { var svc GenericService certHashes := map[string]env.Setter{} for _, endpt := range []service.Endpoint{service.EndpointInternal, service.EndpointPublic} { @@ -187,20 +188,18 @@ func (a *APIService) ValidateCertSecrets( svc = a.Internal } - hash, ctrlResult, err := svc.ValidateCertSecret(ctx, h, namespace) + hash, err := svc.ValidateCertSecret(ctx, h, namespace) if err != nil { - return "", ctrlResult, err - } else if (ctrlResult != ctrl.Result{}) { - return "", ctrlResult, nil + return "", err } certHashes["cert-"+endpt.String()] = env.SetValue(hash) } certsHash, err := util.HashOfInputHashes(certHashes) if err != nil { - return "", ctrl.Result{}, err + return "", err } - return certsHash, ctrl.Result{}, nil + return certsHash, nil } // ToService - convert tls.APIService to tls.Service @@ -225,26 +224,23 @@ func (s *GenericService) ValidateCertSecret( ctx context.Context, h *helper.Helper, namespace string, -) (string, ctrl.Result, error) { +) (string, error) { hash := "" endptTLSCfg, err := s.ToService() if err != nil { - return "", ctrl.Result{}, err + return "", err } if endptTLSCfg.SecretName != "" { // validate the cert secret has the expected keys - var ctrlResult reconcile.Result - hash, ctrlResult, err = endptTLSCfg.ValidateCertSecret(ctx, h, namespace) + hash, err = endptTLSCfg.ValidateCertSecret(ctx, h, namespace) if err != nil { - return "", ctrlResult, err - } else if (ctrlResult != ctrl.Result{}) { - return "", ctrlResult, nil + return "", err } } - return hash, ctrl.Result{}, nil + return hash, nil } // ValidateCACertSecret - validates the content of the cert secret to make sure "tls-ca-bundle.pem" key exists @@ -252,7 +248,7 @@ func ValidateCACertSecret( ctx context.Context, c client.Client, caSecret types.NamespacedName, -) (string, ctrl.Result, error) { +) (string, error) { hash, ctrlResult, err := secret.VerifySecret( ctx, caSecret, @@ -260,16 +256,19 @@ func ValidateCACertSecret( c, 5*time.Second) if err != nil { - return "", ctrlResult, err + return "", err } else if (ctrlResult != ctrl.Result{}) { - return "", ctrlResult, nil + return "", k8s_errors.NewNotFound( + appsv1.Resource("Secret"), + fmt.Sprintf("secret %s not found in namespace %s", caSecret.Name, caSecret.Namespace), + ) } - return hash, ctrl.Result{}, nil + return hash, nil } // ValidateCertSecret - validates the content of the cert secret to make sure "tls.key", "tls.crt" and optional "ca.crt" keys exist -func (s *Service) ValidateCertSecret(ctx context.Context, h *helper.Helper, namespace string) (string, ctrl.Result, error) { +func (s *Service) ValidateCertSecret(ctx context.Context, h *helper.Helper, namespace string) (string, error) { // define keys to expect in cert secret keys := []string{PrivateKey, CertKey} if s.CaMount != nil { @@ -283,12 +282,15 @@ func (s *Service) ValidateCertSecret(ctx context.Context, h *helper.Helper, name h.GetClient(), 5*time.Second) if err != nil { - return "", ctrlResult, err + return "", err } else if (ctrlResult != ctrl.Result{}) { - return "", ctrlResult, nil + return "", k8s_errors.NewNotFound( + corev1.Resource("Secret"), + fmt.Sprintf("secret %s not found in namespace %s", s.SecretName, namespace), + ) } - return hash, ctrl.Result{}, nil + return hash, nil } // ValidateEndpointCerts - validates all services from an endpointCfgs and @@ -298,16 +300,14 @@ func ValidateEndpointCerts( h *helper.Helper, namespace string, endpointCfgs map[service.Endpoint]Service, -) (string, ctrl.Result, error) { +) (string, error) { certHashes := map[string]env.Setter{} for endpt, endpointTLSCfg := range endpointCfgs { if endpointTLSCfg.SecretName != "" { // validate the cert secret has the expected keys - hash, ctrlResult, err := endpointTLSCfg.ValidateCertSecret(ctx, h, namespace) + hash, err := endpointTLSCfg.ValidateCertSecret(ctx, h, namespace) if err != nil { - return "", ctrlResult, err - } else if (ctrlResult != ctrl.Result{}) { - return "", ctrlResult, nil + return "", err } certHashes["cert-"+endpt.String()] = env.SetValue(hash) @@ -316,9 +316,9 @@ func ValidateEndpointCerts( certsHash, err := util.HashOfInputHashes(certHashes) if err != nil { - return "", ctrl.Result{}, err + return "", err } - return certsHash, ctrl.Result{}, nil + return certsHash, nil } // getCertMountPath - return certificate mount path