Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

24.8 leak scanning script #696

Closed
wants to merge 3 commits into from
Closed

Conversation

strtgbb
Copy link
Collaborator

@strtgbb strtgbb commented Mar 20, 2025

Changelog category (leave one):

  • CI Fix or Improvement (changelog entry is not required)

Scan all files uploaded to S3 for [A-Z_]*(SECRET|PASSWORD)[A-Z_]*

@strtgbb strtgbb changed the title leak scanning script 24.8 leak scanning script Mar 20, 2025
sudo apt-get install -y dpkg-dev rpm2cpio cpio
- name: Run leak check 1
run: |
python3 scripts/scan_s3_artifacts.py altinity-build-artifacts ${{ env.PREFIX }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@Enmk Enmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It checks for leaks after the leak happened, IMO it would be more productive to scan files before uploading anything, and failing the job if there is a leak.

That way we are preventing the leak rather than detecting it (which would require us to mitigate the leak, which is quite elaborate) and also save enormous amount of time and traffic on pulling every single artifact back from S3 to the runner.

I suggest to modify S3Helper.upload_file/S3Helper.upload_build_directory_to_s3 (maybe something else?)
https://github.com/Altinity/ClickHouse/blob/releases/24.8.14/tests/ci/s3_helper.py

@strtgbb
Copy link
Collaborator Author

strtgbb commented Mar 25, 2025

Checking after is what was initially discussed.
The traffic usage is a good point. Checking before will be cheaper as long as we're using S3.
upload_build_directory_to_s3 ultimately calls upload_file. Unless it contains compressed files, I don't think it needs any special handling.
I don't think there is enough in common between that proposal and the existing script, I will create a new PR.

@strtgbb
Copy link
Collaborator Author

strtgbb commented Mar 26, 2025

See #701

@strtgbb strtgbb closed this Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants