Skip to content

refactored archive installer #12771

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Jun 27, 2025
Merged

refactored archive installer #12771

merged 15 commits into from
Jun 27, 2025

Conversation

macnev2013
Copy link
Contributor

@macnev2013 macnev2013 commented Jun 18, 2025 •
edited
Loading

Motivation

Refactors the ArchiveDownloadAndExtractInstaller class to improve modularity and adds checksum verification support for enhanced security and reliability when downloading packages.

Changes

  • extracted reusable methods from _install method

    • _download_and_extract_archive - downloads and extracts archive
    • _handle_single_directory_extraction - handles logic for extracting all files in a single root directory

  • improves cleanup with proper try/finally block

  • Added checksum URLs for FfmpegPackageInstaller and TerraformPackageInstaller

  • Added checksum verification: New localstack.utils.checksum module with:

    • Support for multiple checksum formats (Standard, BSD, Apache BSD)
    • Automatic algorithm detection (MD5, SHA1, SHA256, SHA512)
    • Comprehensive error handling with custom exceptions
    • Integration with download_and_extract function

@macnev2013 macnev2013 requested a review from alexrashed as a code owner June 18, 2025 07:24
@macnev2013 macnev2013 added the semver: patch Non-breaking changes which can be included in patch releases label Jun 18, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 18, 2025 •
edited
Loading

Test Results - Preflight, Unit

21 744 tests  +17   20 087 ✅ +17   6m 20s ⏱️ -1s
     1 suites ± 0    1 657 💤 ± 0 
     1 files   ± 0        0 ❌ ± 0 

Results for commit 58328ae. ± Comparison against base commit 5783fa3.

♻️ This comment has been updated with latest results.

@macnev2013 macnev2013 marked this pull request as draft June 18, 2025 07:29
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 18, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 9s ⏱️ +2s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 58328ae. ± Comparison against base commit 5783fa3.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 18, 2025 •
edited
Loading

Test Results - Alternative Providers

987 tests  ±0   589 ✅ ±0   29m 49s ⏱️ -50s
  4 suites ±0   398 💤 ±0 
  4 files   ±0     0 ❌ ±0 

Results for commit 7683b05. ± Comparison against base commit ea0a194.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 18, 2025 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   2h 21m 11s ⏱️
5 253 tests 4 327 ✅ 926 💤 0 ❌
5 259 runs  4 327 ✅ 932 💤 0 ❌

Results for commit 58328ae.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 18, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 43m 57s ⏱️ + 1m 3s
4 896 tests ±0  4 122 ✅ ±0  774 💤 ±0  0 ❌ ±0 
4 898 runs  ±0  4 122 ✅ ±0  776 💤 ±0  0 ❌ ±0 

Results for commit 58328ae. ± Comparison against base commit 5783fa3.

♻️ This comment has been updated with latest results.

@macnev2013 macnev2013 marked this pull request as ready for review June 20, 2025 09:26
@macnev2013 macnev2013 marked this pull request as draft June 23, 2025 11:12
@macnev2013 macnev2013 marked this pull request as ready for review June 25, 2025 07:05
alexrashed
alexrashed reviewed Jun 25, 2025
View reviewed changes
Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this great push! In my opinion, this is really a big step towards introducing integrity checks on a broader scope, which will enable us in the future to use external sources without lowering the trust, and we can avoid download issues / detect MITM attacks! 🚀

Everything is looking good, I just added a few comments, questions, and nitpicks. Happy to discuss them directly :)

@macnev2013 macnev2013 requested a review from alexrashed June 26, 2025 09:30
alexrashed
alexrashed approved these changes Jun 26, 2025
View reviewed changes
Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Nice! Thanks a lot for jumping on all the comments! In my opinion, the result is really looking great! I added a few nitpicks, you can maybe directly follow up on these, but nothing is blocking a merge from my point. 💯

localstack-core/localstack/utils/checksum.py
Comment on lines +317 to +328
# Detect algorithm based on checksum length
checksum_length = len(expected_checksum)
if checksum_length == 32:
algorithm = "md5"
elif checksum_length == 40:
algorithm = "sha1"
elif checksum_length == 64:
algorithm = "sha256"
elif checksum_length == 128:
algorithm = "sha512"
else:
raise ChecksumException(f"Unsupported checksum length: {checksum_length}")
Copy link
Member

Choose a reason for hiding this comment

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

nit: This seems a bit brittle and redundant. What if there are two different hash algorithms with the same size? Shouldn't the checksum parsers return the algorithm together with the hashes? Definitely not blocking a merge though :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Changing this would require updating a lot of other places. Would be great if we can tackle this in a follow-up PR.

@macnev2013 macnev2013 added this to the 4.6 milestone Jun 26, 2025
@macnev2013 macnev2013 merged commit 093ddbe into master Jun 27, 2025
39 checks passed
@macnev2013 macnev2013 deleted the bigdata-mirror branch June 27, 2025 04:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexrashed alexrashed alexrashed approved these changes

Assignees
No one assigned
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.6
Development

Successfully merging this pull request may close these issues.
2 participants
@macnev2013 @alexrashed