fileserver: migrate pingcap/tidb release-8.5 artifact downloads to OCI#4293
fileserver: migrate pingcap/tidb release-8.5 artifact downloads to OCI#4293ti-chi-bot[bot] merged 5 commits intomainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the artifact download process for the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully migrates artifact downloads for release-8.5 pipelines from fileserver.pingcap.net to an OCI registry. However, a critical command injection vulnerability has been identified in multiple Jenkinsfiles. This vulnerability arises because untrusted PR titles are used to compute OCI tags, which are then directly interpolated into shell scripts, allowing arbitrary command execution on the CI infrastructure. Additionally, two key issues were noted: the removal of the artifact upload mechanism in pull_build.groovy which could impact downstream jobs, and an inconsistency in pull_e2e_test.groovy where the new download logic does not utilize the dedicated utils container. Addressing the command injection by sanitizing OCI tags or using environment variables, along with resolving the identified inconsistencies, will significantly improve the security and consistency of the new CI workflow.
I am having trouble creating individual review comments. Click here to see my feedback.
pipelines/pingcap/tidb/release-8.5/pull_e2e_test.groovy (59-62)
This section is vulnerable to a critical command injection. The PR title (REFS.pulls[0].title) is used to compute OCI tags, which are then directly interpolated into a shell script. This allows an attacker to execute arbitrary commands on the Jenkins agent via a malicious PR title (e.g., | pd=;id;). Additionally, for consistency with other pipelines in this PR, the artifact download should be performed within the utils container, which also makes the script cleaner by using dir("bin") instead of cd bin.
container("utils") {
dir("bin") {
retry(3) {
sh label: 'download binary', script: """
${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --pd=${OCI_TAG_PD} --tikv=${OCI_TAG_TIKV}
"""
}
}
}
pipelines/pingcap/tidb/release-8.5/pull_check2.groovy (66-68)
The PR title (REFS.pulls[0].title) is used to compute OCI tags (OCI_TAG_PD, OCI_TAG_TIKV) which are then interpolated into a shell script executed via the sh step. Since the PR title is untrusted and the tag computation in component.computeArtifactOciTagFromPR does not sufficiently sanitize shell metacharacters (it only excludes whitespace and backslash), an attacker can inject arbitrary commands into the Jenkins pipeline by crafting a malicious PR title (e.g., feat: something | pd=;id;). This leads to Command Injection on the Jenkins agent.
pipelines/pingcap/tidb/release-8.5/pull_integration_common_test.groovy (76-78)
The PR title (REFS.pulls[0].title) is used to compute OCI tags which are then interpolated into a shell script. This allows for command injection via a malicious PR title. An attacker could use a title like | pd=;id; to execute arbitrary commands on the Jenkins agent.
pipelines/pingcap/tidb/release-8.5/pull_integration_copr_test.groovy (76-78)
The PR title (REFS.pulls[0].title) is used to compute OCI tags which are then interpolated into a shell script. This allows for command injection via a malicious PR title. An attacker could use a title like | pd=;id; to execute arbitrary commands on the Jenkins agent.
pipelines/pingcap/tidb/release-8.5/pull_integration_ddl_test.groovy (78-80)
The PR title (REFS.pulls[0].title) is used to compute OCI tags which are then interpolated into a shell script. This allows for command injection via a malicious PR title. An attacker could use a title like | pd=;id; to execute arbitrary commands on the Jenkins agent.
pipelines/pingcap/tidb/release-8.5/pull_integration_jdbc_test.groovy (75-77)
The PR title (REFS.pulls[0].title) is used to compute OCI tags which are then interpolated into a shell script. This allows for command injection via a malicious PR title. An attacker could use a title like | pd=;id; to execute arbitrary commands on the Jenkins agent.
pipelines/pingcap/tidb/release-8.5/pull_integration_mysql_test.groovy (76-78)
The PR title (REFS.pulls[0].title) is used to compute OCI tags which are then interpolated into a shell script. This allows for command injection via a malicious PR title. An attacker could use a title like | pd=;id; to execute arbitrary commands on the Jenkins agent.
pipelines/pingcap/tidb/release-8.5/pull_integration_nodejs_test.groovy (74-76)
The PR title (REFS.pulls[0].title) is used to compute OCI tags which are then interpolated into a shell script. This allows for command injection via a malicious PR title. An attacker could use a title like | pd=;id; to execute arbitrary commands on the Jenkins agent.
pipelines/pingcap/tidb/release-8.5/pull_integration_python_orm_test.groovy (72-74)
The PR title (REFS.pulls[0].title) is used to compute OCI tags which are then interpolated into a shell script. This allows for command injection via a malicious PR title. An attacker could use a title like | pd=;id; to execute arbitrary commands on the Jenkins agent.
pipelines/pingcap/tidb/release-8.5/pull_integration_tidb_tools_test.groovy (82-84)
The PR title (REFS.pulls[0].title) is used to compute OCI tags which are then interpolated into a shell script. This allows for command injection via a malicious PR title. An attacker could use a title like | pd=;id; to execute arbitrary commands on the Jenkins agent.
pipelines/pingcap/tidb/release-8.5/pull_build.groovy (64-86)
This change removes the logic for packaging and uploading the tidb-server build artifact. This is a significant functional change for the pull_build pipeline, as it will no longer publish its primary artifact for other jobs to consume. While this aligns with migrating away from fileserver.pingcap.net, it implies a shift in the CI workflow where downstream jobs might need to build tidb-server from source themselves, potentially increasing overall CI execution time. Was this change in workflow intended?
Jenkins Replay Status
|
…-from-4274 # Conflicts: # pipelines/pingcap/tidb/release-8.5/pull_check2.groovy # pipelines/pingcap/tidb/release-8.5/pull_e2e_test.groovy
![ti-chi-bot[bot]](https://avatars.githubusercontent.com/in/214286?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR migrates the artifact download process for release-8.5 pipelines of PingCAP's TiDB repository from fileserver.pingcap.net to OCI-based artifact retrieval via the utils container. The changes span YAML and Groovy files, introducing OCI-specific configurations and removing legacy file server dependencies. The implementation aligns with existing patterns in the latest jobs and replaces hardcoded artifact URLs with dynamic OCI tags. The approach is consistent and clean but introduces opportunities for optimization and enhancements in configurability and error handling.
Critical Issues
- Insufficient Error Handling During Artifact Download
- Files: Multiple Groovy files (e.g.,
pull_check2.groovy,pull_common_test.groovy) - Lines: Artifact download steps using
download_pingcap_oci_artifact.sh - Issue: The retry mechanism in case of failure (
retry(3)) is helpful but lacks detailed error reporting if retries fail. Failures could be due to network issues, authentication problems, or incorrect OCI tags, but no specific error messages are logged. - Suggestion: Enhance error handling by capturing the output of
download_pingcap_oci_artifact.shand logging it for debugging:retry(3) { sh label: 'download tidb components', script: """ ${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh \ --pd=${OCI_TAG_PD} --tikv=${OCI_TAG_TIKV} || echo "Artifact download failed. Check OCI_TAG_PD=${OCI_TAG_PD} and OCI_TAG_TIKV=${OCI_TAG_TIKV}" """ }
- Files: Multiple Groovy files (e.g.,
Code Improvements
-
Hardcoded OCI Host Configuration
- Files: Multiple Groovy files
- Lines:
environment { OCI_ARTIFACT_HOST = 'hub-zot.pingcap.net/mirrors/hub' } - Issue: The OCI host URL is hardcoded across all pipelines. If the host changes, updating all files individually will be error-prone.
- Suggestion: Extract the host URL into a centralized configuration file or environment variable that can be reused across pipelines:
environment { OCI_ARTIFACT_HOST = env.OCI_ARTIFACT_HOST ?: 'hub-zot.pingcap.net/mirrors/hub' }
-
Duplicated Artifact Download Logic
- Files: Multiple Groovy files (e.g.,
pull_check2.groovy,pull_common_test.groovy, etc.) - Lines: Artifact download steps using
download_pingcap_oci_artifact.sh - Issue: The artifact download logic is repeated across pipelines, increasing maintenance overhead.
- Suggestion: Extract the artifact download logic into a shared pipeline step or a reusable function:
Replace individual calls with:
def downloadArtifacts(String workspace, String pdTag, String tikvTag) { sh label: 'download tidb components', script: """ ${workspace}/scripts/artifacts/download_pingcap_oci_artifact.sh \ --pd=${pdTag} --tikv=${tikvTag} """ }
container("utils") { dir("bin") { retry(3) { downloadArtifacts(WORKSPACE, OCI_TAG_PD, OCI_TAG_TIKV) } } }
- Files: Multiple Groovy files (e.g.,
Best Practices
-
Missing Documentation for OCI Tags
- Files: Multiple Groovy files
- Lines:
final OCI_TAG_PD,final OCI_TAG_TIKV - Issue: The computation of OCI tags is not documented, making it unclear to future developers how these tags are derived or validated.
- Suggestion: Add comments explaining the logic for
computeArtifactOciTagFromPR:// Computes the OCI tag for the given component based on the base reference, // pull request title, and target branch. Ensure the base_ref and title are valid. final OCI_TAG_PD = component.computeArtifactOciTagFromPR('pd', REFS.base_ref, REFS.pulls[0].title, 'master')
-
Testing Coverage
- Files: Scripts like
download_pingcap_oci_artifact.shused in pipelines - Issue: No information is provided about testing the artifact download script with various OCI tags and environments.
- Suggestion: Ensure the script is tested with both valid and invalid tags, network disruptions, and authentication errors. Add unit tests or mock tests for the script in the repository.
- Files: Scripts like
-
Resource Allocation in YAML Files
- Files: Multiple pod configuration YAML files (e.g.,
pod-pull_check2.yaml,pod-pull_e2e_test.yaml) - Lines:
resources { requests: { memory: 256Mi, cpu: 100m }, limits: { cpu: "1", memory: 4Gi } } - Issue: Resource requests and limits for the
utilscontainer are consistent across all files but might not reflect actual requirements. - Suggestion: Verify if the specified resources (e.g., memory: 256Mi, cpu: 100m) match the usage patterns of the
utilscontainer. Excessive allocation can lead to inefficient resource utilization.
- Files: Multiple pod configuration YAML files (e.g.,
Summary of Recommendations
- Improve error handling and logging for artifact downloads to aid debugging.
- Centralize the OCI host configuration to reduce duplication.
- Refactor repeated artifact download steps into reusable functions.
- Document tag computation logic for clarity and maintainability.
- Verify resource allocations in YAML files and adjust if needed.
- Ensure testing coverage for download scripts under various conditions.
|
Replay focus verification for 6 non-optional Current snapshot:
Live task-item status is continuously updated in the marker comment ( |
Move to agent none and use per-stage Kubernetes agents for isolation.
Checkout into REFS.repo and standardize binary cache key via
prow.getCacheKey('binary', REFS). Reduce retries for artifact downloads,
simplify container usage, add comment for OCI_ARTIFACT_HOST, and remove
Debug and post-reporting steps to streamline the pipeline.
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This pull request migrates artifact downloads for the release-8.5 pipelines of the PingCAP/TiDB project from a legacy file server (fileserver.pingcap.net) to OCI-based artifact pulls. The implementation aligns with previously updated latest pipelines by introducing OCI tags and utilizing a shared script (download_pingcap_oci_artifact.sh) within a dedicated utils container. Overall, the PR is well-structured and achieves the intended migration, but some improvements are suggested to enhance maintainability, reduce redundancy, and ensure better error handling.
Critical Issues
None identified. The changes appear logically correct, and no security vulnerabilities or performance bottlenecks are evident. Validation for Jenkinsfile changes has been performed, reducing risk of regressions.
Code Improvements
-
Redundant OCI Tag Computation:
- Files: All
.groovypipeline files (e.g.,pull_check2.groovy,pull_common_test.groovy, etc.) - Issue:
OCI_TAG_PDandOCI_TAG_TIKVare computed redundantly in each pipeline file. This logic could be centralized to improve maintainability and reduce duplication. - Suggestion: Move OCI tag computation to a shared helper method or script, which can be imported or invoked across pipelines. Example:
final OCI_TAGS = component.computeArtifactOciTags(REFS.base_ref, REFS.pulls[0].title, 'master')
- Files: All
-
Error Handling for Artifact Download:
- Files: All
.groovypipeline files wheredownload_pingcap_oci_artifact.shis invoked (e.g.,pull_check2.groovy,pull_common_test.groovy, etc.) - Issue: The script invocation lacks proper error handling. A failed retry could result in silent pipeline failure or misbehavior.
- Suggestion: Add explicit error checks after script execution. Example:
sh label: "download tidb components", script: """ ${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --pd=${OCI_TAG_PD} --tikv=${OCI_TAG_TIKV} """ if (currentBuild.result == 'FAILURE') { error("Artifact download failed") }
- Files: All
-
Resource Requests for
utilsContainer:- Files: YAML files (e.g.,
pod-pull_check2.yaml,pod-pull_integration_common_test.yaml) - Issue: The
utilscontainer requests 256Mi memory while limiting to 4Gi. This discrepancy could lead to over-allocation. Additionally, CPU requests (100m) are low compared to the 1 core limit. - Suggestion: Reassess resource requests and limits for the
utilscontainer based on actual usage patterns. Consider balanced values like:resources: requests: memory: 1Gi cpu: 500m limits: memory: 2Gi cpu: 1
- Files: YAML files (e.g.,
Best Practices
-
Script Invocation Path Consistency:
- Files: All
.groovyfiles usingdownload_pingcap_oci_artifact.sh - Issue: The script's path (
${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh) is hardcoded, which may lead to issues with workspace changes. - Suggestion: Use a configurable path or ensure the script is verified to exist before invocation:
if (!fileExists("${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh")) { error("Artifact download script is missing") }
- Files: All
-
Documentation for New Environment Variables:
- Files: All
.groovyfiles whereOCI_ARTIFACT_HOSTis introduced. - Issue: The purpose of
OCI_ARTIFACT_HOSTis not documented. Future maintainers may face difficulty understanding its role. - Suggestion: Add comments explaining the variable's purpose and usage. Example:
// OCI_ARTIFACT_HOST: Cache mirror for OCI artifact pulls to improve performance environment { OCI_ARTIFACT_HOST = 'hub-zot.pingcap.net/mirrors/hub' }
- Files: All
-
Testing Coverage:
- Files: All pipeline files.
- Issue: The validation step covers Jenkinsfile syntax, but there is no mention of functional testing for the artifact download logic.
- Suggestion: Add unit or integration tests for
download_pingcap_oci_artifact.shto ensure correct behavior when pulling artifacts. Simulated tests for failure scenarios (e.g., invalid tags or network issues) should also be included.
-
Naming Conventions:
- Files:
.groovypipeline files. - Issue: Variable names like
OCI_TAG_PDandOCI_TAG_TIKVare clear but inconsistent withOCI_ARTIFACT_HOST. Consider uniform naming conventions, e.g.,OCI_PD_TAG,OCI_TIKV_TAG.
- Files:
By addressing these improvement opportunities, the PR can achieve greater maintainability, robustness, and alignment with best practices.
|
Replay follow-up final snapshot (focused 6 non-optional release-8.5 jobs):
Status marker comment has been updated to final ( |
|
Retried
|
|
Update: replay
|
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This pull request migrates the artifact download mechanism for the release-8.5 pipelines of the PingCAP TiDB project from fileserver.pingcap.net to OCI. The approach uses the download_pingcap_oci_artifact.sh script and incorporates a new utils container for downloading artifacts. The changes span multiple YAML and Groovy pipeline files, ensuring consistency across the migration. The PR is generally well-organized, but there are areas that could benefit from improvements to error handling, resource allocation, and documentation.
Critical Issues
-
Error Handling in Artifact Download (Groovy Files):
- Files: All modified
.groovyfiles (e.g.,pipelines/pingcap/tidb/release-8.5/pull_check2.groovyat line 50). - Issue: The artifact download script (
download_pingcap_oci_artifact.sh) is wrapped in a retry mechanism, but no specific error handling logic is present to ensure recoverability or clarity in case of persistent failures. - Suggested Solution:
Add error logging and diagnostic information after retry attempts fail:retry(3) { sh label: 'download tidb components', script: """ ${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --pd=${OCI_TAG_PD} --tikv=${OCI_TAG_TIKV} || echo "Artifact download failed for PD or TiKV. Check network connectivity or artifact availability." """ }
- Files: All modified
-
Resource Allocation for
utilsContainer (YAML Files):- Files: All modified
.yamlfiles (e.g.,pipelines/pingcap/tidb/release-8.5/pod-pull_check2.yamlat line 6). - Issue: The
utilscontainer is assigned resource limits of4Gimemory and1CPU. Depending on the expected workload, this could be excessive or insufficient. Resource usage should be benchmarked to ensure optimal allocations. - Suggested Solution:
Investigate actual resource requirements during artifact download and adjust limits. For example:resources: requests: memory: 128Mi cpu: 100m limits: cpu: "500m" memory: 2Gi
- Files: All modified
Code Improvements
-
Centralize OCI Tag Computation:
- Files: All modified
.groovyfiles (e.g.,pipelines/pingcap/tidb/release-8.5/pull_check2.groovyat lines 7–8). - Issue: The
OCI_TAG_*computation logic is repeated across multiple pipeline files, increasing maintenance complexity. - Suggested Solution:
Move the OCI tag computation logic to a shared Groovy utility function or module:def computeOciTags(refs) { return [ pd: component.computeArtifactOciTagFromPR('pd', refs.base_ref, refs.pulls[0].title, 'master'), tikv: component.computeArtifactOciTagFromPR('tikv', refs.base_ref, refs.pulls[0].title, 'master') ] }
- Files: All modified
-
Remove Unused Debug Code:
- Files:
pipelines/pingcap/tidb/release-8.5/pull_build.groovyat lines 17–92. - Issue: Stages like
Debug infoand redundantarchiveArtifactssteps are removed. While this is intentional, it would be better to replace them with concise logging or diagnostics for pipeline validation and debugging. - Suggested Solution:
Add minimal debug information, such as a single environment variable dump for critical parameters:stage('Debug info') { steps { sh label: 'Print Environment Variables', script: 'printenv | grep -E "OCI_TAG|K8S_NAMESPACE|JOB_NAME"' } }
- Files:
Best Practices
-
Testing Coverage:
- Files: All modified
.groovyand.yamlfiles. - Issue: The PR description mentions validation via
verify-jenkins-pipeline-file.sh, but there is no mention of functional tests for artifact downloads. - Suggested Solution:
Add integration tests to verify OCI artifact downloads and ensure compatibility across different scenarios.
- Files: All modified
-
Documentation for Resource Changes:
- Files: All modified
.yamlfiles. - Issue: Resource changes for the
utilscontainer are not documented, leaving ambiguity around why these specific limits were chosen. - Suggested Solution:
Add comments in the YAML files to explain resource configurations:resources: requests: memory: 256Mi # Minimum memory required for artifact download operations cpu: 100m # Low CPU usage expected limits: cpu: "1" # Upper bound for heavy workloads memory: 4Gi # Set to handle large artifact downloads
- Files: All modified
-
Streamline Naming Conventions:
- Files: All
.groovyand.yamlfiles. - Issue: Variable names like
OCI_TAG_PDandOCI_TAG_TIKVcould be generalized for better readability and extensibility. - Suggested Solution:
Use a more generic naming convention:def ociTags = computeOciTags(REFS) sh label: 'download tidb components', script: """ ${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --pd=${ociTags.pd} --tikv=${ociTags.tikv} """
- Files: All
-
Eliminate Code Duplication:
- Files: Multiple
.groovyfiles (e.g.,pull_check2.groovy,pull_common_test.groovy). - Issue: Identical logic for OCI artifact downloads is repeated across many pipelines.
- Suggested Solution:
Extract common functionality into reusable methods or a shared pipeline step library.
- Files: Multiple
Conclusion
This PR is well-structured and introduces necessary changes to migrate pipelines to OCI artifact downloads. However, addressing error handling, resource allocation, testing, and code reuse will enhance reliability, maintainability, and clarity.
|
Identified and fixed the pod-yaml root cause for Root cause from replay
Fix:
Validation replay after fix:
Also updated the marker status comment to reflect this retry. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wuhuizuo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 participants
Summary
pipelines/pingcap/tidb/release-8.5/**changes from fileserver: migrate pingcap/tidb artifact downloads to OCI registry #4274 into a standalone PRrelease-8.5pipelines away fromfileserver.pingcap.netto OCI artifact pullslatestjobs (OCI_TAG_*+download_pingcap_oci_artifact.shviautilscontainer)Scope
pipelines/pingcap/tidb/release-8.5/Validation
.ci/verify-jenkins-pipeline-file.shRef #4274
Fixes #4292