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

Fix: AWS Copy SSE-C Options + Send all options to copy instead of just params #1779

Merged
merged 2 commits into from
Apr 7, 2024

Conversation

amenophis
Copy link
Contributor

Hi @frankdejonge,
This is the right MR with all changes.

I tested in with the official S3 SDK, not with AsyncAWS.
Not sure we have to do changes for params with AsyncAWS.

Thanks in advance for review 🙏

@frankdejonge frankdejonge merged commit 442b817 into thephpleague:3.x Apr 7, 2024
3 of 6 checks passed
@frankdejonge
Copy link
Member

The change to send all options broke the test suite, it's reverted.

@amenophis
Copy link
Contributor Author

@frankdejonge Forwarding params only make the copy didn't work from S3 to S3 with SSE-C enabled (the head issued by AWS SDK ObjectCopier ends with a 400 from AWS API)
Could you show me what tests are failling with the change ?

Thanks 🙏

@frankdejonge
Copy link
Member

@amenophis I found the error, it had to do with how MetadataDirective was handled (which is required for specifying new metadata during copy. I'm not sure how the options you corrected are forwarded, as metadata of otherwise, but you might need to use the config 'MetadataDirective' => 'REPLACE' for what you're trying to achieve. Hope this helps, 3.27.0 is just released.

@amenophis
Copy link
Contributor Author

I will try this tomorrow.
Thank you for your research!

@amenophis
Copy link
Contributor Author

Hi @frankdejonge
I checked the fix you have merged, it seems to fix our issue, but there is something that i didn't understand about the MetadataDirective.
I've added it in the adapter configuration with REPLACE value, and if i watch at the options sent to the client->copy() method, The MetadataDirective option in defined twice, at root with COPY value and in 'params' with REPLACE value).

I don't think this is the expected behaviour.

WDYT ?

@amenophis
Copy link
Contributor Author

Hi @frankdejonge
WDYT about my last message ?

Thanks 🙏

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