SHIP-0031: Shipwright Trigger

[otaviof]

Shipwright Trigger (EP)

This enhancement proposal describes how to receive external events and trigger Shipwright Builds base on those. The three primary mechanisms described are:

• WebHook: Handles WebHook requests from Git service providers, like GitHub, in order to activate a Build
• Tekton Pipelines: Integrates Tekton Pipelines into Shipwright, Builds will be triggered when a given Pipeline has reach the desired status
• Tekton Custom-Tasks (Run): Integrates Shipwright into Tekton Pipelines via Custom-Tasks, allowing users to call out Shipwright Builds directly from pipelines_

Prototype

The proposal on this document is based on the protype project shipwright-trigger, and the API changes on PR #1008.

WebHook Support

The example uses a GitHub repository which upon receiving new commits (push), it triggers a new BuildRun using the WebHook interface.

Shipwright Build with Push Trigger

---
apiVersion: shipwright.io/v1alpha1
kind: Build
spec:
  trigger:
    secretRef:
      name: webhook-secret
    when:
      - name: push directly on the main branch
        type: Push
        branches:
          - main

Tekton Pipelines Integration

Pipelines

On the Tekton integration demo, the following trigger is added on the Build resource:

---
apiVersion: shipwright.io/v1alpha1
kind: Build
spec:
  trigger:
    when:
      - name: tekton pipeline has "succeeded"
        type: Pipeline
        objectRef:
          name: pipeline-ex
          status:
            - Succeeded

And the following Tekton resources are used, please consider.

Tekton Pipeline

---
apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: pipeline-ex
spec:
  tasks:
    - name: example
      taskSpec:
        steps:
          - image: busybox
            script: echo "example task"

Custom Tasks

For the Custom-Tasks demo, the following Pipeline was employed.

Tekton Pipeline (Custom-Tasks)

---
apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: shipwright-ex
spec:
  tasks:
    - name: before
      taskSpec:
        steps:
          - image: busybox
            script: echo "before buildrun"
    - name: shipwright
      taskRef:
        apiVersion: shipwright.io/v1alpha1
        kind: Build
        name: nodejs-ex
      runAfter:
        - before
    - name: after
      taskSpec:
        steps:
          - image: busybox
            script: echo "after buildrun"
      runAfter: 
        - shipwright

[otaviof]

/retitle SHIP-0031: Shipwright Trigger

[otaviof]

/assign @qu1queee @sbose78 @imjasonh

/cc @SaschaSchwarze0 @HeavyWombat @ricardomaraschini @gabemontero @coreydaley @adambkaplan

[openshift-ci]

@otaviof: GitHub didn't allow me to request PR reviews from the following users: ricardomaraschini.

Note that only shipwright-io members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/assign @qu1queee @sbose78 @imjasonh

/cc @SaschaSchwarze0 @HeavyWombat @ricardomaraschini @gabemontero @coreydaley @adambkaplan

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.

[ricardomaraschini]

What about making this a resource on its own? IIUC the way things are designed here it seems like we would be introducing fields to the Build struct that would be meaningful only to this new trigger project (or operator/controller if I may).

This is just food for thought: what about having a new type called BuildTrigger that refers to a Build by its name and then have this new type living only in the new trigger repo ? I am pretty new to the project so please educate me on why this is not a good idea.

[otaviof]

What about making this a resource on its own? IIUC the way things are designed here it seems like we would be introducing fields to the Build struct that would be meaningful only to this new trigger project (or operator/controller if I may).

There's an intimate relationship between the Build resource and the Git repository itself. For instance, for GitHub WebHook you can employ a shared secret between the repository and the Shipwright Build, in order to validate the caller, increase security.

So, the attribute .spec.trigger.secretRef can be unique per Build, and in case it's shared between more resources, it would be shared via the Secret resource (name).

The .spec.trigger.when[] could be common for more Build resources, indeed. However, I still look at it as means to trigger that specific Build, so once again an "intimate relationship".

[ricardomaraschini]

Missing Image here ?

[otaviof]

We might as well say all the resources involved with the Shipwright Trigger controllers. And for the record, I still need to prototype the Image controller, we could explore this together if you're interested.

[otaviof]

tektoncd/community#523 Custom Tasks Graduation

[SaschaSchwarze0]

Nice write-up.

[SaschaSchwarze0]

I like the pragmatic simplicity here. I think we cover the 99.9 % use case by assuming that there is exactly one Git source (not sure if we ever allow anything else anyway) as I guess we take the repository URL from the source?

Regarding the branches. Is that mandatory? If not providing it, would it take the revision from the source as default? If the branch here is not the source revision, then the BuildRun will need to override this.

[gabemontero]

FWIW the separation of branch references from source revision, context dir, etc. is what was done with the openshift build API.

https://github.com/openshift/api/blob/76ab2e0a141066de3550b89e7376783ae8d2c8b9/build/v1/types.go#L445 and https://github.com/openshift/api/blob/76ab2e0a141066de3550b89e7376783ae8d2c8b9/build/v1/types.go#L600 for reference

Having to specify a branch for certain should NOT BE mandatory. OpenShift Builds has worked that way since day one.

A comment or footnote making that clear feels warranted @otaviof

[otaviof]

I like the pragmatic simplicity here. I think we cover the 99.9 % use case by assuming that there is exactly one Git source (not sure if we ever allow anything else anyway) as I guess we take the repository URL from the source?

Thanks! I think the same API semantics will work for more than a single Git source, given the WebHook event carries on the Git repository references. Based what's provided the WebHook payload we can find all the Builds referring to it.

Note that we may have more than a single Build referring to the Git respository, and therefore the search needs to take the actual triggers in consideration.

Regarding the branches. Is that mandatory? If not providing it, would it take the revision from the source as default? If the branch here is not the source revision, then the BuildRun will need to override this.

Yes, I think that's a good solution! It also matches what @gabemontero brought up, based on how OpenShift works.

[SaschaSchwarze0]

This scenario is important, but is mentioned the first time here while not mentioned in the motivation, or goals. Later there are more details that this works only for images that are managed by the shp image controller.

[otaviof]

Sure! I'll add that up on the goals and more information about the future plans to integrate with Image.

[SaschaSchwarze0]

The secret token is GitHub specific. Is directly below trigger a good location or should it be in the when(s) that are Git source related?

[otaviof]

The WebHook shared secret is directly related to the Git repository itself, so maybe we could avoid adding that under .spec.trigger and let it be part of .spec.source.credentials.

Currently, it looks for a predefined key (github-secret). What do you think about merging this with .spec.source.credentials secret?

[SaschaSchwarze0]

For the Tekton Pipelines watch based trigger, I am looking for a good scenario. EDIT: I read what you have later, but I am still not seeing a good use case. If I would have to chose between a Tekton pipeline that does unit tests, completes, and then a BuildRun starts; or a pipeline that does unit tests and then has a custom task for the shipwright build, then I would always use the latter option.

[otaviof]

I hope that yesterday's community meeting we've been able to discuss about this scenario, let me know if the use case needs to be more descriptive on the EP.

[SaschaSchwarze0]

I lover that we introduce our own webhook handler instead of trying to somehow make this working with Tekton Triggers. It will be much simpler like that for our users.

[SaschaSchwarze0]

Can you please elaborate more the configuration need here? A simple solution would be that our service has a /github endpoint for GitHub and eventually /gitlab for GitLab etc.

[SaschaSchwarze0]

As mentioned earlier, I do not think that the term controller is suitable here because we do not do anything (beside caching) when some Build changes. On the other hand, Tekton Run is really a controller because we do something (setup BuildRun, update Run status).

[otaviof]

I'm borrowing the term "controller" from Kubernetes, as in a actor which watches over a resource moving it to the desired state. In this case the so called "Build Controller" is watching over Builds and feeding the Inventory... what do you think would be a more suitable name to it?

/cc @adambkaplan @gabemontero

[gabemontero]

Made a few initial comments @otaviof

I wouldn't say this is a full review on my part, since I believe you have changes pending from @SaschaSchwarze0 's comments, some of which lined up with ones I was thinking of, and you might have a big enough change coming that it would be prudent to cycle through it again after that.

In particular, I'm curious on where you go with my "triggered by" comment, and the ripple effect it has.

[gabemontero]

I think some explanation and detail on why we are not going with Tekton Triggers is warranted here. Folks outside of our circle who have not been part of our video conferences or slack discussion quite possibly are going to ask that question, and should be able to discover our rationale here.

[otaviof]

Indeed! I'm using the initial work done on #41 as input to explain what we've spoken, so please consider the latest update.

[gabemontero]

FWIW the separation of branch references from source revision, context dir, etc. is what was done with the openshift build API.

https://github.com/openshift/api/blob/76ab2e0a141066de3550b89e7376783ae8d2c8b9/build/v1/types.go#L445 and https://github.com/openshift/api/blob/76ab2e0a141066de3550b89e7376783ae8d2c8b9/build/v1/types.go#L600 for reference

Having to specify a branch for certain should NOT BE mandatory. OpenShift Builds has worked that way since day one.

A comment or footnote making that clear feels warranted @otaviof

[gabemontero]

Yeah I was trying to figure out where status was being managed, and then noticed @SaschaSchwarze0 's comment here.

It would not be in this piece of function you are describing here per se, but again, I was not sure where to put my comment, but as it had some synergy with @SaschaSchwarze0 's ....

I was expecting either new conditions or fields in status (I don't have a strong opinion either way) that captured that his BuildRun was "triggered by" either a webhook, image change, pipeline completion, etc. Analogous to the various trigger by semantics OpenShift Builds has in its BuildRequest: https://github.com/gabemontero/api/blob/f60a0b2883eae330c01d294b3eba2ee9c3fe7a47/build/v1/types.go#L1236-L1269

Also I'm not seeing anything along those lines in https://github.com/shipwright-io/build/pull/1008/files

[lizzzcai]

Hi @otaviof, nice feature! I have a few questions:

1. can you provide more examples about the trigger with Shipwright Build Parameters for example like passing the commit info/version from github/pipelines into my build.
2. is it possible or plan to support triggers from other pipelines/workflow like ArgoCD, as it is a very popular project in the market as well.

thanks.

[otaviof]

1. can you provide more examples about the trigger with [Shipwright Build Parameters](https://github.com/shipwright-io/build/blob/main/docs/build.md#defining-paramvalues) for example like passing the commit info/version from github/pipelines into my build.

Sure! I just modified the Tekton Custom-Tasks section to reflect a real world example, please let me know if it's clear of how the parameters are transferred from the Run instance to Shipwright.

2. is it possible or plan to support triggers from other pipelines/workflow like ArgoCD, as it is a very popular project in the market as well.

That's a great idea! Would you have pointers of how such integration works in ArgoCD? Thanks in advance 👍

[lizzzcai]

Hi @otaviof , the example looks good.

For ArgoCD, one is a custom trigger for shipwright: https://argoproj.github.io/argo-events/sensors/triggers/build-your-own-trigger/
So that can use the argo event to trigger a Build like this: https://argoproj.github.io/argo-events/sensors/triggers/k8s-object-trigger/

Another is just like tekton pipeline, to trigger Build by an Argo workflow. https://argoproj.github.io/argo-workflows/workflow-templates/

apiVersion: shipwright.io/v1alpha1
kind: Build
spec:
  trigger:
    when:
      - name: nodejs-ex workflow has succeeded
        type: Workflow
        objectRef:
          name: nodejs-ex
          status:
            - Succeeded

For custom task, I didn't see the similar concept. But that's good enough already.

[otaviof]

For the record, GitHub sends Actions Workflow events on the same WebHook channel. The design on this proposal is able to support such type of events as well.

[otaviof]

Hi @otaviof , the example looks good.

For ArgoCD, one is a custom trigger for shipwright: https://argoproj.github.io/argo-events/sensors/triggers/build-your-own-trigger/ So that can use the argo event to trigger a Build like this: https://argoproj.github.io/argo-events/sensors/triggers/k8s-object-trigger/

Another is just like tekton pipeline, to trigger Build by an Argo workflow. https://argoproj.github.io/argo-workflows/workflow-templates/

apiVersion: shipwright.io/v1alpha1
kind: Build
spec:
  trigger:
    when:
      - name: nodejs-ex workflow has succeeded
        type: Workflow
        objectRef:
          name: nodejs-ex
          status:
            - Succeeded

For custom task, I didn't see the similar concept. But that's good enough already.

Thanks for the suggestion, indeed the .objectRef construction can also work against ArgoCD Workflow executions ❤️ I think we should keep this integration in the backlog, and pick it up as soon as we start working on the Triggers project. Would be great to have a issue created on which we can discuss during communit meetings and refinement.

Please consider the contact information, we send the meeting invites on the mailing lists.

[otaviof]

Made a few initial comments @otaviof

I wouldn't say this is a full review on my part, since I believe you have changes pending from @SaschaSchwarze0 's comments, some of which lined up with ones I was thinking of, and you might have a big enough change coming that it would be prudent to cycle through it again after that.

In particular, I'm curious on where you go with my "triggered by" comment, and the ripple effect it has.

Please consider the latest chances, I've added a Trigger Cause section to explain how the information will be propagated until end up on the BuildRun status conditions.

[gabemontero]

good progress on the Trigger Cause stuff @otaviof

add a couple of comments around yaml based examples

[gabemontero]

put together a yaml snippet to illustrate this

[otaviof]

Please consider the last changes.

[gabemontero]

OK I don't see any yaml for this point on status and conditions @otaviof

I was expecting, to go along with your yaml examples above, something like

a new constant for condition type like

Triggered Type = "Triggered"

and then

apiVersion: shipwright.io/v1alpha1
kind: BuildRun

...

status:
  conditions:
  - type: Triggered
   status: 
   reason:
   message:

To go along with your statement "The trigger cause must be part of a status condition ..."

[otaviof]

It should be resolve as per: https://github.com/otaviof/community/blob/467cea4c797411ecfcc8e57facf7495b434cd15b/ships/0031-shipwright-trigger.md?plain=1#L297-L301, please consider.

[adambkaplan]

/approve

[adambkaplan]

I recommend that we have separate API objects for each trigger type:

• .when[].pullRequest.branches[]
• .when[].image.images[]
• .when[].pipeline.name

This lets us employ type discriminators and add validation to the API (ex - the Git trigger type should not have any objectRef values)

I won't block merge on this - we can discuss in further detail when the full API is implemented.

[SaschaSchwarze0]

I tend to agree. We do this now with volumes where we inherit the Kubernetes VolumeSource. And imo that's also what we should go to with spec.sources in our future API evolvement.

[otaviof]

Thanks for the recommendation, @adambkaplan. I've adjusted the EP accordingly, and as well the prototype changes. Please consider.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan

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 [adambkaplan]

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

[adambkaplan]

I have created a GitHub discussion so we can think beyond this proposal. It is clear that we will want more mechanisms to trigger builds.

Therefore, I would consider this proposal a "first attempt", where we should feel free to experiment and alter the API as we learn from experience.

Let's continue to develop these ideas in #75.

[SaschaSchwarze0]

I tend to agree. We do this now with volumes where we inherit the Kubernetes VolumeSource. And imo that's also what we should go to with spec.sources in our future API evolvement.

[SaschaSchwarze0]

An alternative would be that we serve different paths for each vendor that we support, /event/github vs /event/gitlab. For usability reasons, it might make sense if we also extend the status of the Build. It will probably be impossible to (always) determine the externally exposed URL of the service especially if it's not just the Ingress but cluster-external components route requests. So, we will need a configuration option where we are told about it. But then we can use that and put the URL of the endpoint into the Build status from where the user who created the Build can copy it into the GitHub / GitLab etc UI.

[gabemontero]

we handle it in the same fashion as @SaschaSchwarze0 describes with openshift builds and that has proven maintainable / successful

+1

[otaviof]

Let me clarify this point a bit better, @SaschaSchwarze0. The need to parse the GitHub event is to determine which exact event is in the request, as you can see on the GitHub WebHook documentation there are a number of those.

So, in one hand, we always need to parse GitHub WebHook request payloads, but in the other we can easily extend the triggers support to more intersting use-cases, like for instance GitHub Actions Workflow.

An alternative would be that we serve different paths for each vendor that we support, /event/github vs /event/gitlab.

That's the current proposal, Sascha. Indeed, we can extend the WebHook endpoints by support different request patterns differentiating providers like GitHub, GitLab, etc.

For usability reasons, it might make sense if we also extend the status of the Build.

Yes, most defintely 👍 And it's also part of the current proposal, suggested by @gabemontero early on.

It will probably be impossible to (always) determine the externally exposed URL of the service especially if it's not just the Ingress but cluster-external components route requests.

The external endpoint URL is responsibility of the cluster administrator, this persona will know in advance the desired URL and create the necessary configuration to expose Shipwright Trigger.

I touched this subject as part of the Risks and Mitigations section which describes the different ways of bringing external traffic into the cluster. For instance, the Ingress Controller as you mentioned, also options like service mesh and OpenShift routes.

So, while the Shipwright Trigger won't be able to know the exact URL, in order to determine the exact circumstance the WebHook request was issued, GitHub offers a very good amount of information to determine repository, originating user, and a lot more.

In other words we need to extend to work invested on parsing the request payload to extract more meta information needed.

So, we will need a configuration option where we are told about it. But then we can use that and put the URL of the endpoint into the Build status from where the user who created the Build can copy it into the GitHub / GitLab etc UI.

Given the security implications explained before, the persona responsible to expose the cluster for external requests will provide the final URL. On this video you can see how the configuration would happen.

[SaschaSchwarze0]

I think these labels will also need to change the behavior of the BuildRun:

• when there is a trigger for a PullRequest, then the repository and revision (commit ID of the PR) will need to be passed to the source step.
• when there is a trigger for a Push, then the revision (commit ID that was pushed) will need to be passed to the source step.
• when there is a trigger for an Image, then I could imagine that the user will want this image to make it somewhere. This is not trivial because it depends on the strategy. For ko, I would want to do a goml set -f .ko.yaml -p defaultBaseImage -v THE_IMAGE_THAT_TRIGGERED. In a Dockerfile, I would want to pass it as a build-arg. So, we need maybe some strategy enhancements to support this. And we need to brainstorm how we can pass this.

While the Image part is probably for the future, we likely need the Git related things already at the beginning.

[gabemontero]

interesting point @SaschaSchwarze0 ... I wonder if some further clarification is needed though as I read you text ^^.

I say that because if one wants to change behavior, then generally in k8s you should use annotations for that vs. labels ... annotations are the more loosely coupled alternative to API change for controlling implementation behavior, where labels are metadata/useful information stored by the implementation when it is finished its intent,
like what @otaviof mentions in this section. So your labels will also need to change the behavior of the BuildRun gave me some pause.

For example, the type of trigger matters, and the type of trigger might lead to different labels being set (git commit vs. image sha, etc.). I could see that.

But then, if we have a "chain" of build runs that manifest from one build run leading to another build run getting triggered, that gets interesting. Do we want shipwright interpreting labels from prior build runs in the chain and adjusting behavior? Is that what you were thinking also? Initially, that seems to not mesh with the k8s annotation vs. label convention.

But may I'm incorrectly reading in between the lines with what you intended.

[SaschaSchwarze0]

Hi @gabemontero, I was not so much looking at the label vs annotation aspect. But yeah, labels, also because of their length limitation could become a problem and annotations might be more correct - or some first-class fields of the BuildRun are used instead, but then it might be not obvious when they come from a user vs a controller (though not different to annotations that could also be user-defined).

[gabemontero]

ok thanks for clarifying @SaschaSchwarze0

also, good point on the label length restrictions, especially in the light on long'ish items like image or commit sha's; something the implementation would minimally have to be cognizant of

I wonder @otaviof if the explicit field vs. label vs. annotation back and forth here should be captured as an open question. I at least would be fine classifying it as such, merging this SHP with that, and then sorting this our during the implementation PR. But of course let's see what sort of consensus arises as others chime in. Perhaps a discussion topic for an upcoming community meeting.

[otaviof]

@SaschaSchwarze0, @gabemontero as we've spoken during the community meeting yesterday (#80) about the objective of passing information via labels, we have a couple o paragraph to summarize the objective with them. Please consider:

https://github.com/otaviof/community/blob/467cea4c797411ecfcc8e57facf7495b434cd15b/ships/0031-shipwright-trigger.md?plain=1#L297-L301

Let me know if that's specific and actionable as we expect.

[gabemontero]

I like the latest text wrt the new trigger controller setting metadata that is then interpreted by the existing build controller, which then sets the first class fields in the status of the build run

The more I think about it, the more I am inclined that annotations and not labels should be set by the trigger controller to convey trigger cause, given the label length restrictions and the fact that we are "affecting" the existing controller's behavior and processing of the build run (in which status fields it sets), where k8s conventions say you should annotations for that.
Also, I would not think the existing build related controllers need to do queries based on any trigger labels, right?

But as I said before, I'm also fine with carrying the annotation vs. label decision as an open question to be sorted out during implementation PR review. If you are not convinced to change from labels to annotations here @otaviof I at least would be good with just adding an open question on this, or if I'm the only vote for annotations and @SaschaSchwarze0 or others are good with labels.

I'm also more inclined to start with annotations vs. first class fields, simply to minimize the API impact. We can always move from annotations to first class fields with relative ease. The other direction is not as easy.

Assuming everyone else is fine with avoiding additional first class fields for this, I say we put to bed the annotation vs. label decision one way or the other and we can resolve this thread.

thanks

[otaviof]

[...]
The more I think about it, the more I am inclined that annotations and not labels should be set by the trigger controller to convey trigger cause, given the label length restrictions and the fact that we are "affecting" the existing controller's behavior and processing of the build run (in which status fields it sets), where k8s conventions say you should annotations for that. Also, I would not think the existing build related controllers need to do queries based on any trigger labels, right?

That's a good point, Gabe! I'll make sure to update the EP to reflect this.

I'm also more inclined to start with annotations vs. first class fields, simply to minimize the API impact. We can always move from annotations to first class fields with relative ease. The other direction is not as easy.

That's exactly the point, if needed be we can "promote" those fields later on.

[otaviof]

Please consider the latest changes.

[gabemontero]

LGTM @otaviof

I'll let @SaschaSchwarze0 make his final pass and when you two are in sync let him tag for merge, given @adambkaplan did the approve

thanks

[SaschaSchwarze0]

/lgtm