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

SNOW-1296805 Remove option tmpdir from SnowflakeEncryptionUtil.decrypt_file #1987

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sfc-gh-stan
Copy link
Contributor

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-1296805

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am modifying authorization mechanisms
    • I am adding new credentials
    • I am modifying OCSP code
    • I am adding a new dependency
  3. Please describe how your code solves the related issue.

shutil.move(tmp_dst_file_name, self.full_dst_file_name)
is problematic where self.tmp_dir are on a different device than target directory (self.full_dst_file_name) in a network filesystem (e.g. persistent storage mounted to a docker container). Cross device file operations could cause the program to hang (especially on windows machines). We can eliminate this by making the decrypted intermediate file is written to the same directory as target directory (self.full_dst_file_name), where we know is writable.

Copy link
Collaborator

@sfc-gh-mkeller sfc-gh-mkeller left a comment

Choose a reason for hiding this comment

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

Great job. Same thing should be done to encrypt_file and forever put temp directory in the rear-view mirror.
Also, be aware that we started down on the .part naming, maybe we should follow that.

src/snowflake/connector/encryption_util.py Outdated Show resolved Hide resolved
@sfc-gh-mkeller
Copy link
Collaborator

Ohh and unfortunately this is a breaking change that needs to be announced ahead of time!

@sfc-gh-stan
Copy link
Contributor Author

Great job. Same thing should be done to encrypt_file and forever put temp directory in the rear-view mirror. Also, be aware that we started down on the .part naming, maybe we should follow that.

encrypt_file should be immune from the cross-device file operation issue because it just writes the encrypted file to a temp dir and later deletes that file. It's also more likely that the we don't have write access to the incoming directory. I prefer keeping tmpdir in encrypt_file for that reason.

For .part naming, what decrypt_file returns should look like <filename>.part<random_suffix>. Immediately after decrypt_file returns, this file gets renamed to just the final filename and the part file is unlinked:

shutil.move(tmp_dst_file_name, self.full_dst_file_name)
. I don't think we have to worry about the filenames here too much?

@sfc-gh-mkeller
Copy link
Collaborator

sfc-gh-mkeller commented Aug 2, 2024

For .part naming, what decrypt_file returns should look like .part<random_suffix>

This was done by design, I wanted to overwrite half-done download with new attempts, so a ctrl+c left file would be cleaned up on a second try

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