Fix building tiflash-sanitizer

[JaySon-Huang]

User description

Fix that git considers the directory as not safe, making tiflash sanitizer failed to build

https://ci.pingcap.net/blue/organizations/jenkins/tiflash-sanitizer-daily/detail/tiflash-sanitizer-daily/1987/pipeline/

[2024-07-01T18:48:01.172Z] fatal: detected dubious ownership in repository at '/home/jenkins/agent/workspace/tiflash-sanitizer-daily/tiflash'
[2024-07-01T18:48:01.172Z] To add an exception for this directory, call:
[2024-07-01T18:48:01.172Z] 
[2024-07-01T18:48:01.172Z]  git config --global --add safe.directory /home/jenkins/agent/workspace/tiflash-sanitizer-daily/tiflash

ref https://github.com/pingcap/tiflash/issues/7193

---

PR Type

Bug fix, Configuration changes

---

Description

• Added commands to configure Git safe directories to avoid build failures due to dubious ownership detection.
• Ensured the directories ${curws}/tiflash/contrib/tiflash-proxy and ${curws}/tiflash are marked as safe.

---

Changes walkthrough 📝

	Relevant files
Configuration changes	tiflash-sanitizer-daily.groovy Configure Git safe directories for tiflash-sanitizer build jenkins/pipelines/ci/tiflash/tiflash-sanitizer-daily.groovy Added commands to configure Git safe directories. Ensured the directories used in the build process are marked as safe. +5/-0

---

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[ti-chi-bot[bot]]

I have already done a preliminary review for you, and I hope to help you do a better job.

Based on the PR description, the key change made is to add two directories to the git safe directory list in order to fix the issue that caused tiflash-sanitizer to fail to build.

However, there are a few potential problems with this solution:

• Adding directories to the git safe directory list could potentially introduce security vulnerabilities, as it allows any code within those directories to execute during a git operation. It's important to ensure that the added directories are actually safe and necessary for the build process.
• The error message "detected dubious ownership in repository" indicates that there may be a deeper issue with the repository ownership or permissions, which could cause other problems down the line.

To address these potential problems, I would suggest the following fixes:

• Instead of blindly adding directories to the git safe directory list, carefully review the contents of each directory and determine if they are actually safe and necessary for the build process. If not, remove them from the list to minimize potential security vulnerabilities.
• Investigate the root cause of the "dubious ownership in repository" error message and fix it if possible. This could involve checking file ownership and permissions, or potentially rebuilding the repository from scratch.

[codiumai-pr-agent-pro[bot]]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	2
🧪 Relevant tests	No
🔒 Security concerns	No
⚡ Key issues to review	Global Configuration: The PR uses --global flag for git config, which affects all repositories on the machine. Consider using --local or --system if appropriate for the scope of this build.

[codiumai-pr-agent-pro[bot]]

PR Code Suggestions ✨

Category	Suggestion	Score
Security	Sanitize or validate the 'curws' variable to prevent command injection ___ **Ensure that the environment variable curws is properly sanitized or validated to prevent potential command injection vulnerabilities when used in shell commands.** [jenkins/pipelines/ci/tiflash/tiflash-sanitizer-daily.groovy [113-117]](https://github.com/PingCAP-QE/ci/pull/3019/files#diff-cf6e0f80b8d57ff16c04804cccb20aa2048f6041c6c159d041d860881b6fb9aeR113-R117) ```diff sh """ git version -git config --global --add safe.directory ${curws}/tiflash/contrib/tiflash-proxy -git config --global --add safe.directory ${curws}/tiflash +git config --global --add safe.directory `echo ${curws}/tiflash/contrib/tiflash-proxy | envsubst` +git config --global --add safe.directory `echo ${curws}/tiflash | envsubst` """ ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9 Why: Ensuring that the 'curws' variable is sanitized or validated is important for preventing potential command injection vulnerabilities, which is a significant security concern.	9
Best practice	Use local Git configuration instead of global to avoid conflicts ___ **To avoid potential global configuration conflicts and ensure that settings are scoped only to the current job, consider using repository-specific configuration instead of global Git configuration.** [jenkins/pipelines/ci/tiflash/tiflash-sanitizer-daily.groovy [113-117]](https://github.com/PingCAP-QE/ci/pull/3019/files#diff-cf6e0f80b8d57ff16c04804cccb20aa2048f6041c6c159d041d860881b6fb9aeR113-R117) ```diff sh """ git version -git config --global --add safe.directory ${curws}/tiflash/contrib/tiflash-proxy -git config --global --add safe.directory ${curws}/tiflash +git config --local --add safe.directory ${curws}/tiflash/contrib/tiflash-proxy +git config --local --add safe.directory ${curws}/tiflash """ ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8 Why: Using local Git configuration helps avoid potential global configuration conflicts and ensures that settings are scoped only to the current job, which is a good practice.	8
Maintainability	Use a script block for executing multiple shell commands ___ **Consider using a script block for better readability and maintainability when executing multiple shell commands. This approach also allows for easier error handling and debugging.** [jenkins/pipelines/ci/tiflash/tiflash-sanitizer-daily.groovy [113-117]](https://github.com/PingCAP-QE/ci/pull/3019/files#diff-cf6e0f80b8d57ff16c04804cccb20aa2048f6041c6c159d041d860881b6fb9aeR113-R117) ```diff -sh """ -git version -git config --global --add safe.directory ${curws}/tiflash/contrib/tiflash-proxy -git config --global --add safe.directory ${curws}/tiflash -""" +script { + sh "git version" + sh "git config --global --add safe.directory ${curws}/tiflash/contrib/tiflash-proxy" + sh "git config --global --add safe.directory ${curws}/tiflash" +} ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7 Why: Using a script block can improve readability and maintainability, and it allows for easier error handling and debugging. However, it is not crucial for functionality.	7
	Add comments to shell commands to explain their purpose ___ **To enhance the clarity and maintainability of the script, consider adding comments explaining the purpose of the Git configuration commands, especially for those who might not be familiar with the context.** [jenkins/pipelines/ci/tiflash/tiflash-sanitizer-daily.groovy [113-117]](https://github.com/PingCAP-QE/ci/pull/3019/files#diff-cf6e0f80b8d57ff16c04804cccb20aa2048f6041c6c159d041d860881b6fb9aeR113-R117) ```diff sh """ git version +# Add tiflash-proxy to safe directory to allow git operations without restrictions git config --global --add safe.directory ${curws}/tiflash/contrib/tiflash-proxy +# Add tiflash to safe directory for the same reason git config --global --add safe.directory ${curws}/tiflash """ ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 6 Why: Adding comments can enhance the clarity and maintainability of the script, especially for those who might not be familiar with the context. However, it is not essential for functionality.	6

[JaySon-Huang]

/hold

https://ci.pingcap.net/job/tiflash-sanitizer-daily/1990/console

[JaySon-Huang]

Try with https://ci.pingcap.net/job/tiflash-sanitizer-daily/1991/console

[ti-chi-bot[bot]]

I have already done a preliminary review for you, and I hope to help you do a better job.

Based on the pull request, the contributor has added commands to configure Git safe directories to avoid build failures due to dubious ownership detection. The contributor ensured that the directories ${curws}/tiflash/contrib/tiflash-proxy and ${curws}/tiflash are marked as safe.

It looks like the changes made in this pull request are related to build improvements. The changes are simple and don't seem to have any potential problems.

As for fixing suggestions, it would be better to add some information on the pull request to give some context to the reviewer. It would be helpful to know what the build failure was before the changes were made. Overall, the changes seem good to merge.

[JaySon-Huang]

Building: https://ci.pingcap.net/job/tiflash-sanitizer-daily/1992/console

[JaySon-Huang]

/hold cancel Building with sanitizer is OK https://ci.pingcap.net/job/tiflash-sanitizer-daily/1992/console

[ti-chi-bot[bot]]

[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

Needs approval from an approver in each of these files: - ~~[jenkins/OWNERS](https://github.com/PingCAP-QE/ci/blob/main/jenkins/OWNERS)~~ [wuhuizuo] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

[ti-chi-bot[bot]]

[LGTM Timeline notifier]

Timeline:

• 2024-07-03 06:00:47.125913871 +0000 UTC m=+1390573.611402703: :ballot_box_with_check: agreed by wuhuizuo.