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: only enable aws-chunked content encoding for S3 #1224

Merged
merged 8 commits into from
Feb 19, 2024

Conversation

lauzadis
Copy link
Member

@lauzadis lauzadis commented Feb 19, 2024

This PR disables aws-chunked content encoding for all services by default, except S3. It also adds support for a new EnableAwsChunked signing attribute which controls enabling aws-chunked content encoding.

Issue #

closes #1217

Description of changes

This change is required because S3 is the only service which explicitly supports aws-chunked content encoding.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@lauzadis lauzadis requested a review from a team as a code owner February 19, 2024 16:06
@@ -20,7 +21,7 @@ import software.amazon.smithy.model.shapes.ServiceShape
* Set default signing attributes in the execution context (which ultimately becomes the signing context) for S3.
*/
class S3SigningConfig : KotlinIntegration {
// auth schemes are de-duped by taking the last one registered
// integrations are de-duped by taking the last one registered
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: is this comment now correct? I wasn't sure why auth schemes were relevant here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think this comment is correct for integrations. All of the registered integrations are "taken" and there is no possibility of duplication.

The comment was correct for auth schemes and I believe it was referring to the order value defined below, indicating that whatever auth scheme this integration registered should be the one that's taken because it'll be the last one (unless a user overrides it even later in client config). That said, I don't see this integration registering an auth scheme so I'm not sure how it's relevant.

Copy link

A new generated diff is ready to view.

Copy link

A new generated diff is ready to view.

@@ -20,7 +21,7 @@ import software.amazon.smithy.model.shapes.ServiceShape
* Set default signing attributes in the execution context (which ultimately becomes the signing context) for S3.
*/
class S3SigningConfig : KotlinIntegration {
// auth schemes are de-duped by taking the last one registered
// integrations are de-duped by taking the last one registered
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think this comment is correct for integrations. All of the registered integrations are "taken" and there is no possibility of duplication.

The comment was correct for auth schemes and I believe it was referring to the order value defined below, indicating that whatever auth scheme this integration registered should be the one that's taken because it'll be the last one (unless a user overrides it even later in client config). That said, I don't see this integration registering an auth scheme so I'm not sure how it's relevant.

Copy link

A new generated diff is ready to view.

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

A new generated diff is ready to view.

@lauzadis lauzadis merged commit 77f3342 into main Feb 19, 2024
16 of 17 checks passed
@lauzadis lauzadis deleted the fix-aws-chunked-signing branch February 19, 2024 20:01
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.

CodeArtifact .publishPackageVersion fails for large generic artifact with 'InvalidSignatureException'
3 participants