Add a CRD for Interceptors #921
Conversation
tekton-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
size/L
|
The following is the coverage report on the affected files.
|
tekton-robot
added
the
needs-rebase
tekton-robot
removed
the
needs-rebase
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
tekton-robot
removed
the
do-not-merge/work-in-progress
|
/assign @wlynch |
|
/cc @bigkevmcd |
| name: cel | ||
| spec: | ||
| clientConfig: | ||
| url: "http://tekton-triggers-core-interceptors.tekton-pipelines.svc/cel" |
There was a problem hiding this comment.
Letting the users set the path definitely seems useful, but it's unclear to me how users would know what Service URL this would be associated with, particularly if there are multiple interceptor services at play.
Few ideas:
- Is the full URL something we can expose in the status and let the Interceptor controller handle filling this in for clients?
- Should these CRDs be produced by Interceptor providers instead (e.g. on startup/control loop)? This has the added benefit of not requiring users to apply the interceptors themselves, sidestepping the apply ordering issues.
There was a problem hiding this comment.
end users do not have to know what the URL is. Only the interceptor provider. I implemented url first since that's the simplest but we will support a ref to an object too .
Is the full URL something we can expose in the status and let the Interceptor controller handle filling this in for clients?
Yes, was planning on adding this as part of the followup that adds service ref support. That being said, it might still make sense to have the same URL resolution happen at the EL level in case the EL receives an event before the Interceptor controller does a reconcile loop.
Should these CRDs be produced by Interceptor providers instead (e.g. on startup/control loop)? This has the added benefit of not requiring users to apply the interceptors themselves, sidestepping the apply ordering issues.
Interesting idea. Maybe? I think it will solve the issue only for the core interceptors that are bundled with the Triggers installation. Otherwise, it will be up to the interceptor provider to ship both the deployment running the logic and the CRD to go with it. They could decide to ship a deployment that creates the CRD on startup (but then that deployment will need to have RBAC roles to create Interceptors)
There was a problem hiding this comment.
but then that deployment will need to have RBAC roles to create Interceptors
mmmmm good point. Perhaps this is better then.
This commit adds the Interceptor CRD type as defined in TEP. The `spec` currently only contains a URL field to locate where the interceptor is running. Other fields will be added as they are implemented in follow ups. Part of tektoncd#868 Signed-off-by: Dibyo Mukherjee <[email protected]>
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]>
We need to wait for the Interceptors CRD to become established before we can create the core interceptors. Otherwise, we run the risk of trying to create the core-interceptors too early leading to installation errors. Signed-off-by: Dibyo Mukherjee <[email protected]>
|
New changes are detected. LGTM label has been removed. |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
OR We can create a symlink for the files present in |
| plural: interceptors | ||
| singular: interceptor | ||
| shortNames: | ||
| - i |
There was a problem hiding this comment.
I personally don't think this shortname is a good idea. I would prefer having none than a 1 character one.
There was a problem hiding this comment.
fair enough. I am also thinking of renaming the actual type from Interceptors to InterceptorType to make it clearer that this CR defines a new "type" or kind of interceptor that can then be used by a end user.
| apiVersion: triggers.tekton.dev/v1alpha1 | ||
| kind: Interceptor | ||
| metadata: | ||
| name: cel |
There was a problem hiding this comment.
Putting them in namespace tekton-pipelines would make sense to me. This is where basically everything else is, right? Especially since they are shipped together.
There was a problem hiding this comment.
Right now this is a cluster wide type i.e. you install the InterceptorType GitHub for your entire cluster. Similar to how you install pipelines for the entire cluster.
| metadata: | ||
| name: cel | ||
| spec: | ||
| clientConfig: |
There was a problem hiding this comment.
WDYT about omitting Config from the name and simply naming the field client?
Every CR is config to an extend and it's clear that this simply defines the client which handles the interceptor.
There was a problem hiding this comment.
I was basing this off of the webhook client config type that k8s webhook types : https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.20/#webhookclientconfig-v1-admissionregistration-k8s-io
On second thought, maybe I can just use that type itself instead of defining my own type for this.
|
|
||
| Spec InterceptorSpec `json:"spec"` | ||
| } | ||
|
|
| name: bitbucket | ||
| spec: | ||
| clientConfig: | ||
| url: "http://tekton-triggers-core-interceptors.tekton-pipelines.svc/bitbucket" |
There was a problem hiding this comment.
just thinking at high level
These interceptors are core interceptors which will be installed as part of triggers installation what if Tekton installs in different namespace other than tekton-pipelines 🤔
In that case will this be responsibility of user/admin who installs Triggers ?
There was a problem hiding this comment.
yeah this is a bit confusing right now. The idea is instead of url we'd also support a service field that contains a reference to the service for the deployed core interceptors. Custom installs would have to change the namespace of the service but would not have to modify a URL.
(I was thinking of adding that as a separate PR but maybe I'll add it to this one itself to make it less confusing).
|
/hold I'm going to split this into a couple of different PRs |
tekton-robot
added
the
needs-rebase
|
@dibyom: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Changes
Broken down into 2 commits for easier review:
Add Interceptor CRD type
The
speccurrently only contains a URL field to locate where the interceptoris running. Other fields will be added as they are implemented in follow ups.
Migrate core interceptors to use Interceptor CRD
Fix core interceptors installation errors
We need to wait for the Interceptors CRD to become established before we can
create the core interceptors. Otherwise, we run the risk of trying to create
the core-interceptors too early leading to installation errors.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Release Notes