chore(ci): Switch to PR Reviews for triggering CI #20892

jszwedko · 2024-07-19T18:17:31Z

Allows us to capture the correct SHA to run CI with.

Allows us to capture the correct SHA to run CI with. Signed-off-by: Jesse Szwedko <[email protected]>

datadog-vectordotdev · 2024-07-19T18:42:56Z

Datadog Report

Branch report: jszwedko/switch-to-pr-review-comments
Commit report: c104210
Test service: vector

✅ 0 Failed, 7 Passed, 0 Skipped, 25.48s Total Time

.github/workflows/comment-trigger.yml

.github/workflows/misc.yml

Signed-off-by: Jesse Szwedko <[email protected]>

xopham

I had the liberty to take a bit of an offensive view on things and made a few comments on parts not directly focus of the PR. I hope you don't mind. Feel free to consider these things in a separate effort in order to not blow up this PR.

I'd like to bring up an architecture question:

I think we are just developing an idea what secure coding for GitHub workflows might mean, but I took some inspiration from REST API security considerations.

Currently, we validate the comment author, check at the later action whether it was a PR review and select the GitHub reference for checkout. This creates a loose link between what was validated and what we run on. This is not necessarily vulnerable, but more likely to cause problems in the future (TOCTU- or parsing-confusion-type vulnerabilities).

Therefore, we might want to avoid such loosely linked references to validation. We could create a validate action that directly emits validated command and GitHub reference as outputs and have these consumed as inputs by subsequent steps, reusable workflows or composite actions thereafter.
What do you think?

xopham · 2024-07-22T08:10:13Z

.github/workflows/ci-review-trigger.yml

+          contains(github.event.review.body, '/ci-run-all')
+          || contains(github.event.review.body, '/ci-run-cli')
+          || contains(github.event.review.body, '/ci-run-misc')
+          || contains(github.event.review.body, '/ci-run-deny')
+          || contains(github.event.review.body, '/ci-run-component-features')
+          || contains(github.event.review.body, '/ci-run-cross')
+          || contains(github.event.review.body, '/ci-run-unit-mac')
+          || contains(github.event.review.body, '/ci-run-unit-windows')
+          || contains(github.event.review.body, '/ci-run-environment')
+          || contains(github.event.review.body, '/ci-run-regression')
+          || contains(github.event.review.body, '/ci-run-k8s')


Would it make sense to make this a startsWith to avoid accidental actions when e.g. including suggestions or other coincidental mentions?

👍 that seems reasonable.

Updated in d97fa61

xopham · 2024-07-22T08:15:43Z

.github/workflows/ci-review-trigger.yml

-      - name: Get PR author
-        id: pr
-        uses: tspascoal/get-user-teams-membership@v3
-        with:
-          username: ${{ github.event.issue.user.login }}
-          team: 'Vector'
-          GITHUB_TOKEN: ${{ steps.generate_token.outputs.token }}
-      - name: Get PR comment author


Neat. Good to remove because this could have technically been vulnerable as others might push to another one's PR 💯

That is true though it would have required:

A maintainer to create a fork

The maintainer to give a non-maintainer write access to their fork

The maintainer to create a PR from their fork

Typically maintainers create branches in the main repository. Agreed though, it is nice to get rid of.

Or steal a GitHub token with write permissions and push changes to a maintainer's branch and merge via comment 😉

xopham · 2024-07-22T08:51:06Z

.github/workflows/ci-integration-review.yml

+        if: ${{ contains(github.event.review.body, '/ci-run-integration-amqp')
+          || contains(github.event.review.body, '/ci-run-integration-all')
+          || contains(github.event.review.body, '/ci-run-all') }}


Would it make sense here and below to make this a startsWith to avoid accidental actions when e.g. including suggestions or other coincidental mentions? Essentially "enforcing" intention to run this and avoiding unintentional execution.

If yes, needs to be changed here and below. I mentioned at one other spot, but occurs throughout the workflows.

Agreed, I'll change this to startsWith.

Updated in d97fa61

xopham · 2024-07-22T09:00:28Z

.github/workflows/ci-integration-review.yml

@@ -112,109 +105,109 @@ jobs:
          command: bash scripts/ci-int-e2e-test.sh int amqp


Besides the core of this PR, but related to the original reason ("vulnerable GH workflows"): given the architecture of these invocations, you might want add scripts/ and the .github/CODEOWNERS themselves to CODEOWNERS: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners#codeowners-and-branch-protection

Obviously depends on how externals can become contributors and whether there is review on every change from external.

I'm not sure I understand this one. Someone on @vectordotdev/vector reviews all external changes regardless. It is reasonable enough to add ownership for all files though. I'll open a separate PR for that.

Opened #20947

xopham · 2024-07-22T09:30:25Z

.github/workflows/cli.yml

-      - name: (PR comment) Get PR branch
-        if: ${{ github.event_name == 'issue_comment' }}
-        uses: xt0rted/pull-request-comment-branch@v2
-        id: comment-branch
-
-      - name: (PR comment) Set latest commit status as pending
-        if: ${{ github.event_name == 'issue_comment' }}
+      - name: (PR review) Set latest commit status as pending
+        if: ${{ github.event_name == 'pull_request_review' }}
        uses: myrotvorets/[email protected]
        with:
-          sha: ${{ steps.comment-branch.outputs.head_sha }}
+          sha: ${{ github.events.review.commit_id }}
          token: ${{ secrets.GITHUB_TOKEN }}
          context: CLI - Linux
          status: pending

-      - name: (PR comment) Checkout PR branch
-        if: ${{ github.event_name == 'issue_comment' }}
+      - name: (PR review) Checkout review SHA
+        if: ${{ github.event_name == 'pull_request_review' }}
        uses: actions/checkout@v3
        with:
-          ref: ${{ steps.comment-branch.outputs.head_ref }}
+          ref: ${{ github.events.review.commit_id }}

      - name: Checkout branch
-        if: ${{ github.event_name != 'issue_comment' }}
+        if: ${{ github.event_name != 'pull_request_review' }}


you could avoid this construct and create a more hard-linked reference: set an input to this reusable workflow to provide the ref, emit a "validated commit ID" in the validate job and pass it as input to these workflows.

the pattern is repeated in several reusable workflows.

Agreed, I did notice this boilerplate repeated across workflows that could be DRY'd up, but didn't want to attempt that refactoring with this change.

ok, sounds good to me

xopham · 2024-07-22T09:41:22Z

.github/workflows/ci-review-trigger.yml

 # appears to be due to the potential length of it.
 #concurrency:
-#  group: ${{ github.workflow }}-${{ github.event.issue.id }}-${{ github.event.comment.body }}
+#  group: ${{ github.workflow }}-${{ github.event.issue.id }}-${{ github.event.review.body }}
 #  cancel-in-progress: true

 jobs:
  validate:


do you need a step here to validate whether the commit it's running on is the latest in the PR? Some labelling and other actions might give a false sense of what has been tested...

I don't think it is necessary that the commit be the latest commit in the PR. The workflows will run again as part of the merge queue so if there are breakages they will be caught then. This comment trigger workflow is just to have the ability to trigger them without adding the PR to the merge queue.

An exception is the component features check which doesn't run on the merge queue, instead it runs nightly, but breakages to that are low urgency and can be resolved later.

Signed-off-by: Jesse Szwedko <[email protected]>

jszwedko · 2024-07-26T20:41:49Z

Currently, we validate the comment author, check at the later action whether it was a PR review and select the GitHub reference for checkout. This creates a loose link between what was validated and what we run on. This is not necessarily vulnerable, but more likely to cause problems in the future (TOCTU- or parsing-confusion-type vulnerabilities).

Therefore, we might want to avoid such loosely linked references to validation. We could create a validate action that directly emits validated command and GitHub reference as outputs and have these consumed as inputs by subsequent steps, reusable workflows or composite actions thereafter. What do you think?

I agree, it does seem like this could be refactored such that the comment triggers provide input to included workflows that includes the ref to checkout. This would also let us delete some of the duplicate steps across the triggered workflows. I'd like to leave this as a follow-up though if it is ok with you.

neuronull

LGTM

xopham

LGTM! Thanks for sharing your insights. That's very helpful 🚀

* chore(ci): Switch to PR Reviews for triggering CI Allows us to capture the correct SHA to run CI with. Signed-off-by: Jesse Szwedko <[email protected]> * PR feedback Signed-off-by: Jesse Szwedko <[email protected]> * use startsWith rather than contains Signed-off-by: Jesse Szwedko <[email protected]> --------- Signed-off-by: Jesse Szwedko <[email protected]>

chore(ci): Switch to PR Reviews for triggering CI

67b0b29

Allows us to capture the correct SHA to run CI with. Signed-off-by: Jesse Szwedko <[email protected]>

jszwedko added the no-changelog Changes in this PR do not need user-facing explanations in the release changelog label Jul 19, 2024

jszwedko marked this pull request as ready for review July 19, 2024 18:17

neuronull reviewed Jul 19, 2024

View reviewed changes

.github/workflows/comment-trigger.yml Show resolved Hide resolved

.github/workflows/misc.yml Outdated Show resolved Hide resolved

PR feedback

aaffa77

Signed-off-by: Jesse Szwedko <[email protected]>

jszwedko requested a review from neuronull July 19, 2024 20:21

xopham reviewed Jul 22, 2024

View reviewed changes

use startsWith rather than contains

d97fa61

Signed-off-by: Jesse Szwedko <[email protected]>

jszwedko requested a review from xopham July 26, 2024 20:41

neuronull approved these changes Jul 26, 2024

View reviewed changes

xopham approved these changes Jul 29, 2024

View reviewed changes

jszwedko added this pull request to the merge queue Jul 30, 2024

Merged via the queue into master with commit 6e803a3 Jul 30, 2024
156 checks passed

jszwedko deleted the jszwedko/switch-to-pr-review-comments branch July 30, 2024 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(ci): Switch to PR Reviews for triggering CI #20892

chore(ci): Switch to PR Reviews for triggering CI #20892

jszwedko commented Jul 19, 2024

datadog-vectordotdev bot commented Jul 19, 2024 •

edited

Loading

xopham left a comment •

edited

Loading

xopham Jul 22, 2024

jszwedko Jul 26, 2024

jszwedko Jul 26, 2024

xopham Jul 22, 2024

jszwedko Jul 26, 2024

xopham Jul 29, 2024

xopham Jul 22, 2024

jszwedko Jul 26, 2024

jszwedko Jul 26, 2024

xopham Jul 22, 2024

jszwedko Jul 26, 2024

jszwedko Jul 26, 2024

xopham Jul 22, 2024

jszwedko Jul 26, 2024

xopham Jul 29, 2024

xopham Jul 22, 2024

jszwedko Jul 26, 2024

jszwedko commented Jul 26, 2024

neuronull left a comment

xopham left a comment

		@@ -112,109 +105,109 @@ jobs:
		command: bash scripts/ci-int-e2e-test.sh int amqp

chore(ci): Switch to PR Reviews for triggering CI #20892

chore(ci): Switch to PR Reviews for triggering CI #20892

Conversation

jszwedko commented Jul 19, 2024

datadog-vectordotdev bot commented Jul 19, 2024 • edited Loading

Datadog Report

xopham left a comment • 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

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

jszwedko commented Jul 26, 2024

neuronull left a comment

Choose a reason for hiding this comment

xopham left a comment

Choose a reason for hiding this comment

datadog-vectordotdev bot commented Jul 19, 2024 •

edited

Loading

xopham left a comment •

edited

Loading