SHIP : Git event-driven build executions #41

sbose78 · 2021-11-03T20:50:20Z

An easy way to setup webhooks for triggered Git event-based builds.

( Extension of the proof of concepts in shipwright-io/build#912 and https://github.com/imjasonh/build-task )

openshift-ci · 2021-11-03T20:50:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from sbose78 after the PR has been reviewed.

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

Needs approval from an approver in each of these files:

OWNERS

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

SaschaSchwarze0 · 2021-11-04T09:42:27Z

ships/0025-event-driven-builds.md

+  ...
+  webhook:
+    type: github
+    imageTagPolicy: short_sha    # optional, allowed values: 'short_sha' , 'branch'. Defaults to 'branch'.


I am not sure if mixing in the concept of image tag policies here into the webhook is correct. Imo an image tag policy should be outside of it as it also makes sense as a standalone concept.

Agreed.

This was mainly brought it to address the issue of receiving events for all branches. I wonder if we should probably support:

what are the branches you wish to support listening on?

what's your tag policy? short_sha' / 'branch'.

We could definitely de-scope things and say, we'll only act on events which come in from the .spec.source.revision only. My over-enthusiastic self wanted to dabble with more than that.

SaschaSchwarze0 · 2021-11-04T09:44:38Z

ships/0025-event-driven-builds.md

+spec:
+  ...
+  ...
+  webhook:


We could generalize this to triggers to support other scenarios in the future such as a daily build.

+1 ..... image change triggers could be a peer type like git/scm webhook triggers ... and a build can have multiple "trigger types"

Sounds good 👍

I'm good with the change you've put in for this @sbose78

SaschaSchwarze0 · 2021-11-04T09:48:10Z

ships/0025-event-driven-builds.md

+While the enhancement could have been scoped to only deal with the branch/revision explicitly specified in the `Build` resource, 
+doing the same would have have excluded some key use cases where users push to a different branch before merging to 'main'. 


Hm, but what is the user's expectation? I'd say if I specify a Build with a source revision set to main, that I only want commits into main to trigger a BuildRun.

I did go in that direction and quickly found it to be too cumbersome to have to define a new Build for every branch. 🤷‍♂️

I see. We'll have to think about a good solution. Let's look at the scenario first: in my experience, repositories typically only have one or two major branches (main and develop) - having two Builds for this would be okayish for me because of how we use those branches internally (the images go into different repos).

The other branches you have in mind, would not that have been pull requests ? Because that would make sense for me as a scenario: define branch A in the build and define that pull requests for that branch should also be built.

Another scenario where branches do not play any role would be to build a tag once it gets pushed.

SaschaSchwarze0 · 2021-11-04T09:54:58Z

ships/0025-event-driven-builds.md

+Similar to the `Drawbacks` section the `Alternatives` section is used to highlight and record other
+possible approaches to delivering the value proposed by an enhancement.


How much simpler would it be if we implement our own webhook endpoint that directly consumes the event data from GitHub or GitLab? I personally think, it is much simpler. And it will be trivial to implement logic there that checks the branch of the payload and compares it with what is defined in the Build.

You mean - write a simple http service which understands the payload and creates corresponding Build/BuildRuns?

SaschaSchwarze0 · 2021-11-04T09:56:10Z

ships/0025-event-driven-builds.md

+    secretRef:                   # optional, will be genereated if not specified.
+      name: my-webhook-secret.  


I wonder if optional is reasonable here because if we generate it, then the user will need to decode it to get the webhook token to then store it in GitHub, right?

Correct, this is more of a knob of convenience :)

sbose78 · 2021-11-29T02:42:59Z

Requesting another round of review, the following changes have been addressed:

Remove support for arbitrary branches.
Renamed webhook to trigger. The type: github is the key to deciding which ClusterTriggerBinding would be used.

gabemontero

Per discussion in the community meeting today, I've asked @sbose78 to add some diagrams to help illustrate things, and he has graciously agreed :-)

SaschaSchwarze0 · 2021-12-08T19:57:44Z

@sbose78 quick formal request: please change the PR title to SHIP-0026 and please also rename the file then. Then you claimed a number. 25 has been taken in the meantime. ;-)

otaviof · 2022-02-15T08:31:24Z

ships/0025-event-driven-builds.md

+
+## Open Questions [optional]
+
+1. Almost every user would need an exposed `Service`, how do we create a vendor-agnostic `Ingress` object ?


Sounds like something for a Helm Chart? Users could choose between Ingress and Route. Since we don't have a Helm Chart yet, installation instructions should do it.

otaviof · 2022-02-15T08:39:37Z

ships/0025-event-driven-builds.md

+specified in the `Build` CR.
+
+
+2. Exposing a webhook URL enables creation of pods ( ie, processes on the node ) by actors who may not necessarily have access to the cluster.


As part of the installation instructions, we must include instructions for the cluster admin to only allow access to certain IP addresses. This filtering ability completely depend on the cluster capabilities, like for instance when using a service mesh in place.

otaviof · 2022-02-15T08:41:24Z

ships/0025-event-driven-builds.md

+
+2. Exposing a webhook URL enables creation of pods ( ie, processes on the node ) by actors who may not necessarily have access to the cluster.
+
+This does open up an attack vector given the execution of the build is done using the configured service account. 


Given we are dealing with external access, we need to be clear that it does require special attention from the cluster admin.

otaviof · 2022-02-15T08:49:11Z

ships/0025-event-driven-builds.md

+---
+title: event-driven-builds
+authors:
+  - "@sbose787"


Little typo here, the extra 7 at the end.

SHIP : Git Event-driven build executions

74c0f59

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 3, 2021

openshift-ci bot requested review from HeavyWombat and SaschaSchwarze0 November 3, 2021 20:50

sbose78 added 2 commits November 3, 2021 16:54

Update 0025-event-driven-builds.md

2cd63d1

Update 0025-event-driven-builds.md

6f7e6d6

sbose78 changed the title ~~SHIP : Git Event-driven build executions~~ SHIP : Git event-driven build executions Nov 3, 2021

typos

bb53631

sbose78 requested review from adambkaplan and imjasonh November 3, 2021 21:03

SaschaSchwarze0 reviewed Nov 4, 2021

View reviewed changes

sbose78 mentioned this pull request Nov 8, 2021

November 8, 2021 Community Meeting #40

Closed

Do not support arbitrary brances

c2c7404

sbose78 mentioned this pull request Nov 29, 2021

November 29, 2021 Community Meeting #43

Closed

gabemontero suggested changes Nov 29, 2021

View reviewed changes

otaviof reviewed Feb 15, 2022

View reviewed changes

ships/0025-event-driven-builds.md

---

title: event-driven-builds

authors:

- "@sbose787"

Copy link

Member

otaviof Feb 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little typo here, the extra 7 at the end.

otaviof mentioned this pull request Mar 24, 2022

SHIP-0031: Shipwright Trigger #68

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SHIP : Git event-driven build executions #41

SHIP : Git event-driven build executions #41

sbose78 commented Nov 3, 2021 •

edited

Loading

openshift-ci bot commented Nov 3, 2021

SaschaSchwarze0 Nov 4, 2021

sbose78 Nov 4, 2021 •

edited

Loading

SaschaSchwarze0 Nov 4, 2021

gabemontero Nov 4, 2021

sbose78 Nov 4, 2021

gabemontero Nov 29, 2021

SaschaSchwarze0 Nov 4, 2021

sbose78 Nov 4, 2021

SaschaSchwarze0 Nov 4, 2021

SaschaSchwarze0 Nov 4, 2021

sbose78 Nov 4, 2021

SaschaSchwarze0 Nov 4, 2021

SaschaSchwarze0 Nov 4, 2021

sbose78 Nov 4, 2021

sbose78 commented Nov 29, 2021

gabemontero left a comment

SaschaSchwarze0 commented Dec 8, 2021

otaviof Feb 15, 2022

otaviof Feb 15, 2022

otaviof Feb 15, 2022

otaviof Feb 15, 2022

		While the enhancement could have been scoped to only deal with the branch/revision explicitly specified in the `Build` resource,
		doing the same would have have excluded some key use cases where users push to a different branch before merging to 'main'.

		Similar to the `Drawbacks` section the `Alternatives` section is used to highlight and record other
		possible approaches to delivering the value proposed by an enhancement.

		secretRef: # optional, will be genereated if not specified.
		name: my-webhook-secret.


		## Open Questions [optional]

		1. Almost every user would need an exposed `Service`, how do we create a vendor-agnostic `Ingress` object ?

		specified in the `Build` CR.


		2. Exposing a webhook URL enables creation of pods ( ie, processes on the node ) by actors who may not necessarily have access to the cluster.


		2. Exposing a webhook URL enables creation of pods ( ie, processes on the node ) by actors who may not necessarily have access to the cluster.

		This does open up an attack vector given the execution of the build is done using the configured service account.

SHIP : Git event-driven build executions #41

Are you sure you want to change the base?

SHIP : Git event-driven build executions #41

Conversation

sbose78 commented Nov 3, 2021 • edited Loading

openshift-ci bot commented Nov 3, 2021

Choose a reason for hiding this comment

sbose78 Nov 4, 2021 • edited Loading

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

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

sbose78 commented Nov 29, 2021

gabemontero left a comment

Choose a reason for hiding this comment

SaschaSchwarze0 commented Dec 8, 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

sbose78 commented Nov 3, 2021 •

edited

Loading

sbose78 Nov 4, 2021 •

edited

Loading