From a68d16cca0aeb3ba71009e391e7915671ca59bfb Mon Sep 17 00:00:00 2001 From: Lawrence Jones 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/bitbucket/bitbucket.go | 2 +- pkg/interceptors/cel/cel.go | 6 +- pkg/interceptors/cel/cel_test.go | 4 +- pkg/interceptors/cel/triggers.go | 11 +-- pkg/interceptors/github/github.go | 2 +- pkg/interceptors/gitlab/gitlab.go | 2 +- pkg/interceptors/interceptors.go | 50 +++++++++++- pkg/interceptors/interceptors_test.go | 102 ++++++++++++++++++++++++ pkg/sink/sink.go | 5 ++ 9 files changed, 169 insertions(+), 15 deletions(-) create mode 100644 pkg/interceptors/interceptors_test.go diff --git a/pkg/interceptors/bitbucket/bitbucket.go b/pkg/interceptors/bitbucket/bitbucket.go index ed7995ea64..90032b1c3a 100644 --- a/pkg/interceptors/bitbucket/bitbucket.go +++ b/pkg/interceptors/bitbucket/bitbucket.go @@ -64,7 +64,7 @@ func (w *Interceptor) ExecuteTrigger(request *http.Request) (*http.Response, err if header == "" { return nil, errors.New("no X-Hub-Signature header set") } - secretToken, err := interceptors.GetSecretToken(w.KubeClientSet, w.Bitbucket.SecretRef, w.EventListenerNamespace) + secretToken, err := interceptors.GetSecretToken(request, w.KubeClientSet, w.Bitbucket.SecretRef, w.EventListenerNamespace) if err != nil { return nil, err } diff --git a/pkg/interceptors/cel/cel.go b/pkg/interceptors/cel/cel.go index bcec54a7fe..b2a1725094 100644 --- a/pkg/interceptors/cel/cel.go +++ b/pkg/interceptors/cel/cel.go @@ -60,7 +60,7 @@ func NewInterceptor(cel *triggersv1.CELInterceptor, k kubernetes.Interface, ns s // ExecuteTrigger is an implementation of the Interceptor interface. func (w *Interceptor) ExecuteTrigger(request *http.Request) (*http.Response, error) { - env, err := makeCelEnv(w.EventListenerNamespace, w.KubeClientSet) + env, err := makeCelEnv(request, w.EventListenerNamespace, w.KubeClientSet) if err != nil { return nil, fmt.Errorf("error creating cel environment: %w", err) } @@ -153,10 +153,10 @@ func evaluate(expr string, env *cel.Env, data map[string]interface{}) (ref.Val, return out, err } -func makeCelEnv(ns string, k kubernetes.Interface) (*cel.Env, error) { +func makeCelEnv(request *http.Request, ns string, k kubernetes.Interface) (*cel.Env, error) { mapStrDyn := decls.NewMapType(decls.String, decls.Dyn) return cel.NewEnv( - Triggers(ns, k), + Triggers(request, ns, k), celext.Strings(), cel.Declarations( decls.NewIdent("body", mapStrDyn, nil), diff --git a/pkg/interceptors/cel/cel_test.go b/pkg/interceptors/cel/cel_test.go index ea138a0289..93dd7a5d7a 100644 --- a/pkg/interceptors/cel/cel_test.go +++ b/pkg/interceptors/cel/cel_test.go @@ -431,7 +431,7 @@ func TestExpressionEvaluation(t *testing.T) { rt.Error(err) } } - env, err := makeCelEnv(testNS, kubeClient) + env, err := makeCelEnv(&http.Request{}, testNS, kubeClient) if err != nil { t.Fatal(err) } @@ -540,7 +540,7 @@ func TestExpressionEvaluation_Error(t *testing.T) { } ns = tt.secretNS } - env, err := makeCelEnv(ns, kubeClient) + env, err := makeCelEnv(&http.Request{}, ns, kubeClient) if err != nil { t.Fatal(err) } diff --git a/pkg/interceptors/cel/triggers.go b/pkg/interceptors/cel/triggers.go index 714b2c499e..87fef1de4d 100644 --- a/pkg/interceptors/cel/triggers.go +++ b/pkg/interceptors/cel/triggers.go @@ -139,11 +139,12 @@ import ( // 'https://example.com/testing'.parseURL().host == 'example.com' // Triggers creates and returns a new cel.Lib with the triggers extensions. -func Triggers(ns string, k kubernetes.Interface) cel.EnvOption { - return cel.Lib(triggersLib{defaultNS: ns, client: k}) +func Triggers(request *http.Request, ns string, k kubernetes.Interface) cel.EnvOption { + return cel.Lib(triggersLib{request: request, defaultNS: ns, client: k}) } type triggersLib struct { + request *http.Request defaultNS string client kubernetes.Interface } @@ -201,7 +202,7 @@ func (t triggersLib) ProgramOptions() []cel.ProgramOption { Unary: parseURLString}, &functions.Overload{ Operator: "compareSecret", - Function: makeCompareSecret(t.defaultNS, t.client)}, + Function: makeCompareSecret(t.request, t.defaultNS, t.client)}, )} } @@ -266,7 +267,7 @@ func decodeB64String(val ref.Val) ref.Val { // makeCompareSecret creates and returns a functions.FunctionOp that wraps the // ns and client in a closure with a function that can compare the string. -func makeCompareSecret(defaultNS string, k kubernetes.Interface) functions.FunctionOp { +func makeCompareSecret(request *http.Request, defaultNS string, k kubernetes.Interface) functions.FunctionOp { return func(vals ...ref.Val) ref.Val { var ok bool compareString, ok := vals[0].(types.String) @@ -300,7 +301,7 @@ func makeCompareSecret(defaultNS string, k kubernetes.Interface) functions.Funct SecretName: string(secretName), Namespace: string(secretNS), } - secretToken, err := interceptors.GetSecretToken(k, secretRef, string(secretNS)) + secretToken, err := interceptors.GetSecretToken(request, k, secretRef, string(secretNS)) if err != nil { return types.NewErr("failed to find secret '%#v' in compareSecret: %w", *secretRef, err) } diff --git a/pkg/interceptors/github/github.go b/pkg/interceptors/github/github.go index fce92fbe03..0921c368d9 100644 --- a/pkg/interceptors/github/github.go +++ b/pkg/interceptors/github/github.go @@ -64,7 +64,7 @@ func (w *Interceptor) ExecuteTrigger(request *http.Request) (*http.Response, err if header == "" { return nil, errors.New("no X-Hub-Signature header set") } - secretToken, err := interceptors.GetSecretToken(w.KubeClientSet, w.GitHub.SecretRef, w.EventListenerNamespace) + secretToken, err := interceptors.GetSecretToken(request, w.KubeClientSet, w.GitHub.SecretRef, w.EventListenerNamespace) if err != nil { return nil, err } diff --git a/pkg/interceptors/gitlab/gitlab.go b/pkg/interceptors/gitlab/gitlab.go index 45aaaaf7ac..f349e7b489 100644 --- a/pkg/interceptors/gitlab/gitlab.go +++ b/pkg/interceptors/gitlab/gitlab.go @@ -54,7 +54,7 @@ func (w *Interceptor) ExecuteTrigger(request *http.Request) (*http.Response, err return nil, errors.New("no X-GitLab-Token header set") } - secretToken, err := interceptors.GetSecretToken(w.KubeClientSet, w.GitLab.SecretRef, w.EventListenerNamespace) + secretToken, err := interceptors.GetSecretToken(request, w.KubeClientSet, w.GitLab.SecretRef, w.EventListenerNamespace) if err != nil { return nil, err } diff --git a/pkg/interceptors/interceptors.go b/pkg/interceptors/interceptors.go index dc35d75c44..dc9a6c4e31 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,46 @@ type Interceptor interface { ExecuteTrigger(req *http.Request) (*http.Response, error) } -func GetSecretToken(cs kubernetes.Interface, sr *triggersv1.SecretRef, eventListenerNamespace string) ([]byte, error) { +type key string + +const requestCacheKey key = "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) { + var cache map[string]interface{} + + cacheKey := path.Join("secret", sr.Namespace, sr.SecretName, sr.SecretKey) + if req != nil { + cache = getCache(req) + if secretValue, ok := cache[cacheKey]; ok { + return secretValue.([]byte), nil + } + } + ns := sr.Namespace if ns == "" { ns = eventListenerNamespace @@ -39,5 +80,10 @@ func GetSecretToken(cs kubernetes.Interface, sr *triggersv1.SecretRef, eventList return nil, err } - return secret.Data[sr.SecretKey], nil + secretValue := secret.Data[sr.SecretKey] + if req != nil { + cache[cacheKey] = secret.Data[sr.SecretKey] + } + + return secretValue, nil } diff --git a/pkg/interceptors/interceptors_test.go b/pkg/interceptors/interceptors_test.go new file mode 100644 index 0000000000..15be7bc950 --- /dev/null +++ b/pkg/interceptors/interceptors_test.go @@ -0,0 +1,102 @@ +/* +Copyright 2019 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package interceptors + +import ( + "bytes" + "context" + "fmt" + "net/http" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + fakekubeclient "knative.dev/pkg/client/injection/kube/client/fake" + rtesting "knative.dev/pkg/reconciler/testing" + + triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1" +) + +const testNS = "testing-ns" + +func Test_GetSecretToken(t *testing.T) { + tests := []struct { + name string + cache map[string]interface{} + wanted []byte + }{ + { + name: "no matching cache entry exists", + cache: make(map[string]interface{}), + wanted: []byte("secret from API"), + }, + { + name: "a matching cache entry exists", + cache: map[string]interface{}{ + fmt.Sprintf("secret/%s/test-secret/token", testNS): []byte("secret from cache"), + }, + wanted: []byte("secret from cache"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(rt *testing.T) { + req := setCache(&http.Request{}, tt.cache) + + ctx, _ := rtesting.SetupFakeContext(t) + kubeClient := fakekubeclient.Get(ctx) + secretRef := makeSecretRef() + + if _, err := kubeClient.CoreV1().Secrets(testNS).Create(makeSecret("secret from API")); err != nil { + rt.Error(err) + } + + secret, err := GetSecretToken(req, kubeClient, &secretRef, testNS) + if err != nil { + rt.Error(err) + } + + if !bytes.Equal(secret, tt.wanted) { + rt.Errorf("Expected '%s', got '%s'", string(tt.wanted), string(secret)) + } + }) + } +} + +func makeSecret(secretText string) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: testNS, + Name: "test-secret", + }, + Data: map[string][]byte{ + "token": []byte(secretText), + }, + } +} + +func makeSecretRef() triggersv1.SecretRef { + return triggersv1.SecretRef{ + SecretKey: "token", + SecretName: "test-secret", + Namespace: testNS, + } +} + +func setCache(req *http.Request, vals map[string]interface{}) *http.Request { + return req.WithContext(context.WithValue(req.Context(), requestCacheKey, vals)) +} diff --git a/pkg/sink/sink.go b/pkg/sink/sink.go index 7199160daf..6d0e30074e 100644 --- a/pkg/sink/sink.go +++ b/pkg/sink/sink.go @@ -187,6 +187,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