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,15 +20,14 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"net/http"
-	"net/url"
 	"path"
 
-	"github.com/tektoncd/triggers/pkg/system"
-
 	"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"
@@ -172,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 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
 	}
-	return &url.URL{
-		Scheme: "http",
-		Host:   fmt.Sprintf("%s.%s.svc", CoreInterceptorsHost, system.DefaultNamespace),
-		Path:   path,
+	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,6 +18,7 @@ package interceptors_test
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
@@ -34,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"
 )
@@ -304,37 +306,80 @@ 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) {
-			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)
 			}
 		})
 	}
 }
 
+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
+		}
+		_, 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)
+		}
+	})
+}
+
 // testServer creates a httptest server with the passed in handler and returns a http.Client that
 // can be used to talk to these interceptors
 func testServer(t testing.TB, handler http.Handler) *http.Client {

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
 		}