Merge pull request #1042 from Deydra71/unused-cert-clenaup

Cleanup unused service certs when using apiOverride

config/samples/tls/custom_route_cert/kustomization.yaml

@@ -0,0 +1,14 @@
+resources:
+- ../../base/openstackcontrolplane
+
+patches:
+- target:
+    kind: OpenStackControlPlane
+    name: .*
+  patch: |-
+    - op: replace
+      path: /metadata/name
+      value: openstack
+- target:
+    kind: OpenStackControlPlane
+  path: patch.yaml

config/samples/tls/custom_route_cert/patch.yaml

@@ -0,0 +1,21 @@
+apiVersion: core.openstack.org/v1beta1
+kind: OpenStackControlPlane
+metadata:
+  name: openstack
+spec:
+  barbican:
+    apiOverride:
+      tls:
+        secretName: barbican-custom-route
+  placement:
+    apiOverride:
+      route:
+        spec:
+          tls:
+            certificate: |
+              CERT123
+            key: |
+              KEY123
+            caCertificate: |
+              CACERT123
+            termination: reencrypt

pkg/openstack/common.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"time"
 
+	certmgrv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
 	"github.com/go-logr/logr"
 	routev1 "github.com/openshift/api/route/v1"
 	barbicanv1 "github.com/openstack-k8s-operators/barbican-operator/api/v1beta1"
@@ -131,7 +132,8 @@ type EndpointDetail struct {
 
 // ServiceTLSDetails - tls settings for the endpoint
 type ServiceTLSDetails struct {
-	Enabled bool
+	Enabled  bool
+	CertName string
 	tls.GenericService
 	tls.Ca
 }
@@ -155,6 +157,7 @@ type RouteDetails struct {
 type RouteTLSDetails struct {
 	Enabled    bool
 	SecretName *string
+	CertName   string
 	IssuerName string
 	tls.Ca
 }
@@ -218,12 +221,12 @@ func EnsureEndpointConfig(
 		}
 
 		ed.Service.OverrideSpec = svcOverrides[ed.Type]
-
 		// TLS on the pod level is enabled if
 		// * TLS is enabled for pod level
 		// * the particular service has not TLS.Disabled set to true
 		if instance.Spec.TLS.PodLevel.Enabled && !serviceTLSDisabled {
 			ed.Service.TLS.Enabled = true
+			ed.Service.TLS.CertName = fmt.Sprintf("%s-svc", ed.Name)
 		} else {
 			ed.Service.TLS.Enabled = false
 		}
@@ -242,6 +245,7 @@ func EnsureEndpointConfig(
 			if instance.Spec.TLS.Ingress.Enabled {
 				// TLS for route enabled if public endpoint TLS is true
 				ed.Route.TLS.Enabled = true
+				ed.Route.TLS.CertName = fmt.Sprintf("%s-route", ed.Name)
 
 				// if a custom cert secret was provided we'll use this for
 				// the route, otherwise the issuer is used to request one
@@ -278,7 +282,6 @@ func EnsureEndpointConfig(
 
 			if ed.Service.TLS.Enabled {
 				ed.Service.TLS.CaBundleSecretName = tls.CABundleSecret
-
 				// if a custom cert secret was provided and ed.Route.Create == false
 				// we'll use this for the service, otherwise issue a cert. This is for
 				// use case where you deploy without ingress/routes and also use
@@ -292,11 +295,25 @@ func EnsureEndpointConfig(
 						}
 						return endpoints, ctrl.Result{}, err
 					}
+					// Delete the issued certificate if it exists
+					cert := certmanager.NewCertificate(
+						&certmgrv1.Certificate{
+							ObjectMeta: metav1.ObjectMeta{
+								Name:      ed.Route.TLS.CertName,
+								Namespace: ed.Namespace,
+							},
+						},
+						5*time.Second,
+					)
+					err = cert.Delete(ctx, helper)
+					if err != nil {
+						return endpoints, ctrl.Result{}, err
+					}
 				} else {
 					// issue a certificate for public pod virthost
 					certRequest := certmanager.CertificateRequest{
 						IssuerName: instance.GetPublicIssuer(),
-						CertName:   fmt.Sprintf("%s-svc", ed.Name),
+						CertName:   ed.Service.TLS.CertName,
 						Hostnames: []string{
 							fmt.Sprintf("%s.%s.svc", ed.Name, instance.Namespace),
 							fmt.Sprintf("%s.%s.svc.%s", ed.Name, instance.Namespace, ClusterInternalDomain),
@@ -346,7 +363,7 @@ func EnsureEndpointConfig(
 				// request certificate
 				certRequest := certmanager.CertificateRequest{
 					IssuerName: instance.GetInternalIssuer(),
-					CertName:   fmt.Sprintf("%s-svc", ed.Name),
+					CertName:   ed.Service.TLS.CertName,
 					Hostnames: []string{
 						fmt.Sprintf("%s.%s.svc", ed.Name, instance.Namespace),
 						fmt.Sprintf("%s.%s.svc.%s", ed.Name, instance.Namespace, ClusterInternalDomain),
@@ -572,10 +589,12 @@ func (ed *EndpointDetail) CreateRoute(
 					}
 				}
 			}
-		} else {
+		}
+
+		if ed.Route.TLS.SecretName == nil && !hasCertInOverrideSpec(ed.Route.OverrideSpec) {
 			certRequest := certmanager.CertificateRequest{
 				IssuerName:  ed.Route.TLS.IssuerName,
-				CertName:    fmt.Sprintf("%s-route", ed.Name),
+				CertName:    ed.Route.TLS.CertName,
 				Hostnames:   []string{*ed.Hostname},
 				Ips:         nil,
 				Annotations: ed.Annotations,
@@ -588,7 +607,7 @@ func (ed *EndpointDetail) CreateRoute(
 			if instance.Spec.TLS.Ingress.Cert.RenewBefore != nil {
 				certRequest.RenewBefore = &instance.Spec.TLS.Ingress.Cert.RenewBefore.Duration
 			}
-			//create the cert using our issuer for the endpoint
+			//create the cert using default issuer for the endpoint
 			certSecret, ctrlResult, err = certmanager.EnsureCert(
 				ctx,
 				helper,
@@ -600,7 +619,6 @@ func (ed *EndpointDetail) CreateRoute(
 				return ctrlResult, nil
 			}
 		}
-
 		// create default TLS route override
 		tlsConfig := &routev1.TLSConfig{
 			Termination:                   routev1.TLSTerminationEdge,
@@ -611,7 +629,7 @@ func (ed *EndpointDetail) CreateRoute(
 		}
 
 		// for internal TLS (TLSE) use routev1.TLSTerminationReencrypt
-		if ed.Service.TLS.Enabled && ed.Service.TLS.SecretName != nil {
+		if ed.Service.TLS.Enabled && (ed.Service.TLS.SecretName != nil || hasCertInOverrideSpec(ed.Route.OverrideSpec)) {
 			// get the TLSInternalCABundleFile to add it to the route
 			// to be able to validate public/internal service endpoints
 			tlsConfig.DestinationCACertificate, ctrlResult, err = secret.GetDataFromSecret(
@@ -648,6 +666,22 @@ func (ed *EndpointDetail) CreateRoute(
 			return ctrlResult, nil
 		}
 
+		// Delete the issued certificate if it exists and custom cert secret or direct TLS data was provided
+		if ed.Route.TLS.SecretName != nil || hasCertInOverrideSpec(ed.Route.OverrideSpec) {
+			cert := certmanager.NewCertificate(
+				&certmgrv1.Certificate{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      ed.Route.TLS.CertName,
+						Namespace: ed.Namespace,
+					},
+				},
+				5*time.Second,
+			)
+			err := cert.Delete(ctx, helper)
+			if err != nil {
+				return ctrl.Result{}, err
+			}
+		}
 		ed.Proto = service.ProtocolHTTPS
 	} else {
 		ed.Proto = service.ProtocolHTTP
@@ -750,3 +784,15 @@ func GetIssuerCertSecret(
 	}
 	return issuer.Spec.CA.SecretName, nil
 }
+
+func hasCertInOverrideSpec(overrideSpec route.OverrideSpec) bool {
+	if overrideSpec.Spec == nil {
+		return false
+	}
+	if overrideSpec.Spec.TLS == nil {
+		return false
+	}
+	return overrideSpec.Spec.TLS.CACertificate != "" &&
+		overrideSpec.Spec.TLS.Certificate != "" &&
+		overrideSpec.Spec.TLS.Key != ""
+}

tests/kuttl/common/custom-barbican-route.yaml

@@ -0,0 +1,10 @@
+# A ctustom barbican secret that's used for route override
+apiVersion: v1
+kind: Secret
+type: kubernetes.io/tls
+metadata:
+  name: barbican-custom-route
+data:
+  ca.crt: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUIyRENDQVgrZ0F3SUJBZ0lVV0FSZ0h3dWF1czl3N3VRMW9wUmxEWFJQUnZzd0NnWUlLb1pJemowRUF3SXcKUWpFTE1Ba0dBMVVFQmhNQ1dGZ3hGVEFUQmdOVkJBY01ERVJsWm1GMWJIUWdRMmwwZVRFY01Cb0dBMVVFQ2d3VApSR1ZtWVhWc2RDQkRiMjF3WVc1NUlFeDBaREFlRncweU5EQTBNamt3TnpRMk5EZGFGdzB5TlRBME1qa3dOelEyCk5EZGFNRUl4Q3pBSkJnTlZCQVlUQWxoWU1SVXdFd1lEVlFRSERBeEVaV1poZFd4MElFTnBkSGt4SERBYUJnTlYKQkFvTUUwUmxabUYxYkhRZ1EyOXRjR0Z1ZVNCTWRHUXdXVEFUQmdjcWhrak9QUUlCQmdncWhrak9QUU1CQndOQwpBQVRnNjk1TVFSRTdlcktoSGZOTDBNQSsyYnZlMVRydXhqTm01a1VKYUNrOENiUVdpZmlmbjlXakhsUG5ORDZ4CmZ0eER5aXM1TXlUdmRqcFluelpUeFNFd28xTXdVVEFkQmdOVkhRNEVGZ1FVVmtqdE5uVVJKNGMra3ZabjcvZkYKRUpoTXo3UXdId1lEVlIwakJCZ3dGb0FVVmtqdE5uVVJKNGMra3ZabjcvZkZFSmhNejdRd0R3WURWUjBUQVFILwpCQVV3QXdFQi96QUtCZ2dxaGtqT1BRUURBZ05IQURCRUFpQXVFeFBZM0pGQnVaMUlGdjVTZitBaTNZSHd0b1NiClB6S0M2RHEwWVJwTnFnSWdUVEs5eWZKcGlKU0h3SnFhMmRXbkQ4ZEprZjhqS0grNi9WSnJSazNtcGtZPQotLS0tLUVORCBDRVJUSUZJQ0FURS0tLS0tCg==
+  tls.crt: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUJmakNDQVNVQ0ZCMmJGdzJNZlJCMHZJQVptTmU4MWFSSklqOEdNQW9HQ0NxR1NNNDlCQU1DTUVJeEN6QUoKQmdOVkJBWVRBbGhZTVJVd0V3WURWUVFIREF4RVpXWmhkV3gwSUVOcGRIa3hIREFhQmdOVkJBb01FMFJsWm1GMQpiSFFnUTI5dGNHRnVlU0JNZEdRd0hoY05NalF3TkRJNU1EYzBOek00V2hjTk1qVXdOREk1TURjME56TTRXakJDCk1Rc3dDUVlEVlFRR0V3SllXREVWTUJNR0ExVUVCd3dNUkdWbVlYVnNkQ0JEYVhSNU1Sd3dHZ1lEVlFRS0RCTkUKWldaaGRXeDBJRU52YlhCaGJua2dUSFJrTUZrd0V3WUhLb1pJemowQ0FRWUlLb1pJemowREFRY0RRZ0FFeHVQeApZWDVTckpweW1QWC9qNjdYWGlIaVN0ZThTNnd6YnVKZDB4TTRIT0RhWmJ0MHIwSVdPOHJSUVVFZUxudWRGb0Y2Ckp1NFIzUVVUOGQrdDN6RFpxVEFLQmdncWhrak9QUVFEQWdOSEFEQkVBaUJZc0cyM0tSZi8zbytSOStpbVdKYTQKOGQyenU5ZU1yNnFrVGc1UTR0ai9zZ0lnTGFveDdRSzJBbzRkWTNubWx5Zzlhc2NzUU5LVjRicGRTWEFYL2RySAo3REU9Ci0tLS0tRU5EIENFUlRJRklDQVRFLS0tLS0K
+  tls.key: LS0tLS1CRUdJTiBFQyBQUklWQVRFIEtFWS0tLS0tCk1IY0NBUUVFSUJnNzNNdzdtWHNpa1BoN3o0c3M3d0Nhd3ZYRkowdkdEUERLb0FNTkJadXRvQW9HQ0NxR1NNNDkKQXdFSG9VUURRZ0FFeHVQeFlYNVNySnB5bVBYL2o2N1hYaUhpU3RlOFM2d3pidUpkMHhNNEhPRGFaYnQwcjBJVwpPOHJSUVVFZUxudWRGb0Y2SnU0UjNRVVQ4ZCt0M3pEWnFRPT0KLS0tLS1FTkQgRUMgUFJJVkFURSBLRVktLS0tLQo=

tests/kuttl/common/osp_check_route_cert.sh

@@ -0,0 +1,57 @@
+#!/bin/bash
+
+ROUTE_NAME=$1
+
+if [[ "$ROUTE_NAME" == "barbican" ]]; then
+    EXPECTED_CERTIFICATE="-----BEGIN CERTIFICATE-----
+MIIBfjCCASUCFB2bFw2MfRB0vIAZmNe81aRJIj8GMAoGCCqGSM49BAMCMEIxCzAJ
+BgNVBAYTAlhYMRUwEwYDVQQHDAxEZWZhdWx0IENpdHkxHDAaBgNVBAoME0RlZmF1
+bHQgQ29tcGFueSBMdGQwHhcNMjQwNDI5MDc0NzM4WhcNMjUwNDI5MDc0NzM4WjBC
+MQswCQYDVQQGEwJYWDEVMBMGA1UEBwwMRGVmYXVsdCBDaXR5MRwwGgYDVQQKDBNE
+ZWZhdWx0IENvbXBhbnkgTHRkMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExuPx
+YX5SrJpymPX/j67XXiHiSte8S6wzbuJd0xM4HODaZbt0r0IWO8rRQUEeLnudFoF6
+Ju4R3QUT8d+t3zDZqTAKBggqhkjOPQQDAgNHADBEAiBYsG23KRf/3o+R9+imWJa4
+8d2zu9eMr6qkTg5Q4tj/sgIgLaox7QK2Ao4dY3nmlyg9ascsQNKV4bpdSXAX/drH
+7DE=
+-----END CERTIFICATE-----"
+
+    EXPECTED_CA_CERTIFICATE="-----BEGIN CERTIFICATE-----
+MIIB2DCCAX+gAwIBAgIUWARgHwuaus9w7uQ1opRlDXRPRvswCgYIKoZIzj0EAwIw
+QjELMAkGA1UEBhMCWFgxFTATBgNVBAcMDERlZmF1bHQgQ2l0eTEcMBoGA1UECgwT
+RGVmYXVsdCBDb21wYW55IEx0ZDAeFw0yNDA0MjkwNzQ2NDdaFw0yNTA0MjkwNzQ2
+NDdaMEIxCzAJBgNVBAYTAlhYMRUwEwYDVQQHDAxEZWZhdWx0IENpdHkxHDAaBgNV
+BAoME0RlZmF1bHQgQ29tcGFueSBMdGQwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNC
+AATg695MQRE7erKhHfNL0MA+2bve1TruxjNm5kUJaCk8CbQWififn9WjHlPnND6x
+ftxDyis5MyTvdjpYnzZTxSEwo1MwUTAdBgNVHQ4EFgQUVkjtNnURJ4c+kvZn7/fF
+EJhMz7QwHwYDVR0jBBgwFoAUVkjtNnURJ4c+kvZn7/fFEJhMz7QwDwYDVR0TAQH/
+BAUwAwEB/zAKBggqhkjOPQQDAgNHADBEAiAuExPY3JFBuZ1IFv5Sf+Ai3YHwtoSb
+PzKC6Dq0YRpNqgIgTTK9yfJpiJSHwJqa2dWnD8dJkf8jKH+6/VJrRk3mpkY=
+-----END CERTIFICATE-----"
+elif [[ "$ROUTE_NAME" == "placement" ]]; then
+    EXPECTED_CERTIFICATE="CERT123"
+    EXPECTED_CA_CERTIFICATE="CACERT123"
+fi
+
+TLS_DATA=$(oc get route ${ROUTE_NAME}-public -n $NAMESPACE -o jsonpath='{.spec.tls}')
+
+# Extract certificates from the route
+ACTUAL_CERTIFICATE=$(echo "$TLS_DATA" | jq -r '.certificate')
+ACTUAL_CA_CERTIFICATE=$(echo "$TLS_DATA" | jq -r '.caCertificate')
+
+TRIMMED_EXPECTED_CERTIFICATE=$(echo "$EXPECTED_CERTIFICATE" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')
+TRIMMED_ACTUAL_CERTIFICATE=$(echo "$ACTUAL_CERTIFICATE" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')
+
+TRIMMED_EXPECTED_CA_CERTIFICATE=$(echo "$EXPECTED_CA_CERTIFICATE" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')
+TRIMMED_ACTUAL_CA_CERTIFICATE=$(echo "$ACTUAL_CA_CERTIFICATE" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')
+
+if [[ "$TRIMMED_EXPECTED_CERTIFICATE" != "$TRIMMED_ACTUAL_CERTIFICATE" ]]; then
+    echo "Certificate does not match for route $ROUTE_NAME in namespace $NAMESPACE."
+    exit 1
+fi
+
+if [[ "$TRIMMED_EXPECTED_CA_CERTIFICATE" != "$TRIMMED_ACTUAL_CA_CERTIFICATE" ]]; then
+    echo "CA Certificate does not match for route $ROUTE_NAME in namespace $NAMESPACE."
+    exit 1
+fi
+
+echo "TLS data matches for route $ROUTE_NAME in namespace $NAMESPACE."

tests/kuttl/tests/ctlplane-tls-custom-route/01-assert-deploy-openstack.yaml

@@ -0,0 +1 @@
+../../common/assert-sample-deployment.yaml

tests/kuttl/tests/ctlplane-tls-custom-route/01-deploy-openstack.yaml

@@ -0,0 +1,5 @@
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+commands:
+  - script: |
+      oc kustomize ../../../../config/samples/base/openstackcontrolplane | oc apply -n $NAMESPACE -f -

tests/kuttl/tests/ctlplane-tls-custom-route/02-assert-custom-route-secret.yaml

@@ -0,0 +1,4 @@
+apiVersion: v1
+kind: Secret
+metadata:
+  name: barbican-custom-route

tests/kuttl/tests/ctlplane-tls-custom-route/02-deploy-custom-route-secret.yaml

@@ -0,0 +1,5 @@
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+commands:
+  - script: |
+      oc apply -n $NAMESPACE -f ../../common/custom-barbican-route.yaml