[Fix][CI] Pin azure/setup-helm to approved SHA#10853
[Fix][CI] Pin azure/setup-helm to approved SHA#10853dybyte wants to merge 1 commit intoapache:devfrom
Conversation
DanielLeens
left a comment
There was a problem hiding this comment.
Thanks for the security cleanup. I re-reviewed the latest head locally against the current upstream/dev. This was a source-level / workflow-level review only; I did not run local Maven in this round.
What This PR Fixes
- User pain: third-party GitHub Actions should be pinned to immutable SHAs instead of mutable tags.
- Fix approach: replace
azure/setup-helm@v4.3.0with a full commit SHA in.github/workflows/backend.yml. - One-line summary: pinning is the right direction, but the current SHA does not pin the same action version that the workflow previously used.
Runtime Chain I Checked
GitHub Build workflow
-> .github/workflows/backend.yml
-> helm-chart-check job
-> Setup Helm
-> helm lint deploy/kubernetes/seatunnel
Key Findings
- The normal
helm-chart-checkpath definitely hits this change. - Before this PR, the workflow was explicitly using
azure/setup-helm@v4.3.0. - I verified with
git ls-remoteagainsthttps://github.com/azure/setup-helmthatv4.3.0maps tob9e51907a09c216f16ebe8536097933489208112, while the SHA in this PR (1a275c3b69536ee54be43f2070a358922e12c8d4) maps tov4.3.1/v4instead. - So this is not a pure SHA pin of the existing version; it silently upgrades the action at the same time.
Issue 1: The pinned SHA does not correspond to the previously configured action version
- Location:
.github/workflows/backend.yml:90 - Why this matters: this workflow path runs on the normal Build flow, so changing the SHA here changes the actual action implementation used by CI. A supply-chain pin should keep behavior stable unless the version upgrade is explicitly intended and reviewed.
- Risk: this PR becomes "pin + upgrade" instead of a pure hardening change, which makes CI behavior harder to reason about and broadens the scope beyond what the PR title suggests.
- Suggested fix:
- Option A (recommended): pin the commit behind
v4.3.0(b9e51907a09c216f16ebe8536097933489208112) so the PR remains behavior-equivalent. - Option B: if the intention is to upgrade to
v4.3.1, please say that explicitly in the PR and review it as a version bump instead of presenting it as a pure pin.
- Option A (recommended): pin the commit behind
- Severity: High
CI note:
- The current fork Build failed in
Run / jdbc-connectors-it-ddl (8, ubuntu-latest)withSeaTunnel job executed failed. - That failure is on a JDBC DDL integration-test path, not on the Helm setup step changed here, so it does not look like evidence against the workflow line itself.
Conclusion: merge after fixes
- Blocking items
- Issue 1 should be fixed before merge.
- Suggested non-blocking follow-up
- After the SHA/version mapping is corrected, rerun Build so the final head has one clean CI pass.
Overall, the hardening direction is good. The blocker is that the current pin changes the action version at the same time, which makes this PR broader than it appears.
|
@DanielLeens |
|
Thanks, that additional constraint helps. If the ASF allowlist only accepts the SHA behind There is still no new commit after Daniel's latest review, so I am treating this as a reply-only follow-up rather than a fresh full re-review. From Daniel's side, either path is reasonable:
Once the branch or PR framing reflects the chosen path, I am happy to re-review the updated state. |
Purpose of this pull request
Fix post-merge CI startup failures caused by the ASF GitHub Actions allowlist policy:
https://github.com/apache/seatunnel/actions/runs/25513725736
The workflow currently uses
azure/setup-helm@v4.3.0, but this tag is not allowed by the ASF approved action patterns. The ASF allowlist already includesazure/setup-helm@1a275c3b69536ee54be43f2070a358922e12c8d4, which corresponds tov4.3.1.This PR updates the workflow to use the approved pinned SHA. This also updates
azure/setup-helmfromv4.3.0to the approvedv4.3.1commit.Does this PR introduce any user-facing change?
How was this patch tested?
Check list
New License Guide
incompatible-changes.mdto describe the incompatibility caused by this PR.