From dbce2b68f112822396071ff22639fc79b8478747 Mon Sep 17 00:00:00 2001
From: Lawrence Jones <lawrjone@gmail.com>
Date: Tue, 26 May 2020 19:17:03 +0100
Subject: [PATCH] Cache trigger secrets for the duration of request

This commit adds a request-local cache for interceptors to leverage
during the processing of triggers. It allows interceptors to avoid doing
expensive work more than once for each request, such as fetching a
Kubernetes secret for validating webhooks.

The implementation uses the request context to provide the cache. This
was the least disruptive method of providing a cache for use with
interceptors, and is appropriate if you consider the cache should live
only for the duration of each request.

Alternative implementations might have used the client-go informers to
extend the Kubernetes client to watch for secrets in the cluster. This
would cause the work required to fetch secrets to scale with the number
of secrets in the cluster, as opposed to making a fresh request per
webhook we process. That said, building caching clients seems like more
work than is necessary for fixing this simple problem, which is why I
went with a simple cache object.

The background for this change was finding Github webhooks timing out
once we exceeded ~40 triggers on our EventListener. While the CEL
filtering was super fast, the validation of Github webhook signatures
was being computed for every trigger, even though each trigger used the
same Github secret. Pulling the secret from Kubernetes was taking about
250ms, which meant 40 triggers exceeded the 10s Github timeout.
---
 pkg/interceptors/interceptors.go | 42 ++++++++++++++++++++++++++++++--
 pkg/sink/sink.go                 |  5 ++++
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/pkg/interceptors/interceptors.go b/pkg/interceptors/interceptors.go
index dc35d75c4..707b7f14e 100644
--- a/pkg/interceptors/interceptors.go
+++ b/pkg/interceptors/interceptors.go
@@ -17,7 +17,9 @@ limitations under the License.
 package interceptors
 
 import (
+	"context"
 	"net/http"
+	"path"
 
 	triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -29,7 +31,40 @@ type Interceptor interface {
 	ExecuteTrigger(req *http.Request) (*http.Response, error)
 }
 
-func GetSecretToken(cs kubernetes.Interface, sr *triggersv1.SecretRef, eventListenerNamespace string) ([]byte, error) {
+const requestCacheKey = "interceptors.RequestCache"
+
+// WithCache clones the given request and sets the request context to include a cache.
+// This allows us to cache results from expensive operations and perform them just once
+// per each trigger.
+//
+// Each request should have its own cache, and those caches should expire once the request
+// is processed. For this reason, it's appropriate to store the cache on the request
+// context.
+func WithCache(req *http.Request) *http.Request {
+	return req.WithContext(context.WithValue(req.Context(), requestCacheKey, make(map[string]interface{})))
+}
+
+func getCache(req *http.Request) map[string]interface{} {
+	if cache, ok := req.Context().Value(requestCacheKey).(map[string]interface{}); ok {
+		return cache
+	}
+
+	return make(map[string]interface{})
+}
+
+// GetSecretToken queries Kubernetes for the given secret reference. We use this function
+// to resolve secret material like Github webhook secrets, and call it once for every
+// trigger that references it.
+//
+// As we may have many triggers that all use the same secret, we cache the secret values
+// in the request cache.
+func GetSecretToken(req *http.Request, cs kubernetes.Interface, sr *triggersv1.SecretRef, eventListenerNamespace string) ([]byte, error) {
+	cacheKey := path.Join("secret", sr.Namespace, sr.SecretName, sr.SecretKey)
+	cache := getCache(req)
+	if secretValue, ok := cache[cacheKey]; ok {
+		return secretValue.([]byte), nil
+	}
+
 	ns := sr.Namespace
 	if ns == "" {
 		ns = eventListenerNamespace
@@ -39,5 +74,8 @@ func GetSecretToken(cs kubernetes.Interface, sr *triggersv1.SecretRef, eventList
 		return nil, err
 	}
 
-	return secret.Data[sr.SecretKey], nil
+	secretValue := secret.Data[sr.SecretKey]
+	cache[cacheKey] = secret.Data[sr.SecretKey]
+
+	return secretValue, nil
 }
diff --git a/pkg/sink/sink.go b/pkg/sink/sink.go
index 113cae3ff..7109eb1fa 100644
--- a/pkg/sink/sink.go
+++ b/pkg/sink/sink.go
@@ -186,6 +186,11 @@ func (r Sink) executeInterceptors(t *triggersv1.EventListenerTrigger, in *http.R
 		Header: in.Header,
 		Body:   ioutil.NopCloser(bytes.NewBuffer(event)),
 	}
+
+	// We create a cache against each request, so whenever we make network calls like
+	// fetching kubernetes secrets, we can do so only once per request.
+	request = interceptors.WithCache(request)
+
 	var resp *http.Response
 	for _, i := range t.Interceptors {
 		var interceptor interceptors.Interceptor