From e7fa4fb8d9b8f8e5e2a20db8d99b291a7680ed36 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Tue, 2 Apr 2024 20:54:51 +0200 Subject: [PATCH] Extract checkout target commit to a composite action (#38682) We already had to do this very sensitive part of the workflow in three places, so it makes sense to extract it to a composite action. This means that we need to do one more checkout from the target branch first (otherwise the action would not be available) but we avoid code duplication and fragility this way. (cherry picked from commit 76c92c3e0a94aa10cdf035c495886f72abb993f8) --- .../actions/checkout_target_commit/action.yml | 78 +++++++++++++ .github/workflows/ci-image-build.yml | 51 ++------- .github/workflows/prod-image-build.yml | 106 +++--------------- 3 files changed, 101 insertions(+), 134 deletions(-) create mode 100644 .github/actions/checkout_target_commit/action.yml diff --git a/.github/actions/checkout_target_commit/action.yml b/.github/actions/checkout_target_commit/action.yml new file mode 100644 index 0000000000000..e90ae0199804c --- /dev/null +++ b/.github/actions/checkout_target_commit/action.yml @@ -0,0 +1,78 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +--- +name: 'Checkout target commit' +description: > + Checks out target commit with the exception of .github scripts directories that come from the target branch +inputs: + target-commit-sha: + description: 'SHA of the target commit to checkout' + required: true + pull-request-target: + description: 'Whether the workflow is a pull request target workflow' + required: true + is-committer-build: + description: 'Whether the build is done by a committer' + required: true +runs: + using: "composite" + steps: + - name: "Checkout target commit" + uses: actions/checkout@v4 + with: + ref: ${{ inputs.target-commit-sha }} + persist-credentials: false + #################################################################################################### + # BE VERY CAREFUL HERE! THIS LINE AND THE END OF THE WARNING. IN PULL REQUEST TARGET WORKFLOW + # WE CHECK OUT THE TARGET COMMIT ABOVE TO BE ABLE TO BUILD THE IMAGE FROM SOURCES FROM THE + # INCOMING PR, RATHER THAN FROM TARGET BRANCH. THIS IS A SECURITY RISK, BECAUSE THE PR + # CAN CONTAIN ANY CODE AND WE EXECUTE IT HERE. THEREFORE, WE NEED TO BE VERY CAREFUL WHAT WE + # DO HERE. WE SHOULD NOT EXECUTE ANY CODE THAT COMES FROM THE PR. WE SHOULD NOT RUN ANY BREEZE + # COMMAND NOR SCRIPTS NOR COMPOSITE ACTIONS. WE SHOULD ONLY RUN CODE THAT IS EMBEDDED DIRECTLY IN + # THIS WORKFLOW - BECAUSE THIS IS THE ONLY CODE THAT WE CAN TRUST. + #################################################################################################### + - name: Checkout target branch to 'target-airflow' folder to use ci/scripts and breeze from there. + uses: actions/checkout@v4 + with: + path: "target-airflow" + ref: ${{ github.base_ref }} + persist-credentials: false + if: inputs.pull-request-target == 'true' && inputs.is-committer-build != 'true' + - name: > + Replace "scripts/ci", "dev", ".github/actions" and ".github/workflows" with the target branch + so that the those directories are not coming from the PR + shell: bash + run: | + echo + echo -e "\033[33m Replace scripts, dev, actions with target branch for non-committer builds!\033[0m" + echo + rm -rfv "scripts/ci" + rm -rfv "dev" + rm -rfv ".github/actions" + rm -rfv ".github/workflows" + mv -v "target-airflow/scripts/ci" "scripts" + mv -v "target-airflow/dev" "." + mv -v "target-airflow/.github/actions" "target-airflow/.github/workflows" ".github" + if: inputs.pull-request-target == 'true' && inputs.is-committer-build != 'true' + #################################################################################################### + # AFTER IT'S SAFE. THE `dev`, `scripts/ci` AND `.github/actions` ARE NOW COMING FROM THE + # BASE_REF - WHICH IS THE TARGET BRANCH OF THE PR. WE CAN TRUST THAT THOSE SCRIPTS ARE SAFE TO RUN. + # ALL THE REST OF THE CODE COMES FROM THE PR, AND FOR EXAMPLE THE CODE IN THE `Dockerfile.ci` CAN + # BE RUN SAFELY AS PART OF DOCKER BUILD. BECAUSE IT RUNS INSIDE THE DOCKER CONTAINER AND IT IS + # ISOLATED FROM THE RUNNER. + #################################################################################################### diff --git a/.github/workflows/ci-image-build.yml b/.github/workflows/ci-image-build.yml index cd11376a82119..d98394c67e5b6 100644 --- a/.github/workflows/ci-image-build.yml +++ b/.github/workflows/ci-image-build.yml @@ -134,54 +134,17 @@ ${{ inputs.do-build == 'true' && inputs.image-tag || '' }}" shell: bash run: docker run -v "${GITHUB_WORKSPACE}:/workspace" -u 0:0 bash -c "rm -rf /workspace/*" if: inputs.do-build == 'true' - - uses: actions/checkout@v4 + - name: "Checkout target branch" + uses: actions/checkout@v4 with: - ref: ${{ inputs.target-commit-sha }} persist-credentials: false + - name: "Checkout target commit" + uses: ./.github/actions/checkout_target_commit if: inputs.do-build == 'true' - #################################################################################################### - # BE VERY CAREFUL HERE! THIS LINE AND THE END OF THE WARNING. IN PULL REQUEST TARGET WORKFLOW - # WE CHECK OUT THE TARGET COMMIT ABOVE TO BE ABLE TO BUILD THE IMAGE FROM SOURCES FROM THE - # INCOMING PR, RATHER THAN FROM TARGET BRANCH. THIS IS A SECURITY RISK, BECAUSE THE PR - # CAN CONTAIN ANY CODE AND WE EXECUTE IT HERE. THEREFORE, WE NEED TO BE VERY CAREFUL WHAT WE - # DO HERE. WE SHOULD NOT EXECUTE ANY CODE THAT COMES FROM THE PR. WE SHOULD NOT RUN ANY BREEZE - # COMMAND NOR SCRIPTS NOR COMPOSITE ACTIONS. WE SHOULD ONLY RUN CODE THAT IS EMBEDDED DIRECTLY IN - # THIS WORKFLOW - BECAUSE THIS IS THE ONLY CODE THAT WE CAN TRUST. - #################################################################################################### - - name: Checkout target branch to 'target-airflow' folder to use ci/scripts and breeze from there. - uses: actions/checkout@v4 with: - path: "target-airflow" - ref: ${{ github.base_ref }} - persist-credentials: false - if: > - inputs.do-build == 'true' && inputs.pull-request-target == 'true' && - inputs.is-committer-build != 'true' - - name: > - Replace "scripts/ci", "dev", ".github/actions" and ".github/workflows" with the target branch - so that the those directories are not coming from the PR - shell: bash - run: | - echo - echo -e "\033[33m Replace scripts, dev, actions with target branch for non-committer builds!\033[0m" - echo - rm -rfv "scripts/ci" - rm -rfv "dev" - rm -rfv ".github/actions" - rm -rfv ".github/workflows" - mv -v "target-airflow/scripts/ci" "scripts" - mv -v "target-airflow/dev" "." - mv -v "target-airflow/.github/actions" "target-airflow/.github/workflows" ".github" - if: > - inputs.do-build == 'true' && inputs.pull-request-target == 'true' && - inputs.is-committer-build != 'true' - #################################################################################################### - # HERE IT'S A BIT SAFER. THE `dev`, `scripts/ci` AND `.github/actions` ARE NOW COMING FROM THE - # BASE_REF - WHICH IS THE TARGET BRANCH OF THE PR. WE CAN TRUST THAT THOSE SCRIPTS ARE SAVE TO RUN. - # ALL THE REST OF THE CODE COMES FROM THE PR, AND FOR EXAMPLE THE CODE IN THE `Dockerfile.ci` CAN - # BE RUN SAFELY AS PART OF DOCKER BUILD. BECAUSE IT RUNS INSIDE THE DOCKER CONTAINER AND IT IS - # ISOLATED FROM THE RUNNER. - #################################################################################################### + target-commit-sha: ${{ inputs.target-commit-sha }} + pull-request-target: ${{ inputs.pull-request-target }} + is-committer-build: ${{ inputs.is-committer-build }} - name: "Cleanup docker" run: ./scripts/ci/cleanup_docker.sh if: inputs.do-build == 'true' diff --git a/.github/workflows/prod-image-build.yml b/.github/workflows/prod-image-build.yml index cfda7661ae2dc..d26a5f3420e50 100644 --- a/.github/workflows/prod-image-build.yml +++ b/.github/workflows/prod-image-build.yml @@ -127,54 +127,17 @@ jobs: shell: bash run: docker run -v "${GITHUB_WORKSPACE}:/workspace" -u 0:0 bash -c "rm -rf /workspace/*" if: inputs.do-build == 'true' && inputs.upload-package-artifact == 'true' - - uses: actions/checkout@v4 - with: - ref: ${{ inputs.target-commit-sha }} - persist-credentials: false - if: inputs.do-build == 'true' && inputs.upload-package-artifact == 'true' - #################################################################################################### - # BE VERY CAREFUL HERE! THIS LINE AND THE END OF THE WARNING. IN PULL REQUEST TARGET WORKFLOW - # WE CHECK OUT THE TARGET COMMIT ABOVE TO BE ABLE TO BUILD THE IMAGE FROM SOURCES FROM THE - # INCOMING PR, RATHER THAN FROM TARGET BRANCH. THIS IS A SECURITY RISK, BECAUSE THE PR - # CAN CONTAIN ANY CODE AND WE EXECUTE IT HERE. THEREFORE, WE NEED TO BE VERY CAREFUL WHAT WE - # DO HERE. WE SHOULD NOT EXECUTE ANY CODE THAT COMES FROM THE PR. WE SHOULD NOT RUN ANY BREEZE - # COMMAND NOR SCRIPTS NOR COMPOSITE ACTIONS. WE SHOULD ONLY RUN CODE THAT IS EMBEDDED DIRECTLY IN - # THIS WORKFLOW - BECAUSE THIS IS THE ONLY CODE THAT WE CAN TRUST. - #################################################################################################### - - name: Checkout target branch to 'target-airflow' folder to use ci/scripts and breeze from there. + - name: "Checkout target branch" uses: actions/checkout@v4 with: - path: "target-airflow" - ref: ${{ github.base_ref }} persist-credentials: false - if: > - inputs.do-build == 'true' && inputs.pull-request-target == 'true' && - inputs.is-committer-build != 'true' && inputs.upload-package-artifact == 'true' - - name: > - Replace "scripts/ci", "dev", ".github/actions" and ".github/workflows" with the target branch - so that the those directories are not coming from the PR - shell: bash - run: | - echo - echo -e "\033[33m Replace scripts, dev, actions with target branch for non-committer builds!\033[0m" - echo - rm -rfv "scripts/ci" - rm -rfv "dev" - rm -rfv ".github/actions" - rm -rfv ".github/workflows" - mv -v "target-airflow/scripts/ci" "scripts" - mv -v "target-airflow/dev" "." - mv -v "target-airflow/.github/actions" "target-airflow/.github/workflows" ".github" - if: > - inputs.do-build == 'true' && inputs.pull-request-target == 'true' && - inputs.is-committer-build != 'true' - #################################################################################################### - # HERE IT'S A BIT SAFER. THE `dev`, `scripts/ci` AND `.github/actions` ARE NOW COMING FROM THE - # BASE_REF - WHICH IS THE TARGET BRANCH OF THE PR. WE CAN TRUST THAT THOSE SCRIPTS ARE SAVE TO RUN. - # ALL THE REST OF THE CODE COMES FROM THE PR, AND FOR EXAMPLE THE CODE IN THE `Dockerfile.ci` CAN - # BE RUN SAFELY AS PART OF DOCKER BUILD. BECAUSE IT RUNS INSIDE THE DOCKER CONTAINER AND IT IS - # ISOLATED FROM THE RUNNER. - #################################################################################################### + - name: "Checkout target commit" + uses: ./.github/actions/checkout_target_commit + with: + target-commit-sha: ${{ inputs.target-commit-sha }} + pull-request-target: ${{ inputs.pull-request-target }} + is-committer-build: ${{ inputs.is-committer-build }} + if: inputs.do-build == 'true' && inputs.upload-package-artifact == 'true' - name: "Cleanup docker" run: ./scripts/ci/cleanup_docker.sh if: inputs.do-build == 'true' && inputs.upload-package-artifact == 'true' @@ -263,54 +226,17 @@ ${{ inputs.do-build == 'true' && inputs.image-tag || '' }}" shell: bash run: docker run -v "${GITHUB_WORKSPACE}:/workspace" -u 0:0 bash -c "rm -rf /workspace/*" if: inputs.do-build == 'true' - - uses: actions/checkout@v4 - with: - ref: ${{ inputs.target-commit-sha }} - persist-credentials: false - if: inputs.do-build == 'true' - #################################################################################################### - # BE VERY CAREFUL HERE! THIS LINE AND THE END OF THE WARNING. IN PULL REQUEST TARGET WORKFLOW - # WE CHECK OUT THE TARGET COMMIT ABOVE TO BE ABLE TO BUILD THE IMAGE FROM SOURCES FROM THE - # INCOMING PR, RATHER THAN FROM TARGET BRANCH. THIS IS A SECURITY RISK, BECAUSE THE PR - # CAN CONTAIN ANY CODE AND WE EXECUTE IT HERE. THEREFORE, WE NEED TO BE VERY CAREFUL WHAT WE - # DO HERE. WE SHOULD NOT EXECUTE ANY CODE THAT COMES FROM THE PR. WE SHOULD NOT RUN ANY BREEZE - # COMMAND NOR SCRIPTS NOR COMPOSITE ACTIONS. WE SHOULD ONLY RUN CODE THAT IS EMBEDDED DIRECTLY IN - # THIS WORKFLOW - BECAUSE THIS IS THE ONLY CODE THAT WE CAN TRUST. - #################################################################################################### - - name: Checkout target branch to 'target-airflow' folder to use ci/scripts and breeze from there. + - name: "Checkout target branch" uses: actions/checkout@v4 with: - path: "target-airflow" - ref: ${{ github.base_ref }} persist-credentials: false - if: > - inputs.do-build == 'true' && inputs.pull-request-target == 'true' && - inputs.is-committer-build != 'true' - - name: > - Replace "scripts/ci", "dev", ".github/actions" and ".github/workflows" with the target branch - so that the those directories are not coming from the PR - shell: bash - run: | - echo - echo -e "\033[33m Replace scripts, dev, actions with target branch for non-committer builds!\033[0m" - echo - rm -rfv "scripts/ci" - rm -rfv "dev" - rm -rfv ".github/actions" - rm -rfv ".github/workflows" - mv -v "target-airflow/scripts/ci" "scripts" - mv -v "target-airflow/dev" "." - mv -v "target-airflow/.github/actions" "target-airflow/.github/workflows" ".github" - if: > - inputs.do-build == 'true' && inputs.pull-request-target == 'true' && - inputs.is-committer-build != 'true' - #################################################################################################### - # HERE IT'S A BIT SAFER. THE `dev`, `scripts/ci` AND `.github/actions` ARE NOW COMING FROM THE - # BASE_REF - WHICH IS THE TARGET BRANCH OF THE PR. WE CAN TRUST THAT THOSE SCRIPTS ARE SAVE TO RUN. - # ALL THE REST OF THE CODE COMES FROM THE PR, AND FOR EXAMPLE THE CODE IN THE `Dockerfile.ci` CAN - # BE RUN SAFELY AS PART OF DOCKER BUILD. BECAUSE IT RUNS INSIDE THE DOCKER CONTAINER AND IT IS - # ISOLATED FROM THE RUNNER. - #################################################################################################### + - name: "Checkout target commit" + uses: ./.github/actions/checkout_target_commit + with: + target-commit-sha: ${{ inputs.target-commit-sha }} + pull-request-target: ${{ inputs.pull-request-target }} + is-committer-build: ${{ inputs.is-committer-build }} + if: inputs.do-build == 'true' - name: "Cleanup docker" run: ./scripts/ci/cleanup_docker.sh if: inputs.do-build == 'true'