Support for dynamic params to TriggerTemplates beyond just event values

[bobcatfish]

Expected Behavior

It should (maybe!) be possible for users to provide values to TriggerTemplate from sources other than events

Actual Behavior

A TriggerTemplate takes parameters, and doesn't need to worry about where those come from e.g.:

spec:
  params:
    - name: gitrevision
      description: The git revision
      default: master
    - name: gitrepositoryurl
      description: The git repository url
    - name: namespace
      description: The namespace to create the resources

At the moment these values can be provided only by a TriggerBinding which can extract them from json data (an event):

spec:
  params: # side note - are these outputs? i think thats the intention...
    - name: gitrevision
      value: $(event.head_commit.id)
    - name: gitrepositoryurl
      value: $(event.repository.url)
    - name: namespace
      value: tekton-pipelines

There is no other way to provide values for the params of a TriggerTemplate.

Steps to Reproduce the Problem

For dogfooding Tekton Pipelines, I wanted to make use of this the Pipeline we have been using for manual releases. However the Pipeline takes a parameter called versionTag:

https://github.com/tektoncd/pipeline/blob/3950521325d4744760a96c18e3d0c67d86495af3/tekton/publish.yaml#L14-L15

    - name: versionTag
      description: The vX.Y.Z version that the artifacts should be tagged with (including `v`)

There are different types of values we want for this:

• When we are creating official releases, we want to be able to extract the version from a combo of the name of the branch (e.g. v0.4) and number of commits so far (i.e. each subsequent commit bumps the patch value, e.g. v0.4.1)
• When we create nightly releases, we want to generate the version tag based on the date and the commit we are building from (e.g. vYYYYMMDD-commitsha)

With the current implementation for Triggers (and Prow!), in order to accomplish the above, we need to modify the Pipeline itself to do this. And we'd need either 2 totally different Pipelines or a flag to distinguish between the official + nightly use cases.

My opinion is that it's reasonable to create a Pipeline like this that takes a version parameter without being concerned with how to create it, and I think it makes sense for the Triggering system to be able to make this data available.

Additional info

Here is a proposal for how we could do this:

1. TriggerBinding would specifically be about extracting values from Events only, so it could be renamed (Proposal to rename TriggerBinding #62) to something like EventParser or ParmsFromEvent or something better but specifcally about "events" - EventBinding?
2. We could provide an interface (you guessed it, a container!) for users to provide additional parameters.

E.g. EventListener could look like this:

apiVersion: tekton.dev/v1alpha1
kind: EventListener
metadata:
  name: listener
  namespace: tekton-pipelines
spec:
  serviceAccountName: default
  triggers:
    - bindings:
        - name: github-binding
        - name: nightly-version-binding 
      template:
        name: pipeline-template

nightly-version-binding would be an instance of at type called something like ParamBinding that is a container specification - the interface would be something like the container has to write a json file to an expected location with the values it exports, e.g. something like:

apiVersion: tekton.dev/v1alpha1
kind: ParamBinding
metadata:
  name: nightly-version-binding
spec:
  outputs:
   - "versionTag"
  container:
  - name: generate-release-version
    image: alpine/git
    workingDir: "/workspace/go/src/github.com/bobcatfish/catservice/"
    command:
    - /bin/sh
    args:
    - -ce
    - |
      set -e
      set -x

      COMMIT=$(git rev-parse HEAD | cut -c 1-10)
      DATE=$(date +"%Y%m%d")
      VERSION_TAG="$DATE-$COMMIT"

      echo "{'versionTag': '$VERSION_TAG'}" > "/builder/home/outputs.json"

3. In the future, we could even extend this to support cases like filtering! For example @wlynch has been looking into use cases where we need additional info from github to make a decision, e.g. "is the user who opened the PR a member of the organziation". We could use something like this to provide inputs to the filtering logic.

4. And this could be extended to allow for caching info - to continue the use case from (3), say we wanted to know if the user who opened the PR is a member of the org - we could have a mechanism that is caching data like this from GitHub.

What I like about (3) and (4) is that it make it possible to separate the responsibilities of obtaining data from the responsibility of acting on that data.

[wlynch]

I like it!

Might be a noob questions, but would we need to explicitly list all fields in outputs? It seems like it would be great if we could treat the output of the step similar to the event in TriggerTemplates.

e.g. for the examples you gave above, being able to reference the ParamBinding in the TriggerBinding:

spec:
  params: # side note - are these outputs? i think thats the intention...
    - name: version
      value: $(event.head_commit.id)
    - name: version
      value: $(nightly-version-binding.versionTag)

For filtering, this may not solve all problems since we might want to make some bindings conditional (e.g. don't call out to GitHub to check membership if we don't even care about the repo).

[vtereso]

In the case of creating a release, there is no event to process? A "manual" TaskRun/Pipeline could use whatever parameters necessary to do this. Alternatively, if a tag was added to the repository, a "tag trigger" (or some such event that contains the necessary params) could take this event information and work as current to upload a release?

[vtereso]

I think having additional parameters exterior to the event might be unavoidable though and we wouldn't want to ruin re-usability. To this point, the initial thought is to push more into the EventListener since it isn't reusable.

[vtereso]

Also, this gets at the discussion of reusability in general and how much there really is. Spoke with @ncskier about this yesterday and a TriggerTemplate can only be as reusable as the resources it is templating (e.g. a Pipeline). I can easily be swayed to the pursuit of reusability, but I don't believe the case is as strong as with core Tekton. If tools like tkn or the skylark approach pick up steam, maybe a catalog is unnecessary and reusability becomes less important? Just a thought.

[vtereso]

@bobcatfish Do you feel that this has been "solved" by adding interceptors as well as static parameters in the EventListener?

[afrittoli]

The way I implemented this for now is an initContainer in the CronJob that triggers the EventListener: https://github.com/tektoncd/plumbing/blob/master/tekton/config/nightly-release-cron-base/trigger-with-uuid.yaml#L35.

I could do the same with an interceptor, but it's not clear for me yet how to best use them.
Since interceptors are services, if I have to create a new service for every time I need to inject an extra parameter, I will end up with a lot of services that do nothing most of the time.
I could use one interceptor to handle different pipelines, but then I would lose the separation of concerns, and my interceptor would become more complex and hard to maintain.

[ncskier]

In PR #438, a discussion started about the use-case of chaining Interceptors: #438 (comment)

@ncskier A use case for chained interceptors - validate using a Github interceptors, add a webhook interceptor to call out to GH for adding additional info, and then perhaps a CEL filter. I think these would be fairly common.

The use-case described made me think about this issue, and whether it should be the job of an Interceptor to "call out" for additional information, or if we need a separate mechanism in Triggers for this.

Interceptors were initially built as a way to validate events. However, their role has now grown to include calling out for additional information (the feature called "dynamic params" in this issue).

If we're happy with Interceptors taking on this dual-role, then we can close this issue. However, I think that it would be helpful to revisit this issue, because @afrittoli raised some good points about why we might not want to use Interceptors to get "dynamic params" :

Since interceptors are services, if I have to create a new service for every time I need to inject an extra parameter, I will end up with a lot of services that do nothing most of the time.
I could use one interceptor to handle different pipelines, but then I would lose the separation of concerns, and my interceptor would become more complex and hard to maintain.

I'm interested to know if anything about this issue has changed since it was last discussed in October 😅

[wlynch]

I think with the inclusion of WebhookInterceptors we can close this out.

Since interceptors are services, if I have to create a new service for every time I need to inject an extra parameter, I will end up with a lot of services that do nothing most of the time.
I could use one interceptor to handle different pipelines, but then I would lose the separation of concerns, and my interceptor would become more complex and hard to maintain.

I think #270 will help this. You can either provide a Service, or a URL (where you could host multiple interceptor endpoints). I suspect possibly adding a URL path to Services may also help this out this particular case, this way you could deploy a single Service and route accordingly.

I suspect this is an area we can learn from Knative Addressables (e.g. https://github.com/knative/pkg/blob/d9a38f13e8b9aa736f714b793ee28788de1b30e0/apis/duck/v1beta1/destination.go#L26-L47)

[tekton-robot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

[tekton-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[bobcatfish]

I feel like interceptors are the answer to this these days 🤔 , do you agree @dibyom @wlynch ?

[dibyom]

I agree. Interceptors (both CEL overlays + Webhooks) can modify the event body and add extra "dynamic" parameters