From 2b20f4c8f5b01eaf8812440589d667e518663b0d Mon Sep 17 00:00:00 2001 From: Matt Muller Date: Thu, 10 Oct 2024 11:46:11 -0400 Subject: [PATCH] Fixes from changed spec and new MPU behavior --- gems/aws-sdk-core/CHANGELOG.md | 2 +- .../plugins/checksum_algorithm.rb | 18 +++--- .../lib/aws-sdk-core/shared_config.rb | 2 +- .../aws/plugins/checksum_algorithm_spec.rb | 22 ++++---- gems/aws-sdk-s3/lib/aws-sdk-s3/client.rb | 2 +- .../lib/aws-sdk-s3/file_downloader.rb | 23 +------- .../lib/aws-sdk-s3/multipart_file_uploader.rb | 11 ++-- .../aws-sdk-s3/plugins/checksum_algorithm.rb | 17 ------ .../spec/object/download_file_spec.rb | 10 ---- .../spec/object/upload_file_spec.rb | 56 +++++++++---------- .../spec/plugins/checksum_algorithm_spec.rb | 6 -- 11 files changed, 57 insertions(+), 112 deletions(-) diff --git a/gems/aws-sdk-core/CHANGELOG.md b/gems/aws-sdk-core/CHANGELOG.md index 707b0354e78..be353df1180 100644 --- a/gems/aws-sdk-core/CHANGELOG.md +++ b/gems/aws-sdk-core/CHANGELOG.md @@ -3,7 +3,7 @@ Unreleased Changes * Feature - Always calculate request checksums for operations that support or require it. Supported config options are `WHEN_SUPPORTED` and `WHEN_REQUIRED`. The default value is `WHEN_SUPPORTED`. This option is configured in code with `:request_checksum_calculation`, in the shared config file as `request_checksum_calculation`, and in the ENV as `ENV['AWS_REQUEST_CHECKSUM_CALCULATION']`. -* Feature - Always validate response checksums for operations that support or require it. Supported config options are `WHEN_SUPPORTED` and `WHEN_REQUIRED`. The default value is `WHEN_SUPPORTED`. This option is configured in code with `:response_checksum_calculation`, in the shared config file as `response_checksum_calculation`, and in the ENV as `ENV['AWS_RESPONSE_CHECKSUM_CALCULATION']`. +* Feature - Always validate response checksums for operations that support or require it. Supported config options are `WHEN_SUPPORTED` and `WHEN_REQUIRED`. The default value is `WHEN_SUPPORTED`. This option is configured in code with `:response_checksum_validation`, in the shared config file as `response_checksum_validation`, and in the ENV as `ENV['AWS_response_checksum_validation']`. 3.209.1 (2024-09-25) ------------------ diff --git a/gems/aws-sdk-core/lib/aws-sdk-core/plugins/checksum_algorithm.rb b/gems/aws-sdk-core/lib/aws-sdk-core/plugins/checksum_algorithm.rb index 3c585f31894..bbb80b4e799 100644 --- a/gems/aws-sdk-core/lib/aws-sdk-core/plugins/checksum_algorithm.rb +++ b/gems/aws-sdk-core/lib/aws-sdk-core/plugins/checksum_algorithm.rb @@ -55,7 +55,7 @@ class ChecksumAlgorithm < Seahorse::Client::Plugin resolve_request_checksum_calculation(cfg) end - option(:response_checksum_calculation, + option(:response_checksum_validation, doc_default: 'WHEN_SUPPORTED', doc_type: 'String', docstring: <<~DOCS) do |cfg| @@ -69,7 +69,7 @@ class ChecksumAlgorithm < Seahorse::Client::Plugin response payloads of operations unless the checksum algorithm is supported and the `requestValidationModeMember` member is set to `ENABLED`. DOCS - resolve_response_checksum_calculation(cfg) + resolve_response_checksum_validation(cfg) end class << self @@ -113,15 +113,15 @@ def resolve_request_checksum_calculation(cfg) mode end - def resolve_response_checksum_calculation(cfg) - mode = ENV['AWS_RESPONSE_CHECKSUM_CALCULATION'] || - Aws.shared_config.response_checksum_calculation(profile: cfg.profile) || + def resolve_response_checksum_validation(cfg) + mode = ENV['AWS_response_checksum_validation'] || + Aws.shared_config.response_checksum_validation(profile: cfg.profile) || 'WHEN_SUPPORTED' mode = mode.upcase unless %w[WHEN_SUPPORTED WHEN_REQUIRED].include?(mode) raise ArgumentError, - 'expected :response_checksum_calculation or' \ - " ENV['AWS_RESPONSE_CHECKSUM_CALCULATION'] to be " \ + 'expected :response_checksum_validation or' \ + " ENV['AWS_response_checksum_validation'] to be " \ '`WHEN_SUPPORTED` or `WHEN_REQUIRED`.' end mode @@ -160,7 +160,7 @@ def call(context) context[:http_checksum] ||= {} # Set validation mode to enabled when supported. - if context.config.response_checksum_calculation == 'WHEN_SUPPORTED' + if context.config.response_checksum_validation == 'WHEN_SUPPORTED' enable_request_validation_mode(context) end @@ -229,7 +229,7 @@ def add_request_config_metric(config, metrics) end def add_response_config_metric(config, metrics) - case config.response_checksum_calculation + case config.response_checksum_validation when 'WHEN_SUPPORTED' metrics << 'FLEXIBLE_CHECKSUMS_RES_WHEN_SUPPORTED' when 'WHEN_REQUIRED' diff --git a/gems/aws-sdk-core/lib/aws-sdk-core/shared_config.rb b/gems/aws-sdk-core/lib/aws-sdk-core/shared_config.rb index 8dfe8cf73fa..1fea8bdbccd 100644 --- a/gems/aws-sdk-core/lib/aws-sdk-core/shared_config.rb +++ b/gems/aws-sdk-core/lib/aws-sdk-core/shared_config.rb @@ -213,7 +213,7 @@ def self.config_reader(*attrs) :adaptive_retry_wait_to_fill, :correct_clock_skew, :request_checksum_calculation, - :response_checksum_calculation, + :response_checksum_validation, :csm_client_id, :csm_enabled, :csm_host, diff --git a/gems/aws-sdk-core/spec/aws/plugins/checksum_algorithm_spec.rb b/gems/aws-sdk-core/spec/aws/plugins/checksum_algorithm_spec.rb index 43109d54618..9d5025a8dfa 100644 --- a/gems/aws-sdk-core/spec/aws/plugins/checksum_algorithm_spec.rb +++ b/gems/aws-sdk-core/spec/aws/plugins/checksum_algorithm_spec.rb @@ -127,31 +127,31 @@ module Plugins end end - describe 'response_checksum_calculation' do + describe 'response_checksum_validation' do it 'is configured to always verify by default' do - expect(client.config.response_checksum_calculation) + expect(client.config.response_checksum_validation) .to eq('WHEN_SUPPORTED') end it 'can be configured using shared config' do allow_any_instance_of(Aws::SharedConfig) - .to receive(:response_checksum_calculation) + .to receive(:response_checksum_validation) .and_return('WHEN_REQUIRED') - expect(client.config.response_checksum_calculation) + expect(client.config.response_checksum_validation) .to eq('WHEN_REQUIRED') end it 'can be configured using ENV with precedence over shared config' do allow_any_instance_of(Aws::SharedConfig) - .to receive(:response_checksum_calculation) + .to receive(:response_checksum_validation) .and_return('WHEN_SUPPORTED') - ENV['AWS_RESPONSE_CHECKSUM_CALCULATION'] = 'WHEN_REQUIRED' - expect(client.config.response_checksum_calculation) + ENV['AWS_response_checksum_validation'] = 'WHEN_REQUIRED' + expect(client.config.response_checksum_validation) .to eq('WHEN_REQUIRED') end - it 'raises when response_checksum_calculation is not valid' do - ENV['AWS_RESPONSE_CHECKSUM_CALCULATION'] = 'peccy' + it 'raises when response_checksum_validation is not valid' do + ENV['AWS_response_checksum_validation'] = 'peccy' expect { client }.to raise_error(ArgumentError, /WHEN_SUPPORTED/) end end @@ -373,7 +373,7 @@ def stub_client(client) it 'WHEN_REQUIRED; not ENABLED; does not validate the checksum' do client = checksum_client.new( stub_responses: true, - response_checksum_calculation: 'WHEN_REQUIRED' + response_checksum_validation: 'WHEN_REQUIRED' ) stub_client(client) resp = client.http_checksum_operation @@ -383,7 +383,7 @@ def stub_client(client) it 'WHEN_REQUIRED; ENABLED; validates the checksum' do client = checksum_client.new( stub_responses: true, - response_checksum_calculation: 'WHEN_REQUIRED' + response_checksum_validation: 'WHEN_REQUIRED' ) stub_client(client) resp = client.http_checksum_operation(validation_mode: 'ENABLED') diff --git a/gems/aws-sdk-s3/lib/aws-sdk-s3/client.rb b/gems/aws-sdk-s3/lib/aws-sdk-s3/client.rb index 2951635a939..f76b9554f32 100644 --- a/gems/aws-sdk-s3/lib/aws-sdk-s3/client.rb +++ b/gems/aws-sdk-s3/lib/aws-sdk-s3/client.rb @@ -360,7 +360,7 @@ class Client < Seahorse::Client::Base # where server-side-encryption is used with customer-provided keys. # This should only be disabled for local testing. # - # @option options [String] :response_checksum_calculation ("WHEN_SUPPORTED") + # @option options [String] :response_checksum_validation ("WHEN_SUPPORTED") # Determines when checksum validation will be performed on response payloads. Values are: # # * `WHEN_SUPPORTED` - (default) When set, checksum validation is performed on all diff --git a/gems/aws-sdk-s3/lib/aws-sdk-s3/file_downloader.rb b/gems/aws-sdk-s3/lib/aws-sdk-s3/file_downloader.rb index 9e5b3b91b73..beb7351c836 100644 --- a/gems/aws-sdk-s3/lib/aws-sdk-s3/file_downloader.rb +++ b/gems/aws-sdk-s3/lib/aws-sdk-s3/file_downloader.rb @@ -31,15 +31,7 @@ def download(destination, options = {}) key: options[:key], } @params[:version_id] = options[:version_id] if options[:version_id] - - # checksum_mode only supports the value "ENABLED" - # falsey values (false/nil) or "DISABLED" should be considered - # disabled and the api parameter should be unset. - if (checksum_mode = options.fetch(:checksum_mode, 'ENABLED')) - @params[:checksum_mode] = checksum_mode unless checksum_mode.upcase == 'DISABLED' - end @on_checksum_validated = options[:on_checksum_validated] - @progress_callback = options[:progress_callback] validate! @@ -67,11 +59,6 @@ def download(destination, options = {}) private def validate! - if @on_checksum_validated && @params[:checksum_mode] != 'ENABLED' - raise ArgumentError, "You must set checksum_mode: 'ENABLED' " + - "when providing a on_checksum_validated callback" - end - if @on_checksum_validated && !@on_checksum_validated.respond_to?(:call) raise ArgumentError, 'on_checksum_validated must be callable' end @@ -164,9 +151,7 @@ def multithreaded_get_by_parts(n_parts, total_size) def download_in_threads(pending, total_size) threads = [] - if @progress_callback - progress = MultipartProgress.new(pending, total_size, @progress_callback) - end + progress = MultipartProgress.new(pending, total_size, @progress_callback) if @progress_callback @thread_count.times do thread = Thread.new do begin @@ -208,9 +193,7 @@ def single_request return resp unless @on_checksum_validated - if resp.checksum_validated - @on_checksum_validated.call(resp.checksum_validated, resp) - end + @on_checksum_validated.call(resp.checksum_validated, resp) if resp.checksum_validated resp end @@ -251,7 +234,7 @@ def each(&block) end # @api private - class MultipartProgress + class MultipartProgress def initialize(parts, total_size, progress_callback) @bytes_received = Array.new(parts.size, 0) @part_sizes = parts.map(&:size) diff --git a/gems/aws-sdk-s3/lib/aws-sdk-s3/multipart_file_uploader.rb b/gems/aws-sdk-s3/lib/aws-sdk-s3/multipart_file_uploader.rb index 5b2274e89f3..331d283af2d 100644 --- a/gems/aws-sdk-s3/lib/aws-sdk-s3/multipart_file_uploader.rb +++ b/gems/aws-sdk-s3/lib/aws-sdk-s3/multipart_file_uploader.rb @@ -162,12 +162,11 @@ def upload_in_threads(pending, completed, options) completed_part = { etag: resp.etag, part_number: part[:part_number] - }.tap do |h| - # get checksum algorithm from request params (default or part) - algorithm = resp.context.params[:checksum_algorithm] - k = "checksum_#{algorithm.downcase}".to_sym - # get the requested checksum from the response - h[k] = resp.send(k) + } + # get the requested checksum from the response + if part[:checksum_algorithm] + k = "checksum_#{part[:checksum_algorithm].downcase}".to_sym + completed_part[k] = resp[k] end completed.push(completed_part) end diff --git a/gems/aws-sdk-s3/lib/aws-sdk-s3/plugins/checksum_algorithm.rb b/gems/aws-sdk-s3/lib/aws-sdk-s3/plugins/checksum_algorithm.rb index 15116c6e11e..c8a5ed4ba70 100644 --- a/gems/aws-sdk-s3/lib/aws-sdk-s3/plugins/checksum_algorithm.rb +++ b/gems/aws-sdk-s3/lib/aws-sdk-s3/plugins/checksum_algorithm.rb @@ -18,29 +18,12 @@ def call(context) end end - # create_multipart_upload is not modeled with httpChecksum. For - # multipart requests, we must set a default for the checksum algorithm - # for it to f unction. - class DefaultMultipartChecksumHandler < Seahorse::Client::Handler - def call(context) - default = Aws::Plugins::ChecksumAlgorithm::DEFAULT_CHECKSUM - context.params[:checksum_algorithm] ||= default - @handler.call(context) - end - end - def add_handlers(handlers, _config) handlers.add( SkipWholeMultipartGetChecksumsHandler, step: :initialize, operations: [:get_object] ) - - handlers.add( - DefaultMultipartChecksumHandler, - step: :initialize, - operations: [:create_multipart_upload] - ) end end end diff --git a/gems/aws-sdk-s3/spec/object/download_file_spec.rb b/gems/aws-sdk-s3/spec/object/download_file_spec.rb index be61d31f9b1..d22bae4752b 100644 --- a/gems/aws-sdk-s3/spec/object/download_file_spec.rb +++ b/gems/aws-sdk-s3/spec/object/download_file_spec.rb @@ -48,7 +48,6 @@ module S3 allow(client).to receive(:head_object).with({ bucket: 'bucket', key: 'small', - checksum_mode: 'ENABLED', part_number: 1 }).and_return( client.stub_data( @@ -62,7 +61,6 @@ module S3 bucket: 'bucket', key: 'small', part_number: 1, - checksum_mode: 'ENABLED', version_id: version_id }).and_return( client.stub_data( @@ -75,7 +73,6 @@ module S3 allow(client).to receive(:head_object).with({ bucket: 'bucket', key: 'large', - checksum_mode: 'ENABLED', part_number: 1 }).and_return( client.stub_data( @@ -87,7 +84,6 @@ module S3 allow(client).to receive(:head_object).with({ bucket: 'bucket', - checksum_mode: 'ENABLED', key: 'large' }).and_return( client.stub_data( @@ -99,7 +95,6 @@ module S3 allow(client).to receive(:head_object).with({ bucket: 'bucket', key: 'single', - checksum_mode: 'ENABLED', part_number: 1 }).and_return( client.stub_data( @@ -114,7 +109,6 @@ module S3 expect(client).to receive(:get_object).with({ bucket: 'bucket', key: 'small', - checksum_mode: 'ENABLED', response_target: path }).exactly(1).times @@ -126,7 +120,6 @@ module S3 expect(client).to receive(:get_object).with({ bucket: 'bucket', key: 'small', - checksum_mode: 'ENABLED', response_target: path, on_chunk_received: instance_of(Proc) }) do |args| @@ -149,7 +142,6 @@ module S3 expect(client).to receive(:head_object).with({ bucket: 'bucket', key: 'large', - checksum_mode: 'ENABLED', part_number: 1 }).exactly(1).times @@ -189,7 +181,6 @@ module S3 expect(client).to receive(:head_object).with({ bucket: 'bucket', key: 'single', - checksum_mode: 'ENABLED', part_number: 1 }).exactly(1).times @@ -204,7 +195,6 @@ module S3 expect(client).to receive(:get_object).with({ bucket: 'bucket', key: 'small', - checksum_mode: 'ENABLED', version_id: version_id, response_target: path }).exactly(1).times diff --git a/gems/aws-sdk-s3/spec/object/upload_file_spec.rb b/gems/aws-sdk-s3/spec/object/upload_file_spec.rb index a894d1782a0..0e147d43775 100644 --- a/gems/aws-sdk-s3/spec/object/upload_file_spec.rb +++ b/gems/aws-sdk-s3/spec/object/upload_file_spec.rb @@ -131,37 +131,37 @@ module S3 context 'large objects' do it 'uses multipart APIs for objects >= 100MB' do client.stub_responses(:create_multipart_upload, upload_id: 'id') - client.stub_responses(:upload_part, etag: 'etag', checksum_crc32: 'checksum') + client.stub_responses(:upload_part, etag: 'etag') expect(client).to receive(:complete_multipart_upload).with( bucket: 'bucket', key: 'key', upload_id: 'id', multipart_upload: { parts: [ - { checksum_crc32: 'checksum', etag: 'etag', part_number: 1 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 2 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 3 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 4 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 5 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 6 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 7 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 8 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 9 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 10 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 11 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 12 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 13 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 14 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 15 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 16 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 17 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 18 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 19 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 20 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 21 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 22 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 23 }, - { checksum_crc32: 'checksum', etag: 'etag', part_number: 24 } + { etag: 'etag', part_number: 1 }, + { etag: 'etag', part_number: 2 }, + { etag: 'etag', part_number: 3 }, + { etag: 'etag', part_number: 4 }, + { etag: 'etag', part_number: 5 }, + { etag: 'etag', part_number: 6 }, + { etag: 'etag', part_number: 7 }, + { etag: 'etag', part_number: 8 }, + { etag: 'etag', part_number: 9 }, + { etag: 'etag', part_number: 10 }, + { etag: 'etag', part_number: 11 }, + { etag: 'etag', part_number: 12 }, + { etag: 'etag', part_number: 13 }, + { etag: 'etag', part_number: 14 }, + { etag: 'etag', part_number: 15 }, + { etag: 'etag', part_number: 16 }, + { etag: 'etag', part_number: 17 }, + { etag: 'etag', part_number: 18 }, + { etag: 'etag', part_number: 19 }, + { etag: 'etag', part_number: 20 }, + { etag: 'etag', part_number: 21 }, + { etag: 'etag', part_number: 22 }, + { etag: 'etag', part_number: 23 }, + { etag: 'etag', part_number: 24 } ] } ) @@ -176,11 +176,7 @@ module S3 client.stub_responses(:complete_multipart_upload) expect(client).to receive(:upload_part).exactly(24).times do |args| args[:on_chunk_sent].call(args[:body], args[:body].size, args[:body].size) - double( - context: double(params: { checksum_algorithm: 'crc32' }), - checksum_crc32: 'checksum', - etag: 'etag' - ) + double(etag: 'etag') end callback = proc do |bytes, totals| expect(bytes.size).to eq(24) diff --git a/gems/aws-sdk-s3/spec/plugins/checksum_algorithm_spec.rb b/gems/aws-sdk-s3/spec/plugins/checksum_algorithm_spec.rb index 245dae36a2d..f338fcb0317 100644 --- a/gems/aws-sdk-s3/spec/plugins/checksum_algorithm_spec.rb +++ b/gems/aws-sdk-s3/spec/plugins/checksum_algorithm_spec.rb @@ -77,12 +77,6 @@ module Plugins resp = client.get_object(bucket: bucket, key: key, checksum_mode: 'ENABLED') expect(resp.context[:http_checksum][:validated]).to be_nil end - - it 'sets a default checksum algorithm for create_multipart_upload' do - resp = client.create_multipart_upload(bucket: bucket, key: key) - expect(resp.context.params[:checksum_algorithm]) - .to eq Aws::Plugins::ChecksumAlgorithm::DEFAULT_CHECKSUM - end end end end