Skip to content

fix aws header list parsing #6435

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 1 commit into from
Jul 12, 2022
Merged

fix aws header list parsing #6435

merged 1 commit into from
Jul 12, 2022

Conversation

thrau
Copy link
Member

@thrau thrau commented Jul 12, 2022

This PR fixes an issue with the ASF parser, where it didn't consider list-encoded types in header locations. See my comment in #6422, fix should be fairly self-explanatory.

I also added an integration test, but the feature is not implemented in moto yet, so cannot fix the issue right away.

List header shapes

We previously made the assumption that shapes with the location "header" are only ever primitive types. However, "ObjectAttributes" in "s3.GetObjectAttributes" is a list shape which has the location "header".

The header arrives as, e.g., x-amz-object-attributes: ObjectSize,StorageClass.

I briefly verified that it's the only one. Here's the list of all shape members with location "header" that are not strings:

appconfig CreateHostedConfigurationVersion LatestVersionNumber Integer integer
ebs CompleteSnapshot ChangedBlocksCount ChangedBlocksCount integer
ebs PutSnapshotBlock DataLength DataLength integer
ebs PutSnapshotBlock Progress Progress integer
s3 CopyObject CopySourceIfModifiedSince CopySourceIfModifiedSince timestamp
s3 CopyObject CopySourceIfUnmodifiedSince CopySourceIfUnmodifiedSince timestamp
s3 CopyObject Expires Expires timestamp
s3 CopyObject BucketKeyEnabled BucketKeyEnabled boolean
s3 CopyObject ObjectLockRetainUntilDate ObjectLockRetainUntilDate timestamp
s3 CreateBucket ObjectLockEnabledForBucket ObjectLockEnabledForBucket boolean
s3 CreateMultipartUpload Expires Expires timestamp
s3 CreateMultipartUpload BucketKeyEnabled BucketKeyEnabled boolean
s3 CreateMultipartUpload ObjectLockRetainUntilDate ObjectLockRetainUntilDate timestamp
s3 DeleteObject BypassGovernanceRetention BypassGovernanceRetention boolean
s3 DeleteObjects BypassGovernanceRetention BypassGovernanceRetention boolean
s3 GetObject IfModifiedSince IfModifiedSince timestamp
s3 GetObject IfUnmodifiedSince IfUnmodifiedSince timestamp
s3 GetObjectAttributes MaxParts MaxParts integer
s3 GetObjectAttributes PartNumberMarker PartNumberMarker integer
s3 GetObjectAttributes ObjectAttributes ObjectAttributesList list
s3 HeadObject IfModifiedSince IfModifiedSince timestamp
s3 HeadObject IfUnmodifiedSince IfUnmodifiedSince timestamp
s3control CreateBucket ObjectLockEnabledForBucket ObjectLockEnabledForBucket boolean
s3control PutBucketPolicy ConfirmRemoveSelfBucketAccess ConfirmRemoveSelfBucketAccess boolean
sagemaker-runtime InvokeEndpointAsync RequestTTLSeconds RequestTTLSecondsHeader integer

@thrau thrau requested a review from alexrashed as a code owner July 12, 2022 11:37
@thrau thrau temporarily deployed to localstack-ext-tests July 12, 2022 11:37 Inactive
alexrashed
alexrashed approved these changes Jul 12, 2022
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.

Great catch! Thanks for the fix, the tests, and validating the integration test against AWS! 🔝

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.008%) to 91.674% when pulling cc5e191 on fix-asf-header-list-parsing into 3a48895 on master.

@github-actions
Copy link

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   1h 6m 26s ⏱️ + 5m 31s
1 127 tests +1  1 087 ✔️ ±0  40 💤 +1  0 ❌ ±0 
1 442 runs  +1  1 373 ✔️ ±0  69 💤 +1  0 ❌ ±0 

Results for commit cc5e191. ± Comparison against base commit 3a48895.

@thrau thrau merged commit 2c85ccd into master Jul 12, 2022
@thrau thrau deleted the fix-asf-header-list-parsing branch July 12, 2022 12:51
@localstack localstack locked and limited conversation to collaborators Jul 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alexrashed alexrashed alexrashed approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thrau @coveralls @alexrashed