Add ClusterInterceptor CRD for registering interceptors #960

dibyom · 2021-02-17T19:32:42Z

Changes

This commit adds a new CRD type called ClusterInterceptor. In TEP-0026 this
type was called InterceptorConfig

The spec currently only contains a clientConfig field to locate where
the interceptoris running. Other fields will be added as they are
implemented in follow ups.

This commit also adds a simple reconciler for this type that resolves
the clientConfig to a URL and adds it to the status.address.url field.

Part of #868

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

A new CRD called ClusterInterceptor has been added that allows for users to register new Interceptor types.

tekton-robot · 2021-02-17T19:34:22Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/triggers/v1alpha1/interceptor_type_defaults.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/interceptor_type_types.go	Do not exist	0.0%
pkg/apis/triggers/v1alpha1/interceptor_type_validation.go	Do not exist	100.0%

tekton-robot · 2021-02-17T21:52:30Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/triggers/v1alpha1/interceptor_type_defaults.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/interceptor_type_types.go	Do not exist	0.0%
pkg/apis/triggers/v1alpha1/interceptor_type_validation.go	Do not exist	100.0%

tekton-robot · 2021-02-17T22:47:26Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/triggers/v1alpha1/interceptor_type_defaults.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/interceptor_type_types.go	Do not exist	88.9%
pkg/apis/triggers/v1alpha1/interceptor_type_validation.go	Do not exist	100.0%

tekton-robot · 2021-02-18T03:55:35Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/triggers/v1alpha1/interceptor_type_defaults.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/interceptor_type_types.go	Do not exist	88.9%
pkg/apis/triggers/v1alpha1/interceptor_type_validation.go	Do not exist	100.0%
pkg/reconciler/v1alpha1/interceptortype/controller.go	Do not exist	0.0%
pkg/reconciler/v1alpha1/interceptortype/interceptortype.go	Do not exist	90.9%

tekton-robot · 2021-02-18T04:54:31Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/triggers/v1alpha1/interceptor_type_defaults.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/interceptor_type_types.go	Do not exist	88.9%
pkg/apis/triggers/v1alpha1/interceptor_type_validation.go	Do not exist	100.0%
pkg/reconciler/v1alpha1/interceptortype/controller.go	Do not exist	0.0%
pkg/reconciler/v1alpha1/interceptortype/interceptortype.go	Do not exist	90.9%

tekton-robot · 2021-02-18T05:00:04Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/triggers/v1alpha1/interceptor_type_defaults.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/interceptor_type_types.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/interceptor_type_validation.go	Do not exist	100.0%
pkg/reconciler/v1alpha1/interceptortype/controller.go	Do not exist	0.0%
pkg/reconciler/v1alpha1/interceptortype/interceptortype.go	Do not exist	90.9%

dibyom · 2021-02-18T15:53:57Z

/cc @savitaashture @wlynch @MarcelMue @bigkevmcd

I broke off #921 into multiple PRs. One change I did was to call the CRD InterceptorType instead of just Interceptor to indicate that it defines a new type or kind of interceptor.

tekton-robot · 2021-02-18T19:16:33Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/triggers/v1alpha1/interceptor_type_defaults.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/interceptor_type_types.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/interceptor_type_validation.go	Do not exist	100.0%
pkg/reconciler/v1alpha1/interceptortype/controller.go	Do not exist	0.0%
pkg/reconciler/v1alpha1/interceptortype/interceptortype.go	Do not exist	90.9%

wlynch · 2021-02-18T23:01:43Z

docs/interceptortypes.md

+-->
+# InterceptorType
+
+A `InterceptorType` is cluster scoped resource that registers a new Interceptor that 


I'm not 100% sure we want this to be cluster scoped. In particular, I'm thinking about how we can expose custom interceptors that can only be used within a single tenant namespace.

If its not cluster scoped, each new namespace would have to install their own version of Interceptors....could we not use RBAC to restrict if certain interceptors need to be limited?

chatted with @wlynch about this. We will start off with a cluster scoped CRD but will implement a Namespace scoped version of this in the future as well.

Thanks! Let's update the docs to reflect the change.

wlynch · 2021-02-18T23:12:11Z

pkg/apis/triggers/v1alpha1/interceptor_type_types.go

+
+// InterceptorTypeSpec describes the Spec for an InterceptorType
+type InterceptorTypeSpec struct {
+	ClientConfig ClientConfig `json:"clientConfig"`


Should we reuse WebhookClientConfig?

At the very least we can likely reuse the ServiceReference if we want to use the apis.URL for URL. The cert data is also useful for HTTPS.

I think using a WebhookClientConfig is a good idea - maybe just reimplementing the type here if we are concerned about dependencies.

yeah, I am basically reimplementing the type here....two differences:

using apis.URL instead of string for the URL type for better validation/json marshalling

we do not have a caCert field type (yet!) for HTTPS

Pulling in a new dependency for just the two structs seems much so I decided to copy the structs. It also allows us to make changes (e.g. making the port a *int instead of int as suggested below)

wlynch · 2021-02-18T23:15:51Z

docs/interceptortypes.md

+
+## Syntax
+
+To define a configuration file for an `Interceptor` resource, you can specify


bikeshed: I kind of like InterceptorConfiguration to be consistent with ValidatingWebhookConfiguration.

Other good ideas to borrow from admission webhooks we should look into later:

Version info (e.g. have different routes based on interceptor version)

Timeout

hmmm yeah...this CRD is definitely based on the WebhookConfiguration CRDs....I don't have a strong preference on calling this InterceptorConfguration or something else --> I thought it was a bit confusing in the sense its not clear if it is some interceptor configuration that can be reused within a trigger vs configuration for a new kind of interceptor.

The InterceptorType name can be a bit awkward a times too though like we now have a file called interceptor_type_types.go

I personally don't love the Configuration suffix. It makes sense if we have a different Interceptor object but a Configuration suffix without an Interceptor CRD seems weird to me.

No strong opinions though.

docs/interceptortypes.md

pkg/apis/triggers/v1alpha1/interceptor_type_defaults.go

tekton-robot · 2021-02-19T22:51:51Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/triggers/v1alpha1/interceptor_type_defaults.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/interceptor_type_types.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/interceptor_type_validation.go	Do not exist	100.0%
pkg/reconciler/v1alpha1/interceptortype/controller.go	Do not exist	0.0%
pkg/reconciler/v1alpha1/interceptortype/interceptortype.go	Do not exist	90.9%

wlynch

/lgtm w/ minor doc changes.

Since this is a new type, would appreciate a second pair of eyes as an approver!

wlynch · 2021-02-24T23:20:13Z

docs/clusterinterceptors.md

+- [Interceptors](#interceptors)
+  - [Syntax](#syntax)
+    - [clientConfig](#clientConfig)
+
+## Syntax
+
+To define a configuration file for an `Interceptor` resource, you can specify
+the following fields:
+
+- Required:
+  - [`apiVersion`][kubernetes-overview] - Specifies the API version, for example
+    `triggers.tekton.dev/v1alpha1`.
+  - [`kind`][kubernetes-overview] - Specifies the `Trigger` resource
+    object.
+  - [`metadata`][kubernetes-overview] - Specifies data to uniquely identify the
+    `Interceptor` resource object, for example a `name`.
+  - [`spec`][kubernetes-overview] - Specifies the configuration information for
+    your Interceptor resource object. The spec include:
+    - [`clientConfig`] -  Specifies how a client (e.g. an EventListener) can communicate with the Interceptor.


Few more Interceptor -> ClusterInterceptor replacements need to be made in here.

dibyom · 2021-02-26T19:12:29Z

/cc @sergetron for a docs review!

tekton-robot · 2021-02-26T19:14:06Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/triggers/v1alpha1/cluster_interceptor_defaults.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/cluster_interceptor_types.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/cluster_interceptor_validation.go	Do not exist	100.0%
pkg/reconciler/v1alpha1/clusterinterceptor/clusterinterceptor.go	Do not exist	90.9%
pkg/reconciler/v1alpha1/clusterinterceptor/controller.go	Do not exist	0.0%

tekton-robot · 2021-03-04T19:49:33Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/triggers/v1alpha1/cluster_interceptor_defaults.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/cluster_interceptor_types.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/cluster_interceptor_validation.go	Do not exist	100.0%
pkg/reconciler/v1alpha1/clusterinterceptor/clusterinterceptor.go	Do not exist	90.9%
pkg/reconciler/v1alpha1/clusterinterceptor/controller.go	Do not exist	0.0%

tekton-robot · 2021-03-10T15:08:13Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/triggers/v1alpha1/cluster_interceptor_defaults.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/cluster_interceptor_types.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/cluster_interceptor_validation.go	Do not exist	100.0%
pkg/reconciler/v1alpha1/clusterinterceptor/clusterinterceptor.go	Do not exist	90.9%
pkg/reconciler/v1alpha1/clusterinterceptor/controller.go	Do not exist	0.0%

dibyom · 2021-03-10T16:28:04Z

/test pull-tekton-triggers-integration-tests

pkg/apis/triggers/v1alpha1/cluster_interceptor_defaults.go

pkg/apis/triggers/v1alpha1/cluster_interceptor_defaults_test.go

pkg/apis/triggers/v1alpha1/cluster_interceptor_types.go

pkg/apis/triggers/v1alpha1/cluster_interceptor_types_test.go

pkg/apis/triggers/v1alpha1/cluster_interceptor_validation.go

savitaashture

/lgtm

This commit adds a new CRD type called ClusterInterceptor. In TEP-0026 this type was called InterceptorConfiguration The `spec` currently only contains a clientConfig field to locate where the interceptor is running. Other fields will be added as they are implemented in follow ups. This commit also adds a simple reconciler for this type that resolves the clientConfig to a URL and adds it to the `status.address.url` field. Part of tektoncd#868 Signed-off-by: Dibyo Mukherjee <[email protected]>

tekton-robot · 2021-03-11T16:49:01Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/triggers/v1alpha1/cluster_interceptor_defaults.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/cluster_interceptor_types.go	Do not exist	100.0%
pkg/apis/triggers/v1alpha1/cluster_interceptor_validation.go	Do not exist	100.0%
pkg/reconciler/v1alpha1/clusterinterceptor/clusterinterceptor.go	Do not exist	90.9%
pkg/reconciler/v1alpha1/clusterinterceptor/controller.go	Do not exist	0.0%

wlynch

/lgtm

tekton-robot · 2021-03-11T17:02:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: savitaashture, wlynch

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [savitaashture,wlynch]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 17, 2021

tekton-robot requested review from bobcatfish and ncskier February 17, 2021 19:32

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 17, 2021

dibyom force-pushed the itype branch from 5b10dd7 to 75acab0 Compare February 17, 2021 21:51

dibyom force-pushed the itype branch from 75acab0 to 6116a65 Compare February 17, 2021 22:45

dibyom force-pushed the itype branch from 6116a65 to d2f6958 Compare February 18, 2021 03:54

dibyom force-pushed the itype branch from d2f6958 to 12eca86 Compare February 18, 2021 04:52

dibyom force-pushed the itype branch from 12eca86 to 572e2a4 Compare February 18, 2021 04:58

dibyom marked this pull request as ready for review February 18, 2021 15:52

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 18, 2021

tekton-robot requested review from bigkevmcd, MarcelMue, savitaashture and wlynch February 18, 2021 15:53

dibyom force-pushed the itype branch from 572e2a4 to 742dbd0 Compare February 18, 2021 19:15

dibyom assigned wlynch and savitaashture Feb 18, 2021

wlynch reviewed Feb 18, 2021

View reviewed changes

dibyom force-pushed the itype branch from 742dbd0 to 1e3d813 Compare February 19, 2021 22:50

dibyom changed the title ~~Add InterceptorType as a cluster scoped CRD for registering interceptors~~ Add ClusterInterceptor as a cluster scoped CRD for registering interceptors Feb 24, 2021

dibyom changed the title ~~Add ClusterInterceptor as a cluster scoped CRD for registering interceptors~~ Add ClusterInterceptor CRD for registering interceptors Feb 24, 2021

wlynch reviewed Feb 24, 2021

View reviewed changes

dibyom mentioned this pull request Feb 25, 2021

Write a new Tekton Triggers Overview #972

Merged

4 tasks

dibyom force-pushed the itype branch from 09ceca4 to 9667090 Compare February 26, 2021 19:12

This was referenced Feb 26, 2021

Migrate core interceptors to use InterceptorType CRD #976

Merged

Add a CRD for Interceptors #921

Closed

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2021

dibyom force-pushed the itype branch from 9667090 to 857ecff Compare March 4, 2021 19:48

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2021

Base automatically changed from master to main March 10, 2021 15:03

savitaashture reviewed Mar 10, 2021

View reviewed changes

savitaashture reviewed Mar 11, 2021

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2021

savitaashture approved these changes Mar 11, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2021

dibyom force-pushed the itype branch from 857ecff to b261fc9 Compare March 11, 2021 16:47

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2021

wlynch approved these changes Mar 11, 2021

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2021

tekton-robot merged commit 61544a0 into tektoncd:main Mar 11, 2021

dibyom deleted the itype branch March 18, 2021 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ClusterInterceptor CRD for registering interceptors #960

Add ClusterInterceptor CRD for registering interceptors #960

dibyom commented Feb 17, 2021 •

edited

Loading

tekton-robot commented Feb 17, 2021

tekton-robot commented Feb 17, 2021

tekton-robot commented Feb 17, 2021

tekton-robot commented Feb 18, 2021

tekton-robot commented Feb 18, 2021

tekton-robot commented Feb 18, 2021

dibyom commented Feb 18, 2021

tekton-robot commented Feb 18, 2021

wlynch Feb 18, 2021

dibyom Feb 19, 2021

dibyom Feb 23, 2021

wlynch Feb 23, 2021

wlynch Feb 18, 2021

MarcelMue Feb 19, 2021

dibyom Feb 19, 2021

wlynch Feb 18, 2021

dibyom Feb 19, 2021 •

edited

Loading

MarcelMue Feb 23, 2021

tekton-robot commented Feb 19, 2021

wlynch left a comment

wlynch Feb 24, 2021

dibyom commented Feb 26, 2021

tekton-robot commented Feb 26, 2021

tekton-robot commented Mar 4, 2021

tekton-robot commented Mar 10, 2021

dibyom commented Mar 10, 2021

savitaashture left a comment

tekton-robot commented Mar 11, 2021

wlynch left a comment

tekton-robot commented Mar 11, 2021


		## Syntax

		To define a configuration file for an `Interceptor` resource, you can specify

Add ClusterInterceptor CRD for registering interceptors #960

Add ClusterInterceptor CRD for registering interceptors #960

Conversation

dibyom commented Feb 17, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Feb 17, 2021

tekton-robot commented Feb 17, 2021

tekton-robot commented Feb 17, 2021

tekton-robot commented Feb 18, 2021

tekton-robot commented Feb 18, 2021

tekton-robot commented Feb 18, 2021

dibyom commented Feb 18, 2021

tekton-robot commented Feb 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibyom Feb 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Feb 19, 2021

wlynch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibyom commented Feb 26, 2021

tekton-robot commented Feb 26, 2021

tekton-robot commented Mar 4, 2021

tekton-robot commented Mar 10, 2021

dibyom commented Mar 10, 2021

savitaashture left a comment

Choose a reason for hiding this comment

tekton-robot commented Mar 11, 2021

wlynch left a comment

Choose a reason for hiding this comment

tekton-robot commented Mar 11, 2021

dibyom commented Feb 17, 2021 •

edited

Loading

dibyom Feb 19, 2021 •

edited

Loading