Repository: jetstack/cert-manager (go)

Issue:
Metrics for deleted Certificates should be cleaned up
> Bugs should be filed for issues encountered whilst operating cert-manager.
> You should first attempt to resolve your issues through the community support
> channels, e.g. Slack, in order to rule out individual configuration errors.
> Please provide as much detail as possible.

**Describe the bug**:
A given `Certificate` will have a correspoding `certmanager_certificate_expiration_timestamp_seconds` Prometheus metric exposed. If the certificate is deleted, the metric is not cleaned up and continues to report the expiration time from the point the certificate was deleted. Metrics are only cleaned up if the pod is restarted

![image](https://user-images.githubusercontent.com/17579850/52427456-d88e3d80-2af7-11e9-9c7f-e98ab5da1af1.png)


**Expected behaviour**:
Metrics for deleted certificates should be regularly cleaned up

**Steps to reproduce the bug**:
- Create Certificate and issue successfully
- `certmanager_certificate_expiration_timestamp_seconds` metric should be created for Certificate
- Delete Certificate; metric will continue to be exposed

**Anything else we need to know?**:

**Environment details:**:
- Kubernetes version (e.g. v1.10.2): 1.11.3
- Cloud-provider/provisioner (e.g. GKE, kops AWS, etc): GKE
- cert-manager version (e.g. v0.4.0): v0.6.0
- Install method (e.g. helm or static manifests): Helm (Custom)

/kind bug


Pull request:
Clean up Prometheus metrics for deleted Certificates
<!--  Thanks for sending a pull request!  Here are some tips for you:
1. If this is your first time, read our contributor guidelines https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. If you want *faster* PR reviews, read how: https://git.k8s.io/community/contributors/guide/pull-requests.md#best-practices-for-faster-reviews
3. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
4. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What this PR does / why we need it**:
Prometheus metrics for Certificates will be cleaned up by deleting entries for deleted Certificates, and keeping track of which Certificates are currently registered by Prometheus.
This prevents metrics for deleted Certificates from being exposed past their use.

**Which issue this PR fixes**: fixes #1332 

**Special notes for your reviewer**:
The metrics are currently cleaned up every minute via a goroutine in the certificate controller - I would be happy to discuss a more refined garbage collection scheme to implement.

**Release note**:
<!--  Steps to write your release note:
1. Use the release-note-* labels to set the release note state (if you have access)
2. Enter your extended release note in the below block; leaving it blank means using the PR title as the release note. If no release note is required, just write `NONE`.
-->
```release-note
Prometheus metrics for deleted Certificates are cleaned up
```


Patch:
diff --git a/pkg/controller/certificates/controller.go b/pkg/controller/certificates/controller.go
index 598115c99..c637d3aa6 100644
--- a/pkg/controller/certificates/controller.go
+++ b/pkg/controller/certificates/controller.go
@@ -109,6 +109,7 @@ func New(ctx *controllerpkg.Context) *Controller {
 
 	ctrl.helper = issuer.NewHelper(ctrl.issuerLister, ctrl.clusterIssuerLister)
 	ctrl.metrics = metrics.Default
+	ctrl.metrics.SetActiveCertificates(ctrl.certificateLister)
 	ctrl.helper = issuer.NewHelper(ctrl.issuerLister, ctrl.clusterIssuerLister)
 	ctrl.issuerFactory = issuer.NewIssuerFactory(ctx)
 	ctrl.clock = clock.RealClock{}
diff --git a/pkg/metrics/BUILD.bazel b/pkg/metrics/BUILD.bazel
index d2eef4ae7..86c02bb59 100644
--- a/pkg/metrics/BUILD.bazel
+++ b/pkg/metrics/BUILD.bazel
@@ -7,6 +7,7 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//pkg/apis/certmanager/v1alpha1:go_default_library",
+        "//pkg/client/listers/certmanager/v1alpha1:go_default_library",
         "//pkg/logs:go_default_library",
         "//pkg/util/errors:go_default_library",
         "//pkg/util/kube:go_default_library",
@@ -14,7 +15,10 @@ go_library(
         "//vendor/github.com/prometheus/client_golang/prometheus:go_default_library",
         "//vendor/github.com/prometheus/client_golang/prometheus/promhttp:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/labels:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library",
         "//vendor/k8s.io/client-go/listers/core/v1:go_default_library",
+        "//vendor/k8s.io/client-go/tools/cache:go_default_library",
     ],
 )
 
@@ -36,5 +40,9 @@ go_test(
     name = "go_default_test",
     srcs = ["metrics_test.go"],
     embed = [":go_default_library"],
-    deps = ["//vendor/github.com/prometheus/client_golang/prometheus/testutil:go_default_library"],
+    deps = [
+        "//pkg/apis/certmanager/v1alpha1:go_default_library",
+        "//vendor/github.com/prometheus/client_golang/prometheus/testutil:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
+    ],
 )
diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go
index ee5d9e3c8..45a981364 100644
--- a/pkg/metrics/metrics.go
+++ b/pkg/metrics/metrics.go
@@ -23,15 +23,20 @@ import (
 	"context"
 	"crypto/x509"
 	"net/http"
+	"sync"
 	"time"
 
 	"github.com/gorilla/mux"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/util/wait"
 	corelisters "k8s.io/client-go/listers/core/v1"
+	"k8s.io/client-go/tools/cache"
 
 	"github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1"
+	cmlisters "github.com/jetstack/cert-manager/pkg/client/listers/certmanager/v1alpha1"
 	logf "github.com/jetstack/cert-manager/pkg/logs"
 	"github.com/jetstack/cert-manager/pkg/util/errors"
 	"github.com/jetstack/cert-manager/pkg/util/kube"
@@ -93,9 +98,21 @@ var ControllerSyncCallCount = prometheus.NewCounterVec(
 	[]string{"controller"},
 )
 
+// registeredCertificates holds the set of all certificates which are currently
+// registered by Prometheus
+var registeredCertificates = &struct {
+	certificates map[string]struct{}
+	mtx          sync.Mutex
+}{
+	certificates: make(map[string]struct{}),
+}
+
+var activeCertificates cmlisters.CertificateLister
+
 type Metrics struct {
 	ctx context.Context
 	http.Server
+	activeCertificates cmlisters.CertificateLister
 
 	// TODO (@dippynark): switch this to use an interface to make it testable
 	registry                         *prometheus.Registry
@@ -118,6 +135,7 @@ func New(ctx context.Context) *Metrics {
 			MaxHeaderBytes: prometheusMetricsServerMaxHeaderBytes,
 			Handler:        router,
 		},
+		activeCertificates:               nil,
 		registry:                         prometheus.NewRegistry(),
 		CertificateExpiryTimeSeconds:     CertificateExpiryTimeSeconds,
 		ACMEClientRequestDurationSeconds: ACMEClientRequestDurationSeconds,
@@ -166,6 +184,8 @@ func (m *Metrics) Start(stopCh <-chan struct{}) {
 
 	}()
 
+	go wait.Until(func() { m.cleanUp() }, time.Minute, stopCh)
+
 	m.waitShutdown(stopCh)
 }
 
@@ -185,15 +205,79 @@ func (m *Metrics) UpdateCertificateExpiry(crt *v1alpha1.Certificate, secretListe
 		return
 	}
 
-	updateX509Expiry(crt.Name, crt.Namespace, cert)
+	updateX509Expiry(crt, cert)
 }
 
-func updateX509Expiry(name, namespace string, cert *x509.Certificate) {
-	// set certificate expiry time
+func updateX509Expiry(crt *v1alpha1.Certificate, cert *x509.Certificate) {
 	expiryTime := cert.NotAfter
+	key, err := cache.DeletionHandlingMetaNamespaceKeyFunc(crt)
+	if err != nil {
+		return
+	}
+
+	registeredCertificates.mtx.Lock()
+	defer registeredCertificates.mtx.Unlock()
+	// set certificate expiry time
 	CertificateExpiryTimeSeconds.With(prometheus.Labels{
-		"name":      name,
-		"namespace": namespace}).Set(float64(expiryTime.Unix()))
+		"name":      crt.Name,
+		"namespace": crt.Namespace}).Set(float64(expiryTime.Unix()))
+	registeredCertificates.certificates[key] = struct{}{}
+}
+
+func (m *Metrics) SetActiveCertificates(cl cmlisters.CertificateLister) {
+	m.activeCertificates = cl
+}
+
+func (m *Metrics) cleanUp() {
+	log := logf.FromContext(m.ctx)
+	log.V(logf.DebugLevel).Info("attempting to clean up metrics for recently deleted certificates")
+
+	if activeCertificates == nil {
+		log.V(logf.DebugLevel).Info("active certificates is still uninitialized")
+		return
+	}
+
+	activeCrts, err := activeCertificates.List(labels.Everything())
+	if err != nil {
+		log.Error(err, "error retrieving active certificates")
+		return
+	}
+
+	cleanUpCertificates(activeCrts)
+}
+
+func cleanUpCertificates(activeCrts []*v1alpha1.Certificate) {
+	activeMap := make(map[string]struct{}, len(activeCrts))
+	for _, crt := range activeCrts {
+		key, err := cache.DeletionHandlingMetaNamespaceKeyFunc(crt)
+		if err != nil {
+			continue
+		}
+
+		activeMap[key] = struct{}{}
+	}
+
+	registeredCertificates.mtx.Lock()
+	defer registeredCertificates.mtx.Unlock()
+	var toCleanUp []string
+	for key := range registeredCertificates.certificates {
+		if _, found := activeMap[key]; !found {
+			toCleanUp = append(toCleanUp, key)
+		}
+	}
+
+	for _, key := range toCleanUp {
+		namespace, name, err := cache.SplitMetaNamespaceKey(key)
+		if err != nil {
+			continue
+		}
+
+		CertificateExpiryTimeSeconds.Delete(prometheus.Labels{
+			"name":      name,
+			"namespace": namespace,
+		})
+		delete(registeredCertificates.certificates, key)
+	}
 }
 
 func (m *Metrics) IncrementSyncCallCount(controllerName string) {


Test patch:
diff --git a/pkg/metrics/metrics_test.go b/pkg/metrics/metrics_test.go
index 10d5d4dc8..65a6f1f4f 100644
--- a/pkg/metrics/metrics_test.go
+++ b/pkg/metrics/metrics_test.go
@@ -23,6 +23,9 @@ import (
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus/testutil"
+
+	"github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func TestUpdateCertificateExpiry(t *testing.T) {
@@ -30,29 +33,149 @@ func TestUpdateCertificateExpiry(t *testing.T) {
 	# HELP certmanager_certificate_expiration_timestamp_seconds The date after which the certificate expires. Expressed as a Unix Epoch Time.
 	# TYPE certmanager_certificate_expiration_timestamp_seconds gauge
 `
-
 	type testT struct {
-		expected  string
-		name      string
-		namespace string
-		cert      *x509.Certificate
+		crt      *v1alpha1.Certificate
+		cert     *x509.Certificate
+		expected string
 	}
 	tests := map[string]testT{
 		"first": {
-			name:      "something",
-			namespace: "default",
-			expected: `
-	certmanager_certificate_expiration_timestamp_seconds{name="something",namespace="default"} 2.208988804e+09
-`,
+			crt: &v1alpha1.Certificate{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "something",
+					Namespace: "default",
+				},
+			},
 			cert: &x509.Certificate{
 				// fixed expiry time for testing
 				NotAfter: time.Unix(2208988804, 0),
 			},
+			expected: `
+	certmanager_certificate_expiration_timestamp_seconds{name="something",namespace="default"} 2.208988804e+09
+`,
 		},
 	}
 	for n, test := range tests {
 		t.Run(n, func(t *testing.T) {
-			updateX509Expiry(test.name, test.namespace, test.cert)
+			defer cleanUpCertificates(nil)
+
+			updateX509Expiry(test.crt, test.cert)
+
+			if err := testutil.CollectAndCompare(
+				CertificateExpiryTimeSeconds,
+				strings.NewReader(metadata+test.expected),
+				"certmanager_certificate_expiration_timestamp_seconds",
+			); err != nil {
+				t.Errorf("unexpected collecting result:\n%s", err)
+			}
+		})
+	}
+}
+
+func TestCleanUp(t *testing.T) {
+	const metadata = `
+	# HELP certmanager_certificate_expiration_timestamp_seconds The date after which the certificate expires. Expressed as a Unix Epoch Time.
+	# TYPE certmanager_certificate_expiration_timestamp_seconds gauge
+`
+	type testT struct {
+		active   map[*v1alpha1.Certificate]*x509.Certificate
+		inactive map[*v1alpha1.Certificate]*x509.Certificate
+		expected string
+	}
+	tests := map[string]testT{
+		"active and inactive": {
+			active: map[*v1alpha1.Certificate]*x509.Certificate{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "something",
+						Namespace: "default",
+					},
+				}: {
+					// fixed expiry time for testing
+					NotAfter: time.Unix(2208988804, 0),
+				},
+			},
+			inactive: map[*v1alpha1.Certificate]*x509.Certificate{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "something-else",
+						Namespace: "default",
+					},
+				}: {
+					// fixed expiry time for testing
+					NotAfter: time.Unix(2208988804, 0),
+				},
+			},
+			expected: `
+	certmanager_certificate_expiration_timestamp_seconds{name="something",namespace="default"} 2.208988804e+09
+`,
+		},
+		"only active": {
+			active: map[*v1alpha1.Certificate]*x509.Certificate{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "something",
+						Namespace: "default",
+					},
+				}: {
+					// fixed expiry time for testing
+					NotAfter: time.Unix(2208988804, 0),
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "something-else",
+						Namespace: "default",
+					},
+				}: {
+					// fixed expiry time for testing
+					NotAfter: time.Unix(2208988804, 0),
+				},
+			},
+			inactive: map[*v1alpha1.Certificate]*x509.Certificate{},
+			expected: `
+	certmanager_certificate_expiration_timestamp_seconds{name="something",namespace="default"} 2.208988804e+09
+	certmanager_certificate_expiration_timestamp_seconds{name="something-else",namespace="default"} 2.208988804e+09
+`,
+		},
+		"only inactive": {
+			active: map[*v1alpha1.Certificate]*x509.Certificate{},
+			inactive: map[*v1alpha1.Certificate]*x509.Certificate{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "something",
+						Namespace: "default",
+					},
+				}: {
+					// fixed expiry time for testing
+					NotAfter: time.Unix(2208988804, 0),
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "something-else",
+						Namespace: "default",
+					},
+				}: {
+					// fixed expiry time for testing
+					NotAfter: time.Unix(2208988804, 0),
+				},
+			},
+			expected: "",
+		},
+	}
+	for n, test := range tests {
+		t.Run(n, func(t *testing.T) {
+			defer cleanUpCertificates(nil)
+
+			var activeCrts []*v1alpha1.Certificate
+			for crt, cert := range test.active {
+				updateX509Expiry(crt, cert)
+				activeCrts = append(activeCrts, crt)
+			}
+			for crt, cert := range test.inactive {
+				updateX509Expiry(crt, cert)
+			}
+
+			cleanUpCertificates(activeCrts)
 
 			if err := testutil.CollectAndCompare(
 				CertificateExpiryTimeSeconds,