Migrate core interceptors to use Interceptor CRD

This commit does the following:
1. Add YAML definitions for each core interceptor using the Interceptor CRD.
2. Modify the EventListener to use the Interceptor CRD to find the Interceptor's URL.
3. Update roles/RBAC/unit tests for the above 2 changes.

Fixes tektoncd#868

Signed-off-by: Dibyo Mukherjee <[email protected]>

cmd/eventlistenersink/main.go

@@ -113,6 +113,7 @@ func main() {
 		TriggerBindingLister:        factory.Triggers().V1alpha1().TriggerBindings().Lister(),
 		ClusterTriggerBindingLister: factory.Triggers().V1alpha1().ClusterTriggerBindings().Lister(),
 		TriggerTemplateLister:       factory.Triggers().V1alpha1().TriggerTemplates().Lister(),
+		InterceptorLister:           factory.Triggers().V1alpha1().Interceptors().Lister(),
 	}
 
 	// Listen and serve

config/core-interceptors.yaml

@@ -0,0 +1,70 @@
+# Copyright 2020 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.
+
+apiVersion: triggers.tekton.dev/v1alpha1
+kind: Interceptor
+metadata:
+  name: cel
+spec:
+  clientConfig:
+    url: "http://tekton-triggers-core-interceptors.tekton-pipelines.svc/cel"
+  # Until #869 is implemented, the params field is only informational
+  params:
+    - name: "filter"
+      description: "A CEL expression that evaluates to true or false"
+    - name: "overlays"
+      description: "CEL expressions whose values get mapped to user defined keys"
+---
+apiVersion: triggers.tekton.dev/v1alpha1
+kind: Interceptor
+metadata:
+  name: bitbucket
+spec:
+  clientConfig:
+    url: "http://tekton-triggers-core-interceptors.tekton-pipelines.svc/bitbucket"
+  # Until #869 is implemented, the params field is only informational
+  params:
+    - name: "eventTypes"
+      description: "A list of event types to accept"
+    - name: "secretRef"
+      description: "A reference to a secret containing a shared token for payload validation"
+---
+apiVersion: triggers.tekton.dev/v1alpha1
+kind: Interceptor
+metadata:
+  name: github
+spec:
+  clientConfig:
+    url: "http://tekton-triggers-core-interceptors.tekton-pipelines.svc/github"
+  # Until #869 is implemented, the params field is only informational
+  params:
+    - name: "eventTypes"
+      description: "A list of event types to accept"
+    - name: "secretRef"
+      description: "A reference to a secret containing a shared token for payload validation"
+---
+apiVersion: triggers.tekton.dev/v1alpha1
+kind: Interceptor
+metadata:
+  name: gitlab
+spec:
+  clientConfig:
+    url: "http://tekton-triggers-core-interceptors.tekton-pipelines.svc/gitlab"
+  # Until #869 is implemented, the params field is only informational
+  params:
+    - name: "eventTypes"
+      description: "A list of event types to accept"
+    - name: "secretRef"
+      description: "A reference to a secret containing a shared token for payload validation"
+---

examples/rbac.yaml

@@ -13,9 +13,8 @@ rules:
   resources: ["eventlisteners", "triggerbindings", "triggertemplates", "triggers"]
   verbs: ["get", "list", "watch"]
 - apiGroups: [""]
- # secrets are only needed for GitHub/GitLab interceptors
  # configmaps is needed for updating logging config
-  resources: ["configmaps", "secrets"]
+  resources: ["configmaps"]
   verbs: ["get", "list", "watch"]
  # Permissions to create resources in associated TriggerTemplates
 - apiGroups: ["tekton.dev"]
@@ -48,7 +47,7 @@ metadata:
 rules:
   # EventListeners need to be able to fetch any clustertriggerbindings
 - apiGroups: ["triggers.tekton.dev"]
-  resources: ["clustertriggerbindings"]
+  resources: ["clustertriggerbindings", "interceptors"]
   verbs: ["get", "list", "watch"]
 ---
 apiVersion: rbac.authorization.k8s.io/v1

pkg/interceptors/interceptors.go

@@ -20,14 +20,14 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"net/http"
-	"net/url"
-	"os"
 	"path"
 
 	"google.golang.org/grpc/codes"
+	"knative.dev/pkg/apis"
 
 	triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -171,28 +171,40 @@ func UnmarshalParams(ip map[string]interface{}, p interface{}) error {
 	return nil
 }
 
-// ResolveURL returns the URL for the given core interceptor
-func ResolveURL(i *triggersv1.TriggerInterceptor) *url.URL {
+// Getname returns the name for the given core interceptor
+func GetName(i *triggersv1.TriggerInterceptor) string {
 	// This is temporary until we implement #868
-	path := ""
+	name := ""
 	switch {
 	case i.Bitbucket != nil:
-		path = "bitbucket"
+		name = "bitbucket"
 	case i.CEL != nil:
-		path = "cel"
+		name = "cel"
 	case i.GitHub != nil:
-		path = "github"
+		name = "github"
 	case i.GitLab != nil:
-		path = "gitlab"
+		name = "gitlab"
 	}
-	return &url.URL{
-		Scheme: "http",
-		Host:   fmt.Sprintf("%s.%s.svc", CoreInterceptorsHost, os.Getenv("TEKTON_INSTALL_NAMESPACE")),
-		Path:   path,
+	return name
+}
+
+// Resolve resolves the given CEL name to the
+type InterceptorGetter func(name string) (*triggersv1.Interceptor, error)
+
+var ErrNoURL = errors.New("interceptor URL was not found")
+
+func ResolveToURL(getter InterceptorGetter, name string) (*apis.URL, error) {
+	ic, err := getter(name)
+	if err != nil {
+		return nil, err
+	}
+	url := ic.Spec.ClientConfig.URL
+	if url == nil {
+		return nil, ErrNoURL
 	}
+	return ic.Spec.ClientConfig.URL, nil
 }
 
-// Execute executes the InterceptorRequest using the given httpClient
 func Execute(ctx context.Context, client *http.Client, req *triggersv1.InterceptorRequest, url string) (*triggersv1.InterceptorResponse, error) {
 	b, err := json.Marshal(req)
 	if err != nil {

pkg/interceptors/interceptors_test.go

@@ -18,11 +18,11 @@ package interceptors_test
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
-	"os"
 	"strings"
 	"testing"
 
@@ -35,6 +35,7 @@ import (
 	"google.golang.org/grpc/codes"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"knative.dev/pkg/apis"
 	fakekubeclient "knative.dev/pkg/client/injection/kube/client/fake"
 	rtesting "knative.dev/pkg/reconciler/testing"
 )
@@ -305,48 +306,76 @@ func TestResolvePath(t *testing.T) {
 		in: triggersv1.EventInterceptor{
 			CEL: &triggersv1.CELInterceptor{},
 		},
-		want: "http://tekton-triggers-core-interceptors.tekton-pipelines.svc/cel",
+		want: "cel",
 	}, {
 		in: triggersv1.EventInterceptor{
 			GitLab: &triggersv1.GitLabInterceptor{},
 		},
-		want: "http://tekton-triggers-core-interceptors.tekton-pipelines.svc/gitlab",
+		want: "gitlab",
 	}, {
 		in: triggersv1.EventInterceptor{
 			GitHub: &triggersv1.GitHubInterceptor{},
 		},
-		want: "http://tekton-triggers-core-interceptors.tekton-pipelines.svc/github",
+		want: "github",
 	}, {
 		in: triggersv1.EventInterceptor{
 			Bitbucket: &triggersv1.BitbucketInterceptor{},
 		},
-		want: "http://tekton-triggers-core-interceptors.tekton-pipelines.svc/bitbucket",
+		want: "bitbucket",
 	}, {
 		in: triggersv1.EventInterceptor{
 			Webhook: &triggersv1.WebhookInterceptor{},
 		},
-		want: "http://tekton-triggers-core-interceptors.tekton-pipelines.svc",
+		want: "",
 	}} {
 		t.Run(tc.want, func(t *testing.T) {
-			os.Setenv("TEKTON_INSTALL_NAMESPACE", "tekton-pipelines")
-			t.Cleanup(func() { os.Unsetenv("TEKTON_INSTALL_NAMESPACE") })
-			got := interceptors.ResolveURL(&tc.in)
-			if tc.want != got.String() {
+			got := interceptors.GetName(&tc.in)
+			if tc.want != got {
 				t.Fatalf("ResolveURL() want: %s; got: %s", tc.want, got)
 			}
 		})
 	}
+}
 
-	t.Run("different namespaces", func(t *testing.T) {
-		os.Setenv("TEKTON_INSTALL_NAMESPACE", "another-namespace")
-		t.Cleanup(func() { os.Unsetenv("TEKTON_INSTALL_NAMESPACE") })
-		in := &triggersv1.EventInterceptor{
-			Bitbucket: &triggersv1.BitbucketInterceptor{},
+func TestResolveToURL(t *testing.T) {
+	t.Run("resolves URL", func(t *testing.T) {
+		fakeGetter := func(n string) (*triggersv1.Interceptor, error) {
+			return &triggersv1.Interceptor{
+				Spec: triggersv1.InterceptorSpec{
+					ClientConfig: triggersv1.ClientConfig{
+						URL: &apis.URL{
+							Scheme: "http",
+							Host:   "some-host",
+							Path:   n,
+						},
+					},
+				},
+			}, nil
+		}
+
+		got, err := interceptors.ResolveToURL(fakeGetter, "cel")
+		if err != nil {
+			t.Fatalf("ResolveToURL() error: %s", err)
+		}
+		want := "http://some-host/cel"
+		if got.String() != want {
+			t.Fatalf("ResolveToURL want: %s; got: %s", want, got.String())
+		}
+	})
+
+	t.Run("interceptor has no URL", func(t *testing.T) {
+		fakeGetter := func(name string) (*triggersv1.Interceptor, error) {
+			return &triggersv1.Interceptor{
+				Spec: triggersv1.InterceptorSpec{
+					ClientConfig: triggersv1.ClientConfig{
+						URL: nil,
+					},
+				},
+			}, nil
 		}
-		got := interceptors.ResolveURL(in)
-		want := "http://tekton-triggers-core-interceptors.another-namespace.svc/bitbucket"
-		if want != got.String() {
-			t.Fatalf("ResolveURL() want: %s; got: %s", want, got)
+		_, err := interceptors.ResolveToURL(fakeGetter, "cel")
+		if !errors.Is(err, interceptors.ErrNoURL) {
+			t.Fatalf("ResolveToURL expected error to be %s but got %s", interceptors.ErrNoURL, err)
 		}
 	})
 }

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

@@ -26,7 +26,6 @@ import (
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1"
 	listers "github.com/tektoncd/triggers/pkg/client/listers/triggers/v1alpha1"
-	"github.com/tektoncd/triggers/pkg/system"
 	"go.uber.org/zap"
 	"golang.org/x/xerrors"
 	appsv1 "k8s.io/api/apps/v1"
@@ -463,9 +462,6 @@ func getContainer(el *v1alpha1.EventListener, c Config) corev1.Container {
 				FieldPath: "metadata.namespace",
 			},
 		},
-	}, {
-		Name:  "TEKTON_INSTALL_NAMESPACE",
-		Value: system.GetNamespace(),
 	}}
 
 	certEnv := map[string]*corev1.EnvVarSource{}

pkg/reconciler/v1alpha1/eventlistener/eventlistener_test.go

@@ -248,9 +248,6 @@ func makeDeployment(ops ...func(d *appsv1.Deployment)) *appsv1.Deployment {
 									FieldPath: "metadata.namespace",
 								},
 							},
-						}, {
-							Name:  "TEKTON_INSTALL_NAMESPACE",
-							Value: "tekton-pipelines",
 						}},
 					}},
 					Volumes: []corev1.Volume{{
@@ -336,9 +333,6 @@ var withTLSConfig = func(d *appsv1.Deployment) {
 					FieldPath: "metadata.namespace",
 				},
 			},
-		}, {
-			Name:  "TEKTON_INSTALL_NAMESPACE",
-			Value: "tekton-pipelines",
 		}, {
 			Name: "TLS_CERT",
 			ValueFrom: &corev1.EnvVarSource{

pkg/sink/sink.go

@@ -61,6 +61,7 @@ type Sink struct {
 	TriggerBindingLister        listers.TriggerBindingLister
 	ClusterTriggerBindingLister listers.ClusterTriggerBindingLister
 	TriggerTemplateLister       listers.TriggerTemplateLister
+	InterceptorLister           listers.InterceptorLister
 }
 
 // Response defines the HTTP body that the Sink responds to events with.
@@ -116,6 +117,8 @@ func (r Sink) HandleEvent(response http.ResponseWriter, request *http.Request) {
 	triggers, err := r.merge(el.Spec.Triggers, trItems)
 	if err != nil {
 		r.Logger.Errorf("Error merging Triggers: %s", err)
+		response.WriteHeader(http.StatusInternalServerError)
+		return
 	}
 	result := make(chan int, 10)
 	// Execute each Trigger
@@ -290,9 +293,14 @@ func (r Sink) ExecuteInterceptors(t triggersv1.Trigger, in *http.Request, event
 			request.Body = string(payload)
 			continue
 		}
-		// TODO: Plumb through context from EL
 		request.InterceptorParams = interceptors.GetInterceptorParams(i)
-		interceptorResponse, err := interceptors.Execute(context.Background(), r.HTTPClient, &request, interceptors.ResolveURL(i).String())
+		url, err := interceptors.ResolveToURL(r.InterceptorLister.Get, interceptors.GetName(i))
+		if err != nil {
+			return nil, nil, nil, fmt.Errorf("could not resolve interceptor URL: %w", err)
+		}
+
+		// TODO: Plumb through context from EL
+		interceptorResponse, err := interceptors.Execute(context.Background(), r.HTTPClient, &request, url.String())
 		if err != nil {
 			return nil, nil, nil, err
 		}