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

Skip multipart if body size is less than chunk. #283

Merged
merged 4 commits into from
Aug 1, 2016

Conversation

brettcave
Copy link
Contributor

Fixes fog/fog#3899

@geemus did not write a test for this, spent a bit of time dabbling with shindo(nt) and have a real (non mock) test but couldn't get AWS auth params passed to run the test. Have tested manually with 0b, 5242879b and 10m files and all upload correctly with multipart being set on file size >= multipart_chunk.

@brettcave
Copy link
Contributor Author

@geemus thinking about also adding in a minimum chunk size check in this PR too. According to http://docs.aws.amazon.com/AmazonS3/latest/API/mpUploadComplete.html the minimum chunk size is 5242880. This means that if I set the chunk size to 5242879 and upload a 5242880 file, the upload will fail with something similar to:

Excon::Error::BadRequest: Expected(200) <=> Actual(400 Bad Request)
excon.error.response
  :body          => "<Error><Code>EntityTooSmall</Code><Message>Your proposed upload is smaller than the minimum allowed size</Message><ProposedSize>5242879</ProposedSize><MinSizeAllowed>5242880</MinSizeAllowed><PartNumber>1</PartNumber>....

Something like this would fail fast on the client before attempting the upload (instead of uploading 5242879 bytes first and then failing), but not sure if service-side checks should fall in the scope of the library. Your thoughts?

--- a/lib/fog/aws/models/storage/file.rb
+++ b/lib/fog/aws/models/storage/file.rb
@@ -205,6 +205,8 @@ module Fog
           options['x-amz-storage-class'] = storage_class if storage_class
           options.merge!(encryption_headers)

+          raise "multipart_chunk_size too small, minimum 5242880b" if multipart_chunk_size && multipart_chunk_size<5242880
+
           if multipart_chunk_size && Fog::Storage.get_body_size(body) >= multipart_chunk_size && body.respond_to?(:read)

@geemus
Copy link
Member

geemus commented Jul 29, 2016

Ah, didn't know about that minimum. Yeah, maybe we should just do if body < 5242879 so that it would "just work" instead of erroring. Maybe fail fast would be better though, as you suggest. What do you think?

@brettcave
Copy link
Contributor Author

It might be better to validate the parameter on the setter, so have overridden it in the same way that acl does (for consistency). Calling .multipart_chunk_size=1 will raise an exception and not set as MP upload, but the .save will still work (less likely to break any implementations with chunks too small).

@brettcave
Copy link
Contributor Author

http://docs.aws.amazon.com/AmazonS3/latest/dev/UploadingObjects.html

Upload objects in a single operation—With a single PUT operation you can upload objects up to 5 GB in size.

Also automatically set multipart for file sizes > 5gb.

@geemus
Copy link
Member

geemus commented Aug 1, 2016

Definitely sounds like a nicer/clearer approach. Thanks!

@geemus geemus merged commit db9dc34 into fog:master Aug 1, 2016
dmbrooking added a commit to datapipe/fog-aws that referenced this pull request Jul 7, 2017
* Fix for empty ETag values

Fog aws fails if API response contains empty ETag tags. It may cause when working with S3 clones (like minio in my case), that not provides ETags in their API answer.

* Allow case-insensitive record comparison

* Parse EbsOptimized parameter in launch configuration description

* updated CHANGELOG.md

* Bump to 0.9.3

* update CHANGELOG.md

* AWS DNS - support newer DNS hosted zone IDs for dualstack ELBs

* Replaces usage of Digest with OpenSSL::Digest for fog#261
The Digest class has some thread safety issues fixed in

aws/aws-sdk-ruby#529
aws/aws-sdk-ruby#525
ruby/ruby@c02fa39

* new India Region to fog-aws

* Added region Mumbai ap-south-1

* update CHANGELOG.md

* Bump to 0.9.4

* udpate CHANGELOG.md

* add default region to use_iam_profile

* make sure to mock tests for region in use_iam_profile

* stub 404 as well

* modify db snapshot attribute api implementation

* copy db snapshot api implementation

* added EOF's

* implemented copy db snapshot parser

* updated parser used in copy_db_snapshot

* fix rescue to use new excon error namespacing

* Added tests for db snapshots

* added mock for modify_db_snapshot_attribute request

* removed fog mock from db snapshot tests

* Expanding IAM support

Adding:
* Create/Delete Policy Versions
* Set Default Policy Version
* Add Update Assume Role Policy

* Add List Attached Role Policies

Add list policy versions

Add more IAM policy tests

* Incorrectly used policy_name rather than arn

Correct request name to delete_policy_version

Correct policy list version parser

Correct update assume role policy

* Copy DB Snapshot mock correct response

* RDS Snapshot format corrected

* Correct DB Snapshot tests

* update CHANGELOG.md

* Bump to 0.10.0

* Change DBSubnetGroup to DBSubnetGroupName model cluster while creation

* test(ci): fix 1.9 builds with json >= 2.0

* update CHANGELOG.md

* test(ci): bind mime-types dep for 1.9 builds

* Removed Extra Space fir proper alignment

* ECS container credentials

* Skip multipart if body size is less than chunk. Fixes fog/fog#3899

* Static method from storage class for fog/fog#3899

* Raise an error (on the setter) if chunk size is too small - fixes fog#283

* (Automatically) set multipart chunk size if the file is too big for a single PUT operation and mp was not set.

* GitHub does no longer provide http:// pages

* update CHANGELOG.md

* Bump to 0.11.0

* update CHANGELOG.md

* added DBSubnetGroupName to DbClusterParser

* Refactore fetching of ip permission

* Implements revoke egress rule

* Implements authorize egress rule

* Tests authorize egress rule

* Tests revoke egress rule

* Ensure compatibility with all CI test cases

* Feedback : enhance test

* Mapped 'DBSubnetGroup' to 'DBSubnetGroupName' parser

* indented

* Added Modify Subnet Group

* change sets

* additional actions

* stack policy

* additional parameters

* Add attribute is default in vpc

* add support endpoint and models/requests for trusted advisor checks

* fix tests

* fix 1.8

* update CHANGELOG.md

* Bump to 0.12.0

* update CHANGELOG.md

* Correct optional parameter naming in documentation for Fog::AWS::Autoscaling::Real#put_scaling_policy

* create, describe, and destroy elastic file systems

* create, describe and destroy mount targets

* fix 1.8

* describe mount target security groups

* modify mount target security groups

* remove debugging code

* fix tests

* fix tests in real mode

* fix some mocking behavior

* fix describe mount targets

* NotFound changes

* there seems to be a weird condition where sometimes the efs api returns one error, and other times a different error

* add wait_for to handle eventual consistency

* fix specs

* accessing compute from efs does weird things

* added target_group_arns to autoscaling group model

* fixed parser to parse target groups from autoscaling group description

* fix S3 #delete_multiple_objects for UTF-8 names

* mime types gem update

* added ohio region with az's

* made east-1 to east-2

* polishing with alphabetical order

* improve tests and initialize ASG with empty targetGroupARNs

* Update db_cluster_parser.rb

* Update modify_db_subnet_group.rb

* Add Fog::AWS::STS.Mock#assume_role method

* Adjust arn and expiration field formats

* Add AssumedRoleId and RequestId response fields

* Add mocked response headers

* data pipeline mocks

* fix 1.8

* Fix the bug to show the warning deprecated usage of the Code Climate Test Reporter when running tests.

* Fix the bug to show the message when running tests that SimpleCov failed to recognize the test framework and/or suite used.

* Modify using Code Climate.

* add creation date to image object

* added a flag for enhanced networking to images

* final changes for enhanced networking

* Modified the parser so that it can parse the Status filed while describing a change set

* Fix the bug that can't create fifo queue.

* Added the Capabilities parameter to be able to change IAM related stuff

* relevant changes to tests for creation Date and enhanced networking support

* Bump to 0.13.0

* update CHANGELOG.md

* Add new t2.xlarge, t2.2xlarge and r4 class instances.
Correctly set r3.large to not have EBS optimization available.
Remove a couple spurious spaces.

* fix host header with another port on s3

* Mark AWS metadata calls as idempotent

Mark calls to metadata services for AWS as idempotent. This means that excon
will retry request up to 3 times in case there is a (temporary) issue on the
metadata service.

* Bump to 1.0.0

* update CHANGELOG.md

* Added support for attaching auto sclaing groups to target groups

This is in reference to fog#328
Unfortunately I will not have time to implement support for managing
the actual ALBs and all that is reated to them.

* Updated ELB Dual Stack hosted zone DNS records

* Update aws.rb

* added az's for canada and london

* exempt mock only tests from live test run

These tests reference objects that do not exist and do not succeed in a
live environment.

* remove assertion during live test run

During a live test run there are quite a few suspended processes.

```
suspend processes
	processes suspended - returns []
		expected => []
		returned => [{"ProcessName"=>"RemoveFromLoadBalancerLowPriority", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}, {"ProcessName"=>"Launch", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}, {"ProcessName"=>"HealthCheck", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}, {"ProcessName"=>"AddToLoadBalancer", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}, {"ProcessName"=>"AlarmNotification", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}, {"ProcessName"=>"ScheduledActions", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}, {"ProcessName"=>"AZRebalance", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}, {"ProcessName"=>"Terminate", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}, {"ProcessName"=>"ReplaceUnhealthy", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}]
```

* fix escaped characters in AutoScaling::ValidationError message

Converts

```
AWS::AutoScaling | tag requests (aws, auto_scaling)
	1 validation error detected: Value '{&quot;Key&quot;=&gt;&quot;Name&quot;, &quot;PropagateAtLaunch&quot;=&gt;true, &quot;ResourceId&quot;=&gt;&quot;fog-test-1481831027&quot;, &quot;ResourceType&quot;=&gt;&quot;auto-scaling-group&quot;, &quot;Value&quot;=&gt;&quot;fog-test-1481831027&quot;}' at 'tags.1.member.key' failed to satisfy constraint: Member must have length less than or equal to 128 (Fog::AWS::AutoScaling::ValidationError)
```

to

```
AWS::AutoScaling | tag requests (aws, auto_scaling)
  1 validation error detected: Value '{"Key"=>"Name", "PropagateAtLaunch"=>true, "ResourceId"=>"fog-test-1481831149", "ResourceType"=>"auto-scaling-group", "Value"=>"fog-test-1481831149"}' at 'tags.1.member.key' failed to satisfy constraint: Member must have length less than or equal to 128 (Fog::AWS::AutoScaling::ValidationError)
```

* fix creating tags with #create_auto_scaling_group

* remove unused variables

* update CHANGELOG.md

* Bump to 1.1.0

* update CHANGELOG.md

* Pin nokogiri gem for Ruby 1.9

Nokogiri >= 1.7 requires Ruby 2.1 or higher.

* Add Gemfile-ruby-2.0 for Ruby 2.0 on Travis CI

Pin Nokogiri to version 1.6.x for Ruby 2.0, since Nokogiri 1.7.x requires Ruby 2.1 or higher.

* Support Partial reservations by parsing an array of recurring charges

This is a semi-breaking change: the recurringCharges field is switching from a
Hash to an Array, however it never before contained any values because it was
unable to parse the AWS XML response. The likelihood of breaking anybody's
"working" code is rather low given that it never worked.

While the AWS API currently supports only a single type of recurring charge,
hourly, they've left the ability to have more/other recurring charge types in
the future, so we should reflect that in the parsed data structure as well.

* Add 'scope' parameter to the reserved instance parser

* Documentation for Reserved Instance recurringCharges and scope

* mocks around iam policies

* instance profile mocks

* instance profile mocks and models

* 1.8 fixes

* more instance profile mocks

* update CHANGELOG.md

* Bump to 1.2.0

* update CHANGELOG.md

* Add missing 'self'

* Ensure get_object and head_object errors are correctly mocked.

* Ensure get_bucket_object_versions errors are correctly mocked.

* Remove unnecessary comment from delete_bucket_policy.

* Removed versioning convenience tag.

* add natGatewayId to describe_route_tables

* subnet fixes

* need to return subnet id in instance mocks
* subnet -> network interfaces relationship
* move addresses to and from vpcs
* bump compute api version to 2016-11-15
* [mock] set dnsname for instance if the vpc is setup for it

* cant pass both public_ip and association_id to the same api call

* route tables need tags too

* route tables need tags too

* mock spot requests

* update CHANGELOG.md

* Bump to 1.2.1

* update CHANGELOG.md

* modify volumes

* model tests as well

* fix 1.8 syntax error

* Add check for self.etag before running gsub

* Add new i3 class instances.

* classic link enhancements

* spec and mock fixes

* authorize vpc security group to rds security group

* fix create_db_subnet_group mock

* subnet group mocks didnt actually delete the subnet group

* update CHANGELOG.md

* Bump to 1.3.0

* update CHANGELOG.md

* Skip region fetch

* ET-2352 Implements exponential backoff for compute requests.

No new test failures.

* ET-2352 Throw RequestLimitExceeded error if out of retries.

* ET-2352 Use retries instead of recursion.

* add p2 instance types

* Handle multipart upload of empty files

* Fixed credential refresh

* Add a top-level require that matches the gem name

Reduce confusion by allowing the gem to be required via a file that
matches the gem name.

References fog/fog#3959

* fix fog#369

* update CHANGELOG.md

* Bump fog-aws to 1.4.0

* Fix AWS credential mocking

The change introduced in fog#252 does not actually prevent HTTP requests
from being made because of a missing early return statement.

* Ruby 1.8 compat

* Add MaxResults filter to describe reserved instances offreings
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