Skip to content

Commit

Permalink
Package upload more atomic
Browse files Browse the repository at this point in the history
During package uploads, packages are now uploaded as temporary filenames
and then moved to their final names in bulk.

This prevents:
1. corrupted files with visible paths if the transfer is interrupted
2. a user downloading a corrupted file while it is being transferred

Ultimately, this improves (but cannot completely fix) the possibility of
race conditions between deb-s3 and other jobs downloading from the
repository.

Manifests are uploaded as before, since they have to be uploaded as soon
as possible, before the lock is released.
  • Loading branch information
Firobe committed Apr 7, 2022
1 parent 72d0f82 commit 89bee38
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 18 deletions.
24 changes: 12 additions & 12 deletions Gemfile.lock
Original file line number Diff line number Diff line change
@@ -1,30 +1,30 @@
PATH
remote: .
specs:
deb-s3 (0.11.2)
deb-s3 (0.11.4)
aws-sdk-s3 (~> 1)
thor (~> 0.19.0)

GEM
remote: https://rubygems.org/
specs:
aws-eventstream (1.1.1)
aws-partitions (1.442.0)
aws-sdk-core (3.113.1)
aws-eventstream (1.2.0)
aws-partitions (1.574.0)
aws-sdk-core (3.130.0)
aws-eventstream (~> 1, >= 1.0.2)
aws-partitions (~> 1, >= 1.239.0)
aws-partitions (~> 1, >= 1.525.0)
aws-sigv4 (~> 1.1)
jmespath (~> 1.0)
aws-sdk-kms (1.43.0)
aws-sdk-core (~> 3, >= 3.112.0)
aws-sdk-kms (1.55.0)
aws-sdk-core (~> 3, >= 3.127.0)
aws-sigv4 (~> 1.1)
aws-sdk-s3 (1.93.0)
aws-sdk-core (~> 3, >= 3.112.0)
aws-sdk-s3 (1.113.0)
aws-sdk-core (~> 3, >= 3.127.0)
aws-sdk-kms (~> 1)
aws-sigv4 (~> 1.1)
aws-sigv4 (1.2.3)
aws-sigv4 (~> 1.4)
aws-sigv4 (1.4.0)
aws-eventstream (~> 1, >= 1.0.2)
jmespath (1.4.0)
jmespath (1.6.1)
minitest (5.14.4)
rake (13.0.3)
thor (0.19.4)
Expand Down
23 changes: 18 additions & 5 deletions lib/deb/s3/cli.rb
Original file line number Diff line number Diff line change
Expand Up @@ -243,33 +243,46 @@ def upload(*files)
end
end

# Remember which files were moved to temporary filenames
filenames_to_move = []
log_action = lambda {|f| sublog("Transferring #{f}") }
atomic_action = lambda {|f|
log_action.call(f)
filenames_to_move.append(f)
}

# upload the manifest
log("Uploading new manifests to S3")
manifests.each_value do |manifest|
begin
manifest.write_manifests_to_s3 { |f| sublog("Transferring #{f}") }
manifest.write_manifests_to_s3 &log_action
rescue Deb::S3::Utils::AlreadyExistsError => e
error("Uploading manifest failed because: #{e}")
end
release.update_manifest(manifest)
end
release.write_to_s3 { |f| sublog("Transferring #{f}") }

release.write_to_s3 &log_action
# release the lock and upload the packages (assuming their names are unique)
if options[:lock] && @lock_acquired
Deb::S3::Lock.unlock(options[:codename], component, options[:arch], options[:cache_control])
log("Lock released.")
early_release = true
end
log("Uploading packages to S3")
log("Uploading packages to S3 as temporary files")
manifests.each_value do |manifest|
begin
manifest.write_packages_to_s3 { |f| sublog("Transferring #{f}") }
manifest.write_packages_to_s3 &atomic_action
rescue Deb::S3::Utils::AlreadyExistsError => e
error("Uploading manifest failed because: #{e}")
end
end

log("Moving temporary files to their final paths")
filenames_to_move.each do |f|
sublog("Moving #{f}")
Deb::S3::Utils.s3_finish_atomic_store(f)
end

log("Update complete.")
ensure
if !early_release && options[:lock] && @lock_acquired
Expand Down
2 changes: 1 addition & 1 deletion lib/deb/s3/manifest.rb
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ def write_packages_to_s3
# store any packages that need to be stored
@packages_to_be_upload.each do |pkg|
yield pkg.url_filename(@codename) if block_given?
s3_store(pkg.filename, pkg.url_filename(@codename), 'application/octet-stream; charset=binary', self.cache_control, self.fail_if_exists)
s3_atomic_store(pkg.filename, pkg.url_filename(@codename), 'application/octet-stream; charset=binary', self.cache_control, self.fail_if_exists)
end
end
end
Expand Down
30 changes: 30 additions & 0 deletions lib/deb/s3/utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,36 @@ def s3_store(path, filename=nil, content_type='application/octet-stream; charset
end
end

# Similar to s3_store, but uploads the file with a temporary filename instead,
# so the object is not visible with its final filename while being uploaded.
# The files then need to be moved to their actual filename with
# s3_finish_atomic_store
def s3_atomic_store(path, filename=nil, content_type='application/octet-stream; charset=binary', cache_control=nil, fail_if_exists=false, suffix=".deb-s3-temp")
filename = File.basename(path) unless filename
# TODO: check that the filename does not exist
temp_filename = filename + suffix
s3_store(path, filename=temp_filename, content_type, cache_control, fail_if_exists)
end

# See s3_atomic_store
def s3_finish_atomic_store(filename, suffix=".deb-s3-temp")
temp_filename = filename + suffix
# Can only copy up to 5 Gb files
s3_copy(temp_filename, filename)
# There is no move is S3, only copy then delete.
s3_remove(temp_filename)
end

def s3_copy(filename_from, filename_to)
full_from_path = Deb::S3::Utils.bucket + "/" + s3_path(filename_from)
Deb::S3::Utils.s3.copy_object(
:bucket => Deb::S3::Utils.bucket,
:acl => Deb::S3::Utils.access_policy,
:copy_source => full_from_path,
:key => s3_path(filename_to)
)
end

def s3_remove(path)
if s3_exists?(path)
Deb::S3::Utils.s3.delete_object(
Expand Down

0 comments on commit 89bee38

Please sign in to comment.